From d1035223a903674dab3e5e5b170c5d2929c00147 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Thu, 6 Jul 2017 18:47:25 +0200 Subject: [PATCH 01/11] Check integer overflow/underflow in Range* iterators --- src/libcore/iter/range.rs | 75 ++++++++++++++++++++++++++------------- src/libcore/tests/iter.rs | 12 +++++++ 2 files changed, 62 insertions(+), 25 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index 32c32e327eb2d..6fd0ac77183bd 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -10,7 +10,7 @@ use convert::TryFrom; use mem; -use ops::{self, Add, Sub}; +use ops; use usize; use super::{FusedIterator, TrustedLen}; @@ -36,14 +36,11 @@ pub trait Step: Clone + PartialOrd + Sized { /// Replaces this step with `0`, returning itself fn replace_zero(&mut self) -> Self; - /// Adds one to this step, returning the result - fn add_one(&self) -> Self; - - /// Subtracts one to this step, returning the result - fn sub_one(&self) -> Self; - /// Add an usize, returning None on overflow fn add_usize(&self, n: usize) -> Option; + + /// Subtracts an usize, returning None on overflow + fn sub_usize(&self, n: usize) -> Option; } // These are still macro-generated because the integer literals resolve to different types. @@ -58,16 +55,6 @@ macro_rules! step_identical_methods { fn replace_zero(&mut self) -> Self { mem::replace(self, 0) } - - #[inline] - fn add_one(&self) -> Self { - Add::add(*self, 1) - } - - #[inline] - fn sub_one(&self) -> Self { - Sub::sub(*self, 1) - } } } @@ -96,6 +83,14 @@ macro_rules! step_impl_unsigned { } } + #[inline] + fn sub_usize(&self, n: usize) -> Option { + match <$t>::try_from(n) { + Ok(n_as_t) => self.checked_sub(n_as_t), + Err(_) => None, + } + } + step_identical_methods!(); } )*) @@ -136,6 +131,23 @@ macro_rules! step_impl_signed { Err(_) => None, } } + #[inline] + fn sub_usize(&self, n: usize) -> Option { + match <$unsigned>::try_from(n) { + Ok(n_as_unsigned) => { + // Wrapping in unsigned space handles cases like + // `-120_i8.add_usize(200) == Some(80_i8)`, + // even though 200_usize is out of range for i8. + let wrapped = (*self as $unsigned).wrapping_sub(n_as_unsigned) as $t; + if wrapped <= *self { + Some(wrapped) + } else { + None // Subtraction underflowed + } + } + Err(_) => None, + } + } step_identical_methods!(); } @@ -158,6 +170,11 @@ macro_rules! step_impl_no_between { self.checked_add(n as $t) } + #[inline] + fn sub_usize(&self, n: usize) -> Option { + self.checked_sub(n as $t) + } + step_identical_methods!(); } )*) @@ -214,7 +231,8 @@ impl Iterator for ops::Range { #[inline] fn next(&mut self) -> Option { if self.start < self.end { - let mut n = self.start.add_one(); + // `start + 1` should not overflow since `end` exists such that `start < end` + let mut n = self.start.add_usize(1).expect("overflow in Range::next"); mem::swap(&mut n, &mut self.start); Some(n) } else { @@ -234,7 +252,8 @@ impl Iterator for ops::Range { fn nth(&mut self, n: usize) -> Option { if let Some(plus_n) = self.start.add_usize(n) { if plus_n < self.end { - self.start = plus_n.add_one(); + // `plus_n + 1` should not overflow since `end` exists such that `plus_n < end` + self.start = plus_n.add_usize(1).expect("overflow in Range::nth"); return Some(plus_n) } } @@ -263,7 +282,8 @@ impl DoubleEndedIterator for ops::Range { #[inline] fn next_back(&mut self) -> Option { if self.start < self.end { - self.end = self.end.sub_one(); + // `end - 1` should not overflow since `start` exists such that `start < end` + self.end = self.end.sub_usize(1).expect("overflow in Range::nth_back"); Some(self.end.clone()) } else { None @@ -280,7 +300,8 @@ impl Iterator for ops::RangeFrom { #[inline] fn next(&mut self) -> Option { - let mut n = self.start.add_one(); + // Overflow can happen here. Panic when it does. + let mut n = self.start.add_usize(1).expect("overflow in RangeFrom::next"); mem::swap(&mut n, &mut self.start); Some(n) } @@ -292,8 +313,9 @@ impl Iterator for ops::RangeFrom { #[inline] fn nth(&mut self, n: usize) -> Option { + // Overflow can happen here. Panic when it does. let plus_n = self.start.add_usize(n).expect("overflow in RangeFrom::nth"); - self.start = plus_n.add_one(); + self.start = plus_n.add_usize(1).expect("overflow in RangeFrom::nth"); Some(plus_n) } } @@ -311,7 +333,8 @@ impl Iterator for ops::RangeInclusive { match self.start.partial_cmp(&self.end) { Some(Less) => { - let n = self.start.add_one(); + // `start + 1` should not overflow since `end` exists such that `start < end` + let n = self.start.add_usize(1).expect("overflow in RangeInclusive::next"); Some(mem::replace(&mut self.start, n)) }, Some(Equal) => { @@ -342,7 +365,8 @@ impl Iterator for ops::RangeInclusive { match plus_n.partial_cmp(&self.end) { Some(Less) => { - self.start = plus_n.add_one(); + // `plus_n + 1` should not overflow since `end` exists such that `plus_n < end` + self.start = plus_n.add_usize(1).expect("overflow in RangeInclusive::nth"); return Some(plus_n) } Some(Equal) => { @@ -368,7 +392,8 @@ impl DoubleEndedIterator for ops::RangeInclusive { match self.start.partial_cmp(&self.end) { Some(Less) => { - let n = self.end.sub_one(); + // `end - 1` should not overflow since `start` exists such that `start < end` + let n = self.end.sub_usize(1).expect("overflow in RangeInclusive::next_back"); Some(mem::replace(&mut self.end, n)) }, Some(Equal) => { diff --git a/src/libcore/tests/iter.rs b/src/libcore/tests/iter.rs index a1249a5f22cf7..efac235ee60fc 100644 --- a/src/libcore/tests/iter.rs +++ b/src/libcore/tests/iter.rs @@ -1168,6 +1168,18 @@ fn test_range_step() { assert_eq!((isize::MIN..isize::MAX).step_by(1).size_hint(), (usize::MAX, Some(usize::MAX))); } +#[test] +#[should_panic] +fn test_range_from_next_overflow() { + (255u8..).next(); +} + +#[test] +#[should_panic] +fn test_range_from_nth_overflow() { + (200u8..).nth(55); +} + #[test] fn test_repeat() { let mut it = repeat(42); From 3e8a7620ed667ee7848c3204bfadfc5eef752efe Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Thu, 6 Jul 2017 23:58:10 +0200 Subject: [PATCH 02/11] =?UTF-8?q?Don=E2=80=99t=20reset=20RangeInclusive=20?= =?UTF-8?q?to=201...0=20after=20last=20iteration.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead try to set start to end+1, and if that overflows set end to start-1. --- src/libcore/iter/range.rs | 67 +++++++++++++++++++-------------------- src/libcore/tests/iter.rs | 67 ++++++++++++++++----------------------- 2 files changed, 59 insertions(+), 75 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index 6fd0ac77183bd..aa66e4ae214cf 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -30,12 +30,6 @@ pub trait Step: Clone + PartialOrd + Sized { /// without overflow. fn steps_between(start: &Self, end: &Self) -> Option; - /// Replaces this step with `1`, returning itself - fn replace_one(&mut self) -> Self; - - /// Replaces this step with `0`, returning itself - fn replace_zero(&mut self) -> Self; - /// Add an usize, returning None on overflow fn add_usize(&self, n: usize) -> Option; @@ -43,21 +37,6 @@ pub trait Step: Clone + PartialOrd + Sized { fn sub_usize(&self, n: usize) -> Option; } -// These are still macro-generated because the integer literals resolve to different types. -macro_rules! step_identical_methods { - () => { - #[inline] - fn replace_one(&mut self) -> Self { - mem::replace(self, 1) - } - - #[inline] - fn replace_zero(&mut self) -> Self { - mem::replace(self, 0) - } - } -} - macro_rules! step_impl_unsigned { ($($t:ty)*) => ($( #[unstable(feature = "step_trait", @@ -90,8 +69,6 @@ macro_rules! step_impl_unsigned { Err(_) => None, } } - - step_identical_methods!(); } )*) } @@ -148,8 +125,6 @@ macro_rules! step_impl_signed { Err(_) => None, } } - - step_identical_methods!(); } )*) } @@ -174,8 +149,6 @@ macro_rules! step_impl_no_between { fn sub_usize(&self, n: usize) -> Option { self.checked_sub(n as $t) } - - step_identical_methods!(); } )*) } @@ -338,8 +311,15 @@ impl Iterator for ops::RangeInclusive { Some(mem::replace(&mut self.start, n)) }, Some(Equal) => { - let last = self.start.replace_one(); - self.end.replace_zero(); + let last; + if let Some(end_plus_one) = self.end.add_usize(1) { + last = mem::replace(&mut self.start, end_plus_one); + } else { + last = self.start.clone(); + // `start == end`, and `end + 1` underflowed. + // `start - 1` overflowing would imply a type with only one valid value? + self.end = self.start.sub_usize(1).expect("overflow in RangeInclusive::next"); + } Some(last) }, _ => None, @@ -370,16 +350,26 @@ impl Iterator for ops::RangeInclusive { return Some(plus_n) } Some(Equal) => { - self.start.replace_one(); - self.end.replace_zero(); + if let Some(end_plus_one) = self.end.add_usize(1) { + self.start = end_plus_one + } else { + // `start == end`, and `end + 1` underflowed. + // `start - 1` overflowing would imply a type with only one valid value? + self.end = self.start.sub_usize(1).expect("overflow in RangeInclusive::nth") + } return Some(plus_n) } _ => {} } } - self.start.replace_one(); - self.end.replace_zero(); + if let Some(end_plus_one) = self.end.add_usize(1) { + self.start = end_plus_one + } else { + // `start == end`, and `end + 1` underflowed. + // `start - 1` overflowing would imply a type with only one valid value? + self.end = self.start.sub_usize(1).expect("overflow in RangeInclusive::nth") + } None } } @@ -397,8 +387,15 @@ impl DoubleEndedIterator for ops::RangeInclusive { Some(mem::replace(&mut self.end, n)) }, Some(Equal) => { - let last = self.end.replace_zero(); - self.start.replace_one(); + let last; + if let Some(start_minus_one) = self.start.sub_usize(1) { + last = mem::replace(&mut self.end, start_minus_one); + } else { + last = self.end.clone(); + // `start == end`, and `start - 1` underflowed. + // `end + 1` overflowing would imply a type with only one valid value? + self.start = self.start.add_usize(1).expect("overflow in RangeInclusive::next_back"); + } Some(last) }, _ => None, diff --git a/src/libcore/tests/iter.rs b/src/libcore/tests/iter.rs index efac235ee60fc..2618688e9768a 100644 --- a/src/libcore/tests/iter.rs +++ b/src/libcore/tests/iter.rs @@ -1080,20 +1080,41 @@ fn test_range() { fn test_range_inclusive_exhaustion() { let mut r = 10...10; assert_eq!(r.next(), Some(10)); - assert_eq!(r, 1...0); + assert_eq!(r, 11...10); + assert_eq!(r.next(), None); + assert_eq!(r, 11...10); let mut r = 10...10; assert_eq!(r.next_back(), Some(10)); + assert_eq!(r, 10...9); + assert_eq!(r.next_back(), None); + assert_eq!(r, 10...9); + + let mut r = 0_u32...0; + assert_eq!(r.next_back(), Some(0)); + assert_eq!(r, 1...0); + assert_eq!(r.next_back(), None); assert_eq!(r, 1...0); let mut r = 10...12; assert_eq!(r.nth(2), Some(12)); - assert_eq!(r, 1...0); + assert_eq!(r, 13...12); let mut r = 10...12; assert_eq!(r.nth(5), None); - assert_eq!(r, 1...0); + assert_eq!(r, 13...12); + + let mut r = 127_i8...127; + assert_eq!(r.next(), Some(127)); + assert_eq!(r, 127...126); + assert_eq!(r.next(), None); + assert_eq!(r, 127...126); + let mut r = 255_u8...255; + assert_eq!(r.next(), Some(255)); + assert_eq!(r, 255...254); + assert_eq!(r.next(), None); + assert_eq!(r, 255...254); } #[test] @@ -1142,7 +1163,7 @@ fn test_range_inclusive_nth() { assert_eq!(r.is_empty(), false); assert_eq!(r.nth(10), None); assert_eq!(r.is_empty(), true); - assert_eq!(r, 1...0); // We may not want to document/promise this detail + assert_eq!(r, 21...20); } #[test] @@ -1263,40 +1284,6 @@ fn test_chain_fold() { } #[test] -fn test_step_replace_unsigned() { - let mut x = 4u32; - let y = x.replace_zero(); - assert_eq!(x, 0); - assert_eq!(y, 4); - - x = 5; - let y = x.replace_one(); - assert_eq!(x, 1); - assert_eq!(y, 5); -} - -#[test] -fn test_step_replace_signed() { - let mut x = 4i32; - let y = x.replace_zero(); - assert_eq!(x, 0); - assert_eq!(y, 4); - - x = 5; - let y = x.replace_one(); - assert_eq!(x, 1); - assert_eq!(y, 5); -} - -#[test] -fn test_step_replace_no_between() { - let mut x = 4u128; - let y = x.replace_zero(); - assert_eq!(x, 0); - assert_eq!(y, 4); - - x = 5; - let y = x.replace_one(); - assert_eq!(x, 1); - assert_eq!(y, 5); +fn test_step_add_usize() { + assert_eq!((-120_i8).add_usize(200), Some(80)); } From 5334226d9e55512f562830d75909e06d03896c40 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Fri, 7 Jul 2017 10:12:46 +0200 Subject: [PATCH 03/11] Rename Step::{add,sub}_usize to Step::forward and Step::backward --- src/libcore/iter/range.rs | 96 +++++++++++++++++++++------------------ src/libcore/tests/iter.rs | 14 +++++- 2 files changed, 65 insertions(+), 45 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index aa66e4ae214cf..8fcaa5d61bf45 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -15,32 +15,42 @@ use usize; use super::{FusedIterator, TrustedLen}; -/// Objects that can be stepped over in both directions. -/// -/// The `steps_between` function provides a way to efficiently compare -/// two `Step` objects. +/// Objects that have a notion of *successor* and *predecessor* +/// for the purpose of range iterators. #[unstable(feature = "step_trait", - reason = "likely to be replaced by finer-grained traits", + reason = "recently redesigned", issue = "42168")] pub trait Step: Clone + PartialOrd + Sized { - /// Returns the number of steps between two step objects. The count is - /// inclusive of `start` and exclusive of `end`. + /// Returns the number of *successor* steps needed to get from `start` to `end`. /// - /// Returns `None` if it is not possible to calculate `steps_between` - /// without overflow. + /// Returns `None` if that number would overflow `usize` + /// (or is infinite, if `end` would never be reached). + /// Returns `Some(0)` if `start` comes after (is greater than) or equals `end`. fn steps_between(start: &Self, end: &Self) -> Option; - /// Add an usize, returning None on overflow - fn add_usize(&self, n: usize) -> Option; + /// Returns the value that would be obtained by taking the *successor* of `self`, + /// `step_count` times. + /// + /// Returns `None` if this would overflow the range of values supported by the type `Self`. + /// + /// Note: `step_count == 1` is a common case, + /// used for example in `Iterator::next` for ranges. + fn forward(&self, step_count: usize) -> Option; - /// Subtracts an usize, returning None on overflow - fn sub_usize(&self, n: usize) -> Option; + /// Returns the value that would be obtained by taking the *predecessor* of `self`, + /// `step_count` times. + /// + /// Returns `None` if this would overflow the range of values supported by the type `Self`. + /// + /// Note: `step_count == 1` is a common case, + /// used for example in `Iterator::next_back` for ranges. + fn backward(&self, step_count: usize) -> Option; } macro_rules! step_impl_unsigned { ($($t:ty)*) => ($( #[unstable(feature = "step_trait", - reason = "likely to be replaced by finer-grained traits", + reason = "recently redesigned", issue = "42168")] impl Step for $t { #[inline] @@ -55,7 +65,7 @@ macro_rules! step_impl_unsigned { } #[inline] - fn add_usize(&self, n: usize) -> Option { + fn forward(&self, n: usize) -> Option { match <$t>::try_from(n) { Ok(n_as_t) => self.checked_add(n_as_t), Err(_) => None, @@ -63,7 +73,7 @@ macro_rules! step_impl_unsigned { } #[inline] - fn sub_usize(&self, n: usize) -> Option { + fn backward(&self, n: usize) -> Option { match <$t>::try_from(n) { Ok(n_as_t) => self.checked_sub(n_as_t), Err(_) => None, @@ -75,7 +85,7 @@ macro_rules! step_impl_unsigned { macro_rules! step_impl_signed { ($( [$t:ty : $unsigned:ty] )*) => ($( #[unstable(feature = "step_trait", - reason = "likely to be replaced by finer-grained traits", + reason = "recently redesigned", issue = "42168")] impl Step for $t { #[inline] @@ -92,11 +102,11 @@ macro_rules! step_impl_signed { } #[inline] - fn add_usize(&self, n: usize) -> Option { + fn forward(&self, n: usize) -> Option { match <$unsigned>::try_from(n) { Ok(n_as_unsigned) => { // Wrapping in unsigned space handles cases like - // `-120_i8.add_usize(200) == Some(80_i8)`, + // `-120_i8.forward(200) == Some(80_i8)`, // even though 200_usize is out of range for i8. let wrapped = (*self as $unsigned).wrapping_add(n_as_unsigned) as $t; if wrapped >= *self { @@ -109,11 +119,11 @@ macro_rules! step_impl_signed { } } #[inline] - fn sub_usize(&self, n: usize) -> Option { + fn backward(&self, n: usize) -> Option { match <$unsigned>::try_from(n) { Ok(n_as_unsigned) => { // Wrapping in unsigned space handles cases like - // `-120_i8.add_usize(200) == Some(80_i8)`, + // `-120_i8.forward(200) == Some(80_i8)`, // even though 200_usize is out of range for i8. let wrapped = (*self as $unsigned).wrapping_sub(n_as_unsigned) as $t; if wrapped <= *self { @@ -132,7 +142,7 @@ macro_rules! step_impl_signed { macro_rules! step_impl_no_between { ($($t:ty)*) => ($( #[unstable(feature = "step_trait", - reason = "likely to be replaced by finer-grained traits", + reason = "recently redesigned", issue = "42168")] impl Step for $t { #[inline] @@ -141,12 +151,12 @@ macro_rules! step_impl_no_between { } #[inline] - fn add_usize(&self, n: usize) -> Option { + fn forward(&self, n: usize) -> Option { self.checked_add(n as $t) } #[inline] - fn sub_usize(&self, n: usize) -> Option { + fn backward(&self, n: usize) -> Option { self.checked_sub(n as $t) } } @@ -205,7 +215,7 @@ impl Iterator for ops::Range { fn next(&mut self) -> Option { if self.start < self.end { // `start + 1` should not overflow since `end` exists such that `start < end` - let mut n = self.start.add_usize(1).expect("overflow in Range::next"); + let mut n = self.start.forward(1).expect("overflow in Range::next"); mem::swap(&mut n, &mut self.start); Some(n) } else { @@ -223,10 +233,10 @@ impl Iterator for ops::Range { #[inline] fn nth(&mut self, n: usize) -> Option { - if let Some(plus_n) = self.start.add_usize(n) { + if let Some(plus_n) = self.start.forward(n) { if plus_n < self.end { // `plus_n + 1` should not overflow since `end` exists such that `plus_n < end` - self.start = plus_n.add_usize(1).expect("overflow in Range::nth"); + self.start = plus_n.forward(1).expect("overflow in Range::nth"); return Some(plus_n) } } @@ -256,7 +266,7 @@ impl DoubleEndedIterator for ops::Range { fn next_back(&mut self) -> Option { if self.start < self.end { // `end - 1` should not overflow since `start` exists such that `start < end` - self.end = self.end.sub_usize(1).expect("overflow in Range::nth_back"); + self.end = self.end.backward(1).expect("overflow in Range::nth_back"); Some(self.end.clone()) } else { None @@ -274,7 +284,7 @@ impl Iterator for ops::RangeFrom { #[inline] fn next(&mut self) -> Option { // Overflow can happen here. Panic when it does. - let mut n = self.start.add_usize(1).expect("overflow in RangeFrom::next"); + let mut n = self.start.forward(1).expect("overflow in RangeFrom::next"); mem::swap(&mut n, &mut self.start); Some(n) } @@ -287,8 +297,8 @@ impl Iterator for ops::RangeFrom { #[inline] fn nth(&mut self, n: usize) -> Option { // Overflow can happen here. Panic when it does. - let plus_n = self.start.add_usize(n).expect("overflow in RangeFrom::nth"); - self.start = plus_n.add_usize(1).expect("overflow in RangeFrom::nth"); + let plus_n = self.start.forward(n).expect("overflow in RangeFrom::nth"); + self.start = plus_n.forward(1).expect("overflow in RangeFrom::nth"); Some(plus_n) } } @@ -307,18 +317,18 @@ impl Iterator for ops::RangeInclusive { match self.start.partial_cmp(&self.end) { Some(Less) => { // `start + 1` should not overflow since `end` exists such that `start < end` - let n = self.start.add_usize(1).expect("overflow in RangeInclusive::next"); + let n = self.start.forward(1).expect("overflow in RangeInclusive::next"); Some(mem::replace(&mut self.start, n)) }, Some(Equal) => { let last; - if let Some(end_plus_one) = self.end.add_usize(1) { + if let Some(end_plus_one) = self.end.forward(1) { last = mem::replace(&mut self.start, end_plus_one); } else { last = self.start.clone(); // `start == end`, and `end + 1` underflowed. // `start - 1` overflowing would imply a type with only one valid value? - self.end = self.start.sub_usize(1).expect("overflow in RangeInclusive::next"); + self.end = self.start.backward(1).expect("overflow in RangeInclusive::next"); } Some(last) }, @@ -340,22 +350,22 @@ impl Iterator for ops::RangeInclusive { #[inline] fn nth(&mut self, n: usize) -> Option { - if let Some(plus_n) = self.start.add_usize(n) { + if let Some(plus_n) = self.start.forward(n) { use cmp::Ordering::*; match plus_n.partial_cmp(&self.end) { Some(Less) => { // `plus_n + 1` should not overflow since `end` exists such that `plus_n < end` - self.start = plus_n.add_usize(1).expect("overflow in RangeInclusive::nth"); + self.start = plus_n.forward(1).expect("overflow in RangeInclusive::nth"); return Some(plus_n) } Some(Equal) => { - if let Some(end_plus_one) = self.end.add_usize(1) { + if let Some(end_plus_one) = self.end.forward(1) { self.start = end_plus_one } else { // `start == end`, and `end + 1` underflowed. // `start - 1` overflowing would imply a type with only one valid value? - self.end = self.start.sub_usize(1).expect("overflow in RangeInclusive::nth") + self.end = self.start.backward(1).expect("overflow in RangeInclusive::nth") } return Some(plus_n) } @@ -363,12 +373,12 @@ impl Iterator for ops::RangeInclusive { } } - if let Some(end_plus_one) = self.end.add_usize(1) { + if let Some(end_plus_one) = self.end.forward(1) { self.start = end_plus_one } else { // `start == end`, and `end + 1` underflowed. // `start - 1` overflowing would imply a type with only one valid value? - self.end = self.start.sub_usize(1).expect("overflow in RangeInclusive::nth") + self.end = self.start.backward(1).expect("overflow in RangeInclusive::nth") } None } @@ -383,18 +393,18 @@ impl DoubleEndedIterator for ops::RangeInclusive { match self.start.partial_cmp(&self.end) { Some(Less) => { // `end - 1` should not overflow since `start` exists such that `start < end` - let n = self.end.sub_usize(1).expect("overflow in RangeInclusive::next_back"); + let n = self.end.backward(1).expect("overflow in RangeInclusive::next_back"); Some(mem::replace(&mut self.end, n)) }, Some(Equal) => { let last; - if let Some(start_minus_one) = self.start.sub_usize(1) { + if let Some(start_minus_one) = self.start.backward(1) { last = mem::replace(&mut self.end, start_minus_one); } else { last = self.end.clone(); // `start == end`, and `start - 1` underflowed. // `end + 1` overflowing would imply a type with only one valid value? - self.start = self.start.add_usize(1).expect("overflow in RangeInclusive::next_back"); + self.start = self.start.forward(1).expect("overflow in RangeInclusive::next_back"); } Some(last) }, diff --git a/src/libcore/tests/iter.rs b/src/libcore/tests/iter.rs index 2618688e9768a..a7320329d9369 100644 --- a/src/libcore/tests/iter.rs +++ b/src/libcore/tests/iter.rs @@ -1284,6 +1284,16 @@ fn test_chain_fold() { } #[test] -fn test_step_add_usize() { - assert_eq!((-120_i8).add_usize(200), Some(80)); +fn test_steps_between() { + assert_eq!(Step::steps_between(&-120_i8, &80_i8), Some(200_usize)); +} + +#[test] +fn test_step_forward() { + assert_eq!((-120_i8).forward(200_usize), Some(80_i8)); +} + +#[test] +fn test_step_backward() { + assert_eq!((120_i8).backward(200_usize), Some(-80_i8)); } From 32d963e46873a5788a719578c47d6e9890ec5ec4 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Fri, 7 Jul 2017 12:21:06 +0200 Subject: [PATCH 04/11] Rewrite mixed-width mixed-signedness integer arithmetic in Step impls --- src/libcore/iter/range.rs | 257 +++++++++++++++++++++++--------------- 1 file changed, 153 insertions(+), 104 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index 8fcaa5d61bf45..e46eba949d655 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -47,133 +47,182 @@ pub trait Step: Clone + PartialOrd + Sized { fn backward(&self, step_count: usize) -> Option; } -macro_rules! step_impl_unsigned { - ($($t:ty)*) => ($( - #[unstable(feature = "step_trait", - reason = "recently redesigned", - issue = "42168")] - impl Step for $t { - #[inline] - #[allow(trivial_numeric_casts)] - fn steps_between(start: &$t, end: &$t) -> Option { - if *start < *end { - // Note: We assume $t <= usize here - Some((*end - *start) as usize) - } else { - Some(0) +macro_rules! step_integer_impls { + ( + narrower than or same width as usize: + $( [ $narrower_unsigned:ident $narrower_signed: ident ] ),+; + wider than usize: + $( [ $wider_unsigned:ident $wider_signed: ident ] ),+; + ) => { + $( + #[unstable(feature = "step_trait", + reason = "recently redesigned", + issue = "42168")] + impl Step for $narrower_unsigned { + #[inline] + fn steps_between(start: &Self, end: &Self) -> Option { + if *start < *end { + // This relies on $narrower_unsigned <= usize + Some((*end - *start) as usize) + } else { + Some(0) + } } - } - #[inline] - fn forward(&self, n: usize) -> Option { - match <$t>::try_from(n) { - Ok(n_as_t) => self.checked_add(n_as_t), - Err(_) => None, + #[inline] + fn forward(&self, n: usize) -> Option { + match Self::try_from(n) { + Ok(n_converted) => self.checked_add(n_converted), + Err(_) => None, // if n is out of range, `something_unsigned + n` is too + } } - } - #[inline] - fn backward(&self, n: usize) -> Option { - match <$t>::try_from(n) { - Ok(n_as_t) => self.checked_sub(n_as_t), - Err(_) => None, + #[inline] + fn backward(&self, n: usize) -> Option { + match Self::try_from(n) { + Ok(n_converted) => self.checked_sub(n_converted), + Err(_) => None, // if n is out of range, `something_in_range - n` is too + } } } - } - )*) -} -macro_rules! step_impl_signed { - ($( [$t:ty : $unsigned:ty] )*) => ($( - #[unstable(feature = "step_trait", - reason = "recently redesigned", - issue = "42168")] - impl Step for $t { - #[inline] - #[allow(trivial_numeric_casts)] - fn steps_between(start: &$t, end: &$t) -> Option { - if *start < *end { - // Note: We assume $t <= isize here - // Use .wrapping_sub and cast to usize to compute the - // difference that may not fit inside the range of isize. - Some((*end as isize).wrapping_sub(*start as isize) as usize) - } else { - Some(0) + + #[unstable(feature = "step_trait", + reason = "recently redesigned", + issue = "42168")] + impl Step for $narrower_signed { + #[inline] + fn steps_between(start: &Self, end: &Self) -> Option { + if *start < *end { + // This relies on $narrower_signed <= usize + // + // Casting to isize extends the width but preserves the sign. + // Use wrapping_sub in isize space and cast to usize + // to compute the difference that may not fit inside the range of isize. + Some((*end as isize).wrapping_sub(*start as isize) as usize) + } else { + Some(0) + } } - } - #[inline] - fn forward(&self, n: usize) -> Option { - match <$unsigned>::try_from(n) { - Ok(n_as_unsigned) => { - // Wrapping in unsigned space handles cases like - // `-120_i8.forward(200) == Some(80_i8)`, - // even though 200_usize is out of range for i8. - let wrapped = (*self as $unsigned).wrapping_add(n_as_unsigned) as $t; - if wrapped >= *self { - Some(wrapped) - } else { - None // Addition overflowed + #[inline] + fn forward(&self, n: usize) -> Option { + match <$narrower_unsigned>::try_from(n) { + Ok(n_unsigned) => { + // Wrapping in unsigned space handles cases like + // `-120_i8.forward(200) == Some(80_i8)`, + // even though 200_usize is out of range for i8. + let self_unsigned = *self as $narrower_unsigned; + let wrapped = self_unsigned.wrapping_add(n_unsigned) as Self; + if wrapped >= *self { + Some(wrapped) + } else { + None // Addition overflowed + } } + // If n is out of range of e.g. u8, + // then it is bigger than the entire range for i8 is wide + // so `any_i8 + n` would overflow i8. + Err(_) => None, } - Err(_) => None, } - } - #[inline] - fn backward(&self, n: usize) -> Option { - match <$unsigned>::try_from(n) { - Ok(n_as_unsigned) => { - // Wrapping in unsigned space handles cases like - // `-120_i8.forward(200) == Some(80_i8)`, - // even though 200_usize is out of range for i8. - let wrapped = (*self as $unsigned).wrapping_sub(n_as_unsigned) as $t; - if wrapped <= *self { - Some(wrapped) - } else { - None // Subtraction underflowed + #[inline] + fn backward(&self, n: usize) -> Option { + match <$narrower_unsigned>::try_from(n) { + Ok(n_unsigned) => { + // Wrapping in unsigned space handles cases like + // `-120_i8.forward(200) == Some(80_i8)`, + // even though 200_usize is out of range for i8. + let self_unsigned = *self as $narrower_unsigned; + let wrapped = self_unsigned.wrapping_sub(n_unsigned) as Self; + if wrapped <= *self { + Some(wrapped) + } else { + None // Subtraction underflowed + } } + // If n is out of range of e.g. u8, + // then it is bigger than the entire range for i8 is wide + // so `any_i8 - n` would underflow i8. + Err(_) => None, } - Err(_) => None, } } - } - )*) -} + )+ + + $( + #[unstable(feature = "step_trait", + reason = "recently redesigned", + issue = "42168")] + impl Step for $wider_unsigned { + #[inline] + fn steps_between(start: &Self, end: &Self) -> Option { + if *start < *end { + usize::try_from(*end - *start).ok() + } else { + Some(0) + } + } -macro_rules! step_impl_no_between { - ($($t:ty)*) => ($( - #[unstable(feature = "step_trait", - reason = "recently redesigned", - issue = "42168")] - impl Step for $t { - #[inline] - fn steps_between(_start: &Self, _end: &Self) -> Option { - None - } + #[inline] + fn forward(&self, n: usize) -> Option { + self.checked_add(n as Self) + } - #[inline] - fn forward(&self, n: usize) -> Option { - self.checked_add(n as $t) + #[inline] + fn backward(&self, n: usize) -> Option { + self.checked_sub(n as Self) + } } - #[inline] - fn backward(&self, n: usize) -> Option { - self.checked_sub(n as $t) + #[unstable(feature = "step_trait", + reason = "recently redesigned", + issue = "42168")] + impl Step for $wider_signed { + #[inline] + fn steps_between(start: &Self, end: &Self) -> Option { + if *start < *end { + match end.checked_sub(*start) { + Some(diff) => usize::try_from(diff).ok(), + // If the difference is too big for e.g. i128, + // it’s also gonna be too big for usize with fewer bits. + None => None + } + } else { + Some(0) + } + } + + #[inline] + fn forward(&self, n: usize) -> Option { + self.checked_add(n as Self) + } + + #[inline] + fn backward(&self, n: usize) -> Option { + self.checked_sub(n as Self) + } } - } - )*) + )+ + } } -step_impl_unsigned!(usize u8 u16 u32); -step_impl_signed!([isize: usize] [i8: u8] [i16: u16] [i32: u32]); -#[cfg(target_pointer_width = "64")] -step_impl_unsigned!(u64); #[cfg(target_pointer_width = "64")] -step_impl_signed!([i64: u64]); -// If the target pointer width is not 64-bits, we -// assume here that it is less than 64-bits. -#[cfg(not(target_pointer_width = "64"))] -step_impl_no_between!(u64 i64); -step_impl_no_between!(u128 i128); +step_integer_impls! { + narrower than or same width as usize: [u8 i8], [u16 i16], [u32 i32], [u64 i64], [usize isize]; + wider than usize: [u128 i128]; +} + +#[cfg(target_pointer_width = "32")] +step_integer_impls! { + narrower than or same width as usize: [u8 i8], [u16 i16], [u32 i32], [usize isize]; + wider than usize: [u64 i64], [u128 i128]; +} + +#[cfg(target_pointer_width = "16")] +step_integer_impls! { + narrower than or same width as usize: [u8 i8], [u16 i16], [usize isize]; + wider than usize: [u32 i32], [u64 i64], [u128 i128]; +} macro_rules! range_exact_iter_impl { ($($t:ty)*) => ($( From 9d4dc29fa537e2a8425bb79901ccf4fd0d04992a Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Fri, 7 Jul 2017 19:05:27 +0200 Subject: [PATCH 05/11] Add tests for Step trait methods --- src/libcore/tests/iter.rs | 69 +++++++++++++++++-- src/libcore/tests/lib.rs | 1 + .../run-pass/impl-trait/example-calendar.rs | 22 ++---- 3 files changed, 73 insertions(+), 19 deletions(-) diff --git a/src/libcore/tests/iter.rs b/src/libcore/tests/iter.rs index a7320329d9369..2b2c11b98cd69 100644 --- a/src/libcore/tests/iter.rs +++ b/src/libcore/tests/iter.rs @@ -9,8 +9,7 @@ // except according to those terms. use core::iter::*; -use core::{i8, i16, isize}; -use core::usize; +use core::{i8, i16, isize, u16, usize, i128, u128}; #[test] fn test_lt() { @@ -1285,15 +1284,77 @@ fn test_chain_fold() { #[test] fn test_steps_between() { + assert_eq!(Step::steps_between(&20_u8, &200_u8), Some(180_usize)); + assert_eq!(Step::steps_between(&-20_i8, &80_i8), Some(100_usize)); assert_eq!(Step::steps_between(&-120_i8, &80_i8), Some(200_usize)); + + assert_eq!(Step::steps_between(&20_u16, &40_100_u16), Some(40_080_usize)); + assert_eq!(Step::steps_between(&-20_i16, &80_i16), Some(100_usize)); + assert_eq!(Step::steps_between(&-20_030_i16, &20_050_i16), Some(40_080_usize)); + + assert_eq!(Step::steps_between(&20_u32, &4_000_100_u32), Some(4_000_080_usize)); + assert_eq!(Step::steps_between(&-20_i32, &80_i32), Some(100_usize)); + assert_eq!(Step::steps_between(&-2_000_030_i32, &2_000_050_i32), Some(4_000_080_usize)); + + // Skip u64 / i64 to avoid dealing with 32-bit vs 64-bit platforms. + + assert_eq!(Step::steps_between(&20_u128, &200_u128), Some(180_usize)); + assert_eq!(Step::steps_between(&-20_i128, &80_i128), Some(100_usize)); + if cfg!(target_pointer_width = "64") { + assert_eq!(Step::steps_between(&20_u128, &0x1_0000_0000_0000_0019_u128), Some(usize::MAX)); + assert_eq!(Step::steps_between(&20_i128, &0x1_0000_0000_0000_0019_i128), Some(usize::MAX)); + } + assert_eq!(Step::steps_between(&20_u128, &0x1_0000_0000_0000_0020_u128), None); + assert_eq!(Step::steps_between(&20_i128, &0x1_0000_0000_0000_0020_i128), None); + assert_eq!(Step::steps_between(&-0x1_0000_0000_0000_0000_i128, &0x1_0000_0000_0000_0000_i128), None); } #[test] fn test_step_forward() { - assert_eq!((-120_i8).forward(200_usize), Some(80_i8)); + assert_eq!((55_u8).forward(200_usize), Some(255_u8)); + assert_eq!((252_u8).forward(200_usize), None); + assert_eq!((0_u8).forward(256_usize), None); + assert_eq!((-110_i8).forward(200_usize), Some(90_i8)); + assert_eq!((-110_i8).forward(248_usize), None); + assert_eq!((-126_i8).forward(256_usize), None); + + assert_eq!((35_u16).forward(100_usize), Some(135_u16)); + assert_eq!((35_u16).forward(65500_usize), Some(u16::MAX)); + assert_eq!((36_u16).forward(65500_usize), None); + assert_eq!((-110_i16).forward(200_usize), Some(90_i16)); + assert_eq!((-20_030_i16).forward(50_050_usize), Some(30_020_i16)); + assert_eq!((-10_i16).forward(40_000_usize), None); + assert_eq!((-10_i16).forward(70_000_usize), None); + + assert_eq!((10_u128).forward(70_000_usize), Some(70_010_u128)); + assert_eq!((10_i128).forward(70_030_usize), Some(70_020_i128)); + assert_eq!((0xffff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_u128).forward(0xff_usize), Some(u128::MAX)); + assert_eq!((0xffff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_u128).forward(0x100_usize), None); + assert_eq!((0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).forward(0xff_usize), Some(i128::MAX)); + assert_eq!((0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).forward(0x100_usize), None); } #[test] fn test_step_backward() { - assert_eq!((120_i8).backward(200_usize), Some(-80_i8)); + assert_eq!((255_u8).backward(200_usize), Some(55_u8)); + assert_eq!((100_u8).backward(200_usize), None); + assert_eq!((255_u8).backward(256_usize), None); + assert_eq!((90_i8).backward(200_usize), Some(-110_i8)); + assert_eq!((110_i8).backward(248_usize), None); + assert_eq!((127_i8).backward(256_usize), None); + + assert_eq!((135_u16).backward(100_usize), Some(35_u16)); + assert_eq!((u16::MAX).backward(65500_usize), Some(35_u16)); + assert_eq!((10_u16).backward(11_usize), None); + assert_eq!((90_i16).backward(200_usize), Some(-110_i16)); + assert_eq!((30_020_i16).backward(50_050_usize), Some(-20_030_i16)); + assert_eq!((-10_i16).backward(40_000_usize), None); + assert_eq!((-10_i16).backward(70_000_usize), None); + + assert_eq!((70_010_u128).backward(70_000_usize), Some(10_u128)); + assert_eq!((70_020_i128).backward(70_030_usize), Some(10_i128)); + assert_eq!((10_u128).backward(7_usize), Some(3_u128)); + assert_eq!((10_u128).backward(11_usize), None); + assert_eq!((-0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).backward(0xfe_usize), Some(i128::MIN)); + assert_eq!((-0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).backward(0xff_usize), Some(i128::MIN)); } diff --git a/src/libcore/tests/lib.rs b/src/libcore/tests/lib.rs index 26e4c21dc8f4d..c62e1042f7066 100644 --- a/src/libcore/tests/lib.rs +++ b/src/libcore/tests/lib.rs @@ -23,6 +23,7 @@ #![feature(flt2dec)] #![feature(fmt_internals)] #![feature(iterator_step_by)] +#![feature(i128)] #![feature(i128_type)] #![feature(inclusive_range)] #![feature(inclusive_range_syntax)] diff --git a/src/test/run-pass/impl-trait/example-calendar.rs b/src/test/run-pass/impl-trait/example-calendar.rs index 84d86cfdf65a4..820371e013f8e 100644 --- a/src/test/run-pass/impl-trait/example-calendar.rs +++ b/src/test/run-pass/impl-trait/example-calendar.rs @@ -166,23 +166,15 @@ impl std::iter::Step for NaiveDate { unimplemented!() } - fn replace_one(&mut self) -> Self { - mem::replace(self, NaiveDate(0, 0, 1)) - } - - fn replace_zero(&mut self) -> Self { - mem::replace(self, NaiveDate(0, 0, 0)) - } - - fn add_one(&self) -> Self { - self.succ() - } - - fn sub_one(&self) -> Self { - unimplemented!() + fn forward(&self, step_count: usize) -> Option { + let mut result = *self; + for _ in 0..step_count { + result = result.succ(); + } + Some(result) } - fn add_usize(&self, _: usize) -> Option { + fn backward(&self, _step_count: usize) -> Option { unimplemented!() } } From c0b4440309cd971fcbda9611f9419906f715ab81 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sat, 8 Jul 2017 11:52:00 +0200 Subject: [PATCH 06/11] Implement TrustedLen for ranges of all integer types --- src/libcore/iter/range.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index e46eba949d655..2a56334fc1a4a 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -61,6 +61,8 @@ macro_rules! step_integer_impls { impl Step for $narrower_unsigned { #[inline] fn steps_between(start: &Self, end: &Self) -> Option { + // NOTE: the safety of `unsafe impl TrustedLen` depends on + // this being correct! if *start < *end { // This relies on $narrower_unsigned <= usize Some((*end - *start) as usize) @@ -92,6 +94,8 @@ macro_rules! step_integer_impls { impl Step for $narrower_signed { #[inline] fn steps_between(start: &Self, end: &Self) -> Option { + // NOTE: the safety of `unsafe impl TrustedLen` depends on + // this being correct! if *start < *end { // This relies on $narrower_signed <= usize // @@ -156,6 +160,8 @@ macro_rules! step_integer_impls { impl Step for $wider_unsigned { #[inline] fn steps_between(start: &Self, end: &Self) -> Option { + // NOTE: the safety of `unsafe impl TrustedLen` depends on + // this being correct! if *start < *end { usize::try_from(*end - *start).ok() } else { @@ -180,6 +186,8 @@ macro_rules! step_integer_impls { impl Step for $wider_signed { #[inline] fn steps_between(start: &Self, end: &Self) -> Option { + // NOTE: the safety of `unsafe impl TrustedLen` depends on + // this being correct! if *start < *end { match end.checked_sub(*start) { Some(diff) => usize::try_from(diff).ok(), @@ -274,6 +282,7 @@ impl Iterator for ops::Range { #[inline] fn size_hint(&self) -> (usize, Option) { + // NOTE: the safety of `unsafe impl TrustedLen` depends on this being correct! match Step::steps_between(&self.start, &self.end) { Some(hint) => (hint, Some(hint)), None => (0, None) @@ -306,8 +315,14 @@ range_incl_exact_iter_impl!(u8 u16 i8 i16); // // They need to guarantee that .size_hint() is either exact, or that // the upper bound is None when it does not fit the type limits. -range_trusted_len_impl!(usize isize u8 i8 u16 i16 u32 i32 i64 u64); -range_incl_trusted_len_impl!(usize isize u8 i8 u16 i16 u32 i32 i64 u64); +range_trusted_len_impl! { + usize u8 u16 u32 u64 u128 + isize i8 i16 i32 i64 i128 +} +range_incl_trusted_len_impl! { + usize u8 u16 u32 u64 u128 + isize i8 i16 i32 i64 i128 +} #[stable(feature = "rust1", since = "1.0.0")] impl DoubleEndedIterator for ops::Range { @@ -387,6 +402,8 @@ impl Iterator for ops::RangeInclusive { #[inline] fn size_hint(&self) -> (usize, Option) { + // NOTE: the safety of `unsafe impl TrustedLen` depends on this being correct! + if !(self.start <= self.end) { return (0, Some(0)); } From b270e7f524c7a2ff41beb3bf93edf23e2ffc7e22 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sat, 8 Jul 2017 12:09:21 +0200 Subject: [PATCH 07/11] Tighter size_hint lower bound for ranges larger than usize --- src/libcore/iter/range.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index 2a56334fc1a4a..d20e84c7f8913 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -285,7 +285,7 @@ impl Iterator for ops::Range { // NOTE: the safety of `unsafe impl TrustedLen` depends on this being correct! match Step::steps_between(&self.start, &self.end) { Some(hint) => (hint, Some(hint)), - None => (0, None) + None => (usize::MAX, None) } } @@ -410,7 +410,7 @@ impl Iterator for ops::RangeInclusive { match Step::steps_between(&self.start, &self.end) { Some(hint) => (hint.saturating_add(1), hint.checked_add(1)), - None => (0, None), + None => (usize::MAX, None), } } From 287ce4c4ea03bb0ae152a8fcb82921fae25ae865 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sat, 8 Jul 2017 21:54:30 +0200 Subject: [PATCH 08/11] Remove ExactSizeIterator impls for RangeInclusive<{i16,u16}> They are incorrect on 16-bit platforms since the return value of `len()` might overflow `usize`. Impls for `Range` and `Range` are similarly incorrect, but were stabilized in Rust 1.0.0 so removing them would be a breaking change. `(0..66_000_u32).len()` for example will compile without error or warnings on 16-bit platforms, but panic at run-time. --- src/libcore/iter/range.rs | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index d20e84c7f8913..ab6a3a06d6937 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -305,11 +305,34 @@ impl Iterator for ops::Range { } // These macros generate `ExactSizeIterator` impls for various range types. -// Range<{u,i}64> and RangeInclusive<{u,i}{32,64,size}> are excluded -// because they cannot guarantee having a length <= usize::MAX, which is -// required by ExactSizeIterator. -range_exact_iter_impl!(usize u8 u16 u32 isize i8 i16 i32); -range_incl_exact_iter_impl!(u8 u16 i8 i16); +// +// * `ExactSizeIterator::len` is required to always return an exact `usize`, +// so no range can be longer than usize::MAX. +// * For integer types in `Range<_>` this is the case for types narrower than or as wide as `usize`. +// For integer types in `RangeInclusive<_>` +// this is the case for types *strictly narrower* than `usize` +// since e.g. `(0...u64::MAX).len()` would be `u64::MAX + 1`. +// * It hurts portability to have APIs (including `impl`s) that are only available on some platforms, +// so only `impl`s that are always valid should exist, regardless of the current target platform. +// (NOTE: https://github.com/rust-lang/rust/issues/41619 might change this.) +// * Support exists in-tree for MSP430: https://forge.rust-lang.org/platform-support.html#tier-3 +// and out-of-tree for AVR: https://github.com/avr-rust/rust +// Both are platforms where `usize` is 16 bits wide. +range_exact_iter_impl! { + usize u8 u16 + isize i8 i16 + + // These are incorrect per the reasoning above, + // but removing them would be a breaking change as they were stabilized in Rust 1.0.0. + // So `(0..66_000_u32).len()` for example will compile without error or warnings + // on 16-bit platforms, but panic at run-time. + u32 + i32 +} +range_incl_exact_iter_impl! { + u8 + i8 +} // These macros generate `TrustedLen` impls. // From eed9884343d344c890ce1fe840d7fb679b71f3bb Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sat, 8 Jul 2017 12:07:43 +0200 Subject: [PATCH 09/11] Add a comment on the `start > end` check in RangeInclusive::size_hint --- src/libcore/iter/range.rs | 7 +++++-- src/libcore/tests/iter.rs | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index ab6a3a06d6937..95f89b3eef78b 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -425,12 +425,15 @@ impl Iterator for ops::RangeInclusive { #[inline] fn size_hint(&self) -> (usize, Option) { - // NOTE: the safety of `unsafe impl TrustedLen` depends on this being correct! - + // This seems redundant with a similar comparison that `Step::steps_between` + // implementations need to do, but it separates the `start > end` case from `start == end`. + // `steps_between` returns `Some(0)` in both of these cases, but we only want to add 1 + // in the latter. if !(self.start <= self.end) { return (0, Some(0)); } + // NOTE: the safety of `unsafe impl TrustedLen` depends on this being correct! match Step::steps_between(&self.start, &self.end) { Some(hint) => (hint.saturating_add(1), hint.checked_add(1)), None => (usize::MAX, None), diff --git a/src/libcore/tests/iter.rs b/src/libcore/tests/iter.rs index 2b2c11b98cd69..65929b37b74f9 100644 --- a/src/libcore/tests/iter.rs +++ b/src/libcore/tests/iter.rs @@ -1165,6 +1165,24 @@ fn test_range_inclusive_nth() { assert_eq!(r, 21...20); } +#[test] +fn test_range_len() { + assert_eq!((0..10_u8).len(), 10); + assert_eq!((9..10_u8).len(), 1); + assert_eq!((10..10_u8).len(), 0); + assert_eq!((11..10_u8).len(), 0); + assert_eq!((100..10_u8).len(), 0); +} + +#[test] +fn test_range_inclusive_len() { + assert_eq!((0...10_u8).len(), 11); + assert_eq!((9...10_u8).len(), 2); + assert_eq!((10...10_u8).len(), 1); + assert_eq!((11...10_u8).len(), 0); + assert_eq!((100...10_u8).len(), 0); +} + #[test] fn test_range_step() { #![allow(deprecated)] From 0a622694ca07b04d38c29c921fe7e41f1b34673b Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sun, 9 Jul 2017 09:14:03 +0200 Subject: [PATCH 10/11] Make tidy happy --- src/libcore/iter/range.rs | 6 ++++-- src/libcore/tests/iter.rs | 15 ++++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index 95f89b3eef78b..9a0f78c3dada8 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -312,7 +312,8 @@ impl Iterator for ops::Range { // For integer types in `RangeInclusive<_>` // this is the case for types *strictly narrower* than `usize` // since e.g. `(0...u64::MAX).len()` would be `u64::MAX + 1`. -// * It hurts portability to have APIs (including `impl`s) that are only available on some platforms, +// * It hurts portability to have APIs (including `impl`s) +// that are only available on some platforms, // so only `impl`s that are always valid should exist, regardless of the current target platform. // (NOTE: https://github.com/rust-lang/rust/issues/41619 might change this.) // * Support exists in-tree for MSP430: https://forge.rust-lang.org/platform-support.html#tier-3 @@ -496,7 +497,8 @@ impl DoubleEndedIterator for ops::RangeInclusive { last = self.end.clone(); // `start == end`, and `start - 1` underflowed. // `end + 1` overflowing would imply a type with only one valid value? - self.start = self.start.forward(1).expect("overflow in RangeInclusive::next_back"); + self.start = + self.start.forward(1).expect("overflow in RangeInclusive::next_back"); } Some(last) }, diff --git a/src/libcore/tests/iter.rs b/src/libcore/tests/iter.rs index 65929b37b74f9..873bd2907d096 100644 --- a/src/libcore/tests/iter.rs +++ b/src/libcore/tests/iter.rs @@ -1324,7 +1324,8 @@ fn test_steps_between() { } assert_eq!(Step::steps_between(&20_u128, &0x1_0000_0000_0000_0020_u128), None); assert_eq!(Step::steps_between(&20_i128, &0x1_0000_0000_0000_0020_i128), None); - assert_eq!(Step::steps_between(&-0x1_0000_0000_0000_0000_i128, &0x1_0000_0000_0000_0000_i128), None); + assert_eq!(Step::steps_between(&-0x1_0000_0000_0000_0000_i128, &0x1_0000_0000_0000_0000_i128), + None); } #[test] @@ -1346,9 +1347,11 @@ fn test_step_forward() { assert_eq!((10_u128).forward(70_000_usize), Some(70_010_u128)); assert_eq!((10_i128).forward(70_030_usize), Some(70_020_i128)); - assert_eq!((0xffff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_u128).forward(0xff_usize), Some(u128::MAX)); + assert_eq!((0xffff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_u128).forward(0xff_usize), + Some(u128::MAX)); assert_eq!((0xffff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_u128).forward(0x100_usize), None); - assert_eq!((0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).forward(0xff_usize), Some(i128::MAX)); + assert_eq!((0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).forward(0xff_usize), + Some(i128::MAX)); assert_eq!((0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).forward(0x100_usize), None); } @@ -1373,6 +1376,8 @@ fn test_step_backward() { assert_eq!((70_020_i128).backward(70_030_usize), Some(10_i128)); assert_eq!((10_u128).backward(7_usize), Some(3_u128)); assert_eq!((10_u128).backward(11_usize), None); - assert_eq!((-0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).backward(0xfe_usize), Some(i128::MIN)); - assert_eq!((-0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).backward(0xff_usize), Some(i128::MIN)); + assert_eq!((-0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).backward(0xfe_usize), + Some(i128::MIN)); + assert_eq!((-0x7fff_ffff_ffff_ffff__ffff_ffff_ffff_ff00_i128).backward(0xff_usize), + Some(i128::MIN)); } From 0ac7fca8341f132d228d95fa7c6d5e9c07763969 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sun, 9 Jul 2017 09:33:41 +0200 Subject: [PATCH 11/11] Implement TrustedLen for ranges of all T: Step, make Step an unsafe trait. --- src/libcore/iter/range.rs | 73 ++++++++----------- .../run-pass/impl-trait/example-calendar.rs | 2 +- 2 files changed, 31 insertions(+), 44 deletions(-) diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs index 9a0f78c3dada8..9beda167f8cae 100644 --- a/src/libcore/iter/range.rs +++ b/src/libcore/iter/range.rs @@ -17,15 +17,23 @@ use super::{FusedIterator, TrustedLen}; /// Objects that have a notion of *successor* and *predecessor* /// for the purpose of range iterators. +/// +/// This trait is `unsafe` because implementations of the `unsafe` trait `TrustedLen` +/// depend on its implementations being correct. #[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")] -pub trait Step: Clone + PartialOrd + Sized { +pub unsafe trait Step: Clone + PartialOrd + Sized { /// Returns the number of *successor* steps needed to get from `start` to `end`. /// /// Returns `None` if that number would overflow `usize` /// (or is infinite, if `end` would never be reached). - /// Returns `Some(0)` if `start` comes after (is greater than) or equals `end`. + /// + /// This must hold for any `a`, `b`, and `n`: + /// + /// * `steps_between(&a, &b) == Some(0)` if and only if `a >= b`. + /// * `steps_between(&a, &b) == Some(n)` if and only if `a.forward(n) == Some(b)` + /// * `steps_between(&a, &b) == Some(n)` if and only if `b.backward(n) == Some(a)` fn steps_between(start: &Self, end: &Self) -> Option; /// Returns the value that would be obtained by taking the *successor* of `self`, @@ -35,6 +43,10 @@ pub trait Step: Clone + PartialOrd + Sized { /// /// Note: `step_count == 1` is a common case, /// used for example in `Iterator::next` for ranges. + /// + /// This must hold for any `a`, `n`, and `m` where `n + m` doesn’t overflow: + /// + /// * `a.forward(n).and_then(|x| x.forward(m)) == a.forward(n + m)` fn forward(&self, step_count: usize) -> Option; /// Returns the value that would be obtained by taking the *predecessor* of `self`, @@ -44,6 +56,10 @@ pub trait Step: Clone + PartialOrd + Sized { /// /// Note: `step_count == 1` is a common case, /// used for example in `Iterator::next_back` for ranges. + /// + /// This must hold for any `a`, `n`, and `m` where `n + m` doesn’t overflow: + /// + /// * `a.backward(n).and_then(|x| x.backward(m)) == a.backward(n + m)` fn backward(&self, step_count: usize) -> Option; } @@ -58,11 +74,9 @@ macro_rules! step_integer_impls { #[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")] - impl Step for $narrower_unsigned { + unsafe impl Step for $narrower_unsigned { #[inline] fn steps_between(start: &Self, end: &Self) -> Option { - // NOTE: the safety of `unsafe impl TrustedLen` depends on - // this being correct! if *start < *end { // This relies on $narrower_unsigned <= usize Some((*end - *start) as usize) @@ -91,11 +105,9 @@ macro_rules! step_integer_impls { #[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")] - impl Step for $narrower_signed { + unsafe impl Step for $narrower_signed { #[inline] fn steps_between(start: &Self, end: &Self) -> Option { - // NOTE: the safety of `unsafe impl TrustedLen` depends on - // this being correct! if *start < *end { // This relies on $narrower_signed <= usize // @@ -157,11 +169,9 @@ macro_rules! step_integer_impls { #[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")] - impl Step for $wider_unsigned { + unsafe impl Step for $wider_unsigned { #[inline] fn steps_between(start: &Self, end: &Self) -> Option { - // NOTE: the safety of `unsafe impl TrustedLen` depends on - // this being correct! if *start < *end { usize::try_from(*end - *start).ok() } else { @@ -183,11 +193,9 @@ macro_rules! step_integer_impls { #[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")] - impl Step for $wider_signed { + unsafe impl Step for $wider_signed { #[inline] fn steps_between(start: &Self, end: &Self) -> Option { - // NOTE: the safety of `unsafe impl TrustedLen` depends on - // this being correct! if *start < *end { match end.checked_sub(*start) { Some(diff) => usize::try_from(diff).ok(), @@ -248,22 +256,6 @@ macro_rules! range_incl_exact_iter_impl { )*) } -macro_rules! range_trusted_len_impl { - ($($t:ty)*) => ($( - #[unstable(feature = "trusted_len", issue = "37572")] - unsafe impl TrustedLen for ops::Range<$t> { } - )*) -} - -macro_rules! range_incl_trusted_len_impl { - ($($t:ty)*) => ($( - #[unstable(feature = "inclusive_range", - reason = "recently added, follows RFC", - issue = "28237")] - unsafe impl TrustedLen for ops::RangeInclusive<$t> { } - )*) -} - #[stable(feature = "rust1", since = "1.0.0")] impl Iterator for ops::Range { type Item = A; @@ -335,19 +327,6 @@ range_incl_exact_iter_impl! { i8 } -// These macros generate `TrustedLen` impls. -// -// They need to guarantee that .size_hint() is either exact, or that -// the upper bound is None when it does not fit the type limits. -range_trusted_len_impl! { - usize u8 u16 u32 u64 u128 - isize i8 i16 i32 i64 i128 -} -range_incl_trusted_len_impl! { - usize u8 u16 u32 u64 u128 - isize i8 i16 i32 i64 i128 -} - #[stable(feature = "rust1", since = "1.0.0")] impl DoubleEndedIterator for ops::Range { #[inline] @@ -362,6 +341,9 @@ impl DoubleEndedIterator for ops::Range { } } +#[unstable(feature = "trusted_len", issue = "37572")] +unsafe impl TrustedLen for ops::Range {} + #[unstable(feature = "fused", issue = "35602")] impl FusedIterator for ops::Range {} @@ -507,5 +489,10 @@ impl DoubleEndedIterator for ops::RangeInclusive { } } +#[unstable(feature = "inclusive_range", + reason = "recently added, follows RFC", + issue = "28237")] +unsafe impl TrustedLen for ops::RangeInclusive { } + #[unstable(feature = "fused", issue = "35602")] impl FusedIterator for ops::RangeInclusive {} diff --git a/src/test/run-pass/impl-trait/example-calendar.rs b/src/test/run-pass/impl-trait/example-calendar.rs index 820371e013f8e..6561d419357f9 100644 --- a/src/test/run-pass/impl-trait/example-calendar.rs +++ b/src/test/run-pass/impl-trait/example-calendar.rs @@ -161,7 +161,7 @@ impl<'a, 'b> std::ops::Add<&'b NaiveDate> for &'a NaiveDate { } } -impl std::iter::Step for NaiveDate { +unsafe impl std::iter::Step for NaiveDate { fn steps_between(_: &Self, _: &Self) -> Option { unimplemented!() }