Skip to content

Remove collection conformances to Equatable and Hashable #124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

timvermeulen
Copy link
Contributor

Implementing Equatable (and especially Hashable) correctly isn't straightforward for some of the non-trivial collection wrappers in this package, if we consider collections to be equal if they produce the same elements. Here are some examples of comparisons that should evaluate to true, despite the collections having unequal base collections:

[1, 3, 5].striding(by: 1)         == [1, 2, 3, 4, 5].striding(by: 2)
[1, 2].windows(ofCount: 10)       == [1, 2, 3].windows(ofCount: 100)
product([1, 2], "")               == product([1, 2, 3], "")
chain([1, 2], 3..<6)              == chain([1, 2, 3], 4..<6)
[1, 2].combinations(ofCount: 0)   == [1, 2, 3].combinations(ofCount: 0)
[1, 1, 1, 2].uniquePermutations() == [1, 2].uniquePermutations()
[1, 1, 1].cycled(times: 2)        == [1, 1].cycled(times: 3)

Implementing Hashable is often even harder because equal values should produce equal hash values, and therefore every collection needs to hash in the exact same way as all other collections equal to it.

For the moment we're removing all conformances of the sequence and collection types to Equatable and Hashable to avoid any ambiguity on this front, which is consistent with sequence and collection adaptors in the standard library. Most of the time s1.elementsEqual(s2) should be preferred anyway, because it lets you compare sequences that aren't instances of the same type.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

@timvermeulen timvermeulen added the source breaking This change affects existing source code label Apr 9, 2021
@timvermeulen
Copy link
Contributor Author

@swift-ci please test

@natecook1000
Copy link
Member

@swift-ci Please test

Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+2 −209

🎉

@natecook1000 natecook1000 merged commit abc5559 into apple:main Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
source breaking This change affects existing source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants