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

Make implementing ClassType in declare_class! safe #528

Open
madsmtm opened this issue Oct 26, 2023 · 2 comments
Open

Make implementing ClassType in declare_class! safe #528

madsmtm opened this issue Oct 26, 2023 · 2 comments
Labels
A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates enhancement New feature or request

Comments

@madsmtm
Copy link
Owner

madsmtm commented Oct 26, 2023

Implementing ClassType in declare_class! is unsafe because we need the user to uphold a few safety guarantees, and that was just the most convenient place to put the unsafe keyword.

After #521 though, the only safety guarantees that the user needs to uphold are:

  1. Any invariants that the superclass ClassType::Super may have must be upheld.
  2. ClassType::Mutability must be correct.
  3. Drop must be implemented correctly.

We should work on ways to make fulfilling these unsafe requirements more granular.

One possibility for requirement 2 would be to migrate from an associated type Mutability to a constant, that you must initialize with unsafe if you need certain features:

pub struct Mutability {}

impl Mutability {
    pub const fn interior_mutable() -> Self {
        todo!()
    }

    /// # Safety
    /// ... clearly detailed docs for when this is safe ...
    pub const unsafe fn main_thread_only() -> Self {
        todo!()
    }

    // ...
}

// Usage

declare_class!(
    struct MyClass;

    unsafe impl ClassType for MyClass {
        type Super = NSObject;
        const MUTABILITY: Mutability = Mutability::interior_mutable();
        // Or
        // SAFETY: ...
        // const MUTABILITY: Mutability = unsafe { Mutability::main_thread_only() };
        const NAME: &'static str = "MyClass";
    }

    // ...
);
@madsmtm madsmtm added enhancement New feature or request A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates labels Oct 26, 2023
@madsmtm madsmtm added this to the Polish icrate milestone Oct 26, 2023
@madsmtm
Copy link
Owner Author

madsmtm commented Nov 28, 2023

Hmm, just realized that the proposed idea won't work, since you could do const MUTABILITY: Mutability = NSView::MUTABILITY;, and thereby bypass the unsafe.

@madsmtm
Copy link
Owner Author

madsmtm commented Sep 17, 2024

Since #563, the associated Mutability type is changed to ThreadKind, and I've implemented a check to ensure that you can't set an incorrect thread kind.

I'm considering moving Drop to a fn dealloc(&self), to avoid the mutability issue if you call self.retain() inside dealloc. This would also be a slight perf hit, since you could no longer do things like self.ivars_mut().my_ivar.get_mut(), you'd still have to do interior mutability inside your deallocation method, but that's probably fine. Besides, instance variables themselves can still use &mut safely, it's just the class itself which can't.

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
Projects
None yet
Development

No branches or pull requests

1 participant