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

RFC for inclusive ranges with ... #1192

Merged
merged 3 commits into from
Sep 4, 2015
Merged

Conversation

huonw
Copy link
Member

@huonw huonw commented Jul 7, 2015

@Kimundi
Copy link
Member

Kimundi commented Jul 7, 2015

Imo we should try providing a (x..y).inclusive() method first. I'm also worried about potentially stealing the syntactic space for variadic generics


This struct implements the standard traits (`Clone`, `Debug` etc.),
but, unlike the other `Range*` types, does not implement `Iterator`
directly, since it cannot do so correctly without more internal
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why more state is needed? Why not just increment start while start is less than or equal to end?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsingh93 The problem is that 0...u8::MAX has 257 states: Some(i) for all i; and None.
So you can't modify only one bound and get all the states. You have two options:

  • extra flag for "inclusive case yielded"
  • do a checked_add on each step, and if it fails then decrement the maximum instead of increment the minimum (symmetric logic applies for next_back)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so couldn't RangeInclusive simply get another field that is set during desugaring?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed it with a few people while writing the API, but got such negative feedback that I didn't even think to put it as an alternative. However, several people seem to be in support of it.

@eddyb
Copy link
Member

eddyb commented Jul 7, 2015

@Kimundi VG can still use <Args...> and (args...) because start... is not exactly meaningful (it goes up to infinity... inclusively?), while I expect ...end to work.

@alexcrichton alexcrichton added T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. labels Jul 8, 2015
@nikomatsakis
Copy link
Contributor

I want inclusive ranges, and I want the syntax to be x...y, because it's analogous with patterns. I also want exclusive patterns, but I'm content to wait. Similarly, I think we should support ...y. It seems clear that it is useful in the same way that ..y is useful. If I want the (bigint) numbers from 0 to n inclusive, I should be able to write ...BigInt(N), and not BigInt(0)...BigInt(N).

As for variadics, if the syntaxes turn out to be in conflict in some way, I'd rather have ... for inclusive ranges, given the precedent from patterns. I think it'd be more confusing otherwise. (My impression when we settled on changing patterns to ... was that we aimed to eventually support both inclusive/exclusive patterns/ranges).

@nikomatsakis
Copy link
Contributor

Oh, also: I think that x...y should be an iterator, even if it requires an extra field. I think we should make it work as analogously to x..y as possible. Any discrepancies will be annoying warts. I don't think people store "inclusive ranges" as a datastructure very often (if ever), so the fact that the struct has an extra field shouldn't be a big deal. The usual way to use this is either as an iterator (in which case you want the field), or &vec[x...y], in which case hopefully everything is inlined and you don't ever create the struct in the first place.

@Gankra
Copy link
Contributor

Gankra commented Jul 8, 2015

Agreed with niko, though I'd be interested in investigating the API and performance impact of doing the hack I suggest to avoid a second field. You need to do a check in every step anyway. Might as well fold it into the add, right?

Things that come to mind:

  • You can't have inclusive range iterators of unit types (what a tragedy!)
  • I find the step API to be frustratingly opaque, but it seems like it has all the parts for doing checked adds. For types like BigInts these can return unconditional Some and therefore the checks should be const-folded fairly easy.

@petrochenkov
Copy link
Contributor

How common are inclusive ranges? I'm wary of putting a language sugar into places where a programmer benefits from it like once a year. (Library sugar like (x..y).inclusive() is ok.)

On the other side, inclusive ranges do improve consistency and completeness of the language, that already has inclusive range patterns and exclusive range expressions. On these grounds, I'd suggest to extend the RFC with half-open inclusive range ...a and exclusive range patterns and position it as a single complete "language beauty package".

@nikomatsakis
Copy link
Contributor

@petrochenkov I agree that this works best if we have half-open inclusive ranges and exclusive range patterns as well. If we didn't already have inclusive patterns, I would prefer a method too.

@huonw huonw self-assigned this Jul 8, 2015
@quantheory
Copy link
Contributor

CC #947

@blaenk
Copy link
Contributor

blaenk commented Jul 16, 2015

Yeah LGTM.

@huonw
Copy link
Member Author

huonw commented Aug 3, 2015

I've updated the RFC to include ...b, and to give a design that allows a...b to implement Iterator directly, to match a..b.

@nikomatsakis
Copy link
Contributor

Hear ye, hear ye. This RFC is entering final comment period.

@nikomatsakis nikomatsakis added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Aug 7, 2015
@nikomatsakis
Copy link
Contributor

@huonw looks good to me.

@aturon aturon mentioned this pull request Aug 7, 2015
@hanna-kruppe
Copy link

I realize this problem already exists with patterns, but I don't like that ... is so close to .., visually. Mixing them up can result in off-by-one errors, one of two hard problems in computer science. Debugging those should not be made even harder by by making the two kinds of ranges identical from afar or in a hurry. To quote python-ideas (referring to Tim Peters):

Syntax must not look like grit on Tim's screen!

Therefore I favor (a..b).inclusive(), despite the inconsistency with patterns.

@tbu-
Copy link
Contributor

tbu- commented Aug 11, 2015

I think it's too similar to ..., I wouldn't add it just for the sake of consistency with patterns. Python, for example, only has exclusive ranges as well, and is one of the more convenient programming languages – if we introduce this I'd be in favor of a method like (a..b).inclusive(), because that makes it loud and obvious that this is not a normal range.

@alexcrichton
Copy link
Member

The libs team discussed this at triage today, and our conclusion was that we're ok with the structures defined here. The best route forward is probably to leave the finished field as #[unstable] for the time being to leave the door open to possible future improvements, but otherwise we're ok with it being public.

@bluss
Copy link
Member

bluss commented Aug 13, 2015

Will slices and vectors be indexed with inclusive ranges?

@huonw
Copy link
Member Author

huonw commented Aug 13, 2015

@bluss not sure; definitely not initially, and seems like that would be best be considered/proposed as a separate RFC.

@bluss
Copy link
Member

bluss commented Aug 13, 2015

Slice use of inclusive ranges seems to be an aspect that needs to be covered as well. Without it, this RFC adds an inconsistency. If we don't want to allow &foo[a...b], then maybe we should not have this syntax? Or will users be ok with the logic that half open ranges are index ranges, and closed ranges are not?

@blaenk
Copy link
Contributor

blaenk commented Aug 24, 2015

Heh, it didn't occur to me that this RFC wouldn't include slicing of slices/vectors with inclusive ranges. I do think we should support it, I think it'll be weird to have inclusive ranges but not support them in the area that is probably most commonly used, even if it eventually comes later. It feels to me like it should be a complete package.

@placrosse
Copy link

@rkjnsn +1 for a...b

Consistency with match patterns for both .. and ... will result in the least surprising behavior

@liigo
Copy link
Contributor

liigo commented Sep 2, 2015

@rkruppe

as an extreme example, consider 99999999 vs 999999999

You are just quibbling. Yes 99999999 vs 999999999 is not a significant difference, so does ........ vs .......... But we are comparing .. and ..., two dots and three dots, not many dots.

@gilescope
Copy link

+1 for ... as inclusive.

@main--
Copy link

main-- commented Sep 2, 2015

While I agree that the current proposal may lead to confusion because of the small visual difference, I really don't think there's any way around this unless patterns are changed as well.

Otherwise, the resulting syntax asymmetry is simply much worse.

@glaebhoerl
Copy link
Contributor

I think the fact that we already have ... patterns is what sways me towards that syntax as well.

@nikomatsakis
Copy link
Contributor

My feeling is that we already had this debate back when we adopted .. (for ranges) and ... for patterns. I think the consistency there is important and worthwhile.

@nikomatsakis
Copy link
Contributor

Huzzah! The language design subteam has decided to accept this RFC. There were two major points of contention:

  • whether or not ... ought to be an iterator, or merely support into iterator. The decision was to make ... an interator, which implies an extra field in the implementation to indicate termination.
  • whether to accept the ... syntax at all, or some other alternative such as an inclusive method. The conclusion was that consistency with ... patterns was desirable, as well as concise notation for use cases like vec.remove(1...5).

@blaenk
Copy link
Contributor

blaenk commented Sep 4, 2015

Awesome.

@Stebalien
Copy link
Contributor

So, after the fact (sorry if this is way to late but this hasn't been stabalized yet)... How about using an enum to represent RangeInclusive? For example:

enum RangeInclusive<T> {
    Empty,
    NonEmpty {
        start: T,
        end: T,
    }
}

Rational:

  1. finished is very iterator specific. Regardless of what happens, I think this field should be called empty.
  2. start/end don't make sense if the range is empty. Using an enum prevents coders from using the start/end of spent ranges. Basically, this makes it impossible for the coder to do something like foo(my_range.take(10)); bar(my_range) and forget to check finished in bar.
  3. If we ever get more space optimizations (specifically, utf8 code point ones) 'a'...'z' should be the same size as 'a'..'z'.

tari pushed a commit to tari/rfcs that referenced this pull request Sep 8, 2015
@aturon aturon mentioned this pull request Oct 13, 2015
@huonw huonw deleted the inclusive branch December 21, 2015 04:34
bors added a commit to rust-lang/rust that referenced this pull request Mar 6, 2016
This PR implements [RFC 1192](https://github.com/rust-lang/rfcs/blob/master/text/1192-inclusive-ranges.md), which is triple-dot syntax for inclusive range expressions. The new stuff is behind two feature gates (one for the syntax and one for the std::ops types). This replaces the deprecated functionality in std::iter. Along the way I simplified the desugaring for all ranges.

This is my first contribution to rust which changes more than one character outside of a test or comment, so please review carefully! Some of the individual commit messages have more of my notes. Also thanks for putting up with my dumb questions in #rust-internals.

- For implementing `std::ops::RangeInclusive`, I took @Stebalien's suggestion from rust-lang/rfcs#1192 (comment). It seemed to me to make the implementation easier and increase type safety. If that stands, the RFC should be amended to avoid confusion.
- I also kind of like @glaebhoerl's [idea](rust-lang/rfcs#1254 (comment)), which is unified inclusive/exclusive range syntax something like `x>..=y`. We can experiment with this while everything is behind a feature gate.
- There are a couple of FIXMEs left (see the last commit). I didn't know what to do about `RangeArgument` and I haven't added `Index` impls yet. Those should be discussed/finished before merging.

cc @gankro since you [complained](https://www.reddit.com/r/rust/comments/3xkfro/what_happened_to_inclusive_ranges/cy5j0yq)
cc #27777 #30877 #1192 rust-lang/rfcs#1254
relevant to #28237 (tracking issue)
@kennytm kennytm mentioned this pull request Mar 27, 2017
@purpleposeidon
Copy link

I rather missed the boat on this alternative syntax suggestion: low ==> high.

@Centril Centril added A-syntax Syntax related proposals & ideas A-ranges Proposals relating to ranges. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ranges Proposals relating to ranges. A-syntax Syntax related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.