Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rule to prefer weak captures over unowned captures #238

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

calda
Copy link
Member

@calda calda commented Sep 6, 2023

Summary

This PR proposes a new rule to prefer using weak captures over unowned captures.

Reasoning

unowned captures are unsafe because they will cause the application to crash if the referenced object has been deallocated. weak captures are safer because they require the author to explicitly consider and handle the case where the referenced object no longer exists.

// WRONG: Crashes if `planet` has been deallocated when the closure is called.

spaceship.travel(to: planet, onArrival: { [unowned planet] in
  planet.colonize()
})

spaceship.travel(to: planet, nextDestination: { [unowned planet] in
  planet.moons.first ?? planet.sun
})
// RIGHT: Explicitly handles case where `planet` has been deallocated.

spaceship.travel(to: planet, onArrival: { [weak planet] in
  guard let planet else { return }
  planet.colonize()
})

// For closures that return a non-optional value, you could either 
// use `fatalError` to avoid having to return anything:
spaceship.travel(to: planet, nextDestination: { [weak planet] in
  guard let planet else {
    fatalError("Planet was unexpectedly deallocated before reached by spaceship")
  }
  
  return planet.moons.first ?? planet.sun
})

// Or you could return a placeholder value with an optional `assertionFailure`:
spaceship.travel(to: planet, nextDestination: { [weak planet] in     
  guard let planet else {
    assertionFailure("Planet was unexpectedly deallocated before reached by spaceship")
    return Planet()
  }
  
  return planet.moons.first ?? planet.sun
})

Please react with 👍/👎 if you agree or disagree with this proposal.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jqsilver jqsilver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes just to get this back into your queue.

@calda calda requested a review from jqsilver September 20, 2023 18:22
@calda calda merged commit fab63dd into master Sep 20, 2023
3 checks passed
@calda calda deleted the cal--no-unowned-captures branch September 20, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants