[clangd] Refine the way of checking a declaration is referenced by the written code.

[hokein]

Summary

The previous solution (checking the AST) is not a reliable way to
determine whether a declaration is explicitly referenced by the source
code, we are still missing a few cases.

[ilya-biryukov]

The idea of using the AST-based approach here was really nice, it was less expensive and seemed to clearly look at the semantic.
I wonder if there's a way to keep it on the AST level, without looking at the source locations.

What are the cases we're trying to filter out? Only implicit constructor or anything else?

unittests/clangd/XRefsTests.cpp

1228

I'm missing what does this test actually tests.
The absence of implicit references (I guess constructor expressions)?

1259

Shouldn't the usings always be qualified? Isn't this a compiler error?

[hokein]

Update based on our offline discussion.

[hokein]

What are the cases we're trying to filter out? Only implicit constructor or anything else?

I think implicit constructor is the only case we care about. We also looked through the caller implementation of handleDeclOccurence, it is safe to assume it.

unittests/clangd/XRefsTests.cpp

1228

This test tests the cases where symbol is being marked implicit incorrectly, which will result in no result in xref.

1259

Yes, this is a compiler error, but clang's error recovery can handle it. Fixed.

[ilya-biryukov]

LGTM. Many thanks for the fix, really important to get those cases right.

unittests/clangd/XRefsTests.cpp

1228

Thanks for clarification! The tested cases were so simple that I though they worked before.
(Which shows this change is very important, IMHO)

clangd/XRefs.cpp

public:
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
                       ArrayRef<index::SymbolRelation> Relations,
                       SourceLocation Loc,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
     if (Loc == SearchedLocation) {
-      // Check whether the E has an implicit AST node (e.g. ImplicitCastExpr).
-      auto hasImplicitExpr = [](const Expr *E) {
-        if (!E || E->child_begin() == E->child_end())
+      // Check whether the give ND is explicitly referenced by the written
+      // identifier at Loc in the source code.
+      auto IsWrittenInCode = [this](const NamedDecl *ND, SourceLocation Loc) {
+        if (!ND)
           return false;
-        // Use the first child is good enough for most cases -- normally the
-        // expression returned by handleDeclOccurence contains exactly one
-        // child expression.
-        const auto *FirstChild = *E->child_begin();
-        return isa<ExprWithCleanups>(FirstChild) ||
-               isa<MaterializeTemporaryExpr>(FirstChild) ||
-               isa<CXXBindTemporaryExpr>(FirstChild) ||
-               isa<ImplicitCastExpr>(FirstChild);
+        auto &SM = AST.getSourceManager();
+        auto FileIDAndOffset = SM.getDecomposedLoc(Loc);
+        auto Code = SM.getBufferData(FileIDAndOffset.first);
+        auto IdentifierLength =
+            clang::Lexer::MeasureTokenLength(Loc, SM, AST.getLangOpts());
+        StringRef WrittenName =
+            Code.substr(FileIDAndOffset.second, IdentifierLength);
+        return WrittenName == ND->getDeclName().getAsString();
       };
-
-      bool IsExplicit = !hasImplicitExpr(ASTNode.OrigE);
+      bool WrittenInCode = IsWrittenInCode(dyn_cast<NamedDecl>(D), Loc);
       // Find and add definition declarations (for GoToDefinition).
       // We don't use parameter `D`, as Parameter `D` is the canonical
       // declaration, which is the first declaration of a redeclarable
       // declaration, and it could be a forward declaration.
       if (const auto *Def = getDefinition(D)) {
-        Decls[Def] |= IsExplicit;
+        Decls[Def] |= WrittenInCode;
       } else {
         // Couldn't find a definition, fall back to use `D`.
-        Decls[D] |= IsExplicit;
+        Decls[D] |= WrittenInCode;
       }
     }
     return true;
   }
 
 private:
   void finish() override {
     // Also handle possible macro at the searched location.

unittests/clangd/XRefsTests.cpp

for (const char *Test : Tests) {
     for (const auto &R : T.ranges())
       ExpectedLocations.push_back(RangeIs(R));
     EXPECT_THAT(findReferences(AST, T.point()),
                 ElementsAreArray(ExpectedLocations))
         << Test;
   }
 }
 
+TEST(FindReferences, ExplicitSymbols) {
+  const char *Tests[] = {
+      R"cpp(
+      struct Foo { Foo* [self]() const; };
+      void f() {
+        if (Foo* T = foo.[^self]()) {} // Foo member call expr.
+      }
+      )cpp",
+
+      R"cpp(
+      struct Foo { Foo(int); };
+      Foo f() {
+        int [b];
+        return [^b]; // Foo constructor expr.
+      }
+      )cpp",
+
+      R"cpp(
+      struct Foo {};
+      void g(Foo);
+      Foo [f]();
+      void call() {
+        g([^f]());  // Foo constructor expr.
+      }
+      )cpp",
+
+      R"cpp(
+      void [foo](int);
+      void [foo](double);
+
+      namespace ns {
+      using [fo^o];
+      }
+      )cpp",
+  };
+  for (const char *Test : Tests) {
+    Annotations T(Test);
+    auto AST = TestTU::withCode(T.code()).build();
+    std::vector<Matcher<Location>> ExpectedLocations;
+    for (const auto &R : T.ranges())
+      ExpectedLocations.push_back(RangeIs(R));
+    EXPECT_THAT(findReferences(AST, T.point()),
+                ElementsAreArray(ExpectedLocations))
+        << Test;
+  }
+}
+
 TEST(FindReferences, NeedsIndex) {
   const char *Header = "int foo();";
   Annotations Main("int main() { [[f^oo]](); }");
   TestTU TU;
   TU.Code = Main.code();
   TU.HeaderCode = Header;
   auto AST = TU.build();