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

.to_d acts differently in jRuby 9.0.x than Ruby 2.2.x #3504

Closed
ootoovak opened this Issue Nov 29, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@ootoovak

ootoovak commented Nov 29, 2015

How to replicate

Open up IRB in jRuby 9.0.4.0:

require "bigdecimal"; nil.to_s.to_d
=> #<BigDecimal:4d6025c5,'0.0',1(4)>

Opening up IRB in Ruby 2.2.3:

require "bigdecimal"; nil.to_s.to_d
NoMethodError: undefined method `to_d' for "":String
    from (irb):1
    from /Users/ootoovak/.rubies/ruby-2.2.3/bin/irb:11:in `<main>'

System

OS X 10.11.1
java version "1.8.0_60"
Java(TM) SE Runtime Environment (build 1.8.0_60-b27)
Java HotSpot(TM) 64-Bit Server VM (build 25.60-b23, mixed mode)
  • To be clear I prefer jRuby's behaviour in this case but just wanted to point out the difference. In fact I would prefer to be able to do nil.to_d given a choice.
@ahorek

This comment has been minimized.

Contributor

ahorek commented Nov 30, 2015

.to_d is actually defined in bigdecimal/util - http://apidock.com/ruby/String/to_d

MRI

require 'bigdecimal'
require 'bigdecimal/util'

nil.to_s.to_d
=> #<BigDecimal:333b970,'0.0',9(9)>
@ootoovak

This comment has been minimized.

ootoovak commented Nov 30, 2015

Ah, so jRuby includes the 'bigdecimal/util' lib by default when requiring 'bigdecimal'? That's fine (as I said I prefer it, given those two options) but I still just wanted to note the difference if jRuby was going for the exact same syntax behaviour as MRI (for better or worse). If not 👍 and just good to know and keep an eye out for given that context.

@enebo

This comment has been minimized.

Member

enebo commented Dec 1, 2015

Hmmm. I wonder what is loading bigdecimal/util? It is something in our bootstrapping that needs it and we just happen to include it (and obviously MRI is not doing that). I guess if we knew what was loading it then perhaps we can try and do something else. However, in this case any foreign code you interact with might load this library (or over time you may end up inadvertently requiring it) so having any code which expects this to be undefined would end up being dicey...

In my mind it is unclear this is really an issue we should address.

[Actually having written all that I did a grep and I think we might have made some simpler ext mistake. I see in core/src/main/ruby/jruby/bigdecimal.rb we define BigMath in Ruby. This will force require bigdecimal/util (log and exp need .to_d). We must be requiring this file on boot. Do we really need to require this file for this iml? We could move requires into the log and exp methods but that will have a perf impact...hmm]

@enebo

This comment has been minimized.

Member

enebo commented Jun 14, 2017

I looked at this for a minute and realize that our kernel loads kernel/bigdecimal.rb for BigMath (which I think normally should only be included for bigdecimal/math but I am guessing isn't?). This file will unconditionally load bigdecimal/utils since it uses it for its impl.

I think the real issue now is determining whether we are unconditionally loading bigdecimal/math when we shouldn't be or something needs it.

@kares kares added this to the JRuby 9.2.0.0 milestone Jun 23, 2017

@kares kares self-assigned this Apr 11, 2018

kares added a commit to kares/jruby that referenced this issue Apr 11, 2018

avoid loading core_ext bigdecimal/util.rb with BigDecimal library
... for better MRI compatibility (and less surprises) - closing jruby#3504

kares added a commit that referenced this issue Apr 12, 2018

avoid loading core_ext bigdecimal/util.rb with BigDecimal library
... for better MRI compatibility (and less surprises) - closing #3504

@kares kares closed this Apr 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment