From 85b23e1a470f667d40e74d9e77cc11c8b2399cf0 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Mon, 11 Sep 2023 06:20:57 +0200 Subject: [PATCH] Emit main thread requirements on protocols --- crates/header-translator/src/cache.rs | 35 +++++++----- crates/header-translator/src/config.rs | 3 + crates/header-translator/src/data/AppKit.rs | 11 ++++ .../header-translator/src/data/Automator.rs | 1 + crates/header-translator/src/data/OSAKit.rs | 1 + crates/header-translator/src/method.rs | 3 +- crates/header-translator/src/stmt.rs | 57 ++++++++++++++++++- .../header-translator/translation-config.toml | 8 ++- crates/icrate/CHANGELOG.md | 8 ++- crates/icrate/src/common.rs | 4 +- crates/icrate/src/generated | 2 +- crates/test-ui/Cargo.toml | 3 + ...clare_class_delegate_not_mainthreadonly.rs | 43 ++++++++++++++ ...e_class_delegate_not_mainthreadonly.stderr | 21 +++++++ .../ui/implement_protocol_missing_super.rs | 19 +++++++ .../implement_protocol_missing_super.stderr | 29 ++++++++++ .../test-ui/ui/msg_send_invalid_error.stderr | 10 ++-- crates/test-ui/ui/nsarray_not_message.stderr | 26 ++++----- 18 files changed, 241 insertions(+), 43 deletions(-) create mode 100644 crates/test-ui/ui/declare_class_delegate_not_mainthreadonly.rs create mode 100644 crates/test-ui/ui/declare_class_delegate_not_mainthreadonly.stderr create mode 100644 crates/test-ui/ui/implement_protocol_missing_super.rs create mode 100644 crates/test-ui/ui/implement_protocol_missing_super.stderr diff --git a/crates/header-translator/src/cache.rs b/crates/header-translator/src/cache.rs index b536cab70..eb2a91d8b 100644 --- a/crates/header-translator/src/cache.rs +++ b/crates/header-translator/src/cache.rs @@ -13,23 +13,32 @@ use crate::Mutability; #[derive(Debug, PartialEq, Clone)] pub struct Cache<'a> { config: &'a Config, - mainthreadonly_classes: BTreeSet, + mainthreadonly_items: BTreeSet, } 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()); + } + _ => {} } } } @@ -37,7 +46,7 @@ impl<'a> Cache<'a> { Self { config, - mainthreadonly_classes, + mainthreadonly_items, } } @@ -100,7 +109,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 +120,13 @@ impl<'a> Cache<'a> { // include optional arguments like `Option<&NSView>` or // `&NSArray`. 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. diff --git a/crates/header-translator/src/config.rs b/crates/header-translator/src/config.rs index 8a82abaf0..703da63de 100644 --- a/crates/header-translator/src/config.rs +++ b/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, + #[serde(default)] pub methods: HashMap, } diff --git a/crates/header-translator/src/data/AppKit.rs b/crates/header-translator/src/data/AppKit.rs index 57bc56ac4..6f023800e 100644 --- a/crates/header-translator/src/data/AppKit.rs +++ b/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 diff --git a/crates/header-translator/src/data/Automator.rs b/crates/header-translator/src/data/Automator.rs index 89a871f29..9902f69f3 100644 --- a/crates/header-translator/src/data/Automator.rs +++ b/crates/header-translator/src/data/Automator.rs @@ -1,2 +1,3 @@ data! { + class AMWorkflowController: MainThreadOnly {} } diff --git a/crates/header-translator/src/data/OSAKit.rs b/crates/header-translator/src/data/OSAKit.rs index 89a871f29..5391fdea3 100644 --- a/crates/header-translator/src/data/OSAKit.rs +++ b/crates/header-translator/src/data/OSAKit.rs @@ -1,2 +1,3 @@ data! { + class OSAScriptController: MainThreadOnly {} } diff --git a/crates/header-translator/src/method.rs b/crates/header-translator/src/method.rs index 4e1987531..7a3d97b8e 100644 --- a/crates/header-translator/src/method.rs +++ b/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, ")")?; diff --git a/crates/header-translator/src/stmt.rs b/crates/header-translator/src/stmt.rs index 58ea349da..0413fa514 100644 --- a/crates/header-translator/src/stmt.rs +++ b/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 { diff --git a/crates/header-translator/translation-config.toml b/crates/header-translator/translation-config.toml index f586ef040..fb72a78ed 100644 --- a/crates/header-translator/translation-config.toml +++ b/crates/header-translator/translation-config.toml @@ -852,6 +852,12 @@ 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"] + # Both protocols and classes [protocol.NSTextAttachmentCell] renamed = "NSTextAttachmentCellProtocol" @@ -1614,7 +1620,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 diff --git a/crates/icrate/CHANGELOG.md b/crates/icrate/CHANGELOG.md index ce992dc9d..ca1d11a9c 100644 --- a/crates/icrate/CHANGELOG.md +++ b/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 diff --git a/crates/icrate/src/common.rs b/crates/icrate/src/common.rs index b6ba9c38f..641f0fd61 100644 --- a/crates/icrate/src/common.rs +++ b/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}; diff --git a/crates/icrate/src/generated b/crates/icrate/src/generated index cf4125408..5d2649286 160000 --- a/crates/icrate/src/generated +++ b/crates/icrate/src/generated @@ -1 +1 @@ -Subproject commit cf4125408d57e55376449b5b35d9b78f6b4e4a72 +Subproject commit 5d26492863681c7e2f9f1251bf56e19da988f57a diff --git a/crates/test-ui/Cargo.toml b/crates/test-ui/Cargo.toml index a61d07a3e..2fbdba4ec 100644 --- a/crates/test-ui/Cargo.toml +++ b/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"] diff --git a/crates/test-ui/ui/declare_class_delegate_not_mainthreadonly.rs b/crates/test-ui/ui/declare_class_delegate_not_mainthreadonly.rs new file mode 100644 index 000000000..af10f877b --- /dev/null +++ b/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; + } +); + +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)); +} diff --git a/crates/test-ui/ui/declare_class_delegate_not_mainthreadonly.stderr b/crates/test-ui/ui/declare_class_delegate_not_mainthreadonly.stderr new file mode 100644 index 000000000..127a469d4 --- /dev/null +++ b/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) diff --git a/crates/test-ui/ui/implement_protocol_missing_super.rs b/crates/test-ui/ui/implement_protocol_missing_super.rs new file mode 100644 index 000000000..7e2fd36ad --- /dev/null +++ b/crates/test-ui/ui/implement_protocol_missing_super.rs @@ -0,0 +1,19 @@ +//! Test that implementing traits like `NSApplicationDelegate` requires super +//! protocols like `NSObjectProtocol` to also be implemented. +use icrate::AppKit::NSApplicationDelegate; +use icrate::Foundation::NSObject; +use objc2::{declare_class, mutability, ClassType}; + +declare_class!( + struct CustomObject; + + unsafe impl ClassType for CustomObject { + type Super = NSObject; + type Mutability = mutability::MainThreadOnly; + const NAME: &'static str = "CustomObject"; + } + + unsafe impl NSApplicationDelegate for CustomObject {} +); + +fn main() {} diff --git a/crates/test-ui/ui/implement_protocol_missing_super.stderr b/crates/test-ui/ui/implement_protocol_missing_super.stderr new file mode 100644 index 000000000..b45a0c989 --- /dev/null +++ b/crates/test-ui/ui/implement_protocol_missing_super.stderr @@ -0,0 +1,29 @@ +error[E0277]: the trait bound `CustomObject: NSObjectProtocol` is not satisfied + --> ui/implement_protocol_missing_super.rs + | + | unsafe impl NSApplicationDelegate for CustomObject {} + | ^^^^^^^^^^^^ the trait `NSObjectProtocol` is not implemented for `CustomObject` + | + = help: the following other types implement trait `NSObjectProtocol`: + ProtocolObject + NSObject + __NSProxy + NSApplication + NSCollectionView + NSCollectionLayoutSection + NSCollectionLayoutGroupCustomItem + NSControl + and $N others +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) diff --git a/crates/test-ui/ui/msg_send_invalid_error.stderr b/crates/test-ui/ui/msg_send_invalid_error.stderr index 00ee6e294..ffd9cd0ca 100644 --- a/crates/test-ui/ui/msg_send_invalid_error.stderr +++ b/crates/test-ui/ui/msg_send_invalid_error.stderr @@ -38,11 +38,11 @@ error[E0277]: the trait bound `i32: Message` is not satisfied Exception ProtocolObject

AnyObject - NSArray - NSMutableArray - NSDictionary - NSMutableDictionary - NSSet + __RcTestObject + NSObject + __NSProxy + NSApplication + NSCollectionView and $N others note: required by a bound in `__send_message_error` --> $WORKSPACE/crates/objc2/src/message/mod.rs diff --git a/crates/test-ui/ui/nsarray_not_message.stderr b/crates/test-ui/ui/nsarray_not_message.stderr index f4a9f4ca4..8da89e478 100644 --- a/crates/test-ui/ui/nsarray_not_message.stderr +++ b/crates/test-ui/ui/nsarray_not_message.stderr @@ -8,11 +8,11 @@ error[E0277]: the trait bound `i32: Message` is not satisfied Exception ProtocolObject

AnyObject - NSArray - NSMutableArray - NSDictionary - NSMutableDictionary - NSSet + __RcTestObject + NSObject + __NSProxy + NSApplication + NSCollectionView and $N others note: required by a bound in `Foundation::__NSArray::>::new` --> $WORKSPACE/crates/icrate/src/generated/Foundation/NSArray.rs @@ -38,14 +38,14 @@ error[E0277]: the trait bound `Id: ClassType` is not satisfied | required by a bound introduced by this call | = help: the following other types implement trait `ClassType`: - NSArray - NSMutableArray - NSDictionary - NSMutableDictionary - NSSet - NSMutableSet - NSEnumerator - NSAppleEventDescriptor + __RcTestObject + NSObject + __NSProxy + NSApplication + NSCollectionView + NSCollectionLayoutSection + NSCollectionLayoutGroupCustomItem + NSControl and $N others = note: required for `Id` to implement `IsRetainable` note: required by a bound in `icrate::Foundation::array::>::from_slice`