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

[jalil-salame]

Implement Step for NonZero unsigned integers as discussed in libs-team#130.

step_nonzero_impls was adapted from step_integer_impls and the tests were adapted from the step tests.

It seems to compile and pass the tests c:

I would like to test the edge cases, specially around overflows. To ensure safe code cannot generate a 0 NonZero, but I don't know how to go about it. I would love some feedback on that (and the PR overall).

The impls are tagges with #[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]. I assumed this was appropriate, but I am not sure.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[jalil-salame]

UI tests are failing because the output of diagnostics was reorganized now that NonZero<u*> implement Step. A preliminary look at the contribution docs (https://rustc-dev-guide.rust-lang.org/tests/ui.html#ui-tests) does not show how to update the test output.

Its getting late so I'll take a look at it tomorrow and see if I find a way to update the ui tests.

[cuviper]

You can use ./x.py test --bless ui to update the expected output.

I know you already went through an ACP, but I think this still needs a libs-api reviewer since it is instantly stable -- Step is unstable, but it expands the stable impl<A> Iterator for Range<A> etc.

@rustbot label -T-libs +T-libs-api
r? libs-api

[the8472]

Regarding the stable API surface: We should also add the appropriate ExactSizeIterator impls, without repeating the 16-bit-platform mistakes.

[jalil-salame]

I'm having issues with the ui tests due to NixOS not having the library headers for rust-LLD in the right place. I'll see if I can fix later today. Apparently the failing tests were not preventing me from updating the ui tests 🤦

[jalil-salame]

Regarding the stable API surface: We should also add the appropriate ExactSizeIterator impls, without repeating the 16-bit-platform mistakes.

I see that TrustedRandomAccess is cfg gated based on the pointer width but that is not the case for ExactSizeIterator for Range and RangeInclusive. See here. I assume that is because you don't want to implement a non-marker trait on types conditionally.

[jalil-salame]

@joshtriplett friendly ping. Should I assign someone else if you are too busy?

[joshtriplett]

r? libs-api

[Amanieu]

@rfcbot fcp merge

[rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[leonardo-m]

See also #73121

[leonardo-m]

do you want to reword your issue accordingly, or close it with this PR and create a new issue regarding NonZero* construction?

For now I've reworded and renamed that ER issue. But I'm now less sure it's a useful enough thing to have.

[jalil-salame]

do you want to reword your issue accordingly, or close it with this PR and create a new issue regarding NonZero* construction?

For now I've reworded and renamed that ER issue. But I'm now less sure it's a useful enough thing to have.

Having the issue there is nice though, it might prompt some further discussion in the future, although NonZeroUsize::new(1).unwrap() should be so simple to optimize that I'd file a rustc/llvm bug if the panic is kept.

[leonardo-m]

although NonZeroUsize::new(1).unwrap() should be so simple to optimize that I'd file a rustc/llvm bug if the panic is kept.

My point isn't to remove the panic from the binary, but to remove it from the source code. I try to minimize the number of visible unwraps in my Rust modules.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[programmerjake]

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.

[jalil-salame]

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.

[programmerjake]

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

[jalil-salame]

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?