Ruby 2.7: Better support of Integer#[] with range arguments #2182

gogainda · 2020-12-08T13:25:11Z

Related to #2004
Implementing https://bugs.ruby-lang.org/issues/8842

This PR is basically translation of this commit to Ruby language

The only test which is not passing for Integer#[] is

1)
Integer#[] fixnum when range passed raises FloatDomainError if any boundary is infinity ERROR
Expected FloatDomainError ((?-mix:-Infinity))
but got: FloatDomainError (Infinity)
<internal:core> core/type.rb:298:in `to_int'
<internal:core> core/type.rb:298:in `check_funcall'
<internal:core> core/type.rb:274:in `convert_type'
<internal:core> core/type.rb:186:in `rb_to_int_fallback'
<internal:core> core/integer.rb:100:in `>>'
<internal:core> core/integer.rb:100:in `[]'
/Users/novoi/projects/truffleruby-ws/truffleruby/spec/ruby/core/integer/element_reference_spec.rb:148:in `block (6 levels) in <top (required)>'
/Users/novoi/projects/truffleruby-ws/truffleruby/spec/ruby/core/integer/element_reference_spec.rb:148:in `block (5 levels) in <top (required)>'
/Users/novoi/projects/truffleruby-ws/truffleruby/spec/ruby/core/integer/element_reference_spec.rb:3:in `<top (required)>'

which I am not sure how to fix currently (corrected: fixed)

Ruby code is not very idiomatic right now, but I will work on it later. As of now I will focus on review comments if any:)

graalvmbot · 2020-12-08T13:42:54Z

Hello Igor Victor, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address gogainda -(at)- yandex -(dot)- ru. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

graalvmbot · 2020-12-08T17:22:57Z

Hello Igor Novokshonov, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address gogainda -(at)- yandex -(dot)- ru. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

…ting issues

gogainda · 2020-12-10T09:49:29Z

src/main/ruby/truffleruby/core/integer.rb

@@ -38,6 +38,7 @@
 Object.deprecate_constant :Fixnum, :Bignum

 class Integer < Numeric
+  FIXNUM_MAX = 0x3fffffff


@eregon not sure that it's safe to hardcode this value, but I did't find better solution, mb you know?

It seems odd to me we even need this value to implement Integer#[], do you know which arguments return it?

We don't need this anymore.

eregon

Translating C code from MRI to Ruby leads to rather unreadable Ruby code, as you can see.

Could you try to write an implementation based on the specs/tests and not based on what MRI does in C code?
I think that would be a lot more readable.

I think you could use Primitive.range_normalized_start_length(range, self.bit_length) to convert all kind of ranges to a [start, length] pair, so there is no need to deal with various begin/endless Ranges explicitly.

graalvmbot · 2020-12-10T12:42:55Z

Igor Novokshonov has signed the Oracle Contributor Agreement (based on email address gogainda -(at)- yandex -(dot)- ru) so can contribute to this repository.

gogainda · 2020-12-14T22:10:45Z

I rewrote the logic as you suggested, but not quite happy with results.
spec/ruby tests are passing, but some of them contradict to the ruby tests. For example test "ignores negative upper boundary", which I think is not a valid example because according to ruby tests -16[-66..-66] should return 0 and def -66 should not be ignored.

eregon · 2020-12-15T12:24:18Z

which I think is not a valid example because according to ruby tests -16[-66..-66] should return 0 and def -66 should not be ignored.

I'm not sure to follow, do you have an example where it behaves differently on MRI?
(-16)[-66..-66] is 0 on MRI.

gogainda · 2020-12-15T13:32:04Z

this implementation works as per element_reference_tags test, but some of the mri tests are still failing, this is why I reverted "exclude :test_aref, "needs investigation", specifically my impl is not correctly handling ranges starting with negative values. Should I try to come up with solution which will fix element_reference_tags and ruby mri tests?
(btw what is the naming convention for different types of tests in truffleruby: mri tests, specs, etc)

eregon · 2020-12-15T14:12:04Z

this implementation works as per element_reference_tags test, but some of the mri tests are still failing, this is why I reverted "exclude :test_aref, "needs investigation", specifically my impl is not correctly handling ranges starting with negative values.
Should I try to come up with solution which will fix element_reference_tags and ruby mri tests?

Yes, we should try to make it correct.
Even better would be to add a new spec example which clearly shows the behavior for such cases (TestInteger#test_aref mixes tons of things, so there is no easy way to know what the behavior should be).
Maybe Primitive.range_to_int_range is useful here, if you need to differentiate e.g. n[0..3] and n[-2..3].

(btw what is the naming convention for different types of tests in truffleruby: mri tests, specs, etc)

Yeah we call them "specs" and "MRI tests".

gogainda · 2020-12-15T23:17:49Z

@eregon all tests should be passing now. Additionally, I removed one misleading test case from specs

eregon · 2020-12-16T11:49:40Z

src/main/ruby/truffleruby/core/integer.rb

@@ -38,6 +38,7 @@
 Object.deprecate_constant :Fixnum, :Bignum

 class Integer < Numeric
+  FIXNUM_MAX = 0x3fffffff


We don't need this anymore.

eregon · 2020-12-16T11:51:48Z

src/main/ruby/truffleruby/core/integer.rb

+    if range.exclude_end?
+      (1 << idx) - 1
+    else
+      (1 << (idx +1)) - 1


eregon · 2020-12-16T11:52:30Z

src/main/ruby/truffleruby/core/integer.rb

+    end
+  end
+
+  private def validate_range(index)


Could you inline this method?

eregon · 2020-12-16T11:56:06Z

src/main/ruby/truffleruby/core/integer.rb

+    raise FloatDomainError , '-Infinity' if index.begin == -Float::INFINITY || index.end == -Float::INFINITY
+  end
+
+  private def mask(range, idx)


Could you move this logic in the two callers, and e.g. do len += 1 unless range.exclude_end? so then it's just (1 << len) - 1?

eregon · 2020-12-16T11:57:56Z

src/main/ruby/truffleruby/core/integer.rb

+    end
+  end
+
+  private def handle_aref(index, len)


We shouldn't have extra methods on Integer, even if private.
We should move them to class methods on a new Truffle::IntegerOperations module.
I can do that.

I addressed all the comments apart from this one, not sure what I can do here. More general question, trying to avoid creating extra private methods is dictated by how truffleruby treats such code or it's code style preference?

It's because a Ruby program might notice this, e.g. some Ruby program would defined a public Integer#handle_aref, then it could conflict. It's also visible via private_methods, send, etc.

gogainda · 2020-12-18T14:32:54Z

@eregon anything else I should do here?

eregon

Looks great now, thank you for the PR!

eregon · 2020-12-18T16:52:48Z

src/main/ruby/truffleruby/core/integer.rb

+    end
+  end
+
+  private def handle_aref(index, len)


It's because a Ruby program might notice this, e.g. some Ruby program would defined a public Integer#handle_aref, then it could conflict. It's also visible via private_methods, send, etc.

gogainda · 2020-12-18T20:11:33Z

@eregon do I need to squash it? btw, I forgot to add a change log

#2182) PullRequest: truffleruby/2274

eregon · 2020-12-19T12:05:46Z

It's merged now, so it can't be squashed, but I think that's OK because the commits only touched that area of the code.

Could you make a PR to add the ChangeLog entry?

eregon · 2020-12-19T12:16:56Z

@gogainda Actually I'll make a PR adding the ChangeLog entry, I'm already doing another PR touching the ChangeLog anyway.

Ruby 2.7: Better support of Integer#[] with range arguments

f77f2a2

gogainda force-pushed the feature/ruby2.7_better_support_for_integer_with_range_arguments branch from 80bbe8d to f77f2a2 Compare December 8, 2020 17:20

Ruby 2.7: Better support of Integer#[] with range arguments - Fix lin…

cda845a

…ting issues

gogainda commented Dec 10, 2020

View reviewed changes

eregon reviewed Dec 10, 2020

View reviewed changes

graalvmbot added the oca-signed label Dec 10, 2020

gogainda added 3 commits December 14, 2020 18:37

Ruby 2.7: first approach

5a7bbb8

Ruby 2.7: rewrite some logic

61f2703

Ruby 2.7: rewrite some logic

1382260

Ruby 2.7: extend logic

a260037

eregon reviewed Dec 16, 2020

View reviewed changes

Ruby 2.7: address review comments

f427326

gogainda marked this pull request as ready for review December 18, 2020 14:32

eregon approved these changes Dec 18, 2020

View reviewed changes

eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Dec 18, 2020

eregon self-assigned this Dec 18, 2020

eregon added this to the 21.1.0 milestone Dec 18, 2020

graalvmbot pushed a commit that referenced this pull request Dec 18, 2020

[GR-18163] Ruby 2.7: Better support of Integer#[] with range arguments (

ef6fd86

#2182) PullRequest: truffleruby/2274

graalvmbot merged commit f427326 into oracle:master Dec 18, 2020

graalvmbot pushed a commit that referenced this pull request Dec 21, 2020

Add ChangeLog entry for #2182

c766c95

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ruby 2.7: Better support of Integer#[] with range arguments #2182

Ruby 2.7: Better support of Integer#[] with range arguments #2182

gogainda commented Dec 8, 2020 •

edited

Loading

graalvmbot commented Dec 8, 2020

graalvmbot commented Dec 8, 2020

gogainda Dec 10, 2020

eregon Dec 10, 2020

eregon Dec 16, 2020

eregon left a comment •

edited

Loading

graalvmbot commented Dec 10, 2020

gogainda commented Dec 14, 2020

eregon commented Dec 15, 2020

gogainda commented Dec 15, 2020

eregon commented Dec 15, 2020

gogainda commented Dec 15, 2020

eregon Dec 16, 2020

eregon Dec 16, 2020

eregon Dec 16, 2020

eregon Dec 16, 2020

eregon Dec 16, 2020

gogainda Dec 17, 2020

eregon Dec 18, 2020 •

edited

Loading

gogainda commented Dec 18, 2020

eregon left a comment

eregon Dec 18, 2020 •

edited

Loading

gogainda commented Dec 18, 2020

eregon commented Dec 19, 2020

eregon commented Dec 19, 2020

Ruby 2.7: Better support of Integer#[] with range arguments #2182

Ruby 2.7: Better support of Integer#[] with range arguments #2182

Conversation

gogainda commented Dec 8, 2020 • edited Loading

graalvmbot commented Dec 8, 2020

graalvmbot commented Dec 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon left a comment • edited Loading

Choose a reason for hiding this comment

graalvmbot commented Dec 10, 2020

gogainda commented Dec 14, 2020

eregon commented Dec 15, 2020

gogainda commented Dec 15, 2020

eregon commented Dec 15, 2020

gogainda commented Dec 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

gogainda commented Dec 18, 2020

eregon left a comment

Choose a reason for hiding this comment

eregon Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

gogainda commented Dec 18, 2020

eregon commented Dec 19, 2020

eregon commented Dec 19, 2020

gogainda commented Dec 8, 2020 •

edited

Loading

eregon left a comment •

edited

Loading

eregon Dec 18, 2020 •

edited

Loading

eregon Dec 18, 2020 •

edited

Loading