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

[calda]

This PR fixes #70089 and #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 #70089. Fixes #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 #69911 since it's relevant (which is how I ended up noticing this edge case).

edit: fixed now 👍🏻

[calda]

This is the core of the change. To allow an unwrapped guard let self else { return } binding to be used as implicit self, the closure has to use the ASTScope::lookupSingleLocalDecl lookup behavior for implicit self. Previously we only did this for closures with a [weak self] capture, but in reality any closure could need this behavior.

The other changes in the PR are a rework required to reject invalid code like #70575 (comment), which was previously rejected thanks to this problematic behavior here where the closures didn't use the newer lookup behavior.

[calda]

hey @xedin, I updated the PR with a bunch of additional tests that cover #70089, #69911, and #70575 (comment). 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.

[hamishknight]

Thanks for fixing this!

[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

[hamishknight]

Sorry for dropping the ball on this again, I have a few more comments (mostly minor, though I came up with a couple of cases around shouldOnlyWarn I think we probably ought to fix), but otherwise this looks good to me

[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]

Thanks for the cleanup!

[slavapestov]

I feel like this goes against the spirit of the proposal. Wouldn't you want to write let closure = self.someInstanceMethod for consistency and to make the self capture explicit? Or is this another historical quirk?

[calda]

Here's an example that fails to compile without this:

class Test {
  func foo() {
    let closure = someInstanceMethod
    closure()
  }
  
  func someInstanceMethod() { }
}

Allowing this to compile seems reasonable to me. It's also required for source compatibility.

However, if you instead access someInstanceMethod inside a closure, explicit self is required as you'd definitely expect:

class Test {
  func foo() {
    _ = {
      let closure = someInstanceMethod // error
      closure()
    }
  }
  
  func someInstanceMethod() { }
}

I double-checked locally, the reason this works is that the implicit self decl is validated from both the context of the outer ClosureExpr and the AutoClosureExpr. This happens because the AST is (ClosureExpr (DotSyntaxCallExpr (AutoClosureExpr (DotSyntaxCallExpr)))), and both of the DSCEs are validated separately within the context of their containing closure using different rules. Looking at the git history I think this has been the behavior of the diagnostic's walker since 2019 or so, maybe longer.

[Jumhyn]

Yeah, doing the self capture analysis for bound, unapplied method references was discussed back in the SE-0269 days but never made its way into being an actual proposal. Not sure there'd be appetite for introducing additional cases where implicit self would be disallowed at this point, though I'd probably count myself in favor :)

[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]

Thanks for sticking with this! LGTM

[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: #73482