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

Send and Sync for declared classes #634

Open
PaulDance opened this issue Jun 27, 2024 · 3 comments
Open

Send and Sync for declared classes #634

PaulDance opened this issue Jun 27, 2024 · 3 comments
Labels
A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates enhancement New feature or request question Further information is requested
Milestone

Comments

@PaulDance
Copy link
Contributor

Hi again!

This is a bit of a mix between a question and a feature suggestion, I guess.

Question part

When using objc2::declare_class!, values of the resulting type are never Send nor Sync, even when its instance variables type verifies <Self as DeclaredClass>::Ivars: Send + Sync, due to the use of ManuallyDrop. However, the objc2::mutability module's types seem to suggest that when the ivars are indeed Send + Sync, then the global type is so as well, but needs to be added manually. That last part feels quite scary to write, especially considering the ManuallyDrop in the wrapper type and regarding Sync, even when having a static_assertions::assert_impl_all!(MyClassIvars: Send, Sync); for safety.

Therefore, the questions:

  • Does "though such implementations must be provided manually" already concern a declared class or is it just for "special" types?
  • Is it actually sound to unsafe impl these traits on a declared class when the right conditions between the DeclaredClass::Ivars and ClassType::Mutability declarations are met?

Suggestion part

  • If it is indeed correct, maybe clarifying this part in objc2::declare_class!' and the obcj2::mutability types' documentations would be nice? It would be easy to do and would help reassuring users (amongst them me, that is) when they have to unsafe impl the concerned traits.
  • Similarly, wouldn't it be possible to automatically add these implementations when the aforementioned conditions are met? It could either be done in objc2::declare_class! by somehow conditionally do so depending on whether the traits are verified, or by adding blanket implementations resembling
    unsafe impl<DC> Send /* or Sync*/ for DC
    where
        DC: DeclaredClass,
        <DC as DeclaredClass>::Ivars: Send, // or Sync
        DC: ClassType<Mutability = Immutable /* or the other types adequately */>,
    {}
    if that's even possible, maybe with a temporary sub-trait or private sealed trait to make things work. Another possibility would be to wrap ManuallyDrop and implement the traits for it only when the contained ivars have them. This could help a lot in various situations with respect to safety, async being my primary concern, especially regarding the generated binding crates.
@madsmtm madsmtm added enhancement New feature or request question Further information is requested A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates labels Jun 27, 2024
@madsmtm
Copy link
Owner

madsmtm commented Jun 27, 2024

due to the use of ManuallyDrop

Actually, it's because the type is converted to roughly:

struct MyClass {
    superclass: NSObject,
    ivars: PhantomData<Ivars>,
}

And the superclass NSObject is neither Send nor Sync.

Does "though such implementations must be provided manually" already concern a declared class or is it just for "special" types?

Yeah, this applies to user-created classes too. Note that I'm working on changing a lot of things in that module, see #563.

Is it actually sound to unsafe impl these traits on a declared class when the right conditions between the DeclaredClass::Ivars and ClassType::Mutability declarations are met?

It's been a while since I looked into this myself, so the details are a bit out of my cache, but I think the requirements are:

  • The ivars must be Send / Sync.
  • The superclass must be thread safe. This is true for NSObject which most objects inherit, but we cannot mark that as Send / Sync, since that'd allow passing objects that are not thread safe to other threads (e.g. allow converting &NSView to &NSObject, pass that to another thread, and then upcast it into an &NSView again).

clarifying this in [...] documentations

Definitely won't close this before that is done ;)

automatically add these implementations

Yeah, that should be doable, ideally we'd mark NSObject as !Send + !Sync and take advantage of that in coherence, though I think we can do it using the fact that NSObject is declared as mutability::Root, and then wrap superclasses in a helper type to make the compiler figure out the rest.

I'll try to make this work some time after I resolve #563, thanks for the excellent suggestion!

@madsmtm madsmtm added this to the objc2 v0.6 milestone Jun 27, 2024
@PaulDance
Copy link
Contributor Author

Cool, thanks!

The superclass must be thread safe.

Out of curiosity, how can we determine this in the general case? The documentation does not really ever mention such low-level concern, or is it that it only does when there is concern for it?

For example, an exotic class I'll have to inherit from is NEFilterDataProvider, which does not mention anything close to thread safety. Note that this particular class is in practice never actually instantiated by the user program, but rather automagically by the NetworkExtension framework, meaning my concern will not apply to it as my program will never have to manipulate such objects, but it makes me wonder about other cases I might encounter in the future.

@madsmtm
Copy link
Owner

madsmtm commented Jun 28, 2024

how can we determine this in the general case

The only indicators for whether something is thread-safe is:
a. The documentation stating that it is.
b. The class being marked as NS_SWIFT_SENDABLE in the header.

  • This makes it conform to Swift's Sendable protocol.
  • And makes us implement Send and Sync for the class.

If either of these are not present, then you should generally assume the class to be thread-unsafe.

Specifically for NSObject, we can infer that since it has subclasses that are thread-safe, in itself it must be thread-safe (and also just because we know the implementation, and know that the operations in there are thread-safe).

You can apply similar reasoning to other classes, but it's difficult!

an exotic class I'll have to inherit from is NEFilterDataProvider, which does not mention anything close to thread safety

Yeah, there isn't really anything I can do to help with this, Objective-C is just by default thread unsafe, and Apple's APIs are underdocumented in this regard, that's just the sad state of affairs (though they are focused on it themselves, e.g. with the upcoming Swift 6 release they're really going all-in on thread safety in Swift).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants