[clangd] Include refs of base method in refs for derived method.

[usaxena95]

Summary

Addresses https://github.com/clangd/clangd/issues/881

Includes refs of base class method in refs of derived class method.
Previously we reported base class method's refs only for decl of derived
class method. Ideally this should work for all usages of derived class method.

Related patch:
https://github.com/llvm/llvm-project/commit/fbeff2ec2bc6e44b92931207b0063f83ff7a3b3a.

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

1395

it's important to note that this doesn't only suppress xrefs behaviour outside of the declaration, but it also suppresses xrefs on parts of the declaration apart from the function's name (like override/virtual keywords). it's unfortunate that we don't have tests to explicitly point out that fact.
I am not sure why the decision was to exclude them in the first place, I am definitely in favor of the new behaviour (we already provide go-to-def on override keyword for example, not having xrefs on it seems surprising). I suppose @hokein can tell if there are any other concerns I am missing.

clang-tools-extra/clangd/unittests/XRefsTests.cpp

1993

can we instead turn this test into a multi point check? (e.g. check that references in virt^ual void fu^nc() over^ride; ... D->fu^nc(); are all the same?

[hokein]

clang-tools-extra/clangd/XRefs.cpp

1395

yeah, I think the initial design was to return all base-class references when xrefs is called at the *declaration* (our previous concern was that base-class refs might add a lot of noises to the final results, which might be not what users want). Now we have user complains, let's do it for all places (LG the change).

Re the behavior on override keyword, this is probably an oversight (xrefs was not considered when we extended go-to-def). The implementation of go-to-def is ad-hoc, and only works for override, and final keywords (due to the limitation of AST). not sure it is a really useful feature, for consistency, it would be nice to have it in xrefs. I think it can be addressed in a follow-up patch.

[usaxena95]

Addressed comments.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/XRefs.cpp

1397

note that there's a behaviour change here as well. not sure how often it matters in practice, but previously we would still get the overriden methods for a symbol even if we fail to generate symbolid for it. let's keep it the same.

clang-tools-extra/clangd/unittests/XRefsTests.cpp

1787

nit: I'd add braces here, even though this is still a single statement, it spans multiple lines and becomes confusing.

[usaxena95]

Addressed comments.

[usaxena95]

clang-tools-extra/clangd/XRefs.cpp

1397

Hmm. You are right. Even in getOverriddenMethods we find the symbolID *optionally* and we always traverse the complete type hierarchy. Let's do the same here for consistency.

clang-tools-extra/clangd/XRefs.cpp

if (Macro) {
     RelationsRequest OverriddenBy;
     if (Index) {
       OverriddenBy.Predicate = RelationKind::OverriddenBy;
       for (const NamedDecl *ND : Decls) {
         // Special case: For virtual methods, report decl/def of overrides and
         // references to all overridden methods in complete type hierarchy.
         if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(ND)) {
           if (CMD->isVirtual())
-            if (IdentifierAtCursor && SM.getSpellingLoc(CMD->getLocation()) ==
-                                          IdentifierAtCursor->location()) {
-              if (auto ID = getSymbolID(CMD))
-                OverriddenBy.Subjects.insert(ID);
+            if (auto ID = getSymbolID(CMD)) {
               getOverriddenMethods(CMD, OverriddenMethods);
             }
         }
       }
     }
 
     // We traverse the AST to find references in the main file.
     auto MainFileRefs = findRefs(Targets, AST, /*PerToken=*/false);
     // We may get multiple refs with the same location and different Roles, as

clang-tools-extra/clangd/unittests/XRefsTests.cpp

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,
       UnorderedElementsAreArray(ExpectedLocations))
       << Test;
 }
 
 TEST(FindReferences, WithinAST) {
   const char *Tests[] = {
       R"cpp(// Local variable
         int main() {
           int $def[[foo]];
           [[^foo]] = 2;
llvm::StringRef Test =
         };
         void test(Derived* D) {
           D->func();  // No references to the overrides.
         })cpp";
   checkFindRefs(Test, /*UseIndex=*/true);
 }
 
 TEST(FindReferences, RefsToBaseMethod) {
-  llvm::StringRef Test =
+  const char *Tests[] = {
       R"cpp(
         class BaseBase {
         public:
           virtual void [[func]]();
         };
         class Base : public BaseBase {
         public:
           void [[func]]() override;
         };
         class Derived : public Base {
         public:
           void $decl[[fu^nc]]() override;
         };
         void test(BaseBase* BB, Base* B, Derived* D) {
           // refs to overridden methods in complete type hierarchy are reported.
           BB->[[func]]();
           B->[[func]]();
           D->[[func]]();
-        })cpp";
+        }
+      )cpp",
+      R"cpp(
+        class A {
+        public:
+          virtual void [[foo]]();
+        };
+        class B : public A {
+        public:
+          void $decl[[foo]]() override;
+        };
+        void test(A* a, B* b) {
+          a->[[foo]]();
+          b->[[fo^o]]();
+        }
+      )cpp",
+  };
+  for (const char *Test : Tests)
     checkFindRefs(Test, /*UseIndex=*/true);
 }
 
 TEST(FindReferences, MainFileReferencesOnly) {
   llvm::StringRef Test =
       R"cpp(
         void test() {
           int [[fo^o]] = 1;
           // refs not from main file should not be included.