Foundation ownership fixes #40
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
madsmtm
force-pushed
the
ownership-fixes
branch
from
1b122f7
to
cd7b9d4
Compare
Immutable types like NSString implement `copy` by simply retaining the pointer; hence, having an `Owned` NSString (which allows access to `&mut NSString`) is never valid, since it would be possible to create an aliased mutable reference. So instead we now have an `Ownership` type on INSObject that indicates whether the type is mutable. Fixes SSheldon/rust-objc-foundation#10 (see that issue for alternative ways to fix this).
INSArray::Own -> INSArray::ItemOwnership INSDictionary::Own -> INSDictionary::ValueOwnership Also remove `NSSharedArray` and `NSMutableSharedArray`
Also add a few bound checks since throwing exceptions from Objective-C is still UB
madsmtm
force-pushed
the
ownership-fixes
branch
from
5ca96d1
to
1d53a3c
Compare
See the code comments and my explanation here: SSheldon/rust-objc#103 (comment) This also has the nice "side-effect" of fixing the memory leaks that as_str was otherwise exhibiting when using non-ascii strings, see SSheldon/rust-objc-foundation#15.
madsmtm
force-pushed
the
ownership-fixes
branch
from
1d53a3c
to
518a05c
Compare
The `Any` bound is equivalent to 'static, and that is too restrictive; we want to allow creating custom classes which reference something from the outer environment
Anything else would be unsound, since the user must verify that the object actually implements the protocols / inherits the classes!
madsmtm
force-pushed
the
ownership-fixes
branch
from
0b42ca9
to
08f7e6d
Compare
NSValue implements `INSObject`, and therefore has a blank `new` method, but the extractor method did not account for this discrepancy.
madsmtm
force-pushed
the
ownership-fixes
branch
2 times, most recently
from
fdcecc4
to
2fb4fb8
Compare
Some classes (NSValue is the current example of this, but it's probably not the only case) don't want to handle cases where the users didn't supply a value, and implement `init` with throwing an exception or returning `nil`. We could panic in those cases (though we can't currently catch GNUStep exceptions), but instead we should just simply not provide a way to construct these invalid instances.
madsmtm
force-pushed
the
ownership-fixes
branch
from
e1cd54f
to
dc0cb3a
Compare
madsmtm
added a commit
that referenced
this pull request
Nov 2, 2021
madsmtm
added a commit
that referenced
this pull request
Nov 2, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Intended to fix most of the soundness issues discussed in #29.
Ownershiptype toINSObjectto allow specifying immutable objects.INSObject,INSCopying,INSString, ...) unsafenewreturnnil, so I chose to removeINSObject::newsince some classes clearly don't want to be constructed without their desired data. This is probably a temporary solution...)NSString#24, also included the test for\u{feff}strings uncovered in TestNSStringusing the "Big List of Naughty Strings" #47 and TestNSStringusing the "Big List of Naughty Strings" #47)I also renamed a lot of methods to better match Rust naming.