[swift-evolution] Pitch: Remove `NonObjectiveCBase` and replace `isUniquelyReferenced` by `isUniquelyReferencedUnsafe`

Mon Jul 18 16:49:06 CDT 2016

TL;DR:  Remove the isUniquelyReferenced<T: NonObjectiveCBase>(_ object: T) API. All its current use cases can be satisfied equally performant by the isUniquelyReferencedNonObjC<T : AnyObject>(_ object: T) API.

This will reduce API surface by both the NonObjectiveCBase class and the isUniquelyReferenced API.

I think this is pretty uncontroversial, too?

[Today’s implementation of both APIs is the same builtin call. The compiler can optimize out the check for objective-c class ancestry when it sees that the static type of ‘object’ is not an @objc-class.]

> On Jul 16, 2016, at 10:50 PM, Arnold Schwaighofer via swift-evolution <swift-evolution at swift.org> wrote:
> 
> Updated proposal:
> 
> Remove NonObjectiveCBase and isUniquelyReferenced
> 
> 	• Proposal: SE-0000
> 	• Author: Arnold Schwaighofer
> 	• Status: Pitch
> 	• Review manager: TBD
> Introduction
> 
> Remove NonObjectiveCBase and isUniquelyReferenced<T: NonObjectiveCBase>(_ object: T). isUniquelyReferenced can be replaced by isUniquelyReferencedNonObjC<T: AnyObject>(_ object: T). This replacement is as performant as the call to isUniquelyReferenced in cases where the compiler has static knowledge that the type of object is a native Swift class. This change will remove surface API.
> 
> 	• Swift-evolution thread: Pitch
> 	• Swift bug: SR-1962
> 	• Branch with change to stdlib: remove_nonobjectivecbase
> Motivation
> 
> Today we have isUniquelyReferenced which only works on subclasses of NonObjectiveCBase, and we have isUniquelyReferencedNonObjC which also works on @objc classes.
> 
> class SwiftKlazz : NonObjectiveCBase
>  {}
> 
> class ObjcKlazz :
>  NSObject {}
> 
> expectTrue(
> isUniquelyReferenced
> (SwiftKlazz()))
> expectFalse(
> isUniquelyReferencedNonObjC
> (ObjcKlazz()))
> 
> 
> // Would not compile:
> 
> expectFalse(
> isUniquelyReferenced(ObjcKlazz()))
> In most cases we expect developers to be using the ManagedBufferPointer type. In cases where they want to use a custom class they would use isUniquelyReferenced today and can use isUniquelyReferencedNonObjC in the future.
> 
> class
>  SwiftKlazz {}
> 
> expectTrue(
> isUniquelyReferencedNonObjC(SwiftKlazz()))
> Removing isUniquelyReferenced<T : NonObjectiveCBase> will allow us to remove the NonObjectiveCBase class from the standard library thereby further shrinking API surface.
> 
> Proposed solution
> 
> Remove isUniquelyReferenced<T : NonObjectiveCBase> and remove the NonObjectiveCBase class from the standard library. Clients of the the isUniquelyReferenced API can be migrated to use isUniquelyReferencedNonObjC. In most cases -- where the type of the object parameter is statically known to be a native non- at objc class -- the resulting code will have identical performance characteristics. In cases where the type is statically not known it will have the slight overhead of first checking that the dynamic type is not an @objc class.
> 
> Detailed design
> 
> Todays APIs that can be used to check uniqueness is the family of isUniquelyReferenced functions.
> 
> /// Returns `true` iff `object` is a non-`@objc` class instance with
> /// a single strong reference.
> ///
> /// * Does *not* modify `object`; the use of `inout` is an
> ///   implementation artifact.
> /// * If `object` is an Objective-C class instance, returns `false`.
> /// * Weak references do not affect the result of this function.
> ///
> /// Useful for implementing the copy-on-write optimization for the
> /// deep storage of value types:
> ///
> ///     mutating func modifyMe(_ arg: X) {
> ///       if isUniquelyReferencedNonObjC(&myStorage) {
> ///         myStorage.modifyInPlace(arg)
> ///       }
> ///       else {
> ///         myStorage = self.createModified(myStorage, arg)
> ///       }
> ///     }
> public func isUniquelyReferencedNonObjC<T : AnyObject>(_ object: inout T) -> Bool
> public func isUniquelyReferencedNonObjC<T : AnyObject>(_ object: inout T?) -> Bool
> 
> 
> 
> /// A common base class for classes that need to be non-`@objc`,
> /// recognizably in the type system.
> public class NonObjectiveCBase
>  {
>   
> public init
> () {}
> }
> 
> 
> public func isUniquelyReferenced<T : NonObjectiveCBase
> >(
>   
> _ object: inout T
> 
> ) 
> -> Bool
> And the somewhat higher level APIs that can be used to model a storage with several elements ManagedBufferPointer.
> 
> /// Contains a buffer object, and provides access to an instance of
> /// `Header` and contiguous storage for an arbitrary number of
> /// `Element` instances stored in that buffer.
> ///
> /// For most purposes, the `ManagedBuffer` class works fine for this
> /// purpose, and can simply be used on its own.  However, in cases
> /// where objects of various different classes must serve as storage,
> /// `ManagedBufferPointer` is needed.
> ///
> /// A valid buffer class is non-`@objc`, with no declared stored
> ///   properties.  Its `deinit` must destroy its
> ///   stored `Header` and any constructed `Element`s.
> /// `Header` and contiguous storage for an arbitrary number of
> /// `Element` instances stored in that buffer.
> public struct ManagedBufferPointer<Header, Element> : Equatable
>  {
>   
> /// Create with new storage containing an initial `Header` and space
> 
>   
> /// for at least `minimumCapacity` `element`s.
> 
>   
> ///
> 
>   
> /// - parameter bufferClass: The class of the object used for storage.
> 
>   
> /// - parameter minimumCapacity: The minimum number of `Element`s that
> 
>   
> ///   must be able to be stored in the new buffer.
> 
>   
> /// - parameter initialHeader: A function that produces the initial
> 
>   
> ///   `Header` instance stored in the buffer, given the `buffer`
> 
>   
> ///   object and a function that can be called on it to get the actual
> 
>   
> ///   number of allocated elements.
> 
>   
> ///
> 
>   
> /// - Precondition: `minimumCapacity >= 0`, and the type indicated by
> 
>   
> ///   `bufferClass` is a non-`@objc` class with no declared stored
> 
>   
> ///   properties.  The `deinit` of `bufferClass` must destroy its
> 
>   
> ///   stored `Header` and any constructed `Element`s.
> 
>   
> public init
> (
>     bufferClass: 
> AnyClass
> ,
>     minimumCapacity: 
> Int
> ,
>     initialHeader: 
> @noescape (buffer: AnyObject, capacity: @noescape (AnyObject) -> Int) throws ->
>  Header
>   ) 
> rethrows
> 
> 
>   
> /// Returns `true` iff `self` holds the only strong reference to its buffer.
> 
>   
> ///
> 
>   
> /// See `isUniquelyReferenced` for details.
> 
>   
> public mutating func holdsUniqueReference() -> Bool
> 
> 
>   
> /// Returns `true` iff either `self` holds the only strong reference
> 
>   
> /// to its buffer or the pinned has been 'pinned'.
> 
>   
> ///
> 
>   
> /// See `isUniquelyReferenced` for details.
> 
>   
> public mutating func holdsUniqueOrPinnedReference() -> Bool
> 
> 
>   
> internal var _nativeBuffer: Builtin.
> NativeObject
> }
> 
> 
> /// A class whose instances contain a property of type `Header` and raw
> /// storage for an array of `Element`, whose size is determined at
> /// instance creation.
> public class ManagedBuffer<Header, Element>
> 
>   
> : ManagedProtoBuffer<Header, Element>
>  {
> 
>   
> /// Create a new instance of the most-derived class, calling
> 
>   
> /// `initialHeader` on the partially-constructed object to
> 
>   
> /// generate an initial `Header`.
> 
>   
> public final class func create
> (
>     minimumCapacity: 
> Int
> ,
>     initialHeader: 
> @noescape (ManagedProtoBuffer<Header, Element>) throws ->
>  Header
>   ) 
> rethrows -> ManagedBuffer<Header, Element>
>  {
> 
>     
> let p = try ManagedBufferPointer<Header, Element>
> (
>       bufferClass: 
> self
> ,
>       minimumCapacity: minimumCapacity,
>       initialHeader: { buffer, _ 
> in
> 
>         
> try
>  initialHeader(
>           
> unsafeDowncast(buffer, to: ManagedProtoBuffer<Header, Element>.self
> ))
>       })
> 
>     
> return unsafeDowncast(p.buffer, to: ManagedBuffer<Header, Element>.self
> )
>   }
> }
> 
> We propose to remove the NonObjectiveCBase class and isUniquelyReferenced<T: NonObjectiveCBase>(_ object: T>.
> 
> Impact on existing code
> 
> Existing code that uses isUniquelyReferenced will need to remove the NonObjectiveCBase base class and replace calls to isUniquelyReferenced by isUniquelyReferencedNonObjC. The old API will be marked unavailable to help migration.
> 
> Alternatives considered
> 
> Leave the status quo and pay for type safety with additional API surface. Another alternative we considered -- the first version of this proposal -- was to replace the isUniquelyReferenced API by an isUniquelyReferencedUnsafe<T: AnyObject>(_ object: T) API that would assume the object to be a non- at objc class and only check this precondition under -Onone. There is however no good reason to keep this API given that the isUniquelyReferencedNonObjC is as performant when the type is statically known to be non- at objc class.
>> On Jul 16, 2016, at 10:12 PM, Arnold via swift-evolution <swift-evolution at swift.org> wrote:
>> 
>> Sent from my iPhone
>> 
>> On Jul 16, 2016, at 9:41 PM, Arnold <aschwaighofer at apple.com> wrote:
>> 
>>> 
>>> 
>>> Sent from my iPhone
>>> 
>>> On Jul 16, 2016, at 9:23 PM, Andrew Trick <atrick at apple.com> wrote:
>>> 
>>>> 
>>>>> On Jul 16, 2016, at 9:17 PM, Arnold <aschwaighofer at apple.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On Jul 16, 2016, at 8:45 PM, Andrew Trick <atrick at apple.com> wrote:
>>>>> 
>>>>>> 
>>>>>>> On Jul 16, 2016, at 8:36 PM, Arnold <aschwaighofer at apple.com> wrote:
>>>>>>> 
>>>>>>> Thank you for the feedback. Answers online.
>>>>>>> 
>>>>>>> Sent from my iPhone
>>>>>>> 
>>>>>>> On Jul 16, 2016, at 7:38 PM, Andrew Trick <atrick at apple.com> wrote:
>>>>>>> 
>>>>>>>> 
>>>>>>>>> On Jul 16, 2016, at 6:46 PM, Arnold Schwaighofer via swift-evolution <swift-evolution at swift.org> wrote:
>>>>>>>>> 
>>>>>>>>> Replace isUniquelyReferenced<T : NonObjectiveCBase> by isUniquelyReferencedUnsafe<T: AnyObject> and remove the NonObjectiveCBase class from the standard library.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> So we’ll have:
>>>>>>>> 
>>>>>>>> - isUniquelyReferencedNonObjC(object): true iff object is uniquely referenced and NonObjC
>>>>>>>> 
>>>>>>>> - isUniquelyReferencedUnsafe(object): true iff object is uniquely reference, assert NonObjC
>>>>>>>> 
>>>>>>>> I’m going to be picky. The “Unsafe” suffix doesn’t make sense to me. If you think this is an unsafe API then it should be:
>>>>>>>> “unsafeIsUniquelyReferenced”.
>>>>>>>> 
>>>>>>>> But I don’t really see how it is unsafe in the usual sense. If you want to convey that the programmer needs to satisfy some precondition, which is not generally associated with unsafety, then it should be:
>>>>>>>> “isUniquelyReferencedAssumingNonObjC”
>>>>>>>> 
>>>>>>> 
>>>>>>> Makes sense to me. I think it is unsafe in the sense if you don't satisfy the precondition the resulting behavior is undefined in modes other than -Onone and not checked by a precondition predicate that traps.
>>>>>>> 
>>>>>>>> unsafeIsUniquelyReferenced
>>>>>>> 
>>>>>>> I find it kind of nice to recognize a predicate by the 'is' prefix. All unsafe APIs start with the word unsafe though. I could not find an unsafe freestanding predicate.
>>>>>>> 
>>>>>>> [As the implementor of the underlying builtin you may remember that it is not actually undefined and will return a implementation defined value (false) for objc classes. But we might not want to guarantee this going forward.]
>>>>>> 
>>>>>> Oh yeah. I think I only kept the two versions for fear of breaking the API. Since you’re renaming the second one anyway, why not just delete it with a fixit that it's renamed to isUniquelyReferencedNonObjC?
>>>>>> 
>>>>>> The “assuming” version of the API is extremely confusing in addition to being useless.
>>>>> 
>>>>> The unsafe version would allow us to emit more efficient code (in the future) for a static unknown object type but we know it is a native type (but not which so we can't just cast it, this is public API so we can't cast to Builtin.NativeObject).
>>>>> 
>>>>>   var self: AnyObject // really: AnyNativeObject
>>>>>   ...
>>>>>   if (!unsafeIsUniquelyReferenced(&self))
>>>>>     self = self.copy()
>>>>>   }
>>>>> 
>>>>> I admit this is somewhat contrived and am happy to just nuke the API if we agree there is no value in the use case above.
>>>> 
>>>> There is no sense advertising this API under some new name if it hasn’t even been implemented. The API can be added when it makes sense.
>>>> 
>>>> +1 for eliminating it.
>>> 
>>> Today you can implement something similar using NonObjectiveCBase as your base class:
>>> 
>>>  var self: NonObjectiveCBase
>>>  ...
>>>  if (isUniquelyReferenced(&self) {...}
>>> 
>>> And get the runtime performance of the native check.
>> 
>> Actually, this code could just implement their own 'class NonObjCBase' base class and just use isUniquelyReferencedNonObj for the same performance as before ...
>>> 
>>> If we implemented 'unsafeIsUniquelyReferenced' we would just unsafeBitCast the 'object' argument to Builtin.NativeObject and get the same performance.
>>> 
>>> I admit that this may be far fetched but I am trying to anticipated possible use cases that work today.
>>> 
>>> That use case will still work after nuking the API using the 'NonObjC' variant albeit slightly slower.
>>> 
>>> If we need to support it with best performance we can always bring the API back as you said.
>>> 
>>> +1 for nuking it from me
>>> 
>>> I will change the proposal.
>> _______________________________________________
>> swift-evolution mailing list
>> swift-evolution at swift.org
>> https://lists.swift.org/mailman/listinfo/swift-evolution
> 
> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org
> https://lists.swift.org/mailman/listinfo/swift-evolution