From fbeff2ec2bc6e44b92931207b0063f83ff7a3b3a Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena Date: Mon, 1 Feb 2021 21:17:53 +0100 Subject: [PATCH] [clangd] Report only decl of overridding method in xref. See: https://github.com/clangd/clangd/issues/668 ``` struct A { virtual void foo() = 0; }; struct B : A { void foo() override; }; ``` Find refs on `A::foo()` will show: - decls of `A::foo()` - decls of `B::foo()` - refs to `A::foo()` - no refs to `B::foo()`. Differential Revision: https://reviews.llvm.org/D95812 --- clang-tools-extra/clangd/XRefs.cpp | 62 ++++++++++++------- clang-tools-extra/clangd/XRefs.h | 2 + .../clangd/unittests/XRefsTests.cpp | 14 ++++- 3 files changed, 52 insertions(+), 26 deletions(-) diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index 55b55da80fda..b6a0be4a0a0a 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -1300,7 +1300,7 @@ ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit, return {}; } - llvm::DenseSet IDs, Overrides; + llvm::DenseSet IDs; const auto *IdentifierAtCursor = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens()); @@ -1339,25 +1339,19 @@ ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit, if (auto ID = getSymbolID(D)) Targets.insert(ID); + RelationsRequest OverriddenBy; if (Index) { - RelationsRequest FindOverrides; - FindOverrides.Predicate = RelationKind::OverriddenBy; + OverriddenBy.Predicate = RelationKind::OverriddenBy; for (const NamedDecl *ND : Decls) { - // Special case: virtual void meth^od() = 0 includes refs of overrides. + // Special case: Inlcude declaration of overridding methods. if (const auto *CMD = llvm::dyn_cast(ND)) { - if (CMD->isPure()) + if (CMD->isVirtual()) if (IdentifierAtCursor && SM.getSpellingLoc(CMD->getLocation()) == IdentifierAtCursor->location()) if (auto ID = getSymbolID(CMD)) - FindOverrides.Subjects.insert(ID); + OverriddenBy.Subjects.insert(ID); } } - if (!FindOverrides.Subjects.empty()) - Index->relations(FindOverrides, - [&](const SymbolID &Subject, const Symbol &Object) { - Overrides.insert(Object.ID); - }); - Targets.insert(Overrides.begin(), Overrides.end()); } // We traverse the AST to find references in the main file. @@ -1376,17 +1370,37 @@ ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit, ReferencesResult::Reference Result; Result.Loc.range = Ref.range(SM); Result.Loc.uri = URIMainFile; - // Overrides are always considered references, not defs/decls. - if (!Overrides.contains(Ref.Target)) { - if (Ref.Role & static_cast(index::SymbolRole::Declaration)) - Result.Attributes |= ReferencesResult::Declaration; - // clang-index doesn't report definitions as declarations, but they are. - if (Ref.Role & static_cast(index::SymbolRole::Definition)) - Result.Attributes |= - ReferencesResult::Definition | ReferencesResult::Declaration; - } + if (Ref.Role & static_cast(index::SymbolRole::Declaration)) + Result.Attributes |= ReferencesResult::Declaration; + // clang-index doesn't report definitions as declarations, but they are. + if (Ref.Role & static_cast(index::SymbolRole::Definition)) + Result.Attributes |= + ReferencesResult::Definition | ReferencesResult::Declaration; Results.References.push_back(std::move(Result)); } + // Add decl/def of overridding methods. + if (Index && Results.References.size() <= Limit && + !OverriddenBy.Subjects.empty()) + Index->relations( + OverriddenBy, [&](const SymbolID &Subject, const Symbol &Object) { + if (auto LSPLoc = + toLSPLocation(Object.CanonicalDeclaration, *MainFilePath)) { + ReferencesResult::Reference Result; + Result.Loc = std::move(*LSPLoc); + Result.Attributes = + ReferencesResult::Declaration | ReferencesResult::Override; + Results.References.push_back(std::move(Result)); + } + if (auto LSPLoc = toLSPLocation(Object.Definition, *MainFilePath)) { + ReferencesResult::Reference Result; + Result.Loc = std::move(*LSPLoc); + Result.Attributes = ReferencesResult::Declaration | + ReferencesResult::Definition | + ReferencesResult::Override; + Results.References.push_back(std::move(Result)); + } + }); + if (Index && Results.References.size() <= Limit) { for (const Decl *D : Decls) { // Not all symbols can be referenced from outside (e.g. @@ -1429,13 +1443,11 @@ ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit, }); }; QueryIndex(std::move(IDs), /*AllowAttributes=*/true); - // FIXME: Currently we surface decls/defs/refs of derived methods. - // Maybe we'd prefer decls/defs of derived methods, and refs of base methods? - QueryIndex(std::move(Overrides), /*AllowAttributes=*/false); if (Results.References.size() > Limit) { Results.HasMore = true; Results.References.resize(Limit); } + // FIXME: Report refs of base methods. return Results; } @@ -1507,6 +1519,8 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, OS << " [decl]"; if (R.Attributes & ReferencesResult::Definition) OS << " [def]"; + if (R.Attributes & ReferencesResult::Override) + OS << " [override]"; return OS; } diff --git a/clang-tools-extra/clangd/XRefs.h b/clang-tools-extra/clangd/XRefs.h index e4a9f7411d96..3f69106611dc 100644 --- a/clang-tools-extra/clangd/XRefs.h +++ b/clang-tools-extra/clangd/XRefs.h @@ -83,6 +83,8 @@ struct ReferencesResult { enum ReferenceAttributes : unsigned { Declaration = 1 << 0, Definition = 1 << 1, + // The occurrence is an override of the target base method. + Override = 1 << 2, }; struct Reference { Location Loc; diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp index 30a8b0ade1ee..966656e47d8f 100644 --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -1706,6 +1706,15 @@ void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) { for (const auto &R : T.ranges("decl")) ExpectedLocations.push_back( AllOf(RangeIs(R), AttrsAre(ReferencesResult::Declaration))); + for (const auto &R : T.ranges("overridedecl")) + ExpectedLocations.push_back(AllOf( + RangeIs(R), + AttrsAre(ReferencesResult::Declaration | ReferencesResult::Override))); + for (const auto &R : T.ranges("overridedef")) + ExpectedLocations.push_back( + AllOf(RangeIs(R), AttrsAre(ReferencesResult::Declaration | + ReferencesResult::Definition | + ReferencesResult::Override))); EXPECT_THAT( findReferences(AST, T.point(), 0, UseIndex ? TU.index().get() : nullptr) .References, @@ -1871,10 +1880,11 @@ TEST(FindReferences, IncludeOverrides) { }; class Derived : public Base { public: - void [[func]]() override; + void $overridedecl[[func]]() override; }; + void Derived::$overridedef[[func]]() {} void test(Derived* D) { - D->[[func]](); + D->func(); // No references to the overrides. })cpp"; checkFindRefs(Test, /*UseIndex=*/true); }