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

Add begin-less and end-less ranges #7179

Merged
merged 8 commits into from Feb 10, 2019

Conversation

Projects
None yet
10 participants
@asterite
Copy link
Member

commented Dec 11, 2018

Fixes #7170

This PR introduces begin-less and end-less ranges. If most of us think this is a nice and useful feature we could merge it.

An end-less range is just a range with nil as its end. It can be written as (3..) or (3..nil), but of course without an explicit nil is nicer.

A begin-less range is just a range with nil as its beginning. It can be written as ..3 or nil..3.

These ranges have two semantics:

As a range

A range has methods like each, step, etc. An endless range doesn't stop iteration (well, it will, eventually, when it overflows):

(3..).each do |x|
  puts x
  break if some_condition
end

You can't invoke each on a begin-less range (it errors at compile-time, unless the begin is a nilable int, and in that case it raises at runtime). But you can invoke reverse_each:

(..3).reverse_each do |x|
  # ... eventually yields 0, -1, -2
end

Note that a begin-less range isn't equivalent to a range that starts with 0 in these case.

A range also has a === method which can be use in case (and soon in some Enumerable methods ). In this case an endless range means "greater than the beginning of the range":

number = rand(1..10)
case number
when ..3 then "foo"
when (3..5) then "bar"
when (6..) then "bar" # or just else
end

(well, maybe the above isn't a good example because there's still an else nil branch there)

Another example:

numbers = [1, 10, 3, 4, 5, 8]
numbers.select(6..) # => [10, 8]
numbers.select(..6) # => [1, 3, 4, 5]

The above is the same as:

numbers.select(&.>=(6))
numbers.select(&.<=(6))

I think the former is a bit more readable.

There's also Number#clamp, so now there's another way of doing this:

num = some_number
num = 10 if num <= 10

We can do:

num = num.clamp(10..)

Same goes with capping to a max.

In Ruby 2.6 there was also an example like this:

[1, 2, 3].zip(6..) # => [[1, 6], [2, 7], [3, 8]]

which is nice (no need to explicitly specify an upper bound) but currently doesn't work in Crystal (but I might make it work soon).

As an indexer

As an indexer, an endless range means "until the end of the collection". It's equivalent to passing -1, except that you don't have to write that and you don't have to care about .. vs. ...:

ary = [1, 2, 3, 4, 5]
ary[2..] # => [3, 4, 5]
ary[2...] # => [3, 4, 5]

This works for any indexable (Array, Deque) and String.

For a begin-less range it's equivalent to passing 0, but maybe it's a bit more concise:

ary = [1, 2, 3, 4, 5]
ary[..2] # => [1, 2, 3]

The catch-all range

Because the begin and end are now optional, we can also write (..). It doesn't have much use, really. We could forbid it. But right now you can use it:

# replace all elements of an array with a single element
ary = [1, 2, 3, 4, 5]
ary[..] = 3
ary # => [3]

# replace all elements of an array
ary = [1, 2, 3, 4, 5]
ary[..] = [6, 7, 8]
ary # => [6, 7, 8]

# fill an array with a value
ary = [1, 2, 3]
ary.fill(8, ..)
ary # => [8, 8, 8]

# another way to dup an array
ary = [1, 2, 3]
b = ary[..]

And also:

case number
when .. then "duh"
end

Yeah... not a lot of uses, but the semantics are consistent.

Implementation details

Ideally I would change the Range(Int, Int) restrictions to Range(Int?, Int?). However, that gives a compile-error in some cases saying that you can't have Int inside a union type. Instead of trying to fix that I decided to just drop those inner restrictions for two reasons:

  • I don't have much time to think about fixing that error
  • It doesn't matter much because you will still get an (probably uglier) error if you pass a non-number range, but:
  • I don't think there will be many there passing ranges of non-integers to these methods
  • It's still not clear what we want to do with these restrictions: is a Range(Int, Int) actually a thing, or should it be more like Range(< Int, < Int) saying "anything that inherits Int"?

Changing the parser was relatively simple.
Making the changes to Range methods was also simple, but it took a bit of time.
Changing the logic of indexers (ary[3..]) was very easy because all of that logic is in Indexable.range_to_index_and_count (we might want to move that method to Range because it's also used in String which is not an Indexable)

Thoughts?

@asterite asterite force-pushed the asterite:feature/xless-ranges branch 2 times, most recently from d9c1d7a to f83ffbd Dec 11, 2018

@asterite asterite force-pushed the asterite:feature/xless-ranges branch from f83ffbd to 8f01c77 Dec 12, 2018

@j8r

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

What's the supposed output of (..) and (...)?

@asterite

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

@j8r .. and ...

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Range doesn't have to be restricted to Int anyway. Theoretically it can be used with any Comparable that implements #succ.

@j8r

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

I mean col[..] is supposed to return the whole collection, col[...] all but the last one? I've seen a spec for nil..nil but not for nil...nil

@asterite

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

.. and ... are the same. Inclusive and exclusive against infinite is infinite.

@j8r

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

@asterite We have to do ..-2 instead of ... to have all but the last element?
Because we have:

a = [0, 1, 2]
puts a[0...-1] #=> [0, 1] 
puts a[0..-2]  #=> [0, 1]

... won't be equal to [0...-1], but equal to [0..-1] and .. then?

@asterite

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

Your reasoning is correct. However an endless range means "up to infinite" or without an end. In the case of indexing a collection, because the size is finite, it just means to take everything available. There the exclusive/inclusive distinction doesn't make sense.

Maybe to prevent this small confusion we could make an endless exclusive range a syntax error. Thoughts?

@j8r

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

Yes the nillable end of exclusive ranges could be forbidden (like now).

@sudo-nice

This comment has been minimized.

Copy link

commented Dec 13, 2018

@asterite There is an example above you wrote:

(3..).reverse_each do |x|
  # ... eventually yields 0, -1, -2
end

Did you really mean it should yield 3, 2, 1, 0, -1, -2, ... MINUS INFINITY?

@sudo-nice

This comment has been minimized.

Copy link

commented Dec 13, 2018

So that .reverse_each on end-less range would mean start when the range starts, but go the opposite direction?

@asterite

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2018

Oops, I had a typo in the example. Now it's fixed.

@yxhuvud

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

Regarding the catch-all range, while it isn't amazing for one dimensional arrays, it will probably be a godsend to people implementing matrices with multiple dimensions.

Show resolved Hide resolved spec/std/indexable_spec.cr Outdated
Show resolved Hide resolved spec/std/indexable_spec.cr
@@ -301,6 +356,49 @@ describe "Range" do
it "is not empty with ... and begin.succ == end" do
(1...2).reverse_each.to_a.should eq([1])
end

it "raises on endless range" do

This comment has been minimized.

Copy link
@RX14

RX14 Dec 16, 2018

Member

Can we have a spec for the reverse iterator working too?

This comment has been minimized.

Copy link
@asterite

asterite Dec 16, 2018

Author Member

Unless I'm misunderstanding something, this is the spec for the reverse iterator. Or did you mean something else?

This comment has been minimized.

Copy link
@RX14

RX14 Dec 17, 2018

Member

This is the spec for the reverse_each iterator raising, but not actually working (calling .next)

This comment has been minimized.

Copy link
@asterite

asterite Dec 17, 2018

Author Member

Ooooh... you are right! In fact this wasn't working. I added a spec and also fixed the code.

Show resolved Hide resolved spec/std/range_spec.cr
Show resolved Hide resolved spec/std/range_spec.cr
Show resolved Hide resolved src/range.cr Outdated
Show resolved Hide resolved src/range.cr
@asterite

This comment has been minimized.

Copy link
Member Author

commented Dec 16, 2018

I originally had two commits, one for end-less range and another for begin-less range. I'll add more commits that fix the existing issues and then we can squash it into a single commit, after all the changes aren't very big.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Dec 16, 2018

@RX14 All comments addressed (except one I didn't understand).

Given that this PR also adds specs for Range#step, I'd like to merge this first and then I can send a PR fixing it for negative steps (it's very similar to with positive steps except we have to use pred).

@asterite asterite force-pushed the asterite:feature/xless-ranges branch 2 times, most recently from fc4ebe8 to 941b3e9 Dec 16, 2018

@RX14

RX14 approved these changes Dec 17, 2018

@RX14 RX14 requested a review from bcardiff Dec 17, 2018

@RX14 RX14 added the topic:compiler label Dec 17, 2018

@RX14 RX14 added this to the 0.27.1 milestone Dec 17, 2018

@bew

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

Is foo ..2 the same as foo(..2) or foo..2 ?
(this is important for method calls when generated from macros for example)

@asterite

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

@bew Excellent question!

I decided it's parsed as foo..2, so as a range, not as foo(..2). I think parsing it the other way would mean a breaking change, plus you can always disambiguate the two cases with parentheses.

But we can discuss it, of course.

@bew

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

Yes, foo..2 is the most logical and easiest to parse from the compiler point of view, but for us humans it's not that easy and logical I think..

The following thought is for ranges in general:
I would prefer to make 1 .. 3 an error, and force the 1..3 or 1..(2 + 1) (no spaces around ..) for ranges.
btw, iirc I never saw ranges with spaces

This way you would easily disambiguate the 2: when there is no space it would ALWAYS be a range, when there is spaces it can only be a partial range (begin and/or end -less range)

This can probably wait a bit, I'll open a discussion about this when this PR is merged

@bcardiff bcardiff added this to the 0.28.0 milestone Jan 30, 2019

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

Can anyone reproduce the error in linux 64 bits? I'm having a bit of trouble trying to reproduce it...

@j8r

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

No problem on my side with the feature/xless-ranges branch. I've ran make before doing this.

/app # ./bin/crystal build  -o .build/std_spec spec/std_spec.cr?
Using compiled compiler at `.build/crystal'
/app # uname -a
Linux f5b436f201b2 4.18.0-11-generic #12-Ubuntu SMP Tue Oct 23 19:22:37 UTC 2018 x86_64 Linux
/app # cat /etc/alpine-release 
3.9.0
/app # git describe --tags --long --always
0.25.0-579-gcbf479d31
@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

@j8r Thank you! But that's not exactly the command that's run. The full command is:

./bin/crystal build -D preview_overflow -D compiler_rt  -o .build/std_spec spec/std_spec.cr

Check the -D flags there.

@j8r

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

No problem too @asterite :

./bin/crystal build -D preview_overflow -D compiler_rt  -o .build/std_spec spec/
std_spec.cr
Using compiled compiler at `.build/crystal'
@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

Hmm... I don't know what's going on then...

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

(@j8r thank you!)

@asterite asterite force-pushed the asterite:feature/xless-ranges branch from cbf479d to 3153f4f Feb 9, 2019

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2019

CI passed this time after merging #7397 🎉 😁

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2019

Merge? 😺

@straight-shoota straight-shoota merged commit 0ce1328 into crystal-lang:master Feb 10, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@straight-shoota

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

Thanks @asterite!

@asterite asterite deleted the asterite:feature/xless-ranges branch Mar 30, 2019

straight-shoota added a commit to straight-shoota/crystal-book that referenced this pull request Apr 2, 2019

Improve description of range literal
Also includes documentation of begin-less and end-less ranges introduced by crystal-lang/crystal#7179

@bcardiff bcardiff added topic:lang and removed topic:compiler labels Apr 9, 2019

RX14 added a commit to crystal-lang/crystal-book that referenced this pull request Apr 14, 2019

Improve description of range literal (#332)
* Improve description of range literal

Also includes documentation of begin-less and end-less ranges introduced by crystal-lang/crystal#7179

* fixup! Improve description of range literal

* Update syntax_and_semantics/literals/range.md

Co-Authored-By: straight-shoota <johannes.mueller@smj-fulda.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.