[clang-tools-extra] r326313 - [clangd] Prefer the definition of a TagDecl (e.g. class) as CanonicalDeclaration.
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 28 01:33:16 PST 2018
Author: ioeric
Date: Wed Feb 28 01:33:15 2018
New Revision: 326313
URL: http://llvm.org/viewvc/llvm-project?rev=326313&view=rev
Log:
[clangd] Prefer the definition of a TagDecl (e.g. class) as CanonicalDeclaration.
Summary:
Currently, we pick the first declaration of a symbol in a TU, which is considered
canonical in the clangIndex, as the canonical declaration in clangd. This causes
forward declarations that might appear in a random header to be used as a
canonical declaration, which is not desirable for features like go-to-declaration
or include insertion.
For example, for class X, we would consider the forward declaration in fwd.h to
be the canonical declaration, while the preferred canonical declaration should
be the actual definition in x.h.
```
// fwd.h
class X; // forward decl
// x.h
class X {};
```
This patch fixes the issue by making symbol collector favor the actual definition of
a TagDecl (i.e. class/struct/enum/union) found in a header file over the first seen
declarations in a TU. Other symbol types like functions are not handled because
using the first seen declarations as canonical declarations is usually a good
heuristic for them.
Reviewers: sammccall
Subscribers: klimek, ilya-biryukov, jkorous-apple, cfe-commits
Differential Revision: https://reviews.llvm.org/D43823
Modified:
clang-tools-extra/trunk/clangd/index/FileIndex.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.h
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
Modified: clang-tools-extra/trunk/clangd/index/FileIndex.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.cpp?rev=326313&r1=326312&r2=326313&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/FileIndex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp Wed Feb 28 01:33:15 2018
@@ -20,11 +20,6 @@ std::unique_ptr<SymbolSlab> indexAST(AST
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
Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=326313&r1=326312&r2=326313&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Wed Feb 28 01:33:15 2018
@@ -115,9 +115,7 @@ bool shouldFilterDecl(const NamedDecl *N
// * 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
// 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, So
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::handleDeclOccurenc
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::addDeclar
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;
Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.h?rev=326313&r1=326312&r2=326313&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.h (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.h Wed Feb 28 01:33:15 2018
@@ -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
Modified: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp?rev=326313&r1=326312&r2=326313&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp Wed Feb 28 01:33:15 2018
@@ -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, CollectSymbo
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, CollectSymbo
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, CollectSymbo
}
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, SymbolRelati
#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, CustomURISch
#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, InvalidURISc
}
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, FallbackToFi
}
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, IgnoreNamele
}
TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) {
- CollectorOpts.IndexMainFiles = false;
Annotations Header(R"(
#define FF(name) \
@@ -342,33 +330,7 @@ TEST_F(SymbolCollectorTest, SymbolFormed
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, SymbolFormed
}
TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) {
- CollectorOpts.IndexMainFiles = false;
const std::string Header = R"(
class Foo {};
void f1();
@@ -402,25 +363,6 @@ TEST_F(SymbolCollectorTest, IgnoreSymbol
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
More information about the cfe-commits
mailing list