This is an archive of the discontinued LLVM Phabricator instance.

[clang] Properly parse variable template requires clause in lambda
ClosedPublic

Authored by rymiel on Mar 15 2023, 7:50 AM.

Details

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

Diff Detail

Event Timeline

rymiel created this revision.Mar 15 2023, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2023, 7:50 AM
rymiel requested review of this revision.Mar 15 2023, 7:50 AM
erichkeane added inline comments.Mar 15 2023, 7:55 AM
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 added inline comments.Mar 15 2023, 8:07 AM
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 added inline comments.Mar 15 2023, 8:17 AM
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 updated this revision to Diff 505499.Mar 15 2023, 8:24 AM

Slightly rewrite CheckForNonPrimary for slightly better clarity

rymiel marked 2 inline comments as done.Mar 15 2023, 8:25 AM
rymiel added inline comments.
clang/lib/Sema/SemaConcept.cpp
116

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

rymiel marked an inline comment as done.Mar 15 2023, 8:26 AM
erichkeane added inline comments.Mar 15 2023, 8:39 AM
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 updated this revision to Diff 505525.Mar 15 2023, 9:10 AM

utilize getCurFunction()

rymiel marked an inline comment as done.Mar 15 2023, 9:12 AM
rymiel added inline comments.
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())

rymiel marked an inline comment as done.Mar 15 2023, 9:12 AM
erichkeane accepted this revision.Mar 15 2023, 9:16 AM

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.

This revision is now accepted and ready to land.Mar 15 2023, 9:16 AM

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

rymiel updated this revision to Diff 505534.Mar 15 2023, 9:28 AM

Use dyn_cast_if_present, otherwise we segfault in some tests

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

Yep, found an issue already ;;

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.

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

Yep, found an issue already ;;

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

Do we need a release note for this?

Do we need a release note for this?

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

rymiel updated this revision to Diff 506121.Mar 17 2023, 9:54 AM

Add release note

This revision was landed with ongoing or failed builds.Mar 17 2023, 1:31 PM
This revision was automatically updated to reflect the committed changes.