[Pitch] Add warning to move capture list to from inner closure to outer closure whenever the outer closure creates an implicit strong reference to the captured object

Motivation 1: Retain cycles

Let's say I have the following type:

class Foo {
    
    lazy var outerClosure = {
        let innerClosure = { [weak self] in
            
        }
    }

    init() {
        outerClosure()
    }

    deinit {
        print("deinit")
    }
}

And I run the following code:

var fooInstance: Foo? = Foo()
fooInstance = nil
print("fooInstance set to nil")

The output is

"fooInstance set to nil"

"deinit" is never printed, so there is a retain cycle. However, if I change my outerClosure lazy var to this:

lazy var outerClosure = {
    let innerClosure = {
            
    }
}

The output is this:

deinit
fooInstance set to nil

Even though the only thing I changed was removing [weak self] in, so [weak self] in is indirectly causing a retain cycle, even though the language guide advises it's supposed to do the opposite:

A weak reference is a reference that doesn’t keep a strong hold on the instance it refers to, and so doesn’t stop ARC from disposing of the referenced instance. This behavior prevents the reference from becoming part of a strong reference cycle.

As better explained by @wtedst, what's causing the retain cycle is the outer closure needs to capture self before passing it into the inner closure.

Motivation 2: Avoid unwanted object lifetime extension

If I change Foo to:

class Foo {

    init() {
        DispatchQueue.main.async {
            print("escaping closure starts executing")
            let innerClosure = { [weak self] in
                print(self == nil)
            }
            innerClosure()
            print("escaping closure about to be deallocated")
        }
    }

    deinit {
        print("deinit")
    }
}

The output is:

fooInstance set to nil
escaping closure starts executing
false
escaping closure about to be deallocated
deinit

Even though there’s no retain cycle here, I would not have expected the lifetime of self to be extended by the lifetime of the escaping closure. I'm probably not the only one, so I think my proposed solution would make this behaviour more clear.

Potential solution (Not feasible): Make these implicit captures weak if they are generated by a weak capture

This would change the behaviour of people’s software, so I don’t think this is possible.

Proposed solution: Add warning to move capture list to from inner closure to outer closure whenever the outer closure creates an implicit strong reference to the captured object

Warning: object captured weakly from an outer closure that doesn't explicitly capture the object.
Capture the object in the outer closure instead to silence this warning.
If unspecified, strong capture will be used.

To silence the warning, the user can change their code to this:

lazy var outerClosure = { [weak self] in
    let innerClosure = {
            
    }
}

Or If they want the existing behaviour without a warning, they can do this:

lazy var outerClosure = { [self] in
    let innerClosure = { [weak self] in
            
    }
}

If self is already captured within the body like this, I would expect no warning:

lazy var outerClosure = {
    print(self)
    let innerClosure = { [weak self] in
            
    }
}

Final thoughts

This will create quite a few warnings in people's projects, however in addition to stopping weak indirectly causing retain cycles, it will inform devs of how Swift capture semantics work.

Haven't seen any other discussion on Swift forums about this issue? Would love to hear any feedback or alternate ways we can solve this problem.

I'd hope this is possible as theoretically we can display the warning whenever the compiler generates the implicit strong reference. However, I do wonder if I've overlooked a case where the warning would be inaccurate

4 Likes

This is not entirely true: what causing the retain cycle is a strong capture into the outer closure (escapingClosure), as it needs to capture self before passing it into the inner closure. When you remove the [weak self] capture, neither closure uses self anymore, so the outer closure won't strongly capture it. What happens in your example is effectively equivalent to

lazy var escapingClosure = { [self] in // strongly capture `self`
    let innerClosure = { [weak self] in
    
    }
}

as you note later. The capture list is only released when the closure itself gets released, so as long as escapingClosure stays in memory, it doesn't matter at all what happens inside it and whether any further captures are weak or unowned.

I would counter that: it is the default behaviour of closures to keep their capture list alive as long as they are in memory, and it is the expected behaviour; that's why we can modify it with weak or unowned if we want it to change.

This is not possible not only due to the potential source breakage, but because oftentimes it is impossible to know at compile time if there's anything inside the closure that captures it strongly or not (you may create a weakly and a strongly capturing closure in different branches of an if-else statement, for instance).


But otherwise I agree that there's a room to make this behaviour more visible. A warning suggesting to introduce an explicit [self] in into the outer closure sounds good to me.

2 Likes

Thanks for the input! I've clarified that [weak self] is only indirectly causing a retain cycle.

I guess maybe it's just not what I would've expected, I'll change my assertion here.

One other thing: I've since realised that my lazy var escapingClosure was not an escaping closure, so I've renamed it to outerClosure, so just a heads up to anyone reading this as to why Nikita's reply uses this name.

Great pitch, I like it. It fits the safety direction of Swift.
These are common mistakes we see in pull requests. In callback heavy code it is easy to make such a mistake.
I am sure such sorts of mistakes have to be caught by the compiler.

1 Like

Typical case:

dispatchGroup.notify(queue: .global(qos: .userInitiated)) { [weak self] in 
  /// lots of if-else statements
  DispatchQueue.main.async { [weak self] in 
    self?.doSmth() 
  }
}

The following code:

processState { [weak self] state in
  switch state {
  case .orderEditOperation:
   DispatchQueue.main.async { [weak self] in self?.doSmth() }
    
  case .idle:
    if condition {
      DispatchQueue.main.async { [weak self] in self?.doSmth() }
    } else {
        guard let orderId = state.orderId else {
          DispatchQueue.main.async { [weak self] in self?.handleError() }
          return
        }
            
      nextNetworkRequest(orderId) { [weak self] result in
        guard let self = self, case .orderOperation = self.state else {
          DispatchQueue.main.async { [weak self] in self?.handleError() }
          return
        }
        
        DispatchQueue.main.async { [weak self] in self?.doSmth() }
      }
    }
  }
}

will look like

processState { state in // no need to write [weak self]
  switch state {
  case .orderEditOperation:
   DispatchQueue.main.async { [weak self] in self?.doSmth() }
    
  case .idle:
    if condition {
      DispatchQueue.main.async { [weak self] in self?.doSmth() }
    } else {
        guard let orderId = state.orderId else {
          DispatchQueue.main.async { [weak self] in self?.handleError() }
          return
        }
            
      nextNetworkRequest(orderId) { result in // no need to write [weak self]
        guard let self = self, case .orderOperation = self.state else {
          DispatchQueue.main.async { [weak self] in self?.handleError() }
          return
        }
        
        DispatchQueue.main.async { [weak self] in self?.doSmth() }
      }
    }
  }
}

or we can make inner self references weak if outer one is weak:

processState { [weak self] state in
  switch state {
  case .orderEditOperation:
   DispatchQueue.main.async { self?.doSmth() }
    
  case .idle:
    if condition {
      DispatchQueue.main.async { self?.doSmth() }
    } else {
        guard let orderId = state.orderId else {
          DispatchQueue.main.async { self?.handleError() }
          return
        }
            
      nextNetworkRequest(orderId) { result in
        guard let self = self, case .orderOperation = self.state else {
          DispatchQueue.main.async { self?.handleError() }
          return
        }
        
        DispatchQueue.main.async { self?.doSmth() }
      }
    }
  }
}

Both variants prevents from mistakes, but second one looks much more clear.

Hey Dmitry, thanks for the input!

My pitch as I envision it would show a warning here if you didn't write either [self] in or [weak self] in here, because this is the outermost closure and self isn't captured explicitly. Are you still in favour considering this?

I would also ideally like us not to have to capture weak self here, but that would be a source breaking change and is apparently not possible:

2 Likes

Warning is good solution, I didn't keep in mind ABI. Thanks for paying attention to this.

Terms of Service

Privacy Policy

Cookie Policy