[clangd] Show correct hover tooltip for non-preamble macro definition.

[ilya-golovenko]

Summary

Incorrect definition is shown in tooltip when hovering over a non-preamble macro definition.
In the example below the definition will have 'namespace ns {}' instead of correct '#define FOO 1'.

namespace ns {
#define [[FO^O]] 1
}

[ilya-golovenko]

I'm not quite sure my fix is the best way to fix the issue, so any advices are appreciated.

[kadircet]

Sigh, (I think) this is working for macros defined in preamble region as a side effect of preamble being loaded separately and before anything in the main file. E.g.

#define FO^O 1
#define FOO 2

void foo() { int x = FOO; };

hovering over the first definition of FOO (at carrot) will yield #define FOO 2, because that's the latest definition before main file.

That being said, I don't really think there's much value in showing the hovercard at definition of a macro, as that's what we show anyways. (showing the surrounding namespace is even worse, but that's a different problem and i thought we've solved it recently, are you sure you are seeing that behaviour with a build from head?).
So I believe you are rather trying to fix the issue of displaying misleading hover info, and if that's the case I think we should rather fix that by not displaying hover infos at the macro definition locations.

WDYT?

[ilya-golovenko]

I agree the fix is not correct. I will check how it currently works with clangd built from master branch.

I'm not quite sure there should be no tooltip for a #define. Such tooltip is not really useful, but at the same time clangd shows tooltips for function declarations/defnitions, namespaces, variables, etc. and not showing it for macro definition will be a little bit inconsistent from my point of view.

[kadircet]

I'm not quite sure there should be no tooltip for a #define. Such tooltip is not really useful, but at the same time clangd shows tooltips for function declarations/defnitions, namespaces, variables, etc. and not showing it for macro definition will be a little bit inconsistent from my point of view.

We actually had a couple users saying those are redundant too. I agree with them mostly, i suppose these are only useful because we talk about the associated namespace/class/accesspecifer. E.g. you can easily figure out what's the class owning a field, and whether it is private/public without worrying traversing upwards.
Who knows, maybe we should only be displaying that information on the declaration/definition of symbols rather than all the stuff that's already probably next to user's cursor (I say probably because you can have declarations coming from macro invocations).

clang-tools-extra/clangd/SourceCode.cpp

if (!IdentifierInfo || !IdentifierInfo->hadMacroDefinition())
     return None;
 
   // Get the definition just before the searched location so that a macro
   // referenced in a '#undef MACRO' can still be found. Note that we only do
   // that if Loc is not pointing at start of file.
   if (SM.getLocForStartOfFile(SM.getFileID(Loc)) != Loc)
     Loc = Loc.getLocWithOffset(-1);
   MacroDefinition MacroDef = PP.getMacroDefinitionAtLoc(IdentifierInfo, Loc);
+  // If the definition was not found before the current token location then
+  // it is possible the current token is a macro definition itself.
+  if (!MacroDef) {
+    Loc = SpelledTok.location().getLocWithOffset(1);
+    MacroDef = PP.getMacroDefinitionAtLoc(IdentifierInfo, Loc);
+  }
   if (auto *MI = MacroDef.getMacroInfo())
     return DefinedMacro{
         IdentifierInfo->getName(), MI,
         translatePreamblePatchLocation(MI->getDefinitionLoc(), SM)};
   return None;
 }
 
 llvm::Expected<std::string> Edit::apply() const {

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

const std::function<void(HoverInfo &)> ExpectedBuilder;
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "MACRO";
             HI.Kind = index::SymbolKind::Macro;
             HI.Definition = "#define MACRO 0";
           }},
       {
           R"cpp(// Macro
+            namespace {
+            #define [[MA^CRO]] 0
+            }
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "MACRO";
+            HI.Kind = index::SymbolKind::Macro;
+            HI.Definition = "#define MACRO 0";
+          }},
+      {
+          R"cpp(// Macro
             #define MACRO 0
             #define MACRO2 ^[[MACRO]]
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "MACRO";
             HI.Kind = index::SymbolKind::Macro;
             HI.Definition = "#define MACRO 0";
           }},