[WIP] Conditionally conform stdlib types to Hashable #14527

lorentey · 2018-02-10T18:48:11Z

As a followup to SE-0143, this adds conditional conformance to Hashable to the following types in the standard library:

Optional
Array, ContiguousArray, ArraySlice
Dictionary
Range, ClosedRange
AnyCollection
DictionaryLiteral
CollectionOfOne

This PR also adds unconditional conformance to Hashable for EmptyCollection.

This makes synthesized Hashable implementations (SE-0185) available for structs and enums that include fields of these types, freeing programmers from having to manually deal with hash combinators.

As part of this work, this PR also adds conditional conformances to Equatable to the following types:

AnyCollection
DictionaryLiteral
CollectionOfOne

Resolves SR-6910.

@lorentey

…d Array types. (#14247) * Add conditional Hashable conformance to Optional, Dictionary, Array, ArraySlice and ContiguousArray * Modified hashValue implementations The hashValues are now calculated similar to the automatically synthesized values when conforming to Hashable. This entails using _combineHashValues as values of the collections are iterated - as well as calling _mixInt before returning the hash. * Added FIXMEs as suggested by Max Moiseev * Use checkHashable to check Hashable conformance * Use 2 space indentation * Hashing of Dictionary is now independent of traversal order * Added a test to proof failure of (previous) wrong implementation of Dictionary hashValue. Unfortunately it does not work. * Removed '_mixInt' from 'hashValue' implementation of Optional and Array types based on recommendations from lorentey * Another attempt at detecting bad hashing due to traversal order * Dictionary Hashable validation tests now detect bad hashing due to dependence on traversal order * Removed superfluous initial _mixInt call for Dictionary hashValue implementation. * Add more elements to dictionary in test to increase the number of possible permutations - making it more likely to detect order-dependent hashes * Added Hashable conformance to CollectionOfOne, EmptyCollection and Range types * Fix indirect referral to the only member of CollectionOfOne * Re-added Hashable conformance to Range after merge from master * Change hashValue based on comment from @lorentey * Remove tests for conditional Hashable conformance for Range types. This is left for a followup PR * Added tests for CollectionOfOne and EmptyCollection * Added conditional conformance fo Equatable and Hashable for DictionaryLiteral. Added tests too. * Added conditional Equatable and Hashable conformance to Slice * Use 'elementsEqual' for Slice equality operator * Fixed documentation comment and indentation * Fix DictionaryLiteral equality implementation * Revert "Fix DictionaryLiteral equality implementation" This reverts commit 7fc1510. * Fix DictionaryLiteral equality implementation * Use equalElements(:by:) to compare DictionaryLiteral elements * Added conditional conformance for Equatable and Hashable to AnyCollection * Revert "Use 'elementsEqual' for Slice equality operator" This reverts commit 0ba2278. * Revert "Added conditional Equatable and Hashable conformance to Slice" This reverts commit 84f9934. * Added conditional conformance for Equatable and Hashable for ClosedRange

lorentey · 2018-02-10T18:54:36Z

This is a continuation of @mortenbekditlevsen's PR #14247; having a dedicated branch makes it easier to collaborate on landing this change.

xwu · 2018-02-10T18:59:01Z

@lorentey Perhaps this is obvious to all involved already, but I am discovering that in fact Swift ignores @available attributes for extensions, so it appears that we can't use that solution for the standard library changes here to coordinate with SwiftPM.

lorentey · 2018-02-10T19:13:44Z

@Xvu That's unfortunate; I think we may still get it to work by also adding the attribute to the individual members that implement the conformances. (At the cost of adding more warnings to stdlib's build logs.)

Or we may just leave these types out for now; however, shipping conditional conformances without full stdlib coverage would probably lead to people adding these conformances in their own code, causing even more issues. cc @airspeedswift

xwu · 2018-02-10T19:36:20Z

We didn't have this problem with Array, for instance, because == was already provided by the standard library since it was so useful even without conditional conformance.

I really think DictionaryLiteral qualifies as a corner case that can be dropped if needed. If you think about it, the scenario we're in arose because DictionaryLiteral didn't ship with ==, not because now we're trying to state the conditional conformance.

lorentey · 2018-02-10T20:32:42Z

I agree that DictionaryLiteral should not delay this PR; we can add its conformances later(*), if this proves to be a difficult issue. (We've already punted Slice to a followup PR.) But we're not yet ready to merge this PR anyway, so we have a little time to work on a solution before doing that.

The hashValue implementations in this PR are all new, and they could easily cause similar compatibility issues in other packages. (Thankfully swiftpm is not affected.) They need to have the same availability attribute.

(*) However, later cannot mean a subsequent Swift release, because that would leave the door wide open for even more serious problems. If the stdlib didn't provide conditional Equatable conformance to DictionaryLiteral etc., then the logical next step for packages currently implementing '==' overloads would be to add these conformances on their own. (What would happen when someone tried to use two of these packages in the same binary?)

Ideally, the stdlib should add conformances for its own protocols to its own types whenever doing that makes even the slightest sense, to prevent packages from trying to do it themselves.

lorentey · 2018-02-10T20:49:35Z

Adding @availability attributes to DictionaryLiteral.== in the stdlib and swiftpm (introduced and obsoleted, respectively) resolves the test failure. It also allows swiftpm to keep source compatibility with previous compiler versions (but not with earlier revisions of 4.1/5.0 in git).

xwu · 2018-02-10T20:51:17Z

Well, actually... A key difference between hashValue and == is that the latter is independently useful. There's essentially no point in implementing hashValue without conformance to Hashable, and since conditional conformances have not previously been available, I doubt that there are packages "out there" for which these problems arise. If adding @availability attributes to == will successfully fix problems, then we should do that, but I don't think it's necessary for hashValue.

As to your general point, I would disagree that the bar for adding conditional conformances for standard library protocols is that it "makes even the slightest sense." When there are multiple reasonable semantics for doing so, adopting one such conditional conformance bars end users from conforming in any other way.

This is particularly problematic when the standard library type in question is generic: It may make extraordinary sense to conform StandardLibraryType<MyCustomType> to StandardLibraryProtocol with some particular semantics, but the standard library vending a "slightest-sense" implementation of StandardLibraryType<T> : StandardLibraryProtocol where T : SomeOtherConstraint would stop me from doing so.

In general, we should reiterate that conforming a type you don't own to a protocol you don't own is not advised. We don't need to undertake heroics to design around such shenanigans, just as we don't in general make design decisions explicitly to prevent abuses.

lorentey · 2018-02-10T21:31:45Z

The "slightest sense" argument clearly does not apply to DictionaryLiteral, for which Equatable conformance obviously makes perfect sense. You may be right about hashValue, but why have the compatibility risk?

In any case, my opinion is that users should never, ever conform stdlib types to stdlib protocols, conditionally or otherwise -- that is the sole prerogative of the standard library. The way to customize/override the behavior of stdlib types is to wrap them in little structs, making the deviation explicit. Do you have an example where such a conformance may in fact be necessary? (We should probably take this to the forums, though.)

xwu · 2018-02-10T21:44:47Z

I think we largely agree on the general principle.

As to the appending of attributes, have you explored avoiding build warnings by instead using conditional compilation directives?

lorentey · 2018-02-10T22:03:57Z

#if swift(>=x.y) works great in swiftpm and other external packages, but sadly it doesn't work inside the stdlib. (We don't make separate builds of the stdlib for each distinct language version supported by the compiler, because we need to support linking code built in different version modes together in the same binary.)

If @available applied to conformances, then that would fix the warnings, though.

…tions Now that Array and Dictionary conform to Hashable, we need to make sure that their bridged counterparts provide the same hash values when converted to AnyHashable.

[conditional-hashable] Silence warning (unused binding) and break to 80 characters

@available

This assumes these will land in Swift 4.1; the attributes need to be adjusted if that turns out not to be the case. It seems @available for protocol conformances is not yet functional. I added attributes for those anyway, marked with FIXME(conformance-availability). # Conflicts: # stdlib/public/core/ExistentialCollection.swift.gyb # stdlib/public/core/Mirror.swift

…texts yet Array and Dictionary are now conditionally Hashable, so the importer wants to use them when importing NSArray and NSDictionary types in hashable contexts. Unfortunately, this currently means that a type like NSSet<NSDictionary<NSString *, id> *> * gets imported as Set<Dictionary<String, Any>>, which is invalid — Dictionary.Value needs to be Hashable, too: Set<Dictionary<String, AnyHashable>> For now, work around this by explicitly turning NSArray and NSDictionary into AnyHashable when they are used as the first type parameter of NSSet or NSDictionary, ignoring Hashable conformance in this case. This reverts to the previous behavior.

lorentey · 2018-02-11T02:00:59Z

@swift-ci please test

lorentey · 2018-02-11T02:02:47Z

@swift-ci please test source compatibility

lorentey · 2018-02-11T02:03:21Z

@swift-ci please smoke test compiler performance

lorentey · 2018-02-11T02:09:27Z

I pushed some changes to fix the two failures in the Swift tests. I also added @available attributes to pave the way for a fix for the compatibility issue with swiftpm.

I expect the Linux build will still fail for now. (In fact, the package manager failure will prevent the compiler performance and source compatibility tests from succeeding, either. D'oh.)

…mbers

xwu · 2018-02-11T02:23:45Z

Since you've annotated ClosedRange.Index, I've taken the same approach to add in the remainder of the annotations to changes in #12777.

swift-ci · 2018-02-11T02:37:45Z

Build comment file:

Compilation-performance test failed

lorentey · 2018-02-11T02:45:35Z

Oops, that was a serious oversight on my part; I intended to annotate ClosedRange itself. 🤦‍♂️💦

But if we mark the ones we add now, we should also mark the conformances added for SE-0188, so let's consider it a lucky mistake. (We may end up choosing to remove all of these attributes but the one for DictionaryLiteral.==, though. Your point about hashValue not likely to be defined elsewhere is valid.)

lorentey · 2018-02-11T02:47:19Z

@swift-ci Please test OS X platform

xwu · 2018-02-11T06:51:48Z

Fun: @_implements doesn't work in an extension at the moment. :/

…plements" This reverts commit b5a1d4b, reversing changes made to d03f0b3.

…ations

When implementing a protocol requirement, additional doc comments are unnecessary unless there are additional semantics peculiar to the specific implementation. The manually propagated documentation had several problems, including copypasta and inconsistent terminology. Therefore, if `T.==` doesn't have any doc comments and the implementation of `T.hashValue` is unremarkable, remove the unnecessary doc comment.

…ashValue

xwu · 2018-02-11T09:23:08Z

@swift-ci Please smoke test

xwu · 2018-02-11T20:24:36Z

@lorentey We don't need @available attributes for hashValue. Swift does the right thing here: end user definitions shadow library-vended ones without any ambiguity.

[Edit: As long as the user isn't in turn vending a public implementation of hashValue as part of a module for downstream consumption, which would then cause ambiguity at that use site downstream because two imported modules that don't shadow each other would conflict. But that's just...madness.]

The one scenario where this fails is in the case of operators. Down the road, we'll want to fix that issue definitively; operator overload resolution has all sorts of messy issues at the moment.

xwu · 2018-02-11T20:36:29Z

With respect to Equatable conformance, though, with time I'm still not very happy with this @available workaround.

The underscored attribute @_implements was created for this purpose and we should make it work appropriately. With such an implementation, end users can continue to have their custom == shadow our implementation if they have defined one and seamlessly enjoy the benefit of Hashable conformance.

Until then, if the worry is that leaving out a conditional conformance will cause end users to use their own, we can prohibit such use with a placeholder such as extension AnyCollection: Equatable where Element == Never.

First, make sure combining hash values isn't done with xor. Second, Optional is now conditionally Hashable, so let's use it!

xwu · 2018-02-11T21:11:46Z

...I have an alternative solution, based on that realization about non-operator methods not causing source breakage.

…uatable

xwu · 2018-02-11T21:43:50Z

@swift-ci Please smoke test

xwu · 2018-02-11T22:35:39Z

Please test with the following PR: apple/swift-package-manager#1495
@swift-ci Please smoke test Linux platform

xwu · 2018-02-12T00:20:50Z

Huzzah!

mortenbekditlevsen · 2018-02-17T20:36:53Z

May I ask: What is the next step for this PR?

airspeedswift · 2018-02-28T17:24:10Z

It was a little late in the 4.1 convergence cycle to get this in for that release, especially as it involves workarounds, adding an _-ed protocol beneath Equatable etc. We also need to discuss exactly which types should get this – I think the AnyCollection conformance to Equatable is debatable so needs to be discussed on evolution (though most of the other conformances are probably a simple matter that everyone will agree on).

mortenbekditlevsen · 2018-03-01T06:50:01Z

Super - thanks for the update! I'll move parts of the implementation to 'user space' until it lands in Swift 4.2 or Swift 5.0. :-)

xwu · 2018-03-01T06:53:12Z

@mortenbekditlevsen That's...not ideal if you can avoid it. Doing so guarantees that we'll be source-breaking when this does land.

mortenbekditlevsen · 2018-03-01T07:00:53Z

@xwu Thanks - it's only for some non-public libraries that we use internally at work, so we'll be fine with the source breaking update.
I'm just so eager to get rid of all our own (sub-par) Hashable conformance implementations. :-)

tkrajacic · 2018-03-16T18:00:36Z

Will this realistically be possible to land in Swift 4.2?

I have seen there are a few open PRs concerning hashing and quite the discussion about the Hashable API. Would it make sense to implement this using the existing hashing API or will this need to be postponed until that discussion is sorted out?

lorentey · 2018-03-20T14:14:45Z

This change is orthogonal to Hashable discussions, and it can be done without waiting for the outcome of those. SE-0143 does not explicitly mention Hashable conformances, but we consider these changes to be mostly covered by it; an amendment might be in order to spell this out explicitly.

To get things moving again, I'm going to rebase and update this PR to incorporate recent changes in hashing, which made hash compositions much easier to implement.

While the source compatibility issue is important, I believe adding the _Equatable protocol to work around @_implements limitations is not the right way to approach this problem. With apologies to @xwu, I'm going to revert that change, along with the DictionaryLiteral change that triggered the swiftpm issue. (We should work this through in a separate PR.)

AnyCollection's conditional conformances have proved controversial, so I'm going to remove those from this PR as well, pending further discussion on swift-evolution.

lorentey · 2018-03-20T21:38:46Z

I opened #15382 as the rebased & updated version of this PR, with a view towards landing it soon.

mortenbekditlevsen and others added 2 commits February 10, 2018 18:32

Silence warning and break to 80 characters

86ffc42

lorentey mentioned this pull request Feb 10, 2018

[WIP] Conditional conformance to Hashable for Optional, Dictionary and Array types. #14247

Merged

lorentey and others added 4 commits February 10, 2018 23:02

[Foundation] NSArray, NSDictionary: Add custom AnyHashable representa…

bd88afc

…tions Now that Array and Dictionary conform to Hashable, we need to make sure that their bridged counterparts provide the same hash values when converted to AnyHashable.

Merge pull request #14528 from xwu/conditional-hashable-xwu-patch

8d146e9

[conditional-hashable] Silence warning (unused binding) and break to 80 characters

apple deleted a comment from swift-ci Feb 11, 2018

[stdlib] Add availability annotation to other new conformances and me…

1f39896

…mbers

[stdlib] ClosedRange: Describe availability of Hashable conformance

0582667

apple deleted a comment from swift-ci Feb 11, 2018

xwu added 5 commits February 11, 2018 00:57

Revert "Merge pull request #14536 from xwu/conditional-hashable-at-im…

9b5c23c

…plements" This reverts commit b5a1d4b, reversing changes made to d03f0b3.

[stdlib] Add @_inlineable attribute with FIXME to hashValue implement…

6120d56

…ations

[stdlib] Remove unnecessary type annotations in hashValue

a9b1e9a

[stdlib] Mix lowerBound a little in Range.hashValue and ClosedRange.h…

0e0dbc0

…ashValue

xwu added 3 commits February 11, 2018 14:53

[stdlib] Remove unnecessary attributes and clarify comments

427d771

[stdlib] Restore an attribute for now

08302c6

[stdlib] Update FlattenCollection.Index hashValue

3d3ed0a

First, make sure combining hash values isn't done with xor. Second, Optional is now conditionally Hashable, so let's use it!

[stdlib] Attempt alternative design for conditional conformance to Eq…

7b3bc6c

…uatable

[ClangImporter] Break to 80 columns (NFC)

81d3aac

lorentey mentioned this pull request Mar 20, 2018

[SE-0143] Amend to explicitly allow conditional conformances to Hashable apple/swift-evolution#808

Merged

lorentey closed this Mar 20, 2018

lorentey mentioned this pull request Mar 20, 2018

[SE-0143][stdlib] Conditionally conform stdlib types to Hashable #15382

Merged

lorentey deleted the conditional-hashable branch April 28, 2018 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Conditionally conform stdlib types to Hashable #14527

[WIP] Conditionally conform stdlib types to Hashable #14527

lorentey commented Feb 10, 2018 •

edited by xwu

lorentey commented Feb 10, 2018

xwu commented Feb 10, 2018

lorentey commented Feb 10, 2018

xwu commented Feb 10, 2018

lorentey commented Feb 10, 2018

lorentey commented Feb 10, 2018

xwu commented Feb 10, 2018 •

edited

lorentey commented Feb 10, 2018

xwu commented Feb 10, 2018

lorentey commented Feb 10, 2018 •

edited

lorentey commented Feb 11, 2018

lorentey commented Feb 11, 2018

lorentey commented Feb 11, 2018

lorentey commented Feb 11, 2018

xwu commented Feb 11, 2018

swift-ci commented Feb 11, 2018

lorentey commented Feb 11, 2018

lorentey commented Feb 11, 2018

xwu commented Feb 11, 2018 •

edited

xwu commented Feb 11, 2018

xwu commented Feb 11, 2018 •

edited

xwu commented Feb 11, 2018 •

edited

xwu commented Feb 11, 2018 •

edited

xwu commented Feb 11, 2018

xwu commented Feb 11, 2018

xwu commented Feb 12, 2018

mortenbekditlevsen commented Feb 17, 2018

airspeedswift commented Feb 28, 2018

mortenbekditlevsen commented Mar 1, 2018

xwu commented Mar 1, 2018

mortenbekditlevsen commented Mar 1, 2018

tkrajacic commented Mar 16, 2018

lorentey commented Mar 20, 2018 •

edited

lorentey commented Mar 20, 2018

[WIP] Conditionally conform stdlib types to Hashable #14527

[WIP] Conditionally conform stdlib types to Hashable #14527

Conversation

lorentey commented Feb 10, 2018 • edited by xwu

lorentey commented Feb 10, 2018

xwu commented Feb 10, 2018

lorentey commented Feb 10, 2018

xwu commented Feb 10, 2018

lorentey commented Feb 10, 2018

lorentey commented Feb 10, 2018

xwu commented Feb 10, 2018 • edited

lorentey commented Feb 10, 2018

xwu commented Feb 10, 2018

lorentey commented Feb 10, 2018 • edited

lorentey commented Feb 11, 2018

lorentey commented Feb 11, 2018

lorentey commented Feb 11, 2018

lorentey commented Feb 11, 2018

xwu commented Feb 11, 2018

swift-ci commented Feb 11, 2018

Build comment file:

Compilation-performance test failed

lorentey commented Feb 11, 2018

lorentey commented Feb 11, 2018

xwu commented Feb 11, 2018 • edited

xwu commented Feb 11, 2018

xwu commented Feb 11, 2018 • edited

xwu commented Feb 11, 2018 • edited

xwu commented Feb 11, 2018 • edited

xwu commented Feb 11, 2018

xwu commented Feb 11, 2018

xwu commented Feb 12, 2018

mortenbekditlevsen commented Feb 17, 2018

airspeedswift commented Feb 28, 2018

mortenbekditlevsen commented Mar 1, 2018

xwu commented Mar 1, 2018

mortenbekditlevsen commented Mar 1, 2018

tkrajacic commented Mar 16, 2018

lorentey commented Mar 20, 2018 • edited

lorentey commented Mar 20, 2018

lorentey commented Feb 10, 2018 •

edited by xwu

xwu commented Feb 10, 2018 •

edited

lorentey commented Feb 10, 2018 •

edited

xwu commented Feb 11, 2018 •

edited

xwu commented Feb 11, 2018 •

edited

xwu commented Feb 11, 2018 •

edited

xwu commented Feb 11, 2018 •

edited

lorentey commented Mar 20, 2018 •

edited