Skip to content

Conversation

@calda
Copy link
Contributor

@calda calda commented Mar 30, 2023

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

@xedin
Copy link
Contributor

xedin commented Apr 4, 2023

@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
Copy link
Contributor

xedin commented Apr 4, 2023

@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
Copy link
Contributor Author

calda commented Apr 4, 2023

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
Copy link
Contributor

xedin commented Apr 4, 2023

@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
Copy link
Contributor Author

calda commented Apr 7, 2023

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 requested a review from ahoppen April 10, 2023 17:57
@xedin
Copy link
Contributor

xedin commented Apr 10, 2023

@ahoppen WDYT?

@xedin
Copy link
Contributor

xedin commented Apr 10, 2023

@swift-ci please test

func, builderType)) {
func, builderType,
/*ClosuresInResultBuilderDontParticipateInInference=*/
false)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahoppen Should we flip default to false maybe instead?

@xedin
Copy link
Contributor

xedin commented Apr 10, 2023

@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
Copy link
Contributor Author

calda commented Apr 10, 2023

Sure @xedin, here you go: 7c78ad2

@xedin
Copy link
Contributor

xedin commented Apr 10, 2023

@swift-ci Please SourceKit stress test

@xedin
Copy link
Contributor

xedin commented Apr 10, 2023

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants