Enable SE-0365 behavior in Swift 5 mode

[calda]

This PR enables the behavior defined in SE-0365 for Swift 5 language modes.

The expands on #40702, which implemented SE-0365 only in Swift 6 mode.

[calda]

@xwu @xedin This is a follow-up to our discussion here: #61513 (comment)

[calda]

I noticed the previous implementation was incorrectly rejecting guard let self and if let self shorthand conditions, which have an "implicit self" expr on the RHS. These are always allowed so we shouldn't be running these diagnostics on them.

[calda]

This now compiles in Swift 5 mode

[calda]

This now warns in Swift 5 mode (it's an error in Swift 6 mode)

[xwu]

cc @hborla @rjmccall

[hborla]

The implementation approach looks good! I only have a few minor comments about the language used in the names and comments in the code.

[xwu]

To me, these explanations are a lot more clear, thanks!

[calda]

Thank you all for the feedback and discussion -- I really appreciate it!

Unfortunately, I was running some additional tests and found an issue with this approach. Since the implicit self decl in the AST refers to the closure's strongly-typed self ParamDecl, this causes self to be captured strongly even in a closure that declares [weak self].

Here's the sample I was testing with (collapsed for brevity):

func runInOneSecond(_ closure: @escaping @Sendable () -> Void) {
  Task {
    try! await Task.sleep(nanoseconds: 1_000_000_000)
    closure()
  }
}

final class Weak: Sendable {
  let property = "Self exists"

  func test() {
    runInOneSecond { [weak self] in
      guard let self else { 
        print("Self doesn't exist")
        return
      }

      print(property)
    }
  }
}

Weak().test()
try await Task.sleep(nanoseconds: 2_000_000_000)

In Swift 6 mode, self is captured weakly as expected. In Swift 5, however, self is captured strongly.

Fixing this for escaping closures in Swift 5 while maintaining source compatibility would require modifying how captures are generated, and ignoring the decl in the AST in favor of doing a manual lookup like we do in MiscDiagnostics. This would actually change the behavior of the program, which sounds problematic based on the discussion above.

@xwu, given this information I think implementing SE-0365 for escaping closures in Swift 5 is not viable.

I think our best option here would be to:

• continue disallowing implicit self in escaping weak self closures until Swift 6
• continue allowing implicit self in non-escaping weak self closures in Swift 5 (required for source-compatibility), but emit a warning unless the code would compile correctly in Swift 6 (effectively what the code in this PR is already doing, but for non-escaping closures specifically).

Thoughts? You mentioned in #61513 (comment) that this may require an amendment / re-review of SE-0365. If that's the case, I can submit an amendment to the proposal.

[xwu]

In Swift 5, however, self is captured strongly.

In shipping versions of Swift, there is a bug involving [weak self] only with non-escaping closures. That is to say, without #40702, the following code doesn't compile, so there is no source compatibility to maintain:

func f(_ closure: @escaping () -> Void) { closure() }
final class C { 
  var property = "Hello, world!" 
  func test() { 
    f { [weak self] in print(property) }
    // error: reference to property 'property' in closure requires explicit use of 'self' to make capture semantics explicit
  } 
}

Put another way, unless I'm mistaken, it's not accurate to say that the code in your example "is captured strongly" in Swift 5; the Swift 5 compiler refuses to capture it strongly or weakly as it shouldn't compile at all.

If this PR causes the code in your example to compile in the Swift 5 language mode with a strong capture, then it should not land in that condition; and if #40702 is what caused the code to compile in that way, then it will need to be backed out. We need to maintain source compatibility with the shipping compiler's existing buggy behavior, but we absolutely shouldn't widen the bug in the Swift 5 language mode to involve escaping closures too.

[calda]

If this PR causes the code in your example to compile in the Swift 5 language mode with a strong capture, then it should not land in that condition

Yes, this is an issue specifically introduced by this pull request (and not present on main or in #40702).

I think we should update this PR to disallow implicit self in escaping weak self closures, until Swift 6 (where self is correctly captured weakly).

That would mean that, in Swift 5, SE-0365 would only apply to non-escaping closures and not escaping closures (it applies to both in Swift 6). Is that acceptable? Would that change require a re-review of SE-0365?

[xwu]

I think we should update this PR to disallow implicit self in escaping weak self closures, until Swift 6 (where it works correctly).

What is the technical barrier to implementing SE-0365 as approved for the Swift 5 language mode—i.e., to capturing self weakly in escaping closures?

That would mean that, in Swift 5, SE-0365 would only apply to non-escaping closures and not escaping closures (it applies to both in Swift 6). Is that acceptable? Would that change require a re-review of SE-0365?

Yes, it would require a re-review. However, the language workgroup discussed that if the reason for limiting a feature to the Swift 6 language mode is technical only, they would like to see that addressed in the implementation side, and if it cannot be, they would favor not landing the feature at all (i.e., backing out #40702).

[calda]

What is the technical barrier to implementing SE-0365 as approved for the Swift 5 language mode?

We would have to fix this issue where using implicit self causes self to be captured strongly. This would require changing how captures are generated, and ignoring the decl in the AST in favor of a doing a manual lookup.

I personally agree with the discussion above that circumventing the AST and doing manual lookups like this in MiscDiagnostics is acceptable since it doesn't affect the behavior of the program itself, but that this probably wouldn't be acceptable elsewhere (e.g. swiftSIL/SILGenFunction.cpp, where the captures are generated) since it would cause the actual behavior of the program at runtime to no longer reflect the AST.

[calda]

if it cannot be [addressed on the implementation side], they would favor not landing the feature at all

What would this mean for SE-0365? Would it effectively move from being approved to being rejected / returned? Would there be a path for landing this change in Swift 6 mode (as currently implemented on main, by #40702)? Can we re-review it (if necessary) and accept it with a final status of "Accepted, Implemented in Swift 6"?

[xwu]

We would have to fix this issue where using implicit self causes self to be captured strongly. This would require changing how captures are generated, and ignoring the decl in the AST in favor of a doing a manual lookup.

Sorry, I'm not understanding something. In the shipping Swift compiler, the escaping example above doesn't compile at all, so there's nothing to "change" as there's no status quo—your implementation of SE-0365 which generates the "correct" AST shouldn't need to be gated to the Swift 6 language mode, and thus there wouldn't be anything to "circumvent"?

What would this mean for SE-0365? Would it effectively move from being approved to being rejected / returned? Would there be a path for landing this change in Swift 6 mode (as currently implemented on main, by #40702)?

I think the consensus of the language workgroup discussion is that we'll want to clarify the technical issues here before determining if there's a process issue.

[calda]

your implementation of SE-0365 which generates the "correct" AST shouldn't need to be gated to the Swift 6 language mode

This change must be gated to the Swift 6 language mode, because it's source breaking. This "incorrect" AST in Swift 5 is the specific reason why implicit self is unexpectedly permitted in non-escaping weak self closures. Changing the AST as required by SE-0365 also fixes that bug (which we can't do until Swift 6 🫠).

[xedin]

doing manual lookups like this in MiscDiagnostics is acceptable since it doesn't affect the behavior of the program itself

I'm okay to agree to disagree on this but the point I was trying (and keep trying) to make is that diagnostics shouldn't be misled about what declaration has been selected. It's okay to perform a lookup and double-check (there are examples of that) as a dedicated method but we shouldn't be returning results which are inconsistent with solutions produced by the solver.

[xwu]

This change must be gated to the Swift 6 language mode, because it's source breaking. This "incorrect" AST in Swift 5 is the specific reason why implicit self is unexpectedly permitted in non-escaping weak self closures.

I guess what I'm asking is, can permitting implicit strong self in non-escaping weak self closures for the Swift 5 language mode be re-implemented (specifically for non-escaping weak self closures, of course) on top of the "correct" AST?

[xedin]

can permitting implicit strong self in non-escaping weak self closures for the Swift 5 language mode be re-implemented (specifically for non-escaping weak self closures, of course) on top of the "correct" AST?

I think the answer is yes but that is a source breaking change.

[xwu]

I think the answer is yes but that is a source breaking change.

Sorry, what source would break? Perhaps I am expressing myself in a confusing way, so let me try to rephrase. Is it technically possible to implement a version of Swift where both the behavior of SE-0365 is implemented and non-escaping closures capturing weak self are permitted to use implicit strong self?

[calda]

I investigated that approach and I don't think it's feasible. I believe the main obstacle with that approach is that determining whether a closure is escaping or non-escaping happens very late in the type-checking process, after the closure body has already been type-checked with the "incorrect" AST. So by the time we have enough information to know how implicit self should behave, it's too late to actually do anything about it.

[xedin]

I concur with that assessment, we cannot actually go either or here because of how type and effects checking are implemented.

[xwu]

Got it, I think that’s grounds to have the workgroup revisit what it would like to do here, then…

[calda]

@xwu, here's a PR that updates the proposal text with my proposed amendments: swiftlang/swift-evolution#1819

[rjmccall]

I apologize if you've already considered this approach, but I do think there's a way to make SE-0365 fully supported on Swift <6 regardless of the escaping-ness of closures.

SE-0365 introduces a predicate on variables for whether they are non-optional rebindings of the contextual self parameter. This predicate does not depend on whether the rebinding occurs in an escaping or non-escaping closure. In Swift 6, this predicate is of course what we use to decide whether the lexical self is valid to implicitly use. In Swift <6, because of this source-compatibility problem, we cannot decide that immediately. However, we can get the effect we want by using this same predicate to affect the lookup of self for implicit self references. That is, we say that the lookup rule for implicit self is:

1. Look up self lexically.
2. In non-Swift 6 modes, if the result of (1) is not a non-optional rebinding of the contextual self parameter, ignore it and use the contextual self parameter.

While this might look superficially like it will cause the bad lookup rule to start affecting escaping closures in Swift <6 modes, that's not actually the case. Recall that the existing lookup rule for implicit self in Swift <6 modes is simply to use the contextual self parameter, i.e. exactly the result of the bad lookup rule, so if this was going to have widespread problems, we'd already be seeing them. The reason this existing rule has no impact on escaping closures is because it is illegal (a hard error) to implicitly reference the contextual self parameter in escaping closures, and that continues to be the case under SE-0365, which affects only these non-optional rebindings. So whenever (2) kicks in, we are essentially queuing up an error later if it turns out that we're in an escaping closure, which is exactly what we want. And when (2) doesn't kick in in Swift <6 modes, while we may be formally changing the result of lookup, that should have no semantic impact because the predicate guarantees that we've got the same value.

Now, we might want a specialized diagnostic when diagnosing the implicit reference to self if it happens after (2) kicks in, but that should be achievable without breaking the compiler.

[calda]

Thanks for the idea @rjmccall! I hadn't considered that specific approach -- it sounds really promising to me. I'll try this out and let folks know how it goes

[calda]

That approach works perfectly, thank you very much for the suggestion @rjmccall!

I pushed a commit with these changes (8528021) -- in Swift 5 mode we now preserve the old lookup behavior in cases where self hasn't been unwrapped yet, and use the new lookup behavior if self has been unwrapped. All of the tests pass, and using implicit self in an escaping closure correctly results in a weak capture.

@xedin, would you mind taking a look at the latest changes? This approach seems much better since we no longer need to perform manual lookups in MiscDiagnostics.

[calda]

One question: we probably also want a test case that validates that self is actually captured weakly when using implicit self in Swift 5 mode. What's the best way to create a test case for this?

I've been testing this locally by running this code and checking the console output. Is there a way to write a test case that does this, or is there a different preferred way to test this sort of thing?

[xedin]

@calda Sure! I will take a look tomorrow.

[xedin]

LGTM!

[xedin]

I've been testing this locally by running this code and checking the console output. Is there a way to write a test case that does this, or is there a different preferred way to test this sort of thing?

You can turn this in an executable test.

[calda]

Thanks, I added an additional test case for that. Tests pass for me locally, could you trigger smoke tests and source compatibility tests?

[xedin]

@swift-ci please test

[xedin]

@swift-ci please test source compatibility

[calda]

This failure is weird -- it doesn't seem to be a compiler error:

sandbox-exec: <internal init prelude>:456:12: unable to open system.sb: not found
	(apply error x)

Are those tests flaky sometimes?

[xedin]

Yep, let's re-trigger.

[xedin]

@swift-ci please test source compatibility

[xedin]

@shahmishal Is the sandbox issue that occurs in protobuf project being tracked?

[xedin]

@swift-ci please test source compatibility

[calda]

Tests are passing, great! Really appreciate your help with this change over the past few months @xedin, couldn't have done it without you :)

[xedin]

No worries, happy to help!

[xedin]

Thanks for seeing this through!