[GR-26781] Implement Hash.ruby2_keywords_hash{,?} and **kwargs accept non-Symbols keys in 2.7

PullRequest: truffleruby/2215

Author: eregon

CHANGELOG.md

@@ -31,6 +31,7 @@ Compatibility:
 * Implemented `BigDecimal` as C extension to improve compatibility.
 * Comment lines can be placed between fluent dot now (#2004, @ssnickolay).
 * Implemented `rb_make_exception`.
+* `**kwargs` now accept non-Symbol keys like Ruby 2.7.
 
 Performance:
 

spec/ruby/core/hash/ruby2_keywords_hash_spec.rb

@@ -0,0 +1,47 @@
+require_relative '../../spec_helper'
+require_relative 'fixtures/classes'
+
+ruby_version_is "2.7" do
+  describe "Hash.ruby2_keywords_hash?" do
+    it "returns false if the Hash is not a keywords Hash" do
+      Hash.ruby2_keywords_hash?({}).should == false
+    end
+
+    it "returns true if the Hash is a keywords Hash marked by Module#ruby2_keywords" do
+      obj = Class.new {
+        ruby2_keywords def m(*args)
+          args.last
+        end
+      }.new
+      Hash.ruby2_keywords_hash?(obj.m(a: 1)).should == true
+    end
+
+    it "raises TypeError for non-Hash" do
+      -> { Hash.ruby2_keywords_hash?(nil) }.should raise_error(TypeError)
+    end
+  end
+
+  describe "Hash.ruby2_keywords_hash" do
+    it "returns a copy of a Hash and marks the copy as a keywords Hash" do
+      h = {a: 1}.freeze
+      kw = Hash.ruby2_keywords_hash(h)
+      Hash.ruby2_keywords_hash?(h).should == false
+      Hash.ruby2_keywords_hash?(kw).should == true
+      kw.should == h
+    end
+
+    it "returns an instance of the subclass if called on an instance of a subclass of Hash" do
+      h = HashSpecs::MyHash.new
+      h[:a] = 1
+      kw = Hash.ruby2_keywords_hash(h)
+      kw.class.should == HashSpecs::MyHash
+      Hash.ruby2_keywords_hash?(h).should == false
+      Hash.ruby2_keywords_hash?(kw).should == true
+      kw.should == h
+    end
+
+    it "raises TypeError for non-Hash" do
+      -> { Hash.ruby2_keywords_hash(nil) }.should raise_error(TypeError)
+    end
+  end
+end

spec/ruby/language/block_spec.rb

@@ -873,14 +873,20 @@ def m(a) yield a end
     end.call(1, 2, 3).should == [[], 1, 2, 3]
   end
 
-  it "are required" do
+  it "are required for a lambda" do
     -> {
       -> *a, b do
         [a, b]
       end.call
     }.should raise_error(ArgumentError)
   end
 
+  it "are assigned to nil when not enough arguments are given to a proc" do
+    proc do |a, *b, c|
+      [a, b, c]
+    end.call.should == [nil, [], nil]
+  end
+
   describe "with required args" do
 
     it "gathers remaining args in the splat" do

spec/tags/core/hash/ruby2_keywords_hash_tags.txt

@@ -0,0 +1 @@
+fails:Hash.ruby2_keywords_hash? returns true if the Hash is a keywords Hash marked by Module#ruby2_keywords

spec/tags/language/hash_tags.txt

@@ -1,13 +1,7 @@
 fails:A method assigns local variables from method parameters for definition 'def m() end'
 fails:A method assigns local variables from method parameters for definition 'def m(*a) a end'
-fails:A method assigns local variables from method parameters for definition 'def m(a=1, **) a end'
-fails:A method assigns local variables from method parameters for definition 'def m(*a, **) a end'
-fails:A method assigns local variables from method parameters for definition 'def m(*, **k) k end'
 fails:A method assigns local variables from method parameters for definition 'def m(a = nil, **k) [a, k] end'
 fails:A method assigns local variables from method parameters for definition 'def m(*a, **k) [a, k] end'
-fails:A method assigns local variables from method parameters for definition 'def m(**k); k end;'
-fails:A method assigns local variables from method parameters for definition 'def m(a:, **) a end'
-fails:A method assigns local variables from method parameters for definition 'def m(a:, **k) [a, k] end'
 fails:A method assigns local variables from method parameters for definition 'def m(a, **nil); a end;'
 fails:A method when passing an empty keyword splat to a method that does not accept keywords for definition 'def m(*a); a; end'
 fails:A method when passing an empty keyword splat to a method that does not accept keywords for definition 'def m(a); a; end'

spec/tags/language/method_tags.txt

@@ -173,6 +173,23 @@ public boolean isSmallArrayOfPairs(Object[] args, RubyLanguage language) {
 
     }
 
+    @CoreMethod(names = "ruby2_keywords_hash?", onSingleton = true, required = 1)
+    public abstract static class IsRuby2KeywordsHashNode extends CoreMethodArrayArgumentsNode {
+        @Specialization
+        protected boolean isRuby2KeywordsHash(RubyHash hash) {
+            return hash.ruby2_keywords;
+        }
+    }
+
+    @Primitive(name = "hash_mark_ruby2_keywords")
+    public abstract static class HashMarkRuby2KeywordsNode extends CoreMethodArrayArgumentsNode {
+        @Specialization
+        protected RubyHash markRuby2Keywords(RubyHash hash) {
+            hash.ruby2_keywords = true;
+            return hash;
+        }
+    }
+
     @ImportStatic(HashGuards.class)
     public abstract static class HashLookupOrExecuteDefaultNode extends RubyContextNode {
 

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

@@ -29,6 +29,7 @@ public class RubyHash extends RubyDynamicObject implements ObjectGraphNode {
     public Entry firstInSequence;
     public Entry lastInSequence;
     public boolean compareByIdentity;
+    public boolean ruby2_keywords = false;
 
     public RubyHash(
             RubyClass rubyClass,

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

@@ -9,6 +9,7 @@
  */
 package org.truffleruby.language.arguments;
 
+import com.oracle.truffle.api.CompilerDirectives;
 import org.truffleruby.RubyLanguage;
 import org.truffleruby.collections.BiConsumerNode;
 import org.truffleruby.core.hash.RubyHash;
@@ -42,9 +43,6 @@ public CheckKeywordArityNode(RubyLanguage language, Arity arity, RubyNode body)
         this.arity = arity;
         this.body = body;
         this.readUserKeywordsHashNode = new ReadUserKeywordsHashNode(arity.getRequired());
-        this.checkKeywordArgumentsNode = new CheckKeywordArgumentsNode(language, arity);
-        this.eachKeyNode = EachKeyValueNode.create();
-
     }
 
     @Override
@@ -74,47 +72,59 @@ private void checkArity(VirtualFrame frame) {
             throw new RaiseException(getContext(), coreExceptions().argumentError(given, arity.getRequired(), this));
         }
 
-        if (keywordArguments != null) {
-            receivedKeywordsProfile.enter();
-            eachKeyNode.executeEachKeyValue(frame, keywordArguments, checkKeywordArgumentsNode, null);
+        if (!arity.hasKeywordsRest() && keywordArguments != null) {
+            checkKeywordArguments(frame, keywordArguments);
+        }
+    }
+
+    void checkKeywordArguments(VirtualFrame frame, RubyHash keywordArguments) {
+        if (eachKeyNode == null) {
+            CompilerDirectives.transferToInterpreterAndInvalidate();
+            eachKeyNode = insert(EachKeyValueNode.create());
         }
+        if (checkKeywordArgumentsNode == null) {
+            CompilerDirectives.transferToInterpreterAndInvalidate();
+            checkKeywordArgumentsNode = insert(new CheckKeywordArgumentsNode(getLanguage(), arity));
+        }
+        eachKeyNode.executeEachKeyValue(frame, keywordArguments, checkKeywordArgumentsNode, null);
     }
 
     private static class CheckKeywordArgumentsNode extends RubyContextNode implements BiConsumerNode {
 
-        private final boolean checkAllowedKeywords;
         private final boolean doesNotAcceptExtraArguments;
         private final int required;
         @CompilationFinal(dimensions = 1) private final RubySymbol[] allowedKeywords;
 
         private final ConditionProfile isSymbolProfile = ConditionProfile.create();
         private final BranchProfile tooManyKeywordsProfile = BranchProfile.create();
-        private final BranchProfile unknownKeywordProfile;
+        private final BranchProfile unknownKeywordProfile = BranchProfile.create();
 
         public CheckKeywordArgumentsNode(RubyLanguage language, Arity arity) {
-            checkAllowedKeywords = !arity.hasKeywordsRest();
+            assert !arity.hasKeywordsRest();
             doesNotAcceptExtraArguments = !arity.hasRest() && arity.getOptional() == 0;
             required = arity.getRequired();
-            allowedKeywords = checkAllowedKeywords ? keywordsAsSymbols(language, arity) : null;
-            unknownKeywordProfile = checkAllowedKeywords ? BranchProfile.create() : null;
+            allowedKeywords = keywordsAsSymbols(language, arity);
         }
 
         @Override
         public void accept(VirtualFrame frame, Object key, Object value, Object state) {
             if (isSymbolProfile.profile(key instanceof RubySymbol)) {
-                if (checkAllowedKeywords && !keywordAllowed(key)) {
+                if (!keywordAllowed(key)) {
                     unknownKeywordProfile.enter();
                     throw new RaiseException(
                             getContext(),
                             coreExceptions().argumentErrorUnknownKeyword((RubySymbol) key, this));
                 }
             } else {
-                final int given = RubyArguments.getArgumentsCount(frame); // -1 for keyword hash, +1 for reject Hash with non-Symbol key
+                // the Hash would be split and a reject Hash be created to hold non-Symbols when there is no **kwrest parameter,
+                // so we need to check if an extra argument is allowed
+                final int given = RubyArguments.getArgumentsCount(frame); // -1 for keyword hash, +1 for reject Hash with non-Symbol keys
                 if (doesNotAcceptExtraArguments && given > required) {
                     tooManyKeywordsProfile.enter();
                     throw new RaiseException(getContext(), coreExceptions().argumentError(given, required, this));
                 }
             }
+
         }
 
         @ExplodeLoop