[ji] avoid NPE when Java collection is not clone-able (the "usual" way)

GH-4721
jruby · Feb 6, 2018 · edd18dd · edd18dd
1 parent 5cdbe74
commit edd18dd
Showing 5 changed files with 96 additions and 6 deletions.
diff --git a/core/src/main/java/org/jruby/javasupport/ext/JavaUtil.java b/core/src/main/java/org/jruby/javasupport/ext/JavaUtil.java
@@ -226,8 +226,8 @@ public static IRubyObject op_minus(final ThreadContext context, final IRubyObjec
         @JRubyMethod
         public static IRubyObject dup(final ThreadContext context, final IRubyObject self) {
             java.util.Collection coll = unwrapIfJavaObject(self);
-            final JavaProxy dup = (JavaProxy) self.dup();
-            if ( coll == dup.getObject() ) { // not Cloneable
+            final JavaProxy dup = (JavaProxy) ((RubyBasicObject) self).dup();
+            if ( coll == dup.getObject() && ! (coll instanceof Cloneable) ) {
                 dup.setObject( tryNewEqualInstance(coll) );
             }
             return dup;
@@ -236,8 +236,8 @@ public static IRubyObject dup(final ThreadContext context, final IRubyObject sel
         @JRubyMethod
         public static IRubyObject clone(final ThreadContext context, final IRubyObject self) {
             java.util.Collection coll = unwrapIfJavaObject(self);
-            final JavaProxy dup = (JavaProxy) self.rbClone();
-            if ( coll == dup.getObject() ) { // not Cloneable
+            final JavaProxy dup = (JavaProxy) ((RubyBasicObject) self).rbClone();
+            if ( coll == dup.getObject() && ! (coll instanceof Cloneable) ) {
                 dup.setObject( tryNewEqualInstance(coll) );
             }
             return dup;
@@ -608,8 +608,10 @@ private static java.util.Collection tryNewEqualInstance(final java.util.Collecti
                     }
                 }
             }
-            if ( CAN_SET_ACCESSIBLE ) best.setAccessible(true);
-            return (java.util.Collection) best.newInstance(coll);
+            if ( best != null ) {
+                if ( CAN_SET_ACCESSIBLE ) best.setAccessible(true);
+                return (java.util.Collection) best.newInstance(coll);
+            }
         }
         catch (IllegalAccessException e) {
             // fallback on getConstructor();

diff --git a/spec/java_integration/extensions/collection_spec.rb b/spec/java_integration/extensions/collection_spec.rb
@@ -139,6 +139,19 @@
     expect( arr.dup ).to be_a java.util.concurrent.LinkedBlockingQueue
     expect( arr.dup.poll ).to eq 42
     expect( arr.to_a ).to eql [42]
+
+    # immutable and Cloneable
+    arr = Java::java_integration::fixtures::coll::CloneableImmutableList.new
+    expect( arr.dup ).to equal arr
+
+    # non-Cloneable with public <init>()
+    arr = Java::java_integration::fixtures::coll::NonCloneableList.new; arr.add(42)
+    expect( arr.dup ).to eql arr
+    expect( arr.dup ).to_not equal arr
+
+    # immutable and non-Cloneable
+    # arr = Java::java_integration::fixtures::coll::NonCloneableImmutableList.new
+    # expect( arr.dup ).to equal arr
   end
 
   it 'clones' do
@@ -157,6 +170,15 @@
     expect( set.clone ).to be_a java.util.concurrent.CopyOnWriteArraySet
     set.clone.add '1'
     expect( set.to_a ).to eql ['0']
+
+    # immutable and Cloneable
+    arr = Java::java_integration::fixtures::coll::CloneableImmutableList.new
+    expect( arr.clone ).to equal arr
+
+    # non-Cloneable with public <init>()
+    arr = Java::java_integration::fixtures::coll::NonCloneableList.new; arr.add(42)
+    expect( arr.clone ).to eql arr
+    expect( arr.clone ).to_not equal arr
   end
 
   it '#include?' do

diff --git a/spec/java_integration/fixtures/coll/CloneableImmutableList.java b/spec/java_integration/fixtures/coll/CloneableImmutableList.java
@@ -0,0 +1,22 @@
+package java_integration.fixtures.coll;
+
+import java.util.*;
+
+public final class CloneableImmutableList extends AbstractList<Object> implements Cloneable {
+    private final List<Object> storage = Arrays.asList(1, 2, 3);
+
+    @Override
+    public int size() {
+        return storage.size();
+    }
+
+    @Override
+    public Object get(int index) {
+        return storage.get(index);
+    }
+
+    @Override
+    public Object clone() {
+        return this;
+    }
+}
diff --git a/spec/java_integration/fixtures/coll/NonCloneableImmutableList.java b/spec/java_integration/fixtures/coll/NonCloneableImmutableList.java
@@ -0,0 +1,17 @@
+package java_integration.fixtures.coll;
+
+import java.util.*;
+
+public final class NonCloneableImmutableList extends AbstractList<Object> {
+    private final List<Object> storage = Arrays.asList(1, 2, 3);
+
+    @Override
+    public int size() {
+        return storage.size();
+    }
+
+    @Override
+    public Object get(int index) {
+        return storage.get(index);
+    }
+}
diff --git a/spec/java_integration/fixtures/coll/NonCloneableList.java b/spec/java_integration/fixtures/coll/NonCloneableList.java
@@ -0,0 +1,27 @@
+package java_integration.fixtures.coll;
+
+import java.util.*;
+
+public class NonCloneableList extends AbstractList<Object> {
+
+    final List<Object> storage = new ArrayList();
+
+    public NonCloneableList() {} // newInstance + addAll works for dup
+
+    @Override
+    public int size() {
+        return storage.size();
+    }
+
+    @Override
+    public Object get(int index) {
+        return storage.get(index);
+    }
+
+    @Override
+    public boolean add(Object e) {
+        storage.add(e);
+        return true;
+    }
+
+}