Fix concatenation of multiple ** arguments with duplicate keys (#1469)

PullRequest: truffleruby/678

Author: aardvark179

CHANGELOG.md

@@ -5,6 +5,8 @@ Bug fixes:
 * Implement `rb_io_wait_writable` (#1586).
 * Fixed error when using arrows keys first within `irb` or `pry` (#1478, #1486).
 * Coerce the right hand side for all `BigDecimal` operations (#1598).
+* Combining multiple `**` arguments containing duplicate keys produced
+  an incorrect hash. This has now been fixed (#1469).
 
 Changes:
 

spec/ruby/language/method_spec.rb

@@ -574,6 +574,7 @@ def m(**k) k end
       m(a: 1, b: 2).should == { a: 1, b: 2 }
       m(*[]).should == {}
       m(**{}).should == {}
+      m(**{a: 1, b: 2}, **{a: 4, c: 7}).should == { a: 4, b: 2, c: 7 }
       lambda { m(2) }.should raise_error(ArgumentError)
     end
 

src/main/java/org/truffleruby/core/hash/BucketsStrategy.java

@@ -60,6 +60,7 @@ public static DynamicObject create(RubyContext context, Collection<KeyValue> ent
 
             final int index = BucketsStrategy.getBucketIndex(hashed, newEntries.length);
             Entry bucketEntry = newEntries[index];
+            boolean duplicateKey = false;
 
             if (bucketEntry == null) {
                 newEntries[index] = newEntry;
@@ -72,47 +73,33 @@ && areKeysEqual(context, bucketEntry.getKey(), key, byIdentity)) {
                         bucketEntry.setValue(entry.getValue());
 
                         actualSize--;
-
-                        if (bucketEntry.getPreviousInSequence() != null) {
-                            bucketEntry.getPreviousInSequence().setNextInSequence(bucketEntry.getNextInSequence());
-                        }
-
-                        if (bucketEntry.getNextInSequence() != null) {
-                            bucketEntry.getNextInSequence().setPreviousInSequence(bucketEntry.getPreviousInSequence());
-                        }
-
-                        if (bucketEntry == lastInSequence) {
-                            lastInSequence = bucketEntry.getPreviousInSequence();
-                        }
-
-                        // We wasted by allocating newEntry, but never mind
-                        newEntry = bucketEntry;
-                        previousInBucket = null;
-
+                        duplicateKey = true;
                         break;
                     }
 
                     previousInBucket = bucketEntry;
                     bucketEntry = bucketEntry.getNextInLookup();
                 }
 
-                if (previousInBucket != null) {
+                if (!duplicateKey && previousInBucket != null) {
                     previousInBucket.setNextInLookup(newEntry);
                 }
             }
 
-            if (firstInSequence == null) {
-                firstInSequence = newEntry;
-            }
+            if (!duplicateKey) {
+                if (firstInSequence == null) {
+                    firstInSequence = newEntry;
+                }
 
-            if (lastInSequence != null) {
-                lastInSequence.setNextInSequence(newEntry);
-            }
+                if (lastInSequence != null) {
+                    lastInSequence.setNextInSequence(newEntry);
+                }
 
-            newEntry.setPreviousInSequence(lastInSequence);
-            newEntry.setNextInSequence(null);
+                newEntry.setPreviousInSequence(lastInSequence);
+                newEntry.setNextInSequence(null);
 
-            lastInSequence = newEntry;
+                lastInSequence = newEntry;
+            }
         }
 
         final DynamicObject nil = context.getCoreLibrary().getNil();