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

Development Compiler
,
1

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... ↩ī¸Ž

4 Likes