[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:

Ok, I have followed a similar approach to #113182. There is no `RecoveryExpr` but `invalid` is set, which should work to easily identify something went mising.

Note that I had to patch AstWriterDecl and AstReaderDecl too: there is an extra boolean to flag the initialization of the underlying pointer.

I also changed

```cpp
  Expr *TypeConstraint = D->getPlaceholderTypeConstraint();
  Record.push_back(!!TypeConstraint);
```

to

```cpp
Record.push_back(D->hasPlaceholderTypeConstraint());
```

Which I think it is the intended meaning from looking at the reader

```cpp
    bool HasTypeConstraint = Record.readInt();
    D = NonTypeTemplateParmDecl::CreateDeserialized(Context, ID,
                                                    HasTypeConstraint);
    break;
```

Otherwise there will be no space for the pointer on the trailing object, but `hasPlaceholderTypeConstraint` would be returning `true` and that looks inconsistent to me, even if (I imagine) it is unlikely `setPlaceholderTypeConstraint` would be called on a deserialized `NonTypeTemplateParmDecl`

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