Request for feedback: emitting warnings when weak capture list items cause strong captures

[jamieQ (Jamie)]

i have an outstanding draft PR that i've been trying to progress recently in which i'm looking to add diagnostics for situations like the following (example copied from this issue):

class ClosureWrapper {
    var closure: () -> Void = {}
    init() {
        self.closure = {
            let otherClosure: () -> Void = { [weak self] in
                self?.closure = {}
            }
        }
    }
    deinit { print("deinit") }
}

var instance: ClosureWrapper? = ClosureWrapper()
instance = nil // lost, but not deinitialized

i'd like to emit a warning diagnostic in such cases along the lines of:

test.swift:8:52: warning: 'weak' capture list item 'self' implicitly captured as strong reference in outer scope
  5 |     var closure: () -> Void = {}
  6 |     init() {
  7 |         self.closure = {
    |                        `- note: 'self' implicitly captured here
  8 |             let otherClosure: () -> Void = { [weak self] in
    |                                                    `- warning: 'weak' capture list item 'self' implicitly captured as strong reference in outer scope
  9 |                 self?.closure = {}
 10 |             }

the intent of the programmer in such cases is presumably to avoid strongly capturing the reference by using a weak capture list item, but since the capture list item is nested, its presence induces a strong capture in the outer closure.

in order to emit an appropriate warning, we need to be able to adequately characterize the situation we're looking to diagnose. colloquially it's something like:

1. there's a 'weakified'^[1] capture list item nested in an escaping closure
2. there are no other 'explicit' references to the 'weakified' capture's referent

to try and make this somewhat more precise, here's the algorithm as currently implemented in the PR:

📝 Click for some pseudocode

1. use an ASTWalker to walk expressions in MiscDiagnostics.cpp
   • when visiting a Decl (pre)
     • check if the decl is a closure list item with the properties:
       • it is nested in a parent escaping closure
       • it has weak/unowned reference ownership
       • it is initialized via a DeclRefExpr whose referenced Decl has strong reference ownership
     • if the Decl meets these criteria, append a WeakToStrongCaptureItemInfo entry that contains:
       • the VarDecl of the capture item
       • the ValueDecl of the item's referent
       • the DeclRefExpr from the capture binding
       • the ReferenceOwnership of the capture list item
   • when visiting an Expr (pre)
     • if the node is an AbstractClosureExpr
       • push it onto a stack tracking the current closure context
       • note whether it is an escaping closure
     • if the node is a DeclRefExpr
       • add it to a map from the current closure context to a set of all DeclRefExprs (transitively) contained within this closure context
   • when visiting an Expr (post)
     • if the node is an AbstractClosureExpr
       • pop the top closure of our stack
       • if the stack isn't empty, add the DREs from the popped context into the new top-of-stack
2. after collecting data from the ASTWalker, attempt to diagnose by doing the following:
   • for each recorded WeakToStrongCaptureItemInfo
     • walk up the DeclContext hierarchy starting from that of the capture item to that of its referent
     • when we encounter a DeclContext that is an AbstractClosureExpr:
       • if the context is both escaping and has none of its own capture list items that reference the original item's referent, record it as a candidate for the diagnostic
       • for each DeclRefExpr associated with this closure context, check if it:
         • refers to the capture item's referent
         • is not an expression from the initializer of a 'weakified' capture list item
       • if these properties hold, then the reference is assumed to be 'explicit' and imply an intentional strong capture of the 'weakified' capture item, so we will not diagnose it
       • conversely, if they do not, then we continue the search for additional references
       • once we finish the DeclContext walk, if we've found no 'explicit' references, then we emit the diagnostic warning

i'd appreciate input on any of the following (or, really, anything that comes to mind on this subject):

1. does the logic generally seem correct for the intent of the diagnostic?
2. does the current implementation algorithm seem reasonable?
   • it's almost certainly not the most efficient... but also perhaps not the easiest to follow.
   • i can imagine an alternate approach where we walk the AST multiple times; once to find things we may want to diagnose, and then again (perhaps repeatedly) to see if we should actually do so.
3. are there cases in which this will produce false positives or negatives?
   • e.g. local functions – i haven't thought too much about how these should be handled, or their impact on the current approach.

thanks in advance for your time and consideration!

---

1. i.e. a capture of a strong variable bound to a less-than-strong capture list item. alternative jargon suggestions welcome... ↩︎

[jamieQ (Jamie)]

update: it was suggested in code review that it may be simpler (both to understand and implement this diagnostic) if the diagnostic is produced whenever a weak capture re-binds a strongly-referenced declaration that is captured by an intervening escaping closure (and isn't itself in a capture list). this change allows the diagnostic algorithm to be simplified quite a bit (the PR has been updated with a new, more straightforward approach). it also means some cases that previously would not have produced warnings now will, e.g.

func escape(_ e: @escaping () -> Void) {}

class C {
  func f() {}

  func g() {
    escape {
           `- note: 'self' implicitly captured here
      _ = self // even though `self` is spelled out explicitly, we'd still diagnose this
      escape { [weak self] in _ = self }
                     `- warning: 'weak' capture implicitly captures 'self' as strong reference in outer scope
    }
  }
}

there are a couple questions i have about how to handle certain code patterns involving concurrency that i've outlined here which i'd appreciate any input on. additionally, any thoughts on the proposed change to emit the diagnostic in more cases, and any general editorial input on the wording would also be valuable.

[jamieQ (Jamie)]

update on this – i recently made some further progress in implementing this diagnostic. behaviorally the logic matches what i outlined in the prior post, but now there is also a suppression mechanism. if you explicitly assign the capture list binding then no diagnostic will be emitted, e.g.

// in a class method:
escapingClosure {
  // this implies you are aware of the strong capture in the outer scope
  // and will silence the diagnostic 👇
  escapingClosure { [weak self = self] in
    ...
  }
}

i ran the code through the source compatibility suite with the new warning upgrade to an error to see what failed to build, and these were the results:

Project	SHA	Error locations
Dimillian/ACHNBrowserUI	5b393fb	TurnipExchangeService.swift:61
AsyncNinja/AsyncNinja (5.0)	12887fc	EventSource_ToFuture.swift:55, :178, appleOS_Retainer.swift:90, :170, :231, :291, :404
badoo/Chatto	3e4b1a7	BaseChatViewController+Changes.swift:200
xmartlabs/Eureka	523af88	SelectorViewController.swift:194
SwifterSwift/SwifterSwift	ccdc4e4	UIImageViewExtensions.swift:40 (unowned variant)
vapor/sqlite-nio (dep of fluent-sqlite-driver & sqlite-kit)	tag 1.12.2	SQLiteConnection+Hooks.swift:498, :529, :558, :588, :619

i think several of these would not have been reported with the analysis pitched earlier where the diagnostic would have been suppressed if the capture list's referent was explicitly used elsewhere in an outer scope. but, they are also cases that might still be good to surface to developers – in some instances there is potential lifetime extension that may be undesirable.

i'm curious what other people think – given these cases what's your opinion on the 'signal to noise ratio'?

also, if you're willing, i'd appreciate more 'real world' feedback to help inform this – here's a toolchain that you can use to test out the changes (macos only. lmk if there's interest in a linux one and i'll see if i can get that sorted).

lastly, i'd like to solicit feedback on the diagnostic wording, and thoughts on rollout.

for an example like this:

func callback(_ c: @escaping () -> Void) {}

class Klass {
    var closure: () -> Void = {}

    func doit() {
        self.closure = {
            callback { [weak self] in
                _ = self
            }
        }
    }
}

the current wording is like:

947 |     func doit() {
948 |         self.closure = {
    |                        |- note: 'self' implicitly strongly captured here
    |                        `- note: add 'self' as a capture list item to silence
949 |             callback { [weak self] in
    |                              |- warning: 'weak' ownership of capture 'self' differs from implicitly-captured strong reference in outer scope
    |                              `- note: explicitly assign the capture list item to silence
950 |                 _ = self
951 |             }

what do you think?

regarding rollout – there are a couple options:

1. unconditionally enable it
2. make it a 'suppressed by default' diagnostic that you have to explicitly enable with a diagnotic control flag

i lean toward just enabling it by default.

in either case though, it should also have a new diagnostic group to allow upgrading/downgrading to/from an error if desired. any input on the naming of that group, or existing ones that might make sense to utilize? ImplicitStrongCapture or maybe just ImplicitCapture are what have been floating around in my mind.

[cal (Cal Stephens)]

Nice work on this change!

Would be great for this warning to be enabled by default. This is a pretty common issue. This came up enough at Airbnb that folks wrote a SwiftSyntax based linter to detect these sorts of patterns (among others). Having this built into the compiler is way better.

@jamieQ

ImplicitStrongCapture or maybe just ImplicitCapture are what have been floating around in my mind.

I like including “Strong capture” in the name to contrast with the corresponding explicit weak capture.

[cal (Cal Stephens)]

What’s the purpose of this weak self = self silencing mechanism?

escape {
  escape { [weak self = self] in // suppress the warning
    ...
  }
}

In a vacuum, that pattern isn’t especially meaningful, and it feels non-idiomatic to write a “redundant” capture RHS in that way.

Adding an explicit capture to the parent closure feels like a much more obvious way to acknowledge the capture and suppress the warning. The strong self capture will be obvious to all future readers of the code.

escape { [self] in // suppress the warning
  escape { [weak self] in
    ...
  }
}

This seems supported by your diagnostic, which is good. What’s the thinking around why the [weak self = self] suppression mechanism is also necessary?

[pyrtsa (Pyry Jahkola)]

@cal

escape { [self] in // suppress the warning
  escape { [weak self] in
    ...
  }
}

Great point! Another alternative way to suppress the warning is of course a [weak self] capture in the outer closure.

[jamieQ (Jamie)]

@cal

What’s the purpose of this weak self = self silencing mechanism?
[...] it feels non-idiomatic to write a “redundant” capture RHS in that way.

a few thoughts on this.

first, during review the logic was changed from a stronger check where it would warn if the weak capture created the only reference to its referent in an outer escaping closure. since the logic was relaxed we will now warn in more scenarios where there may not actually be anything problematic happening. in such cases it can be nice to have an 'escape hatch' where writing something a little unusual signals to the compiler that you 'know what you're doing'. using the weak x = x pattern to communicate this seemed good because:

1. i think it's fairly unusual in practice, so would 'stand out' a little bit, which could be a useful signal
2. it is semantically identical to the original code
   • [weak x] has almost exactly the same AST representation as [weak x = x]
3. it was easy to implement a fixit for (maybe not a compelling justification, but still true)

moving or introducing new capture list items however can change behavior more meaningfully. two examples that came up are this somewhat contrived one:

var obj = Obj()
escape { // ℹ️ suggest adding strong capture list item
  obj = Obj()
  escape { [weak obj] in // ⚠️ 
    obj?.doStuff()
  }
}

// if we add the outer strong capture, we've changed semantics:

var obj = Obj()
escape { [obj] in
  obj = Obj() // 🛑 Cannot assign to value: 'obj' is an immutable capture
  escape { [weak obj] in
    obj?.doStuff()
  }
}

and these cases which have weird potential interplay with isolation inference:

actor A {
  func f() {
    Task { // ℹ️ recommend a capture
        // if we add `[weak self]` then the Task become nonisolated
        escape { [weak self] in ... } // ⚠️
    }
  }

  static func paramIsolated(
    isolation anActor: isolated A
  ) {
    Task { // ℹ️ recommend a capture
        // both `[anActor]` & `[weak anActor]` change inference to nonisolated
        escape { [weak anActor] in ... } // ⚠️
    }
  }
}

probably in most cases the thing to do is still either hoist the capture, or add a new one in an outer scope. it's a future enhancement we could make to add fixits to offer those options.

[cal (Cal Stephens)]

Makes sense! Agreed the explicit strong capture version isn’t totally sufficient.