[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

107–108

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

107–108

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

107–108

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

107–108

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

[erichkeane]

clang/lib/Parse/ParseExprCXX.cpp

1283 ↗

(On Diff #505499)

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 ↗

(On Diff #505499)

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/lib/Sema/SemaConcept.cpp

return CheckConstraintExpression(BO.getLHS(), NextToken,
            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) &&
-           (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
-          // the binary operator, and we're only going to parse 'func' as the
-          // first operand, and complain that it is of non-bool type.
-          getBinOpPrecedence(NextToken.getKind(),
-                             /*GreaterThanIsOperator=*/true,
-                             getLangOpts().CPlusPlus11) > prec::LogicalAnd;
+    if (!PossibleNonPrimary)
+      return;
+
+        //
+        // However, if we're in a lambda, this might also be:
+        // []<typename> requires var () {};
+        // Which also looks like a function call due to the lambda parentheses,
+        // but unlike the first case, isn't an error, so this check is skipped.
+           isa<UnresolvedLookupExpr>(ConstraintExpression) &&
+           !dyn_cast_if_present<LambdaScopeInfo>(getCurFunction())) ||
   };
 
   // An atomic constraint!
   if (ConstraintExpression->isTypeDependent()) {
     CheckForNonPrimary();
     return true;
   }
 

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;
 }