- Notifications
You must be signed in to change notification settings - Fork5.4k
[Bug #21513] Raise on converting endless range to set#13902
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for working on this.
range.c Outdated
| range_to_set(VALUErange) | ||
| { | ||
| if (NIL_P(RANGE_END(range))) { | ||
| rb_raise(rb_eRangeError,"cannot convert endless range to a set"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I believe that endless range error should be the same error type as beginless range error. Callingto_set on a beginless range currently raisesTypeError. This method should probably have beginless range raiseRangeError instead, and include a test and an example for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes sense. Should we also tweakRange#to_a to raiseRangeError in both cases?
./ruby -e'(..1).to_a'-e:1:in'Range#each': can't iterate from NilClass (TypeError)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes sense. Should we also tweak
Range#to_ato raiseRangeErrorin both cases?
@nobu what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It feels clearer for me.
@knu Any thoughts?
If we will change it too, maybe ruby/spec also needs to be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should perhaps check the size of a given Enumerable in Set.new and raise an error when it is infinity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should perhaps check the size of a given Enumerable in Set.new and raise an error when it is infinity.
Seems good to me. I'll try to follow this path
viralpraxisJul 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I tried out this idea and it seems to work fine (we now raiseArgumentError error for any endless enumerable, e.g.1.upto(Float::INFINITY).
$ ./ruby -e'Set.new((1..nil))'-e:1:in'Set#initialize': cannot initialize Set from a enumerable with infinite size (ArgumentError)from -e:1:in'<main>'$ ./ruby -e'(1..nil).to_set' -e:1:in'Enumerable#to_set': cannot initialize Set from a enumerable with infinite size (ArgumentError)from -e:1:in'<main>'
Uh oh!
There was an error while loading.Please reload this page.
4b9f122 to31a3861CompareThere's a SEGV in the ZJIT test suite:https://github.com/ruby/ruby/actions/runs/16305765996/job/46051383888?pr=13902 Not sure if it's related to my changes |
31a3861 toa5a7db6Compare
ZJIT error seems entirely unrelated to your change from a quick glance |
Looks like a weird GC issue related to marking though so paging in@k0kubun for good measure |
Uh oh!
There was an error while loading.Please reload this page.
a5a7db6 toe1b6f53Comparee1b6f53 toca35402Compare This comment has been minimized.
This comment has been minimized.
k0kubun commentedJul 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I agree that it's probably not related to the changes. We're looking into GC issues with ZJIT. |
4fcebf3 to76fe90bCompareref:https://bugs.ruby-lang.org/issues/21513Before this patch, trying to convert endless range (e.g. `(1..)`) to set(using `to_set`) would hang
76fe90b to7753557Compare
Uh oh!
There was an error while loading.Please reload this page.
Before this patch, trying to convert endless range (e.g.
(1..)or any other inifinte enumerable) to set (usingto_set) would hang