[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

213

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

213

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

213

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

213

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

[ilya-biryukov]

clangd/XRefs.cpp

138

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(),
         SourceLocation BeforeSearchedLocation = Mgr.getMacroArgExpandedLocation(
             Mgr.getLocForStartOfFile(DecLoc.first)
                 .getLocWithOffset(DecLoc.second - 1));
         MacroDefinition MacroDef =
             PP.getMacroDefinitionAtLoc(IdentifierInfo, BeforeSearchedLocation);
         MacroInfo *MacroInf = MacroDef.getMacroInfo();
         if (MacroInf) {
           MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
+          // Clear all collected delcarations if this is a macro search.
+          //
+          // In theory, there should be no declarataions being collected when we
+          // search a source location that refers to a macro.
+          // The occurrence location returned by `handleDeclOccurence` is
+          // limited (FID, Offset are from expansion location), we will collect
+          // all declarations inside the macro.
+          //
+          // FIXME: Avoid adding decls from inside macros in handlDeclOccurence.
+          Decls.clear();
         }
       }
     }
   }
 };
 
 llvm::Optional<Location>
 makeLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
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 D : Decls) {
     auto Loc = findNameLoc(D);
     auto L = makeLocation(AST, SourceRange(Loc, Loc));
     if (L)
       Result.push_back(*L);
   }
 
   for (auto Item : MacroInfos) {

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();