Proposal: Change rules for implicit captures due to nested closure captures


(Lily Ballard) #1

In Swift code today, when using nested closures, if the inner closure
weakly captures an object (e.g. `self`) that isn't otherwise captured by
the outer closure, the outer closure implicitly strongly captures the
object. This behavior is unlikely to be what the programmer intended,
and results in unwanted object lifetime extension without making it
obvious in the code.

In practice, you'll find this happening in code that looks like

class SomeViewController: UIViewController { // ... func
foo(url: NSURL) { let task =
NSURLSession.sharedSession().dataTaskWithURL(url) { data, response,
error in let result = // process data
dispatch_async(dispatch_get_main_queue()) { [weak self] in
self?.handleResult(result) } }
task.resume() } }

In here, at first glance it looks like the view controller is being
weakly referenced. But in actuality, the view controller is being
retained for the duration of the background processing, and is then only
converted to a weak reference at the moment where it tries to hop back
on to the main thread. So it's basically the worst of both worlds; the
view controller lives far longer than intended, but it goes away right
at the moment where it could be useful again. It's even worse if the
programmer expected the view controller's deinit() to cancel the network
task, as that can never happen. The fix for this code is to move the
`[weak self]` up to the outer closure, but since the outer closure never
actually touches self directly, it's not immediately obvious that this
is required.

My proposal is to change the rules so that whenever a closure captures
an object only because a nested closure did so, then the outer closure
should capture it using the same ownership semantics (this includes
unowned(unsafe), unowned(safe), weak, and strong). If there are multiple
nested closures that capture it, then we use the following rules:

* If all nested captures use the same ownership, then the outer capture
  uses that ownership.
* If any nested capture is strong, the outer capture is strong.
* If at least one nested capture is weak, and at least one capture is
  unowned or unowned(unsafe), the outer capture is strong. This is
  because there's no (safe) way to convert from weak -> unowned, or from
  unowned -> weak, and we should not crash upon the creation of the
  nested closure, so the outer capture must be strong.
* If at least one nested capture is unowned(unsafe), and at least one
  nested capture is unowned(safe), then the outer capture is
  unowned(safe).

This can be visualized with the following diagram, where the outer
closures uses the right-most node that covers all the children:

.--- weak <-------------------------------. /
\ strong < + no capture
\ / '--- unowned(safe)
<--- unowned(unsafe) --'

-Kevin Ballard


(Jordan Rose) #2

Good idea, Kevin. Something like this seems reasonable (though I admit I just skimmed the message). In the mean time, maybe we can add a warning for implicit captures with a different strength?

(I'd also be happy to treat "unowned" + "weak" as "error, be explicit" rather than "strong".)

There is one case where an explicit capture behaves differently from an implicit one: capturing local variables (as opposed to local constants). There's an example of this in the Swift Programming Language book, in the reference section: "Capture Lists <https://developer.apple.com/library/ios/documentation/Swift/Conceptual/Swift_Programming_Language/Expressions.html#//apple_ref/doc/uid/TP40014097-CH32-ID544>".

Jordan

···

On Dec 8, 2015, at 12:44, Kevin Ballard via swift-evolution <swift-evolution@swift.org> wrote:

In Swift code today, when using nested closures, if the inner closure weakly captures an object (e.g. `self`) that isn't otherwise captured by the outer closure, the outer closure implicitly strongly captures the object. This behavior is unlikely to be what the programmer intended, and results in unwanted object lifetime extension without making it obvious in the code.

In practice, you'll find this happening in code that looks like

class SomeViewController: UIViewController {
    // ...
    func foo(url: NSURL) {
        let task = NSURLSession.sharedSession().dataTaskWithURL(url) { data, response, error in
            let result = // process data
            dispatch_async(dispatch_get_main_queue()) { [weak self] in
                self?.handleResult(result)
            }
        }
        task.resume()
    }
}

In here, at first glance it looks like the view controller is being weakly referenced. But in actuality, the view controller is being retained for the duration of the background processing, and is then only converted to a weak reference at the moment where it tries to hop back on to the main thread. So it's basically the worst of both worlds; the view controller lives far longer than intended, but it goes away right at the moment where it could be useful again. It's even worse if the programmer expected the view controller's deinit() to cancel the network task, as that can never happen. The fix for this code is to move the `[weak self]` up to the outer closure, but since the outer closure never actually touches self directly, it's not immediately obvious that this is required.

My proposal is to change the rules so that whenever a closure captures an object only because a nested closure did so, then the outer closure should capture it using the same ownership semantics (this includes unowned(unsafe), unowned(safe), weak, and strong). If there are multiple nested closures that capture it, then we use the following rules:

* If all nested captures use the same ownership, then the outer capture uses that ownership.
* If any nested capture is strong, the outer capture is strong.
* If at least one nested capture is weak, and at least one capture is unowned or unowned(unsafe), the outer capture is strong. This is because there's no (safe) way to convert from weak -> unowned, or from unowned -> weak, and we should not crash upon the creation of the nested closure, so the outer capture must be strong.
* If at least one nested capture is unowned(unsafe), and at least one nested capture is unowned(safe), then the outer capture is unowned(safe).

This can be visualized with the following diagram, where the outer closures uses the right-most node that covers all the children:

         .--- weak <-------------------------------.
        / \
strong < + no capture
        \ /
         '--- unowned(safe) <--- unowned(unsafe) --'

-Kevin Ballard

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Lily Ballard) #3

Good idea, Kevin. Something like this seems reasonable (though I admit
I just skimmed the message). In the mean time, maybe we can add a
warning for implicit captures with a different strength?

A warning seems like a reasonable starting point. Maybe warn in 2.x, and
then switch over to the different capture rules for 3.x? That way we
wouldn't be changing the actual behavior of anyone's code until 3.x
(though I'm willing to bet that nearly all affected cases are
unintentional strong captures, but changing variable lifetimes can still
have unexpected consequences).

(I'd also be happy to treat "unowned" + "weak" as "error, be explicit"
rather than "strong".)

Good idea.

There is one case where an explicit capture behaves differently from
an implicit one: capturing local variables (as opposed to local
constants). There's an example of this in the Swift Programming
Language book, in the reference section: "Capture Lists[1]".

Good point. I was thinking primarily about captures of self, but the
rules would apply to other captures too.

I'd actually like us to have some means by which to explicitly list a
local variable in a capture list and still retain the capture-by-
reference semantics. I'm just not sure what that would look like.

That said, I'm not sure if this will be a problem for anyone. I'm trying
to come up with examples of code that will need to add an explicit
capture list to preserve current behavior, but that this capture list
will break an implicit by-reference capture in a way that matters. And
the best I can do is a contrived example that doesn't provide any
explanation for why it wants to maintain current behavior. Specifically,
the desired behavior here that breaks is having an outer closure capture
the variable by-reference, and then the inner closure constructs the
weak reference out of whatever value the variable has at the time the
outer closure is invoked:

class Obj: CustomDebugStringConvertible { var name: String
init(name: String) { self.name = name } var
debugDescription: String { return "Obj(name: \(String(reflecting:
name)))" } }

var obj: Obj = Obj(name: "foo") let f = { let g = { [weak obj] in
print("g: \(obj)") } g() } obj = Obj(name: "bar") f()

And even that can recover the existing behavior by just adding a `_ =
obj` inside of the outer closure (as opposed to adding a capture list).

I'd be interested to know if anyone can come up with a practical example
of code where this will be a problem.

-Kevin Ballard

···

On Tue, Dec 8, 2015, at 04:07 PM, Jordan Rose wrote:

On Dec 8, 2015, at 12:44, Kevin Ballard via swift-evolution <swift- >> evolution@swift.org> wrote:

In Swift code today, when using nested closures, if the inner closure
weakly captures an object (e.g. `self`) that isn't otherwise captured
by the outer closure, the outer closure implicitly strongly captures
the object. This behavior is unlikely to be what the programmer
intended, and results in unwanted object lifetime extension without
making it obvious in the code.

In practice, you'll find this happening in code that looks like

class SomeViewController: UIViewController { // ... func
foo(url: NSURL) { let task =
NSURLSession.sharedSession().dataTaskWithURL(url) { data, response,
error in let result = // process data
dispatch_async(dispatch_get_main_queue()) { [weak self] in
self?.handleResult(result) } }
task.resume() } }

In here, at first glance it looks like the view controller is being
weakly referenced. But in actuality, the view controller is being
retained for the duration of the background processing, and is then
only converted to a weak reference at the moment where it tries to
hop back on to the main thread. So it's basically the worst of both
worlds; the view controller lives far longer than intended, but it
goes away right at the moment where it could be useful again. It's
even worse if the programmer expected the view controller's deinit()
to cancel the network task, as that can never happen. The fix for
this code is to move the `[weak self]` up to the outer closure, but
since the outer closure never actually touches self directly, it's
not immediately obvious that this is required.

My proposal is to change the rules so that whenever a closure
captures an object only because a nested closure did so, then the
outer closure should capture it using the same ownership semantics
(this includes unowned(unsafe), unowned(safe), weak, and strong). If
there are multiple nested closures that capture it, then we use the
following rules:

* If all nested captures use the same ownership, then the outer
  capture uses that ownership.
* If any nested capture is strong, the outer capture is strong.
* If at least one nested capture is weak, and at least one capture is
  unowned or unowned(unsafe), the outer capture is strong. This is
  because there's no (safe) way to convert from weak -> unowned, or
  from unowned -> weak, and we should not crash upon the creation of
  the nested closure, so the outer capture must be strong.
* If at least one nested capture is unowned(unsafe), and at least one
  nested capture is unowned(safe), then the outer capture is
  unowned(safe).

This can be visualized with the following diagram, where the outer
closures uses the right-most node that covers all the children:

.--- weak <-------------------------------. /
\ strong < + no capture
\ / '---
unowned(safe) <--- unowned(unsafe) --'

-Kevin Ballard

_______________________________________________
swift-evolution mailing list swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

Links:

  1. https://developer.apple.com/library/ios/documentation/Swift/Conceptual/Swift_Programming_Language/Expressions.html#//apple_ref/doc/uid/TP40014097-CH32-ID544