Implement RangeArgument for RangeInclusive and RangeInclusiveTo

[SimonSapin]

This preserves the semantics of RangeArgument::end being exclusive, and converts inclusive ranges to exclusive ones the same way impl<T> ops::Index<ops::RangeInclusive<usize>> for [T] (7eb7c56) does: add one to the end bound and panic on overflow.

This required one change and one limitation:

• Since the result of that addition is not stored somewhere we can reference, RangeArgument::end (and RangeArgument::start for consistency) was changed to return a copy rather than a reference. This effectively removes some impls where T: !Copy.
• Since there is no trait with the checked_add method, RangeArgument is implemented for inclusive ranges of each primitive integer types rather than generically.

I think both these points are OK since the only usage of RangeArgument in std is with ranges of usize.

If this change is accepted I’d like to nominate RangeArgument / #30877 for FCP and stabilization. I believe this can happen even if the inclusive range syntax and types are still unstable.

For what it’s worth, I just copied RangeArgument into rust-url to be able to use it there.

CC @alexcrichton & libs team

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[durka]

The message is wrong, isn't it? When this is expanded for u8, (0...255).end() will panic here. Same for the one above.

[durka]

This is the best solution without changing the semantics of RangeArgument, but I'm not sure that's the best approach. It feels gross that you can't really use RangeArgument for a range that goes to the rails (like 0...255), which was the main motivation for implementing inclusive ranges. IMO it would be better to change RangeArgument to return an enum like enum Bounded<T> { Inclusive(&T), Exclusive(&T) } (maybe clones instead of references, haven't thought it through) at both ends.

[SimonSapin]

RangeArgument is only used in std with ranges of usize, so this would be 0...4294967295 or 0...18446744073709551615 rather than 0..255. We could change RangeArgument::end (I don’t think changing start is useful) to return an enum as you suggest, but that would only push the panic to each caller (namely Vec::drain, String::drain, and VecDeque::drain) since in-memory collections can not have an item at index usize::MAX (this would make the length >= usize::MAX + 1).

(For what it’s worth, my opinion is that inclusive ranges are not useful for slicing, only as iterators. I’m only making this PR to push for stabilizing RangeArgument.)

[durka]

Hmm well it's easy enough to fix the message to concat!("inclusive range to maximum ", stringify!($Int)) at least.

I see your point but I'm not sure I like privileging usize. And I don't like adding more panics to the standard library. But there's no reason to listen to me! I also understand the desire for stabilization, of course, but it may be premature since I think that range syntax is going to become more flexible (@aturon keeps reminding me to write that RFC...).

[bluss]

This design makes inclusive ranges extra panicky. with extra branches. We can use a different design to only panic once (out of bounds). I suggested something like that with the IntoCheckedRange design.

Fewer branches and more tailored bounds checking code makes the bounds checks easier to optimize out in more cases.

[alexcrichton]

This is an interesting change to see to RangeArgument which has otherwise been relatively stable, and this should also be carefully considered as new implementors of that trait will increase the stable surface area of the standard library. Fun things to consider!

cc @rust-lang/libs

[alexcrichton]

The libs team discussed this during triage yesterday but we unfortunately didn't reach a firm conclusion. There was some sentiment that if we're going to make the RangeArgument trait more generic we should go "all the way" with perhaps a custom enum type (as I believe has been proposed in previous RFCs), but that's a somewhat large change to these APIs from what it currently is.

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with comments addressed!

[SimonSapin]

It’s not clear how I can address "we unfortunately didn't reach a firm conclusion", but oh well.

For what it’s worth I don’t care much about inclusive ranges for slicing (as opposed to as iterators), I was only hoping to nudge RangeArgument toward stabilization.