[clangd] Guard against trivial FunctionProtoTypeLoc when creating inlay hints

[HighCommander4]

Fixes #142608

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: Nathan Ridge (HighCommander4)

Changes

Fixes #142608

---

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

2 Files Affected:

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

diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 1b1bcf78c9855..9a401b31eeee9 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -368,6 +368,14 @@ static FunctionProtoTypeLoc getPrototypeLoc(Expr *Fn) {
   }
 
   if (auto F = Target.getAs<FunctionProtoTypeLoc>()) {
+    // In some edge cases the AST can contain a "trivial" FunctionProtoTypeLoc
+    // which has null parameters. Avoid these as they don't contain useful
+    // information.
+    for (const auto *Param : F.getParams()) {
+      if (!Param)
+        return {};
+    }
+
     return F;
   }
 
diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 77d78b8777fe3..8ed8401f9fce9 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -997,11 +997,16 @@ TEST(ParameterHints, FunctionPointer) {
     f3_t f3;
     using f4_t = void(__stdcall *)(int param);
     f4_t f4;
+    __attribute__((noreturn)) f4_t f5;
     void bar() {
       f1($f1[[42]]);
       f2($f2[[42]]);
       f3($f3[[42]]);
       f4($f4[[42]]);
+      // This one runs into an edge case in clang's type model
+      // and we can't extract the parameter name. But at least
+      // we shouldn't crash.
+      f5(42);
     }
   )cpp",
       ExpectedHint{"param: ", "f1"}, ExpectedHint{"param: ", "f2"},

[zyn0217]

LGTM as a workaround

Though I'm still wondering if there's an underlying bug in Parser

[hokein]

nit: We have a duplicated one in SemaCodeComplete.cpp, see the FIXME on line 337. I guess the same issue should happen there as well (signaturehelp?). I see options 1) duplicate the workaround 2) unify these two implementations. Either works for me.

[HighCommander4]

Thanks for the reminder about this. Now that we've upstreamed HeuristicResolver, it's a good place for a unified implementation of this sort of thing. I'll do that in #143240.

[HighCommander4]

Posted #143345 for this.

[hokein]

This smells like a clang bug — having a nullptr parameter shouldn’t happen. That said, I’m not objecting to the change here; it makes the tool more robust.

[HighCommander4]

My impression from having read some of the related code is that it's a known deficiency that having null parameters can sometimes happen.

TypeLoc has a method named initialize(), which is described as follows:

  /// Initializes this to state that every location in this
  /// type is the given location.
  ///
  /// This method exists to provide a simple transition for code that
  /// relies on location-less types.
  void initialize(ASTContext &Context, SourceLocation Loc) const {

The implementation of initialize for FunctionTypeLoc does set the parameters to null.

And there are various places in the code where we call initialize. In addition to the noreturn scenario exercised in this patch's testcase, which will be improved in #143143, another notable call site is ASTContext::getTrivialTypeSourceInfo. That in turn has many callers, including this one used to handle an invalid declarator.