kbobyrev updated this revision to Diff 365094. kbobyrev marked 15 inline comments as done. kbobyrev added a comment.
Thank you for the great suggestions! Indeed, we already have the confusion regarding IWYU having the CLI flag and users are thinking we provide some sort of integration/implementation. Having even more confusion (implementing similar) functionality for our needs is not what we want (also, apologies for confusion,"as a library" was more about the patch in this context - meaning not a "usable" feature yet but a smaller scope where we just start the implementation as the library that can only be used in tests for now). Hence, renamed the feature to "Include Sanity" (close to Include Hygiene but sounds better IMO, please let me know if you think otherwise). I have addressed the review comments and will base my next patch on this (this patch is probably mostly the blocker for the upcoming ones). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105426/new/ https://reviews.llvm.org/D105426 Files: clang-tools-extra/clangd/CMakeLists.txt clang-tools-extra/clangd/IncludeSanity.cpp clang-tools-extra/clangd/IncludeSanity.h clang-tools-extra/clangd/unittests/CMakeLists.txt clang-tools-extra/clangd/unittests/IncludeSanityTests.cpp
Index: clang-tools-extra/clangd/unittests/IncludeSanityTests.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clangd/unittests/IncludeSanityTests.cpp @@ -0,0 +1,136 @@ +//===--- IWYUTests.cpp ------------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "Annotations.h" +#include "IncludeSanity.h" +#include "TestTU.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace clang { +namespace clangd { +namespace { + +TEST(IWYU, ReferencedLocations) { + struct TestCase { + std::string HeaderCode; + std::string MainCode; + }; + TestCase Cases[] = { + // DeclRefExpr + { + "int ^x();", + "int y = x();", + }, + // RecordDecl + { + "class ^X;", + "X *y;", + }, + // TypedefType and UsingDecls + { + "using ^Integer = int;", + "Integer x;", + }, + { + "namespace ns { struct ^X; struct ^X {}; }", + "using ns::X;", + }, + { + "namespace ns { struct X; struct X {}; }", + "using namespace ns;", + }, + { + "struct ^A {}; using B = A; using ^C = B;", + "C a;", + }, + { + "typedef bool ^Y; template <typename T> struct ^X {};", + "X<Y> x;", + }, + { + "struct Foo; struct ^Foo{}; typedef Foo ^Bar;", + "Bar b;", + }, + // MemberExpr + { + "struct ^X{int ^a;}; X ^foo();", + "int y = foo().a;", + }, + // Expr (type is traversed) + { + "class ^X{}; X ^foo();", + "auto bar() { return foo(); }", + }, + // Redecls + { + "class ^X; class ^X{}; class ^X;", + "X *y;", + }, + // Constructor + { + "struct ^X { ^X(int) {} int ^foo(); };", + "auto x = X(42); auto y = x.foo();", + }, + // Static function + { + "struct ^X { static bool ^foo(); }; bool X::^foo() {}", + "auto b = X::foo();", + }, + // TemplateRecordDecl + { + "template <typename> class ^X;", + "X<int> *y;", + }, + // Type name not spelled out in code + { + "class ^X{}; X ^getX();", + "auto x = getX();", + }, + // Enums + { + "enum ^Color { ^Red = 42, Green = 9000};", + "int MyColor = Red;", + }, + { + "struct ^X { enum ^Language { ^CXX = 42, Python = 9000}; };", + "int Lang = X::CXX;", + }, + { + // When a type is resolved via a using declaration, the + // UsingShadowDecl is not referenced in the AST. + // Compare to TypedefType, or DeclRefExpr::getFoundDecl(). + // ^ + "namespace ns { class ^X; }; using ns::X;", + "X *y;", + }}; + for (const TestCase &T : Cases) { + TestTU TU; + TU.Code = T.MainCode; + Annotations Header(T.HeaderCode); + TU.HeaderCode = Header.code().str(); + auto AST = TU.build(); + + std::vector<Position> Points; + for (const auto &Loc : findReferencedLocations(AST)) { + if (AST.getSourceManager().getBufferName(Loc).endswith( + TU.HeaderFilename)) { + Points.push_back(offsetToPosition( + TU.HeaderCode, AST.getSourceManager().getFileOffset(Loc))); + } + } + llvm::sort(Points); + + EXPECT_EQ(Points, Header.points()) << T.HeaderCode << "\n---\n" + << T.MainCode; + } +} + +} // namespace +} // namespace clangd +} // namespace clang Index: clang-tools-extra/clangd/unittests/CMakeLists.txt =================================================================== --- clang-tools-extra/clangd/unittests/CMakeLists.txt +++ clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -58,6 +58,7 @@ HeadersTests.cpp HeaderSourceSwitchTests.cpp HoverTests.cpp + IncludeSanityTests.cpp IndexActionTests.cpp IndexTests.cpp InlayHintTests.cpp Index: clang-tools-extra/clangd/IncludeSanity.h =================================================================== --- /dev/null +++ clang-tools-extra/clangd/IncludeSanity.h @@ -0,0 +1,58 @@ +//===--- IncludeSanity.h - Unused/Missing Headers Analysis ------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// Include Sanity is clangd functionality for providing diagnostics for misuse +/// of transitive headers and unused includes. It is inspired by +/// Include-What-You-Use tool (https://include-what-you-use.org/). Our goal is +/// to provide useful warnings in most popular scenarios but not 1:1 exact +/// feature compatibility. +/// +/// FIXME(kirillbobyrev): Add support for IWYU pragmas. +/// FIXME(kirillbobyrev): Add support for standard library headers. +/// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_SANITY_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_SANITY_H + +#include "Headers.h" +#include "ParsedAST.h" +#include "clang/Basic/SourceLocation.h" +#include "llvm/ADT/DenseSet.h" + +namespace clang { +namespace clangd { + +using ReferencedLocations = llvm::DenseSet<SourceLocation>; +/// Finds locations of all symbols used in the main file. +/// +/// Uses RecursiveASTVisitor to go through main file AST and computes all the +/// locations used symbols are coming from. Returned locations may be macro +/// expansions, and are not resolved to their spelling/expansion location. These +/// locations are later used to determine which headers should be marked as +/// "used" and "directly used". +/// +/// We use this to compute unused headers, so we: +/// +/// - cover the whole file in a single traversal for efficiency +/// - don't attempt to describe where symbols were referenced from in +/// ambiguous cases (e.g. implicitly used symbols, multiple declarations) +/// - err on the side of reporting all possible locations +ReferencedLocations findReferencedLocations(ParsedAST &AST); + +/// Retrieves IDs of all files containing SourceLocations from \p Locs. Those +/// locations could be within macro expansions and are not resolved to their +/// spelling locations. +llvm::DenseSet<FileID> findReferencedFiles(const ReferencedLocations &Locs, + const SourceManager &SM); + +} // namespace clangd +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_SANITY_H Index: clang-tools-extra/clangd/IncludeSanity.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clangd/IncludeSanity.cpp @@ -0,0 +1,158 @@ +//===--- IncludeSanity.cpp - Unused/Missing Headers Analysis ----*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "IncludeSanity.h" +#include "support/Logger.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/Basic/SourceLocation.h" + +namespace clang { +namespace clangd { +namespace { + +/// Crawler traverses the AST and feeds in the locations of (sometimes +/// implicitly) used symbols into \p Result. +class ReferencedLocationCrawler + : public RecursiveASTVisitor<ReferencedLocationCrawler> { +public: + ReferencedLocationCrawler(ReferencedLocations &Result) : Result(Result) {} + + bool VisitDeclRefExpr(DeclRefExpr *DRE) { + add(DRE->getDecl()); + add(DRE->getFoundDecl()); + return true; + } + + bool VisitMemberExpr(MemberExpr *ME) { + add(ME->getMemberDecl()); + add(ME->getFoundDecl().getDecl()); + return true; + } + + bool VisitTagType(TagType *TT) { + add(TT->getDecl()); + return true; + } + + bool VisitCXXConstructExpr(CXXConstructExpr *CCE) { + add(CCE->getConstructor()); + return true; + } + + bool VisitTemplateSpecializationType(TemplateSpecializationType *TST) { + if (isNew(TST)) { + add(TST->getTemplateName().getAsTemplateDecl()); // Primary template. + add(TST->getAsCXXRecordDecl()); // Specialization + } + return true; + } + + bool VisitTypedefType(TypedefType *TT) { + add(TT->getDecl()); + return true; + } + + // Consider types of any subexpression used, even if the type is not named. + // This is helpful in getFoo().bar(), where Foo must be complete. + // FIXME(kirillbobyrev): Should we tweak this? It may not be desirable to + // consider types "used" when they are not directly spelled in code. + bool VisitExpr(Expr *E) { + TraverseType(E->getType()); + return true; + } + + bool TraverseType(QualType T) { + if (isNew(T.getTypePtrOrNull())) { // don't care about quals + Base::TraverseType(T); + } + return true; + } + + bool VisitUsingDecl(UsingDecl *D) { + for (const auto *Shadow : D->shadows()) { + add(Shadow->getTargetDecl()); + } + return true; + } + +private: + using Base = RecursiveASTVisitor<ReferencedLocationCrawler>; + + void add(const Decl *D) { + if (!D || !isNew(D->getCanonicalDecl())) { + return; + } + for (const Decl *Redecl : D->redecls()) { + Result.insert(Redecl->getLocation()); + } + } + + bool isNew(const void *P) { return P && Visited.insert(P).second; } + + ReferencedLocations &Result; + llvm::DenseSet<const void *> Visited; +}; + +// Given a set of referenced FileIDs, determines all the potentially-referenced +// files and macros by traversing expansion/spelling locations of macro IDs. +// This is used to map the referenced SourceLocations onto real files. +struct ReferencedFiles { + ReferencedFiles(const SourceManager &SM) : SM(SM) {} + llvm::DenseSet<FileID> Files; + llvm::DenseSet<FileID> Macros; + const SourceManager &SM; + + void add(SourceLocation Loc) { add(SM.getFileID(Loc), Loc); } + + void add(FileID FID, SourceLocation Loc) { + if (FID.isInvalid()) + return; + assert(SM.isInFileID(Loc, FID)); + if (Loc.isFileID()) { + Files.insert(FID); + return; + } + // Don't process the same macro FID twice. + if (!Macros.insert(FID).second) + return; + const auto &Exp = SM.getSLocEntry(FID).getExpansion(); + add(Exp.getSpellingLoc()); + add(Exp.getExpansionLocStart()); + add(Exp.getExpansionLocEnd()); + } +}; + +} // namespace + +ReferencedLocations findReferencedLocations(ParsedAST &AST) { + ReferencedLocations Result; + ReferencedLocationCrawler Crawler(Result); + Crawler.TraverseAST(AST.getASTContext()); + // FIXME(kirillbobyrev): Handle macros. + return Result; +} + +llvm::DenseSet<FileID> findReferencedFiles(const ReferencedLocations &Locs, + const SourceManager &SM) { + std::vector<SourceLocation> Sorted{Locs.begin(), Locs.end()}; + llvm::sort(Sorted); // This groups by FileID! + ReferencedFiles Result(SM); + for (auto It = Sorted.begin(); It < Sorted.end();) { + auto FID = SM.getFileID(*It); + Result.add(FID, *It); + // Cheaply skip over all the other locations from the same FileID. + // This avoids lots of redundant Loc->File lookups for the same file. + do { + ++It; + } while (It != Sorted.end() && SM.isInFileID(*It, FID)); + } + return std::move(Result.Files); +} + +} // namespace clangd +} // namespace clang Index: clang-tools-extra/clangd/CMakeLists.txt =================================================================== --- clang-tools-extra/clangd/CMakeLists.txt +++ clang-tools-extra/clangd/CMakeLists.txt @@ -84,6 +84,7 @@ HeuristicResolver.cpp Hover.cpp IncludeFixer.cpp + IncludeSanity.cpp InlayHints.cpp JSONTransport.cpp PathMapping.cpp
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
