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

Fix endless range include #36460

Merged
merged 1 commit into from Sep 9, 2019

Conversation

@aldhsu
Copy link
Contributor

aldhsu commented Jun 10, 2019

Summary

Problem

See #36451
Core extension on ranges calls #last.
Ruby 2.6.0 Endless ranges blow up on calling #last:

ruby -e "(1..).last"                                                                                                                                                                               2.6.2 help-plan-scoring b2c08bc ✗
Traceback (most recent call last):
        1: from -e:1:in `<main>'
-e:1:in `last': cannot get the last element of endless range (RangeError)

Solution

-#end seems directly substitutable, (#last acts like #end if #last is called without args) and does not blow up and returns nil if no end is defined, from the source of #last

               static VALUE
range_last(int argc, VALUE *argv, VALUE range)
{
    if (NIL_P(RANGE_END(range))) {
        rb_raise(rb_eRangeError, "cannot get the last element of endless range");
    }
    if (argc == 0) return RANGE_END(range);
    return rb_ary_last(argc, argv, rb_Array(range));
}

Source of #end:

               static VALUE
range_end(VALUE range)
{
    return RANGE_END(range);
}
@rails-bot rails-bot bot added the activesupport label Jun 10, 2019
@aldhsu aldhsu force-pushed the aldhsu:fix-endless-range-include branch from cfc044b to bfbb715 Jun 12, 2019
@aldhsu aldhsu force-pushed the aldhsu:fix-endless-range-include branch from bfbb715 to 10334b4 Sep 8, 2019
@aldhsu

This comment has been minimized.

Copy link
Contributor Author

aldhsu commented Sep 8, 2019

Rebased on master.

@eugeneius

This comment has been minimized.

Copy link
Member

eugeneius commented Sep 8, 2019

Could you add a changelog entry and squash your commits? Otherwise, this looks good to me. 👍

@aldhsu aldhsu force-pushed the aldhsu:fix-endless-range-include branch 3 times, most recently from 4529eb2 to ccc091b Sep 8, 2019
@aldhsu aldhsu force-pushed the aldhsu:fix-endless-range-include branch 2 times, most recently from 4e122a2 to 0f7afd8 Sep 8, 2019
@aldhsu aldhsu force-pushed the aldhsu:fix-endless-range-include branch from 0f7afd8 to f7b5338 Sep 8, 2019
@aldhsu aldhsu force-pushed the aldhsu:fix-endless-range-include branch from f7b5338 to d45a7f1 Sep 8, 2019
@aldhsu

This comment has been minimized.

Copy link
Contributor Author

aldhsu commented Sep 8, 2019

Thanks @kamipo and @eugeneius for the reviews!

@kamipo kamipo merged commit bfacd45 into rails:master Sep 9, 2019
2 checks passed
2 checks passed
buildkite/rails Build #63522 passed (16 minutes, 40 seconds)
Details
codeclimate All good!
Details
@aldhsu aldhsu deleted the aldhsu:fix-endless-range-include branch Sep 9, 2019
kamipo added a commit that referenced this pull request Sep 9, 2019
@pond

This comment has been minimized.

Copy link
Contributor

pond commented Sep 12, 2019

This misses some other cases - see #37172 and comments therein. I'll take the approach established by this PR for tests etc., & get a patch together to fill in the rest of the gaps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.