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

Change Allocated<T> in preparation for arbitrary self types #520

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

madsmtm
Copy link
Owner

@madsmtm madsmtm commented Sep 28, 2023

Arbitary self types (tracking issue: rust-lang/rust#44874) are not yet stable, but whatever form they end up as, we'll need to use a different type than Option<Allocated<T>>, since you can't (and won't be able to) use that as self: Option<Allocated<Self>>.

My proposal here is to introduce a new type, since we likely still need to be able to differentiate between "this type is newly allocated, and we don't know the nullability" (such as when sending to init methods) vs. "this type is allocated, and we know that it is non-null" (such as when receiving from init methods).

EDIT: See below for the alternative I'm going with.

@madsmtm madsmtm added the A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates label Sep 28, 2023
@madsmtm madsmtm added this to the Usable icrate milestone Sep 28, 2023
@madsmtm
Copy link
Owner Author

madsmtm commented Sep 28, 2023

I think there might be an alternative path:

We use struct Allocated<T>(Option<NonNull<T>>) everywhere, and then inside declare_class! when initializing instance variables, we do a NULL check to ensure that the allocation succeeded (e.g. we're not out of memory).

With a bit of macro trickery, inlining and LLVM optimizations, we can probably fairly reliably make it so that this NULL check is omitted when the declared method is called from Objective-C (since we know that receivers can't be null there), while still being included if someone initialize their instance variables outside of that.

Not sure if we want to base our design on non-guaranteed optimizations like this, though? On the other hand, initialization code inside declare_class! should be very linear with regards to Allocated, and OOM is a very, very rare situation (in contrast to e.g. initialization failure), so perhaps it's fine to hide it a bit more from our API?

I guess I'll have to experiment with it.

EDIT: See this gist (select "Show Assembly"), it seems likely that this idea will indeed work, so that's what I'll go with.

@madsmtm madsmtm force-pushed the option-allocated branch 3 times, most recently from b006791 to 58c5466 Compare September 28, 2023 13:31
@madsmtm madsmtm changed the title Change Option<Allocated<T>> in preparation for arbitrary self types Change Allocated<T> in preparation for arbitrary self types Sep 28, 2023
@madsmtm
Copy link
Owner Author

madsmtm commented Sep 28, 2023

Another benefit is that also allows making convenience initializers every so slightly easier:

declare_class!(
   ...

    #[initConvenience]
    fn init_convenience(this: Allocated<Self>) -> Id<Self> {
        // Before
        let this = unsafe { msg_send![Some(this), init] };
        // After
        let this = unsafe { msg_send![this, init] };

        // Do something with `this`
        this.ivar = 42;
        this
    }
);

Which is important for extern_methods! and declare_class! being more symmetric (which in turn will make #455 easier to progress on).

@madsmtm madsmtm marked this pull request as ready for review September 28, 2023 14:18
@madsmtm
Copy link
Owner Author

madsmtm commented Sep 28, 2023

Sadly, messaging init methods with the wrong return Id<R> compared to Allocated<T> (i.e. T != R) now gives worse diagnostics. I'm fairly sure this can be fixed, but it requires refactoring how message sending works (i.e. instead of using a trait, move MsgSendId to be inherent methods on RetainSemantics. Actually this has potential to improve diagnostics a lot).

But I'm going to go through with it anyway, the diagnostic is not that much worse.

@madsmtm madsmtm merged commit 045c53f into master Sep 28, 2023
19 checks passed
@madsmtm madsmtm deleted the option-allocated branch September 28, 2023 22:28
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant