[clangd] Do not report anonymous entities in findExplicitReferences

[ilya-biryukov]

Summary

Otherwise every client dealing with name location should handle
anonymous names in a special manner.

This seems too error-prone, clients can probably handle anonymous
entities they care about differently.

[ilya-biryukov]

Stumbled upon it when trying to move semantic highlight to use findExplicitReferences.
This will create some trouble for D68937, but I believe it's actually worth it. NameLoc was never intended to point outside the name token.

We should be better off without anonymous names here in the long run.

[hokein]

clang-tools-extra/clangd/FindTarget.cpp

454

nit: I think if (!ND->getDeclName().getAsIdentifierInfo()) is enough, getAsIdentifierInfo returns null if the name is not an identifier.

[kadircet]

clang-tools-extra/clangd/FindTarget.cpp

453

i believe this will break D68937, which relies on findExplicitReferences to even rename unnamed parameters.

[merge_guards_bot]

Build result: pass - 59699 tests passed, 0 failed and 763 were skipped.
Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.cpp

453

That's true, we'll have to handle them separately.

It's a bit more code, but I think it's worth it: having a name location pointing to commas or closing parentheses is super unexpected, having some custom logic to name unnamed things is probably a bit more code, but it should be less surprising.
Sorry about that.

454

Good point, I've also tried that, but ended up with the current version.

The proposed solution also removes references to constructors (and I presume various special names like operator+, etc.)
I'm not sure what to do with those, they pose a different problem: unlike anonymous names, they're actually spelled out in the source code.

However, having a single SourceLocation is definitely not enough to figure out the interesting source ranges, so we'll have to send more information about the name location to the clients...

I'm currently considering saving a DeclarationNameInfo in the ReferenceLoc, but I'll need to check whether that's actually possible in all cases, so decided to move this into a separate change.

clang-tools-extra/clangd/FindTarget.cpp

void VisitNamespaceAliasDecl(const NamespaceAliasDecl *D) {
                                   /*IsDecl=*/false,
                                   {D->getAliasedNamespace()}});
     }
 
     void VisitNamedDecl(const NamedDecl *ND) {
       // FIXME: decide on how to surface destructors when we need them.
       if (llvm::isa<CXXDestructorDecl>(ND))
         return;
-      Refs.push_back(ReferenceLoc{
-          getQualifierLoc(*ND), ND->getLocation(), /*IsDecl=*/true, {ND}});
+      // Filter anonymous decls, name location will point outside the name token
+      // and the clients are not prepared to handle that.
+      if (ND->getDeclName().isIdentifier() &&
+          !ND->getDeclName().getAsIdentifierInfo())
+        return;
+      Refs.push_back(ReferenceLoc{getQualifierLoc(*ND),
+                                  ND->getLocation(),
+                                  /*IsDecl=*/true,
+                                  {ND}});
     }
   };
 
   Visitor V;
   V.Visit(D);
   return V.Refs;
 }
 

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

std::pair</*Code*/ llvm::StringRef, /*References*/ llvm::StringRef> Cases[] =
                };
                // base initializer
                class $4^Derived : public $5^Base {
                  $6^Base $7^B;
                  $8^Derived() : $9^Base() {}
                };
                // delegating initializer
                class $10^Foo {
-                 $11^Foo(int$12^);
-                 $13^Foo(): $14^Foo(111) {}
+                 $11^Foo(int);
+                 $12^Foo(): $13^Foo(111) {}
                };
              }
            )cpp",
            "0: targets = {X}, decl\n"
            "1: targets = {foo()::X::abc}, decl\n"
            "2: targets = {foo()::X::X}, decl\n"
            "3: targets = {foo()::X::abc}\n"
            "4: targets = {Derived}, decl\n"
            "5: targets = {Base}\n"
            "6: targets = {Base}\n"
            "7: targets = {foo()::Derived::B}, decl\n"
            "8: targets = {foo()::Derived::Derived}, decl\n"
            "9: targets = {Base}\n"
            "10: targets = {Foo}, decl\n"
            "11: targets = {foo()::Foo::Foo}, decl\n"
-           // FIXME: we should exclude the built-in type.
-           "12: targets = {}, decl\n"
-           "13: targets = {foo()::Foo::Foo}, decl\n"
-           "14: targets = {Foo}\n"},
-
+           "12: targets = {foo()::Foo::Foo}, decl\n"
+           "13: targets = {Foo}\n"},
+          // Anonymous entities should not be reported.
+          {
+              R"cpp(
+             void foo() {
+              class {} $0^x;
+              int (*$1^fptr)(int $2^a, int) = nullptr;
+             }
+           )cpp",
+              "0: targets = {x}, decl\n"
+              "1: targets = {fptr}, decl\n"
+              "2: targets = {a}, decl\n"},
       };
 
   for (const auto &C : Cases) {
     llvm::StringRef ExpectedCode = C.first;
     llvm::StringRef ExpectedRefs = C.second;
 
     auto Actual =
         annotateReferencesInFoo(llvm::Annotations(ExpectedCode).code());
     EXPECT_EQ(ExpectedCode, Actual.AnnotatedCode);
     EXPECT_EQ(ExpectedRefs, Actual.DumpedReferences) << ExpectedCode;
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang