Redo how ownership/mutability works

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.

Author: madsmtm

Cargo.lock

@@ -144,7 +144,7 @@ correctly.
 The `NSData` example again.
 
 ```rust
-let obj: Id<NSObject, Shared> = unsafe { msg_send_id![class!(NSData), new] };
+let obj: Id<NSObject> = unsafe { msg_send_id![class!(NSData), new] };
 let length: NSUInteger = unsafe { msg_send![&obj, length] };
 // `obj` goes out of scope, `release` is automatically sent to the object
 ```
@@ -181,13 +181,14 @@ extern_class!(
 
     unsafe impl ClassType for NSData {
         type Super = NSObject;
+        type Mutability = ImmutableWithMutableSubclass<NSMutableData>;
     }
 );
 
 extern_methods!(
     unsafe impl NSData {
         #[method_id(new)]
-        fn new() -> Id<Self, Shared>;
+        fn new() -> Id<Self>;
 
         #[method(length)]
         fn length(&self) -> NSUInteger;

LAYERED_SAFETY.md

@@ -7,7 +7,6 @@ use crate::file::File;
 use crate::id::ItemIdentifier;
 use crate::method::Method;
 use crate::output::Output;
-use crate::rust_type::Ownership;
 use crate::stmt::Stmt;
 
 #[derive(Debug, PartialEq, Clone)]
@@ -37,14 +36,12 @@ impl ClassCache {
 #[derive(Debug, PartialEq, Clone)]
 pub struct Cache<'a> {
     classes: BTreeMap<ItemIdentifier, ClassCache>,
-    ownership_map: BTreeMap<String, Ownership>,
     config: &'a Config,
 }
 
 impl<'a> Cache<'a> {
     pub fn new(output: &Output, config: &'a Config) -> Self {
         let mut classes: BTreeMap<_, ClassCache> = BTreeMap::new();
-        let mut ownership_map: BTreeMap<_, Ownership> = BTreeMap::new();
 
         for (name, library) in &output.libraries {
             let _span = debug_span!("library", name).entered();
@@ -55,20 +52,11 @@ impl<'a> Cache<'a> {
                         let cache = classes.entry(cls.clone()).or_default();
                         cache.to_emit.push(method_cache);
                     }
-                    if let Stmt::ClassDecl { id, ownership, .. } = stmt {
-                        if *ownership != Ownership::default() {
-                            ownership_map.insert(id.name.clone(), ownership.clone());
-                        }
-                    }
                 }
             }
         }
 
-        Self {
-            classes,
-            ownership_map,
-            config,
-        }
+        Self { classes, config }
     }
 
     fn cache_stmt(stmt: &Stmt) -> Option<(&ItemIdentifier, MethodCache)> {
@@ -159,6 +147,7 @@ impl<'a> Cache<'a> {
 
         let mut new_stmts = Vec::new();
         for stmt in &mut file.stmts {
+            #[allow(clippy::single_match)] // There will be others
             match stmt {
                 Stmt::ClassDecl {
                     id,
@@ -182,7 +171,7 @@ impl<'a> Cache<'a> {
                     for (superclass, _) in &*superclasses {
                         if let Some(cache) = self.classes.get(superclass) {
                             new_stmts.extend(cache.to_emit.iter().filter_map(|cache| {
-                                let mut methods: Vec<_> = cache
+                                let methods: Vec<_> = cache
                                     .methods
                                     .iter()
                                     .filter(|method| !seen_methods.contains(&method.id()))
@@ -197,8 +186,6 @@ impl<'a> Cache<'a> {
                                     return None;
                                 }
 
-                                self.update_methods(&mut methods, &id.name);
-
                                 Some(Stmt::Methods {
                                     cls: id.clone(),
                                     generics: generics.clone(),
@@ -217,12 +204,6 @@ impl<'a> Cache<'a> {
                         }
                     }
                 }
-                Stmt::Methods { cls, methods, .. } => {
-                    self.update_methods(methods, &cls.name);
-                }
-                Stmt::ProtocolDecl { id, methods, .. } => {
-                    self.update_methods(methods, &id.name);
-                }
                 _ => {}
             }
         }
@@ -257,17 +238,4 @@ impl<'a> Cache<'a> {
             file.stmts.push(stmt);
         }
     }
-
-    fn update_methods(&self, methods: &mut [Method], self_means: &str) {
-        for method in methods {
-            // Beware! We make instance methods return `Owned` as well, though
-            // those are basically never safe (since they'd refer to mutable
-            // data without a lifetime tied to the primary owner).
-            method.result_type.set_ownership(|name| {
-                let name = if name == "Self" { self_means } else { name };
-
-                self.ownership_map.get(name).cloned().unwrap_or_default()
-            });
-        }
-    }
 }

crates/header-translator/src/cache.rs

@@ -6,8 +6,7 @@ use std::path::Path;
 use serde::Deserialize;
 
 use crate::data;
-use crate::rust_type::Ownership;
-use crate::stmt::Derives;
+use crate::stmt::{Derives, Mutability};
 
 #[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
 #[serde(deny_unknown_fields)]
@@ -96,9 +95,8 @@ pub struct ClassData {
     pub categories: HashMap<String, CategoryData>,
     #[serde(default)]
     pub derives: Derives,
-    #[serde(rename = "owned")]
-    #[serde(default)]
-    pub ownership: Ownership,
+    #[serde(skip)]
+    pub mutability: Mutability,
     #[serde(rename = "skipped-protocols")]
     #[serde(default)]
     pub skipped_protocols: HashSet<String>,

crates/header-translator/src/config.rs

@@ -1,2 +1,5 @@
 data! {
+    // Subclasses `NSMutableAttributedString`, though I think this should
+    // actually be `InteriorMutable`?
+    class NSTextStorage: Mutable {}
 }

crates/header-translator/src/data/AppKit.rs

@@ -1,9 +1,12 @@
 data! {
-    class NSArray {
+    // SAFETY: `new` or `initWithObjects:` may choose to deduplicate arrays,
+    // and returning mutable references to those would be unsound - hence
+    // `NSArray` cannot be mutable.
+    class NSArray: ImmutableWithMutableSubclass<Foundation::NSMutableArray> {
         unsafe -count;
     }
 
-    class NSMutableArray: Owned {
+    class NSMutableArray: MutableWithImmutableSuperclass<Foundation::NSArray> {
         unsafe mut -removeAllObjects;
         mut -addObject;
         mut -insertObject_atIndex;
@@ -13,7 +16,7 @@ data! {
         mut -sortUsingFunction_context;
     }
 
-    class NSString {
+    class NSString: ImmutableWithMutableSubclass<Foundation::NSMutableString> {
         unsafe -init;
         unsafe -compare;
         unsafe -hasPrefix;
@@ -30,7 +33,7 @@ data! {
         unsafe +stringWithString;
     }
 
-    class NSMutableString: Owned {
+    class NSMutableString: MutableWithImmutableSuperclass<Foundation::NSString> {
         unsafe -initWithCapacity;
         unsafe +stringWithCapacity;
         unsafe -initWithString;
@@ -39,14 +42,20 @@ data! {
         unsafe mut -setString;
     }
 
-    class NSAttributedString {
+    // Allowed to be just `Immutable` since we've removed the `NSCopying` and
+    // `NSMutableCopying` impls from these for now (they'd return the wrong
+    // type).
+    class NSSimpleCString: Immutable {}
+    class NSConstantString: Immutable {}
+
+    class NSAttributedString: ImmutableWithMutableSubclass<Foundation::NSMutableAttributedString> {
         unsafe -initWithString;
         unsafe -initWithAttributedString;
         unsafe -string;
         unsafe -length;
     }
 
-    class NSMutableAttributedString: Owned {
+    class NSMutableAttributedString: MutableWithImmutableSuperclass<Foundation::NSAttributedString> {
         unsafe -initWithString;
         unsafe -initWithAttributedString;
         unsafe mut -setAttributedString;
@@ -57,32 +66,34 @@ data! {
         unsafe -infoDictionary;
     }
 
-    class NSData {
+    class NSData: ImmutableWithMutableSubclass<Foundation::NSMutableData> {
         unsafe -initWithData;
         unsafe +dataWithData;
         unsafe -length;
         unsafe -bytes;
     }
 
-    class NSMutableData: Owned {
+    class NSMutableData: MutableWithImmutableSuperclass<Foundation::NSData> {
         unsafe +dataWithData;
         unsafe -initWithCapacity;
         unsafe +dataWithCapacity;
         unsafe mut -setLength;
         unsafe mut -mutableBytes;
     }
 
-    class NSDictionary {
+    // Allowed to be just `Mutable` since we've removed the `NSCopying` and
+    // `NSMutableCopying` impls from this for now (since they'd return the
+    // wrong type).
+    class NSPurgeableData: Mutable {}
+
+    class NSDictionary: ImmutableWithMutableSubclass<Foundation::NSMutableDictionary> {
         unsafe -count;
-        unsafe -objectForKey;
-        unsafe -allKeys;
-        unsafe -allValues;
     }
 
-    class NSMutableDictionary: Owned {
-        unsafe mut -setDictionary;
+    class NSMutableDictionary: MutableWithImmutableSuperclass<Foundation::NSDictionary> {
         unsafe mut -removeObjectForKey;
         unsafe mut -removeAllObjects;
+        mut -setDictionary;
     }
 
     class NSError {
@@ -130,20 +141,23 @@ data! {
         unsafe -operatingSystemVersion;
     }
 
-    class NSSet {
+    class NSSet: ImmutableWithMutableSubclass<Foundation::NSMutableSet> {
         unsafe -count;
     }
 
-    class NSMutableSet: Owned {
+    class NSMutableSet: MutableWithImmutableSuperclass<Foundation::NSSet> {
         unsafe mut -removeAllObjects;
         mut -addObject;
     }
 
-    class NSMutableCharacterSet: Owned {}
+    class NSCharacterSet: ImmutableWithMutableSubclass<Foundation::NSMutableCharacterSet> {}
+    class NSMutableCharacterSet: MutableWithImmutableSuperclass<Foundation::NSCharacterSet> {}
 
-    class NSMutableOrderedSet: Owned {}
+    class NSOrderedSet: ImmutableWithMutableSubclass<Foundation::NSMutableOrderedSet> {}
+    class NSMutableOrderedSet: MutableWithImmutableSuperclass<Foundation::NSOrderedSet> {}
 
-    class NSMutableIndexSet: Owned {}
+    class NSIndexSet: ImmutableWithMutableSubclass<Foundation::NSMutableIndexSet> {}
+    class NSMutableIndexSet: MutableWithImmutableSuperclass<Foundation::NSIndexSet> {}
 
     class NSNumber {
         unsafe -initWithChar;
@@ -196,5 +210,6 @@ data! {
         unsafe -stringValue;
     }
 
-    class NSMutableURLRequest: Owned {}
+    class NSURLRequest: ImmutableWithMutableSubclass<Foundation::NSMutableURLRequest> {}
+    class NSMutableURLRequest: MutableWithImmutableSuperclass<Foundation::NSURLRequest> {}
 }

crates/header-translator/src/data/Foundation.rs

@@ -41,16 +41,16 @@ marked safe, so such an improvement can be made in a minor version!
 
 ```rust , ignore
 data! {
-    // Everywhere that the class is returned, it is as
-    // `Id<MyMutableClass, Owned>`.
-    //
-    // Specifying this is _not_ unsafe, but every method that returns this
-    // class must be checked for safety with it in mind.
-    class MyMutableClass: Owned {
+    class MyClass {
         // The class method `new` is safe.
         unsafe +new;
         // The method `init` is safe.
         unsafe -init;
+    }
+
+    class MyImmutableClass: ImmutableWithMutableSubclass<MyMutableClass> {}
+
+    class MyMutableClass: MutableWithImmutableSuperclass<MyImmutableClass> {
         // The method `mutate` takes &mut self, and is safe.
         unsafe mut -mutate;
         // The method `mutateUnchecked` takes &mut self, but is not safe.

crates/header-translator/src/data/README.md

@@ -45,6 +45,32 @@ macro_rules! data {
     };
 }
 
+macro_rules! __set_mutability {
+    ($data:expr;) => {};
+    ($data:expr; ImmutableWithMutableSubclass<$framework:ident::$subclass:ident>) => {
+        $data.mutability = $crate::stmt::Mutability::ImmutableWithMutableSubclass(
+            $crate::ItemIdentifier::from_raw(
+                stringify!($subclass).to_string(),
+                stringify!($framework).to_string(),
+            ),
+        );
+    };
+    ($data:expr; MutableWithImmutableSuperclass<$framework:ident::$superclass:ident>) => {
+        $data.mutability = $crate::stmt::Mutability::MutableWithImmutableSuperclass(
+            $crate::ItemIdentifier::from_raw(
+                stringify!($superclass).to_string(),
+                stringify!($framework).to_string(),
+            ),
+        );
+    };
+    ($data:expr; Immutable) => {
+        $data.mutability = $crate::stmt::Mutability::Immutable;
+    };
+    ($data:expr; Mutable) => {
+        $data.mutability = $crate::stmt::Mutability::Mutable;
+    };
+}
+
 macro_rules! __data_inner {
     // Base case
     (
@@ -54,7 +80,7 @@ macro_rules! __data_inner {
     (
         @($config:expr)
 
-        class $class:ident $(: $ownership:ident)? {
+        class $class:ident $(: $mutability:ident $(<$framework:ident::$ty:ident>)?)? {
             $($methods:tt)*
         }
 
@@ -63,7 +89,10 @@ macro_rules! __data_inner {
         #[allow(unused_mut)]
         let mut data = $config.class_data.entry(stringify!($class).to_string()).or_default();
 
-        $(data.ownership = $crate::rust_type::Ownership::$ownership;)?
+        __set_mutability! {
+            data;
+            $($mutability $(<$framework::$ty>)?)?
+        }
 
         __data_methods! {
             @(data)