Merge pull request #509 from madsmtm/protocol-requirements

Fix main thread requirements on protocols

crates/header-translator/src/cache.rs

@@ -13,31 +13,52 @@ use crate::Mutability;
 #[derive(Debug, PartialEq, Clone)]
 pub struct Cache<'a> {
     config: &'a Config,
-    mainthreadonly_classes: BTreeSet<ItemIdentifier>,
+    mainthreadonly_items: BTreeSet<ItemIdentifier>,
 }
 
 impl<'a> Cache<'a> {
     pub fn new(output: &Output, config: &'a Config) -> Self {
-        let mut mainthreadonly_classes = BTreeSet::new();
+        let mut mainthreadonly_items = BTreeSet::new();
 
         for library in output.libraries.values() {
             for file in library.files.values() {
                 for stmt in file.stmts.iter() {
-                    if let Stmt::ClassDecl {
-                        id,
-                        mutability: Mutability::MainThreadOnly,
-                        ..
-                    } = stmt
-                    {
-                        mainthreadonly_classes.insert(id.clone());
+                    match stmt {
+                        Stmt::ClassDecl {
+                            id,
+                            mutability: Mutability::MainThreadOnly,
+                            ..
+                        } => {
+                            mainthreadonly_items.insert(id.clone());
+                        }
+                        Stmt::ProtocolDecl {
+                            id,
+                            required_mainthreadonly: true,
+                            ..
+                        } => {
+                            mainthreadonly_items.insert(id.clone());
+                        }
+                        Stmt::ProtocolDecl {
+                            id,
+                            required_mainthreadonly: false,
+                            protocols,
+                            ..
+                        } => {
+                            for protocol in protocols {
+                                if mainthreadonly_items.contains(protocol) {
+                                    let _ = mainthreadonly_items.insert(id.clone());
+                                }
+                            }
+                        }
+                        _ => {}
                     }
                 }
             }
         }
 
         Self {
             config,
-            mainthreadonly_classes,
+            mainthreadonly_items,
         }
     }
 
@@ -100,50 +121,51 @@ impl<'a> Cache<'a> {
                     for method in methods.iter_mut() {
                         let mut result_type_contains_mainthreadonly: bool = false;
                         method.result_type.visit_required_types(&mut |id| {
-                            if self.mainthreadonly_classes.contains(id) {
+                            if self.mainthreadonly_items.contains(id) {
                                 result_type_contains_mainthreadonly = true;
                             }
                         });
 
-                        match (method.is_class, self.mainthreadonly_classes.contains(id)) {
-                            // MainThreadOnly class with static method
-                            (true, true) => {
-                                // Assume the method needs main thread
-                                result_type_contains_mainthreadonly = true;
-                            }
-                            // Class with static method
-                            (true, false) => {
-                                // Continue with the normal check
-                            }
-                            // MainThreadOnly class with non-static method
-                            (false, true) => {
-                                // Method is already required to run on main
-                                // thread, so no need to add MainThreadMarker
-                                continue;
-                            }
-                            // Class with non-static method
-                            (false, false) => {
-                                // Continue with the normal check
-                            }
+                        let mut any_argument_contains_mainthreadonly: bool = false;
+                        for (_, argument) in method.arguments.iter() {
+                            // Important: We only visit the top-level types, to not
+                            // include optional arguments like `Option<&NSView>` or
+                            // `&NSArray<NSView>`.
+                            argument.visit_toplevel_types(&mut |id| {
+                                if self.mainthreadonly_items.contains(id) {
+                                    any_argument_contains_mainthreadonly = true;
+                                }
+                            });
                         }
 
-                        if result_type_contains_mainthreadonly {
-                            let mut any_argument_contains_mainthreadonly: bool = false;
-                            for (_, argument) in method.arguments.iter() {
-                                // Important: We only visit the top-level types, to not
-                                // include e.g. `Option<&NSView>` or `&NSArray<NSView>`.
-                                argument.visit_toplevel_types(&mut |id| {
-                                    if self.mainthreadonly_classes.contains(id) {
-                                        any_argument_contains_mainthreadonly = true;
-                                    }
-                                });
+                        if self.mainthreadonly_items.contains(id) {
+                            if method.is_class {
+                                // Assume the method needs main thread if it's
+                                // declared on a main thread only class.
+                                result_type_contains_mainthreadonly = true;
+                            } else {
+                                // Method takes `&self` or `&mut self`, or is
+                                // an initialization method, all of which
+                                // already require the main thread.
+                                //
+                                // Note: Initialization methods can be passed
+                                // `None`, but in that case the return will
+                                // always be NULL.
+                                any_argument_contains_mainthreadonly = true;
                             }
+                        }
 
-                            // Apply main thread only, unless a (required)
-                            // argument was main thread only.
-                            if !any_argument_contains_mainthreadonly {
-                                method.mainthreadonly = true;
-                            }
+                        if any_argument_contains_mainthreadonly {
+                            // MainThreadMarker can be retrieved from
+                            // `MainThreadMarker::from` inside these methods,
+                            // and hence passing it is redundant.
+                            method.mainthreadonly = false;
+                        } else if result_type_contains_mainthreadonly {
+                            method.mainthreadonly = true;
+                        } else {
+                            // If neither, then we respect any annotation
+                            // the method may have had before
+                            // method.mainthreadonly = method.mainthreadonly;
                         }
                     }
                 }

crates/header-translator/src/config.rs

@@ -146,6 +146,9 @@ pub struct ProtocolData {
     #[serde(default)]
     pub skipped: bool,
     #[serde(default)]
+    #[serde(rename = "requires-mainthreadonly")]
+    pub requires_mainthreadonly: Option<bool>,
+    #[serde(default)]
     pub methods: HashMap<String, MethodData>,
 }
 

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

@@ -30,6 +30,13 @@ data! {
         // `run` cannot be safe, the user must ensure there is no re-entrancy.
     }
 
+    class NSController: MainThreadOnly {}
+    class NSObjectController: MainThreadOnly {}
+    class NSArrayController: MainThreadOnly {}
+    class NSDictionaryController: MainThreadOnly {}
+    class NSTreeController: MainThreadOnly {}
+    class NSUserDefaultsController: MainThreadOnly {}
+
     // Documentation says:
     // > Color objects are immutable and thread-safe
     //
@@ -38,6 +45,8 @@ data! {
         unsafe -clear;
     }
 
+    class NSColorPicker: MainThreadOnly {}
+
     class NSControl {
         unsafe -isEnabled;
         unsafe -setEnabled;
@@ -81,6 +90,8 @@ data! {
 
     }
 
+    class NSFontManager: MainThreadOnly {}
+
     // Documented Thread-Unsafe, but:
     // > One thread can create an NSImage object, draw to the image buffer,
     // > and pass it off to the main thread for drawing. The underlying image

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

@@ -1,2 +1,3 @@
 data! {
+    class AMWorkflowController: MainThreadOnly {}
 }

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

@@ -1,2 +1,3 @@
 data! {
+    class OSAScriptController: MainThreadOnly {}
 }

crates/header-translator/src/id.rs

@@ -1,4 +1,6 @@
+use core::cmp::Ordering;
 use core::fmt;
+use core::hash;
 
 use clang::Entity;
 
@@ -20,7 +22,7 @@ impl ToOptionString for Option<String> {
     }
 }
 
-#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+#[derive(Debug, Clone)]
 pub struct ItemIdentifier<N = String> {
     /// Names in Objective-C are global, so this is always enough to uniquely
     /// identify the item.
@@ -31,6 +33,32 @@ pub struct ItemIdentifier<N = String> {
     pub file_name: Option<String>,
 }
 
+impl<N: PartialEq> PartialEq for ItemIdentifier<N> {
+    fn eq(&self, other: &Self) -> bool {
+        self.name == other.name
+    }
+}
+
+impl<N: Eq> Eq for ItemIdentifier<N> {}
+
+impl<N: hash::Hash> hash::Hash for ItemIdentifier<N> {
+    fn hash<H: hash::Hasher>(&self, state: &mut H) {
+        self.name.hash(state);
+    }
+}
+
+impl<N: Ord> PartialOrd for ItemIdentifier<N> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl<N: Ord> Ord for ItemIdentifier<N> {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.name.cmp(&other.name)
+    }
+}
+
 impl<N: ToOptionString> ItemIdentifier<N> {
     pub fn from_raw(name: N, library: String) -> Self {
         Self {

crates/header-translator/src/method.rs

@@ -700,8 +700,7 @@ impl fmt::Display for Method {
             let param = handle_reserved(&crate::to_snake_case(param));
             write!(f, "{param}: {arg_ty}, ")?;
         }
-        // FIXME: Skipping main thread only on protocols for now
-        if self.mainthreadonly && !self.is_protocol {
+        if self.mainthreadonly {
             write!(f, "mtm: MainThreadMarker")?;
         }
         write!(f, ")")?;