Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager via email or direct message on the forums. If you send me email, please put "SE-0261" somewhere in the subject line.
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I like this addition because it provides a standard way to identify an entity which changes over time. In particular, it provides developers types they can use to differentiate between identity and equality.
Without this, a developer might be tempted to provide identity comparison using equality semantics (e.g.: two objects “equal” by an identifier property of the object). Access to concise code, free behaviours and operators are opened.
With that in mind and providing I’m not misunderstanding things, I’d like the standard library to offer some API which makes conforming to this type useful out of the box. I know the proposal is tightly focused, but the burden is currently on the developer to do something useful with the conformance.
One way I could be misunderstanding things is this: can I conform a type to this protocol and use the === operator with two objects or values of this conforming type? If so, that’s useful out of the box.
If not, can another proposal follow this one (perhaps with some strong candidates from SwiftUI, for example) for useful API? I understand this proposal was rushed due to an impending ABI lockdown. Would the suggested proposal be subject to the lockdown, too?
I guess I would be tempted to spell the associated type out (Identifier) even with the property being abbreviated (id), but I don’t have strong feelings about it.
I wonder how many sneaky bugs will happen because of people thinking that default id for an object won't be reused
class Foo: Identifiable { }
func suspiciousFunction(_ arr: inout [Foo]) {
// remove all existing elements
arr.removeAll()
// replace with a brand new object
arr.append(Foo())
}
var someArray = [Foo()]
let oldIDs = someArray.map { $0.id }
suspiciousFunction(&someArray)
let newIDs = someArray.map { $0.id }
assert(oldIDs == newIDs, "We don't want suspiciousFunction to add or remove any objects")
// assertion doesn't trigger, as if all old objects are still in the array
It's not guaranteed to trigger. The original Foo is gone, and the new one may use the same id. It's a very common pitfall to compare reusable id for identity.
Default implementation of id uses ObjectIdentifier which is basically the address of an object. When I run the code sample I pasted, it just so happens that after original object is deinited, the object created by suspiciousFunction is allocated in the exact same place, which gives it the same ObjectIdentifier which makes it look like it's the old object. As Lantua said it's not guaranteed.
I'd like to focus specifically on the question: "Does this proposal fit well with the feel and direction of Swift?"
Recognizing that there is value in getting this protocol incorporated in time for the Swift 5.1 ABI, just as there was value in having Result in time for the Swift 5.0 ABI, I hope that there will be a plan of record in short order as to how better to integrate into the rest of the standard library.
Our documentation for ObjectIdentifier reads as follows:
In Swift, only class instances and metatypes have unique identities. There is no notion of identity for structs, enums, functions, or tuples.
This obviously will change, and changing the documentation is straightforward, but it is an indication that this protocol does revise existing assumptions built into the design of the standard library.
If what's proposed here had originated from within the standard library itself instead of being an existing protocol sunk down from SwiftUI, I imagine that the more consistent naming would have been CustomIdentifiable (in the same vein as CustomStringConvertible). We would then say that all class instances and metatypes have unique identities by default, and structs/enums/functions/tuples have no unique identities by default, but any type that can conform to a protocol can be given a custom notion of identity by conforming to CustomIdentifiable.
Along the same train of thought, it would then make sense for ObjectIdentifier to have an initializer init<T: [Custom]Identifiable>(_: T), and it would be reasonable to have a discussion as to whether ===<T: [Custom]Identifiable>(lhs: T, rhs: T) should exist. (The issue is that such an overload, if we're to go with the notion that the protocol allows for a custom notion of identity, should be preferred over the non-customized version).
None of this needs to happen immediately, and--again--having the protocol in time for Swift 5.1 has value. However, it would be unfortunate if such additions continue to be reviewed on an expedited basis without follow-up to work on the "fitting well" aspect of things. The analogous follow-on discussion for Result, for example, has not yet taken place despite statements of intention to do so.
An instance of a reference type maintains its identity even when it is mutated; that is, after all, the point. If an object of the same type exists at the same address, how is that meaningfully different from mutating the existing object?
There's no guarantee that the second object would have the same type. Also, you cannot mutate immutable objects, but you can delete old and create a new one with different contents. Recipe for disaster if you have a cache of something computed from that contents, keyed by 'id'
It is probably worth calling out in the proposal text that Identifiable is designed to compose well with a protocol like Equatable to actually perform a diff operation featuring changes. As it is written, it kinda sounds like just conforming to Identifiable on its own is all that is needed for identity-bound value diffing to be possible.
There is also a fair amount of prior art in the community of people creating a protocol named Identifiable as exactly that, Equatable + an id requirement, so calling out the distinction would eliminate some confusion when people come to migrate code.
The underlying problem is a semantic problem with the proposal. It fails to specify whether (a) the lifetime of the Identifier is the same as the lifetime of the instance it identifies, or (b) the Identifer for an instance is "consumed" for the lifetime of the app execution.
If the answer is (a), as implied by the proposed default implementation, then I would be a huge -1 on there being a default implementation at all, since it's incredibly dangerous for unsuspecting users of this protocol. In that case, I think the protocol needs to be implemented manually. (The implementer could decide to use ObjectIdentifier if that was appropriate in a particular case.)
If the answer is (b), then a default implementation would be fine, just not the one proposed.
Identifiable sounds like the same concept as the one for the identity operators (=== and !==), yet it's not that clear they're the same or that they should be the same. I think a clarification is needed: either Identifiable is linked to the identity operators, or one of the two (identifiable / identity operators) should use another name.
I agree the memory address of an object can be a treacherous identifier given the address gets reused. This is not an issue for === because it can only compare living objects, but as soon as you extract the address without keeping the object alive you end up having multiple things with the same identifier.
Perhaps ObjectIdentifier should somehow prevent the address from being reused while the identifier lives; that'd make it a better identifier. I recently abandoned the use of ObjectIdentifier in many places over this issue.
ObjectIdentifier must not retain the object in question, and there's no other way to keep an arbitrary Objective-C object's memory from being freed. Beyond that, it's expected to be trivially-copyable, so it can't really do anything that has side effects. It may not be the tool for your job, but it's not something we can change.