- Notifications
You must be signed in to change notification settings - Fork 10.6k
[Sema]Coerce to rvalue when withoutActuallyEscaping receives an lvalue argument #79238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
f6dc044 to 3523a94 Compare lib/Sema/CSApply.cpp Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do exactly what TypeOf does here, use coerceCallArguments before attempting to use args, that would make sure that AST is updated correctly and call gets LoadExpr as well as nonescaping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the advice.
When I attempted to replicate the behavior of TypeOf, another crash occurred
SIL verification failed: cannot call an async function from a non async function: https://github.com/swiftlang/swift/blob/main/lib/SIL/Verifier/SILVerifier.cpp#L2057
With the following code:
func test( f: inout () -> Void ) { withoutActuallyEscaping(f) { $0() } } It appears that withoutActuallyEscaping(_:do:) is regarded as an async function.
(call_expr type="<null>" isolation_crossing="none" (declref_expr type="(@escaping () -> Void, (@escaping () -> Void) async -> Void) async -> Void" location=79078.swift:37:5 range=[79078.swift:37:5 - line:37:5] decl="Swift.(file).withoutActuallyEscaping(_:do:)" function_ref=single apply) What I did
auto *args = apply->getArgs(); auto appliedWrappers = solution.getAppliedPropertyWrappers( calleeLocator.getAnchor()); auto fnType = cs.getType(fn)->getAs<FunctionType>(); args = coerceCallArguments( args, fnType, declRef, apply, locator.withPathElement(ConstraintLocator::ApplyArgument), appliedWrappers); if (!args) return nullptr; auto *nonescaping = cs.coerceToRValue(args->getExpr(0)); I will investigate the issue. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I’ve identified the cause of the SIL verification failed issue.
It seems that the withAsync(true) is always being set:
https://github.com/swiftlang/swift/blob/main/lib/Sema/TypeOfReference.cpp#L2278
However, I don’t think this is necessary when the code is not in an async context. To address this, I updated the implementation by passing the DeclContext to the getTypeOfReferenceWithSpecialTypeCheckingSemantics function. This allowed me to set an isAsync flag based on the current context.
You can check the changes here:
ebca3f8
3523a94 to 4ff3542 Compare 4ff3542 to 7365d09 Compare aa75aad to 4fd9a44 Compare | @swift-ci please smoke test |
| I apologize for not noticing this change earlier. I believe this change was expected since it's not in an async context, but please correct me if I'm wrong. |
| FunctionType::ExtInfoBuilder() | ||
| .withNoEscape(true) | ||
| .withAsync(true) | ||
| .withAsync(isAsync) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this was originally added in #35664
| CS.getConstraintLocator(locator, ConstraintLocator::ThrownErrorType), | ||
| 0); | ||
| FunctionType::Param arg(escapeClosure); | ||
| bool isAsync = CS.isAsynchronousContext(useDC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a problem with this approach because closures are allowed to assume async keyword but ClosureEffectsRequest only checks presence of await so context cannot always tell you at this point exactly whether a closure is async or not when it has withoutActuallyEscaping in its body.
I this the main issue here is that we need to set the bit on an interface type at the point where we don't actually know whether a type that is passed in as the first argument is async or not.
I think we need a special constraint that would allow to form bodyClosure type based on escapeClosureType and the same thing for refType when types are sufficiently resolved...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meanwhile maybe there is a way to workaround in SILGen the fact that there is always a async as a temporary fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for my late reply.
I've been exploring potential workarounds in SILGen for the async requirement, but it's challenging to me to find a viable solution for now.
I attempted several approaches, including implementing the following change:
stzn@e07814b
I mainly tried to make types async (e.g. from () -> () to () async -> ()).
While this modification resolved the SIL verification failure for the inout sync pattern, it unfortunately triggered a different assertion failure when running the following code:
func checkAssumeMainActor() { MainActor.assumeIsolated {} }The resulting error message was:
Assertion failed: ((!F || opTI->isABICompatibleWith(resTI, *F).isCompatible()) && "Can not convert in between ABI incompatible function types"), function create, file SILInstructions.cpp, line 2819. Despite attempting to make various components async, I haven't been able to resolve this assertion error. Furthermore, I'm uncertain whether my current approach is the correct path forward.
Would you be able to provide some guidance on this matter?
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xedin
I'd like to hear your thoughts on the workaround direction. Thank you.
Resolves #79078
When
withoutActuallyEscapingreceives an lvalue argument, the compiler crashed sinceFunctionTypeis expected as the type ofNonescapingClosureValue, but in the case of lvalue, it has the typeLValueType. Then,closureFnTyis null and aborted.https://github.com/swiftlang/swift/blob/main/lib/AST/ASTVerifier.cpp#L2353
It could be an lvalue when the argument is
inout,consuming,sending.If I understand correctly, the
bodyparameter ofwithoutActuallyEscapingexpects an rvalue. I also referred to the below comment.https://github.com/swiftlang/swift/blob/main/lib/Sema/TypeOfReference.cpp#L2245
Given this, it seems we need to coerce the lvalue to an rvalue.
--
Regarding this, we always set
withAsync(true)in the case ofwithoutActuallyEscaping(https://github.com/swiftlang/swift/blob/main/lib/Sema/TypeOfReference.cpp#L2278). As a result, the async version gets selected by the ConstraintSystem even when it's not async.https://github.com/swiftlang/swift/blob/main/lib/Sema/CSSimplify.cpp#L1919
However, I believe this approach is incorrect when the declaration is not in an async context. Therefore, it is necessary to check whether the context is asynchronous or not.