[clang] [clang][Sema] Fix initialization of `NonTypeTemplateParmDecl`... (PR #121768)

[Alejandro Álvarez Ayllón via cfe-commits]

================
@@ -1228,35 +1228,45 @@ bool Sema::AttachTypeConstraint(AutoTypeLoc TL,
                                 NonTypeTemplateParmDecl *NewConstrainedParm,
                                 NonTypeTemplateParmDecl *OrigConstrainedParm,
                                 SourceLocation EllipsisLoc) {
-  if (NewConstrainedParm->getType().getNonPackExpansionType() != TL.getType() ||
-      TL.getAutoKeyword() != AutoTypeKeyword::Auto) {
-    Diag(NewConstrainedParm->getTypeSourceInfo()->getTypeLoc().getBeginLoc(),
-         diag::err_unsupported_placeholder_constraint)
-        << NewConstrainedParm->getTypeSourceInfo()
-               ->getTypeLoc()
-               .getSourceRange();
-    return true;
-  }
-  // FIXME: Concepts: This should be the type of the placeholder, but this is
-  // unclear in the wording right now.
-  DeclRefExpr *Ref =
-      BuildDeclRefExpr(OrigConstrainedParm, OrigConstrainedParm->getType(),
-                       VK_PRValue, OrigConstrainedParm->getLocation());
-  if (!Ref)
-    return true;
-  ExprResult ImmediatelyDeclaredConstraint = formImmediatelyDeclaredConstraint(
-      *this, TL.getNestedNameSpecifierLoc(), TL.getConceptNameInfo(),
-      TL.getNamedConcept(), /*FoundDecl=*/TL.getFoundDecl(), TL.getLAngleLoc(),
-      TL.getRAngleLoc(), BuildDecltypeType(Ref),
-      OrigConstrainedParm->getLocation(),
-      [&](TemplateArgumentListInfo &ConstraintArgs) {
-        for (unsigned I = 0, C = TL.getNumArgs(); I != C; ++I)
-          ConstraintArgs.addArgument(TL.getArgLoc(I));
-      },
-      EllipsisLoc);
+  ExprResult ImmediatelyDeclaredConstraint = [&] {
+    if (NewConstrainedParm->getType().getNonPackExpansionType() !=
+            TL.getType() ||
+        TL.getAutoKeyword() != AutoTypeKeyword::Auto) {
+      Diag(NewConstrainedParm->getTypeSourceInfo()->getTypeLoc().getBeginLoc(),
+           diag::err_unsupported_placeholder_constraint)
+          << NewConstrainedParm->getTypeSourceInfo()
+                 ->getTypeLoc()
+                 .getSourceRange();
+      return ExprResult();
+    }
+
+    // FIXME: Concepts: This should be the type of the placeholder, but this is
+    // unclear in the wording right now.
+    DeclRefExpr *Ref =
+        BuildDeclRefExpr(OrigConstrainedParm, OrigConstrainedParm->getType(),
+                         VK_PRValue, OrigConstrainedParm->getLocation());
+    assert(Ref != nullptr && "Unexpected nullptr!");
+
+    return formImmediatelyDeclaredConstraint(
+        *this, TL.getNestedNameSpecifierLoc(), TL.getConceptNameInfo(),
+        TL.getNamedConcept(), /*FoundDecl=*/TL.getFoundDecl(),
+        TL.getLAngleLoc(), TL.getRAngleLoc(), BuildDecltypeType(Ref),
+        OrigConstrainedParm->getLocation(),
+        [&](TemplateArgumentListInfo &ConstraintArgs) {
+          for (unsigned I = 0, C = TL.getNumArgs(); I != C; ++I)
+            ConstraintArgs.addArgument(TL.getArgLoc(I));
+        },
+        EllipsisLoc);
+  }();
+
   if (ImmediatelyDeclaredConstraint.isInvalid() ||
-      !ImmediatelyDeclaredConstraint.isUsable())
+      !ImmediatelyDeclaredConstraint.isUsable()) {
+    NewConstrainedParm->setPlaceholderTypeConstraint(
+        RecoveryExpr::Create(Context, OrigConstrainedParm->getType(),
----------------
alejandro-alvarez-sonarsource wrote:

The reasons for the `RecoveryExpr` is, indeed, like @erichkeane said, to be able to identify on the AST places where things were missing during parsing.

For this particular use case, the frontend (an analyzers) can easily tell apart 

1. There is no type constraint (`nullptr`)
2. There should be one but we could not parse it (`RecoveryExpr`)

The second point, IIUC [the documentation](https://clang.llvm.org/doxygen/classclang_1_1RecoveryExpr.html#details), is one of the reasons to use `RecoveryExpr`. Option 1 fixes the crash but loses information that can be used for better diagnosis.

> which was found during reduction of an unrelated bug

Not really, and I apologize if I have given this impression.
We have had few crashes and assertions when parsing `mp-units` with missing headers. This was not accidentally found because of #118288 (which is my other PR based on errors from this project).

>  I mostly say this because always initializing the placeholder pointer to null would have been a much simpler fix, which would potentially cover other instances of a similar bug.

I agree with you here, and the trailing object should probably be initialized to null on the constructor of `NonTypeTemplateParmDecl` to catch any other accidental uninitialized value. I am happy to do that too if no one objects.

https://github.com/llvm/llvm-project/pull/121768