Merge pull request #445 from madsmtm/improve-new

Improve `new` and `init` method handling

Author: madsmtm

crates/header-translator/src/cache.rs

@@ -1,99 +1,22 @@
 use std::collections::BTreeMap;
 use std::mem;
 
-use crate::availability::Availability;
-use crate::config::{ClassData, Config};
+use crate::config::Config;
 use crate::file::File;
 use crate::id::ItemIdentifier;
 use crate::method::Method;
 use crate::output::Output;
 use crate::stmt::Stmt;
 
-#[derive(Debug, PartialEq, Clone)]
-struct MethodCache {
-    availability: Availability,
-    methods: Vec<Method>,
-    category: ItemIdentifier<Option<String>>,
-}
-
-#[derive(Debug, PartialEq, Clone, Default)]
-struct ClassCache {
-    /// Methods that should be duplicated onto any subclass.
-    to_emit: Vec<MethodCache>,
-    // We don't need availability here, since a superclass' availability
-    // should always be greater than the subclass'.
-}
-
-impl ClassCache {
-    fn all_methods_data(&self) -> impl Iterator<Item = (bool, &str)> {
-        self.to_emit
-            .iter()
-            .flat_map(|cache| cache.methods.iter().map(|m| m.id()))
-    }
-}
-
 /// A helper struct for doing global analysis on the output.
 #[derive(Debug, PartialEq, Clone)]
 pub struct Cache<'a> {
-    classes: BTreeMap<ItemIdentifier, ClassCache>,
     config: &'a Config,
 }
 
 impl<'a> Cache<'a> {
-    pub fn new(output: &Output, config: &'a Config) -> Self {
-        let mut classes: BTreeMap<_, ClassCache> = BTreeMap::new();
-
-        for (name, library) in &output.libraries {
-            let _span = debug_span!("library", name).entered();
-            for (name, file) in &library.files {
-                let _span = debug_span!("file", name).entered();
-                for stmt in &file.stmts {
-                    if let Some((cls, method_cache)) = Self::cache_stmt(stmt) {
-                        let cache = classes.entry(cls.clone()).or_default();
-                        cache.to_emit.push(method_cache);
-                    }
-                }
-            }
-        }
-
-        Self { classes, config }
-    }
-
-    fn cache_stmt(stmt: &Stmt) -> Option<(&ItemIdentifier, MethodCache)> {
-        if let Stmt::Methods {
-            cls,
-            generics: _,
-            category,
-            availability,
-            superclasses: _,
-            methods,
-            description,
-        } = stmt
-        {
-            let _span = debug_span!("Stmt::Methods", ?cls).entered();
-            let methods: Vec<Method> = methods
-                .iter()
-                .filter(|method| method.emit_on_subclasses())
-                .cloned()
-                .collect();
-            if methods.is_empty() {
-                return None;
-            }
-            if description.is_some() {
-                warn!(description, "description was set");
-            }
-            let category = category.clone().with_new_path(cls);
-            Some((
-                cls,
-                MethodCache {
-                    availability: availability.clone(),
-                    methods,
-                    category,
-                },
-            ))
-        } else {
-            None
-        }
+    pub fn new(_output: &Output, config: &'a Config) -> Self {
+        Self { config }
     }
 
     pub fn update(&self, output: &mut Output) {
@@ -145,70 +68,6 @@ 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,
-                    generics,
-                    superclasses,
-                    ..
-                } => {
-                    let _span = debug_span!("Stmt::ClassDecl", ?id).entered();
-                    let data = self.config.class_data.get(&id.name);
-
-                    // Used for duplicate checking (sometimes the subclass
-                    // defines the same method that the superclass did).
-                    let mut seen_methods: Vec<_> = self
-                        .classes
-                        .get(id)
-                        .map(|cache| cache.all_methods_data())
-                        .into_iter()
-                        .flatten()
-                        .collect();
-
-                    for (superclass, _) in &*superclasses {
-                        if let Some(cache) = self.classes.get(superclass) {
-                            new_stmts.extend(cache.to_emit.iter().filter_map(|cache| {
-                                let methods: Vec<_> = cache
-                                    .methods
-                                    .iter()
-                                    .filter(|method| !seen_methods.contains(&method.id()))
-                                    .filter_map(|method| {
-                                        method.clone().update(ClassData::get_method_data(
-                                            data,
-                                            &method.fn_name,
-                                        ))
-                                    })
-                                    .collect();
-                                if methods.is_empty() {
-                                    return None;
-                                }
-
-                                Some(Stmt::Methods {
-                                    cls: id.clone(),
-                                    generics: generics.clone(),
-                                    category: cache.category.clone(),
-                                    availability: cache.availability.clone(),
-                                    superclasses: superclasses.clone(),
-                                    methods,
-                                    description: Some(format!(
-                                        "Methods declared on superclass `{}`",
-                                        superclass.name
-                                    )),
-                                })
-                            }));
-
-                            seen_methods.extend(cache.all_methods_data());
-                        }
-                    }
-                }
-                _ => {}
-            }
-        }
-        file.stmts.extend(new_stmts);
-
         // Fix up a few typedef + enum declarations
         let mut iter = mem::take(&mut file.stmts).into_iter().peekable();
         while let Some(stmt) = iter.next() {

crates/header-translator/src/config.rs

@@ -157,6 +157,17 @@ pub struct MethodData {
     pub mutating: Option<bool>,
 }
 
+impl MethodData {
+    pub(crate) fn merge_with_superclass(self, superclass: Self) -> Self {
+        Self {
+            // Only use `unsafe` from itself, never take if from the superclass
+            unsafe_: self.unsafe_,
+            skipped: self.skipped | superclass.skipped,
+            mutating: self.mutating.or(superclass.mutating),
+        }
+    }
+}
+
 #[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
 #[serde(deny_unknown_fields)]
 pub struct FnData {

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

@@ -3,10 +3,12 @@ data! {
     // and returning mutable references to those would be unsound - hence
     // `NSArray` cannot be mutable.
     class NSArray: ImmutableWithMutableSubclass<Foundation::NSMutableArray> {
+        unsafe -init;
         unsafe -count;
     }
 
     class NSMutableArray: MutableWithImmutableSuperclass<Foundation::NSArray> {
+        unsafe -init;
         unsafe -removeAllObjects;
     }
 
@@ -28,6 +30,7 @@ data! {
     }
 
     class NSMutableString: MutableWithImmutableSuperclass<Foundation::NSString> {
+        unsafe -init;
         unsafe -initWithCapacity;
         unsafe +stringWithCapacity;
         unsafe -initWithString;
@@ -43,13 +46,15 @@ data! {
     class NSConstantString: Immutable {}
 
     class NSAttributedString: ImmutableWithMutableSubclass<Foundation::NSMutableAttributedString> {
+        unsafe -init;
         unsafe -initWithString;
         unsafe -initWithAttributedString;
         unsafe -string;
         unsafe -length;
     }
 
     class NSMutableAttributedString: MutableWithImmutableSuperclass<Foundation::NSAttributedString> {
+        unsafe -init;
         unsafe -initWithString;
         unsafe -initWithAttributedString;
         unsafe -setAttributedString;
@@ -61,13 +66,15 @@ data! {
     }
 
     class NSData: ImmutableWithMutableSubclass<Foundation::NSMutableData> {
+        unsafe -init;
         unsafe -initWithData;
         unsafe +dataWithData;
         unsafe -length;
         unsafe -bytes;
     }
 
     class NSMutableData: MutableWithImmutableSuperclass<Foundation::NSData> {
+        unsafe -init;
         unsafe +dataWithData;
         unsafe -initWithCapacity;
         unsafe +dataWithCapacity;
@@ -81,10 +88,12 @@ data! {
     class NSPurgeableData: Mutable {}
 
     class NSDictionary: ImmutableWithMutableSubclass<Foundation::NSMutableDictionary> {
+        unsafe -init;
         unsafe -count;
     }
 
     class NSMutableDictionary: MutableWithImmutableSuperclass<Foundation::NSDictionary> {
+        unsafe -init;
         unsafe -removeObjectForKey;
         unsafe -removeAllObjects;
     }
@@ -103,6 +112,7 @@ data! {
     }
 
     class NSLock {
+        unsafe -init;
         unsafe -name;
         unsafe -setName;
     }
@@ -120,6 +130,7 @@ data! {
     }
 
     class NSThread {
+        unsafe -init;
         unsafe +currentThread;
         unsafe +mainThread;
         unsafe -name;
@@ -135,10 +146,12 @@ data! {
     }
 
     class NSSet: ImmutableWithMutableSubclass<Foundation::NSMutableSet> {
+        unsafe -init;
         unsafe -count;
     }
 
     class NSMutableSet: MutableWithImmutableSuperclass<Foundation::NSSet> {
+        unsafe -init;
         unsafe -removeAllObjects;
     }
 

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

@@ -152,6 +152,41 @@ macro_rules! __data_methods {
     (
         @($data:expr)
     ) => {};
+    // Mark init (and by extension, `new`) method as safe
+    (
+        @($data:expr)
+
+        unsafe -init;
+
+        $($rest:tt)*
+    ) => {
+        let mut method_data = $data.methods.entry("init".to_string()).or_default();
+        method_data.unsafe_ = false;
+
+        let mut method_data = $data.methods.entry("new".to_string()).or_default();
+        method_data.unsafe_ = false;
+
+        __data_methods! {
+            @($data)
+            $($rest)*
+        }
+    };
+    // Mark new (and by extension, `init`) method as safe
+    (
+        @($data:expr)
+
+        unsafe +new;
+
+        $($rest:tt)*
+    ) => {
+        __data_methods! {
+            @($data)
+
+            unsafe -init;
+
+            $($rest)*
+        }
+    };
     // Mark method as safe
     (
         @($data:expr)

crates/header-translator/src/main.rs

@@ -178,6 +178,7 @@ fn main() -> Result<(), BoxError> {
         }
         writeln!(cargo_toml, "]")?;
     }
+    drop(cargo_toml);
     drop(span);
 
     let _span = info_span!("formatting").entered();

crates/header-translator/src/method.rs

@@ -242,8 +242,12 @@ pub struct Method {
 
 impl Method {
     /// Value that uniquely identifies the method in a class.
-    pub fn id(&self) -> (bool, &str) {
-        (self.is_class, &self.selector)
+    pub fn id(&self) -> (bool, String) {
+        (self.is_class, self.selector.clone())
+    }
+
+    pub(crate) fn usable_in_default_id(&self) -> bool {
+        self.selector == "new" && self.is_class && self.arguments.is_empty() && self.safe
     }
 
     fn parent_type_data(entity: &Entity<'_>, context: &Context<'_>) -> (bool, bool) {
@@ -320,17 +324,6 @@ impl Method {
         }
     }
 
-    pub fn update(mut self, data: MethodData) -> Option<Self> {
-        if data.skipped {
-            return None;
-        }
-
-        self.mutating = data.mutating.unwrap_or(false);
-        self.safe = !data.unsafe_;
-
-        Some(self)
-    }
-
     pub fn visit_required_types(&self, mut f: impl FnMut(&ItemIdentifier)) {
         for (_, arg) in &self.arguments {
             arg.visit_required_types(&mut f);
@@ -432,7 +425,8 @@ impl<'tu> PartialMethod<'tu> {
         }
 
         let result_type = entity.get_result_type().expect("method return type");
-        let mut result_type = Ty::parse_method_return(result_type, context);
+        let default_nonnull = (selector == "init" && !is_class) || (selector == "new" && is_class);
+        let mut result_type = Ty::parse_method_return(result_type, default_nonnull, context);
 
         let memory_management = MemoryManagement::new(is_class, &selector, &result_type, modifiers);
 
@@ -590,10 +584,9 @@ impl Method {
             return false;
         }
         if self.is_class {
-            !matches!(&*self.selector, "new" | "supportsSecureCoding")
+            true
         } else {
             self.memory_management == MemoryManagement::IdInit
-                && !matches!(&*self.selector, "init" | "initWithCoder:")
         }
     }
 }