[clangd] Hide inlay hints when using a macro as a calling argument that with a param comment

[zyounan]

Summary

We don't want to produce inlay hints for arguments for which
user has left param name comments. But we're not decomposing
location of the parameter correctly at the moment because the
location we've passed into SM.getDecomposedLoc is not always
FileID.

Fixes clangd/clangd#1495

[zyounan]

Note that we still have some issues on inlay hints with macro expansion even after D133982.

void func(int a, int b);
#define CALL(a, b) func(int(a), b)

CALL(3, 4);  // CALL(3, b: 4)

This is because spelledForExpanded doesn't return counterpart spelling ( int(3) -> 3 ). (I guess it is due to patch D134618.)

void func(int a, int b);
void work(int a, int &b);
#define CALL(a, b) { func(int(a), b); work(a, b); }

int z = 42;
CALL(3, z);  // CALL(a: 3, &b:b: 4)

This is because call expression handling logic (i.e. InlayHintVisitor::processCall) doesn't know the macro expansion context of expression, resulting producing two inlay hints at the same spelled position.

[zyounan]

clang-tools-extra/clangd/InlayHints.cpp

521

stupid question: I was suspicious with this check that when would Decomposed.first not being the same as MainFileID? Should the "top-caller" of the macro always be in main file? I didn't find a case that differs, except when getDecomposedLoc provides wrong FileID.

[zyounan]

Ping

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

519–520

I admit I struggle with this spelling/expansion loc stuff. But having spent a bit of time reading the implementations of these functions, would I be correct to say that another, perhaps easier to follow, way to express the same thing would be:

auto Decomposed = SM.getDecomposedLoc(SM.getFileLoc(E->getBeginLoc()));

?

521

I tried adding an assertion in this early-return branch, and it tripped in ParameterHints.IncludeAtNonGlobalScope.

The code there is:

Annotations FooInc(R"cpp(
  void bar() { foo(42); }
)cpp");
Annotations FooCC(R"cpp(
  struct S {
    void foo(int param);
    #include "foo.inc"
  };
)cpp");

so I guess the decomposed loc there is a file loc, but not in the main file (but rather in foo.inc).

[zyounan]

Address the comments

clang-tools-extra/clangd/InlayHints.cpp

519–520

Yes, indeed. SM.getFileLoc performs exactly the same function as getTopMacroCallerLoc and getDecomposedExpansionLoc:
If Loc is a FileID, just return Loc.
In another case it falls into a loop until the result is a FileID: First obtain the "top macro caller" location, then the beginning expansion location.

521

Ah, I see. Thanks!

[zyounan]

Update

[nridge]

Thanks!

clang-tools-extra/clangd/InlayHints.cpp

return Type->isLValueReferenceType() &&
            !isExpandedFromParameterPack(ForwardedParam);
   }
 
   // Checks if "E" is spelled in the main file and preceded by a C-style comment
   // whose contents match ParamName (allowing for whitespace and an optional "="
   // at the end.
   bool isPrecededByParamNameComment(const Expr *E, StringRef ParamName) {
     auto &SM = AST.getSourceManager();
-    auto ExprStartLoc = SM.getTopMacroCallerLoc(E->getBeginLoc());
-    auto Decomposed = SM.getDecomposedLoc(ExprStartLoc);
+    auto FileLoc = SM.getFileLoc(E->getBeginLoc());
+    auto Decomposed = SM.getDecomposedLoc(FileLoc);
     if (Decomposed.first != MainFileID)
       return false;
 
     StringRef SourcePrefix = MainFileBuf.substr(0, Decomposed.second);
     // Allow whitespace between comment and expression.
     SourcePrefix = SourcePrefix.rtrim();
     // Check for comment ending.
     if (!SourcePrefix.consume_back("*/"))
       return false;

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

 TEST(ParameterHints, ParamNameComment) {
   // Do not hint an argument which already has a comment
   // with the parameter name preceding it.
   assertParameterHints(R"cpp(
     void foo(int param);
     void bar() {
       foo(/*param*/42);
       foo( /* param = */ 42);
+#define X 42
+#define Y X
+#define Z(...) Y
+      foo(/*param=*/Z(a));
+      foo($macro[[Z(a)]]);
       foo(/* the answer */$param[[42]]);
     }
   )cpp",
+                       ExpectedHint{"param: ", "macro"},
                        ExpectedHint{"param: ", "param"});
 }
 
 TEST(ParameterHints, SetterFunctions) {
   assertParameterHints(R"cpp(
     struct S {
       void setParent(S* parent);
       void set_parent(S* parent);