[SE-0365] Fix issue where implicit self was unexpectedly disallowed in nested closures

[calda]

This PR fixes https://github.com/apple/swift/issues/70089 and https://github.com/apple/swift/issues/69911. Previously, the following sample code unexpectedly failed to compile:

class Test {
  var x: Int = 0

  func f() {
    doVoidStuff { [weak self] in
      guard let self else { return }
            
      doVoidStuff { [self] in
        x += 1 // error: reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit
      }

      doVoidStuffNonEscaping {
        x += 1 // error: reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit
      }
    }
  }
}

func doVoidStuff(_: @escaping () -> Void) {}
func doVoidStuffNonEscaping(_: () -> Void) {}

Both of these examples are expected to compile based on the behavior described here in SE-0365:

// Also allowed:
couldCauseRetainCycle { [weak self] in
  guard let self else { return }
  foo()

  couldCauseRetainCycle { [self] in
    bar()
  }
}

While SE-0365 doesn't explicitly address the behavior of non-escaping closures, it does say that the behavior should follow the same rules as SE-0269, which does allow implicit self in nested non-escaping closures:

doVoidStuff { [self] in
  doVoidStuffNonEscaping {
    x += 1 // ok
  }
}

Now we allow implicit self in these nested closures, as long as either:

• the closure is non-escaping, or
• the closure explicitly captures self in a valid way

Please review: @xedin @jumhyn (hi again, thanks!)

Fixes https://github.com/apple/swift/issues/70089. Fixes https://github.com/apple/swift/issues/69911.

[calda]

Hi @xedin could you review this bug fix? Thanks!

[calda]

Still looking for a review for this bug fix, @xedin could you help?

Also tagging other folks who GitHub lists as reviewers for these files: @hborla @slavapestov

Thanks!

[xedin]

Sorry I missed a notification about this, I'll take a look next week!

[calda]

Thank you! 🙇🏻

[calda]

I noticed the current implementation in this PR has an issue where it unexpectedly allows these examples to compile even though they shouldn't:

doVoidStuff { [self = MyTestClass()] in
  doVoidStuff { [self] in
    x += 1
  }
}

doVoidStuff { [weak self] in
  guard let self = self ?? MyTestClass.staticOptional else { return }
  doVoidStuff { [self] in
    x += 1
  }
}

I have a rough change put together locally which fixes this issue. I plan on cleaning it up tomorrow and updating this PR. I'm also taking a look at https://github.com/apple/swift/issues/69911 since it's relevant (which is how I ended up noticing this edge case).

edit: fixed now 👍🏻

[calda]

hey @xedin, I updated the PR with a bunch of additional tests that cover https://github.com/apple/swift/issues/70089, https://github.com/apple/swift/issues/69911, and https://github.com/apple/swift/pull/70575#issuecomment-1951685335. All of the new and existing tests pass! So this is ready for a review now, once you have a chance.

[xedin]

Sorry about the delay, @hamishknight is going to take a look at this one.

[calda]

Thank you for the very thorough review @hamishknight! I especially appreciate that you noticed and called out several existing bugs in the previous implementation. Great that we caught those in time for Swift 6, considering the fix is source-breaking.

I updated the PR to fix all of the issues you mentioned, and added even more tests.

[calda]

I also just pushed some final cleanup that improves the quality of some of the informational diagnostics.

Before we would sometimes emit confusing 'variable other than self captured here under the name self does not enable implicit self' diagnostics even for closures with a [self] capture, and would sometimes emit capture 'self' explicitly to enable implicit 'self' in this closure diagnostics in cases where adding an explicit self capture wouldn't actually fix the issue (because some parent closure was invalid). Both of those edge cases are fixed and improved now 👍🏻

[hamishknight]

Thanks! I'll try and take a look tomorrow

[hamishknight]

Sorry, I haven't forgotten about this, I'll try and take a look today or tomorrow

[calda]

Thanks again for the review @hamishknight! I pushed a change that fixes the edge cases you pointed out, plus your other suggestions.

[calda]

Thanks again @hamishknight! Glad we could iron out more of these edge cases -- updated the PR to incorporate your suggestions.

[calda]

Thanks for catching so many edge cases @hamishknight! Updated to fix those unexpected new errors, and also cleaned up the implementation of shouldOnlyWarn.

[slavapestov]

@swift-ci Please smoke test

[slavapestov]

@swift-ci Please test source compatibility

[calda]

Tests passed!

The three failures in the source compatibility suite look unrelated to me: https://ci.swift.org/job/swift-PR-source-compat-suite-macos/1585/testReport

[hamishknight]

The three failures in the source compatibility suite look unrelated to me

Yeah they're also failing on main: https://ci.swift.org/job/swift-main-source-compat-suite/767/

[calda]

Thanks everyone! 🙇🏻 I also posted a PR for the Swift 6.0 release branch: https://github.com/apple/swift/pull/73482