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

Address @Shared sendability. #3329

Merged
merged 16 commits into from
Sep 6, 2024
Merged

Address @Shared sendability. #3329

merged 16 commits into from
Sep 6, 2024

Conversation

mbrandonw
Copy link
Member

No description provided.

@@ -87,7 +87,7 @@ let package = Package(
)

#if compiler(>=6)
for target in package.targets where target.type != .system {
for target in package.targets where target.type != .system && target.type != .test {
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this temporarily so that we don't get flooded with warnings in test targets, but maybe we should keep this around until we've addressed all/most warnings in TCA proper, and then we can later concentrate on tests?

@@ -7,9 +7,9 @@
///
/// See the article <doc:SharingState> for more information, in particular the
/// <doc:SharingState#Custom-persistence> section.
public protocol PersistenceReaderKey<Value> {
public protocol PersistenceReaderKey<Value>: Sendable {
Copy link
Member Author

Choose a reason for hiding this comment

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

Persistence keys need to be sendable because they are captured in some LockIsolated's, which currently require Sendable values and @Sendable transaction closures. If we weaken that, then perhaps this can be weakened too?

/// A type that can be loaded or subscribed to in an external system.
associatedtype Value
associatedtype Value: Sendable
Copy link
Member Author

Choose a reason for hiding this comment

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

Value is captured in a variety of sendable contexts, such as in the subscription endpoint for persistence strategies. So I think this does really need to be full blown Sendable.

@@ -154,7 +154,7 @@ extension PersistenceReaderKey {
/// A type defining a user defaults persistence strategy.
///
/// See ``PersistenceReaderKey/appStorage(_:)-4l5b`` to create values of this type.
public struct AppStorageKey<Value> {
public struct AppStorageKey<Value: Sendable>: Sendable {
private let lookup: any Lookup<Value>
private let key: String
private let store: UserDefaults
Copy link
Member Author

Choose a reason for hiding this comment

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

Technically UserDefaults is not Sendable even though the docs specifically say "User defaults is thread-safe". So, it should be ok to make this UncheckedSendable, but it was strangely causing some test failures. So I need to look at this again.

private var subscription: Shared<Value>.Subscription?
private var subscription: Shared<Value>.Subscription!
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'd prefer for this to be a let so that we don't have to do @unchecked Sendable above, but we need lazy initialization. And to make that clear I went ahead and made it an implicitly unwrapped optional.

Comment on lines 15 to 17
public struct Shared<Value: Sendable> {
private let reference: any Reference
private let keyPath: AnyKeyPath
Copy link
Member Author

Choose a reason for hiding this comment

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

Technically this should now be:

public struct Shared<Value: Sendable> {

…and we should drop the conditional conformance below. But there are some weird interactions with _AnyKeyPath. Let's chat about this today.

Comment on lines +414 to +417
fatalError("Conformance of 'Shared<Value>' to 'MutableCollection' is unavailable.")
}
set {
self._wrappedValue[position] = newValue.wrappedValue
fatalError("Conformance of 'Shared<Value>' to 'MutableCollection' is unavailable.")
Copy link
Member Author

Choose a reason for hiding this comment

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

This subscript is unavailable so it should be ok to fatalError. If we don't fatal error, then we get sendability warnings that can't be fixed because you can't conditionally conform to a protocol with a constraint against a marker protocol, like Sendable.

@@ -1,7 +1,7 @@
final class DefaultSubscript<Value>: Hashable {
var value: Value
final class DefaultSubscript<Value: Sendable>: Hashable, Sendable {
Copy link
Member

Choose a reason for hiding this comment

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

We know we want to remove these helpers in the future, but since they're use by store.scope let's split off a non-sendable version for that.

@mbrandonw mbrandonw merged commit 7c6fb26 into main Sep 6, 2024
8 checks passed
@mbrandonw mbrandonw deleted the shared-sendability branch September 6, 2024 20:28
mergify bot added a commit to cgrindel/rules_swift_package_manager that referenced this pull request Sep 16, 2024
…ure to from: "1.15.0" (#1230)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[pointfreeco/swift-composable-architecture](https://redirect.github.com/pointfreeco/swift-composable-architecture)
| minor | `from: "1.13.1"` -> `from: "1.15.0"` |

---

### Release Notes

<details>
<summary>pointfreeco/swift-composable-architecture
(pointfreeco/swift-composable-architecture)</summary>

###
[`v1.15.0`](https://redirect.github.com/pointfreeco/swift-composable-architecture/releases/tag/1.15.0)

[Compare
Source](https://redirect.github.com/pointfreeco/swift-composable-architecture/compare/1.14.0...1.15.0)

#### What's Changed

- Added: Complete Swift 6 Language Mode support
([pointfreeco/swift-composable-architecture#3282,
[pointfreeco/swift-composable-architecture#3318;
[pointfreeco/swift-composable-architecture#3317;
[pointfreeco/swift-composable-architecture#3321;
[pointfreeco/swift-composable-architecture#3325;
[pointfreeco/swift-composable-architecture#3326;
[pointfreeco/swift-composable-architecture#3320;
[pointfreeco/swift-composable-architecture#3333;
[pointfreeco/swift-composable-architecture#3329).
- Fixed: Warn when bindable store is sent a binding action without
having integrated with `BindingReducer`
([pointfreeco/swift-composable-architecture#3347).
- Fixed: Remove and lock subscriptions from the Combine subject powering
Store and Shared subscriptions (thanks
[@&#8203;iampatbrown](https://redirect.github.com/iampatbrown),
[pointfreeco/swift-composable-architecture#2699).
This should improve memory and avoid potential issues related to
concurrent access to the publisher.
- Fixed: Avoid potential deadlock in `Shared` by dispatching to the main
actor
([pointfreeco/swift-composable-architecture#3356).
- Fixed: Swift Testing support for concurrent tests by bucketing
cancellation identifiers to each `@Test`
([pointfreeco/swift-composable-architecture#3374).
- Infrastructure: Remove disfavored `NSObject.observe` overload in favor
of SwiftNavigation's `observe`
([pointfreeco/swift-composable-architecture#3316)
- Infrastructure: Fix typo and deprecated content in `Performance.md`
(thanks [@&#8203;MaraMincho](https://redirect.github.com/MaraMincho),
[pointfreeco/swift-composable-architecture#3323);
'WhatIsNavigation.md' fixes (thanks
[@&#8203;stealmh](https://redirect.github.com/stealmh),
[pointfreeco/swift-composable-architecture#3345);
fix typos (thanks
[@&#8203;stealmh](https://redirect.github.com/stealmh),
[pointfreeco/swift-composable-architecture#3349;
thanks [@&#8203;O-O-wl](https://redirect.github.com/O-O-wl),
[pointfreeco/swift-composable-architecture#3354);
replace deprecated `viewStore` with `store` in documentation (thanks
[@&#8203;qwerty3345](https://redirect.github.com/qwerty3345),
[pointfreeco/swift-composable-architecture#3341).
- Infrastructure: Added failing test to document behavior with
observation/identified array
([pointfreeco/swift-composable-architecture#3346).
- Infrastructure: Update `05-HigherOrderReducers` CaseStudies (thanks
[@&#8203;qwerty3345](https://redirect.github.com/qwerty3345),
[pointfreeco/swift-composable-architecture#3342);
update `04-Navigation-Lists-NavigateAndLoad` CaseStudies (thanks
[@&#8203;MaraMincho](https://redirect.github.com/MaraMincho),
[pointfreeco/swift-composable-architecture#3327).
- Infrastructure: Issue template updates
([pointfreeco/swift-composable-architecture#3363).
- Infrastructure: Apply `any` to all existential protocol uses (thanks
[@&#8203;qwerty3345](https://redirect.github.com/qwerty3345),
[pointfreeco/swift-composable-architecture#3370).
- Infrastructure: Improve CI job performance
([pointfreeco/swift-composable-architecture#3357).
- Infrastructure: Remove outdated `#if` branching in the library
([pointfreeco/swift-composable-architecture#3376).

#### New Contributors

- [@&#8203;stealmh](https://redirect.github.com/stealmh) made their
first contribution in
[pointfreeco/swift-composable-architecture#3345
- [@&#8203;O-O-wl](https://redirect.github.com/O-O-wl) made their first
contribution in
[pointfreeco/swift-composable-architecture#3354

**Full Changelog**:
pointfreeco/swift-composable-architecture@1.14.0...1.15.0

###
[`v1.14.0`](https://redirect.github.com/pointfreeco/swift-composable-architecture/releases/tag/1.14.0)

[Compare
Source](https://redirect.github.com/pointfreeco/swift-composable-architecture/compare/1.13.1...1.14.0)

#### What's Changed

- Added: Isolate `Store`, `TestStore`, and various view helpers to the
`@MainActor`
([pointfreeco/swift-composable-architecture#3277,
[pointfreeco/swift-composable-architecture#3283).
This has been done with `@preconcurrency` for backwards compatibility.
If you are using strict concurrency it may require you add `@MainActor`
annotations to any SwiftUI view helpers that access the store.
- Fixed: Don't eagerly dismiss an alert presented by another
([pointfreeco/swift-composable-architecture#3309).
- Infrastructure: Bump Swift Navigation version
([pointfreeco/swift-composable-architecture#3310).
- Infrastructure: Documentation fixes (thanks
[@&#8203;woohyunjin06](https://redirect.github.com/woohyunjin06),
[pointfreeco/swift-composable-architecture#3296;
[@&#8203;MaraMincho](https://redirect.github.com/MaraMincho),
[pointfreeco/swift-composable-architecture#3299).

#### New Contributors

- [@&#8203;woohyunjin06](https://redirect.github.com/woohyunjin06) made
their first contribution in
[pointfreeco/swift-composable-architecture#3296
- [@&#8203;MaraMincho](https://redirect.github.com/MaraMincho) made
their first contribution in
[pointfreeco/swift-composable-architecture#3299
- [@&#8203;qwerty3345](https://redirect.github.com/qwerty3345) made
their first contribution in
[pointfreeco/swift-composable-architecture#3305

**Full Changelog**:
pointfreeco/swift-composable-architecture@1.13.1...1.14.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config
help](https://redirect.github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://redirect.github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44MC4wIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: cgrindel-self-hosted-renovate[bot] <139595543+cgrindel-self-hosted-renovate[bot]@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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