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

Making Id::retain safer #399

Closed
madsmtm opened this issue Jan 26, 2023 · 4 comments · Fixed by #419
Closed

Making Id::retain safer #399

madsmtm opened this issue Jan 26, 2023 · 4 comments · Fixed by #419
Labels
A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates enhancement New feature or request
Milestone

Comments

@madsmtm
Copy link
Owner

madsmtm commented Jan 26, 2023

For some things like NSMutableString (and by extension, NSString), calling Id::retain on &T cannot be safe, since there could be a mutable reference to it somewhere else in the program (and the lifetime is erased by calling Id::retain, that's kinda the whole point).

The solution there is to use NSCopying::copy to instead store a copy of your string (which is cheap when given &NSString, and correct when given &NSMutableString).

But for types like NSView and NSWindow, this is overly restrictive, since we know there can never be a mutable reference to these (they always use interior mutability); so perhaps we can make a trait for expressing this? (This comes up especially often in user-defined init methods).

@madsmtm madsmtm added enhancement New feature or request A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates labels Jan 26, 2023
This was referenced Jan 26, 2023
@madsmtm madsmtm added this to the objc2 v0.3 milestone Jan 27, 2023
@madsmtm
Copy link
Owner Author

madsmtm commented Feb 2, 2023

Making retain is safe for a specific class does require that no subclasses ever use Id<_, Owned>. Which is, well, hard!

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 2, 2023

This also relates to #401, #265 and #316.

I'm starting to cook up a scheme like this:

trait ClassKind: Sealed {}

// Fallback for everything that hasn't been specified
struct Unknown;
impl ClassKind for Unknown {}

/// `Id::retain` is safe
struct Immutable;
impl ClassKind for Immutable {}

/// Immutable, but disallows `Id::retain`
struct ImmutableWithMutableSubclass;
impl ClassKind for ImmutableWithMutableSubclass {}

/// Mutable, mutable methods use `&mut self`. `Id::retain` is not safe
struct Mutable;
impl ClassKind for Mutable {}

/// Mutable, mutable methods use `&self`. Not Send + Sync. `Id::retain` is safe
struct InteriorMutable;
impl ClassKind for InteriorMutable {}

/// Same as InteriorMutable, but only on the main thread. `Id::retain` is safe
struct MainThreadOnly;
impl ClassKind for MainThreadOnly {}

trait ClassType {
    type Kind: ClassKind;
    // ... Super + NAME

    fn alloc()
    where Self::Kind != MainThreadOnly || Unknown;

    fn alloc(mtm: MainThreadMarker)
    where Self::Kind == MainThreadOnly;
}

// Maybe even:
unsafe impl Send + Sync for Immutable {}
unsafe impl Send + Sync for ImmutableWithMutableSubclass {}
unsafe impl Send + Sync for Mutable {}

And with a bit of extra trait bounds, we could even ensure that e.g. an Immutable class cannot subclass a Mutable one.

Still very unsure about this!

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 2, 2023

With that, we could perhaps even get rid of the Ownership parameter on Id, by the observation that ownership is actually something inherent in the type, not distinct from it.

let mutable_string: Id<NSMutableString> = NSMutableString::new();

// These are safe, since their lifetime is still tied to the mutable string
let _: &mut NSString = &mut mutable_string;
let string: &NSString = &mutable_string;
// string.retain() would not be possible, since `NSString` is `ImmutableWithMutableSubclass`

// Implicitly relinquish "owned" ownership of the variable here
let string: Id<NSString> = Id::into_super(mutable_string);
// So retaining the `NSMutableString` here is still safe, we statically know that nothing can modify the string any more
let string2: Id<NSString> = string.clone();

And for custom subclasses on NSObject:

declare_class!(
    struct MyClass {
        ivar: IvarEncode<u32, "_ivar">,
    }

    mod ivars;

    unsafe impl ClassType for MyClass {
        type Kind = Mutable;
        type Super = NSObject;
    }
    
    unsafe impl MyClass {
        #[method_id(init)]
        fn init(this: Allocated<Self>) -> Option<Id<Self>> {
            let this = unsafe { msg_send_id![super(this), init] }?;
            Ivar::write(&mut this.ivar, 5);
            Some(this)
        }
    }
);

extern_methods!(
    unsafe impl MyClass {
        #[method_id(new)]
        fn new() -> Option<Id<Self>>;
    }
);

let obj = MyClass::new()?;
*obj.ivar = 2; // Allowed, the object is mutable
// let obj2 = obj.clone(); // Disallowed

And custom subclasses on NSView:

declare_class!(
    struct MyView {
        ivar: IvarEncode<Cell<u32>, "_ivar">,
    }

    mod ivars;

    unsafe impl ClassType for MyView {
        type Kind = MainThreadOnly;
        type Super = NSView;
    }
    
    unsafe impl MyView {
        #[method_id(init)]
        fn init(this: Allocated<Self>) -> Option<Id<Self>> {
            let this = unsafe { msg_send_id![super(this), init] }?;
            Ivar::write(&mut this.ivar, Cell::new(5)); // Ideally, mutability would be allowed in `init`
            Some(this)
        }
    }
);

let obj = MyView::new()?;
// *obj.ivar = 2; // Disallowed, the object itself is immutable
obj.ivar.set(2); // But we can still use `Cell`'s powers
let obj2 = obj.clone(); // Also allowed

// But `obj` would also be !Send + !Sync

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 2, 2023

Crazy idea: We could extend msg_send!/extern_methods! parameters to allow Id<T> (and just drop it after the message send), such that taking a Mutable class as argument, would usually be unsafe since we could easily modify the class afterwards, now becomes safe!

Moved to #439.

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

Successfully merging a pull request may close this issue.

1 participant