Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking Issue for new_range_api (part of RFC 3550) #125687

Open
5 of 12 tasks
pitaj opened this issue May 28, 2024 · 6 comments
Open
5 of 12 tasks

Tracking Issue for new_range_api (part of RFC 3550) #125687

pitaj opened this issue May 28, 2024 · 6 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@pitaj
Copy link
Contributor

pitaj commented May 28, 2024

Feature gate: #![feature(new_range_api)]

This is a tracking issue for the library additions that are part of RFC 3550: New range types

Tracking issue for the language change: #123741

This feature introduces new types implementing Copy and IntoIterator that are meant to replace the legacy range types, which are !Copy and implement Iterator directly.

Public API

// core::range

#[derive(Clone, Copy, Default, PartialEq, Eq, Hash)]
pub struct Range<Idx> {
    pub start: Idx,
    pub end: Idx,
}
#[derive(Clone, Copy, Default, PartialEq, Eq, Hash)]
pub struct RangeInclusive<Idx> {
    pub start: Idx,
    pub end: Idx,
}
#[derive(Clone, Copy, Default, PartialEq, Eq, Hash)]
pub struct RangeFrom<Idx> {
    pub start: Idx,
}

pub struct IterRange<Idx> { <private fields> }
pub struct IterRangeInclusive { <private fields> }
pub struct IterRangeFrom { <private fields> }

impl<Idx: Step> Iterator for IterRange<Idx> { ... }
impl<Idx: Step> Iterator for IterRangeInclusive<Idx> { ... }
impl<Idx: Step> Iterator for IterRangeFrom<Idx> { ... }

impl<Idx: Step> IntoIterator for Range<Idx> {
    type IntoIter = IterRange<Idx>;
    ...
}
impl<Idx: Step> IntoIterator for RangeInclusive<Idx> {
    type IntoIter = IterRangeInclusive<Idx>;
    ...
}
impl<Idx: Step> IntoIterator for RangeFrom<Idx> {
    type IntoIter = IterRangeFrom<Idx>;
    ...
}

See the RFC for more details.

Steps / History

Unresolved Questions

  • Figure out a way to have IterRangeFrom overflow panic only after yielding the maximum value, without impacting release mode
  • How can we / should we optimize IterRangeInclusive now that it is not bound by the API constraints of legacy RangeInclusive?
  • The set of inherent methods copied from Iterator present on the new range types (Decide which methods to add to new Range types #125589)
  • The exact module paths and type names
    • Should the new types live at std::ops::range:: instead?
    • IterRange, IterRangeInclusive or just Iter, IterInclusive? Or RangeIter, RangeInclusiveIter, ...?
  • Should other range-related items (like RangeBounds) also be moved under the range module?
  • Should RangeFrom even implement IntoIterator, or should it require an explicit .iter() call? Using it as an iterator can be a footgun, usually people want start..=MAX instead. Also, it is inconsistent with RangeTo, which doesn't implement IntoIterator either. But, it's extremely useful for it.zip(1..)
  • Should there be a way to get an iterator that modifies the range in place, rather than taking the range by value? That would allow things like range.by_ref().next().
  • Should there be an infallible conversion from legacy to new RangeInclusive?
impl<Idx> From<legacy::RangeInclusive<Idx>> for RangeInclusive<Idx> {
    // How do we handle the `exhausted` case?
}

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@pitaj pitaj added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels May 28, 2024
@joshtriplett
Copy link
Member

Capturing something from a verbal discussion:

In theory, since the new range types aren't iterators directly, and just get turned into iterators, we could automatically handle for i in 10..0 and do the expected thing. However, @eholk pointed out that while that'd be intuitive for integer literals, it'd be surprising for the general m..n to implicitly do reverse iteration. So, we shouldn't do this.

@joshtriplett
Copy link
Member

Should RangeFrom even implement IntoIterator, or should it require an explicit .iter() call? Using it as an iterator can be a footgun, usually people want start..=MAX instead. Also, it is inconsistent with RangeTo, which doesn't implement IntoIterator either. But, it's extremely useful for it.zip(1..).

We discussed this in today's @rust-lang/libs-api meeting, and our conclusion was: the usefulness of .zip(n..) is high enough that we should make this work. We should ensure that it yields values up to the maximum integer value, and then panics if asked for another value.

@Amanieu
Copy link
Member

Amanieu commented Jun 5, 2024

We discussed the unresolved questions in the libs-api meeting yesterday.

There was a lot of opposition to inherent methods, especially map and rev, because they would need to behave differently on a value type than on an iterator. map would be expected to work like Option::map and apply a function to the start and end bounds. rev would be expected to return a range (not an iterator).

However there was some support for adding an iter inherent method which would be a short-hand for .clone().into_iter(). We felt that the additional 5 characters of into_iter (especially the underscore) would make people's code too verbose for such a common operation.

  • The exact module paths and type names

    • Should the new types live at std::ops::range:: instead?
    • IterRange, IterRangeInclusive or just Iter, IterInclusive? Or RangeIter, RangeInclusiveIter, ...?
  • Should other range-related items (like RangeBounds) also be moved under the range module?

We were mostly happy with how things are currently set out in the PR.

  • Should RangeFrom even implement IntoIterator, or should it require an explicit .iter() call? Using it as an iterator can be a footgun, usually people want start..=MAX instead. Also, it is inconsistent with RangeTo, which doesn't implement IntoIterator either. But, it's extremely useful for it.zip(1..)

It should implement IntoIterator since, as mentioned, it is useful for things like it.zip(1..).

We also discussed the RangeFrom iterator: the implementation should add a hidden bool so that any overflow panics happen after returning the maximum integer value, not before. To avoid issues with LLVM's loop optimizations, we may want this bool to only be used in debug builds: in release builds it may acceptable to allow the iterator to wrap.

  • Should there be a way to get an iterator that modifies the range in place, rather than taking the range by value? That would allow things like range.by_ref().next().

This should be deferred for a future API proposal, only if it turns out there is a need for this.

  • Should there be an infallible conversion from legacy to new RangeInclusive?

Yes there should be. The case of converting an exhausted RangeInclusive iterator should be extremely rare in practice, so it's fine to panic in that case.

@pitaj
Copy link
Contributor Author

pitaj commented Jun 10, 2024

There was a lot of opposition to inherent methods, especially map and rev, because they would need to behave differently on a value type than on an iterator.

I understand the arguments for purity here, but the immense utility these offer should be taken into account. I'll remove them from the PR, but I think the question should be reconsidered before the edition change is stabilized.

Arguments for `map` and `rev`

I think range types are in a very unique position, here. map and rev are

  1. commonly used in the wild
  2. present in much documentation
  3. demonstrated in countless learning materials
  4. a useful entry into iterator chains

While shaving five characters and the shift necessary for the underscore is helpful, the remaining seven characters and two shifts for .iter() is still a burden and needless visual noise.

My position is that there is no other meaning we could assign to map that would be acceptable given the history of using it in this position.

map is expected to map the bounds, not the yielded elements

  1. It is already a widely used pattern
  2. Changing map to mean "map the bounds" would be a sizeable hazard, because it could silently change the meaning of for (1..5).map(|n| n*2) { ... } across an edition boundary.
  3. So, a function doing so would need to be named something like map_bounds anyways.
  4. Range is not like Option or Vec. It's not commonly used to hold values, it's purpose is to generate values.

rev is expected to return something like RevRange

IMO this argument is stronger, because for x in (1..5).rev() would still work. That covers one of the most common use-cases. However, I don't see much use for a RevRange type.

What would it implement besides IntoIterator?

  • not RangeBounds
  • probably not Index
  • could have is_empty and contains but those are direction-agnostic

It it only serves as an iterable, we may as well just use Rev<IterRange<_>> anyways.

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Jul 6, 2024
Add `new_range_api` for RFC 3550

Initial implementation for rust-lang#125687

This includes a `From<legacy::RangeInclusive> for RangeInclusive` impl for convenience, instead of the `TryFrom` impl from the RFC. Having `From` is highly convenient and the debug assert should find almost all misuses.

This includes re-exports of all existing `Range` types under `core::range`, plus the range-related traits (`RangeBounds`, `Step`, `OneSidedRange`) and the `Bound` enum.

Currently the iterators are just wrappers around the old range types.

Tracking issues:

- rust-lang#123741
- rust-lang#125687
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 6, 2024
Rollup merge of rust-lang#125751 - pitaj:new_range_api, r=jhpratt

Add `new_range_api` for RFC 3550

Initial implementation for rust-lang#125687

This includes a `From<legacy::RangeInclusive> for RangeInclusive` impl for convenience, instead of the `TryFrom` impl from the RFC. Having `From` is highly convenient and the debug assert should find almost all misuses.

This includes re-exports of all existing `Range` types under `core::range`, plus the range-related traits (`RangeBounds`, `Step`, `OneSidedRange`) and the `Bound` enum.

Currently the iterators are just wrappers around the old range types.

Tracking issues:

- rust-lang#123741
- rust-lang#125687
@scottmcm
Copy link
Member

scottmcm commented Aug 1, 2024

We also discussed the RangeFrom iterator: the implementation should add a hidden bool so that any overflow panics happen after returning the maximum integer value, not before. To avoid issues with LLVM's loop optimizations, we may want this bool to only be used in debug builds: in release builds it may acceptable to allow the iterator to wrap.

Hmm, so in release mode the iterator would have extra size that's not used? Seems like a shame, though I guess there's precedent with Fuse.

@dvdsk
Copy link
Contributor

dvdsk commented Sep 16, 2024

I wish we had a map transforming the bounds of a Range. Though I get that we can not get it since it would get really confusing if the range types implemented IntoIterator.

Therefore I really like the idea of a map_bounds function. Re-creating a range simply to map the bounds takes a lot of space for a simple thing. That tents to get me out of the flow of reading any involved algorithm. For me it would be most usefull if map_bounds is added to the RangeBounds trait.

Example use case:

I have log store that tracks logs from a sensor net. It uses a timeseries database. The database stores logs based on their time in seconds, the log store api wants jiff::Timestamp. When fetching a range I need to convert between those.

    fn get(
        &mut self,
        range: RangeInclusive<jiff::Timestamp>,
    ) -> Result<ApiResult<Vec<ErrorEvent>>> {
        let range = RangeInclusive::new(
            range.start().as_second() as u64,
            range.end().as_second() as u64,
        );
       //timeseries lookup using range.

I use RangeInclusive even though impl RangeBounds would be nicer however if I where to do the latter the code for converting the Range would become even longer.

edit: added use case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants