diff options
4 files changed, 77 insertions, 86 deletions
diff --git a/clang-tools-extra/clangd/index/FileIndex.cpp b/clang-tools-extra/clangd/index/FileIndex.cpp index 8a8d179557a..4f1f10a44c0 100644 --- a/clang-tools-extra/clangd/index/FileIndex.cpp +++ b/clang-tools-extra/clangd/index/FileIndex.cpp @@ -20,11 +20,6 @@ std::unique_ptr<SymbolSlab> indexAST(ASTContext &Ctx, std::shared_ptr<Preprocessor> PP, llvm::ArrayRef<const Decl *> Decls) { SymbolCollector::Options CollectorOpts; - // Although we do not index symbols in main files (e.g. cpp file), information - // in main files like definition locations of class declarations will still be - // collected; thus, the index works for go-to-definition. - // FIXME(ioeric): get rid of `IndexMainFiles` as this is always set to false. - CollectorOpts.IndexMainFiles = false; // FIXME(ioeric): we might also want to collect include headers. We would need // to make sure all includes are canonicalized (with CanonicalIncludes), which // is not trivial given the current way of collecting symbols: we only have diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp index f8fb2c29570..c75184010ee 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -115,9 +115,7 @@ bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx, // * enum constants in unscoped enum decl (e.g. "red" in "enum {red};") auto InTopLevelScope = hasDeclContext( anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl())); - if (match(decl(allOf(Opts.IndexMainFiles - ? decl() - : decl(unless(isExpansionInMainFile())), + if (match(decl(allOf(unless(isExpansionInMainFile()), anyOf(InTopLevelScope, hasDeclContext(enumDecl(InTopLevelScope, unless(isScoped())))))), @@ -177,11 +175,9 @@ getIncludeHeader(const SourceManager &SM, SourceLocation Loc, // For symbols defined inside macros: // * use expansion location, if the symbol is formed via macro concatenation. // * use spelling location, otherwise. -llvm::Optional<SymbolLocation> -getSymbolLocation(const NamedDecl &D, SourceManager &SM, - const SymbolCollector::Options &Opts, - const clang::LangOptions& LangOpts, - std::string &FileURIStorage) { +llvm::Optional<SymbolLocation> getSymbolLocation( + const NamedDecl &D, SourceManager &SM, const SymbolCollector::Options &Opts, + const clang::LangOptions &LangOpts, std::string &FileURIStorage) { SourceLocation SpellingLoc = SM.getSpellingLoc(D.getLocation()); if (D.getLocation().isMacroID()) { std::string PrintLoc = SpellingLoc.printToString(SM); @@ -209,6 +205,19 @@ getSymbolLocation(const NamedDecl &D, SourceManager &SM, return std::move(Result); } +// Checks whether \p ND is a definition of a TagDecl (class/struct/enum/union) +// in a header file, in which case clangd would prefer to use ND as a canonical +// declaration. +// FIXME: handle symbol types that are not TagDecl (e.g. functions), if using +// the the first seen declaration as canonical declaration is not a good enough +// heuristic. +bool isPreferredDeclaration(const NamedDecl &ND, index::SymbolRoleSet Roles) { + using namespace clang::ast_matchers; + return (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) && + llvm::isa<TagDecl>(&ND) && + match(decl(isExpansionInMainFile()), ND, ND.getASTContext()).empty(); +} + } // namespace SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {} @@ -241,12 +250,20 @@ bool SymbolCollector::handleDeclOccurence( if (index::generateUSRForDecl(ND, USR)) return true; + const NamedDecl &OriginalDecl = *cast<NamedDecl>(ASTNode.OrigD); auto ID = SymbolID(USR); - const Symbol* BasicSymbol = Symbols.find(ID); + const Symbol *BasicSymbol = Symbols.find(ID); if (!BasicSymbol) // Regardless of role, ND is the canonical declaration. BasicSymbol = addDeclaration(*ND, std::move(ID)); + else if (isPreferredDeclaration(OriginalDecl, Roles)) + // If OriginalDecl is preferred, replace the existing canonical + // declaration (e.g. a class forward declaration). There should be at most + // one duplicate as we expect to see only one preferred declaration per + // TU, because in practice they are definitions. + BasicSymbol = addDeclaration(OriginalDecl, std::move(ID)); + if (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) - addDefinition(*cast<NamedDecl>(ASTNode.OrigD), *BasicSymbol); + addDefinition(OriginalDecl, *BasicSymbol); } return true; } @@ -271,8 +288,6 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, std::tie(S.Scope, S.Name) = splitQualifiedName(QName); S.SymInfo = index::getSymbolInfo(&ND); std::string FileURI; - // FIXME: we may want a different "canonical" heuristic than clang chooses. - // Clang seems to choose the first, which may not have the most information. if (auto DeclLoc = getSymbolLocation(ND, SM, Opts, ASTCtx->getLangOpts(), FileURI)) S.CanonicalDeclaration = *DeclLoc; diff --git a/clang-tools-extra/clangd/index/SymbolCollector.h b/clang-tools-extra/clangd/index/SymbolCollector.h index a39c70779bd..c18f74a8362 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.h +++ b/clang-tools-extra/clangd/index/SymbolCollector.h @@ -20,7 +20,11 @@ namespace clangd { /// \brief Collect top-level symbols from an AST. These are symbols defined /// immediately inside a namespace or a translation unit scope. For example, -/// symbols in classes or functions are not collected. +/// symbols in classes or functions are not collected. Note that this only +/// collects symbols that declared in at least one file that is not a main +/// file (i.e. the source file corresponding to a TU). These are symbols that +/// can be imported by other files by including the file where symbols are +/// declared. /// /// Clients (e.g. clangd) can use SymbolCollector together with /// index::indexTopLevelDecls to retrieve all symbols when the source file is @@ -28,9 +32,6 @@ namespace clangd { class SymbolCollector : public index::IndexDataConsumer { public: struct Options { - /// Whether to collect symbols in main files (e.g. the source file - /// corresponding to a TU). - bool IndexMainFiles = false; /// When symbol paths cannot be resolved to absolute paths (e.g. files in /// VFS that does not have absolute path), combine the fallback directory /// with symbols' paths to get absolute paths. This must be an absolute diff --git a/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp b/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp index 27cf6308100..00cab62aba1 100644 --- a/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp +++ b/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp @@ -47,6 +47,7 @@ MATCHER_P(Snippet, S, "") { } MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; } MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; } +MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; } MATCHER_P(IncludeHeader, P, "") { return arg.Detail && arg.Detail->IncludeHeader == P; } @@ -152,7 +153,6 @@ protected: }; TEST_F(SymbolCollectorTest, CollectSymbols) { - CollectorOpts.IndexMainFiles = true; const std::string Header = R"( class Foo { void f(); @@ -161,8 +161,7 @@ TEST_F(SymbolCollectorTest, CollectSymbols) { inline void f2() {} static const int KInt = 2; const char* kStr = "123"; - )"; - const std::string Main = R"( + namespace { void ff() {} // ignore } @@ -190,7 +189,7 @@ TEST_F(SymbolCollectorTest, CollectSymbols) { using bar::v2; } // namespace foo )"; - runSymbolCollector(Header, Main); + runSymbolCollector(Header, /*Main=*/""); EXPECT_THAT(Symbols, UnorderedElementsAreArray( {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"), @@ -200,7 +199,6 @@ TEST_F(SymbolCollectorTest, CollectSymbols) { } TEST_F(SymbolCollectorTest, Locations) { - CollectorOpts.IndexMainFiles = true; Annotations Header(R"cpp( // Declared in header, defined in main. extern int $xdecl[[X]]; @@ -216,7 +214,7 @@ TEST_F(SymbolCollectorTest, Locations) { void $printdef[[print]]() {} // Declared/defined in main only. - int $y[[Y]]; + int Y; )cpp"); runSymbolCollector(Header.code(), Main.code()); EXPECT_THAT( @@ -228,20 +226,16 @@ TEST_F(SymbolCollectorTest, Locations) { DefRange(Main.offsetRange("clsdef"))), AllOf(QName("print"), DeclRange(Header.offsetRange("printdecl")), DefRange(Main.offsetRange("printdef"))), - AllOf(QName("Z"), DeclRange(Header.offsetRange("zdecl"))), - AllOf(QName("Y"), DeclRange(Main.offsetRange("y")), - DefRange(Main.offsetRange("y"))))); + AllOf(QName("Z"), DeclRange(Header.offsetRange("zdecl"))))); } TEST_F(SymbolCollectorTest, SymbolRelativeNoFallback) { - CollectorOpts.IndexMainFiles = false; runSymbolCollector("class Foo {};", /*Main=*/""); - EXPECT_THAT(Symbols, - UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI)))); + EXPECT_THAT(Symbols, UnorderedElementsAre( + AllOf(QName("Foo"), DeclURI(TestHeaderURI)))); } TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) { - CollectorOpts.IndexMainFiles = false; TestHeaderName = "x.h"; TestFileName = "x.cpp"; TestHeaderURI = URI::createFile(testPath(TestHeaderName)).toString(); @@ -253,7 +247,6 @@ TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) { #ifndef LLVM_ON_WIN32 TEST_F(SymbolCollectorTest, CustomURIScheme) { - CollectorOpts.IndexMainFiles = false; // Use test URI scheme from URITests.cpp CollectorOpts.URISchemes.insert(CollectorOpts.URISchemes.begin(), "unittest"); TestHeaderName = testPath("test-root/x.h"); @@ -265,7 +258,6 @@ TEST_F(SymbolCollectorTest, CustomURIScheme) { #endif TEST_F(SymbolCollectorTest, InvalidURIScheme) { - CollectorOpts.IndexMainFiles = false; // Use test URI scheme from URITests.cpp CollectorOpts.URISchemes = {"invalid"}; runSymbolCollector("class Foo {};", /*Main=*/""); @@ -273,7 +265,6 @@ TEST_F(SymbolCollectorTest, InvalidURIScheme) { } TEST_F(SymbolCollectorTest, FallbackToFileURI) { - CollectorOpts.IndexMainFiles = false; // Use test URI scheme from URITests.cpp CollectorOpts.URISchemes = {"invalid", "file"}; runSymbolCollector("class Foo {};", /*Main=*/""); @@ -282,7 +273,6 @@ TEST_F(SymbolCollectorTest, FallbackToFileURI) { } TEST_F(SymbolCollectorTest, IncludeEnums) { - CollectorOpts.IndexMainFiles = false; const std::string Header = R"( enum { Red @@ -306,7 +296,6 @@ TEST_F(SymbolCollectorTest, IncludeEnums) { } TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) { - CollectorOpts.IndexMainFiles = false; const std::string Header = R"( struct { int a; @@ -318,7 +307,6 @@ TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) { } TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) { - CollectorOpts.IndexMainFiles = false; Annotations Header(R"( #define FF(name) \ @@ -342,33 +330,7 @@ TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) { DeclURI(TestHeaderURI)))); } -TEST_F(SymbolCollectorTest, SymbolFormedFromMacroInMainFile) { - CollectorOpts.IndexMainFiles = true; - - Annotations Main(R"( - #define FF(name) \ - class name##_Test {}; - - $expansion[[FF]](abc); - - #define FF2() \ - class $spelling[[Test]] {}; - - FF2(); - )"); - runSymbolCollector(/*Header=*/"", Main.code()); - EXPECT_THAT( - Symbols, - UnorderedElementsAre( - AllOf(QName("abc_Test"), DeclRange(Main.offsetRange("expansion")), - DeclURI(TestFileURI)), - AllOf(QName("Test"), DeclRange(Main.offsetRange("spelling")), - DeclURI(TestFileURI)))); -} - TEST_F(SymbolCollectorTest, SymbolFormedByCLI) { - CollectorOpts.IndexMainFiles = false; - Annotations Header(R"( #ifdef NAME class $expansion[[NAME]] {}; @@ -384,7 +346,6 @@ TEST_F(SymbolCollectorTest, SymbolFormedByCLI) { } TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) { - CollectorOpts.IndexMainFiles = false; const std::string Header = R"( class Foo {}; void f1(); @@ -402,25 +363,6 @@ TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) { UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2"))); } -TEST_F(SymbolCollectorTest, IncludeSymbolsInMainFile) { - CollectorOpts.IndexMainFiles = true; - const std::string Header = R"( - class Foo {}; - void f1(); - inline void f2() {} - )"; - const std::string Main = R"( - namespace { - void ff() {} // ignore - } - void main_f() {} - void f1() {} - )"; - runSymbolCollector(Header, Main); - EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("f1"), - QName("f2"), QName("main_f"))); -} - TEST_F(SymbolCollectorTest, IgnoreClassMembers) { const std::string Header = R"( class Foo { @@ -620,6 +562,44 @@ TEST_F(SymbolCollectorTest, IWYUPragma) { IncludeHeader("\"the/good/header.h\"")))); } +TEST_F(SymbolCollectorTest, AvoidUsingFwdDeclsAsCanonicalDecls) { + CollectorOpts.CollectIncludePath = true; + Annotations Header(R"( + // Forward declarations of TagDecls. + class C; + struct S; + union U; + + // Canonical declarations. + class $cdecl[[C]] {}; + struct $sdecl[[S]] {}; + union $udecl[[U]] {int x; bool y;}; + )"); + runSymbolCollector(Header.code(), /*Main=*/""); + EXPECT_THAT(Symbols, + UnorderedElementsAre( + AllOf(QName("C"), DeclURI(TestHeaderURI), + DeclRange(Header.offsetRange("cdecl")), + IncludeHeader(TestHeaderURI), DefURI(TestHeaderURI), + DefRange(Header.offsetRange("cdecl"))), + AllOf(QName("S"), DeclURI(TestHeaderURI), + DeclRange(Header.offsetRange("sdecl")), + IncludeHeader(TestHeaderURI), DefURI(TestHeaderURI), + DefRange(Header.offsetRange("sdecl"))), + AllOf(QName("U"), DeclURI(TestHeaderURI), + DeclRange(Header.offsetRange("udecl")), + IncludeHeader(TestHeaderURI), DefURI(TestHeaderURI), + DefRange(Header.offsetRange("udecl"))))); +} + +TEST_F(SymbolCollectorTest, ClassForwardDeclarationIsCanonical) { + CollectorOpts.CollectIncludePath = true; + runSymbolCollector(/*Header=*/"class X;", /*Main=*/"class X {};"); + EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf( + QName("X"), DeclURI(TestHeaderURI), + IncludeHeader(TestHeaderURI), DefURI(TestFileURI)))); +} + } // namespace } // namespace clangd } // namespace clang |
