base compareTo should not silence all Ruby raised exceptions (fixes #2232)

kares · kares · commit 771afb025d96 · 2015-08-18T08:42:25.000+02:00
this no longer makese sense - and leads to confusing behavior when Ruby objects are used within Java (delegating to &lt;=&gt; which might be user-defined)

every-one is expected to handle &lt;=&gt; on 1.9 (and return nil if not comparable)
on 1.8 we still swallow a NoMethodError as Object does not provide &lt;=&gt;
diff --git a/core/src/main/java/org/jruby/RubyBasicObject.java b/core/src/main/java/org/jruby/RubyBasicObject.java
@@ -1115,18 +1115,17 @@ public IRubyObject op_not_equal(ThreadContext context, IRubyObject other) {
      * @return 0 if equal,
      *         &lt; 0 if this is less than other,
      *         &gt; 0 if this is greater than other
-     * @throws IllegalArgumentException if the objects cannot be compared.
      */
     public int compareTo(IRubyObject other) {
-        try {
-            IRubyObject cmp = invokedynamic(getRuntime().getCurrentContext(),
-                    this, OP_CMP, other);
+        final Ruby runtime = getRuntime();
 
-            // if RubyBasicObject#op_cmp is used, the result may be nil
-            if (!cmp.isNil()) {
-                return (int) cmp.convertToInteger().getLongValue();
-            }
-        } catch (RaiseException ex) {
+        if ( runtime.is1_8() ) return compareTo18(runtime, other);
+
+        IRubyObject cmp = invokedynamic(runtime.getCurrentContext(), this, OP_CMP, other);
+
+        // if RubyBasicObject#op_cmp is used, the result may be nil (not comparable)
+        if ( ! cmp.isNil() ) {
+            return (int) cmp.convertToInteger().getLongValue();
         }
 
         /* We used to raise an error if two IRubyObject were not comparable, but
@@ -1141,6 +1140,27 @@ public int compareTo(IRubyObject other) {
         return 0;
     }
 
+    // on 1.8 nil (Object in general) is not comparable using <=> by default
+    private int compareTo18(final Ruby runtime, IRubyObject other) {
+        final ThreadContext context = runtime.getCurrentContext();
+        final IRubyObject $ex = context.getErrorInfo();
+        try {
+            IRubyObject cmp = invokedynamic(context, this, OP_CMP, other);
+            if ( ! cmp.isNil() ) {
+                return (int) cmp.convertToInteger().getLongValue();
+            }
+        }
+        catch (RaiseException e) {
+            RubyException re = e.getException();
+            if ( re instanceof RubyNoMethodError ) {
+                // e.g. NoMethodError: undefined method `<=>' for #<Object:0xceb431e>
+                context.setErrorInfo($ex); return 0;
+            }
+            throw e;
+        }
+        return 0;
+    }
+
     public IRubyObject op_equal(ThreadContext context, IRubyObject obj) {
         return op_equal_19(context, obj);
     }
@@ -1878,11 +1898,10 @@ public IRubyObject eql_p(IRubyObject obj) {
     }
 
     public IRubyObject op_cmp(ThreadContext context, IRubyObject other) {
-        Ruby runtime = context.runtime;
         if (this == other || invokedynamic(context, this, OP_EQUAL, other).isTrue()){
-            return RubyFixnum.zero(runtime);
+            return RubyFixnum.zero(context.runtime);
         }
-        return runtime.getNil();
+        return context.nil;
     }
 
     /** rb_obj_init_copy
diff --git a/spec/java_integration/exceptions/rescue_spec.rb b/spec/java_integration/exceptions/rescue_spec.rb
@@ -225,4 +225,28 @@ def obj.go
 
   end
 
+  SampleTask = Struct.new(:time) do
+    include Comparable
+
+    def <=>(that)
+      raise ArgumentError.new("unexpected #{self.inspect}") unless self.time
+      raise ArgumentError.new("unexpected #{that.inspect}") unless that.time
+      self.time <=> that.time
+    end
+  end
+
+  it 'does not swallow Ruby errors on compareTo' do
+    queue = java.util.PriorityQueue.new(10)
+    queue.add t2 = SampleTask.new(2)
+    queue.add t1 = SampleTask.new(1)
+    begin
+      queue.add SampleTask.new(nil)
+    rescue ArgumentError => e
+      expect( e.message ).to start_with 'unexpected #<struct SampleTask'
+    else
+      fail 'compareTo did not raise'
+    end
+    expect( queue.first ).to be t1
+  end
+
 end
