Rust: Tracking issue for `start`, `end` and `new` methods of RangeInclusive

[kennytm]

Tracking issue for the start, end and new methods of RangeInclusive.

(Originally the tracking issue for rust-lang/rfcs#1980 the start and end fields, but we decided to make the representation private in https://github.com/rust-lang/rust/issues/49022#issuecomment-377533282)

Steps:

• [x] Implementation #49724
• [ ] Stabilization PR https://github.com/rust-lang/rust/pull/50758

Unresolved questions:

• [x] Should we want to expose the fields as public (stabilize as is), or simply provide getter methods and don't show the implementation detail (redesign from scratch)?

This issue is separated from #28237 since there were vocal oppositions from the community (see #47813) due to performance loss if we commit to the two-field design.

If you want start and end, please use these for now:

• x.start() ↦ x.next().unwrap()
• x.end() ↦ x.next_back().unwrap()

[scottmcm]

Two important points here:

• There's no performance loss if you use internal iteration (fold & friends), just like other types (such as Chain) that can be faster if you use internal iteration instead of a for loop.
• There _is_ core additional complexity to iterating RangeInclusive compared to a Range, since the loop body fundamentally must do something different in the last iteration in the interesting case

As I've said before, the optimal choice here would be to only have the range types be IntoIterator (not Iterator), but that change requires a time machine, so I think being like Range is best.

[SimonSapin]

The libs team discussed this and the consensus was to make all fields private, provide accessor methods, and add a boolean field to implement iteration in a way that hopefully optimizes better.

impl<Idx> RangeInclusive<Idx> {
    pub fn start(&self) -> &Idx {…}
    pub fn end(&self) -> &Idx {…}
}

If someone has a use case for moving bounds out, they should request something like pub fn into_inner(self) -> (Idx, Idx) to be added. The methods of the RangeBounds trait (formerly RangeArgument) are to be renamed start_bound and end_bound to avoid name collision.

@rfcbot fcp merge

[rfcbot]

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

• [x] @Kimundi
• [x] @SimonSapin
• [x] @alexcrichton
• [x] @aturon
• [x] @dtolnay
• [ ] @sfackler
• [ ] @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

[scottmcm]

What exact this this FCPing? It feels odd that the last two times the class was modified it was via RFC, but this time it's just a "hopefully", not even a PR.

[SimonSapin]

This is not making again deep changes to the data structure or syntax, only picking one of two already-proposed alternatives. And the more conservative one, since methods can keep working even if the internal structure changes.

[kennytm]

Simply making the fields private will break lowering of ..= 😒

error[E0451]: field `start` of struct `std::ops::RangeInclusive` is private
   --> librustc/ty/layout.rs:646:34
    |
646 |             self.valid_range == (0..=1)
    |                                  ^ field `start` is private

error[E0451]: field `end` of struct `std::ops::RangeInclusive` is private
   --> librustc/ty/layout.rs:646:38
    |
646 |             self.valid_range == (0..=1)
    |                                      ^ field `end` is private

Therefore x..=y will need to lower to something like RangeInclusive::new(x, y). Should the constructor be exposed or #[doc(hidden)]? Or we keep the fields public and perma-unstable them?

[SimonSapin]

I’d be fine with #[doc(hidden)] perma-unstable public fields, but we might want an constructor function anyway if the fields are "effectively private". Is lowering to a call difficult or inconvenient?

[kennytm]

It needs a special case in https://github.com/rust-lang/rust/blob/56714acc5eb0687ed9a7566fdebe5528657fc5b/src/librustc/hir/lowering.rs#L3082 for the ExprKind::Range(Some(..), Some(..), Closed) case. Not very difficult, just different.

[scottmcm]

I'm not sure that a doc(hidden) perma-unstable function is better than doc(hidden) perma-unstable fields.

[SimonSapin]

Why not stabilize that function? Other range types can be created as struct literals in addition to the dedicated syntax.

[sfackler]

I agree with @SimonSapin, it seems reasonable to have a stable constructor for the type.

[crlf0710]

While i think those methods are fine, i believe (at least) the constructor should be const fn...

[scottmcm]

What advantage does the RangeInclusive::new have over just using ..=? And if there is something, wouldn't it also apply to having a Range::new in addition to ..? (I don't consider the compiler initializing unstable fields to be a problem.)

[kennytm]

@scottmcm There is no advantage using the method.

..= and .. are simply syntactic sugar, a..b will be lowered as Range { start: a, end: b } before type-checking. The stability of the fields are irrelevant; the problem with ..= is that the compiler cannot initialize private fields (unless we use some hygiene hack to put the span inside std::ops which I think is even worse). Therefore a public method RangeInclusive::new() needs to be introduced as the lowering of a..=b.

The question is only whether we want to eventually stabilize RangeInclusive::new(), or keep it #[doc(hidden)] #[unstable] forever.

[scottmcm]

@kennytm Fair enough, makes sense if the goal is private fields.

A thought, since I don't know what was discussed: could the struct be #[non_exhaustive] so start and end are pub (and thus matchable, move-from-able, etc), but the struct can still be changed if needed? (That of course still needs the constructor, but might avoid future asks for into_inner, start_mut, etc.)

[kennytm]

@scottmcm This will allow mutating start and end without touching the hidden field, potentially breaking some invariants for RangeInclusive.

[rfcbot]

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

[rfcbot]

The final comment period is now complete.

[ghost]

This makes it very difficult/impossible when start and end are non-Copy types.
(It has broken my math_traits crate, which relied on destructing RangeInclusive.)

If someone has a use case for moving bounds out, they should request something like pub fn into_inner(self) -> (Idx, Idx) to be added.

I hereby request pub fn into_inner(self) -> (Idx, Idx).

[SimonSapin]

Adding into_inner sounds good to me. Wanna send a PR?

[ghost]

@SimonSapin done! https://github.com/rust-lang/rust/pull/50574