[Review] SE-188 Make stdlib index types Hashable

Hello, Swift community!

The review of “SE-0188: Make stdlib index types Hashable” begins now and runs through November 14th, 2017. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0188-stdlib-index-types-hashable.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to me as the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

  https://github.com/apple/swift-evolution/blob/master/proposals/0188-stdlib-index-types-hashable.md

Reply text
Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.

When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

As always, thank you for contributing to the evolution of Swift.

Ben Cohen
Review Manager

  https://github.com/apple/swift-evolution/blob/master/proposals/0188-stdlib-index-types-hashable.md

• What is your evaluation of the proposal?

This all seems very sensible, but here's my big question:

  AnyIndex, which type erases any index type at run-time, would not be hashable since it might wrap a non-hashable type.

Why not? Specifically, why shouldn't we require `Hashable` conformance on indices? Adding it would require changes to conforming types, sure, but indices are already required to be `Equatable`, and `Hashable` conformance just got really easy to add, and custom `Collection`s are a relatively rare and advanced feature.

Is it worth a source break to add it? Personally, I think so—but even if you disagree, I think we should document why we decided not to add it.

• Is the problem being addressed significant enough to warrant a change to Swift?

Yes.

• Does this proposal fit well with the feel and direction of Swift?

Yes.

• If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A.

• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading, nothing more.

···

--
Brent Royal-Gordon
Architechies

  • What is your evaluation of the proposal?

„That’s not possible yet?“ :wink:

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes.
Conformance to Hashable might be useful in other areas besides key-paths, and even if not: The change shouldn’t do any harm

  • Does this proposal fit well with the feel and direction of Swift?

I hope… it’s something that doesn’t increase language complexity, but rather simplifies by filling a gap.

Proposal link:
https://github.com/apple/swift-evolution/blob/master/
proposals/0188-stdlib-index-types-hashable.md

• What is your evaluation of the proposal?

+1 but would like it to further and require all index types to be Hashable.

• Is the problem being addressed significant enough to warrant a change to
Swift?

It has been a problem for me in the past, however just requiring the
standard library ones to be hashable would solve the problem of third party
libraries, therefore suggest all hashable.

• Does this proposal fit well with the feel and direction of Swift?

Yes, this is the sort of thing that needs finalising before ABI stability
else we will be stuck with a problem.

• If you have used other languages or libraries with a similar feature, how
do you feel that this proposal compares to those?

Yes, Java, Scala, etc. Most languages have a hashable index so you can have
a set on indices etc.

• How much effort did you put into your review? A glance, a quick reading,
or an in-depth study?

Have read the review and have also implemented my own collections, which
used hashable indices.

  -- Howard.

···

On 9 November 2017 at 12:55, Ben Cohen via swift-evolution < swift-evolution@swift.org> wrote:

Hello, Swift community!

The review of “SE-0188: Make stdlib index types Hashable” begins now and
runs through November 14th, 2017. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/
proposals/0188-stdlib-index-types-hashable.md

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at

https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to me as the
review manager. When replying, please try to keep the proposal link at the
top of the message:

Proposal link:
https://github.com/apple/swift-evolution/blob/master/
proposals/0188-stdlib-index-types-hashable.md

Reply text

Other replies

*What goes into a review?*

The goal of the review process is to improve the proposal under review
through constructive criticism and, eventually, determine the direction of
Swift.

When writing your review, here are some questions you might want to answer
in your review:

• What is your evaluation of the proposal?
• Is the problem being addressed significant enough to warrant a change to
Swift?
• Does this proposal fit well with the feel and direction of Swift?
• If you have used other languages or libraries with a similar feature,
how do you feel that this proposal compares to those?
• How much effort did you put into your review? A glance, a quick reading,
or an in-depth study?

More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

As always, thank you for contributing to the evolution of Swift.

Ben Cohen
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

>
https://github.com/apple/swift-evolution/blob/master/proposals/0188-stdlib-index-types-hashable.md
>
> • What is your evaluation of the proposal?

This all seems very sensible, but here's my big question:

        AnyIndex, which type erases any index type at run-time, would not
be hashable since it might wrap a non-hashable type.

Why not? Specifically, why shouldn't we require `Hashable` conformance on
indices? Adding it would require changes to conforming types, sure, but
indices are already required to be `Equatable`, and `Hashable` conformance
just got really easy to add, and custom `Collection`s are a relatively rare
and advanced feature.

For a source-breaking change, that’s the wrong question to ask. It’s not
“why not,” but “why so”? It’s so easy to add the conformance, and any type
can opt into it so easily, what is the gain by forcing it and can it be
justified as a source-breaking change?

···

On Thu, Nov 9, 2017 at 05:28 Brent Royal-Gordon via swift-evolution < swift-evolution@swift.org> wrote:

Is it worth a source break to add it? Personally, I think so—but even if
you disagree, I think we should document why we decided not to add it.

> • Is the problem being addressed significant enough to warrant a change
to Swift?

Yes.

> • Does this proposal fit well with the feel and direction of Swift?

Yes.

> • If you have used other languages or libraries with a similar feature,
how do you feel that this proposal compares to those?

N/A.

> • How much effort did you put into your review? A glance, a quick
reading, or an in-depth study?

Quick reading, nothing more.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

> https://github.com/apple/swift-evolution/blob/master/proposals/0188-stdlib-index-types-hashable.md
>
> • What is your evaluation of the proposal?

This all seems very sensible, but here's my big question:

        AnyIndex, which type erases any index type at run-time, would not be hashable since it might wrap a non-hashable type.

Why not? Specifically, why shouldn't we require `Hashable` conformance on indices? Adding it would require changes to conforming types, sure, but indices are already required to be `Equatable`, and `Hashable` conformance just got really easy to add, and custom `Collection`s are a relatively rare and advanced feature.

For a source-breaking change, that’s the wrong question to ask. It’s not “why not,” but “why so”? It’s so easy to add the conformance, and any type can opt into it so easily, what is the gain by forcing it and can it be justified as a source-breaking change?

In this case we’re talking about the requirements on indices when ABI stability happens. That’s a pretty important point of design to get right. I think Brent *is* asking the right question in this case. I haven’t given it enough thought to form a clear opinion as to the answer yet.

···

Sent from my iPad

On Nov 9, 2017, at 6:29 AM, Xiaodi Wu via swift-evolution <swift-evolution@swift.org> wrote:

On Thu, Nov 9, 2017 at 05:28 Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

Is it worth a source break to add it? Personally, I think so—but even if you disagree, I think we should document why we decided not to add it.

> • Is the problem being addressed significant enough to warrant a change to Swift?

Yes.

> • Does this proposal fit well with the feel and direction of Swift?

Yes.

> • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A.

> • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading, nothing more.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

You're right that source-breaking changes need to be justified. What I would say is:

  1. This proposal as written will require many conditional conformances; it would be better (and would allow us to fully implement it sooner) if they were unconditional.

  2. Relying on `where` clauses does not help you in type-erased code. `AnyIndex` cannot conditionally conform to `Hashable` because it erases types, and it's not clear to me whether `AnyIndex` can be cleanly converted to `AnyHashable` or vice versa. This means code using type erasure may be awkward to write if it needs `Hashable` indices.

  3. The `KeyPath` case discussed in this proposal is another example where ad-hoc `Hashable` indices cause trouble for type erasure (in that `KeyPath`s erase the types of the indices inside them). It would be very weird for `KeyPath`s to only support indices if they happen to also be `Hashable`.

  4. `Hashable` conformance is extremely useful in general. It allows, among other features, use as a `Set` or `Dictionary` key, and apparently now use in `KeyPath` as well. Replacing an `Array` of indices with a `Set`, or switching from some other representation to a `KeyPath`, is the sort of change we would like generic algorithms to be able to make without breaking binary compatibility with their callers.

  5. `Hashable` occupies a special place of importance among our standard protocols. In the past, we've occasionally discussed making it mandatory and impossible to opt out of; many languages actually do that. Even though we haven't, I've seen experts make flat statements along the lines of "All value types should conform to `Hashable`". This is not like requiring an index to be, say, `Strideable` or a `SignedInteger`.

  6. The compiler can now automatically synthesize `Hashable` conformance for many simple types; this makes the burden of requiring a conformance unusually low.

  7. I have written several custom collections, and it has never occurred to me to conform the index to `Hashable`, but now that it's been mentioned I realize that it would have been both easy and extremely useful to do so. My index types would already have been `Hashable` if the standard library had forced me to do so.

Basically, what it comes down to is, `Hashable` is very useful and there are not many plausible index designs which would have trouble supporting it. It is not absolutely necessary to make this change, but I think it would lead to better, more useful types with a fairly low burden.

···

On Nov 9, 2017, at 4:29 AM, Xiaodi Wu <xiaodi.wu@gmail.com> wrote:

Why not? Specifically, why shouldn't we require `Hashable` conformance on indices? Adding it would require changes to conforming types, sure, but indices are already required to be `Equatable`, and `Hashable` conformance just got really easy to add, and custom `Collection`s are a relatively rare and advanced feature.

For a source-breaking change, that’s the wrong question to ask. It’s not “why not,” but “why so”? It’s so easy to add the conformance, and any type can opt into it so easily, what is the gain by forcing it and can it be justified as a source-breaking change?

--
Brent Royal-Gordon
Architechies

Thanks for this, Brent, you make a strong case! I didn't include a change to Collection.Index simply because it doesn't look like missing the constraint is really causing harm, just inconvenience. That said, you’re right that making something Hashable is easier than ever, and if we’re going to make this change, the time is now.

I tried adding the constraint in a branch, and it isn't quite as simple as I had expected. For one thing, CountableRange and CountableClosedRange need the Bound type to be Hashable, not just Strideable, which increases the number of places that could be source breaking. You can see the full diff here:
  https://github.com/apple/swift/pull/12944

Also complicating things is that I don’t know what the migration path would look like—we can’t use availability attributes on associated type constraints, so it could be very difficult to support compiling both existing Swift 4 code and code that works on a compiler that requires the Hashable constraint.

Nate

···

On Nov 9, 2017, at 7:12 PM, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

On Nov 9, 2017, at 4:29 AM, Xiaodi Wu <xiaodi.wu@gmail.com <mailto:xiaodi.wu@gmail.com>> wrote:

Why not? Specifically, why shouldn't we require `Hashable` conformance on indices? Adding it would require changes to conforming types, sure, but indices are already required to be `Equatable`, and `Hashable` conformance just got really easy to add, and custom `Collection`s are a relatively rare and advanced feature.

For a source-breaking change, that’s the wrong question to ask. It’s not “why not,” but “why so”? It’s so easy to add the conformance, and any type can opt into it so easily, what is the gain by forcing it and can it be justified as a source-breaking change?

You're right that source-breaking changes need to be justified. What I would say is:

  1. This proposal as written will require many conditional conformances; it would be better (and would allow us to fully implement it sooner) if they were unconditional.

  2. Relying on `where` clauses does not help you in type-erased code. `AnyIndex` cannot conditionally conform to `Hashable` because it erases types, and it's not clear to me whether `AnyIndex` can be cleanly converted to `AnyHashable` or vice versa. This means code using type erasure may be awkward to write if it needs `Hashable` indices.

  3. The `KeyPath` case discussed in this proposal is another example where ad-hoc `Hashable` indices cause trouble for type erasure (in that `KeyPath`s erase the types of the indices inside them). It would be very weird for `KeyPath`s to only support indices if they happen to also be `Hashable`.

  4. `Hashable` conformance is extremely useful in general. It allows, among other features, use as a `Set` or `Dictionary` key, and apparently now use in `KeyPath` as well. Replacing an `Array` of indices with a `Set`, or switching from some other representation to a `KeyPath`, is the sort of change we would like generic algorithms to be able to make without breaking binary compatibility with their callers.

  5. `Hashable` occupies a special place of importance among our standard protocols. In the past, we've occasionally discussed making it mandatory and impossible to opt out of; many languages actually do that. Even though we haven't, I've seen experts make flat statements along the lines of "All value types should conform to `Hashable`". This is not like requiring an index to be, say, `Strideable` or a `SignedInteger`.

  6. The compiler can now automatically synthesize `Hashable` conformance for many simple types; this makes the burden of requiring a conformance unusually low.

  7. I have written several custom collections, and it has never occurred to me to conform the index to `Hashable`, but now that it's been mentioned I realize that it would have been both easy and extremely useful to do so. My index types would already have been `Hashable` if the standard library had forced me to do so.

Basically, what it comes down to is, `Hashable` is very useful and there are not many plausible index designs which would have trouble supporting it. It is not absolutely necessary to make this change, but I think it would lead to better, more useful types with a fairly low burden.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution