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

declare_class!: Safer initializers #438

Closed
madsmtm opened this issue Apr 13, 2023 · 10 comments · Fixed by #521
Closed

declare_class!: Safer initializers #438

madsmtm opened this issue Apr 13, 2023 · 10 comments · Fixed by #521
Labels
A-framework Affects the framework crates and the translator for them 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 Apr 13, 2023

Use something similar to Swift's rules for initializers, which we may be able to with a combination of types and a proc-macro, to verify that initializers only access the things that they are allowed to.

Should also somehow make ivars safe to declare, by statically ensuring that they are initialized - i.e. this issue ties in heavily with #414).

See also the Objective-C docs on initialization and a bit more about designated initializers.

@madsmtm madsmtm added enhancement New feature or request A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates A-framework Affects the framework crates and the translator for them labels Apr 13, 2023
@madsmtm
Copy link
Owner Author

madsmtm commented May 1, 2023

Interestingly, Objective-C recommends setting custom instance variables after calling the superclass' initializer, while Swift requires you to set them before (as part of their nice two-phase initialization scheme).

I can think of ways to express either in Rust:

// Types in this example put after `let` statements for clarity; they would be inferred in real life.
//
// Note also the `?` after the `msg_send_id`, which is possible since the initializer always (?) returns an Option

declare_class!(
    struct MyClass {
        ivar1: IvarEncode<i32>,
        ivar2: IvarDrop<Id<NSString>>,
    }

    // Now also generates:
    // struct MyClassIvars {
    //     ivar1: i32,
    //     ivar2: Id<NSString>,
    // }
    //
    // struct MyClassPartialInit;

    unsafe impl ClassType for MyClass {
        type Super = NSObject;
        type Mutability = InteriorMutable;
        const NAME: &'static str ="MyClass";
    }

    unsafe impl MyClass {
        // Objective-C style
        #[method_id(init:withArg:)]
        fn init(this: Allocated<Self>, arg: i32) -> Option<Id<Self>> {
            let this: MyClassPartialInit = unsafe { msg_send_id![super(this), init] }?;
            let this: Id<Self> = this.set(MyClassIvars {
                ivar1: arg,
                ivar2: NSString::new(),
            });
            // Calling methods on `this` is... Maybe allowed here?
            // Such methods may be overwritten in subclasses?
            Some(this)
        }

        // Swift style
        #[method_id(init:withArg:)]
        fn init(this: Allocated<Self>, arg: i32) -> Option<Id<Self>> {
            let this: MyClassPartialInit = this.set(MyClassIvars {
                ivar1: arg,
                ivar2: NSString::new(),
            });
            let this: Id<Self> = unsafe { msg_send_id![super(this), init] }?;
            // Calling methods on `this` is allowed here
            // Such methods may be overwritten in subclasses
            Some(this)
        }
    }
);

Unsure which approach would be best.

Note that in any case, by introducing a few helper types, we avoid the need to have a compiler to do the complex "only access instance variables once initialized" logic that Swift does.

@madsmtm
Copy link
Owner Author

madsmtm commented Jun 9, 2023

Would also be nice to allow calling other initalizers, e.g. delegate to a designated initializer:

declare_class!(
    // ...
    unsafe impl MyClass {
        fn designated(self: Allocated<Self>) -> Option<Id<Self>> { ... }

        fn convenience(self: Allocated<Self>) -> Option<Id<Self>> {
            printing!("convenience initializer");
            self.designated()
        }
    }
);

But since we also want to call obj.designated() on Option<Allocated<MyClass>>, perhaps we should move the option-ness of Allocated into itself?

@madsmtm
Copy link
Owner Author

madsmtm commented Jun 9, 2023

Partial initialization could be perhaps done with a struct PartialInit<T: ClassType>(T);.

This would require a new (though possibly internal) trait for declared classes, so that the set method will know which Ivar's to expect.

And also how to set it actually, since it needs to access the ivar offset.

@madsmtm
Copy link
Owner Author

madsmtm commented Jun 9, 2023

Safety-wise, we must require the users that declare ivars to override all designated initializers

@madsmtm
Copy link
Owner Author

madsmtm commented Jun 13, 2023

The benefit of Swift's initialization scheme is that after the super initializer has been called, it is always safe to call methods on the object, no matter if those have been overwritten in a subclass, and now potentially refers to the subclass' instance variables.

The benefit of Objective-C's initialization scheme is that classes that return something else from a super initializer will still work as expected. But perhaps this is only a problem when calling initializers outside of the implementation? E.g. [NSString alloc] returns a static NSPlaceHolderString*, but when subclassing NSString, a normal object is returned.

@madsmtm madsmtm modified the milestones: Polish icrate, Usable icrate Sep 5, 2023
@madsmtm
Copy link
Owner Author

madsmtm commented Sep 26, 2023

If both the class and the ivars implement Drop, then we'll need two drop flags in the Swift case: one for "instance variables are initialized", and one for "the class itself is initialized", while with Objective-C's scheme, we'd only need one drop flag.

@madsmtm
Copy link
Owner Author

madsmtm commented Sep 26, 2023

I think moving forward I'll be using Swift's initialization scheme, even if it may end up being slightly less efficient in certain cases.

I'd still like to create some test cases for Objective-C classes that override init to return a different object, and see how that works with subclassing from Swift, to check if they do something clever here.

@madsmtm
Copy link
Owner Author

madsmtm commented Sep 28, 2023

Okay, so going from PartialInit<T> to Id<T>, are there any paths other than calling an initializer on super? Or can we safely assume that once the super message send succeeds, then we change the drop flag for "the class itself is initialized".


As an optimization, I guess we could omit setting the drop flag when the ivars are initialized, and just always only set it when the entire class is? This would leak in the case of unwinds in the super initializer, but perhaps that's fine?

I'm wondering if we could somehow store a drop flag on the stack instead, perhaps inside the PartialInit struct? That way we could emit code to call the ivar deinitializer if an unwind happens, without having to set the drop flag twice? Though I should probably punt on that as a (potential) future optimization.

@madsmtm
Copy link
Owner Author

madsmtm commented Oct 3, 2023

We may need functionality to unsafely access the instance variables, and convert the Allocated to PartialInit, and PartialInit to Id?

I'm currently looking at NSCopying, which states that there are a few different ways to implement it, one of them being invoking [super copy] and setting the instance variables afterwards - this should ideally also somehow be supported in objc2 with msg_send_id! (although it should definitely be unsafe).

EDIT: NSCopying discussion moved to #534

@madsmtm
Copy link
Owner Author

madsmtm commented Dec 2, 2023

I'd still like to create some test cases for Objective-C classes that override init to return a different object, and see how that works with subclassing from Swift, to check if they do something clever here.

Turns out, they don't, and accesses to the object afterwards fails horribly, so I don't think we'll try to do anything different here (yet).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-framework Affects the framework crates and the translator for them 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