[clangd] Enhance macro hover to see full definition

[malaperle]

Summary

Signed-off-by: Marc-Andre Laperle <[e-mail]>

[malaperle]

Clang-format

[hokein]

clangd/XRefs.cpp

567 ↗

(On Diff #176754)

The comment seems stale.

572 ↗

(On Diff #176754)

if this is a complicated macro (like AST_MATCHER), do we still want to return all the content? they might be less useful than the simple macros.

589 ↗

(On Diff #176754)

We are now returning markdown as default, but some LSP clients might not support markdown. I think we should respect the ClientCapabilities -- there is a hover.contentFormat in TextDocumentClientCapabilities.

[simark]

clangd/XRefs.cpp

572 ↗

(On Diff #176754)

I think it would be hard to decide here what constitutes a complicated or not-complicated macro (one for which we want to return all the content or not). Also, I think it's fine to return the whole content, since frontends generally have good ways to present large contents in popups (hover popups that can be scrolled).

If we realize later we need to limit the size, then we can add an option where the frontend can specify a size limit.

[ilya-biryukov]

clangd/XRefs.cpp

578 ↗

(On Diff #176754)

Unfortunately we can't get the buffer here, because for the preamble macros the files might change on disk after we build the preamble and we can end up reading a different version of the file. Which can in turn lead to crashes as the offsets obtained from the source locations can point outside the buffer for the corresponding file.

This is really annoying and generally the solution is to try find a different way to obtain the same information, e.g. by looking at what MacroInfo has available.
However, I don't know of a good workaround for this. Happy to look into ways of providing something close to a macro definition from MacroInfo or other sources, can scout for this tomorrow.

[malaperle]

clangd/XRefs.cpp

572 ↗

(On Diff #176754)

I think it's OK if the client displays it as it sees fit (scrollbar or trimmed).

578 ↗

(On Diff #176754)

I tried to do something like MacroInfo::dump. One problem is that we don't always have enough information about literals. For example:

#define MACRO 0
int main() { return MACRO; }

Becomes:

#define MACRO numeric_constant

I poked around the Token code and I didn't find a way to retrieve the literal without going through the buffer (Preprocessor::getSpelling for example).

What you mention is only a problem when you use the option of storing preambles on disk, right? So something would need to modify the preamble files on disk, which are stored in some temp folder. It doesn't sound like that could happen frequently. There is also bounds checking in the code above so it shouldn't crash, right? Even if the bounds are incorrect, it will be no different than the Range we return to the client for document/definition, i.e. the client can use the range on a newly modified header and get it wrong. I also tested renaming the pch file and then editing the source file and it results in an invalid AST. So either way modifying the pch would be bad news not for just macro hover.

[ilya-biryukov]

clangd/XRefs.cpp

578 ↗

(On Diff #176754)

The problem shows up when the header files are modified, not the preamble files (otherwise we'll be fine, in clangd we do assume the .pch files we produce are not externally modified).
Which is pretty frequent, and we've seen real crashes coming from fetching documentation from the preambles.

I'll try to investigate how Preprocessor accesses the tokens of macro definitions from preabmle. There are two possible outcomes I expect:

1. we find a way to access the spelling of the tokens in the same way PP does and it's safe,
2. we realize preprocessor might crash with the preabmle because it accesses the spelling of the tokens.

I hope to find (1), but (2) is also very realistic, unfortunately :-(

[ilya-biryukov]

clangd/XRefs.cpp

578 ↗

(On Diff #176754)

I think the preprocessor does the same thing, therefore the compiler can potentially access the header files to get spelling of the tokens coming from headers.
This means that clangd can potentially crash if the headers are changed concurrently with the operations that use preabmles (code completion, being the most common one).

In that case, I agree that it's fine to use the buffers for headers here and we should fix this separately, probably by snapshotting the contents of accessed headers.

[ilya-biryukov]

clangd/XRefs.cpp

578 ↗

(On Diff #176754)

And I'm happy to take a look at reproducing the PP crash that I'm talking about here and fixing this.

[malaperle]

Address comments.

[malaperle]

Remove temporary code.

[malaperle]

Ping?

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

823 ↗

(On Diff #185959)

The contents of the hover is not strictly C++ code, so we shouldn't put the whole block into the C++ code.
Could we return the plain text results for now? The hover implementation should probably create markdown (or some other form of formatted text) on its own, but that's out of scope of this patch.

clangd/Protocol.h

378 ↗

(On Diff #185959)

Could we replace this with a single boolean to simplify the code?

bool HoverSupportsMarkdown = false;

Would also allow to get remove MarkupKindBitset.

[malaperle]

Address comments.

[malaperle]

clangd/ClangdLSPServer.cpp

816 ↗

(On Diff #187783)

I don't know if you meant plain text as non-language-specific markdown or no markdown at all. In VSCode, non-markdown for multiline macros looks bad because the newlines are ignored.

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

816 ↗

(On Diff #187783)

Did not know about that, thanks for pointing it out.
It does not ignore double newlines though (that's what we use in declarations). I suspect it treats plaintext as markdown, but can't be sure.

In any case, converting to a markdown code block here looks incredibly hacky and shaky.
Could we use double-newlines for line breaks in our implementation instead?

This aligns with what we did before this patch for declarations.
If that doesn't work, breaking the multi-line macros in VSCode looks ok, this really feels like a bug in VSCode.

clangd/Protocol.cpp

251 ↗

(On Diff #187783)

NIT: remove braces

255 ↗

(On Diff #187783)

NIT: use range-based-for:

for (auto &&E : A) {
 // ...
}

257 ↗

(On Diff #187783)

NIT: merge ifs for better readability

if (fromJSON(...) && KindOut == Markdown) {
  // ....
}

638 ↗

(On Diff #187783)

use early exits to reduce nesting.

auto T = E.getAsString();
if (!T)
  return false;
// rest of the code...

clangd/XRefs.cpp

563 ↗

(On Diff #187783)

Follow-up for the previous comment:
would it work to do replace(Defintion, "\n", "\n\n") here? (with a fixme comment that otherwise VSCode misbehaves)

[malaperle]

clangd/ClangdLSPServer.cpp

816 ↗

(On Diff #187783)

In any case, converting to a markdown code block here looks incredibly hacky and shaky.

I'm not sure why? ClangdLSPServer knows about client capabilities so it made sense to me to convert it there. Either way, it sounds like I should just remove "HoverSupportsMarkdown" altogether if we're not going to use Markdown.

Could we use double-newlines for line breaks in our implementation instead?

It looks odd. When I double them in the hover contents, the lines are displayed as doubled and some extra backslashes are added on all non-empty lines except the first line. Single new lines and correct backslashes are displayed correctly when Markdown is used. We can just display multiline line macros as one line until we want to handle Markdown (which is how exactly?).

[malaperle]

Remove Markdown support.

[ilya-biryukov]

LGTM. Thanks!

clangd/ClangdLSPServer.cpp

816 ↗

(On Diff #187783)

I'm not sure why? ClangdLSPServer knows about client capabilities so it made sense to me to convert it there. Either way, it sounds like I should just remove "HoverSupportsMarkdown" altogether if we're not going to use Markdown.

Don't get me wrong, adding markdown support seems nice, but let's do it as a separate patch. Here's a few thoughts on this.

Currently ClangdServer already already returns a hover with proper kind and contents, so it is ClangdServer's responsibility to produce markdown or plaintext as of today.
We could consider moving the responsibility to decide whether we should return markdown or plaintext to LSPServer, but then we should also change the types of values ClangdServer returns (to a structured representation that would produce plaintext or markdown, depending on the client settings).

Assuming the result is a code block looks wrong, we do return plain text bits like "declared in namespace std" as a result of hover.

It looks odd. When I double them in the hover contents, the lines are displayed as doubled and some extra backslashes are added on all non-empty lines except the first line. Single new lines and correct backslashes are displayed correctly when Markdown is used. We can just display multiline line macros as one line until we want to handle Markdown (which is how exactly?).

Totally on board with this, multiline marcos are not exactly rare, but we could live with this oddity in VSCode for some time.
Adding basic markdown support shouldn't be too hard, but let's block this patch on it.

[ilya-biryukov]

More concretely, here's the proposed API for the intermediate representation of formatted string: D58547

clang-tools-extra/trunk/clangd/XRefs.cpp

static Hover getHoverContents(QualType T, ASTContext &ASTCtx) {
   llvm::raw_string_ostream OS(TypeText);
   PrintingPolicy Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
   T.print(OS, Policy);
   OS.flush();
   H.contents.value += TypeText;
   return H;
 }
 
-/// Generate a \p Hover object given the macro \p MacroInf.
-static Hover getHoverContents(llvm::StringRef MacroName) {
-  Hover H;
-
-  H.contents.value = "#define ";
-  H.contents.value += MacroName;
+/// Generate a \p Hover object given the macro \p MacroDecl.
+static Hover getHoverContents(MacroDecl Decl, ParsedAST &AST) {
+  SourceManager &SM = AST.getASTContext().getSourceManager();
+  std::string Definition = Decl.Name;
+
+  // Try to get the full definition, not just the name
+  SourceLocation StartLoc = Decl.Info->getDefinitionLoc();
+  SourceLocation EndLoc = Decl.Info->getDefinitionEndLoc();
+  if (EndLoc.isValid()) {
+    EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM,
+                                        AST.getASTContext().getLangOpts());
+    bool Invalid;
+    StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
+    if (!Invalid) {
+      unsigned StartOffset = SM.getFileOffset(StartLoc);
+      unsigned EndOffset = SM.getFileOffset(EndLoc);
+      if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
+        Definition = Buffer.substr(StartOffset, EndOffset - StartOffset).str();
+    }
+  }
 
+  Hover H;
+  H.contents.kind = MarkupKind::PlainText;
+  H.contents.value = "#define " + Definition;
   return H;
 }
 
 namespace {
 /// Computes the deduced type at a given location by visiting the relevant
 /// nodes. We use this to display the actual type when hovering over an "auto"
 /// keyword or "decltype()" expression.
 /// FIXME: This could have been a lot simpler by visiting AutoTypeLocs but it

 llvm::Optional<Hover> getHover(ParsedAST &AST, Position Pos) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   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);
+    return getHoverContents(Symbols.Macros[0], AST);
 
   if (!Symbols.Decls.empty())
     return getHoverContents(Symbols.Decls[0]);
 
   auto DeducedType = getDeducedType(AST, SourceLocationBeg);
   if (DeducedType && !DeducedType->isNull())
     return getHoverContents(*DeducedType, AST.getASTContext());
 

clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

OneTest Tests[] = {
       {
           R"cpp(// Macro
             #define MACRO 0
             #define MACRO 1
             int main() { return ^MACRO; }
             #define MACRO 2
             #undef macro
           )cpp",
-          "#define MACRO",
+          "#define MACRO 1",
+      },
+      {
+          R"cpp(// Macro
+            #define MACRO 0
+            #define MACRO2 ^MACRO
+          )cpp",
+          "#define MACRO 0",
+      },
+      {
+          R"cpp(// Macro
+            #define MACRO {\
+              return 0;\
+            }
+            int main() ^MACRO
+          )cpp",
+          R"cpp(#define MACRO {\
+              return 0;\
+            })cpp",
       },
       {
           R"cpp(// Forward class declaration
             class Foo;
             class Foo {};
             F^oo* foo();
           )cpp",
           "Declared in global namespace\n\nclass Foo {}",