[clangd] Only report explicitly typed symbols during code navigation

[kadircet]

Summary

Clangd was reporting implicit symbols, like results of implicit cast
expressions during code navigation, which is not desired. For example:

struct Foo{ Foo(int); };
void bar(Foo);
vod foo() {
  int x;
  bar(^x);
}

Performing a GoTo on the point specified by ^ would give two results one
pointing to line int x and the other for definition of Foo(int);

[ilya-biryukov]

Haojian is the author, so adding him as a reviewer. LGTM from my side

unittests/clangd/XRefsTests.cpp

416

Does that mean we're not showing constructor results in:

X foo^(something);

It's the only thing that I would miss. OTOH, I also think it should be attached to the parentheses and it's not the case now.

Anyhow, not a huge deal.

[hokein]

+1 on removing the implicit references, sometimes it gives very confusing results.

clangd/XRefs.cpp

112–113

This structure is not used anymore, remove it.

115–116

The comment is stale.

unittests/clangd/XRefsTests.cpp

465–468

Can we add one more testcase?

Foo ab^c;

472–473

The comment is stale.

[kadircet]

• Add more test cases
• Get rid of number of parameters check in implicitness control

[kadircet]

unittests/clangd/XRefsTests.cpp

472–473

No more

[hokein]

unittests/clangd/XRefsTests.cpp

462

nit: keep the number in sort.

483

s/calss/class

[kadircet]

• Address comments

clangd/XRefs.cpp

SymbolLocation getPreferredLocation(const Location &ASTLoc,
   auto Merged = mergeSymbol(ASTSym, IdxSym);
   return Merged.CanonicalDeclaration;
 }
 
 struct MacroDecl {
   llvm::StringRef Name;
   const MacroInfo *Info;
 };
 
-struct DeclInfo {
-  const Decl *D;
-  // Indicates the declaration is referenced by an explicit AST node.
-  bool IsReferencedExplicitly = false;
-};
-
 /// Finds declarations locations that a given source location refers to.
 class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
   std::vector<MacroDecl> MacroInfos;
-  // The value of the map indicates whether the declaration has been referenced
-  // explicitly in the code.
-  // True means the declaration is explicitly referenced at least once; false
-  // otherwise.
-  llvm::DenseMap<const Decl *, bool> Decls;
+  llvm::DenseSet<const Decl *> Decls;
   const SourceLocation &SearchedLocation;
   const ASTContext &AST;
   Preprocessor &PP;
 
 public:
   DeclarationAndMacrosFinder(const SourceLocation &SearchedLocation,
                              ASTContext &AST, Preprocessor &PP)
       : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
 
-  // Get all DeclInfo of the found declarations.
-  // The results are sorted by "IsReferencedExplicitly" and declaration
-  // location.
-  std::vector<DeclInfo> getFoundDecls() const {
-    std::vector<DeclInfo> Result;
-    for (auto It : Decls) {
-      Result.emplace_back();
-      Result.back().D = It.first;
-      Result.back().IsReferencedExplicitly = It.second;
-    }
+  // The results are sorted by declaration location.
+  std::vector<const Decl *> getFoundDecls() const {
+    std::vector<const Decl *> Result;
+    for (const Decl *D : Decls)
+      Result.push_back(D);
 
-    // Sort results. Declarations being referenced explicitly come first.
-    llvm::sort(Result, [](const DeclInfo &L, const DeclInfo &R) {
-      if (L.IsReferencedExplicitly != R.IsReferencedExplicitly)
-        return L.IsReferencedExplicitly > R.IsReferencedExplicitly;
-      return L.D->getBeginLoc() < R.D->getBeginLoc();
+    llvm::sort(Result, [](const Decl *L, const Decl *R) {
+      return L->getBeginLoc() < R->getBeginLoc();
     });
     return Result;
   }
 
   std::vector<MacroDecl> takeMacroInfos() {
     // Don't keep the same Macro info multiple times.
     llvm::sort(MacroInfos, [](const MacroDecl &Left, const MacroDecl &Right) {
       return Left.Info < Right.Info;
handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
     if (Loc == SearchedLocation) {
       auto IsImplicitExpr = [](const Expr *E) {
         if (!E)
           return false;
         // We assume that a constructor expression is implict (was inserted by
         // clang) if it has an invalid paren/brace location, since such
         // experssion is impossible to write down.
         if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(E))
-          return CtorExpr->getNumArgs() > 0 &&
-                 CtorExpr->getParenOrBraceRange().isInvalid();
+          return CtorExpr->getParenOrBraceRange().isInvalid();
         return isa<ImplicitCastExpr>(E);
       };
 
-      bool IsExplicit = !IsImplicitExpr(ASTNode.OrigE);
+      if (IsImplicitExpr(ASTNode.OrigE))
+        return true;
       // 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.insert(Def);
       } else {
         // Couldn't find a definition, fall back to use `D`.
-        Decls[D] |= IsExplicit;
+        Decls.insert(D);
       }
     }
     return true;
   }
 
 private:
   void finish() override {
     // Also handle possible macro at the searched location.
if (!Lexer::getRawToken(Mgr.getSpellingLoc(SearchedLocation), Result, Mgr,
           assert(Decls.empty());
         }
       }
     }
   }
 };
 
 struct IdentifiedSymbol {
-  std::vector<DeclInfo> Decls;
+  std::vector<const Decl *> Decls;
   std::vector<MacroDecl> Macros;
 };
 
 IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) {
   auto DeclMacrosFinder = DeclarationAndMacrosFinder(Pos, AST.getASTContext(),
                                                      AST.getPreprocessor());
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
   // These are up-to-date, and so generally preferred over index results.
   // We perform a single batch index lookup to find additional definitions.
 
   // Results follow the order of Symbols.Decls.
   // Keep track of SymbolID -> index mapping, to fill in index data later.
   llvm::DenseMap<SymbolID, size_t> ResultIndex;
 
   // Emit all symbol locations (declaration or definition) from AST.
-  for (const DeclInfo &DI : Symbols.Decls) {
-    const Decl *D = DI.D;
+  for (const Decl *D : Symbols.Decls) {
     auto Loc = makeLocation(AST, findNameLoc(D), *MainFilePath);
     if (!Loc)
       continue;
 
     Result.emplace_back();
     if (auto *ND = dyn_cast<NamedDecl>(D))
       Result.back().Name = printName(AST.getASTContext(), *ND);
     Result.back().PreferredDeclaration = *Loc;

 
 } // namespace
 
 std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,
                                                       Position Pos) {
   const SourceManager &SM = AST.getASTContext().getSourceManager();
   auto Symbols = getSymbolAtPosition(
       AST, getBeginningOfIdentifier(AST, Pos, SM.getMainFileID()));
-  std::vector<const Decl *> TargetDecls;
-  for (const DeclInfo &DI : Symbols.Decls) {
-    TargetDecls.push_back(DI.D);
-  }
-  auto References = findRefs(TargetDecls, AST);
+  auto References = findRefs(Symbols.Decls, AST);
 
   std::vector<DocumentHighlight> Result;
   for (const auto &Ref : References) {
     DocumentHighlight DH;
     DH.range = getTokenRange(AST, Ref.Loc);
     if (Ref.Role & index::SymbolRoleSet(index::SymbolRole::Write))
       DH.kind = DocumentHighlightKind::Write;
     else if (Ref.Role & index::SymbolRoleSet(index::SymbolRole::Read))
SourceLocation SourceLocationBeg =
       getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
   // Identified symbols at a specific position.
   auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
 
   if (!Symbols.Macros.empty())
     return getHoverContents(Symbols.Macros[0].Name);
 
   if (!Symbols.Decls.empty())
-    return getHoverContents(Symbols.Decls[0].D);
+    return getHoverContents(Symbols.Decls[0]);
 
   auto DeducedType = getDeducedType(AST, SourceLocationBeg);
   if (DeducedType && !DeducedType->isNull())
     return getHoverContents(*DeducedType, AST.getASTContext());
 
   return None;
 }
 
 std::vector<Location> findReferences(ParsedAST &AST, Position Pos,
                                      uint32_t Limit, const SymbolIndex *Index) {
   if (!Limit)
     Limit = std::numeric_limits<uint32_t>::max();
   std::vector<Location> Results;
   const SourceManager &SM = AST.getASTContext().getSourceManager();
   auto MainFilePath =
       getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
   if (!MainFilePath) {
     elog("Failed to get a path for the main file, so no references");
     return Results;
   }
   auto Loc = getBeginningOfIdentifier(AST, Pos, SM.getMainFileID());
   auto Symbols = getSymbolAtPosition(AST, Loc);
 
-  std::vector<const Decl *> TargetDecls;
-  for (const DeclInfo &DI : Symbols.Decls) {
-    if (DI.IsReferencedExplicitly)
-      TargetDecls.push_back(DI.D);
-  }
-
   // We traverse the AST to find references in the main file.
   // TODO: should we handle macros, too?
-  auto MainFileRefs = findRefs(TargetDecls, AST);
+  auto MainFileRefs = findRefs(Symbols.Decls, AST);
   for (const auto &Ref : MainFileRefs) {
     Location Result;
     Result.range = getTokenRange(AST, Ref.Loc);
     Result.uri = URIForFile::canonicalize(*MainFilePath, *MainFilePath);
     Results.push_back(std::move(Result));
   }
 
   // Now query the index for references from other files.
   if (Index && Results.size() < Limit) {
     RefsRequest Req;
     Req.Limit = Limit;
 
-    for (const Decl *D : TargetDecls) {
+    for (const Decl *D : Symbols.Decls) {
       // Not all symbols can be referenced from outside (e.g. function-locals).
       // TODO: we could skip TU-scoped symbols here (e.g. static functions) if
       // we know this file isn't a header. The details might be tricky.
       if (D->getParentFunctionOrMethod())
         continue;
       if (auto ID = getSymbolID(D))
         Req.IDs.insert(*ID);
     }

 std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos) {
   const SourceManager &SM = AST.getASTContext().getSourceManager();
 
   auto Loc = getBeginningOfIdentifier(AST, Pos, SM.getMainFileID());
   auto Symbols = getSymbolAtPosition(AST, Loc);
 
   std::vector<SymbolDetails> Results;
 
-  for (const auto &Sym : Symbols.Decls) {
+  for (const Decl *D : Symbols.Decls) {
     SymbolDetails NewSymbol;
-    if (const NamedDecl *ND = dyn_cast<NamedDecl>(Sym.D)) {
+    if (const NamedDecl *ND = dyn_cast<NamedDecl>(D)) {
       std::string QName = printQualifiedName(*ND);
       std::tie(NewSymbol.containerName, NewSymbol.name) =
           splitQualifiedName(QName);
 
       if (NewSymbol.containerName.empty()) {
         if (const auto *ParentND =
                 dyn_cast_or_null<NamedDecl>(ND->getDeclContext()))
           NewSymbol.containerName = printQualifiedName(*ParentND);
       }
     }
     llvm::SmallString<32> USR;
-    if (!index::generateUSRForDecl(Sym.D, USR)) {
+    if (!index::generateUSRForDecl(D, USR)) {
       NewSymbol.USR = USR.str();
       NewSymbol.ID = SymbolID(NewSymbol.USR);
     }
     Results.push_back(std::move(NewSymbol));
   }
 
   for (const auto &Macro : Symbols.Macros) {
     SymbolDetails NewMacro;

unittests/clangd/SymbolInfoTests.cpp

std::pair<const char *, std::vector<SymbolDetails>>
             bar(const foo&) {}
           };
           void func_baz1(bar) {}
           void func_baz2() {
             foo ff;
             func_baz1(f^f);
           }
         )cpp",
-              {
-                  CreateExpectedSymbolDetails(
-                      "ff", "func_baz2", "c:TestTU.cpp@218@F@func_baz2#@ff"),
-                  CreateExpectedSymbolDetails(
-                      "bar", "bar::", "c:@S@bar@F@bar#&1$@S@foo#"),
-              }},
+              {CreateExpectedSymbolDetails(
+                  "ff", "func_baz2", "c:TestTU.cpp@218@F@func_baz2#@ff")}},
           {
               R"cpp( // Type reference - declaration
           struct foo;
           void bar(fo^o*);
         )cpp",
               {CreateExpectedSymbolDetails("foo", "", "c:@S@foo")}},
           {
               R"cpp( // Type reference - definition

unittests/clangd/XRefsTests.cpp

const char *Tests[] = {
         template <class T>
         T var = T();
 
         template <>
         double [[var]]<int> = 10;
 
         double y = va^r<int>;
       )cpp",
+
+      R"cpp(// No implicit constructors
+        class X {
+          X(X&& x) = default;
+        };
+        X [[makeX]]() {}
+        void foo() {
+          auto x = m^akeX();
+        }
+      )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     llvm::Optional<Range> WantDecl;
     llvm::Optional<Range> WantDef;
     if (!T.ranges().empty())
       WantDecl = WantDef = T.range();
     if (!T.ranges("decl").empty())
auto T = Annotations(R"cpp(
 
     Foo f();
 
     void g(Foo foo);
 
     void call() {
       const char* str = "123";
       Foo a = $1^str;
       Foo b = Foo($2^str);
       Foo c = $3^f();
       $4^g($5^f());
       g($6^str);
+      Foo ab$7^c;
+      Foo ab$8^cd("asdf");
+      Foo foox = Fo$9^o("asdf");
     }
   )cpp");
   auto AST = TestTU::withCode(T.code()).build();
   // Ordered assertions are deliberate: we expect a predictable order.
-  EXPECT_THAT(locateSymbolAt(AST, T.point("1")),
-              ElementsAre(Sym("str"), Sym("Foo")));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("1")), ElementsAre(Sym("str")));
   EXPECT_THAT(locateSymbolAt(AST, T.point("2")), ElementsAre(Sym("str")));
-  EXPECT_THAT(locateSymbolAt(AST, T.point("3")),
-              ElementsAre(Sym("f"), Sym("Foo")));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("3")), ElementsAre(Sym("f")));
   EXPECT_THAT(locateSymbolAt(AST, T.point("4")), ElementsAre(Sym("g")));
-  EXPECT_THAT(locateSymbolAt(AST, T.point("5")),
-              ElementsAre(Sym("f"), Sym("Foo")));
-  EXPECT_THAT(locateSymbolAt(AST, T.point("6")),
-              ElementsAre(Sym("str"), Sym("Foo"), Sym("Foo")));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("5")), ElementsAre(Sym("f")));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("6")), ElementsAre(Sym("str")));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("7")), ElementsAre(Sym("abc")));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("8")),
+              ElementsAre(Sym("Foo"), Sym("abcd")));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("9")),
+              // First one is class definition, second is the constructor.
+              ElementsAre(Sym("Foo"), Sym("Foo")));
 }
 
 TEST(LocateSymbol, RelPathsInCompileCommand) {
   // The source is in "/clangd-test/src".
   // We build in "/clangd-test/build".
 
   Annotations SourceAnnotations(R"cpp(
 #include "header_in_preamble.h"