[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

5 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.

2 Likes

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.