[clang] Turn invented Exprs' source locations in __builtin_dump_struct to empty

[zyn0217]

This reflects the comment in #71366 (comment).

As that PR suggests, the invented CallExpr's source location previously pointed to the beginning of the __builtin_dump_struct. These spurious AST nodes confused clangd while displaying parameter inlay hints.

This patch takes another approach to address the same issue, by turning the location for each argument within an invented call to printf to empty, (maybe) at the risk of breaking the invariant for CallExpr -- The source location for an argument could be invalid from now on.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangd

Author: Younan Zhang (zyn0217)

Changes

This reflects the comment in #71366 (comment).

As that PR suggests, the invented CallExpr's source location previously pointed to the beginning of the __builtin_dump_struct. These spurious AST nodes confused clangd while displaying parameter inlay hints.

This patch takes another approach to address the same issue, by turning the location for each argument within an invented call to printf to empty, (maybe) at the risk of breaking the invariant for CallExpr -- The source location for an argument could be invalid from now on.

---

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

2 Files Affected:

• (modified) clang-tools-extra/clangd/unittests/InlayHintTests.cpp (+27)
• (modified) clang/lib/Sema/SemaChecking.cpp (+3-3)

diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 20c1cdd985dbc01..47af261fad850e8 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1724,6 +1724,33 @@ 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 = $one[[y]]; } // FIXME: Undesired `x = y: y` for this ill-formed expression.
+    };
+
+    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[ $two[[1]] ][ $three[[2]] ]; // `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{"y: ", "one"}, Code),
+                          HintMatcher(ExpectedHint{"Format: ", "Param"}, Code),
+                          HintMatcher(ExpectedHint{"y: ", "two"}, Code),
+                          HintMatcher(ExpectedHint{"z: ", "three"}, Code)));
+}
+
 TEST(ParameterHints, ArgPacksAndConstructors) {
   assertParameterHints(
       R"cpp(
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index ae588db02bbe722..5d348bd712dd104 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -454,13 +454,13 @@ namespace {
 struct BuiltinDumpStructGenerator {
   Sema &S;
   CallExpr *TheCall;
-  SourceLocation Loc = TheCall->getBeginLoc();
+  SourceLocation Loc;
   SmallVector<Expr *, 32> Actions;
   DiagnosticErrorTrap ErrorTracker;
   PrintingPolicy Policy;
 
   BuiltinDumpStructGenerator(Sema &S, CallExpr *TheCall)
-      : S(S), TheCall(TheCall), ErrorTracker(S.getDiagnostics()),
+      : S(S), TheCall(TheCall), Loc(), ErrorTracker(S.getDiagnostics()),
         Policy(S.Context.getPrintingPolicy()) {
     Policy.AnonymousTagLocations = false;
   }
@@ -491,7 +491,7 @@ struct BuiltinDumpStructGenerator {
     // Register a note to explain why we're performing the call.
     Sema::CodeSynthesisContext Ctx;
     Ctx.Kind = Sema::CodeSynthesisContext::BuildingBuiltinDumpStructCall;
-    Ctx.PointOfInstantiation = Loc;
+    Ctx.PointOfInstantiation = TheCall->getBeginLoc();
     Ctx.CallArgs = Args.data();
     Ctx.NumCallArgs = Args.size();
     S.pushCodeSynthesisContext(Ctx);

[HighCommander4]

The CI run shows the test Sema/builtin-dump-struct.c is failing.

[HighCommander4]

It looks like the issue is, if the invented CallExpr is ill-formed and the compiler needs to issue a diagnostic about it, with the invalid SourceLocation it doesn't have a source location to attach to the diagnostic.

I guess this means the approach of using an invalid SourceLocation doesn't work after all?

[zyn0217]

The CI run shows the test Sema/builtin-dump-struct.c is failing.

Oops, I didn't even realize there's such a test for C. I had only run the Sema/builtin-dump-struct.cpp locally and it passed. :(

I guess this means the approach of using an invalid SourceLocation doesn't work after all?

The answer seems no to me. At least we have to preserve these source locations at the Sema level to produce the diagnoses. As to the other approach involving "virtual buffers", I think that requires a more drastic change. I.e. moving the printf function generation to the tokenization level.

Closing this PR now and sorry for the churn!