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 avoid using @unchecked Sendable #237

Merged
merged 5 commits into from Sep 20, 2023
Merged

Conversation

calda
Copy link
Member

@calda calda commented Sep 6, 2023

Summary

This PR proposes a new rule to prefer avoiding usage of @unchecked Sendable. Instead, we should use a standard Sendable conformance instead where possible. If working with a type from a module that has not yet been updated to support Swift Concurrency, suppress concurrency-related errors using @preconcurrency import.

Reasoning

@unchecked Sendable provides no guarantees about the thread safety of a type, and instead unsafely suppresses compiler errors related to concurrency checking.

There are typically other, safer methods for suppressing concurrency-related errors:

1. Use Sendable instead of @unchecked Sendable, with @MainActor if appropriate

A Sendable conformance is the preferred way to declare that a type is thread-safe. The compiler will emit an error if a type conforming to Sendable is not thread-safe. For example, simple value types and immutable classes can always safely conform to Sendable, but mutable classes cannot:

// RIGHT: Simple value types are thread-safe.
struct Planet: Sendable {
  var mass: Double
}

// RIGHT: Immutable classes are thread-safe.
final class Planet: Sendable {
  let mass: Double
}

// WRONG: Mutable classes are not thread-safe.
final class Planet: Sendable {
  // ERROR: stored property 'mass' of 'Sendable'-conforming class 'Planet' is mutable
  var mass: Double
}

// WRONG: @unchecked is unnecessary because the compiler can prove that the type is thread-safe.
struct Planet: @unchecked Sendable {
  var mass: Double
}

Mutable classes can be made Sendable and thread-safe if they are isolated to a single actor / thread / concurrency domain. Any mutable class can be made Sendable by isolating it to a global actor using an annotation like @MainActor (which isolates it to the main actor):

// RIGHT: A mutable class isolated to the main actor is thread-safe.
@MainActor
final class Planet: Sendable {
  var mass: Double
}

// WRONG: @unchecked Sendable is unsafe because mutable classes are not thread-safe.
struct Planet: @unchecked Sendable {
  var mass: Double
}

2. Use @preconcurrency import

If working with a non-Sendable type from a module that hasn't yet adopted Swift concurrency, suppress concurrency-related errors using @preconcurrency import.

/// Defined in `UniverseKit` module
class Planet: PlanetaryBody { 
  var star: Star
}
// WRONG: Unsafely marking a non-thread-safe class as Sendable only to suppress errors
import PlantaryBody

extension PlanetaryBody: @unchecked Sendable { }

// RIGHT
@preconcurreny import PlanetaryBody

3. Restructure code so the compiler can verify that it is thread-safe

If possible, restructure code so that the compiler can verify that it is thread safe. This lets you use a Sendable conformance instead of an unsafe @unchecked Sendable conformance.

When conforming to Sendable, the compiler will emit an error in the future if you attempt to make a change that is not thread-safe. This guaruntee is lost when using @unchecked Sendable, which makes it easier to accidentially introduce changes which are not thread-safe.

For example, given this set of classes:

class PlanetaryBody { 
  let mass: Double  
}

class Planet: PlanetaryBody { 
  let star: Star
}

// NOT IDEAL: no compiler-enforced thread safety.
extension PlanetaryBody: @unchecked Sendable { }

the compiler can't verify PlanetaryBody is Sendable because it is not final. Instead of using @unchecked Sendable, you could restructure the code to not use subclassing:

// BETTER: Compiler-enforced thread safety.
protocol PlanetaryBody: Sendable {
  var mass: Double { get }
}

final class Planet: PlanetaryBody, Sendable {
  let mass: Double
  let star: Star
}

Using @unchecked Sendable when necessary

Sometimes it is truly necessary to use @unchecked Sendable. In these cases, you can add a // swiftlint:disable:next no_unchecked_sendable annotation with an explanation for how we know the type is thread-safe, and why we have to use @unchecked Sendable instead of Sendable.

A canonical, safe use case of @unchecked Sendable is a class where the mutable state is protected by some other thread-safe mechanism like a lock. This type is thread-safe, but the compiler cannot verify this.

struct Atomic<Value> {
  /// `value` is thread-safe because it is manually protected by a lock.
  var value: Value { ... }
}

// WRONG: disallowed by linter
extension Atomic: @unchecked Sendable { }

// WRONG: suppressing lint error without an explanation
// swiftlint:disable:next no_unchecked_sendable
extension Atomic: @unchecked Sendable { }

// RIGHT: suppressing the linter with an explanation why the type is thread-safe
// Atomic is thread-safe because its underlying mutable state is protected by a lock.
// swiftlint:disable:next no_unchecked_sendable
extension Atomic: @unchecked Sendable { }

It is also reasonable to use @unchecked Sendable for types are thread-safe in existing usage but can't be refactored to support a proper Sendable conformance (e.g. due to backwards compatibility constraints):

class PlanetaryBody { 
  let mass: Double  
}

class Planet: PlanetaryBody { 
  let star: Star
}

// WRONG: disallowed by linter
extension PlanetaryBody: @unchecked Sendable { }

// WRONG: suppressing lint error without an explanation
// swiftlint:disable:next no_unchecked_sendable
extension PlanetaryBody: @unchecked Sendable { }

// RIGHT: suppressing the linter with an explanation why the type is thread-safe
// PlanetaryBody cannot conform to Sendable because it is non-final and has subclasses.
// PlanetaryBody itself is safely Sendable because it only consists of immutable values.
// All subclasses of PlanetaryBody are also simple immutable values, so are safely Sendable as well.
// swiftlint:disable:next no_unchecked_sendable
extension PlanetaryBody: @unchecked Sendable { }

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

@calda calda force-pushed the cal--no-unchecked-sendable branch 3 times, most recently from 4c8a1f8 to 02285a2 Compare September 11, 2023 21:58
@@ -36,16 +36,23 @@ custom_rules:
match_kinds:
- identifier
message: "Don't commit `print(…)`, `debugPrint(…)`, or `dump(…)` as they write to standard out in release. Either log to a dedicated logging system or silence this warning in debug-only scenarios explicitly using `// swiftlint:disable:next no_direct_standard_out_logs`"
severity: warning
severity: error
Copy link
Member Author

Choose a reason for hiding this comment

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

We use severity: error for all of these in the apps repo so I updated that here (including for the new no_unchecked_sendable)

README.md Outdated Show resolved Hide resolved
Co-authored-by: Andy Bartholomew <andy.bartholomew@airbnb.com>
@calda calda enabled auto-merge (squash) September 20, 2023 18:22
@calda calda merged commit 1522919 into master Sep 20, 2023
3 checks passed
@calda calda deleted the cal--no-unchecked-sendable branch September 20, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants