Uh oh!
There was an error while loading. Please reload this page.
Remove problematic specialization from RangeInclusive - #68835
Merged
Conversation
rust-highfive
commented
Feb 4, 2020
Contributor
(rust_highfive has picked a reviewer for you, use r? to override) |
Mark-Simulacrum
commented
Feb 4, 2020
Member
Can you run benchmarks locally as well? We should try to make sure this is not regressing performance (or at least be aware if it is). We could also verify that the codegen is nice (particularly that it still unrolls nicely). I'm not running compiler perf tests here since that seems super unlikely to actually be influenced by this. cc @kennytm |
rust-highfive
commented
Feb 5, 2020
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
CAD97
commented
Feb 5, 2020
ContributorAuthor
Speaking of codegen issues... I'll play with this locally now that I know what test to run. I ran libcore tests locally but apparently not the other relevant ones. |
CAD97
commented
Feb 5, 2020
ContributorAuthor
I was unable to tweak the implementation to allow that codegen pass to pass. The other, smaller change approach I suggested trying leads to an ICE, so I can't test if it breaks the codegen test. |
alexcrichton
commented
Feb 5, 2020
Member
Mark-Simulacrum
commented
Feb 5, 2020
Member
Can you update the codegen tests (possibly just commenting out the broken tests for now)? I personally think we should land the soundness fix and then take another look at fixing the codegen. These codegen tests were always pretty brittle I think anyway. |
ContributorAuthor
I disabled the failing codegen test. Interestingly, of the three checks in the file, only one actually started failing. The internal iteration version obviously still works, as that was adjusted in #48012 to be more optimizable. The interesting part is that the original example from #45222 continues to be optimized down to a Details// verify that LLVM recognizes a loop involving 0..=n and will const-fold it.// Example from original issue #45222fnfoo2(n:u64) -> u64{letmut count = 0;for _ in0..n {for j in(0..=n).rev(){
count += j;}}
count
}// CHECK-LABEL: @check_foo2#[no_mangle]pubfncheck_foo2() -> u64{// CHECK: ret i64 500005000000000foo2(100000)}// Simplified example of #45222fntriangle_inc(n:u64) -> u64{letmut count = 0;for j in0 ..= n {
count += j;}
count
}// CHECK-LABEL: @check_triangle_inc#[no_mangle]pubfncheck_triangle_inc() -> u64{// NO-CHECK: ret i64 5000050000triangle_inc(100000)}// Demo in #48012fnfoo3r(n:u64) -> u64{letmut count = 0;(0..n).for_each(|_| {(0 ..= n).rev().for_each(|j| {
count += j;})});
count
}// CHECK-LABEL: @check_foo3r#[no_mangle]pubfncheck_foo3r() -> u64{// CHECK: ret i64 500050000000foo3r(10000)}So... #[inline]fnnext(&mutself) -> Option<A>{ifself.is_empty(){returnNone;}let is_iterating = self.start < self.end;Some(if is_iterating {let n = self.start.add_one();
mem::replace(&mutself.start, n)}else{self.exhausted = true;self.start.clone()})}#[inline]fnnext_back(&mutself) -> Option<A>{ifself.is_empty(){returnNone;}let is_iterating = self.start < self.end;Some(if is_iterating {let n = self.end.sub_one();
mem::replace(&mutself.end, n)}else{self.exhausted = true;self.end.clone()})}Here's a link to the playground and to the compiler explorer that shows this mismatch with minimal code. |
CAD97
commented
Feb 5, 2020
ContributorAuthor
So as it turns out, codegen is bizarre, and flipping the check in |
Mark-Simulacrum
commented
Feb 5, 2020
Member
Those are also different conditions; I think we don't care in practice because is_empty would've already returned false if we were done, though, right? |
CAD97
commented
Feb 5, 2020
ContributorAuthor
Whoops, that messes up a test r = RangeInclusive::new(127i8,127);assert_eq!(r.next(),Some(127));assert_eq!(r.next(),None);Reverting for now, whoops |
CAD97force-pushed
the
sound-range-inclusive
branch
from
February 5, 2020 21:31
2a009de to
90645eaCompareCAD97
commented
Feb 6, 2020
ContributorAuthor
I realized a potential reason why LLVM would do better optimizing the downward direction. And the reason is that downward-inclusive iteration is a lot more common (in C/C++ code) than upward inclusive. Even a top-half-open range backwards is end-inclusive. And while iterate up will typically be written in exclusive style ( Because the reverse iterator is optimized here, I suspect LLVM can be taught to recognize the positive direction as well, though doing so might not be trivial nor reach us quickly. |
Mark-Simulacrumforce-pushed
the
sound-range-inclusive
branch
from
February 6, 2020 21:15
90645ea to
5f0d830CompareMark-Simulacrum
commented
Feb 6, 2020
Member
Okay, pushed up an update (just some changes to the comments) since I want to get this landed quickly. @bors r+ rollup=never Marking as non-rollup to make it easy to bisect regressions to this. |
bors
commented
Feb 6, 2020
Collaborator
📌 Commit 5f0d830a46ebb217aab8dc9e118788b0fc91f712 has been approved by |
Mark-Simulacrum
commented
Feb 6, 2020
Member
Oh, also, I am not filing a new issue for the codegen regression as #45222 is still open anyway... |
rust-highfive
commented
Feb 6, 2020
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
CAD97
commented
Feb 7, 2020
ContributorAuthor
Just FYI I've currently got a tree in my roof and no power, so I won't be able to touch this for a bit, at least until the power comes back on. The comment annotations need to be hidden from the codegen test harness by removing them or adding characters, just commenting out the area isn't enough. That's why I used |
Mark-Simulacrum
commented
Feb 7, 2020
Member
❤️ No worries! I can definitely patch this up. As to the NO-CHECK, makes sense. I guess we can't check if it's in a comment because it's always in a comment... Will fix that shortly. |
Mark-Simulacrumforce-pushed
the
sound-range-inclusive
branch
from
February 7, 2020 01:38
5f0d830 to
cfb5d8dCompareMark-Simulacrum
commented
Feb 7, 2020
Member
@bors r+ rollup=never |
bors
commented
Feb 7, 2020
Collaborator
📌 Commit cfb5d8dce9769a66d3849adaa17a36a6f125af68 has been approved by |
bors
commented
Feb 8, 2020
Collaborator
☔ The latest upstream changes (presumably #68358) made this pull request unmergeable. Please resolve the merge conflicts. |
rust-highfive
commented
Feb 8, 2020
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Mark-Simulacrum
commented
Feb 8, 2020
Member
@bors r- |
CAD97
commented
Feb 8, 2020
ContributorAuthor
The failure previously was a rebase error. Wait, I thought I tested this before the previous message... I re-implemented with this version of |
Mark-Simulacrumforce-pushed
the
sound-range-inclusive
branch
from
February 8, 2020 23:49
989f7cf to
136008cCompareMark-Simulacrum
commented
Feb 8, 2020
Member
Let's leave restoring the performance/codegen test to a future PR; I don't want to get bogged down in figuring it out exactly here, we should fix the Hash/Eq bug at least (which this is doing). I'll wait till PR CI is passing before re-approving. |
Mark-Simulacrum
commented
Feb 9, 2020
Member
@bors r+ |
bors
commented
Feb 9, 2020
Collaborator
📌 Commit 136008c has been approved by |
bors
commented
Feb 10, 2020
Collaborator
bors added a commit
that referenced
this pull request
Feb 10, 2020
Remove problematic specialization from RangeInclusive Fixes#67194 using the approach [outlined by Mark-Simulacrum](#67194 (comment)). > I believe the property we want is that if `PartialEq(&self, &other) == true`, then `self.next() == other.next()`. It is true that this is satisfied by removing the specialization and always doing `is_empty.unwrap_or_default()`; the "wrong" behavior there arises from calling `next()` having an effect on initially empty ranges, as we should be in `is_empty = true` but are not (yet) there. It might be possible to detect that the current state is always empty (i.e., `start > end`) and then not fill in the empty slot. I think this might solve the problem without regressing tests; however, this could have performance implications. > That approach essentially states that we only use the `is_empty` slot for cases where `start <= end`. That means that `Idx: !Step` and `start > end` would both behave the same, and correctly -- we do not need the boolean if we're not ever going to emit any values from the iterator. This is implemented here by replacing the `is_empty: Option<bool>` slot with an `exhausted: bool` slot. This flag is - `false` upon construction, - `false` when iteration has not yielded an element -- importantly, this means it is always `false` for an iterator empty by construction, - `false` when iteration has yielded an element and the iterator is not exhausted, and - only `true` when iteration has been used to exhaust the iterator. For completeness, this also adds a note to the `Debug` representation to note when the range is exhausted.
bors
commented
Feb 10, 2020
Collaborator
☀️ Test successful - checks-azure |
This was referenced Mar 19, 2020
Dylan-DPC-zz pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Mar 20, 2020
…ramertj Derive PartialEq, Eq and Hash for RangeInclusive The manual implementation of `PartialEq`, `Eq` and `Hash` for `RangeInclusive` was functionally equivalent to a derived implementation. This change removes the manual implementation and adds the respective derives. A side effect of this change is that the derives also add implementations for `StructuralPartialEq` and `StructuralEq`, which enables `RangeInclusive` to be used in const generics, closing rust-lang#70155. This change is enabled by rust-lang#68835, which changed the field `is_empty: Option<bool>` to `exhausted: bool` removing the need for *semantic* equality instead of *structural* equality. ## PartialEq original [`PartialEq`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L353-L359) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: PartialEq> PartialEq for RangeInclusive<Idx> { #[inline] fn eq(&self, other: &Self) -> bool { self.start == other.start && self.end == other.end && self.exhausted == other.exhausted } } ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx> crate::marker::StructuralPartialEq for RangeInclusive<Idx> {} #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::cmp::PartialEq> crate::cmp::PartialEq for RangeInclusive<Idx> { #[inline] fn eq(&self, other: &RangeInclusive<Idx>) -> bool { match *other { RangeInclusive { start: ref __self_1_0,end: ref __self_1_1, exhausted: ref __self_1_2 } => match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1, exhausted: ref __self_0_2 } => { (*__self_0_0) == (*__self_1_0) && (*__self_0_1) == (*__self_1_1) && (*__self_0_2) == (*__self_1_2) } }, } } #[inline] fn ne(&self, other: &RangeInclusive<Idx>) -> bool { match *other { RangeInclusive { start: ref __self_1_0, end: ref __self_1_1, exhausted: ref __self_1_2 } => match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1exhausted: ref __self_0_2 } => { (*__self_0_0) != (*__self_1_0) || (*__self_0_1) != (*__self_1_1) || (*__self_0_2) != (*__self_1_2) } }, } } } ``` These implementations both test for *structural* equality, with the same order of field comparisons, and the bound `Idx: PartialEq` is the same. ## Eq original [`Eq`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L361-L362) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: Eq> Eq for RangeInclusive<Idx> {} ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx> crate::marker::StructuralEq for RangeInclusive<Idx> {} #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::cmp::Eq> crate::cmp::Eq for RangeInclusive<Idx> { #[inline] #[doc(hidden)] fn assert_receiver_is_total_eq(&self) -> () { { let _: crate::cmp::AssertParamIsEq<Idx>; let _: crate::cmp::AssertParamIsEq<Idx>; let _: crate::cmp::AssertParamIsEq<bool>; } } } ``` These implementations are equivalent since `Eq` is just a marker trait and the bound `Idx: Eq` is the same. ## Hash original [`Hash`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L364-L371) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: Hash> Hash for RangeInclusive<Idx> { fn hash<H: Hasher>(&self, state: &mut H) { self.start.hash(state); self.end.hash(state); self.exhausted.hash(state); } } ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::hash::Hash> crate::hash::Hash for RangeInclusive<Idx> { fn hash<__H: crate::hash::Hasher>(&self, state: &mut __H) -> () { match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1, exhausted: ref __self_0_2 } => { crate::hash::Hash::hash(&(*__self_0_0), state); crate::hash::Hash::hash(&(*__self_0_1), state); crate::hash::Hash::hash(&(*__self_0_2), state) } } } } ``` These implementations are functionally equivalent, with the same order of field hashing, and the bound `Idx: Hash` is the same.
Dylan-DPC-zz pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Mar 21, 2020
…ramertj Derive PartialEq, Eq and Hash for RangeInclusive The manual implementation of `PartialEq`, `Eq` and `Hash` for `RangeInclusive` was functionally equivalent to a derived implementation. This change removes the manual implementation and adds the respective derives. A side effect of this change is that the derives also add implementations for `StructuralPartialEq` and `StructuralEq`, which enables `RangeInclusive` to be used in const generics, closing rust-lang#70155. This change is enabled by rust-lang#68835, which changed the field `is_empty: Option<bool>` to `exhausted: bool` removing the need for *semantic* equality instead of *structural* equality. ## PartialEq original [`PartialEq`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L353-L359) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: PartialEq> PartialEq for RangeInclusive<Idx> { #[inline] fn eq(&self, other: &Self) -> bool { self.start == other.start && self.end == other.end && self.exhausted == other.exhausted } } ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx> crate::marker::StructuralPartialEq for RangeInclusive<Idx> {} #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::cmp::PartialEq> crate::cmp::PartialEq for RangeInclusive<Idx> { #[inline] fn eq(&self, other: &RangeInclusive<Idx>) -> bool { match *other { RangeInclusive { start: ref __self_1_0,end: ref __self_1_1, exhausted: ref __self_1_2 } => match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1, exhausted: ref __self_0_2 } => { (*__self_0_0) == (*__self_1_0) && (*__self_0_1) == (*__self_1_1) && (*__self_0_2) == (*__self_1_2) } }, } } #[inline] fn ne(&self, other: &RangeInclusive<Idx>) -> bool { match *other { RangeInclusive { start: ref __self_1_0, end: ref __self_1_1, exhausted: ref __self_1_2 } => match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1exhausted: ref __self_0_2 } => { (*__self_0_0) != (*__self_1_0) || (*__self_0_1) != (*__self_1_1) || (*__self_0_2) != (*__self_1_2) } }, } } } ``` These implementations both test for *structural* equality, with the same order of field comparisons, and the bound `Idx: PartialEq` is the same. ## Eq original [`Eq`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L361-L362) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: Eq> Eq for RangeInclusive<Idx> {} ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx> crate::marker::StructuralEq for RangeInclusive<Idx> {} #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::cmp::Eq> crate::cmp::Eq for RangeInclusive<Idx> { #[inline] #[doc(hidden)] fn assert_receiver_is_total_eq(&self) -> () { { let _: crate::cmp::AssertParamIsEq<Idx>; let _: crate::cmp::AssertParamIsEq<Idx>; let _: crate::cmp::AssertParamIsEq<bool>; } } } ``` These implementations are equivalent since `Eq` is just a marker trait and the bound `Idx: Eq` is the same. ## Hash original [`Hash`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L364-L371) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: Hash> Hash for RangeInclusive<Idx> { fn hash<H: Hasher>(&self, state: &mut H) { self.start.hash(state); self.end.hash(state); self.exhausted.hash(state); } } ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::hash::Hash> crate::hash::Hash for RangeInclusive<Idx> { fn hash<__H: crate::hash::Hasher>(&self, state: &mut __H) -> () { match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1, exhausted: ref __self_0_2 } => { crate::hash::Hash::hash(&(*__self_0_0), state); crate::hash::Hash::hash(&(*__self_0_1), state); crate::hash::Hash::hash(&(*__self_0_2), state) } } } } ``` These implementations are functionally equivalent, with the same order of field hashing, and the bound `Idx: Hash` is the same.
Centril added a commit
to Centril/rust
that referenced
this pull request
Mar 21, 2020
…ramertj Derive PartialEq, Eq and Hash for RangeInclusive The manual implementation of `PartialEq`, `Eq` and `Hash` for `RangeInclusive` was functionally equivalent to a derived implementation. This change removes the manual implementation and adds the respective derives. A side effect of this change is that the derives also add implementations for `StructuralPartialEq` and `StructuralEq`, which enables `RangeInclusive` to be used in const generics, closing rust-lang#70155. This change is enabled by rust-lang#68835, which changed the field `is_empty: Option<bool>` to `exhausted: bool` removing the need for *semantic* equality instead of *structural* equality. ## PartialEq original [`PartialEq`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L353-L359) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: PartialEq> PartialEq for RangeInclusive<Idx> { #[inline] fn eq(&self, other: &Self) -> bool { self.start == other.start && self.end == other.end && self.exhausted == other.exhausted } } ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx> crate::marker::StructuralPartialEq for RangeInclusive<Idx> {} #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::cmp::PartialEq> crate::cmp::PartialEq for RangeInclusive<Idx> { #[inline] fn eq(&self, other: &RangeInclusive<Idx>) -> bool { match *other { RangeInclusive { start: ref __self_1_0,end: ref __self_1_1, exhausted: ref __self_1_2 } => match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1, exhausted: ref __self_0_2 } => { (*__self_0_0) == (*__self_1_0) && (*__self_0_1) == (*__self_1_1) && (*__self_0_2) == (*__self_1_2) } }, } } #[inline] fn ne(&self, other: &RangeInclusive<Idx>) -> bool { match *other { RangeInclusive { start: ref __self_1_0, end: ref __self_1_1, exhausted: ref __self_1_2 } => match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1exhausted: ref __self_0_2 } => { (*__self_0_0) != (*__self_1_0) || (*__self_0_1) != (*__self_1_1) || (*__self_0_2) != (*__self_1_2) } }, } } } ``` These implementations both test for *structural* equality, with the same order of field comparisons, and the bound `Idx: PartialEq` is the same. ## Eq original [`Eq`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L361-L362) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: Eq> Eq for RangeInclusive<Idx> {} ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx> crate::marker::StructuralEq for RangeInclusive<Idx> {} #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::cmp::Eq> crate::cmp::Eq for RangeInclusive<Idx> { #[inline] #[doc(hidden)] fn assert_receiver_is_total_eq(&self) -> () { { let _: crate::cmp::AssertParamIsEq<Idx>; let _: crate::cmp::AssertParamIsEq<Idx>; let _: crate::cmp::AssertParamIsEq<bool>; } } } ``` These implementations are equivalent since `Eq` is just a marker trait and the bound `Idx: Eq` is the same. ## Hash original [`Hash`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L364-L371) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: Hash> Hash for RangeInclusive<Idx> { fn hash<H: Hasher>(&self, state: &mut H) { self.start.hash(state); self.end.hash(state); self.exhausted.hash(state); } } ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::hash::Hash> crate::hash::Hash for RangeInclusive<Idx> { fn hash<__H: crate::hash::Hasher>(&self, state: &mut __H) -> () { match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1, exhausted: ref __self_0_2 } => { crate::hash::Hash::hash(&(*__self_0_0), state); crate::hash::Hash::hash(&(*__self_0_1), state); crate::hash::Hash::hash(&(*__self_0_2), state) } } } } ``` These implementations are functionally equivalent, with the same order of field hashing, and the bound `Idx: Hash` is the same.
Centril added a commit
to Centril/rust
that referenced
this pull request
Mar 21, 2020
…ramertj Derive PartialEq, Eq and Hash for RangeInclusive The manual implementation of `PartialEq`, `Eq` and `Hash` for `RangeInclusive` was functionally equivalent to a derived implementation. This change removes the manual implementation and adds the respective derives. A side effect of this change is that the derives also add implementations for `StructuralPartialEq` and `StructuralEq`, which enables `RangeInclusive` to be used in const generics, closing rust-lang#70155. This change is enabled by rust-lang#68835, which changed the field `is_empty: Option<bool>` to `exhausted: bool` removing the need for *semantic* equality instead of *structural* equality. ## PartialEq original [`PartialEq`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L353-L359) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: PartialEq> PartialEq for RangeInclusive<Idx> { #[inline] fn eq(&self, other: &Self) -> bool { self.start == other.start && self.end == other.end && self.exhausted == other.exhausted } } ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx> crate::marker::StructuralPartialEq for RangeInclusive<Idx> {} #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::cmp::PartialEq> crate::cmp::PartialEq for RangeInclusive<Idx> { #[inline] fn eq(&self, other: &RangeInclusive<Idx>) -> bool { match *other { RangeInclusive { start: ref __self_1_0,end: ref __self_1_1, exhausted: ref __self_1_2 } => match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1, exhausted: ref __self_0_2 } => { (*__self_0_0) == (*__self_1_0) && (*__self_0_1) == (*__self_1_1) && (*__self_0_2) == (*__self_1_2) } }, } } #[inline] fn ne(&self, other: &RangeInclusive<Idx>) -> bool { match *other { RangeInclusive { start: ref __self_1_0, end: ref __self_1_1, exhausted: ref __self_1_2 } => match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1exhausted: ref __self_0_2 } => { (*__self_0_0) != (*__self_1_0) || (*__self_0_1) != (*__self_1_1) || (*__self_0_2) != (*__self_1_2) } }, } } } ``` These implementations both test for *structural* equality, with the same order of field comparisons, and the bound `Idx: PartialEq` is the same. ## Eq original [`Eq`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L361-L362) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: Eq> Eq for RangeInclusive<Idx> {} ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx> crate::marker::StructuralEq for RangeInclusive<Idx> {} #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::cmp::Eq> crate::cmp::Eq for RangeInclusive<Idx> { #[inline] #[doc(hidden)] fn assert_receiver_is_total_eq(&self) -> () { { let _: crate::cmp::AssertParamIsEq<Idx>; let _: crate::cmp::AssertParamIsEq<Idx>; let _: crate::cmp::AssertParamIsEq<bool>; } } } ``` These implementations are equivalent since `Eq` is just a marker trait and the bound `Idx: Eq` is the same. ## Hash original [`Hash`](https://github.com/rust-lang/rust/blob/f4c675c476c18b1a11041193f2f59d695b126bc8/src/libcore/ops/range.rs#L364-L371) implementation: ```rust #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: Hash> Hash for RangeInclusive<Idx> { fn hash<H: Hasher>(&self, state: &mut H) { self.start.hash(state); self.end.hash(state); self.exhausted.hash(state); } } ``` expanded derive implementation (using `cargo expand ops::range`): ```rust #[automatically_derived] #[allow(unused_qualifications)] #[stable(feature = "inclusive_range", since = "1.26.0")] impl<Idx: crate::hash::Hash> crate::hash::Hash for RangeInclusive<Idx> { fn hash<__H: crate::hash::Hasher>(&self, state: &mut __H) -> () { match *self { RangeInclusive { start: ref __self_0_0, end: ref __self_0_1, exhausted: ref __self_0_2 } => { crate::hash::Hash::hash(&(*__self_0_0), state); crate::hash::Hash::hash(&(*__self_0_1), state); crate::hash::Hash::hash(&(*__self_0_2), state) } } } } ``` These implementations are functionally equivalent, with the same order of field hashing, and the bound `Idx: Hash` is the same.
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Fixes#67194 using the approach outlined by Mark-Simulacrum.
This is implemented here by replacing the
is_empty: Option<bool>slot with anexhausted: boolslot. This flag isfalseupon construction,falsewhen iteration has not yielded an element -- importantly, this means it is alwaysfalsefor an iterator empty by construction,falsewhen iteration has yielded an element and the iterator is not exhausted, andtruewhen iteration has been used to exhaust the iterator.For completeness, this also adds a note to the
Debugrepresentation to note when the range is exhausted.