Inheriting from BigDecimal returns incorrect class #198

Closed
dekz opened this Issue Jun 6, 2012 · 4 comments

Projects

None yet

4 participants

@dekz
Contributor
dekz commented Jun 6, 2012

When creating a subclass of BigDecimal the class returned on construction is BigDecimal and not the extended class. Discovered by @kouno.

Construction inside RubyBigDecimal.java seems to be the cause of the issue. All BigDecimal#new calls seem to call the BigDecimal argumented constructor.

    public RubyBigDecimal(Ruby runtime, BigDecimal value) {
        super(runtime, runtime.getClass("BigDecimal"));
        this.value = value;
    }

This should be replaced with a helper constructor that takes into account this.metaClass

    private RubyBigDecimal createBigDecimal(Ruby runtime, BigDecimal value) {
        return new RubyBigDecimal(runtime, this.getMetaClass(), value);
    }

And all constructions from operations should use this.

https://gist.github.com/2878757

SHA: 02002ed

@cjheath
cjheath commented Jun 6, 2012

You should be able to work around that by overriding Subclass#new (to call allocate, initialize, then return the object). This should work at the Ruby level before a patch is available to the Java.

@headius
Member
headius commented Aug 7, 2012

The construction sequence for BigDecimal is all wrong; instead of the normal allocate + initialize, it overrides ::new to do the entire process, and ignores the incoming class. The proper fix is to move this logic into initialize and make all appropriate paths use the proper construction sequence rather than shortcutting through newInstance.

Won't make it into 1.7pre2, but I'll mark for 1.7.

@BanzaiMan
Member

Did we do any work for 1.7.0? I'm marking it for 1.7.1 for now.

@headius
Member
headius commented Oct 30, 2012

I have a fix for this that doesn't correct the allocate+initialize sequence, but does use the incoming class as the class for the new object. It fulfills the requirements of this bug, but I'll file another for fixing the construction sequence.

@headius headius added a commit that closed this issue Oct 30, 2012
@headius headius Fix #198
This does not fix the flawed allocate+initialize sequence, but it
does cause our custom "new" to do the right thing. We still need
to fix the general case for subclasses that define their own
initialize method.
d0da16d
@headius headius closed this in d0da16d Oct 30, 2012
@prathamesh-sonpatki prathamesh-sonpatki added a commit to prathamesh-sonpatki/jruby that referenced this issue Dec 3, 2012
@headius @prathamesh-sonpatki headius + prathamesh-sonpatki Fix #198
This does not fix the flawed allocate+initialize sequence, but it
does cause our custom "new" to do the right thing. We still need
to fix the general case for subclasses that define their own
initialize method.
c0bb2c6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment