[Sema]: diagnose implicit strong captures of weak capture list entries #77063
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Sorry for the delay, @hamishknight is going to take a look at the changes soon! |
lib/Sema/TypeCheckCaptures.cpp
Outdated
| Context.Diags.diagnose(weakifiedRef->getLoc(), | ||
| diag::implicit_nonstrong_to_strong_capture); | ||
| Context.Diags.diagnose(CaptureLoc, | ||
| diag::implicit_nonstrong_to_strong_capture_loc); |
There was a problem hiding this comment.
I think we also ought to have a fix-it to explicitly specify the strong capture to silence the diagnostic (we could also suggest moving the weak capture to the outer closure). For a local func, I suppose we could suggest explicitly rebinding the capture (i.e let x = x). That is a bit ugly for self though (i.e you'd need to let `self` = self
There was a problem hiding this comment.
a couple follow-up questions here:
regarding the fix-its – do you think having a fix-it is a blocking requirement for adding a diagnostic like this, or could it be done as a follow-up task? i'm not trying to be lazy, but the 'proper' solution to offer seems like it would be to splice in a weak capture list item into a parent closure (and potentially multiple of them...), and i don't have much domain knowledge about how to do that. offering to make the capture strong seems more straightforward (just delete some known source range), but also seems like the 'wrong' thing to push people towards, given the contexts in which i assume this diagnostic would actually be useful.
re: the local func suggestion – i don't follow the scenario you're alluding to here. would you mind spelling it out more explicitly?
There was a problem hiding this comment.
I'm happy for the fix-it to be done as a follow-up, it doesn't have to block this change
For the local function case I was thinking of something like:
class C {
func foo() {
func bar() { // implicit strong capture here
_ = { [weak self] in }
}
takesEscapingFn(bar)
}
}Although that being said, we don't currently diagnose the implicit use of self in such local functions.
There was a problem hiding this comment.
i think i'd prefer to split out fix-its as a distinct effort then, though if you have ideas for how the diagnostic wording could be changed to suggest a solution i'd be glad to hear them (tack some extra text onto the warning or note?).
in terms of the local funcs... i'm not quite sure what to do there. since they are sort of ignored in terms of other self-capture diagnostics, i'm inclined to at least start by mostly ignoring them. with the current implementation, we get the following (which is perhaps a sort of odd pattern – a local func in a closure):
func localFunc() {
escape {
`- note: 'self' implicitly captured here
func localFunc2() {
escape { [weak self] in }
`- warning: 'weak' capture implicitly captures 'self' as strong reference in outer scope
_ = self
}
}
}which is maybe a bit weird. we currently get no diagnostic in the example you provided. maybe the diagnostic walker could consider func decls in addition to closures?
0b3d1c9 to
47bf774
Compare
|
@swift-ci please smoke test |
4a6f35e to
5d5a8a6
Compare
There was a problem hiding this comment.
@hamishknight i think i addressed all your comments from the initial review (thanks!) – the unresolved ones have some clarifying questions from my end. i've updated the implementation to run as an expression pass per your suggestion, and i think the algorithm is more appropriate now as well.
i've left some more 'TODO' comments in the code which i'd appreciate your insights on, as well as a few PR comments/questions of my own. when you get a chance, another pass on the updated code would be great – thanks in advance!
|
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: |
|
So sorry again! I want to try and get through this PR this week |
Add a diagnostic to warn when a capture list item creates a weak or unowned binding to a strong Decl and in so doing induces an implicit strong capture of the Decl in a parent closure scope.
5d5a8a6 to
d084873
Compare
|
@swift-ci please smoke test macos |
f130230 to
34cc9e3
Compare
There was a problem hiding this comment.
@hamishknight i made another few passes to update things per your most recent feedback, and the logic for the diagnostic is quite a bit simpler now (IMO). i also added a few more tests with some additional 'TODO' comments that i'd be interested if you have any insights on. thanks in advance!
| } | ||
| } | ||
|
|
||
| // TODO: how to deal with @_implicitSelfCapture and isolated captures? |
There was a problem hiding this comment.
some additional context/thoughts on this:
the first is whether the implicit self capture annotation should interact in some way with any of this new diagnostic logic.
the second is that, today, moving an isolated parameter capture from being closed over implicitly (due to a reference within the closure) to being an explicit capture list item can change the inferred isolation of the closure. e.g.
func f(iso: isolated (any Actor)) async {
let x = {
iso.assertIsolated("x fail") // ✅
print("x passed")
await Task.yield()
}
let y = { [iso] in
iso.assertIsolated("y fail") // 🛑
print("y passed")
await Task.yield()
}
await x()
await y()
}
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:
the goal is to emit warnings for such cases along the lines of:
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:
[weak self]in nested closures results in strong capture without warning #72391