Modernize hashing in Foundation's Swift-only types #2118
Conversation
This is to prevent the compiler from synthesizing these for conforming types.
| @@ -75,7 +75,9 @@ internal protocol _SwiftNativeFoundationType : class { | |||
|
|
|||
| func mutableCopy(with zone : NSZone) -> Any | |||
|
|
|||
| func hash(into hasher: inout Hasher) | |||
There was a problem hiding this comment.
In protocol extensions that forward hashing to some core value, it is generally a good idea to implement both hash(into:) and hashValue, so that hashing always matches the behavior of the underlying value. (But if we have to choose one, I'd go with just hash(into:).
|
@swift-ci please test |
| } else { | ||
| hasher.combine(true) |
There was a problem hiding this comment.
The original implementation here was ever so slightly wrong -- it had a variable-length hash encoding without discriminators to mark its boundaries, which could (theoretically) lead to collisions when multiple Calendar values are fed into a hasher.
Granted, I doubt this would be a problem in practice; however, this change makes Calendar's hash encoding match that of the Foundation overlay, which is nice. I made similar changes for TimeZone and Locale.
| return self.rawValue.hashValue | ||
|
|
||
| public func hash(into hasher: inout Hasher) { | ||
| hasher.combine(rawValue) |
There was a problem hiding this comment.
This hash(into:) and the == below are identical to the definitions that the compiler would synthesize on its own. It may be worth considering removing them here (as well as all other RawRepresentable enums).
There was a problem hiding this comment.
I agree, esp. given that on Darwin it's the compiler that synthesizes those conformances, right?
There was a problem hiding this comment.
I looked into it in more detail: on Darwin the hash implementations come from extensions on _SwiftNewtypeWrapper that are identical to similar extensions provided on RawRepresentable. The stdlib also defines a == that’s generic over RawRepresentable types; this gets picked up for Equality conformance.
So if we removed these, then the same definitions would be found on RawRepresentable instead of compiler synthesis. But the end result is effectively the same; we should do it!
| @@ -46,7 +46,7 @@ open class NSCharacterSet : NSObject, NSCopying, NSMutableCopying, NSCoding { | |||
| } | |||
|
|
|||
| open override var hash: Int { | |||
| return Int(bitPattern: CFHash(_cfObject)) | |||
| return Int(bitPattern: _CFNonObjCHash(_cfObject)) | |||
There was a problem hiding this comment.
This fixes an infinite recursion in NSCharacterSet.hash.
| var hash: Int { | ||
| return self.x.hashValue &+ self.y.hashValue | ||
|
|
||
| func hash(into hasher: inout Hasher) { |
There was a problem hiding this comment.
The Hashable conformances for CGPoint, CGSize and CGRect are inconsistent with Apple platforms, where these types aren't currently Hashable. (See https://bugs.swift.org/browse/SR-10476.)
Ah, I missed some CMakeLists.txt additions. The macOS failure is different -- perhaps it just needs a clean build. |
|
@swift-ci please clean test |
|
@swiftix please test clean |
|
@swift-ci please test |
|
@swift-ci test macos |
| /// | ||
| /// - Note: `oracle` is also checked for conformance to the | ||
| /// laws. | ||
| public func checkEquatable<Instances: Collection>( |
There was a problem hiding this comment.
Are we okay having a duplicate implementation of this between SCF and the stdlib tests, or is there something we can do?
This does not block the patch, but it's a point of concern going forward.
There was a problem hiding this comment.
Oh yes! The XCTest-StdlibUnittest dichotomy is super annoying, we should do something about that. Ideally we would have a single unified testing infrastructure.
The high-level behavioral tests should be shared between all platforms, too.
This is the corelibs-Foundation counterpart to swiftlang/swift#23832 for the Foundation SDK overlay. See there for a full description and rationale.
This PR attempts to bring hashing roughly in sync in the two Foundation flavors, although some inconsistencies remain. Like in swiftlang/swift#23832, I'll post self-review notes below to highlight some points.