Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

feat(core): impl Step for NonZero<u*> #127534

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+235 −18
Open

feat(core): impl Step for NonZero<u*> #127534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfe9f8d
feat(core): impl Step for NonZero<u*>
jalil-salame Jul 9, 2024
18adf5f
chore: update ui tests
jalil-salame Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 150 additions & 0 deletions library/core/src/iter/range.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ macro_rules! unsafe_impl_trusted_step {
)*};
}
unsafe_impl_trusted_step![AsciiChar char i8 i16 i32 i64 i128 isize u8 u16 u32 u64 u128 usize Ipv4Addr Ipv6Addr];
unsafe_impl_trusted_step![NonZero<u8> NonZero<u16> NonZero<u32> NonZero<u64> NonZero<u128> NonZero<usize>];

/// Objects that have a notion of *successor* and *predecessor* operations.
///
Expand Down Expand Up @@ -431,6 +432,138 @@ step_integer_impls! {
wider than usize: [u32 i32], [u64 i64], [u128 i128];
}

// These are still macro-generated because the integer literals resolve to different types.
macro_rules! step_nonzero_identical_methods {
($int:ident) => {
#[inline]
unsafe fn forward_unchecked(start: Self, n: usize) -> Self {
// SAFETY: the caller has to guarantee that `start + n` doesn't overflow.
unsafe { Self::new_unchecked(start.get().unchecked_add(n as $int)) }
}

#[inline]
unsafe fn backward_unchecked(start: Self, n: usize) -> Self {
// SAFETY: the caller has to guarantee that `start - n` doesn't overflow or hit zero.
unsafe { Self::new_unchecked(start.get().unchecked_sub(n as $int)) }
}

#[inline]
#[allow(arithmetic_overflow)]
#[rustc_inherit_overflow_checks]
fn forward(start: Self, n: usize) -> Self {
// In debug builds, trigger a panic on overflow.
// This should optimize completely out in release builds.
if Self::forward_checked(start, n).is_none() {
let _ = $int::MAX + 1;
}
// Do saturating math (wrapping math causes UB if it wraps to Zero)
start.saturating_add(n as $int)
}

#[inline]
#[allow(arithmetic_overflow)]
#[rustc_inherit_overflow_checks]
fn backward(start: Self, n: usize) -> Self {
// In debug builds, trigger a panic on overflow.
// This should optimize completely out in release builds.
if Self::backward_checked(start, n).is_none() {
let _ = $int::MIN - 1;
}
// Do saturating math (wrapping math causes UB if it wraps to Zero)
Self::new(start.get().saturating_sub(n as $int)).unwrap_or(Self::MIN)
}
};
}

macro_rules! step_nonzero_impls {
{
narrower than or same width as usize:
$( $narrower:ident ),+;
wider than usize:
$( $wider:ident ),+;
} => {
$(
#[allow(unreachable_patterns)]
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for NonZero<$narrower> {
step_nonzero_identical_methods!($narrower);

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
if *start <= *end {
// This relies on $u_narrower <= usize
Some((end.get() - start.get()) as usize)
} else {
None
}
}

#[inline]
fn forward_checked(start: Self, n: usize) -> Option<Self> {
match $narrower::try_from(n) {
Ok(n) => start.checked_add(n),
Err(_) => None, // if n is out of range, `unsigned_start + n` is too
}
}

#[inline]
fn backward_checked(start: Self, n: usize) -> Option<Self> {
match $narrower::try_from(n) {
// *_sub() is not implemented on NonZero<T>
Ok(n) => start.get().checked_sub(n).and_then(Self::new),
Err(_) => None, // if n is out of range, `unsigned_start - n` is too
}
}
}
)+

$(
#[allow(unreachable_patterns)]
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for NonZero<$wider> {
step_nonzero_identical_methods!($wider);

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
if *start <= *end {
usize::try_from(end.get() - start.get()).ok()
} else {
None
}
}

#[inline]
fn forward_checked(start: Self, n: usize) -> Option<Self> {
start.checked_add(n as $wider)
}

#[inline]
fn backward_checked(start: Self, n: usize) -> Option<Self> {
start.get().checked_sub(n as $wider).and_then(Self::new)
}
}
)+
};
}

#[cfg(target_pointer_width = "64")]
step_nonzero_impls! {
narrower than or same width as usize: u8, u16, u32, u64, usize;
wider than usize: u128;
}

#[cfg(target_pointer_width = "32")]
step_nonzero_impls! {
narrower than or same width as usize: u8, u16, u32, usize;
wider than usize: u64, u128;
}

#[cfg(target_pointer_width = "16")]
step_nonzero_impls! {
narrower than or same width as usize: u8, u16, usize;
wider than usize: u32, u64, u128;
}

#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for char {
#[inline]
Expand Down Expand Up @@ -923,6 +1056,7 @@ impl<A: Step> Iterator for ops::Range<A> {
range_exact_iter_impl! {
usize u8 u16
isize i8 i16
NonZero<usize> NonZero<u8> NonZero<u16>

// These are incorrect per the reasoning above,
// but removing them would be a breaking change as they were stabilized in Rust 1.0.0.
Expand All @@ -932,25 +1066,35 @@ range_exact_iter_impl! {
i32
}

#[cfg(target_pointer_width = "32")]
range_exact_iter_impl! { NonZero<u32> }

#[cfg(target_pointer_width = "64")]
range_exact_iter_impl! { NonZero<u64> }

unsafe_range_trusted_random_access_impl! {
usize u8 u16
isize i8 i16
NonZero<usize> NonZero<u8> NonZero<u16>
}

#[cfg(target_pointer_width = "32")]
unsafe_range_trusted_random_access_impl! {
u32 i32
NonZero<u32>
}

#[cfg(target_pointer_width = "64")]
unsafe_range_trusted_random_access_impl! {
u32 i32
u64 i64
NonZero<u32> NonZero<u64>
}

range_incl_exact_iter_impl! {
u8
i8
NonZero<u8>

// These are incorrect per the reasoning above,
// but removing them would be a breaking change as they were stabilized in Rust 1.26.0.
Expand All @@ -960,6 +1104,12 @@ range_incl_exact_iter_impl! {
i16
}

#[cfg(target_pointer_width = "32")]
Copy link
Member

Choose a reason for hiding this comment

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

do we want these ExactSizeIterator impls to be dependent on pointer size? the impls for primitive integer types don't depend on pointer size, presumably to help you write more portable code that doesn't break on 32 or 16-bit platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a known bug, if they don't depend on the pointer width (size of usize) they can't be ExactSizeIterators (for anything >u16).

(0..=u32::MAX).len() causes an overflow on usize == u16. If you prefer code not to break on usize == u16 platforms, then I would prefer to remove the impls over removing the constraint.

Copy link
Member

@programmerjake programmerjake Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, i'm proposing implementing ExactSizeIterator as follows,
with no #[cfg] attributes (edit: i forgot RangeInclusive still can't have len == 2^16 because NonZero excludes one value):

T Range<NonZero<T>> RangeInclusive<NonZero<T>>
u8 Yes Yes
u16 Yes Yes
u32 No No
u64 No No
u128 No No
usize Yes Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to address this, my preference is implementing it as it currently stands, and this concern was raised after the FCP. Would the changes need a new FCP? Am I allowed to edit the PR after the reviews?

range_incl_exact_iter_impl! { NonZero<u16> }

#[cfg(target_pointer_width = "64")]
range_incl_exact_iter_impl! { NonZero<u32> }

#[stable(feature = "rust1", since = "1.0.0")]
impl<A: Step> DoubleEndedIterator for ops::Range<A> {
#[inline]
Expand Down
67 changes: 67 additions & 0 deletions library/core/tests/iter/range.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,3 +501,70 @@ fn test_double_ended_range() {
panic!("unreachable");
}
}

macro_rules! nz {
($type:ident($val:literal)) => {
$type::new($val).unwrap()
};
}

macro_rules! nonzero_array {
($type:ident[$($val:literal),*]) => {
[$(nz!($type($val))),*]
}
}

macro_rules! nonzero_range {
($type:ident($($left:literal)?..$($right:literal)?)) => {
nz!($type($($left)?))..nz!($type($($right)?))
};
($type:ident($($left:literal)?..=$right:literal)) => {
nz!($type($($left)?))..=nz!($type($right))
};
}

#[test]
fn test_nonzero_range() {
#![allow(deprecated)]
use core::num::{NonZeroU32, NonZeroU8, NonZeroUsize};

assert_eq!(
nonzero_range!(NonZeroUsize(1..=21)).step_by(5).collect::<Vec<_>>(),
nonzero_array!(NonZeroUsize[1, 6, 11, 16, 21])
);
assert_eq!(
nonzero_range!(NonZeroUsize(1..=20)).step_by(5).collect::<Vec<_>>(),
nonzero_array!(NonZeroUsize[1, 6, 11, 16])
);
assert_eq!(
nonzero_range!(NonZeroUsize(1..20)).step_by(5).collect::<Vec<_>>(),
nonzero_array!(NonZeroUsize[1, 6, 11, 16])
);
assert_eq!(
nonzero_range!(NonZeroUsize(1..21)).rev().step_by(5).collect::<Vec<_>>(),
nonzero_array!(NonZeroUsize[20, 15, 10, 5])
);
assert_eq!(
nonzero_range!(NonZeroUsize(1..21)).rev().step_by(6).collect::<Vec<_>>(),
nonzero_array!(NonZeroUsize[20, 14, 8, 2])
);
assert_eq!(
nonzero_range!(NonZeroU8(200..255)).step_by(50).collect::<Vec<_>>(),
nonzero_array!(NonZeroU8[200, 250])
);
assert_eq!(
nonzero_range!(NonZeroUsize(200..5)).step_by(1).collect::<Vec<_>>(),
nonzero_array!(NonZeroUsize[])
);
assert_eq!(
nonzero_range!(NonZeroUsize(200..200)).step_by(1).collect::<Vec<_>>(),
nonzero_array!(NonZeroUsize[])
);

assert_eq!(nonzero_range!(NonZeroU32(10..20)).step_by(1).size_hint(), (10, Some(10)));
assert_eq!(nonzero_range!(NonZeroU32(10..20)).step_by(5).size_hint(), (2, Some(2)));
assert_eq!(nonzero_range!(NonZeroU32(1..21)).rev().step_by(5).size_hint(), (4, Some(4)));
assert_eq!(nonzero_range!(NonZeroU32(1..21)).rev().step_by(6).size_hint(), (4, Some(4)));
assert_eq!(nonzero_range!(NonZeroU32(20..1)).step_by(1).size_hint(), (0, Some(0)));
assert_eq!(nonzero_range!(NonZeroU32(20..20)).step_by(1).size_hint(), (0, Some(0)));
}
24 changes: 12 additions & 12 deletions tests/ui/impl-trait/impl_trait_projections.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ LL | -> <::std::ops::Range<impl Debug> as Iterator>::Item
Char
Ipv4Addr
Ipv6Addr
char
i128
i16
i32
i64
and 8 others
NonZero<u128>
NonZero<u16>
NonZero<u32>
NonZero<u64>
NonZero<u8>
and 14 others
= note: required for `std::ops::Range<impl Debug>` to implement `Iterator`

error[E0277]: the trait bound `impl Debug: Step` is not satisfied
Expand All @@ -59,12 +59,12 @@ LL | | }
Char
Ipv4Addr
Ipv6Addr
char
i128
i16
i32
i64
and 8 others
NonZero<u128>
NonZero<u16>
NonZero<u32>
NonZero<u64>
NonZero<u8>
and 14 others
= note: required for `std::ops::Range<impl Debug>` to implement `Iterator`

error: aborting due to 7 previous errors
Expand Down
12 changes: 6 additions & 6 deletions tests/ui/range/range-1.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ LL | for i in false..true {}
Char
Ipv4Addr
Ipv6Addr
char
i128
i16
i32
i64
and 8 others
NonZero<u128>
NonZero<u16>
NonZero<u32>
NonZero<u64>
NonZero<u8>
and 14 others
= note: required for `std::ops::Range<bool>` to implement `Iterator`
= note: required for `std::ops::Range<bool>` to implement `IntoIterator`

Expand Down
Loading