swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.37k stars 10.34k forks source link

[SR-6696] Value types bridged to Objective-C must be Hashable for -isEqual: to work #49245

Open nolanw opened 6 years ago

nolanw commented 6 years ago
Previous ID SR-6696
Radar rdar://problem/36319754
Original Reporter @nolanw
Type Bug

Attachment: Download

Environment Xcode Version 9.2 (9C40b) Default toolchain
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 0 | |Component/s | Standard Library | |Labels | Bug, Runtime | |Assignee | @lorentey | |Priority | Medium | md5: b462c8bc866585f5bb9e00b013767a87

Issue Description:

When using a Swift value type in Objective-C, it seems that the value type must be Hashable in order for isEqual: to call through to the underlying Swift == implementation:

BOOL ClandIsEqual(id a, id b)
{
    return [a isEqual: b];
}
struct S: Equatable {
    let a: String

    static func == (lhs: S, rhs: S) -> Bool { return lhs.a == rhs.a }
}

let s1 = S(a: "hi")
let s2 = S(a: "hi")

print("# Just Equatable")
print("swift says \(s1 == s2)")
print("c says \(ClandIsEqual(s1, s2))")

print()

struct T: Hashable {
    let a: String

    static func == (lhs: T, rhs: T) -> Bool { return lhs.a == rhs.a }
    var hashValue: Int { return a.hashValue }
}

let t1 = T(a: "hi")
let t2 = T(a: "hi")

print("# Also Hashable")
print("swift says: \(t1 == t2)")
print("c says \(ClandIsEqual(t1, t2))")

outputs:

# Just Equatable
swift says true
c says false

# Also Hashable
swift says: true
c says true

Ideally I'd like something Equatable bridged to Objective-C to respond to isEqual: by calling the underlying == implementation. If Hashable is a hard requirement for some reason, then I guess this is a request for clearer documentation around that fact.

I've attached an Xcode project with the above code. I'm pretty sure it happens when building outside of Xcode but I didn't bother figuring out how to use a bridging header from the command line.

belkadan commented 6 years ago

cc @jckarter. We have a Radar for this, right?

jckarter commented 6 years ago

I can't find it right now, but it is a known issue.

jckarter commented 6 years ago

@swift-ci create

lorentey commented 6 years ago

The problem here is that any class that overrides -[NSObject isEqual:] must also define a corresponding implementation for -[NSObject hash], and we can't provide a good implementation of the latter for Swift types that only conform to Equatable. Cocoa objects must be both equatable and hashable; we can't have one but not the other.

Defining isEqual(_🙂 but keeping the default hash implementation is not an option. One way to resolve this would be to provide a hash override in these cases that simply traps on call. However, I think this would go against the spirit of NSObject, and could cause backward compatibility issues for Cocoa frameworks. (As in, frameworks can't evolve to start storing user-supplied objects in NSSet/NSDictionary/NSCache/etc instances if they did not already did so in their initial release.)

https://bugs.swift.org/browse/SR-7284 is a related issue, dealing with the problem that NSObject subclasses are always hashable, but the Swift values they represent may not be – so sets and dictionaries bridged to Swift may contain elements that aren't Hashable.

belkadan commented 6 years ago

We've pointed out in the past that a hash value of 0 would be valid for any such classes.

lorentey commented 6 years ago

That seems questionable advice to me; ideally we should not continue propagating it. The constant 0 is not really an appropriate hash value for any type that includes more than one distinct value. Such a hash value is attractive as a quick fix, but it causes more problems down the road, by defeating the entire point of hashing. For classes that might get hashed (i.e., all NSObject subclasses except perhaps those whose instances never get passed to frameworks outside the author's control), the only safe option is to implement proper hashing – i.e., to derive a hash value from exactly the same components that the class looks at in its isEqual implementation.

A better advice would be that types conforming to Equatable should also conform to Hashable. Adding the extra conformance usually requires minimal effort, but it unlocks a lot more functionality, and it ensures seamless interoperability with Objective-C interfaces.

belkadan commented 6 years ago

Things are a lot better now than they used to be in that regard, but it's really common to just want to make something Equatable, and not need to put it in an NSDictionary or NSSet. Right now we have a usability trap where equality silently fails. I think it's worth replacing that with a usability trap that hashing is bad.

How about splitting the difference: warning (once per type) if the bad -hash method is actually called?

lorentey commented 6 years ago

That could work!

Although I wonder how much of an overkill it would be if we would produce a compile-time warning whenever an Equatable but not Hashable type is wrapped in a _SwiftValue.

belkadan commented 6 years ago

I don't think we know when that happens. Consider myVal as Any as AnyObject.