Page MenuHomePhabricator

[clang] Correct handling of lambdas in lambda default arguments in dependent contexts.
ClosedPublic

Authored by tahonermann on Sep 8 2022, 8:09 AM.

Details

Summary

Previously, a lambda expression in a dependent context with a default argument containing an immediately invoked lambda expression would produce a closure class object that, if invoked such that the default argument was used, resulted in a compiler crash or one of the following assertion failures during code generation. The failures occurred regardless of whether the lambda expressions were dependent.

clang/lib/CodeGen/CGCall.cpp:
Assertion `(isGenericMethod || Ty->isVariablyModifiedType() || Ty.getNonReferenceType()->isObjCRetainableType() || getContext() .getCanonicalType(Ty.getNonReferenceType()) .getTypePtr() == getContext().getCanonicalType((*Arg)->getType()).getTypePtr()) && "type mismatch in call argument!"' failed.

clang/lib/AST/Decl.cpp:
Assertion `!Init->isValueDependent()' failed.

Default arguments in declarations in local context are instantiated along with their enclosing function or variable template (since such declarations can't be explicitly specialized). Previously, such instantiations were performed at the same time that their associated parameters were instantiated. However, that approach fails in cases like the following in which the context for the inner lambda is the outer lambda, but construction of the outer lambda is dependent on the parameters of the inner lambda. This change resolves this dependency by delaying instantiation of default arguments in local contexts until after construction of the enclosing context.

template <typename T>
auto f() {
  return [](T = []{ return T{}; }()) { return 0; };
}

Refactoring included with this change results in the same code now being used to instantiate default arguments that appear in local context and those that are only instantiated when used at a call site; previously, such code was duplicated and out of sync.

Diff Detail

Event Timeline

tahonermann created this revision.Sep 8 2022, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 8:09 AM
tahonermann requested review of this revision.Sep 8 2022, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 8:09 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Patch seems sound, just a nit that we don't need a bunch of the asserts here, since the 'get' function already asserts it looks. I'd like others to take a look too, but this looks acceptable to me.

clang/lib/Sema/SemaTemplateInstantiate.cpp
1280

This assert is unncessary, the line below will assert. Same with the similar 'assert' later on.

1282

Looks like we don't store the equals-token location, so any attempt here probably would have to have ParmVarDecl store this location somewhere?

2681

Here too.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2302

See above.

2688

And again.

tahonermann added inline comments.Sep 8 2022, 8:29 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
2598–2613

This functionality is now incorporated into Sema::SubstDefaultArguments() and postponed until after construction of enclosing declaration contexts.

2728–2749

I find this code exceedingly troubling, but I haven't been able to figure out how to unify it. Conceptually, instantiation of a default argument should produce the same result (the instantiated default argument is cached after all!) whether it is implicitly always instantiated (as in a declaration in local scope or a friend function definition) or instantiated on demand for a call expression. However, my attempts to unify this code have all resulted in test failures that I have, so far, been unable to explain. Note that much of the code in the ForCallExpr case is replicated in the ConvertParamDefaultArgument() function. I think this should be investigated further and may be a reason to postpone approving this change.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2137

This seems highly questionable to me. I suspect there is a better way to identify the correct context, but I have not so far found it elsewhere. Note that the context needed is for the enclosing function template specialization (not the enclosing template).

2295–2310

This code is effectively replicated in three distinct locations. It is probably worth moving it into a helper function somewhere.

4534

The above removed code has been incorporated into Sema::SubstDefaultArgument().

Rebased and removed redundant asserts pointed out in code review.

tahonermann marked 5 inline comments as done.Oct 1 2022, 5:13 AM
tahonermann added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
1282

That's right. This is a pre-existing issue; there are several places that have this same (or similar) FIXME comment.

erichkeane accepted this revision.Oct 3 2022, 6:58 AM

I'm OK with it as-is, and the refactor to move the replicated code would be acceptable in an NFC followup.

This revision is now accepted and ready to land.Oct 3 2022, 6:58 AM
This revision was landed with ongoing or failed builds.Oct 4 2022, 9:08 AM
This revision was automatically updated to reflect the committed changes.
tahonermann marked an inline comment as done.