realm / realm/SwiftLint

Add rule to warn against @escaping retain cycle

Open
#776 7 comments 20 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

rule-request
Dominant language
Swift
Stars
19.7k
Forks
2.3k
Avg merge
1d 1h
Merged PRs (30d)
11

Description

I'm not quite sure if this is doable, or falls within the realm of what SwiftLint wants to warn against, but I recently ran into this issue and it seemed like in might be a common Swift "gotcha"

Consider the following (contrived) example:

class Sync {
  var callback: () -> Void = {}

  func perform(andThen completionHandler: @escaping () -> Void) {
    callback = completionHandler
  }
}

class Other {
  let sync = Sync()

  deinit {
    print("Deinit called")
  }

  func start() {
    sync.perform(andThen: cleanUp)
  }

  func cleanUp() {
    // Do cleanup
  }
}

If start() is called on an instance of Other, that instance will never deallocate because passing cleanUp to Sync results in a retain cycle. This can be remedied by changing the implementation like so:

sync.perform { [weak self] in
    self?.cleanUp()
}

This seems to happen often enough that it has led to the creation of a µFramework called Weakify, but that doesn't help you find occurrences of this issue. And at least in my case it involved a lot of head scratching to figure out what was going on.

Swift 3 made @nonescaping the default for closure parameters, which makes me think that if we can detect @escaping parameters we will have solved half the puzzle of "should SwiftLint emit a warning".

The other half of the puzzle is detecting that you're passing something to that parameter which will result in a retain cycle. My earlier example could also be expressed as a partially applied currying method:

sync.perform(andThen: Other.cleanUp(self))

So I imagine that there would be a way to detect the above example and then emit a warning.

In addition (but perhaps more complex to detect) would be the following case, in which a capture list is not provided, and self is strongly retained:

sync.perform {
    self.cleanUp()
}

I would appreciate any thoughts on this rule. Are there cases where the retain cycle would be desirable? Does it seem susceptible to false-positives? Does it seem doable?

Thank you very much for your time,
Noah

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

No source file, test, or entry point is named. Start by evaluating whether SwiftLint can detect @escaping parameters, method references, and closures that strongly capture self; done would mean a defined rule scope with acceptable false-positive behavior for the examples described.

Written by the indexing model from the issue text.

Assessment

Tech stack
swift
Domain
tooling
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.