Tracking issue for start, end and new methods of RangeInclusive

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 #49022 (comment))

Steps:

Implementation Introduce RangeInclusive::{new, start, end} methods and make the fields private. #49724
Stabilization PR Stabilise inclusive_range_methods #50758

Unresolved questions:

~~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()

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.

@rfcbot

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

@SimonSapin

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

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.

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.

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.

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?

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?

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.

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

The final comment period is now complete.

Auto merge of #49724 - kennytm:range-inc-start-end-methods, r=Kimundi

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

Rollup merge of rust-lang#50574 - s3bk:range_inclusive_into_inner, r=…

Auto merge of #50758 - varkor:stabilise-inclusive_range_methods, r=Si…

Navigation Menu

Tracking issue for `start`, `end` and `new` methods of RangeInclusive #49022

9 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Tracking issue for start, end and new methods of RangeInclusive #49022

Description

Activity

scottmcm commented on Mar 15, 2018

SimonSapin commented on Mar 30, 2018

rfcbot commented on Mar 30, 2018

scottmcm commented on Mar 31, 2018

SimonSapin commented on Mar 31, 2018

kennytm commented on Apr 5, 2018

SimonSapin commented on Apr 5, 2018

kennytm commented on Apr 5, 2018

9 remaining items

rfcbot commented on Apr 10, 2018

rfcbot commented on Apr 20, 2018

SimonSapin commented on May 9, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Tracking issue for `start`, `end` and `new` methods of RangeInclusive #49022