-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider removing (non-interior) mutability? #563
Comments
I feel like I need to know more about other frameworks before I'd be ready to remove it from Also beneficial: The difficult mutability considerations in future work like #562 and #474 would become much easier. |
Note that it should be possible to re-introduce #[repr(transparent)]
pub struct Owned<T>(Id<T>);
impl Owned<T> {
// Safety: The id must be the only reference to the object.
pub unsafe fn new(id: Id<T>) -> Self {
Self(id)
}
}
impl Owned<NSString> {
pub fn to_str<'r, 's: 'r, 'p: 'r>(&'s self, pool: AutoreleasePool<'p>) -> &'r str {
// SAFETY: The NSString is not mutated for the lifetime of the
// returned string.
//
// We know this because the only methods that mutate the string is in
// `NSMutableString`, and those take `&mut self`.
unsafe { self.0.to_str(pool) }
}
// Safety: The string must not be retained past the lifetime of `&self`,
// and may not be mutated through `NSMutableString`.
pub unsafe fn as_ref(&self) -> &NSString {
&self.0
}
}
impl Deref for Owned<NSMutableString> {
type Target = Owned<NSString>;
fn deref(&self) -> &Owned<NSString> {
// SAFETY: `Owned` is `#[repr(transparent)]`, and `NSString` is a subclass of `NSMutableString`
unsafe { mem::transmute(self) }
}
}
impl DerefMut for Owned<NSMutableString> { ... }
impl Owned<NSMutableString> {
pub fn replaceCharactersInRange_withString(&mut self, range: NSRange, a_string: &NSString) {
// Safe to mutate the string because we take `&mut self`
self.0.replaceCharactersInRange_withString(range, a_string);
}
// other `&mut self` methods
} |
CC @silvanshade, @simlay and @yury. I know that you are not necessarily familiar with |
@madsmtm Yeah, this seems like a complicated and subtle issue and to be honest I'm not really sure what the right approach is in the long run. I think it's probably impossible though to be completely correct in our handling of mutability. For one, like you suggest, the surface area for the APIs is too large to reasonably cover given the resources we have, even if that wasn't a problem (and we had better tools), there's still the problem of the documentation and headers not being perfect in this regard. I also worry that Having to give up on handling mutability accurately is kind of disappointing though. On the other hand, if you look at the current state of interfacing Rust with Objective-C, And I guess even if you did remove some of the mutability-related machinery now to make things a little easier to handle overall, there would still be the possibility of introducing a more accurate interface later on if a suitable approach is found. I don't have a lot of experience programming with Objective-C or Apple frameworks in general so I don't have a lot to add about specific instances where this might be a problem. I think that just from the examples I worked on it would be a little daunting to have to have gone through and figured out where everything should have been mutable in the API and fixed that though. To me it was an acceptable trade off to work with an unsafe interface in order to have practically usable bindings. The alternative would have been rolling the bindings by hand which would have undoubtably been more unsafe. In any case, which ever path you choose here is fine with me. By the way, regarding the clang-importer work, I just want to mention that I haven't forgotten about that, just need a little break to work on something else, since I started to feel a bit burned out after all the llvmup work. But I've since made progress on libraries for another project which are relevant for the CLI part of that so I plan to get back to that stuff soon. |
I'm working on streaming app. Main challenge is to maintain battery life. So in I model via Most interesting mut/throws/safe Apple API is |
@silvanshade, in general good points, agree that loosing mutability and the safety you get from that is unfortunate, but I think the end result will end up being safer (if not only because users will understand it better, and be better equipped to determine when their
No worries at all, take all the time you need, am in no rush here! Personally, I've been doing all sorts of other stuff to avoid having to do that, it really is a daunting task!
Yeah, though as we discussed, that isn't sound, so that wouldn't really work for
Thanks for the example, I'll try to have a closer look at the AVFoundation framework! |
I looked at all usage of
Agree that this can't be modelled automatically by I'll investigate how efficient it is possible to make iterators when we have interior mutability, but still interested in further examples of where mutability might be beneficial to an Objective-C framework (especially methods that return |
I tried to use We could fix this by adding |
This may technically be a breaking change if the user implemented these protocols themselves on a `Mutable` class, but that'd be unsound anyhow, so I'll consider this a correctness fix. This is useful for wgpu, see gfx-rs/wgpu#5641, and the hack will become unnecessary after #563.
This makes it possible to still implement such traits that require mutability, even though the type itself uses interior mutability. The method through which this is accomplished differs a bit depending on the specific trait, but generally we try to implement mutable traits for both `Retained<T>` and `&Retained<T>`. Part of #563
This includes the `mutability` options, along with the IsMutable trait. Part of #563
This allows simplifying `Retained` a lot, since it is now the same as `Arc`, no funny "but sometimes `Box`" business. Additionally, we move `retain` to `Message` instead of being on `ClassType`. Part of #563
Previously, we used `CounterpartOrSelf`, which was a nice hack to work around Rust not having associated type defaults. Now, we use the user-facing `objc2_foundation::[Mutable]CopyingHelper`, which are intended to be implemented by the user alongside `NSCopying`. This is more verbose for the common case where the user just wants to say that something implements `NSCopying`, but is also better separation of concerns, and allows us to simplify `ClassType` further. We could consider a `#[derive(NSCopying)]` that does this, see: #267 Part of #563
So, it took a while, there was a lot of code that I needed to wrangle this assumption out from (it goes all the way back to the original But it's done now, yay! |
So... I've kinda considered this problem many times, #265 contains some of the previous discussion and my head contains even more, but I feel like we're still not at an optimum.
Currently,
objc2
contains a lot of complex machinery to make classes able to opt-in to being&mut
, andicrate
uses this onNSMutableString
,NSMutableAttributedString
,NSMutableArray<T>
,NSMutableSet<T>
,NSMutableDictionary<K, V>
andNSEnumerator<T>
to make accessing the inner state of those types safe.A few examples of current issues in
icrate
with this approach:NSMutableAttributedString::mutableString
must return a reference bound to the attributed string, as otherwise you'd be able to create twoId<NSMutableString>
to the same string. But this doesn't work withId<T>
, so we'd need a lifetime annotation in there somehow (or maybe resort to autorelease pools).NSTreeNode::mutableChildNodes
has to remainunsafe
, as we have no way of preventing calling that multiple times. Or maybeNSTreeNode
also has to be mutable? But that in turn would pushNSTreeController
to being mutable, which in turn needs us to changeNSEditor
.NSThread::threadDictionary
has to remainunsafe
because of thread-safety concerns, it cannot even be used from the owning thread safely.RefCell
.So while
&mut
does work for the select few types we've introduced it on, my main problem is that this is not enough!&mut
is infectious, and we need to introduce it consistently, everywhere!And that, in turn, is just not feasible (time-wise, Apple has a lot of API surface)! Especially so with our goal of making
header-translator
a user-accessible tool, which means user-controlled code, and in turn means users would also have to add extra configuration to handle this for their mutable types, which is a tough ask.Honestly, maybe we should abandon this endeavour, and instead go down the beaten path Swift has taken; do not try to do anything about mutability, i.e. mark
NSString
,NSMutableString
and so on asInteriorMutable
, and have all methods accept&self
?All of this is not to say that
&mut
doesn't have advantages; it does, very much so! And removing it does have several performance-related disadvantages, namely:NSString::as_str
would have to beunsafe
, since the internal storage for the string could now be changed through&self
. This has a perf-impact on theimpl Display for NSString
, as the&mut Formatter<'_>
in there could contain a&NSMutableString
internally that pointed to the same string, and hence we'll probably need some sort of allocation.NSData::bytes
would have to beunsafe
, which in turn means that the nice impls ofDeref
,Index
,Write
and so would be gone.NS_RETURNS_INNER_POINTER
would have to beunsafe
.array.objectAtIndex(i)
.NSString
,NSMutableString
,NSArray
, and so on would loose theirSend + Sync
-ness.Some of these performance implications could be resolved by making extra
unsafe
methods with a safety precondition that the object is not mutated for the lifetime of the return value, although that is still a worse user-experience.The text was updated successfully, but these errors were encountered: