[clang] fix concepts crash on substitution failure during normalization

[mizvekov]

Summary

When substitution failed on the first constrained template argument (but
only the first), we would assert / crash. Checking for failure was only
being performed from the second constraint on.

This changes it so the checking is performed in that case,
and the code is also now simplified a little bit to hopefully
avoid this confusion.

Signed-off-by: Matheus Izvekov <[e-mail]>

[mizvekov]

This partially fixes https://bugs.llvm.org/show_bug.cgi?id=47174

It fixes the crash, but there is however a remaining issue, the bogus template lookup (no template named 'Y') diagnostic from @Quuxplusone's reduced test case is still there.

So far as I have seen though, it is only a diagnostic issue.
I can't find an alternate test case with valid code (non ambiguous overload resolution or otherwise) that triggers that kind of error.

[rsmith]

clang/lib/Sema/SemaConcept.cpp

744

Might be useful to keep this assert; it will make it clearer to future readers that this function isn't intended to handle this case.

754

Doesn't this have a use-after-destruction bug? I'd expect emplace to first destroy its contained value and then construct a new one; in this case the second constructor argument looks like it'll be a reference to a destroyed object. I assume that's why the old code did the two-step construct and assign dance.

[mizvekov]

clang/lib/Sema/SemaConcept.cpp

744

Yeah you are right, the assert makes it obvious this is intended.

754

The constructor used here takes both constraints by value:

NormalizedConstraint(ASTContext &C, NormalizedConstraint LHS,
                     NormalizedConstraint RHS, CompoundConstraintKind Kind)
    : Constraint{CompoundConstraint{
          new (C) std::pair<NormalizedConstraint, NormalizedConstraint>{
              std::move(LHS), std::move(RHS)}, Kind}} { };

[mizvekov]

clang/lib/Sema/SemaConcept.cpp

754

But I will also check if emplace is doing the right thing here or not.

[mizvekov]

• Restore assert.
• Fix incorrekt use of emplace.

clang/lib/Sema/SemaConcept.cpp

static bool substituteParameterMappings(Sema &S, NormalizedConstraint &N,
   std::copy(SubstArgs.arguments().begin(), SubstArgs.arguments().end(),
             N.getAtomicConstraint()->ParameterMapping->begin());
   return false;
 }
 
 Optional<NormalizedConstraint>
 NormalizedConstraint::fromConstraintExprs(Sema &S, NamedDecl *D,
                                           ArrayRef<const Expr *> E) {
   assert(E.size() != 0);
-  auto First = fromConstraintExpr(S, D, E[0]);
-  if (E.size() == 1)
-    return First;
-  auto Second = fromConstraintExpr(S, D, E[1]);
-  if (!Second)
+  auto Conjunction = fromConstraintExpr(S, D, E[0]);
+  if (!Conjunction)
     return None;
-  llvm::Optional<NormalizedConstraint> Conjunction;
-  Conjunction.emplace(S.Context, std::move(*First), std::move(*Second),
-                      CCK_Conjunction);
-  for (unsigned I = 2; I < E.size(); ++I) {
+  for (unsigned I = 1; I < E.size(); ++I) {
     auto Next = fromConstraintExpr(S, D, E[I]);
     if (!Next)
-      return llvm::Optional<NormalizedConstraint>{};
-    NormalizedConstraint NewConjunction(S.Context, std::move(*Conjunction),
+      return None;
+    *Conjunction = NormalizedConstraint(S.Context, std::move(*Conjunction),
                                         std::move(*Next), CCK_Conjunction);
-    *Conjunction = std::move(NewConjunction);
   }
   return Conjunction;
 }
 
 llvm::Optional<NormalizedConstraint>
 NormalizedConstraint::fromConstraintExpr(Sema &S, NamedDecl *D, const Expr *E) {
   assert(E != nullptr);
 
   // C++ [temp.constr.normal]p1.1

clang/test/CXX/temp/temp.constr/temp.constr.normal/p1.cpp

namespace non_type_pack {
   template<int T>
   constexpr void foo() requires C2<T, 2, T> { }
 
   template<int T>
   constexpr void foo() requires C1<T, 2, T> && true { }
 
   static_assert((foo<1>(), true));
 }
+
+namespace PR47174 {
+// This checks that we don't crash with a failed substitution on the first constrained argument when
+// performing normalization.
+template <Bar2 T, True U>
+requires true struct S3; // expected-note {{template is declared here}}
+template <True T, True U>
+requires true struct S3<T, U>; // expected-error {{class template partial specialization is not more specialized than the primary template}}
+
+// Same as above, for the second position (but this was already working).
+template <True T, Bar2 U>
+requires true struct S4; // expected-note {{template is declared here}}
+template <True T, True U>
+requires true struct S4<T, U>; // expected-error {{class template partial specialization is not more specialized than the primary template}}
+} // namespace PR47174