Stabilize the inclusive_range lib feature

[est31]

Stabilizes the lib feature for inclusive ranges,
not the syntax yet as its still being discussed.

Proposed by a comment by @retep998 in the tracking issue #28237.

As beta will likely branch sometime next week and this PR likely requires an FCP, it won't make it into Rust 1.20 any more. That's why I've used 1.21 as the version everywhere.

As for the documentation, I think the documentation only needs updating for the range_inclusive_syntax feature, but if anyone from the docs team thinks otherwise, please say so! (cc @GuillaumeGomez @steveklabnik et al)

r? @nikomatsakis

[kennytm]

According to the prediction thread 1.20 branches on July 20th (two weeks from now), so it should still have time making into the next beta.

[est31]

@kennytm from what I can see is that 1.19 will be released on Jul 21, and beta always branches one week earlier. Maybe its different this time though, no idea. @brson @alexcrichton when will the next beta branch?

[durka]

May want to have a run-pass test exercising all the structs, just to be sure.

[kennytm]

@est31 The current beta was branched on Jun 6th (#42472), and stable was "branched" on the same day (#42455). The prediction was Jun 8th, so I think the prediction is accurate (up to ±2 days).

[GuillaumeGomez]

Just an update of the docs should be fine I guess.

[est31]

@durka I'm not sure whether that's needed. If you have an unstable attribute and somewhere else a stable attribute with the same name, tidy will throw an error.

[nikomatsakis]

Since this is purely concerned with the library side of things, I'm going to move responsibility for this PR over to the libs team. To that end:

r? @alexcrichton

I would suggest that you the libs team do an FCP, however. =)

[alexcrichton]

@est31 can you elaborate on the motivation to do this? It to me naively seems like it's premature to stabilize the structs and not the syntax?

[est31]

Despite the massive bikeshed that is going on in the tracking issue about which syntax to chose for inclusive ranges (I plead guilty of having participated in it for a short time as well :P), there seems to be overwhelming agreement that inclusive ranges are wanted. As such, I think the library component can be stabilized. The functionality has been furthermore through the RFC process and accepted.

I guess the lib feature I propose stabilizing will even be useful for a group of people: library authors who wish to provide compatibility with inclusive ranges but who don't want to require latest versions of Rust. When a Rust with stable inclusive range support ships they can already present their releases which are compatible, while older users without inclusive ranges are still supported.

Of course, the use case I mentioned is pretty niche and I don't know of anyone with such use cases in mind. I don't feel very strongly about this PR, still it would be nice to have IMO.

[durka]

I'll note that despite said massive bikeshed, the lang team did announce a decision. I've started to work on a syntax PR but I'm not sure when I'll be done.

…

On Fri, Jul 7, 2017 at 8:50 PM, est31 ***@***.***> wrote: Despite the massive bikeshed that is going on in the tracking issue about which syntax to chose for inclusive ranges (I plead guilty of having participated in it for a short time as well :P), there seems to be overwhelming agreement that inclusive ranges are wanted. As such, I think the library component can be stabilized. The functionality has been furthermore through the RFC process and accepted. I guess the lib feature I propose stabilizing will even be useful for a group of people: library authors who wish to provide compatibility with inclusive ranges but who don't want to require latest versions of Rust. When a Rust with stable inclusive range support ships they can already present their releases which are compatible, while older users without inclusive ranges are still supported. Of course, the use case I mentioned is pretty niche and I don't know of anyone with such use cases in mind. I don't feel very strongly about this PR, still it would be nice to have IMO. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#43086 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n9AQjOC3niTSkG84feYq7Q5XYYRwks5sLtJBgaJpZM4OPnaX> .

[bors]

☔ The latest upstream changes (presumably #43077) made this pull request unmergeable. Please resolve the merge conflicts.

[SimonSapin]

Before this is stabilized I think the impls of ExactSizeIterator for RangeInclusive<u16> and RangeInclusive<i16> should be removed. The reason is a combination of:

• Per its documentation ExactSizeIterator::len must always return an accurate usize. This implies that it must not be implemented for iterators that can be longer than usize::MAX.
• The "length" of inclusive ranges of integers is one more than the differences between their bounds. (0_u8..255_u8).len() is 256, which overflows u8. So it is only correct to implement ExactSizeIterator for RangeInclusive of an integer type if that width of that integer type is strictly narrower than usize.
• It’s bad for portability to have APIs (including impls) that only exist on some platforms. (For stuff that is necessarily platform-specific, we have std::os::* modules that make this explicit.) So we should only have impl that make sense on every supported platform. This is why src/libcore/iter/range.rs currently has range_incl_exact_iter_impl!(u8 u16 i8 i16); which already excludes u32 and i32.
• As far as I can tell, all platforms that are currently officially supported have pointers (and usize) of at least 32 bits. However AVR support is actively being worked on at https://github.com/avr-rust/rust, and MSP430 support only requires a custom target specification file: https://forge.rust-lang.org/platform-support.html#tier-3. These are both 16-bit.

If someone feels like arguing for intentional non-support of 16-bit platforms in libcore, I think that’s out of scope for this PR and should be decided separately. In the meantime, impls that we might want to remove should not be stabilized.

By the way, the same reasoning (except "plus one") applies to Range<_> so ExactSizeIterator is only correct for integer types narrower or same width as usize. Unfortunately the impls for Range<u32> and Range<i32> were stable in Rust 1.0.0, removing them would be a breaking change. (0..66_000_u32).len() for example would compile and panic on 16-bit platforms.

[durka]

Relevant: rust-lang/rfcs#1748

…

On Sat, Jul 8, 2017 at 2:20 PM, Simon Sapin ***@***.***> wrote: Before this is stabilized I thin the impls of ExactSizeIterator for RangeInclusive<u16> and RangeInclusive<i16> should be removed. The reason is a combination of: - Per its documentation ExactSizeIterator::len must always return an accurate usize. This implies that it must not be implemented for iterators that can be longer than usize::MAX. - The "length" of inclusive ranges of integers is one more than the differences between their bounds. (0_u8..255_u8).len() is 256, which overflows u8. So it is only correct to implement ExactSizeIterator for RangeInclusive of an integer type if that width of that integer type is *strictly narrower* than usize. - It’s bad for portability to have APIs (including impls) that only exist on some platforms. (For stuff that is necessarily platform-specific, we have std::os::* modules that make this explicit.) So we should only have impl that make sense on every supported platform. This is why src/libcore/iter/range.rs currently has range_incl_exact_iter_impl!(u8 u16 i8 i16); which already excludes u32 and i32. - As far as I can tell, all platforms that are currently officially supported have pointers (and usize) of at least 32 bits. However AVR support is actively being worked on at https://github.com/avr-rust/rust, and MSP430 support only requires a custom target specification file: https://forge.rust-lang.org/platform-support.html#tier-3 <https://forge.rust-lang.org/platform-support.html#tier-3>. These are both 16-bit. If someone feels like arguing for intentional non-support of 16-bit platforms in libcore, I think that’s out of scope for this PR and should be decided separately. In the meantime, impls that we might want to remove should not be stabilized. By the way, the same reasoning (except "plus one") applies to Range<_> so ExactSizeIterator is only correct for integer types *narrower or same width as* usize. Unfortunately the impls for Range<u32> and Range<i32> were stable in Rust 1.0.0, removing them would be a breaking change. (0..66_000_u32).len() for example would compile and panic on 16-bit platforms. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#43086 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n2MRnfyd5fADHktVJvFD0Qy4zSkeks5sL8hjgaJpZM4OPnaX> .

[durka]

So basically the lang team says include the impls and then write a lint that detects usage of them and warns that you aren't being 16-bit clean. But leaving them out for now (since no such lint exists yet) is the conservative solution as you say.

…

On Sat, Jul 8, 2017 at 2:27 PM, Alex Burka ***@***.***> wrote: Relevant: rust-lang/rfcs#1748 On Sat, Jul 8, 2017 at 2:20 PM, Simon Sapin ***@***.***> wrote: > Before this is stabilized I thin the impls of ExactSizeIterator for > RangeInclusive<u16> and RangeInclusive<i16> should be removed. The > reason is a combination of: > > - Per its documentation ExactSizeIterator::len must always return an > accurate usize. This implies that it must not be implemented for > iterators that can be longer than usize::MAX. > - The "length" of inclusive ranges of integers is one more than the > differences between their bounds. (0_u8..255_u8).len() is 256, which > overflows u8. So it is only correct to implement ExactSizeIterator > for RangeInclusive of an integer type if that width of that integer > type is *strictly narrower* than usize. > - It’s bad for portability to have APIs (including impls) that only > exist on some platforms. (For stuff that is necessarily platform-specific, > we have std::os::* modules that make this explicit.) So we should > only have impl that make sense on every supported platform. This is > why src/libcore/iter/range.rs currently has range_incl_exact_iter_impl!(u8 > u16 i8 i16); which already excludes u32 and i32. > - As far as I can tell, all platforms that are currently officially > supported have pointers (and usize) of at least 32 bits. However AVR > support is actively being worked on at https://github.com/avr-rust/ru > st, and MSP430 support only requires a custom target specification > file: https://forge.rust-lang.org/platform-support.html#tier-3. These > are both 16-bit. > > If someone feels like arguing for intentional non-support of 16-bit > platforms in libcore, I think that’s out of scope for this PR and should be > decided separately. In the meantime, impls that we might want to remove > should not be stabilized. > > By the way, the same reasoning (except "plus one") applies to Range<_> > so ExactSizeIterator is only correct for integer types *narrower or same > width as* usize. Unfortunately the impls for Range<u32> and Range<i32> > were stable in Rust 1.0.0, removing them would be a breaking change. > (0..66_000_u32).len() for example would compile and panic on 16-bit > platforms. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#43086 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAC3n2MRnfyd5fADHktVJvFD0Qy4zSkeks5sL8hjgaJpZM4OPnaX> > . >

[kennytm]

If RFC 1868 is implemented, should these be allowed?

// ok?
#[cfg(any(target_pointer_width="32", target_pointer_width="64"))]
impl ExactSizeIterator for RangeInclusive<u16> { ... }

// but this maybe bad idea
#[cfg(target_pointer_width="64")]
impl ExactSizeIterator for RangeInclusive<u32> { ... }

[SimonSapin]

@kennytm I think that’s debatable. I’ve added a reminder in the tracking issue #41619.

[oyvindln]

The "length" of inclusive ranges of integers is one more than the differences between their bounds. (0_u8..255_u8).len() is 256, which overflows u8. So it is only correct to implement ExactSizeIterator for RangeInclusive of an integer type if that width of that integer type is strictly narrower than usize.

According to #1748 it's not even clear that usize can be assumed to be larger than u8, let alone u16, so maybe that's something that has be sorted first.

[SimonSapin]

https://en.wikibooks.org/wiki/C_Programming/stdint.h#Integers_wide_enough_to_hold_pointers claims that uintptr_t is at least 16 bits.

[alexcrichton]

Thanks for the explanation @est31, but I feel like in that case this is a bit premature? You in theory can already do this in libraries with the already-stable Bound type. In that sense it seems that this isn't buying us any extra functionality but may end up tying our hands on last-minute details?

[est31]

Anyone else want to weigh in on why this should be merged? I personally want to merge it, but not very strongly as I won't use it or anything. And the functionality will be stabilized sooner or later anyway. If there is no further discussion I'd close the PR in 7 days due to being unwanted.

[aturon]

I tend to agree with @alexcrichton that we should land this together with the syntax, and should push forward on that front.

[scottmcm]

cc @clarcharr, who was also interested in stabilizing the struct (#28237 (comment))

[est31]

@clarcharr any arguments to convince @aturon and @alexcrichton to stabilize this?

[clarfonthey]

I mean, I personally was hoping to get traits into len-trait that require Index<RangeInclusive> but since then I've modified the traits to be forwards compatible without it.

Right now, I very much think that it's worthwhile to allow crate authors the ability to add support for inclusive ranges and test them if we're decided upon the struct itself. I feel like delaying the stabilisation until the syntax is done is just delaying talking about the struct for now, and at the very least we should talk about what needs to be done now.

[clarfonthey]

Plus I know for a fact that this will reduce the boilerplate of adding feature gates on crates to support inclusive ranges. Users can add their own inclusive(a..b) functions as a workaround until then.

Realistically, it'll be at least 1.22 when inclusive range syntax is merged, and getting this in before that would be nice because at least the implementations on the library side will be there.

[carols10cents]

friendly ping to keep this on your radar @est31, looks like there's some merge conflicts!

[est31]

@carols10cents I haven't forgotten about this PR. I was just waiting whether there would be more agreement to get this merged, but apparently there is none, so I'm closing.