[clang] Properly parse variable template requires clause in lambda

[rymiel]

Summary

Since P0857, part of C++20, a *lambda-expression* can contain a
*requires-clause* after its *template-parameter-list*.

While support for this was added as part of
eccc734a69c0c012ae3160887b65a535b35ead3e, one specific case isn't
handled properly, where the *requires-clause* consists of an
instantiation of a boolean variable template. This is due to a
diagnostic check which was written with the assumption that a
*requires-clause* can never be followed by a left parenthesis. This
assumption no longer holds for lambdas.

This diagnostic check would then attempt to perform a "recovery", but it
does so in a valid parse state, resulting in an invalid parse state
instead!

This patch adds a special case when parsing requires clauses of lambda
templates, to skip this diagnostic check.

Fixes https://github.com/llvm/llvm-project/issues/61278
Fixes https://github.com/llvm/llvm-project/issues/61387

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

116

I'd like to expand on the comment above in this case. Also, since we don't decide that this is a trailing requires clause in the lambda parsing, we should probably make this more specific in this condition. I THINK we still want to do the bin-op precedence condition in this case, right?

[rymiel]

clang/lib/Sema/SemaConcept.cpp

116

I'd like to expand on the comment above in this case.

Yes, that's a very good call, doing that now.

Also, since we don't decide that this is a trailing requires clause in the lambda parsing, we should probably make this more specific in this condition.

I'm not 100% sure what you mean here...

I THINK we still want to do the bin-op precedence condition in this case, right?

I think it's still being done, but it's not very clear from the mess of a logic expression

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

116

So my concern is that this is a 'top level' condition here, rather than being 'more specific', but you're right, this is a little confusing logic, and I'm afraid I perhaps got confused as well. So here is the logic as it sits in your patch.

(
  NextToken.is(tok::l_paren) 
  && 
  !IsLambdaRequiresClause 
  &&
  (
    IsTrailingRequiresClause
    ||
    (
       Type->isDependentType() //#1
       &&
       isa<UnresolvedLookupExpr>(ConstraintExpression) 
    ) 
    ||
    Type->isFunctionType()
    ||
    Type->isSpecificBuiltinType(BuiltinType::Overload)
  )
) 
||
getBinOpPrecedence(NextToken.getKind(), /*GreaterThanIsOperator=*/true, getLangOpts().CPlusPlus11) > prec::LogicalAnd;

I suspect we don't want to have this skip the getBinOpPrecedence, which I think you're right, works correctly. I'm a bit concerned about your patch skippinjg the isFunctionType and isSpecificBuiltinType branches.

The one in your reproducer hits only the isDependentType() && isa<ULE>(ConstraintExpr), correct? So unless you have a more specific example where it should also apply when the type is a function/overload set, I suspect the !IsLambdaRequiresClause would be best placed in with the ULE check (~#1).

Does that make sense?

[rymiel]

Slightly rewrite CheckForNonPrimary for slightly better clarity

[rymiel]

clang/lib/Sema/SemaConcept.cpp

116

Yes, that makes a lot of sense, thank you for pointing that out!

[erichkeane]

clang/lib/Parse/ParseExprCXX.cpp

1283

Ok, last bit, I promise :) I see that we set up the lambda scope here and push the lambda scope on 1287. Instead of passing a bool, could we just figure out the IsLambdaRequiresClause based on that instead? See definition of PushLambdaScope here: https://clang.llvm.org/doxygen/Sema_8cpp_source.html#l02141

A test to see if that would work would be the one you have below, PLUS an example of a requires clause INSIDE of a lambda that itself isn't a lambda that would reproduce the warning(though I'm not convinced ATM that is possible, I don't think we allow a template inside block scope, unless there is some trick I'm too uncreative enough to come up with). If none exists, just checking the current scope would work and perhaps be more preferential.

We do this rarely, but we do it at least in SemaStmt.cpp in ActOnCapScopeReturnStmt. Just a dyn_cast<LambdaScopeInfo>(getCurFunction()) might be able to replace the bool all over the place.

[rymiel]

utilize getCurFunction()

[rymiel]

clang/lib/Parse/ParseExprCXX.cpp

1283

Thank you, I knew there was probably some much cleaner way to do this that I simply had no clue about due to my unfamiliarity. I've gone ahead and used dyn_cast<LambdaScopeInfo>(getCurFunction())

[erichkeane]

Thank you! This looks good to me. I presume you don't have commit access, so if you give me your "SomeName <EmailAddress>", I can commit this for you.

[erichkeane]

Ah, also, before commit, ensure that pre-commit CI passes.

[rymiel]

Use dyn_cast_if_present, otherwise we segfault in some tests

[rymiel]

In D146140#4196886, @erichkeane wrote:

Ah, also, before commit, ensure that pre-commit CI passes.

Yep, found an issue already ;;

In D146140#4196866, @erichkeane wrote:

I presume you don't have commit access

I do! I just haven't done anything in this part of the monorepo, which is why i'm so lost.

[erichkeane]

In D146140#4196919, @rymiel wrote:

In D146140#4196886, @erichkeane wrote:

Ah, also, before commit, ensure that pre-commit CI passes.

Yep, found an issue already ;;

In D146140#4196866, @erichkeane wrote:

I presume you don't have commit access

I do! I just haven't done anything in this part of the monorepo, which is why i'm so lost.

Great, even better :)

[cor3ntin]

Do we need a release note for this?

[erichkeane]

In D146140#4201481, @cor3ntin wrote:

Do we need a release note for this?

Yes, good catch! @rymiel : can you please add one?

[rymiel]

Add release note

[erichkeane]

Still LGTM.

clang/include/clang/Parse/Parser.h

public:
 
   ExprResult ParseExpression(TypeCastState isTypeCast = NotTypeCast);
   ExprResult ParseConstantExpressionInExprEvalContext(
       TypeCastState isTypeCast = NotTypeCast);
   ExprResult ParseConstantExpression();
   ExprResult ParseCaseExpression(SourceLocation CaseLoc);
   ExprResult ParseConstraintExpression();
   ExprResult
-  ParseConstraintLogicalAndExpression(bool IsTrailingRequiresClause);
-  ExprResult ParseConstraintLogicalOrExpression(bool IsTrailingRequiresClause);
+  ParseConstraintLogicalAndExpression(bool IsTrailingRequiresClause,
+                                      bool IsLambdaRequiresClause = false);
+  ExprResult
+  ParseConstraintLogicalOrExpression(bool IsTrailingRequiresClause,
+                                     bool IsLambdaRequiresClause = false);
   // Expr that doesn't include commas.
   ExprResult ParseAssignmentExpression(TypeCastState isTypeCast = NotTypeCast);
 
   ExprResult ParseMSAsmIdentifier(llvm::SmallVectorImpl<Token> &LineToks,
                                   unsigned &NumLineToksConsumed,
                                   bool IsUnevaluated);
 
   ExprResult ParseStringLiteralExpression(bool AllowUserDefinedLiteral = false);

clang/include/clang/Sema/Sema.h

ExprResult BuildBlockForLambdaConversion(SourceLocation CurrentLocation,
                                            Expr *Src);
 
   /// Check whether the given expression is a valid constraint expression.
   /// A diagnostic is emitted if it is not, false is returned, and
   /// PossibleNonPrimary will be set to true if the failure might be due to a
   /// non-primary expression being used as an atomic constraint.
   bool CheckConstraintExpression(const Expr *CE, Token NextToken = Token(),
                                  bool *PossibleNonPrimary = nullptr,
-                                 bool IsTrailingRequiresClause = false);
+                                 bool IsTrailingRequiresClause = false,
+                                 bool IsLambdaRequiresClause = false);
 
 private:
   /// Caches pairs of template-like decls whose associated constraints were
   /// checked for subsumption and whether or not the first's constraints did in
   /// fact subsume the second's.
   llvm::DenseMap<std::pair<NamedDecl *, NamedDecl *>, bool> SubsumptionCache;
   /// Caches the normalized associated constraints of declarations (concepts or
   /// constrained declarations). If an error occurred while normalizing the

clang/lib/Parse/ParseExpr.cpp

 /// \verbatim
 ///       C++2a[temp.constr.decl]p1
 ///       constraint-logical-and-expression:
 ///         primary-expression
 ///         constraint-logical-and-expression '&&' primary-expression
 ///
 /// \endverbatim
 ExprResult
-Parser::ParseConstraintLogicalAndExpression(bool IsTrailingRequiresClause) {
+Parser::ParseConstraintLogicalAndExpression(bool IsTrailingRequiresClause,
+                                            bool IsLambdaRequiresClause) {
   EnterExpressionEvaluationContext ConstantEvaluated(
       Actions, Sema::ExpressionEvaluationContext::Unevaluated);
   bool NotPrimaryExpression = false;
   auto ParsePrimary = [&] () {
     ExprResult E = ParseCastExpression(PrimaryExprOnly,
                                        /*isAddressOfOperand=*/false,
                                        /*isTypeCast=*/NotTypeCast,
                                        /*isVectorLiteral=*/false,
if (NotPrimaryExpression ||
         Tok.isOneOf(tok::period, tok::plusplus, tok::minusminus) ||
         (Tok.is(tok::l_square) && !NextToken().is(tok::l_square))) {
       E = RecoverFromNonPrimary(E, /*Note=*/false);
       if (E.isInvalid())
         return ExprError();
       NotPrimaryExpression = false;
     }
     bool PossibleNonPrimary;
-    bool IsConstraintExpr =
-        Actions.CheckConstraintExpression(E.get(), Tok, &PossibleNonPrimary,
-                                          IsTrailingRequiresClause);
+    bool IsConstraintExpr = Actions.CheckConstraintExpression(
+        E.get(), Tok, &PossibleNonPrimary, IsTrailingRequiresClause,
+        IsLambdaRequiresClause);
     if (!IsConstraintExpr || PossibleNonPrimary) {
       // Atomic constraint might be an unparenthesized non-primary expression
       // (such as a binary operator), in which case we might get here (e.g. in
       // 'requires 0 + 1 && true' we would now be at '+', and parse and ignore
       // the rest of the addition expression). Try to parse the rest of it here.
       if (PossibleNonPrimary)
         E = RecoverFromNonPrimary(E, /*Note=*/!IsConstraintExpr);
       Actions.CorrectDelayedTyposInExpr(E);

 ///       C++2a[temp.constr.decl]p1
 ///       constraint-logical-or-expression:
 ///         constraint-logical-and-expression
 ///         constraint-logical-or-expression '||'
 ///             constraint-logical-and-expression
 ///
 /// \endverbatim
 ExprResult
-Parser::ParseConstraintLogicalOrExpression(bool IsTrailingRequiresClause) {
-  ExprResult LHS(ParseConstraintLogicalAndExpression(IsTrailingRequiresClause));
+Parser::ParseConstraintLogicalOrExpression(bool IsTrailingRequiresClause,
+                                           bool IsLambdaRequiresClause) {
+  ExprResult LHS(ParseConstraintLogicalAndExpression(IsTrailingRequiresClause,
+                                                     IsLambdaRequiresClause));
   if (!LHS.isUsable())
     return ExprError();
   while (Tok.is(tok::pipepipe)) {
     SourceLocation LogicalOrLoc = ConsumeToken();
-    ExprResult RHS =
-        ParseConstraintLogicalAndExpression(IsTrailingRequiresClause);
+    ExprResult RHS = ParseConstraintLogicalAndExpression(
+        IsTrailingRequiresClause, IsLambdaRequiresClause);
     if (!RHS.isUsable()) {
       Actions.CorrectDelayedTyposInExpr(LHS);
       return ExprError();
     }
     ExprResult Op = Actions.ActOnBinOp(getCurScope(), LogicalOrLoc,
                                        tok::pipepipe, LHS.get(), RHS.get());
     if (!Op.isUsable()) {
       Actions.CorrectDelayedTyposInExpr(RHS);

clang/lib/Parse/ParseExprCXX.cpp

ExprResult Parser::ParseLambdaExpressionAfterIntroducer(
   PrettyStackTraceLoc CrashInfo(PP.getSourceManager(), LambdaBeginLoc,
                                 "lambda expression parsing");
 
   // Parse lambda-declarator[opt].
   DeclSpec DS(AttrFactory);
   Declarator D(DS, ParsedAttributesView::none(), DeclaratorContext::LambdaExpr);
   TemplateParameterDepthRAII CurTemplateDepthTracker(TemplateParameterDepth);
 
   ParseScope LambdaScope(this, Scope::LambdaScope | Scope::DeclScope |
                                    Scope::FunctionDeclarationScope |
                                    Scope::FunctionPrototypeScope);
 
   Actions.PushLambdaScope();
   Actions.ActOnLambdaExpressionAfterIntroducer(Intro, getCurScope());
 
   ParsedAttributes Attributes(AttrFactory);
   if (getLangOpts().CUDA) {
if (Tok.is(tok::less)) {
     if (TemplateParams.empty()) {
       Diag(RAngleLoc,
            diag::err_lambda_template_parameter_list_empty);
     } else {
       ExprResult RequiresClause;
       if (TryConsumeToken(tok::kw_requires)) {
         RequiresClause =
             Actions.ActOnRequiresClause(ParseConstraintLogicalOrExpression(
-                /*IsTrailingRequiresClause=*/false));
+                /*IsTrailingRequiresClause=*/false,
+                /*IsLambdaRequiresClause=*/true));
         if (RequiresClause.isInvalid())
           SkipUntil({tok::l_brace, tok::l_paren}, StopAtSemi | StopBeforeMatch);
       }
 
       Actions.ActOnLambdaExplicitTemplateParameterList(
           Intro, LAngleLoc, TemplateParams, RAngleLoc, RequiresClause);
       ++CurTemplateDepthTracker;
     }

clang/lib/Sema/SemaConcept.cpp

return BinaryOperator::Create(SemaRef.Context, LHS.get(), RHS.get(),
                                   SemaRef.Context.BoolTy, VK_PRValue,
                                   OK_Ordinary, Loc, FPOptionsOverride{});
   }
 };
 }
 
 bool Sema::CheckConstraintExpression(const Expr *ConstraintExpression,
                                      Token NextToken, bool *PossibleNonPrimary,
-                                     bool IsTrailingRequiresClause) {
+                                     bool IsTrailingRequiresClause,
+                                     bool IsLambdaRequiresClause) {
   // C++2a [temp.constr.atomic]p1
   // ..E shall be a constant expression of type bool.
 
   ConstraintExpression = ConstraintExpression->IgnoreParenImpCasts();
 
   if (LogicalBinOp BO = ConstraintExpression) {
     return CheckConstraintExpression(BO.getLHS(), NextToken,
                                      PossibleNonPrimary) &&
            CheckConstraintExpression(BO.getRHS(), NextToken,
                                      PossibleNonPrimary);
   } else if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpression))
     return CheckConstraintExpression(C->getSubExpr(), NextToken,
                                      PossibleNonPrimary);
 
   QualType Type = ConstraintExpression->getType();
 
   auto CheckForNonPrimary = [&] {
     if (PossibleNonPrimary)
       *PossibleNonPrimary =
           // We have the following case:
           // template<typename> requires func(0) struct S { };
           // The user probably isn't aware of the parentheses required around
           // the function call, and we're only going to parse 'func' as the
           // primary-expression, and complain that it is of non-bool type.
-          (NextToken.is(tok::l_paren) &&
+          (NextToken.is(tok::l_paren) && !IsLambdaRequiresClause &&
            (IsTrailingRequiresClause ||
             (Type->isDependentType() &&
              isa<UnresolvedLookupExpr>(ConstraintExpression)) ||
             Type->isFunctionType() ||
             Type->isSpecificBuiltinType(BuiltinType::Overload))) ||
           // We have the following case:
           // template<typename T> requires size_<T> == 0 struct S { };
           // The user probably isn't aware of the parentheses required around

clang/test/SemaTemplate/concepts.cpp

void g() {
     f3a();
     f3b(); // FIXME: Bad error recovery. expected-error {{undeclared identifier}}
     f4();
     f5();
   }
 }
 
 namespace P0857R0 {
+  template <typename T> static constexpr bool V = true;
+
   void f() {
     auto x = []<bool B> requires B {}; // expected-note {{constraints not satisfied}} expected-note {{false}}
     x.operator()<true>();
     x.operator()<false>(); // expected-error {{no matching member function}}
+
+    auto y = []<typename T> requires V<T> () {};
+    y.operator()<int>(); // OK
   }
 
   template<typename T> concept C = true;
   template<template<typename T> requires C<T> typename U> struct X {};
   template<typename T> requires C<T> struct Y {};
   X<Y> xy;
 }