This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add Macro Expansion to Hover
ClosedPublic

Authored by daiyousei-qz on Jun 5 2022, 9:12 PM.

Details

Summary

This patch adds macro expansion preview to hover info. Basically, the refactor infrastructure for expanding macro is used for this purpose. The following steps are added to getHoverContents for macros:

  1. calling AST.getTokens().expansionStartingAt(...) to get expanded tokens
  2. calling reformat(...) to format expanded tokens

Some opinions are wanted:

  1. Should we present macro expansion before definition in the hover card?
  2. Should we truncate/ignore macro expansion if it's too long? For performance and presentation reason, it might not be a good idea to expand pages worth of tokens in hover card. If so, what's the preferred threshold?

Also, some limitation applies:

  1. Expansion isn't available in macro definition/arguments as the refactor code action isn't either.

Diff Detail

Event Timeline

daiyousei-qz created this revision.Jun 5 2022, 9:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2022, 9:12 PM
daiyousei-qz requested review of this revision.Jun 5 2022, 9:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2022, 9:12 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
daiyousei-qz edited the summary of this revision. (Show Details)Jun 5 2022, 9:14 PM
daiyousei-qz retitled this revision from Add Macro Expansion to Hover to [clangd] Add Macro Expansion to Hover.Jun 5 2022, 10:03 PM

To elaborate on known situation where expansion isn't available:

  1. expansion doesn't work on site of #define
  2. expansion doesn't work for _Pragma(...) and alike
  3. expansion doesn't work with builtin macro
  4. expansion doesn't work for macros as arguments of function-like macro

Basically, anywhere the same code action isn't available. Need this be documented somewhere?

nridge added a comment.EditedJun 20 2022, 12:51 AM

Thank you for working on this! I've been using clangd with this patch applied for a week or so, and it seems to be working well, I haven't run into any issues.

Could you please upload a patch with context, to make it easier to review? (Currently, above each hunk it says "Context not available", which makes it harder to find what code is being changed.)

Should we present macro expansion before definition in the hover card?

Yes, I think putting the expansion first would make sense, because it's more relevant (since it's specific to the invocation).

Should we truncate/ignore macro expansion if it's too long? For performance and presentation reason, it might not be a good idea to expand pages worth of tokens in hover card. If so, what's the preferred threshold?

I think a good client will manage longer content by e.g. making the hover scrollable, so we probably don't need a very restrictive limit, but I guess we should have some limit. Maybe something like 100 lines?

Basically, anywhere the same code action isn't available. Need this be documented somewhere?

If you'd like, we could mention the ability to show macro expansions here, maybe with a screenshot, and then we could list the limitations in the same place.

The repository containing this website content is at https://github.com/llvm/clangd-www/blob/main/features.md

  • add context to patch
  • use a fixed size buffer for expansion text

Sorry I'm a little occupied lately.

Regarding to the order of Definition and MacroExpansion, I experimented putting expansion first and noticed the following:

  1. Short definition and short expansion: order doesn't matter, it's always clear
  2. Short definition and long expansion: putting definition first may have advantage in providing a general overview about the macro
  3. Long definition and long expansion: hover card isn't a great place to read even if it's scrollable. I'd rather jump to definition or use refactor feature to expand the macro in place :(

Therefore, I still put definition in front of the expansion in this update.

Regarding to the size limit of expansion:
Here I'm using a 2048 bytes buffer. If exceeded, no macro expansion will be displayed. It might be hard to limit size in term of lines because the expanded segment of code will be reformated.

use a rebased patch

Thanks for the update!

Therefore, I still put definition in front of the expansion in this update.

Regarding to the size limit of expansion:
Here I'm using a 2048 bytes buffer.

I think these are reasonable choices to start with. We can always tweak them after the fact if we'd like.

clang-tools-extra/clangd/Hover.cpp
676

nit: these two lines can be combined as:

if (auto Expansion = AST.getTokens().expansionStartingAt(&Tok)) {
   ...
}
1091

It would be interesting to have a couple of test cases that exercise the reformatting in non-trivial ways, e.g. long expansions that need to be wrapped onto multiple lines

I would suggest two such test cases, one with the expansion being in a declaration context, and the second an expression context (for this one, to make it long enough, the expansion could contain e.g. an a ? b : c expression)

(I'm suggesting the expression-context testcase in part as a sanity check to make sure that format::reformat() handles such code reasonably in the first place)

nridge added inline comments.Jul 4 2022, 11:26 PM
clang-tools-extra/clangd/Hover.cpp
1091

Phabricator's inter-diff seems to be broken, but glancing through the new revision, it seems like this comment about adding a couple of more test cases hasn't been addressed yet, are you planning to do that in another update?

daiyousei-qz marked an inline comment as done.
daiyousei-qz added a comment.EditedJul 5 2022, 5:32 PM

It looks like my inline comment wasn't submitted (didn't click the submit button on the bottom). Here's my old comment.

Basically, I think if tests are going to be added for the text format, it should be done on format::reformat instead of hover.

clang-tools-extra/clangd/Hover.cpp
1091

Somehow, this comment goes out of the position.

In my opinion, such test should be written against format::reformat() directly instead of hover message in clangd. One problem is that we are using the current style in users' workspace to reformat the definition/expansion, which means the same tokens might present differently given different .clang-format or fallback style that the user has specified. I do agree that, if tokens don't conform a regular c++ expression, like ) . field, the presentation could be bad. But I suppose there's no obvious solution for that.

I would like to apply the updated patch to try it some more, but arc patch is failing for me.

Applying the raw diff manually with patch also fails with the following errors:

$ patch -p1 < ../patches/macro-hover.patch
patching file clang-tools-extra/clangd/Hover.h
Hunk #1 FAILED at 71 (different line endings).
1 out of 1 hunk FAILED -- saving rejects to file clang-tools-extra/clangd/Hover.h.rej
patching file clang-tools-extra/clangd/Hover.cpp
Hunk #1 FAILED at 641 (different line endings).
Hunk #2 FAILED at 670 (different line endings).
Hunk #3 FAILED at 1004 (different line endings).
Hunk #4 FAILED at 1055 (different line endings).
Hunk #5 FAILED at 1175 (different line endings).
5 out of 5 hunks FAILED -- saving rejects to file clang-tools-extra/clangd/Hover.cpp.rej
patching file clang-tools-extra/clangd/unittests/HoverTests.cpp
Hunk #1 FAILED at 478 (different line endings).
Hunk #2 FAILED at 1070 (different line endings).
Hunk #3 FAILED at 1567 (different line endings).
Hunk #4 FAILED at 1577 (different line endings).
Hunk #5 FAILED at 1591 (different line endings).
Hunk #6 FAILED at 2625 (different line endings).
6 out of 6 hunks FAILED -- saving rejects to file clang-tools-extra/clangd/unittests/HoverTests.cpp.rej

I'm guessing related to Windows line endings?

I fixed it locally by running dos2unix on the patch before applying.

However, if you're able to, please update the patch to remove the Windows line endings so it can be applied with arc patch, otherwise the patch metadata is lost during manual application.

nridge added inline comments.Jul 10 2022, 9:03 PM
clang-tools-extra/clangd/Hover.cpp
1091

In my opinion, such test should be written against format::reformat() directly instead of hover message in clangd. One problem is that we are using the current style in users' workspace to reformat the definition/expansion, which means the same tokens might present differently given different .clang-format or fallback style that the user has specified. I do agree that, if tokens don't conform a regular c++ expression, like ) . field, the presentation could be bad. But I suppose there's no obvious solution for that.

Ok, fair enough. I did some manual testing of the updated patch and reformat() seems to have sane behaviour for expressions.

From my point of view, this patch is good to go (with the line endings fixed). However, I will defer approval to @sammccall in case he has additional feedback.

fix line end to LF

fix patch context

Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2022, 10:42 PM
Herald added subscribers: Restricted Project, Enna1. · View Herald Transcript

Sorry, didn't realize this was waiting on me! I like the feature and the threshold seems like a good idea. People might find it inconsistent, but we'll have to see.
I like placing it after the macro definition, I think the cases where expansion is/isn't shown feel more consistent that way.

My main hesitation is that it's nice if the members of HoverInfo are more generic: it ensures some coherence between different types of hovers and makes it easier to reason about changes to the Hoverinfo->markdown.

Concretely I wonder whether we can get away with appending this to the definition as a single block like:

#define DOUBLE(X) X##X

// Expands to:
mm
clang-tools-extra/clangd/Hover.cpp
679

nit: "Buffer" is redundant here

684

nit: the logic here seems a bit fiddly and micro-optimized - a few reallocations to produce a hover card don't matter.

consider just:

string ExpansionText;
for (tok : Expanded) {
  ExpansionText += ExpandedTok.text(SM);
  if (ExpansionText.size() > 2048) {
    ExpansionText.clear();
    break;
  }
}

(or even extract a function so you can early-return "", which is clearer)

1217

I think the ruler + paragraph header might appear too heavyweight, would need to take a look.

I think we're missing tests for present().
(If this can become part of the definition, then no need for new tests)

1218

I think "Expands to" or "Expanded text" is clearer than "macro expansion" - expansion names the process rather than the result.

clang-tools-extra/clangd/unittests/HoverTests.cpp
481–493

nit: these are known as "object-like" macros, I don't really know why

490

this is redundant with the definition, ideally I think we'd omit one (the output is pretty heavyweight).

It's *almost* correct to omit the expansion for object-like macros, the problem with this is:

#define INDIRECT DOUBLE(y)
#define DOUBLE(X) X##X
int INDIRECT; // we *should* show expansion here

Still, this is probably rare enough that it's better to not show expansions for object-like macros on balance?

(It would be possible to actually check for nested expansions but I doubt it's worth the complexity, certainly not in this patch)

resolve review comment

produce expansion even if definition isn't available

daiyousei-qz marked 5 inline comments as done.Jul 19 2022, 10:18 PM

I have also moved the expansion out of the location checking branch. I think it's to find out if a macro actually has a definition which isn't always the case especially for special macros like __FILE__.

Here's the current presentation.

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

I suppose if we don't have good way to detect nested object-like macro, just leave both definition and expansion is a better idea since people could simply ignore the redundant part. Though I don't have a strong opinion on this and could change that if you really want.

sammccall added inline comments.Jul 19 2022, 11:20 PM
clang-tools-extra/clangd/Hover.cpp
1193

Sorry, I think I wasn't clear about what I was asking for...
Rather than merging HoverInfo::MacroExpansion and HoverInfo::Definition during present(), I'd really like to eliminate HoverInfo::MacroExpansion entirely, and have HoverInfo::Definition be:

#define FOO BAR

// Expands to
BAR

This isn't completely consistent with the way scope/access are handled, but those are somewhat more generic features and I'd rather avoid adding new HoverInfo members specific to macros.

(The final presentation does look good, so this is just about the intermediate HoverInfo struct)

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

OK, I also don't feel strongly, so let's go with what you have now.

daiyousei-qz added inline comments.Jul 19 2022, 11:58 PM
clang-tools-extra/clangd/Hover.cpp
1193

I see. Though I'm not sure if putting both of them in a single string will cause problems when doing the format. That is:

#define MACRO token token token token token

// Expands to
token token token token token token token token token

as a whole will be reformatted by clang-format and I'm not sure if the overall layout could always be preserved. Let me do some expriment tomorrow.

remove dedicated MacroExpansion variable (reuse Definition)
added empty MACRO test

@sammccall Hello, is there any update required by me?

Sorry for the delay :-( This looks great!

It looks like this is your first patch, so you don't have commit access. I can land the patch for you.
Can you provide an email address for attribution?

Sorry for the delay :-( This looks great!

It looks like this is your first patch, so you don't have commit access. I can land the patch for you.
Can you provide an email address for attribution?

I had a patch for clang which Nathan had merged for me. Probably it's because of the lack of the metadata since I was using the web interface to submit differentials. Anyway, here's the info:
Qingyuan Zheng qyzheng2@outlook.com

This revision was not accepted when it landed; it landed in state Needs Review.Sep 7 2022, 8:50 AM
This revision was automatically updated to reflect the committed changes.

I take a quick look and it seems the failing tests were timed out. Considering the context of the change, I would guess it might be caused by some huge, nested macro being expanded. But it's weird since the expansion should only happen for Hover. Unfortunately, I don't have any aarch64 device to reproduce this locally and investigate further. Please feel free to reach out to me if there's anything I could help.

897.65s: Clangd Unit Tests :: ./ClangdTests/25/72

I take a quick look and it seems the failing tests were timed out. Considering the context of the change, I would guess it might be caused by some huge, nested macro being expanded. But it's weird since the expansion should only happen for Hover. Unfortunately, I don't have any aarch64 device to reproduce this locally and investigate further. Please feel free to reach out to me if there's anything I could help.

Actually it could be 72142fbac496a66769e16c80b076216d7c449ab2 as well
@sammccall we may need to disable or revert broken tests

After the patch:
Slowest Tests:
--------------------------------------------------------------------------
897.65s: Clangd Unit Tests :: ./ClangdTests/25/72

Before the patch 
Slowest Tests:
--------------------------------------------------------------------------
157.84s: Clangd Unit Tests :: ./ClangdTests/DefineOutlineTest/QualifyReturnValue

@daiyousei-qz what is the current status of this patch? Is it ready to be merged again? (If so, I can do that for you.)

@daiyousei-qz what is the current status of this patch? Is it ready to be merged again? (If so, I can do that for you.)

Honestly, I don't know. I looked at TOT llvm repo in github and it looks like this patch is already there. I guess no further action is needed?

nridge added a comment.Feb 2 2023, 9:44 PM

@daiyousei-qz what is the current status of this patch? Is it ready to be merged again? (If so, I can do that for you.)

Honestly, I don't know. I looked at TOT llvm repo in github and it looks like this patch is already there. I guess no further action is needed?

Ah, my bad, for some reason I thought this had been backed out. Never mind then, and thanks for this improvement!