Emit 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,7 +121,7 @@ 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;
                             }
                         });
@@ -111,13 +132,13 @@ impl<'a> Cache<'a> {
                             // include optional arguments like `Option<&NSView>` or
                             // `&NSArray<NSView>`.
                             argument.visit_toplevel_types(&mut |id| {
-                                if self.mainthreadonly_classes.contains(id) {
+                                if self.mainthreadonly_items.contains(id) {
                                     any_argument_contains_mainthreadonly = true;
                                 }
                             });
                         }
 
-                        if self.mainthreadonly_classes.contains(id) {
+                        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.

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/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, ")")?;

crates/header-translator/src/stmt.rs

@@ -698,7 +698,13 @@ impl Stmt {
 
                 verify_objc_decl(entity, context);
                 let generics = parse_class_generics(entity, context);
-                let protocols = parse_direct_protocols(entity, context);
+                let mut protocols = parse_direct_protocols(entity, context);
+
+                let skipped_protocols = data
+                    .map(|data| data.skipped_protocols.clone())
+                    .unwrap_or_default();
+                protocols.retain(|protocol| !skipped_protocols.contains(&protocol.name));
+
                 let (methods, designated_initializers) = parse_methods(
                     entity,
                     |name| ClassData::get_method_data(data, name),
@@ -825,7 +831,7 @@ impl Stmt {
                     context,
                 );
 
-                let (sendable, mainthreadonly) = parse_attributes(entity, context);
+                let (sendable, mut mainthreadonly) = parse_attributes(entity, context);
 
                 if !designated_initializers.is_empty() {
                     warn!(
@@ -834,6 +840,43 @@ impl Stmt {
                     )
                 }
 
+                // Set the protocol as main thread only if all methods are
+                // main thread only.
+                //
+                // This is done to make the UI nicer when the user tries to
+                // implement such traits.
+                //
+                // Note: This is a deviation from the headers, but I don't
+                // see a way for this to be unsound? As an example, let's say
+                // there is some Objective-C code that assumes it can create
+                // an object which is not `MainThreadOnly`, and then sets it
+                // as the application delegate.
+                //
+                // Rust code that later retrieves the delegate would assume
+                // that the object is `MainThreadOnly`, and could use this
+                // information to create `MainThreadMarker`; but they can
+                // _already_ do that, since the only way to retrieve the
+                // delegate in the first place would be through
+                // `NSApplication`!
+                if !methods.is_empty() && methods.iter().all(|method| method.mainthreadonly) {
+                    mainthreadonly = true;
+                }
+
+                // Overwrite with config preference
+                if let Some(data) = data
+                    .map(|data| data.requires_mainthreadonly)
+                    .unwrap_or_default()
+                {
+                    if mainthreadonly == data {
+                        warn!(
+                            mainthreadonly,
+                            data,
+                            "set requires-mainthreadonly to the same value that it already has"
+                        );
+                    }
+                    mainthreadonly = data;
+                }
+
                 vec![Self::ProtocolDecl {
                     id,
                     actual_name,
@@ -1630,7 +1673,7 @@ impl fmt::Display for Stmt {
                 protocols,
                 methods,
                 required_sendable: _,
-                required_mainthreadonly: _,
+                required_mainthreadonly,
             } => {
                 writeln!(f, "extern_protocol!(")?;
                 write!(f, "{availability}")?;
@@ -1661,6 +1704,14 @@ impl fmt::Display for Stmt {
                 //     }
                 //     write!(f, "Send + Sync")?;
                 // }
+                if *required_mainthreadonly {
+                    if protocols.is_empty() {
+                        write!(f, ": ")?;
+                    } else {
+                        write!(f, "+ ")?;
+                    }
+                    write!(f, "IsMainThreadOnly")?;
+                }
                 writeln!(f, " {{")?;
 
                 for method in methods {

crates/header-translator/translation-config.toml

@@ -852,6 +852,20 @@ skipped = true
 [class.NSCollectionViewDiffableDataSource.methods.initWithCollectionView_itemProvider]
 skipped = true
 
+# Requires `MainThreadOnly`, which I'm not sure is a good idea here?
+[class.NSCollectionViewDiffableDataSource]
+skipped-protocols = ["NSCollectionViewDataSource"]
+[class.NSManagedObjectContext]
+skipped-protocols = ["NSEditor", "NSEditorRegistration"]
+
+# Most methods on these require MainThreadMarker anyhow
+[protocol.NSDraggingInfo]
+requires-mainthreadonly = true
+[protocol.NSBrowserDelegate]
+requires-mainthreadonly = true
+[protocol.NSSplitViewDelegate]
+requires-mainthreadonly = true
+
 # Both protocols and classes
 [protocol.NSTextAttachmentCell]
 renamed = "NSTextAttachmentCellProtocol"
@@ -1614,7 +1628,7 @@ skipped-protocols = ["NSCopying", "NSMutableCopying"]
 skipped-protocols = ["NSCopying", "NSMutableCopying"]
 
 # Uses `NS_SWIFT_UI_ACTOR` on a static, which is hard to support.
-# 
+#
 # Will have to be a method that takes `MainThreadMarker`.
 [static.NSApp]
 skipped = true

crates/icrate/CHANGELOG.md

@@ -31,9 +31,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   `MTLAccelerationStructureCommandEncoder` now take a nullable scratch buffer:
   - `refitAccelerationStructure_descriptor_destination_scratchBuffer_scratchBufferOffset`
   - `refitAccelerationStructure_descriptor_destination_scratchBuffer_scratchBufferOffset_options`
-* **BREAKING**: Marked UI-related types as `MainThreadOnly`. This means that
-  they can now only be constructed on the main thread, meaning you have to
-  aquire a `MainThreadMarker` first.
+* **BREAKING**: Marked UI-related classes as `MainThreadOnly`, and UI-related
+  protocols as `IsMainThreadOnly`.
+
+  This means that they can now only be constructed, retrieved and used on the
+  main thread, meaning you usually have to aquire a `MainThreadMarker` first.
 
   ```rust
   // Before

crates/icrate/src/common.rs

@@ -14,8 +14,8 @@ pub(crate) use std::os::raw::{
 pub(crate) use objc2::ffi::{NSInteger, NSIntegerMax, NSUInteger, NSUIntegerMax, IMP};
 #[cfg(feature = "objective-c")]
 pub(crate) use objc2::mutability::{
-    Immutable, ImmutableWithMutableSubclass, InteriorMutable, IsIdCloneable, MainThreadOnly,
-    Mutable, MutableWithImmutableSuperclass,
+    Immutable, ImmutableWithMutableSubclass, InteriorMutable, IsIdCloneable, IsMainThreadOnly,
+    MainThreadOnly, Mutable, MutableWithImmutableSuperclass,
 };
 #[cfg(feature = "objective-c")]
 pub(crate) use objc2::rc::{Allocated, DefaultId, Id};

crates/test-ui/Cargo.toml

@@ -16,12 +16,15 @@ default = [
     "icrate/Foundation",
     "icrate/Foundation_NSString",
     "icrate/Foundation_NSMutableString",
+    "icrate/Foundation_NSNotification",
     "icrate/Foundation_NSThread",
     "icrate/Foundation_NSError",
     "icrate/Foundation_NSArray",
     "icrate/Foundation_NSMutableArray",
     "icrate/Foundation_NSValue",
     "icrate/Foundation_NSSet",
+    "icrate/AppKit",
+    "icrate/AppKit_NSApplication",
     "objc2/unstable-msg-send-always-comma",
 ]
 std = ["block2/std", "objc2/std", "icrate/std"]

crates/test-ui/ui/declare_class_delegate_not_mainthreadonly.rs

@@ -0,0 +1,43 @@
+//! Test that implementing `NSApplicationDelegate` and similar requires
+//! a `MainThreadOnly` class.
+use icrate::AppKit::{NSApplication, NSApplicationDelegate};
+use icrate::Foundation::{MainThreadMarker, NSNotification, NSObject, NSObjectProtocol};
+use objc2::rc::Id;
+use objc2::runtime::ProtocolObject;
+use objc2::{declare_class, extern_methods, mutability, ClassType};
+
+declare_class!(
+    struct CustomObject;
+
+    unsafe impl ClassType for CustomObject {
+        type Super = NSObject;
+        type Mutability = mutability::InteriorMutable; // Not `MainThreadOnly`
+        const NAME: &'static str = "CustomObject";
+    }
+
+    unsafe impl NSObjectProtocol for CustomObject {}
+
+    unsafe impl NSApplicationDelegate for CustomObject {
+        #[method(applicationDidFinishLaunching:)]
+        unsafe fn application_did_finish_launching(&self, _notification: &NSNotification) {
+            // Unclear for the user how to get a main thread marker if `self` is not `MainThreadOnly`
+            let _mtm = MainThreadMarker::new().unwrap();
+        }
+    }
+);
+
+extern_methods!(
+    unsafe impl CustomObject {
+        #[method_id(new)]
+        fn new(mtm: MainThreadMarker) -> Id<Self>;
+    }
+);
+
+fn main() {
+    let mtm = MainThreadMarker::new().unwrap();
+    let app = NSApplication::sharedApplication(mtm);
+
+    let delegate = CustomObject::new(mtm);
+    let delegate = ProtocolObject::from_ref(&*delegate);
+    app.setDelegate(Some(delegate));
+}

crates/test-ui/ui/declare_class_delegate_not_mainthreadonly.stderr

@@ -0,0 +1,21 @@
+error[E0277]: the trait bound `InteriorMutable: mutability::MutabilityIsMainThreadOnly` is not satisfied
+ --> ui/declare_class_delegate_not_mainthreadonly.rs
+  |
+  |     unsafe impl NSApplicationDelegate for CustomObject {
+  |                                           ^^^^^^^^^^^^ the trait `mutability::MutabilityIsMainThreadOnly` is not implemented for `InteriorMutable`
+  |
+  = help: the trait `mutability::MutabilityIsMainThreadOnly` is implemented for `MainThreadOnly`
+  = note: required for `CustomObject` to implement `IsMainThreadOnly`
+note: required by a bound in `NSApplicationDelegate`
+ --> $WORKSPACE/crates/icrate/src/generated/AppKit/NSApplication.rs
+  |
+  | / extern_protocol!(
+  | |     pub unsafe trait NSApplicationDelegate: NSObjectProtocol + IsMainThreadOnly {
+  | |                      --------------------- required by a bound in this trait
+  | |         #[cfg(feature = "AppKit_NSApplication")]
+  | |         #[optional]
+... |
+  | |     unsafe impl ProtocolType for dyn NSApplicationDelegate {}
+  | | );
+  | |_^ required by this bound in `NSApplicationDelegate`
+  = note: this error originates in the macro `extern_protocol` (in Nightly builds, run with -Z macro-backtrace for more info)