Page MenuHomePhabricator

[clangd] Enhance macro hover to see full definition
ClosedPublic

Authored by malaperle on Dec 3 2018, 9:50 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

malaperle created this revision.Dec 3 2018, 9:50 PM
malaperle updated this revision to Diff 176754.Dec 4 2018, 6:45 PM

Clang-format

hokein added inline comments.Dec 5 2018, 12:20 AM
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 added inline comments.Dec 5 2018, 7:27 AM
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 added inline comments.Dec 5 2018, 10:40 AM
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 marked 2 inline comments as done.Jan 12 2019, 11:07 AM
malaperle added inline comments.
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 added inline comments.Jan 14 2019, 3:26 AM
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 added inline comments.Jan 14 2019, 6:02 AM
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 added inline comments.Jan 14 2019, 6:28 AM
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 updated this revision to Diff 184928.Feb 2 2019, 9:16 PM
malaperle marked 2 inline comments as done.

Address comments.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2019, 9:16 PM
malaperle updated this revision to Diff 185959.Feb 8 2019, 5:54 AM
malaperle marked an inline comment as not done.

Remove temporary code.

ilya-biryukov added inline comments.Feb 19 2019, 4:40 AM
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 updated this revision to Diff 187783.Feb 21 2019, 6:38 AM

Address comments.

malaperle marked an inline comment as done.Feb 21 2019, 6:40 AM
malaperle added inline comments.
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 added inline comments.Feb 21 2019, 7:20 AM
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 marked an inline comment as done.Feb 21 2019, 10:06 PM
malaperle added inline comments.
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?).

Remove Markdown support.

ilya-biryukov accepted this revision.Feb 22 2019, 1:06 AM

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.

This revision is now accepted and ready to land.Feb 22 2019, 1:06 AM

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

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2019, 3:49 PM