[Truffle] Hash#merge! works except for an ordering issue.

spec/truffle/tags/core/hash/merge_tags.txt

@@ -1,15 +1,4 @@
-fails:Hash#merge returns a new hash by combining self with the contents of other
 fails:Hash#merge sets any duplicate key to the value of block if passed a block
 fails:Hash#merge tries to convert the passed argument to a hash using #to_hash
-fails:Hash#merge does not call to_hash on hash subclasses
-fails:Hash#merge returns subclass instance for subclasses
 fails:Hash#merge processes entries with same order as each()
-fails:Hash#merge! adds the entries from other, overwriting duplicate keys. Returns self
-fails:Hash#merge! sets any duplicate key to the value of block if passed a block
-fails:Hash#merge! tries to convert the passed argument to a hash using #to_hash
-fails:Hash#merge! does not call to_hash on hash subclasses
 fails:Hash#merge! processes entries with same order as merge()
-fails:Hash#merge! raises a RuntimeError on a frozen instance that is modified
-fails:Hash#merge! checks frozen status before coercing an object with #to_hash
-fails:Hash#merge! raises a RuntimeError on a frozen instance that would not be modified
-fails:Hash#merge! does not raise an exception if changing the value of an existing key during iteration

truffle/src/main/java/org/jruby/truffle/nodes/core/CoreMethod.java

@@ -40,6 +40,9 @@
 
     boolean needsSelf() default true;
 
+    // TODO CS 3-Mar-15 needsSelf() gets ignored in some cases - see CoreMethodNodeManager#addMethod
+    boolean reallyDoesNeedSelf() default false;
+
     int required() default 0;
 
     int optional() default 0;

truffle/src/main/java/org/jruby/truffle/nodes/core/CoreMethodNodeManager.java

@@ -101,7 +101,7 @@ private static void addMethod(RubyClass rubyObjectClass, MethodDetails methodDet
 
         // Do not use needsSelf=true in module functions, it is either the module/class or the instance.
         // Usage of needsSelf is quite rare for singleton methods (except constructors).
-        final boolean needsSelf = !anno.isModuleFunction() && !anno.onSingleton() && anno.needsSelf();
+        final boolean needsSelf = !anno.isModuleFunction() && !anno.onSingleton() && anno.needsSelf() || anno.reallyDoesNeedSelf();
 
         final RubyRootNode rootNode = makeGenericMethod(context, methodDetails, needsSelf);
 

truffle/src/main/java/org/jruby/truffle/nodes/core/HashNodes.java

@@ -102,7 +102,7 @@ public boolean equalNonHash(RubyHash a, Object b) {
 
     }
 
-    @CoreMethod(names = "[]", onSingleton = true, argumentsAsArray = true)
+    @CoreMethod(names = "[]", onSingleton = true, argumentsAsArray = true, reallyDoesNeedSelf = true)
     public abstract static class ConstructNode extends HashCoreMethodNode {
 
         private final BranchProfile singleObject = BranchProfile.create();
@@ -112,6 +112,7 @@ public abstract static class ConstructNode extends HashCoreMethodNode {
         private final BranchProfile largePackedArray = BranchProfile.create();
         private final BranchProfile otherArray = BranchProfile.create();
         private final BranchProfile singleOther = BranchProfile.create();
+        private final BranchProfile otherHash = BranchProfile.create();
         private final BranchProfile keyValues = BranchProfile.create();
 
         public ConstructNode(RubyContext context, SourceSection sourceSection) {
@@ -124,7 +125,7 @@ public ConstructNode(ConstructNode prev) {
 
         @ExplodeLoop
         @Specialization
-        public RubyHash construct(Object[] args) {
+        public RubyHash construct(RubyClass hashClass, Object[] args) {
             if (args.length == 1) {
                 singleObject.enter();
 
@@ -171,7 +172,7 @@ public RubyHash construct(Object[] args) {
                                 }
                             }
 
-                            return new RubyHash(getContext().getCoreLibrary().getHashClass(), null, null, newStore, size, null);
+                            return new RubyHash(hashClass, null, null, newStore, size, null);
                         } else {
                             largePackedArray.enter();
 
@@ -198,12 +199,16 @@ public RubyHash construct(Object[] args) {
                                 keyValues.add(new KeyValue(pairStore[0], pairStore[1]));
                             }
 
-                            return HashOperations.verySlowFromEntries(getContext(), keyValues);
+                            return HashOperations.verySlowFromEntries(hashClass, keyValues);
                         }
                     } else {
                         otherArray.enter();
                         throw new UnsupportedOperationException("other array");
                     }
+                } else if (arg instanceof RubyHash) {
+                    otherHash.enter();
+
+                    return HashOperations.verySlowFromEntries(hashClass, HashOperations.verySlowToKeyValues((RubyHash) arg));
                 } else {
                     singleOther.enter();
                     throw new UnsupportedOperationException("single other");
@@ -217,7 +222,7 @@ public RubyHash construct(Object[] args) {
                     entries.add(new KeyValue(args[n], args[n + 1]));
                 }
 
-                return HashOperations.verySlowFromEntries(getContext(), entries);
+                return HashOperations.verySlowFromEntries(hashClass, entries);
             }
         }
 
@@ -947,7 +952,7 @@ public RubyHash mergePackedArrayNull(RubyHash hash, RubyHash other) {
             final Object[] store = (Object[]) hash.getStore();
             final Object[] copy = Arrays.copyOf(store, HashOperations.SMALL_HASH_SIZE * 2);
 
-            return new RubyHash(getContext().getCoreLibrary().getHashClass(), hash.getDefaultBlock(), hash.getDefaultValue(), copy, hash.getSize(), null);
+            return new RubyHash(hash.getLogicalClass(), hash.getDefaultBlock(), hash.getDefaultValue(), copy, hash.getSize(), null);
         }
 
         @ExplodeLoop
@@ -987,14 +992,14 @@ public RubyHash mergePackedArrayPackedArray(VirtualFrame frame, RubyHash hash, R
 
             if (mergeFromACount == 0) {
                 nothingFromFirstProfile.enter();
-                return new RubyHash(getContext().getCoreLibrary().getHashClass(), hash.getDefaultBlock(), hash.getDefaultValue(), Arrays.copyOf(storeB, HashOperations.SMALL_HASH_SIZE * 2), storeBSize, null);
+                return new RubyHash(hash.getLogicalClass(), hash.getDefaultBlock(), hash.getDefaultValue(), Arrays.copyOf(storeB, HashOperations.SMALL_HASH_SIZE * 2), storeBSize, null);
             }
 
             considerNothingFromSecondProfile.enter();
 
             if (mergeFromACount == storeB.length) {
                 nothingFromSecondProfile.enter();
-                return new RubyHash(getContext().getCoreLibrary().getHashClass(), hash.getDefaultBlock(), hash.getDefaultValue(), Arrays.copyOf(storeB, HashOperations.SMALL_HASH_SIZE * 2), storeBSize, null);
+                return new RubyHash(hash.getLogicalClass(), hash.getDefaultBlock(), hash.getDefaultValue(), Arrays.copyOf(storeB, HashOperations.SMALL_HASH_SIZE * 2), storeBSize, null);
             }
 
             considerResultIsSmallProfile.enter();
@@ -1022,7 +1027,7 @@ public RubyHash mergePackedArrayPackedArray(VirtualFrame frame, RubyHash hash, R
                     index += 2;
                 }
 
-                return new RubyHash(getContext().getCoreLibrary().getHashClass(), hash.getDefaultBlock(), hash.getDefaultValue(), merged, mergedSize, null);
+                return new RubyHash(hash.getLogicalClass(), hash.getDefaultBlock(), hash.getDefaultValue(), merged, mergedSize, null);
             }
 
             CompilerDirectives.transferToInterpreter();
@@ -1032,7 +1037,7 @@ public RubyHash mergePackedArrayPackedArray(VirtualFrame frame, RubyHash hash, R
 
         @Specialization
         public RubyHash mergeBucketsBuckets(RubyHash hash, RubyHash other) {
-            final RubyHash merged = new RubyHash(getContext().getCoreLibrary().getHashClass(), null, null, new Entry[HashOperations.capacityGreaterThan(hash.getSize() + other.getSize())], 0, null);
+            final RubyHash merged = new RubyHash(hash.getLogicalClass(), null, null, new Entry[HashOperations.capacityGreaterThan(hash.getSize() + other.getSize())], 0, null);
 
             int size = 0;
 

truffle/src/main/java/org/jruby/truffle/runtime/hash/HashOperations.java

@@ -13,6 +13,7 @@
 import org.jruby.truffle.nodes.RubyNode;
 import org.jruby.truffle.runtime.DebugOperations;
 import org.jruby.truffle.runtime.RubyContext;
+import org.jruby.truffle.runtime.core.RubyClass;
 import org.jruby.truffle.runtime.core.RubyHash;
 import org.jruby.truffle.runtime.core.RubyString;
 import org.jruby.util.cli.Options;
@@ -38,11 +39,15 @@ public static int capacityGreaterThan(int size) {
         return CAPACITIES[CAPACITIES.length - 1];
     }
 
-    @CompilerDirectives.TruffleBoundary
     public static RubyHash verySlowFromEntries(RubyContext context, List<KeyValue> entries) {
+        return verySlowFromEntries(context.getCoreLibrary().getHashClass(), entries);
+    }
+
+    @CompilerDirectives.TruffleBoundary
+    public static RubyHash verySlowFromEntries(RubyClass hashClass, List<KeyValue> entries) {
         RubyNode.notDesignedForCompilation();
 
-        final RubyHash hash = new RubyHash(context.getCoreLibrary().getHashClass(), null, null, null, 0, null);
+        final RubyHash hash = new RubyHash(hashClass, null, null, null, 0, null);
         verySlowSetKeyValues(hash, entries);
         return hash;
     }

truffle/src/main/ruby/core.rb

@@ -40,6 +40,7 @@
 require_relative 'core/rubinius/common/exception'
 require_relative 'core/rubinius/common/undefined'
 require_relative 'core/rubinius/common/type'
+require_relative 'core/rubinius/common/hash'
 require_relative 'core/rubinius/common/array'
 require_relative 'core/rubinius/common/kernel'
 require_relative 'core/rubinius/common/identity_map'

truffle/src/main/ruby/core/hash.rb

@@ -20,4 +20,17 @@ def to_hash
     self
   end
 
+  # Implementation of a fundamental Rubinius method that allows their Hash
+  # implementation to work. We probably want to remove uses of this in the long
+  # term, as it creates objects which already exist for them and we have to
+  # create, but for now it allows us to use more Rubinius code unmodified.
+
+  KeyValue = Struct.new(:key, :value)
+
+  def each_item
+    each do |key, value|
+      yield KeyValue.new(key, value)
+    end
+  end
+
 end

truffle/src/main/ruby/core/rubinius/common/float.rb

@@ -24,7 +24,7 @@
 # OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-# Only part of Rubinius' kernel.rb
+# Only part of Rubinius' float.rb
 
 class Float < Numeric
 

truffle/src/main/ruby/core/rubinius/common/hash.rb

@@ -0,0 +1,60 @@
+# Copyright (c) 2007-2014, Evan Phoenix and contributors
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are met:
+#
+# * Redistributions of source code must retain the above copyright notice, this
+#   list of conditions and the following disclaimer.
+# * Redistributions in binary form must reproduce the above copyright notice
+#   this list of conditions and the following disclaimer in the documentation
+#   and/or other materials provided with the distribution.
+# * Neither the name of Rubinius nor the names of its contributors
+#   may be used to endorse or promote products derived from this software
+#   without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE
+# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+# Only part of Rubinius' hash.rb
+
+class Hash
+
+  alias_method :store, :[]=
+
+  # Used internally to get around subclasses redefining #[]=
+  alias_method :__store__, :[]=
+
+  def merge!(other)
+    Rubinius.check_frozen
+
+    other = Rubinius::Type.coerce_to other, Hash, :to_hash
+
+    if block_given?
+      other.each_item do |item|
+        key = item.key
+        if key? key
+          __store__ key, yield(key, self[key], item.value)
+        else
+          __store__ key, item.value
+        end
+      end
+    else
+      other.each_item do |item|
+        __store__ item.key, item.value
+      end
+    end
+    self
+  end
+
+  alias_method :update, :merge!
+
+end