Make RubyHash#put return existing value, as per Map contract.

Fixes #4916.
jruby · Jan 25, 2018 · 8198bd5 · 8198bd5
1 parent 2cb38f3
commit 8198bd5
Showing 3 changed files with 54 additions and 14 deletions.
diff --git a/core/src/main/java/org/jruby/RubyHash.java b/core/src/main/java/org/jruby/RubyHash.java
@@ -502,8 +502,6 @@ protected final void checkIterating() {
             throw getRuntime().newRuntimeError("can't add a new key into hash during iteration");
         }
     }
-    // ------------------------------
-    public static long collisions = 0;
 
     // put implementation
 
@@ -512,26 +510,32 @@ private final void internalPut(final IRubyObject key, final IRubyObject value) {
     }
 
     private final void internalPutSmall(final IRubyObject key, final IRubyObject value) {
-        internalPutSmall(key, value, true);
+        internalPutNoResize(key, value, true);
     }
 
     protected void internalPut(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
         checkResize();
 
-        internalPutSmall(key, value, checkForExisting);
+        internalPutNoResize(key, value, checkForExisting);
     }
 
-    protected void internalPutSmall(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
+    protected final IRubyObject internalJavaPut(final IRubyObject key, final IRubyObject value) {
+        checkResize();
+
+        return internalPutNoResize(key, value, true);
+    }
+
+    protected IRubyObject internalPutNoResize(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
         final int hash = hashValue(key);
         final int i = bucketIndex(hash, table.length);
 
-        // if (table[i] != null) collisions++;
-
         if (checkForExisting) {
             for (RubyHashEntry entry = table[i]; entry != null; entry = entry.next) {
                 if (internalKeyExist(entry, hash, key)) {
+                    IRubyObject existing = entry.value;
                     entry.value = value;
-                    return;
+
+                    return existing;
                 }
             }
         }
@@ -540,6 +544,9 @@ protected void internalPutSmall(final IRubyObject key, final IRubyObject value,
 
         table[i] = new RubyHashEntry(hash, key, value, table[i], head);
         size++;
+
+        // no existing entry
+        return null;
     }
 
     // get implementation
@@ -1003,7 +1010,7 @@ protected void op_asetSmallForString(Ruby runtime, RubyString key, IRubyObject v
         } else {
             checkIterating();
             if (!key.isFrozen()) key = (RubyString)key.dupFrozen();
-            internalPutSmall(key, value, false);
+            internalPutNoResize(key, value, false);
         }
     }
 
@@ -2049,8 +2056,9 @@ public Object get(Object key) {
 
     @Override
     public Object put(Object key, Object value) {
-        internalPut(JavaUtil.convertJavaToUsableRubyObject(getRuntime(), key), JavaUtil.convertJavaToUsableRubyObject(getRuntime(), value));
-        return value;
+        Ruby runtime = getRuntime();
+        IRubyObject existing = internalJavaPut(JavaUtil.convertJavaToUsableRubyObject(runtime, key), JavaUtil.convertJavaToUsableRubyObject(runtime, value));
+        return existing == null ? null : existing.toJava(Object.class);
     }
 
     @Override
@@ -2490,4 +2498,9 @@ public IRubyObject default_value_get(ThreadContext context, IRubyObject[] args)
                 throw context.runtime.newArgumentError(args.length, 1);
         }
     }
+
+    @Deprecated
+    protected void internalPutSmall(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
+        internalPutNoResize(key, value, checkForExisting);
+    }
 }
diff --git a/core/src/main/java/org/jruby/java/proxies/MapJavaProxy.java b/core/src/main/java/org/jruby/java/proxies/MapJavaProxy.java
@@ -178,15 +178,23 @@ public RubyHash delete_ifInternal(final ThreadContext context, final Block block
 
         @Override
         public void internalPut(final IRubyObject key, final IRubyObject value, final boolean checkForExisting) {
-            internalPutSmall(key, value, checkForExisting);
+            internalPutNoResize(key, value, checkForExisting);
         }
 
         @Override
-        protected final void internalPutSmall(IRubyObject key, IRubyObject value, boolean checkForExisting) {
+        protected final IRubyObject internalPutNoResize(IRubyObject key, IRubyObject value, boolean checkForExisting) {
             @SuppressWarnings("unchecked")
+            Ruby runtime = getRuntime();
             final Map<Object, Object> map = mapDelegate();
-            map.put(key.toJava(Object.class), value.toJava(Object.class));
+            Object javaValue = value.toJava(Object.class);
+            Object existing = map.put(key.toJava(Object.class), javaValue);
             setSize( map.size() );
+            if (existing != null) {
+                if (existing == javaValue) return value;
+                return JavaUtil.convertJavaToUsableRubyObject(runtime, existing);
+            }
+            // none existing
+            return null;
         }
 
         @Override

diff --git a/core/src/test/java/org/jruby/test/TestRubyHash.java b/core/src/test/java/org/jruby/test/TestRubyHash.java
@@ -36,6 +36,7 @@
 import org.jruby.RubyHash;
 import org.jruby.RubySymbol;
 import org.jruby.exceptions.RaiseException;
+import org.jruby.runtime.builtin.IRubyObject;
 
 /**
  * @author chadfowler
@@ -194,4 +195,22 @@ public void testGet() {
     public void testDoubleQuotedUtf8HashKey() throws Exception {
         assertEquals("UTF-8", eval("# encoding: utf-8\n h = { \"Ãa1\": true }\n puts h.keys.first.encoding"));
     }
+
+    public void testPutExisting() {
+        Ruby ruby = Ruby.newInstance();
+        IRubyObject hash = ruby.evalScriptlet("{ \"Jane Doe\" => 10, \"Jim Doe\" => 6 }");
+        assertTrue(RubyHash.class.isInstance(hash));
+        RubyHash rubyHash = (RubyHash) hash;
+        // Should return previous value 10, but returns the new value 42
+        assertEquals(10L, rubyHash.put("Jane Doe", 42));
+    }
+
+    public void testPutNotExisting() {
+        Ruby ruby = Ruby.newInstance();
+        IRubyObject hash = ruby.evalScriptlet("{ \"Jim Doe\" => 6 }");
+        assertTrue(RubyHash.class.isInstance(hash));
+        RubyHash rubyHash = (RubyHash) hash;
+        // Should return null because the key wasn't present before, but returns the new value 42
+        assertEquals(null, rubyHash.put("Jane Doe", 42));
+    }
 }