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

Fix support for non-final classes in AtomicReference #58

Merged
merged 3 commits into from
Mar 20, 2023

Conversation

lorentey
Copy link
Member

@lorentey lorentey commented Sep 30, 2022

Swift 5.6/7 generates new compiler warnings when non-final classes conform to AtomicReference. The warnings are pointing out a real design issue: Self is covariant in protocols, but we intend atomic operations to always deal with the base class rather than individual subclasses.

Add a new associated type requirement to AtomicValue to represent the actual type that originally conformed to the protocol, and add explicit same-type requirements to clients to ensure they aren’t asked to return a subclass.

Unfortunately, this change is source breaking:

class Base: AtomicReference {}
class Derived: Base {}

let ref = ManagedAtomic<Derived?>(nil)
  // before: builds, but only because a type safety issue isn't caught by the compiler
  // after: 'ManagedAtomic' requires the types 'Derived' and 'Base' be equivalent

Checklist

  • I've read the Contribution Guidelines
  • My contributions are licensed under the Swift license.
  • I've followed the coding style of the rest of the project.
  • I've added tests covering all new code paths my change adds to the project (if appropriate).
  • I've verified that my change does not break any existing tests.
  • I've updated the documentation if necessary.

@lorentey
Copy link
Member Author

@swift-ci test

@lorentey lorentey linked an issue Sep 30, 2022 that may be closed by this pull request
Swift 5.6/7 generates new compiler warnings when non-final classes
conform to `AtomicReference`. The warnings are pointing out a real
design issue: Self is covariant in protocols, but we intend atomic
operations to always deal with the base class rather than individual
subclasses.

Add a new associated type requirement to AtomicValue to represent the
actual type that originally conformed to the protocol, and add
explicit same-type requirements to clients to ensure they aren’t
asked to return a subclass.

Unfortunately, this change is source breaking:

```
class Base: AtomicReference {}
class Derived: Base {}

let ref = ManagedAtomic<Derived?>(nil)
  // before: OK
  // after: 'ManagedAtomic' requires the types 'Derived' and 'Base' be equivalent
```
@lorentey lorentey force-pushed the fix-non-final-class-warnings branch 2 times, most recently from 263c074 to 4586994 Compare March 18, 2023 05:03
@lorentey lorentey force-pushed the fix-non-final-class-warnings branch from 4586994 to 5730529 Compare March 18, 2023 05:08
@lorentey
Copy link
Member Author

@swift-ci test

@lorentey lorentey added this to the 1.1.0 milestone Mar 18, 2023
@lorentey lorentey changed the title [Draft] Fix support for non-final classes in AtomicReference Fix support for non-final classes in AtomicReference Mar 18, 2023
@lorentey
Copy link
Member Author

lorentey commented Mar 18, 2023

The source breakage seems mostly theoretical; it feels like the risk may be low enough to ship this in a minor release, especially since the compiler warning in 5.7 is highlighting a real correctness issue.

(If it turns out this is too optimistic, then we might be able to do something about it, either by continuing to allow broken code in Swift 5.6 (only rejecting it in 5.7), or by tweaking UnsafeAtomic/ManagedAtomic to allow taking subclasses as their generic argument. I suspect the former option would require huge #if compiler(>=5.7) blocks, duplicating large swaths of code. The latter would require the addition of ambiguous overloads marked @_disfavoredOverload. So I'd rather do neither, if at all possible.)

@lorentey
Copy link
Member Author

@swift-ci test

lorentey added a commit to lorentey/swift-atomics that referenced this pull request Mar 20, 2023
@lorentey
Copy link
Member Author

@swift-ci test

@lorentey
Copy link
Member Author

@swift-ci test macOS platform

@lorentey
Copy link
Member Author

[7/22] Compiling Atomics AtomicOptionalRawRepresentable.swift
/build/swift-atomics/Sources/Atomics/AtomicOptionalRawRepresentable.swift:34:14: error: type of expression is ambiguous without more context
    _storage = Storage(value?.rawValue)
    ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~

Oh, a fix for that is in #73.

lorentey added a commit to lorentey/swift-atomics that referenced this pull request Mar 20, 2023
@lorentey
Copy link
Member Author

@swift-ci test

@lorentey lorentey merged commit ca8a8fc into apple:main Mar 20, 2023
@lorentey lorentey deleted the fix-non-final-class-warnings branch March 20, 2023 23:41
This pull request was closed.
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.

Swift 5.7/6 and Non-Final Class Opt-In to AtomicReference
1 participant