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

Redo how ownership works #419

Merged
merged 7 commits into from
Apr 21, 2023
Merged

Redo how ownership works #419

merged 7 commits into from
Apr 21, 2023

Conversation

madsmtm
Copy link
Owner

@madsmtm madsmtm commented Feb 4, 2023

Add associated type ClassType::Mutability, which describes the mutability of an object. This replaces the Ownership parameter in Id<T, O> (so we're moving the mutability/ownership specification from use site to declaration site, which is a bit more restrictive, but much more succinct, as well as being easier to understand and harder to make mistakes).

Along with this, we can also finally fix NSCopying and NSMutableCopying in a way that ProtocolObject<dyn NSCopying> is allowed.

The rough idea was initially described in #399 (comment), see the code and changelog for details on how the exact design turned out.

@madsmtm madsmtm added enhancement New feature or request A-objc2 Affects the `objc2`, `objc-sys` and/or `objc2-encode` crates labels Feb 4, 2023
@madsmtm
Copy link
Owner Author

madsmtm commented Feb 4, 2023

Which kind of subclassing should be allowed (A -> B means kind A may have superclasses with kind B):

Unknown -> *

Immutable -> Immutable

ImmutableWithMutableSubclass -> ImmutableWithMutableSubclass + Mutable
Mutable -> Mutable

InteriorMutable -> InteriorMutable + MainThreadOnly
MainThreadOnly -> MainThreadOnly

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 13, 2023

I'm wondering how to assign things in icrate, and I actually think we don't really need Unknown, rather it should be called Root, and instead we should just default everything to InteriorMutable (except of course for the things that implement NSMutableCopying)?

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 13, 2023

Also, InteriorMutable and Immutable are currently equivalent? (apart from Send + Sync, but those should be implemented separately)

Though perhaps we can still use it in e.g. NSDictionary to only allow Immutable keys, thereby fixing #337?

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 13, 2023

I considered that it may be favourable to allow changing the class kind from the default to Mutable later on, and would like to make that a non-breaking change, but since we'd still need to change the methods from &self to &mut self, there is not really much value in that (e.g. might be relevant for NSFormatter?)

Changing from the default InteriorMutable to Immutable is safe though!

@madsmtm madsmtm added this to the objc2 v0.3 milestone Feb 14, 2023
@madsmtm madsmtm mentioned this pull request Mar 2, 2023
9 tasks
@madsmtm madsmtm force-pushed the redo-ownership branch 2 times, most recently from 0a5e889 to 708cef4 Compare April 2, 2023 12:42
@madsmtm madsmtm force-pushed the redo-ownership branch 8 times, most recently from 0d36fdd to 4b29af9 Compare April 15, 2023 09:28
@madsmtm madsmtm force-pushed the redo-ownership branch 6 times, most recently from 0c2e3b8 to 5552fdf Compare April 20, 2023 06:27
@madsmtm madsmtm force-pushed the redo-ownership branch 2 times, most recently from 1434eb6 to 01e2f42 Compare April 20, 2023 18:04
@madsmtm madsmtm marked this pull request as ready for review April 21, 2023 15:17
@madsmtm madsmtm mentioned this pull request Apr 21, 2023
@madsmtm madsmtm force-pushed the redo-ownership branch 4 times, most recently from a7080d1 to f08ab2b Compare April 21, 2023 18:40
Add associated type `ClassType::Mutability`, which describes the mutability of an object. This replaces the `Ownership` parameter in `Id<T, O>` (so we're moving the mutability/ownership specification from use site to declaration site, which is a bit more restrictive, but much more succinct, as well as being easier to understand and harder to make mistakes).

Along with this, we can also finally fix `NSCopying` and `NSMutableCopying` in a way that `ProtocolObject<dyn NSCopying>` is allowed.
These are clearly unsound, and even though the error messages are pretty terrible, it is still better to disallow them.
@madsmtm madsmtm merged commit 032809f into master Apr 21, 2023
19 checks passed
@madsmtm madsmtm deleted the redo-ownership branch April 21, 2023 19:41
@madsmtm
Copy link
Owner Author

madsmtm commented Apr 21, 2023

Tagging @simlay, @silvanshade and @mwcampbell as you might want to be notified of this change: Id no longer has an ownership parameter O, instead the information about ownership/mutability is moved to an associated constant ClassType::Mutability, which should feel significantly simpler to use.

The changelog should contain migration instructions, but please let me know if you run into any issues!

The change is primarily done because people (including myself) keep wanting to use &mut self in declare_class!, which is almost always unsound when overriding AppKit classes or implementing delegates - instead one should use &self + interior mutability (instance variables wrapped in Cell).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-objc2 Affects the `objc2`, `objc-sys` and/or `objc2-encode` crates enhancement New feature or request
Projects
None yet
1 participant