[WIP][Sema][stdlib] Emit a deprecation warning when a Hashable type only implements hashValue #20685
Conversation
This file contains hidden or 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
…into:) isn’t SE-206 deprecated hashValue as a protocol requirement. We should gently encourage people to migrate to hash(into:), for its more secure, easier and faster hashing. Emit a compiler warning whenever hashValue has an explicit implementation, but hash(into:) doesn’t.
This replaces the previously compiler-synthesized implementation with one that directly feeds the selector pointer to the hasher. The new definition is functionally equivalent, but warning-free and faster.
This replaces the previously compiler-synthesized implementation with one that directly feeds the wrapped value to the hasher. The new definition is functionally equivalent, but warning-free and faster.
|
This will fail testing for now. I intend to limit the warning to Swift 5 mode soon, which will hide most of the failures. However, let's first see how much effort it would be to migrate the tests over. |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
…sh(into:) SE-0206 deprecated hashValue as a Hashable requirement; the primary hashing interface is now hash(into:). We should migrate existing Hashable conformances to implement the new interface. The manual hash(into:) definitions in this batch of changes are written to prevent nested Hasher sessions, but otherwise they do the same thing as the compiler-synthesized variants — typically forwarding the actual job of hashing to an underlying [Core]Foundation type. The existing hashValue definitions are currently left intact. They should be removed later (i.e., replaced by compiler-synthesized default implementation), so that hashValue gets defined in terms of hash(into:), with random seeding etc.
The explicit implementation diverges from the original hashValue; it properly feeds all components to the hasher.
…ll bytes The hash(into:) definition diverges from hashValue in that it properly hashes *all* bytes contained in the instance, not just an arbitrary prefix.
The new definition does no attempt to replicate hashValue’s #availability check. (Swift uses randomly seeded hashing, so code must not rely on hashValue returning any specific integer value.)
The new definition simply feeds both components to the supplied hasher. This is functionally equivalent to hashValue’s original use of CFHashBytes, although it will generate different hashes.
The new definition is functionally equivalent to the original hashValue implementation, although it produces different hashes. Add FIXME about the hash encoding not being unique.
…*all* components The new definition diverges from the existing hashValue: it feeds all path components to the hasher, rather than just a limited subset. Hashing all the bits that are compared in == is necessary to ensure proper hashing.
The new definition is functionally equivalent to hashValue, although it produces different hash values.
The new definition is functionally equivalent to hashValue, although it produces different hash values.
The new definition normalizes measurements with dimensional units to their base unit, ensuring that the generated hash values are consistent with the definition of ==. It also feeds the unit to the hasher, not just the numerical value. The old hashValue definition was invalid, and has been removed. (I.e., replaced by a compiler-supplied implementation that works in terms of hash(into:).) rdar://problem/46187171
The new definition is the same as the one synthesized by the compiler.
lorentey
force-pushed
the
deprecate-hashvalue-for-real
branch
from
f2e431f to
9bc479a
Compare
|
@swift-ci please test |
This was referenced Nov 29, 2018
|
@lorentey Did everything here get landed? I see various parts in the referenced PRs. |
|
Seems like we got them all. If I'm wrong, please feel free to reopen this. |
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.
SE-206 deprecated
hashValueas aHashablerequirement, and in Swift 4.2, the stdlib's hashing collections have all migrated to usinghash(into:)as the primary hashing interface.Types that only implement
hashValueoften take improper shortcuts by omitting components due to the difficulty of combining hash values. This leads to hash collisions that can break the performance ofSetandDictionary.Hashableby providing an explicithashValuedefinition without also providinghash(into:).hashValueimplementations in place for now. (They should be removed soon.)This is unlikely to land as is: the overlay changes are interesting on their own, and they will probably be spun off into separate PR(s).
rdar://problem/39838478