Merge pull request #419 from madsmtm/redo-ownership

Redo how ownership works

Author: madsmtm

.github/workflows/ci.yml

@@ -144,6 +144,9 @@ jobs:
         key: cargo-${{ github.job }}-${{ matrix.name }}-${{ hashFiles('**/Cargo.lock') }}
 
     - name: cargo doc
+      # Disable cargo doc checking for now, `NSEnumerator2<'a, T>` is broken
+      # on current nightly.
+      if: false
       run: cargo doc --no-deps --document-private-items ${{ matrix.args }}
 
     - name: cargo clippy

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>,
@@ -156,8 +154,7 @@ pub struct MethodData {
     pub unsafe_: bool,
     #[serde(default = "skipped_default")]
     pub skipped: bool,
-    #[serde(default = "mutating_default")]
-    pub mutating: bool,
+    pub mutating: Option<bool>,
 }
 
 #[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
@@ -191,16 +188,12 @@ fn skipped_default() -> bool {
     false
 }
 
-fn mutating_default() -> bool {
-    false
-}
-
 impl Default for MethodData {
     fn default() -> Self {
         Self {
             unsafe_: unsafe_default(),
             skipped: skipped_default(),
-            mutating: mutating_default(),
+            mutating: None,
         }
     }
 }

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,19 +1,16 @@
 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 {
-        unsafe mut -removeAllObjects;
-        mut -addObject;
-        mut -insertObject_atIndex;
-        mut -replaceObjectAtIndex_withObject;
-        mut -removeObjectAtIndex;
-        mut -removeLastObject;
-        mut -sortUsingFunction_context;
+    class NSMutableArray: MutableWithImmutableSuperclass<Foundation::NSArray> {
+        unsafe -removeAllObjects;
     }
 
-    class NSString {
+    class NSString: ImmutableWithMutableSubclass<Foundation::NSMutableString> {
         unsafe -init;
         unsafe -compare;
         unsafe -hasPrefix;
@@ -30,59 +27,66 @@ data! {
         unsafe +stringWithString;
     }
 
-    class NSMutableString: Owned {
+    class NSMutableString: MutableWithImmutableSuperclass<Foundation::NSString> {
         unsafe -initWithCapacity;
         unsafe +stringWithCapacity;
         unsafe -initWithString;
         unsafe +stringWithString;
-        unsafe mut -appendString;
-        unsafe mut -setString;
+        unsafe -appendString;
+        unsafe -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;
+        unsafe -setAttributedString;
     }
 
     class NSBundle {
         unsafe +mainBundle;
         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;
+        unsafe -setLength;
+        unsafe -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;
-        unsafe mut -removeObjectForKey;
-        unsafe mut -removeAllObjects;
+    class NSMutableDictionary: MutableWithImmutableSuperclass<Foundation::NSDictionary> {
+        unsafe -removeObjectForKey;
+        unsafe -removeAllObjects;
     }
 
     class NSError {
@@ -130,20 +134,22 @@ data! {
         unsafe -operatingSystemVersion;
     }
 
-    class NSSet {
+    class NSSet: ImmutableWithMutableSubclass<Foundation::NSMutableSet> {
         unsafe -count;
     }
 
-    class NSMutableSet: Owned {
-        unsafe mut -removeAllObjects;
-        mut -addObject;
+    class NSMutableSet: MutableWithImmutableSuperclass<Foundation::NSSet> {
+        unsafe -removeAllObjects;
     }
 
-    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 +202,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.