[clangd] Fix irrelevant declaratations in goto definition (on macros).

[hokein]

Summary

DeclrationAndMacrosFinder will find some declarations (not macro!) that are
referened inside the macro somehow, isSearchedLocation() is not sufficient, we
don't know whether the searched source location is macro or not.

[ilya-biryukov]

clangd/XRefs.cpp

201

I wonder whether we should fix the DeclrationAndMacrosFinder to not return declarations coming from macro instantiations instead.
There are other clients (e.g. document highlights) that will probably break in the same way.

Do you think it would be possible and it would make sense for DeclrationAndMacrosFinder to only return a macro in this case and not return Decls coming from macro expansions?

[hokein]

clangd/XRefs.cpp

201

I thought about it initially, but the information provided handleDeclOccurrence is limited...the occurrence location (FID and Offset) is expansion location (which is not always we want). That being said, when GoToDefinition on a macro, all declarations inside the macro body will be picked up.

In document hover implementation, we also use the same mechanism to avoid this problem :(

[ilya-biryukov]

clangd/XRefs.cpp

201

As discussed offline, we should probably move this logic to DeclrationAndMacrosFinder so that all its clients (hover, documentHighlights, findDefinitions) are consistent.
Having a single function in DeclrationAndMacrosFinder that returns results (currently there are two: takeDecls() and takeMacros()) would allow that with minimal changes.

[hokein]

Address review comment.

clangd/XRefs.cpp

201

As discussed, made the workaround in Finder to keep the change minimal, no changes are required in client side.

[ilya-biryukov]

clangd/XRefs.cpp

137

NIT: the fixme was a bit hard to follow for me. Maybe make it more clear where the problem should be handled, e.g.

FIXME: we should avoid adding decls from inside macros in handlDeclOccurence.

[ilya-biryukov]

LGTM with a small nit about the comment.

[hokein]

Rephrase the fixme.

clangd/XRefs.cpp

if (!Lexer::getRawToken(SearchedLocation, Result, Mgr, AST.getLangOpts(),
         }
       }
     }
   }
 };
 
 llvm::Optional<Location>
 getDeclarationLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
   SourceLocation LocStart = ValSourceRange.getBegin();
 
   const FileEntry *F =
       SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));
   if (!F)
     return llvm::None;
   SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos) {
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
   IndexOpts.IndexFunctionLocals = true;
 
   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
                      DeclMacrosFinder, IndexOpts);
 
-  std::vector<const Decl *> Decls = DeclMacrosFinder->takeDecls();
   std::vector<MacroDecl> MacroInfos = DeclMacrosFinder->takeMacroInfos();
-
-  for (auto Item : Decls) {
-    auto L = getDeclarationLocation(AST, Item->getSourceRange());
-    if (L)
-      Result.push_back(*L);
-  }
-
+  if (!MacroInfos.empty()) {
     for (auto Item : MacroInfos) {
       SourceRange SR(Item.Info->getDefinitionLoc(),
                      Item.Info->getDefinitionEndLoc());
       auto L = getDeclarationLocation(AST, SR);
       if (L)
         Result.push_back(*L);
     }
 
     return Result;
   }
 
+  for (auto Item : DeclMacrosFinder->takeDecls()) {
+    auto L = getDeclarationLocation(AST, Item->getSourceRange());
+    if (L)
+      Result.push_back(*L);
+  }
+
+  return Result;
+}
+
 namespace {
 
 /// Finds document highlights that a given list of declarations refers to.
 class DocumentHighlightsFinder : public index::IndexDataConsumer {
   std::vector<const Decl *> &Decls;
   std::vector<DocumentHighlight> DocumentHighlights;
   const ASTContext &AST;
 

unittests/clangd/XRefsTests.cpp

const char *Tests[] = {
       R"cpp(// Macro
         #define MACRO 0
         #define [[MACRO 1]]
         int main() { return ^MACRO; }
         #define MACRO 2
         #undef macro
       )cpp",
 
+      R"cpp(// Macro
+       class TTT { public: int a; };
+       #define [[FF(S) if (int b = S.a) {}]]
+       void f() {
+         TTT t;
+         F^F(t);
+       }
+      )cpp",
+
       R"cpp(// Forward class declaration
         class Foo;
         [[class Foo {}]];
         F^oo* foo();
       )cpp",
 
       R"cpp(// Function declaration
         void foo();