Fixing RubyBasicObject#compareTo bug. #600

Merged
merged 1 commit into from Apr 13, 2013

Projects

None yet

2 participants

@rdblue
Contributor
rdblue commented Mar 23, 2013

All IRubyObjects implement Comparable and the default compareTo implementation
delegates to <=>. When the ruby object doesn't implement <=>, ruby raises a
NoMethodError. Because compareTo is called from java but the exception raised
is in ruby, the exception makes no sense: it has the ruby stack when ruby
called into java. For example, this happened on an Iterator that kept min and
max references. This iterator was implemented in java, but called by ruby so
the exception's stack ended with the call to next in java::lang::Iterator#each,
but was a NoMethodError for <=>.

This fix catches the NoMethodError and throws an IllegalArgumentException
wrapping it. This isn't a great solution because java callers expect compareTo
to succeed when objects implement Comparable. There may be a better java
exception to throw. This also updates the javadoc to avoid confusion.

In older versions, specifically 1.6.7, the op_cmp java method is called and
returns nil. Then nil is assumed to be an Integer and coerced, which produces a
TypeError. To be safe, this fix also checks the return value for nil when <=>
succeeds and throws another IllegalArgumentException.

Test update included for RubyHash.

@rdblue rdblue Fixing RubyBasicObject#compareTo bug.
All IRubyObjects implement Comparable and the default compareTo implementation
delegates to <=>. When the ruby object doesn't implement <=>, ruby raises a
NoMethodError. Because compareTo is called from java but the exception raised
is in ruby, the exception makes no sense: it has the ruby stack when ruby
called into java. For example, this happened on an Iterator that kept min and
max references. This iterator was implemented in java, but called by ruby so
the exception's stack ended with the call to next in java::lang::Iterator#each,
but was a NoMethodError for <=>.

This fix catches the NoMethodError and throws an IllegalArgumentException
wrapping it. This isn't a great solution because java callers expect compareTo
to succeed when objects implement Comparable. There may be a better java
exception to throw. This also updates the javadoc to avoid confusion.

In older versions, specifically 1.6.7, the op_cmp java method is called and
returns nil. Then nil is assumed to be an Integer and coerced, which produces a
TypeError. To be safe, this fix also checks the return value for nil when <=>
succeeds and throws another IllegalArgumentException.
99bcf8d
@BanzaiMan BanzaiMan merged commit bfffb0e into jruby:master Apr 13, 2013

1 check passed

default The Travis build passed
Details
@rdblue rdblue deleted the rdblue:fix-cmp branch Apr 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment