swift
[SE-0365] Fix #64757: Unexpected "Implicit use of 'self' in closure" error in closure nested in result builder
#64786
Merged

[SE-0365] Fix #64757: Unexpected "Implicit use of 'self' in closure" error in closure nested in result builder #64786

xedin merged 6 commits into swiftlang:main from calda:cal--64757
calda
calda2 years ago (edited 1 year ago)

This PR fixes an issue with the implementation of SE-0365, where the following code would unexpectedly emit an error:

@resultBuilder
struct VoidBuilder {
    static func buildBlock() -> Void { }
    static func buildPartialBlock<T>(first: T) -> Void { }
    static func buildPartialBlock<T>(accumulated: Void, next: T) -> Void { }
}

final class EscapingWrapper {
    static func wrapper(_ closure: @escaping () -> Void) {
        closure()
    }
}

final class Sample {
    @VoidBuilder
    var void: Void {
        EscapingWrapper.wrapper { [weak self] in
            guard let self else { return } // Unexpected error: Implicit use of 'self' in closure; use 'self.' to make capture semantics explicit
        }
    }
}

Fixes #64757, fixes #71040.

Please review: @xedin

calda calda requested a review from hborla hborla 2 years ago
calda calda requested a review from slavapestov slavapestov 2 years ago
calda calda requested a review from xedin xedin 2 years ago
calda
calda commented on 2023-03-30
Conversation is marked as resolved
Show resolved
lib/Sema/MiscDiagnostics.cpp
2033 auto isDescendantWalker = ExprIsDescendantWalker(const_cast<Expr *>(E));
2034 ACE->walk(isDescendantWalker);
2035 if (!isDescendantWalker.exprIsDescendant) {
2036
ACE = nullptr;
calda2 years ago

The implementation of diagnoseImplicitSelfUseInClosure effectively requires E to be a part of the body of ACE.

For some reason, in this specific case using result builders, this code was being invoked with an expression that wasn't actually a descendant of the closure.

A fix that seems to work well here is to just check whether or not E is a descendant of ACE by walking the AST of the closure and checking if we encounter E.

xedin
xedin2 years ago

@calda I looked at the example under -debug-constraints and I think the main issue here is that guard statement for some reason is type-checked separately in result builder transform context, that's what we need to fix here.

xedin
xedin2 years agoπŸ‘€ 1

@ahoppen Looks like this is the issue here https://github.com/apple/swift/blob/main/lib/Sema/ConstraintSystem.cpp#L7112. Should this be set only in code completion mode?

calda Fix issue where closure implicit self check was being run on expr tha…
1b5dab00
calda Add test case for #64757
548456d6
calda Also walk to Stmts and Decls, since those can contain Exprs
8b193a2f
calda Tweak ClosureExpr inference instead
10878e8a
calda calda force pushed from 693d969d to 10878e8a 2 years ago
calda
calda2 years ago

Thanks for the review and suggestion @xedin.

In 10878e8 I updated the condition you sent to also include isForCodeCompletion() -- is that about what you had in mind?

The tests seem to pass locally after I made this change.

xedin
xedin2 years ago (edited 2 years ago)

@calda I haven't looked at where this flag gets set but I was hoping that this check you added code be moved there so we end up without this option if solver is not in code completion mode...

calda Configure option at call site instead
c4804321
calda
calda2 years ago

Thanks @xedin, that makes sense. I moved the change up to the callsite of the method that configures that option. All the tests pass still with this change.

xedin
xedin approved these changes on 2023-04-10
xedin xedin requested a review from ahoppen ahoppen 2 years ago
xedin
xedin2 years ago

@ahoppen WDYT?

xedin
xedin2 years ago

@swift-ci please test

xedin
xedin commented on 2023-04-10
lib/Sema/TypeCheckStmt.cpp
26512651 TypeChecker::applyResultBuilderBodyTransform(
2652 func, builderType)) {
2652 func, builderType,
2653 /*ClosuresInResultBuilderDontParticipateInInference=*/
2654
false)) {
xedin2 years ago

@ahoppen Should we flip default to false maybe instead?

xedin
xedin2 years ago

@calda Could you please switch default to false instead? I think it shouldn't affect tests but I want to run sourcekit stress tester as well.

calda Update default value of argument
7c78ad28
calda
calda2 years agoπŸ‘ 1

Sure @xedin, here you go: 7c78ad2

xedin
xedin2 years ago

@swift-ci Please SourceKit stress test

xedin
xedin2 years ago

@swift-ci please test

xedin xedin merged 0bdacfb4 into main 2 years ago

Login to write a write a comment.

Login via GitHub

Assignees
No one assigned
Labels
Milestone