This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Fix Unevaluated Lambdas
ClosedPublic

Authored by cor3ntin on Mar 12 2022, 12:02 PM.

Details

Reviewers
shafik
erichkeane
aaron.ballman
Group Reviewers
Restricted Project
Commits
rG3784e8ccfbda: [Clang] Fix Unevaluated Lambdas
Summary

Unlike other types, when lambdas are instanciated,
they are recreated from scratch.
When an unevaluated lambdas appear in the type of a function,
parameter it is instanciated in the wrong declaration context,
as parameters are transformed before the function.

To support lambda in function parameters, we try to
compute whether they are dependant without looking at the
declaration context.

This is a short term stopgap solution to avoid clang
iceing. A better fix might be to inject some kind of
transparent declaration with correctly computed dependency
for function parameters, variable templates, etc.

Fixes https://github.com/llvm/llvm-project/issues/50376
Fixes https://github.com/llvm/llvm-project/issues/51414
Fixes https://github.com/llvm/llvm-project/issues/51416
Fixes https://github.com/llvm/llvm-project/issues/51641
Fixes https://github.com/llvm/llvm-project/issues/54296

Diff Detail

Event Timeline

cor3ntin created this revision.Mar 12 2022, 12:02 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: martong. · View Herald Transcript
cor3ntin requested review of this revision.Mar 12 2022, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2022, 12:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added a subscriber: aaron.ballman.

Adding Erich to look at this WIP for early feedback as he's recently been looking at template instantiation guts rather deeply, but I added some questions.

clang/include/clang/AST/DeclCXX.h
280–282

(Matches the naming style used just above.)

clang/lib/Sema/TreeTransform.h
12989
13007

Can you explain why you only do these transformations when the old class is a dependent context? Also, should this be looking at getDerived().AlwaysRebuild()? I'm not certain -- we're not calling any Rebuild* functions here because lambdas are not rebuilt but generated entirely from scratch. So I'm guessing we don't need to look at AlwaysRebuild, but it's a bit unclear.

13023–13024

Are these changes necessary as part of this patch? I'm a bit worried that only doing this in dependent contexts may change behavior for code like this: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L769

13092

This also seems a bit surprising -- can't the variable declaration be non-templated but still dependent (e.g., an auto variable whose initialization is dependent?)

cor3ntin added inline comments.Mar 14 2022, 7:35 AM
clang/lib/Sema/TreeTransform.h
13007

I was trying to fix some crashes at some point, Not sure that's still needed anymore, I have to check. I suspect you might be right.

cor3ntin updated this revision to Diff 415187.Mar 14 2022, 12:30 PM

Remove prior attempt at fixing this bug (good catch Aaron!)

Can you add a test to ASTImpoterTest.cpp that checks that we import the LambdaDependencyKind correctly?

Can you add a test to ASTImpoterTest.cpp that checks that we import the LambdaDependencyKind correctly?

Sure, if we agree that the direction is sufficiently palatable (I'm not convince it is yet), I can add such a test (and more generally cleanup the test)

aaron.ballman added a reviewer: Restricted Project.Mar 18 2022, 7:46 AM
aaron.ballman added inline comments.
clang/lib/Sema/SemaLambda.cpp
978–980

I don't think we should pass KnownDependent here as a bool, despite it working because of the enumerator value for the known dependent case.

cor3ntin updated this revision to Diff 417761.Mar 23 2022, 3:09 PM
  • Added a couple of tests in ASTImporterTest.cpp
  • Rename elements of LambdaDependencyKind
  • Fix ActOnStartOfLambdaDefinition to not use a boolean for

LambdaDependencyKind.

cor3ntin updated this revision to Diff 417762.Mar 23 2022, 3:09 PM

Formatting.

cor3ntin retitled this revision from [Clang][WIP] Fix Unevaluated Lambdas to [Clang] Fix Unevaluated Lambdas.Mar 23 2022, 3:10 PM

I think this is a sufficient stopgap measure to keep clang from crashing, and I appreciate the FIXME comment. Should the cxx_status page also be updated, or is P0315R4 still partial (and if it is, can we add information to the status page about what's partial if you know it)? Also, this could use a release note about the fixes. So almost LGTM!

clang/lib/Sema/TreeTransform.h
12989

Still need to fix the typos here.

Don't forget about Release notes!

cor3ntin updated this revision to Diff 418269.Mar 25 2022, 10:20 AM
  • Fix a typo (sorry Aaron, I completely missed that)
  • Add a release note
  • Clarify why the implementation status is still partial.

This is a bit of a drive-by change but I agree it's a good thing
to clarify.

This revision is now accepted and ready to land.Mar 25 2022, 10:31 AM
This revision was landed with ongoing or failed builds.Mar 25 2022, 11:16 AM
This revision was automatically updated to reflect the committed changes.

@cor3ntin I wonder if these asserts are due to the partial implementation here: https://github.com/llvm/llvm-project/issues/57960

I am happy to look into it if you can point me in the right direction.