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

Crashing on attempt to convert Rational into BigDecimal #4324

Closed
kirs opened this issue Nov 20, 2016 · 1 comment · Fixed by #4336
Closed

Crashing on attempt to convert Rational into BigDecimal #4324

kirs opened this issue Nov 20, 2016 · 1 comment · Fixed by #4336
Milestone

Comments

@kirs
Copy link
Contributor

kirs commented Nov 20, 2016

Environment

jruby 9.1.6.0 (2.3.1) 2016-11-09
macOS 10.12.1

The reproduce script:

require 'bigdecimal'

expected = BigDecimal("0.333333333333333333E0")
actual = BigDecimal(Rational(1, 3), 0)
puts (expected == actual).inspect

Expected Behavior

CRuby successfully converts Rational into BigDecimal:

$ ruby -v demo.rb
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-darwin15]
true

Actual Behavior

JRuby crashes with java.lang.ArithmeticException.

$ jruby -v --dev demo.rb
jruby 9.1.6.0 (2.3.1) 2016-11-09 0150a76 Java HotSpot(TM) 64-Bit Server VM 25.45-b02 on 1.8.0_45-b14 [darwin-x86_64]
Unhandled Java exception: java.lang.ArithmeticException: Non-terminating decimal expansion; no exact representable decimal result.
java.lang.ArithmeticException: Non-terminating decimal expansion; no exact representable decimal result.
          divide at java/math/BigDecimal.java:1690
          divide at java/math/BigDecimal.java:1723
     newInstance at org/jruby/ext/bigdecimal/RubyBigDecimal.java:490
     newInstance at org/jruby/ext/bigdecimal/RubyBigDecimal.java:626
   newBigDecimal at org/jruby/ext/bigdecimal/RubyBigDecimal.java:226
            call at org/jruby/ext/bigdecimal/RubyBigDecimal$BigDecimalKernelMethods$INVOKER$s$0$1$newBigDecimal.gen:-1
            call at org/jruby/internal/runtime/methods/JavaMethod.java:724
            call at org/jruby/internal/runtime/methods/DynamicMethod.java:208
    cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:358
            call at org/jruby/runtime/callsite/CachingCallSite.java:195
     processCall at org/jruby/ir/interpreter/InterpreterEngine.java:324
       interpret at org/jruby/ir/interpreter/StartupInterpreterEngine.java:73
  INTERPRET_ROOT at org/jruby/ir/interpreter/Interpreter.java:112
         execute at org/jruby/ir/interpreter/Interpreter.java:99
         execute at org/jruby/ir/interpreter/Interpreter.java:35
         execute at org/jruby/ir/IRTranslator.java:42
  runInterpreter at org/jruby/Ruby.java:867
  runInterpreter at org/jruby/Ruby.java:871
     runNormally at org/jruby/Ruby.java:766
     runNormally at org/jruby/Ruby.java:779
     runFromMain at org/jruby/Ruby.java:592
   doRunFromMain at org/jruby/Main.java:425
     internalRun at org/jruby/Main.java:313
             run at org/jruby/Main.java:242
            main at org/jruby/Main.java:204

I discovered this bug when trying to fix failings JRuby tests on Rails master.

@enebo @headius @guilleiguaran

@kirs
Copy link
Contributor Author

kirs commented Nov 22, 2016

Here is the problematic line: BigDecimal(Rational(1, 3), 0).

Regarding the precision (second argument) from the MRI doc:

If omitted or 0, the number of significant digits is determined from the initial value.

It looks like JRuby is missing the "number of significant digits is determined from the initial value" part. Currently it doesn't follow the MRI behaviour.

I tried to look into how MRI does that, but it's crazy: https://github.com/ruby/ruby/blob/ruby_2_3/ext/bigdecimal/bigdecimal.c#L4688-L4807

kirs added a commit to kirs/jruby that referenced this issue Nov 26, 2016
when converting Rational to BigDecimal
Fixes jruby#4324
kirs added a commit to kirs/jruby that referenced this issue Nov 26, 2016
when converting Rational to BigDecimal
Fixes jruby#4324
@kares kares added this to the JRuby 9.1.7.0 milestone Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants