Contract breaking impl From<SizeRange> for Range<usize>

[Centril]

A while back I added the implementation:

/// Given a size range `[low, high]`, then a range`low..(high + 1)` is returned.
/// This will panic if `high == usize::MAX`.
impl From<SizeRange> for Range<usize> {
    fn from(sr: SizeRange) -> Self {
        let (start, end) = sr.extract();
        start..end + 1
    }
}

however, the trait From states that:

Note: this trait must not fail.

Therefore, the implementation above is contract breaking since it may panic in fringe circumstances.
As such, the implementation should be removed. This amounts to a breaking change (so we should probably label the issue as such...)

[AltSysrq]

I doubt anyone is actually using this or will do so in the near future since proptest doesn't ever return this type on its own, so it would require someone else to decide to use SizeRange themselves and then go back into a range. I agree it should be removed eventually, but I'm inclined to wait for the next breaking changes train.

[Centril]

I agree it should be removed eventually, but I'm inclined to wait for the next breaking changes train.

Yeah; I agree with everything you said and this :)

I would suggest that we start with a more elaborate labeling system in this repo with milestones and more labels, which will be more important as proptest grows in importance in the ecosystem (which I'm hoping it will, because I think it is the state of the art wrt. Rust and testing). Something like:

• E-easy
• E-medium
• E-hard
• E-help-wanted
• P-high
• P-low
• breaking-change

and so on...

Here's some inspiration:

• https://github.com/dtolnay/syn/labels
• https://github.com/rust-random/rand/labels
• https://github.com/SergioBenitez/Rocket/labels

[Eh2406]

I just hit a related case, subsequence(s, 0) panics subtracted with overflow do to:

/// Given `low .. high`, then a size range `[low, high)` is the result.
impl From<Range<usize>> for SizeRange {
    fn from(r: Range<usize>) -> Self { size_range(r.start..=r.end - 1) }
}

(note that the 0 is generated from a separate part of the strategy, not a hardcoded value I could easily change.)

[Centril]

Seems I implemented that incorrectly; should be:

/// Given `low .. high`, then a size range `[low, high)` is the result.
impl From<Range<usize>> for SizeRange {
    fn from(r: Range<usize>) -> Self {
        let end = if r.end == 0 { 0 } else { r.end - 1 };
        size_range(r.start..=end)
    }
}

This can only occur if you provide forall x: usize. (x..0).

EDIT: actually... probably not; this would change the semantics given 0..0.
You can't translate that to 0..=0 which would happen if we changed the implementation to that.

EDIT: To actually represent the concept of an empty range in RangeInclusive I think you'd need to define this as (x+1)..x. Maybe SizeRange should be defined as struct SizeRange(Option<RangeInclusive<usize>>) instead...?

EDIT: but if you do that... you can't convert SizeRange to RangeInclusive<usize> anymore... sigh :/

[Eh2406]

Or you can use saturating_sub, in fact that could be a good mitigation or this entire thing.

[Centril]

@Eh2406 if you do r.start..=r.end.saturating_sub(1) then you may still reach cases like 0..0 becoming 0..=0 which is now 1 element more.

[AltSysrq]

I just hit a related case

Fixed in 0.8.6, along with some other issues with subsequence.

The original issue this report is about no longer happens as of 0.8.6 since Range is now used as the internal representation (in exchange for simply not allowing usize::MAX to be an included value at all).

[AltSysrq]

(I'm going to keep this open for the moment since there are some other unaddressed talking points still.)

[AltSysrq]

Removed the impls in question in 0.9.0.