Skip to content

Make core types Sendable and @unchecked Sendable#308

Merged
dfed merged 3 commits intomasterfrom
dfed--sendable
Apr 23, 2024
Merged

Make core types Sendable and @unchecked Sendable#308
dfed merged 3 commits intomasterfrom
dfed--sendable

Conversation

@dfed
Copy link
Copy Markdown
Collaborator

@dfed dfed commented Apr 22, 2024

We've been thread-safe since v1. We just hadn't told the compiler this fact yet.

This PR won't compile on Xcode 12.4 or prior, and we currently support back to Xcode 11. We therefore have two options:

  1. Drop support for these long-useless Xcode builds in a feature version. This is somewhat reasonable since you haven't been able to ship to the App Store with these versions for years. We also haven't run these old versions of Xcode in CI in many years.
  2. Roll a breaking change. I'd prefer to do that with more thought though: if we want a breaking change it'll be a bit before this can land + be released.

I'd be curious to hear from @pwesten and friends how they'd like to approach this before I go and update the README on this PR.

@dfed dfed self-assigned this Apr 22, 2024
@available(watchOS 3, *)
public final class SinglePromptSecureEnclaveValet: NSObject {
public final class SinglePromptSecureEnclaveValet: NSObject, @unchecked Sendable {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The localization context is a var on the type. It's only mutated within a lock, so we are indeed sendable. But the compiler won't be convinced of that... so we use unchecked.

Comment on lines +234 to +236
private var baseKeychainQuery: [String : AnyHashable] {
return service.generateBaseQuery()
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't love this, but AnyHashable isn't Sendable and service.generateBaseQuery() is fast, so in the tug-of-war between "have the compiler check the thing I know is safe" and "we know it's safe so opt out of checking to do something faster" I'm leaning towards the former.

@NickEntin
Copy link
Copy Markdown
Collaborator

This PR won't compile on Xcode 12.4 or prior, and we currently support back to Xcode 11. We therefore have two options:

  1. Drop support for these long-useless Xcode builds in a feature version. This is somewhat reasonable since you haven't been able to ship to the App Store with these versions for years. We also haven't run these old versions of Xcode in CI in many years.
  2. Roll a breaking change. I'd prefer to do that with more thought though: if we want a breaking change it'll be a bit before this can land + be released.

I generally lean towards calling dropped Xcode support a breaking change, but the "haven't been able to ship to the App Store with these versions for years" is an interesting mitigating factor. I'm okay with dropping that as a minor version bump. 👍

@dfed dfed marked this pull request as ready for review April 23, 2024 02:54
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