[Pitch] Declare capture semantics when passing a method as an escaping argument

Passing a function argument by referencing the function by name is very idiomatic, e.g.

func increment(_ x: Int) -> Int {
    x + 1
}

[1, 2, 3].map(increment)

Thus it is natural to do the same for methods in a class.

class Foo {
    private func increment(_ x: Int) -> Int {
        x + 1
    }

    func bar(_ xs: [Int]) -> [Int] {
        xs.map(increment)
    }
}

The above code works fine and there is not an issue in this case. But there is a subtle difference between the two examples. When you pass a method instead of a function, it creates a strong reference to self. This can be a problem in escaping closures which have the same lifecycle as whatever class, as then you can very easily create a reference cycle by accident. In my experience this comes up often when using RxSwift/Combine:

class MyViewController: UIViewController {
    let button = UIButton()
    var count = 0
    let label = UILabel()
    let subject = PassthroughSubject<Void, Never>()
    var bag = Set<AnyCancellable>()
    
    override func viewDidLoad() {
        super.viewDidLoad()
        
        button.addTarget(self, action: #selector(tapped), for: .touchUpInside)
        
        subject
            .sink(receiveValue: increment)
            .store(in: &bag)
    }
    
    func increment() {
        count += 1
        label.text = "\(count)"
    }
    
    @objc func tapped() {
        subject.send(())
    }
}

The problematic line is .sink(receiveValue: increment), which looks really clean but creates a strong reference to self, it should instead be .sink(receiveValue: { [weak self] in self?.increment }). Once you know about this pitfall, you will always write the latter, but I think the compiler could help.

I'm proposing some new syntax for when passing a method as an escaping closure argument. The existing syntax could continue to build, but Swift could emit a warning that a strong reference is created and suggest a fix. This would not be required on anything other than reference types (as I think it's a problem for actors too now?). I could see something like this:

.sink(receiveValue: weak increment)
.sink(receiveValue: weak self.increment)
.sink(receiveValue: strong increment)
.sink(receiveValue: strong increment)
.sink(receiveValue: increment) // warning, passing a method as an escaping closure, this creates a strong reference to self, use `strong` or `weak` to silence this warning.

weak could essentially just translate into .sink(receiveValue: { [weak self] in self?.increment }) and strong would just pass the method as-is.

The only real drawback I see is this creates more implicit behavior that if the instance of your class goes out of memory, then your method won't be called, but I think this is preferable to accidentally creating a reference cycle. Additionally it could emit a lot of warnings for existing projects but it seems best to explicitly mark each usage as strong/weak.

Definitely not a language designer though so I'm curious to hear your thoughts.

8 Likes

While reading this I’m thinking “we probably don’t need any code changes here, just a warning for passing a strong reference to an @escaping argument and some way to silence it” and then that’s basically what you just pitched, though the proposed solution does require code changes to silence it (I don’t know how else you’d do it.) Does a change that allows existing code to continue compiling albeit with a new Warning count as a nonbreaking change?

1 Like

I’m pretty strongly inclined to say “this feature was a mistake and everyone should be using closures instead”, and in fact I did say so recently: Swift Regret: Bound Methods // -dealloc

So if there were just a warning and fix-it here, the fix-it could be to insert the equivalent closure instead. But that’s disruptive enough that it should probably still go through the Evolution process.

5 Likes

What's that closure?

{ [self] in increment() }, with the appropriate arguments (and labels, if any). It shouldn’t automatically add weak, but it can make the capture explicit. (If this gets applied to non-escaping arguments too, the fix-it could omit the capture list in that case.)

5 Likes

What about methods that have a return type with a non-obvious default value? What happens when the capture is weak and the object is deallocated? For example:

func takesClosure(closure: () -> Int) { }

takesClosure(closure: weak self.closure)

Your suggestion must address this case.

1 Like

I think it would be better to separate this pitch into two parts.
The first part proposes only to emit a warning. It is easy to implement and the problem is well defined:

.sink(receiveValue: increment) // warning, passing a method as an escaping closure, this creates a strong reference to self, use `strong` or `weak` to silence this warning.

The second part is about changing the language. The discussion process will be long, and the review process too.

.sink(receiveValue: weak increment)
.sink(receiveValue: weak self.increment)
.sink(receiveValue: strong increment)
.sink(receiveValue: strong increment)
3 Likes

There's another issue, too: code size.

I was looking at code recently similar to this:

enum Delimiter {
  case foo
  case bar

  func write(to buffer: UnsafeMutableBufferPointer<UInt8>) {
    switch self {
      case .foo: // ...
      case .bar: // ...
    }
  }
}

func allocateAndWrite(capacity: Int, writer: (UnsafeMutableBufferPointer<UInt8>) -> Void) {
  let ptr: UnsafeMutableBufferPointer<UInt8> = // allocate space
  writer(ptr)
}

func writeThing() {
  allocateAndWrite(capacity: 100, writer: Delimiter.foo.write)
}

This was used quite a lot, and the writeThing function was generic so there were quite a few specialisations floating around, lots of branches which would sometimes use the delimiter writer but sometimes write something else, etc. Anyway, each call like this generated a bunch of thunks. Like, surprisingly many. I believe they were reabstraction thunks (?), and something about the demangled signature hinted that it was because the function wasn't the correct kind of closure.

Anyway, I managed to achieve a slight but noticeable performance improvement, and a reduction in code size, by rewriting it like this:

enum Delimiter {
  case foo
  case bar

  static func write(_ value: Self) -> (UnsafeMutableBufferPointer<UInt8>) -> Void
    switch value {
      case .foo: return { buffer in
        // write .foo
      }
      case .bar: return { buffer in
        // write .bar
      }
    }
  }
}

func writeThing() {
  allocateAndWrite(capacity: 100, writer: Delimiter.write(.foo))
}

Since then, I've become wary of bound methods. They look nice, but they come with a cost unless they get entirely inlined.

1 Like

Agreed, however this warning…

…references the code changes that would be the other half of the pitch. I guess it should say (and the fixit should be) how to change this into a capture list in current Swift syntax (I guess both an explicit strong reference and a weak one so the programmer can choose the appropriate fixit).

That's a great point. I would say that it would modify the return value to be optional so you would have to use an explicit closure in that case to come up with some default value for when self is deallocated. Although this does make me think that @jrose's suggestion of fixing it with a capture list which explicitly captures self is quite good, in this case the code would continue to work.

That makes sense to me.

The first part proposes only to emit a warning.

I think this is what I'd ultimately propose. I think that having some syntax for basic cases would make the code appear simpler, progressive disclosure and all, but after thinking about it more this doesn't come up that often to warrant an introduction of new syntax (and I don't think that https://fuckingclosuresyntax.com/ needs any new material). The primary motivation is the language makes it easy to shoot yourself in the foot and doesn't tell you that you are holding a gun.

2 Likes

a half way (although good enough) approach would be to require self for strong capturing, and resolve other cases with explicit closures:

.sink(receiveValue: increment) // warning .... fixit: 1) strong... 2) explicit...
  -->

.sink(receiveValue: self.increment) // fixit 1)

.sink { [<#T##capture mode#> self] in // fixit 2)
    guard let self = self else { return <#T##return value#> }
    <#T##todo#>
}
1 Like