Closed madsmtm closed 1 year ago
https://github.com/rust-windowing/winit/pull/2457 highlighted an issue: We'd have to return Id<WinitView, Owned>
if we wanted to make accepts_first_mouse
mutable after the fact - but you can very rarely have an owned view (because other code may hold references to it, unknown to you), so this would be a footgun.
Instead, making it Id<WinitView, Shared>
and then contain accepts_first_mouse: Cell<bool>
would be more correct.
Note that this is not currently possible because the layout of Cell
is not guaranteed, will request this from std
at some point.
Even ignoring thread safety, a problem arises with methods that return references to internal data. For example, the following fails at compile-time because set_bytes
requires &mut self
, but if we were to change things to no longer require mutable pointers, it would cause UB:
let mut data = NSMutableData::with_bytes(b"abc");
let b = data.bytes();
data.set_bytes(b"def");
assert_eq!(b, b"abc");
Similarly for -[NSString UTF8String]
, other methods annotated with NS_RETURNS_INNER_POINTER
, and probably also the collection types - perhaps also when iterating?
Swift "solves" this by just returning UnsafeRawPointer
. And when iterating over Array
, they make a local copy; when iterating over NSMutableArray
, they don't mark the add
method as mutating, so memory safety is quite easy to violate:
import Foundation
var arr = NSMutableArray(array: [1, 2, 3])
for elem in arr {
arr.add(4)
print("test", elem)
}
So yeah, I'm quite conflicted as to how we should do mutability; perhaps things will be clearer once I've done more of #264, and get to see if there are parts other than Foundation that would benefit from mutability.
Another solution: Just keep every mutating method unsafe
. Or maybe add two variants, one fn push(&mut self, item: &T)
and one unsafe fn push_unchecked(&self, item: &T)
?
Would maybe make sense to take the Ownership
parameter from NSCopying
and move it to ClassType
?
Truth be told, I'm fine with having a standard class that's !Send/!Sync
and then having an escape-hatch unsafe
adaptor that is Send/Sync
. In the wide variety of cases, these types end up being thread-unsafe anyhow.
Yeah if it was only thread safety that was the issue, then I would agree, but as noted in https://github.com/madsmtm/objc2/issues/265#issuecomment-1267747513, that still won't allow us to make mutating methods like addObject:
safe
An update: Cell
will soon be Encode
, and as such usable in instance variables - which should allow using NSMutableString
in non-Sync
classes.
More importantly, Swift recently implemented a better concept of sendability, see NS_SWIFT_SENDABLE
and NS_SWIFT_UI_ACTOR
in Foundation/NSObjCRuntime.h
.
Using these attributes, we can automatically mark NSError
, NSDate
, NSFileHandle
and so on Send + Sync
(not sure about NSLock
and subclasses, seems like that shouldn't be either? Or at least not Send
).
But, even more importantly, we can automatically mark NSView
, NSWindow
, ... as "needing a MainThreadMarker
to be created". That is, for every function/method, do an algorithm like:
if method.return_type.includes_ns_swift_ui_actor() {
if method.receiver.is_ns_swift_ui_actor() {
break;
}
for argument in method.arguments {
if argument.is_ns_swift_ui_actor() {
break;
}
}
method.arguments.push(("mtm", Ty::MainThreadMarker));
}
(Note here the difference between includes_ns_swift_ui_actor
and is_ns_swift_ui_actor
; includes_
returns true for NSArray<NSView>
or Option<NSView>
, while is_
doesn't).
So e.g. NSView::new()
would become NSView::new(_mtm: MainThreadMarker)
, while NSWindow::view(&self)
would stay as-is.
A bit unsure where the MainThreadMarker
argument is most convenient to put? As the first argument (after self
, if present), or as the last one?
E.g. the difference between these two:
// As first
let window = NSWindow::initWithContentRect_styleMask_backing_defer(
NSWindow::alloc(),
MainThreadMarker::new(),
NSRect::new(0.0, 0.0),
NSWindowStyleMaskClosable
| NSWindowStyleMaskMiniaturizable
| NSWindowStyleMaskResizable
| NSWindowStyleMaskTitled,
NSBackingStoreBuffered,
false,
);
// As last
let window = NSWindow::initWithContentRect_styleMask_backing_defer(
NSWindow::alloc(),
NSRect::new(0.0, 0.0),
NSWindowStyleMaskClosable
| NSWindowStyleMaskMiniaturizable
| NSWindowStyleMaskResizable
| NSWindowStyleMaskTitled,
NSBackingStoreBuffered,
false,
MainThreadMarker::new(),
);
(Oh, fabulous contexts and capabilities, how you could relieve me of all my troubles with but the smallest of effort)!
EDIT: Parts of this is redundant since mtm.alloc()
, the rest is moved to https://github.com/madsmtm/objc2/pull/359#issuecomment-1518029961
The resolution after https://github.com/madsmtm/objc2/pull/419 is that we'll end up making most classes use interior mutability, but keep mutability in certain core classes like NSMutableString
and NSMutableArray<T>
, since that will allow us to do safe interior pointers and safe iteration.
Relating to the issues noted in the top comment, I believe most of them are addressed:
- Make
NSArray<T>
,NSDictionary<K, V>
and such collection types simpler
Tracked in https://github.com/madsmtm/objc2/issues/316, also completed by the linked PR.
- Since most frameworks use interior mutability and/or are not thread safe anyhow, the usage of the Foundation framework would match the usage of these.
Interior mutability is the default setting for icrate
, we only override it for a few select cases.
- It would be possible for users to inherit
NSString
, without them having to ensure that their object wasSync
Not sure one could do that anyhow, I think Objective-C itself may assume that NSString
is thread-safe? In any case, not a big downside.
Id::retain
could be made safe(r?)
Tracked in https://github.com/madsmtm/objc2/issues/399, obj.retain()
is safe and available for interior-mutable types.
Since the type ownership is no longer on the Id
, this problem is mitigated.
- Using
NSMutableString
inside adeclare_class!
that is not meant to be thread-safe anyhow is easier (e.g. means we won't have to use&mut
inwinit
)
You can probably do Cell<Id<NSMutableString>>
or similar to get that functionality? Or maybe RefCell<Id<NSMutableString>>
. In any case, one can always make a custom helper class like NSMutableStringInteriorMutable
instead.
So the only remaining things in this is the main thread safety stuff that is not really related to this issue, I'll track that in in https://github.com/madsmtm/objc2/pull/359 instead.
I was once convinced that we could restrict mutation to
&mut self
methods, but after having battled with AppKit and seen how Swift does it, I'm beginning to doubt that this is the best way.NSString
is immutable whileNSMutableString
is, naturally, mutable. Since our current implementation only allows mutation ofNSMutableString
through&mut self
, it is safe to make both of theseSend + Sync
; all is well and Rusty.Now, remember that
&NSMutableString -> &NSString
andId<NSMutableString, O> -> Id<NSString, O>
is safe - so if we were to make mutation ofNSMutableString
possible through&self
, not only wouldNSMutableString
be!Sync
, but so wouldNSString
! (They could still beSend
, that would simply allow moving them via.Id<T, Owned>
to a different thread - quite similar tostd::cell::Cell
)That's the primary downside: Our objects can no longer be shared across threads. Downsides that usually apply to Rust code (aliasing optimizations, ...) are void in our case, since
NSString
is alreadyUnsafeCell
.On the other hand, if we were to remove mutability we could:
NSArray<T>
,NSDictionary<K, V>
and such collection types simplerNSString
, without them having to ensure that their object wasSync
Id::retain
could be made safe(r?)NSMutableString
inside adeclare_class!
that is not meant to be thread-safe anyhow is easier (e.g. means we won't have to use&mut
inwinit
)So... Yeah, will think about this a bit, but I think we may have to sacrifice being able to use Objective-C classes across threads (exactly the same compromise Swift does, their
String
isSendable
butNSString
is not).