[clangd] Carefully handle PseudoObjectExprs for inlay hints

[zyn0217]

This closes clangd/clangd#1813.

Not all subexpressions for a PseudoObjectExpr are interesting, and they probably mess up inlay hints.

[llvmbot]

@llvm/pr-subscribers-clangd

Author: Younan Zhang (zyn0217)

Changes

This closes clangd/clangd#1813.

PseudoObjectExprs in C++ are currently not very interesting but probably mess up inlay hints.

---

Full diff: https://github.com/llvm/llvm-project/pull/71366.diff

2 Files Affected:

• (modified) clang-tools-extra/clangd/InlayHints.cpp (+13)
• (modified) clang-tools-extra/clangd/unittests/InlayHintTests.cpp (+24)

diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 3da047f95421385..867c70e5dbc80c6 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -589,6 +589,19 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
     return true;
   }
 
+  bool dataTraverseStmtPre(Stmt *S) {
+    // Do not show inlay hints for PseudoObjectExprs. They're never
+    // genuine user codes in C++.
+    //
+    // For example, __builtin_dump_struct would expand to a PseudoObjectExpr
+    // that includes a couple of calls to a printf function. Printing parameter
+    // names for that anyway would end up with duplicate parameter names (which,
+    // however, got de-duplicated after visiting) for the printf function.
+    if (AST.getLangOpts().CPlusPlus && isa<PseudoObjectExpr>(S))
+      return false;
+    return true;
+  }
+
   bool VisitCallExpr(CallExpr *E) {
     if (!Cfg.InlayHints.Parameters)
       return true;
diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 20c1cdd985dbc01..2b6c27b17b537a9 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1724,6 +1724,30 @@ TEST(InlayHints, RestrictRange) {
               ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, PseudoObjectExpr) {
+  Annotations Code(R"cpp(
+    struct S {
+      __declspec(property(get=GetX, put=PutX)) int x[];
+      int GetX(int y, int z) { return 42 + y; }
+      void PutX(int y) { x = y; } // Not `x = y: y`
+    };
+
+    int printf(const char *Format, ...);
+
+    int main() {
+      S s;
+      __builtin_dump_struct(&s, printf); // Not `Format: __builtin_dump_struct()`
+      printf($Param[["Hello, %d"]], 42); // Normal calls are not affected.
+      return s.x[1][2]; // Not `x[y: 1][z: 2]`
+    }
+  )cpp");
+  auto TU = TestTU::withCode(Code.code());
+  TU.ExtraArgs.push_back("-fms-extensions");
+  auto AST = TU.build();
+  EXPECT_THAT(inlayHints(AST, std::nullopt),
+              ElementsAre(HintMatcher(ExpectedHint{"Format: ", "Param"}, Code)));
+}
+
 TEST(ParameterHints, ArgPacksAndConstructors) {
   assertParameterHints(
       R"cpp(

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[zyn0217]

On second thought I think I should not kill all the expressions involving PseudoObjectExpr: I'm not sure if our ObjC / CUDA support does need that for presenting useful hints.

[HighCommander4]

Is it not possible to tell based on the CallExpr only (rather than checking an ancestor of it during traversal of the tree) that "this CallExpr is not explicitly written in the source"?

[zyn0217]

Is it not possible to tell based on the CallExpr only (rather than checking an ancestor of it during traversal of the tree) that "this CallExpr is not explicitly written in the source"?

I'm not quite sure - Looking into the construction process for a __builtin_dump_struct expression,

llvm-project/clang/lib/Sema/SemaChecking.cpp

Lines 482 to 509 in 4d5a8cc

	bool callPrintFunction(llvm::StringRef Format,
	llvm::ArrayRef<Expr *> Exprs = {}) {
	SmallVector<Expr *, 8> Args;
	assert(TheCall->getNumArgs() >= 2);
	Args.reserve((TheCall->getNumArgs() - 2) + /*Format*/ 1 + Exprs.size());
	Args.assign(TheCall->arg_begin() + 2, TheCall->arg_end());
	Args.push_back(getStringLiteral(Format));
	Args.insert(Args.end(), Exprs.begin(), Exprs.end());
	// Register a note to explain why we're performing the call.
	Sema::CodeSynthesisContext Ctx;
	Ctx.Kind = Sema::CodeSynthesisContext::BuildingBuiltinDumpStructCall;
	Ctx.PointOfInstantiation = Loc;
	Ctx.CallArgs = Args.data();
	Ctx.NumCallArgs = Args.size();
	S.pushCodeSynthesisContext(Ctx);
	ExprResult RealCall =
	S.BuildCallExpr(/*Scope=*/nullptr, TheCall->getArg(1),
	TheCall->getBeginLoc(), Args, TheCall->getRParenLoc());
	S.popCodeSynthesisContext();
	if (!RealCall.isInvalid())
	Actions.push_back(RealCall.get());
	// Bail out if we've hit any errors, even if we managed to build the
	// call. We don't want to produce more than one error.
	return RealCall.isInvalid() || ErrorTracker.hasErrorOccurred();
	}

the part that makes the call unordinary might be the nullish scope. I don't know if that implies "the expression is not written by users"; even so, extracting the scope of a call expression then performing the check doesn't look much more straightforward than this way.

[HighCommander4]

I see; it's a bit unfortunate that such an "invented" call expression gets an ordinary SourceLocation. I would have hoped it gets a source location in some sort of virtual buffer like the rewritten form of >>, or macro expansions.

---

What do you think about the following idea: override the traversal of PseudoObjectExpr, but rather than traversing both the getSyntacticForm() and the semantic expressions, only traverse the getSyntacticForm()?

That leave the door open to a future enhancement where the arguments in the syntactic form (the __builtin_dump_struct) could get parameter hints, which might be useful for a reader to understand what the builtin does.

[HighCommander4]

I see; it's a bit unfortunate that such an "invented" call expression gets an ordinary SourceLocation.

Out of curiosity, I dialled into today's LLVM office hours and asked Aaron Ballman about this. He also did not see an obvious way to tell apart the invented CallExprs from regular CallExprs, but suggested that it could be reasonable to just change the code for building the invented CallExprs to give them an invalid SourceLocation (and then our inlay hints code should filter them out without further changes).

[zyn0217]

Out of curiosity, I dialled into today's LLVM office hours and asked Aaron Ballman about this

Thank you so much for making the effort on this issue. :)

...just change the code for building the invented CallExprs to give them an invalid SourceLocation

Interesting idea and I'll explore it.

[zyn0217]

Since the other PR turned out to be infeasible, I'm overriding TraversePseudoObjectExpr now.

[zyn0217]

Sorry for putting this off; I've updated it and please take another look.
I also changed the PR title since I feel this is not particular to __builtin_dump_struct.

[HighCommander4]

LGTM, thank you!