rust
rust copied to clipboard
rust-lang
Derive `Eq` and `Hash` for `ControlFlow`
There's really no reason for ControlFlow not to derive these traits. This is the part of #96416 that no one objected to, but that PR seems stale. The Eq derive was also requested by @lcnr on Zulip to allow for pattern matching.
This change requires an FCP because it's insta-stable.
Closes #96416.
Hey! It looks like you've submitted a new PR for the library teams!
If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.
Examples of T-libs-api changes:
- Stabilizing library features
- Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
- Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
- Changing public documentation in ways that create new stability guarantees
- Changing observable runtime behavior of library APIs
r? @thomcc
(rust-highfive has picked a reviewer for you, use r? to override)
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:
- [ ] @Amanieu
- [x] @BurntSushi
- [x] @dtolnay
- [x] @joshtriplett
- [x] @m-ou-se
- [ ] @yaahc
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
The
Eqderive was also requested by @lcnr on Zulip to allow for pattern matching.
I'll cross-link https://github.com/rust-lang/rust/pull/102697#issuecomment-1274404667 here, since libs-api seems skeptical of the CONTINUE constant, which would weaken the motivation slightly.
(I still think this PR is worth doing, though.)
Quick request: I want to clean up the comment a bit before this actually gets merged. I hope to get to that in the next day or two. The only way a merge could happen before then is if people were quick with approvals and someone decided it wasn't worth waiting the full ten days, but I thought I'd put in the request just in case. :)
So for me, I suspect these are useful and that we should have them.
But I am a tiny bit skeptical of their utility, and conceptually, I do somewhat wonder what it means to "compare" two different "control flow" values. But maybe I am being too conservative here. And in particular, I can't think of anything specifically bad about these impls, although I do question in what use cases they are useful. (Beyond the, e.g., CONTINUE constant, which I am more skeptical about for already stated reasons.)
There is a part of me that would like to hold off on stabilizing these impls until ControlFlow itself has been stable for a bit and we can get a better idea of how it's actually used. But I don't feel strong enough to register a concern if others are confident that this is totally fine.
until
ControlFlowitself has been stable for a bit
Note that ControlFlow the type has been stable for about a year now: https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1550-2021-09-09
(Some of its associated stuff is still unstable, though.)
But I am a tiny bit skeptical of their utility, and conceptually, I do somewhat wonder what it means to "compare" two different "control flow" values. But maybe I am being too conservative here. And in particular, I can't think of anything specifically bad about these impls, although I do question in what use cases they are useful. (Beyond the, e.g.,
CONTINUEconstant, which I am more skeptical about for already stated reasons.)
When you say "compare", that makes me think of PartialOrd and Ord, which aren't being proposed. Equality comparisons are already allowed, since we implement PartialEq. This really isn't allowing any new operations, in terms of the semantics of the type.
I think we really ought to add the Eq derivation, just on the grounds of being a good neighbor. It's a marker trait: since equality comparisons on this type are total equality comparisons, it makes sense to mark them as such. In addition, there's no reason to prevent users from creating their own constants and using them in matches, even if we end up removing CONTINUE.
I don't actually have a use case in mind for Hash, but I'm not sure we really need one. Just to give a general direction, a use for this might involve memoization or the like. Result already implements Hash, so this is does provide parity with that.
Unlike PartialOrd and Ord, these operations are consistent with the operations that are already meaningful for the type. They just make it more interoperable and pleasant to use. As a final point, the Rust API guidelines say that types should eagerly implement common traits. If these operations are meaningful, the question becomes why we wouldn't want to add them.
Sorry for going into "passionately list all the reasons why we should totally do this" mode. Bad habit of mine.
@scottmcm Oh! For some reason I lumped it in with Try being unstable.
I'd say my concern is very weak then. Weak enough to tick my box.
Equality comparisons are already allowed, since we implement
PartialEq.
Ah okay, I didn't realize this either. I buy it.
Quick request: I want to clean up the comment a bit before this actually gets merged. I hope to get to that in the next day or two.
Done!
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
This will be merged soon.
