[clang][USR] Prevent crashes when parameter lists have nulls

[kadircet]

Summary

Similar to D146426

[kadircet]

I am aware that this null checking at leaves are not considered a sustainable solution and I agree with the sentiment there. But we're seeing an increasing number of crashes in production on invalid code recently. Happy to take a different course if there are alternatives, but as also explained in D146426, the situation around parameter lists having nullptrs seem to be the state for a long time now, e.g:

template <typename T> auto x = [](__fp16) {};
decltype(x<int>);

is a reproducer that crashes even clang-12 due to a nullptr in the paremeter list. Surely it'd be better to fix this invariant, but I am afraid we don't know how to do that immediately and considering people have been dealing with this situation by adding null checks into the places that triggered crashes ever since, I'd like to move forward with this fix until someone can figure out the situation.

[aaron.ballman]

In D146634#4215754, @kadircet wrote:

I am aware that this null checking at leaves are not considered a sustainable solution and I agree with the sentiment there. But we're seeing an increasing number of crashes in production on invalid code recently. Happy to take a different course if there are alternatives, but as also explained in D146426, the situation around parameter lists having nullptrs seem to be the state for a long time now, e.g:

template <typename T> auto x = [](__fp16) {};
decltype(x<int>);

is a reproducer that crashes even clang-12 due to a nullptr in the paremeter list. Surely it'd be better to fix this invariant, but I am afraid we don't know how to do that immediately and considering people have been dealing with this situation by adding null checks into the places that triggered crashes ever since, I'd like to move forward with this fix until someone can figure out the situation.

IMO, if the increasing number of crashes is a concern, now is the time to fix it properly instead of continuing to play whack-a-mole. To me, "fix it properly" means ensuring that we don't have null AST nodes, but if during investigation there's a more foundational problem that prevents us from doing that, we can make a different decision once we have more information.

[ilya-biryukov]

Hopefully this should be addressed by D146971, but it would be great to get a reproducer for this issue to be certain.

[kadircet]

• Add test

[kadircet]

in favor of D149733

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

TEST(SignatureHelp, TemplateArguments) {
   EXPECT_EQ(First.activeParameter, 0);
 
   auto Second = signatures(Top + "bool x = foo<1, ^");
   EXPECT_THAT(Second.signatures,
               ElementsAre(sig("foo<[[int I]], [[int]]>() -> bool")));
   EXPECT_EQ(Second.activeParameter, 1);
 }
 
+TEST(CompletionTest, NoCrashTests) {
+  llvm::StringLiteral Cases[] = {
+      R"cpp(
+    template <typename = int> struct Foo {};
+    auto a = [x(3)](Foo<^>){};
+    )cpp",
+  };
+  for (auto Case : Cases) {
+    SCOPED_TRACE(Case);
+    auto Completions = completions(Case);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang/lib/Index/USRGeneration.cpp

for (unsigned I = 0, N = SpecArgs->size(); I != N; ++I) {
       Out << '#';
       VisitTemplateArgument(SpecArgs->get(I));
     }
     Out << '>';
   }
 
   // Mangle in type information for the arguments.
   for (auto *PD : D->parameters()) {
+    // FIXME: Make sure we don't have nullptrs in parameter lists of function
+    // decls.
+    if (!PD) {
+      IgnoreResults = true;
+      return;
+    }
     Out << '#';
     VisitType(PD->getType());
   }
   if (D->isVariadic())
     Out << '.';
   if (IsTemplate) {
     // Function templates can be overloaded by return type, for example:
     // \code