Delay trailing constraing evaluation so we short-circuit

[erichkeane]

Summary

As reported as a part of PR47508 (and somewhat confirmed in PR44833),
our behavior to fully instantiate the constraint before checking it is
not compliant with the standard.

I found that by delaying this checking until after the function-decl is
created and we CheckInstantiatedFunctionTemplateConstraints, we can take
advantage of its ability to partially instantiate as we short-circuit.

[erichkeane]

The implementation seems to have tried checking this constraint 2x, and the first time required fully instantiating the constraint. The only condition in the check is PartialOrdering here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplateDeduction.cpp#L3654

I wasn't able to get that trigger in any case where constraints mattered, and we don't fail any tests here, so I'm at a loss as to in which way this would not work. I'm hoping to get feedback on the direction if it is wrong.

[erichkeane]

So debugging a DIFFERENT issue, I found that this is likely incomplete since we likely have a similar problem in VisitCXXMethodDecl below this. I can extend the tests here/fix easily enough, if we decide this is the right way around this.

[erichkeane]

Looking into my other issue, I think I'm reasonably convinced this is wrong. I was looking into the PR44833 blocker for 'ranges' which hits the VisitCXXRecordMethod part, and removing THAT breaks the constraint entirely, so I think the answer is a better way of checking these constraints without having to fully instantiate them, not delaying until later

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

Decl *TemplateDeclInstantiator::VisitFunctionDecl(
   NestedNameSpecifierLoc QualifierLoc = D->getQualifierLoc();
   if (QualifierLoc) {
     QualifierLoc = SemaRef.SubstNestedNameSpecifierLoc(QualifierLoc,
                                                        TemplateArgs);
     if (!QualifierLoc)
       return nullptr;
   }
 
-  // FIXME: Concepts: Do not substitute into constraint expressions
-  Expr *TrailingRequiresClause = D->getTrailingRequiresClause();
-  if (TrailingRequiresClause) {
-    EnterExpressionEvaluationContext ConstantEvaluated(
-        SemaRef, Sema::ExpressionEvaluationContext::Unevaluated);
-    ExprResult SubstRC = SemaRef.SubstExpr(TrailingRequiresClause,
-                                           TemplateArgs);
-    if (SubstRC.isInvalid())
-      return nullptr;
-    TrailingRequiresClause = SubstRC.get();
-    if (!SemaRef.CheckConstraintExpression(TrailingRequiresClause))
-      return nullptr;
-  }
-
   // If we're instantiating a local function declaration, put the result
   // in the enclosing namespace; otherwise we need to find the instantiated
   // context.
   DeclContext *DC;
   if (D->isLocalExternDecl()) {
     DC = Owner;
     SemaRef.adjustContextForLocalExternDecl(DC);
   } else if (isFriend && QualifierLoc) {
if (auto *DGuide = dyn_cast<CXXDeductionGuideDecl>(D)) {
     if (DGuide->isCopyDeductionCandidate())
       cast<CXXDeductionGuideDecl>(Function)->setIsCopyDeductionCandidate();
     Function->setAccess(D->getAccess());
   } else {
     Function = FunctionDecl::Create(
         SemaRef.Context, DC, D->getInnerLocStart(), NameInfo, T, TInfo,
         D->getCanonicalDecl()->getStorageClass(), D->UsesFPIntrin(),
         D->isInlineSpecified(), D->hasWrittenPrototype(), D->getConstexprKind(),
-        TrailingRequiresClause);
+        D->getTrailingRequiresClause());
     Function->setRangeEnd(D->getSourceRange().getEnd());
   }
 
   if (D->isInlined())
     Function->setImplicitlyInline();
 
   if (QualifierLoc)
     Function->setQualifierInfo(QualifierLoc);
Decl *TemplateDeclInstantiator::VisitFunctionDecl(
   Function->setLexicalDeclContext(LexicalDC);
 
   // Attach the parameters
   for (unsigned P = 0; P < Params.size(); ++P)
     if (Params[P])
       Params[P]->setOwningFunction(Function);
   Function->setParams(Params);
 
-  if (TrailingRequiresClause)
+  if (Expr *TrailingRequiresClause = D->getTrailingRequiresClause())
     Function->setTrailingRequiresClause(TrailingRequiresClause);
 
   if (TemplateParams) {
     // Our resulting instantiation is actually a function template, since we
     // are substituting only the outer template parameters. For example, given
     //
     //   template<typename T>
     //   struct X {

clang/test/SemaTemplate/trailing-return-short-circuit.cpp

+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template <class T>
+requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){}; // #FOO
+
+template <class T>
+void TrailingReturn(T) // #TRAILING
+requires(sizeof(T) > 2) || T::value // #TRAILING_REQ
+{};
+template<bool B>
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue<true>) <= 2);
+
+template<bool B>
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge<true>) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue<true>{});
+  TrailingReturn(HasValue<true>{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge<true>{});
+  TrailingReturn(HasValueLarge<true>{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue<false>{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue<false>]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue<false>) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue<false>::value' evaluated to false}}
+  TrailingReturn(HasValue<false>{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue<false>]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue<false>) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ{{and 'HasValue<false>::value' evaluated to false}}
+}