swiftlang
[Sema]: diagnose implicit strong captures of weak capture list entries
Add a diagnostic to warn when a capture list item creates a weak or unowned binding to a value with strong ownership that is captured in an outer escaping closure, but is not itself in a capture list.
This change adds a new diagnostic for situations like the following:
class SomeViewController: UIViewController {
// ...
func foo(request: URLRequest) {
let task = URLSession.shared.dataTask(with: request) { data, response, error in
let result = // process data...
DispatchQueue.main.async { [weak self] in // capture list entry causes `self` to be strongly captured in the parent closure scope
self?.handleResult(result)
}
}
task.resume()
}
}
the goal is to emit warnings for such cases along the lines of:
example.swift:10:40: warning: 'weak' capture list item 'self' implicitly captured as strong reference in outer scope
6 | // ...
7 | func foo(request: URLRequest) {
8 | let task = URLSession.shared.dataTask(with: request) { data, response, error in
| `- note: 'self' implicitly captured here
9 | let result = // process data...
10 | DispatchQueue.main.async { [weak self] in
| `- warning: 'weak' capture list item 'self' implicitly captured as strong reference in outer scope
11 | self?.handleResult(result)
12 | }
forum post regarding this change: https://forums.swift.org/t/request-for-feedback-emitting-warnings-when-weak-capture-list-items-cause-strong-captures/78390
Changing this behavior or adding a warning has been brought up many times over the years. I found the following existing references to this issue:
- An old mailing list proposal
- https://github.com/swiftlang/swift/issues/46390
- https://github.com/swiftlang/swift/issues/47470
- https://github.com/swiftlang/swift/issues/72391
- This recent blog post about a memory leak
cc @hborla @slavapestov @xedin this PR is still a work in progress, but if you can spare some time/attention I would appreciate feedback on the direction and anything I may have overlooked. Thanks in advance!
hello again @hborla @slavapestov @xedin – do you think any of you will have bandwidth to provide feedback on this in the relatively-near future? if not, is there anyone else you'd recommend i try to involve?
Sorry for the delay, @hamishknight is going to take a look at the changes soon!
Sorry I didn't get through the rest of this today, I'll try and get to it before the end of the week
@hamishknight if you have time to take another look at this, i'd love to hear the rest of your feedback & continue iterating on it
Sorry! I haven't forgotten about this, I will try again to get to it before the end of the week
@hamishknight i assume you've likely been fairly busy with 6.1 preparations, but any chance you've had time to take another look at this? if not, and you don't foresee having capacity to complete a full review soon, it would be helpful to get guidance on at least the following questions so that i could begin to address them sooner rather than later:
- should the diagnostic should be performed via multiple AST traversals rather than one? (outlined here)
- is adding a fixit of some form a blocker for this proposed change? (mentioned here)
@hamishknight made a few updates here:
- changed the logic to only diagnose when the capture item uses an 'implicit' binding syntax, i.e. where there's no explicit assignment to the new var. this allows a straightforward way to opt out if the code is intentional. also added a fixit to handle this case.
- added new notes to communicate how the diagnostic can be silenced (add a new capture item in an outer closure, or use an explicit assignment for the one that's been diagnosed)
- minor wordsmithing changes to the existing diagnostic text
i did an informal smoke test of the source compat suite with this new diag upgraded to an error and it... didn't finish (20 hrs and running), but also didn't fail 🤷.
i've mostly punted on extending the logic to local functions, but i'm guessing those are a fair bit less common in practice. similarly, i haven't tried to implement the 'insert new capture list item' fixit yet, but on the plus side, i'm much more familiar with this part of the codebase and think i have a better idea how it could be done than the last time i punted 😅.
in the spirit of not letting the perfect be the enemy of the good, if this looks reasonable to you i'd like to try and ship it, and then we can iterate on it further going forward. let me know what you think – thanks!
Thank you! So sorry for never getting back to this, it's back on my queue to review