Page MenuHomePhabricator

Lambdas are not necessarily locals. This resolves DR48250.
ClosedPublic

Authored by davidstone on Mar 22 2021, 7:37 PM.

Diff Detail

Event Timeline

davidstone requested review of this revision.Mar 22 2021, 7:37 PM
davidstone created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2021, 7:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

It looks like this change was originally added 9 years ago with a commit message of

"Lambda closure types are always considered to be like "local" classes,
even if they are not within a function scope. Teach template
instantiation to treat them as such, and make sure that we have a
local instantiation scope when instantiating default arguments and
static data members."

but unfortunately, it does not include the context to explain *why* this is the case. There do not appear to be any tests that capture this, and I haven't found code that fails to compile / causes the compiled code's unit tests to fail.

The previous commit is https://github.com/llvm/llvm-project/commit/a86bc00, and I don't understand enough of what the other code is doing to know if anything else needs to be fixed. I'd also like to add in a test to ensure this doesn't regress in the future, but I'm not sure of what the best place to put the test is. From https://bugs.llvm.org/show_bug.cgi?id=48250

auto lambda = [](auto) {};

template<typename T>
void a(T x) { lambda.operator()(x); }

void b() { a(0); }

Hm, do we ever call FindInstantiatedDecl on a lambda class in the first place? It seems plausible to me that this condition is unreachable. But to the extent that it's reachable, it seems mostly correct: a lambda expression appearing in a non-dependent context might still be dependent (for example, it could be a lambda in the initializer of a variable template), and in that case, it should be found in the local instantiation scope. I think it's not quite correct in that case, though: we should expect to find the closure type in the local instantiation scope only if its template depth (D->getTemplateDepth()) is greater than the number of outer retained template levels. In fact, we could change ParentDependsOnArgs to do that check in general (Decl::getTemplateDepth didn't exist when ParentDependsOnArgs was written).

Ah, I think I see how we get here. I think this fix might possibly break a case such as:

template<typename T> int x = [](auto){ return T(); }.operator()(T());
int y = x<int>;

... where transforming the call to operator() might end up looking for the transformed version of the lambda class; we need to find that in the local instantiation scope.

I think we want something like isa<CXXRecordDecl>(D) && cast<CXXRecordDecl>(D)->isDependentLambda() && cast<CXXRecordDecl>(D)->getTemplateDepth() > TemplateArgs.getNumRetainedOuterLevels(). (But I wonder if we can clean this up by switching to always checking the template depth of D rather than sometimes checking the parent's template depth.)

davidstone updated this revision to Diff 385508.Nov 8 2021, 8:20 AM

Take into account Richard's comments

Is there anything else I need to do to help move this forward?

erichkeane accepted this revision.Feb 10 2022, 8:15 AM

From my look, this seems to fix the problem, and covers the case that Richard brought up, and I have no comments, so I think I'm happy with it. Please give Richard/Aaron ~24 hrs to take a look before committing.

This revision is now accepted and ready to land.Feb 10 2022, 8:15 AM
jloser added a subscriber: jloser.Feb 10 2022, 8:28 AM

Would love to see this cherry-picked into the llvm 14 release branch if possible!

rsmith accepted this revision.Feb 10 2022, 11:45 AM

LGTM.

Please also add something like this as a unit test:

template<typename T> int x = [](auto){ return T(); }.operator()(T());
int y = x<int>;

Add unit test requested by Richard Smith

rsmith accepted this revision.Feb 17 2022, 7:12 PM

Looks good. Do you need someone to land this for you?

Looks good. Do you need someone to land this for you?

Yes, I do.

Looks good. Do you need someone to land this for you?

Yes, I do.

I'll do that for you, working on it now.

This revision was automatically updated to reflect the committed changes.

Would love to see this cherry-picked into the llvm 14 release branch if possible!

I believe there is a bug in github that you can use to request that. This just landed as 0bff3a965022647fcdd17cc8f2217f5a2cd30b4c