Fix error message when there are missing keyword arguments

Author: andrykonchin

spec/ruby/language/keyword_arguments_spec.rb

@@ -61,6 +61,23 @@ def m(*a, kw:)
       -> { m(kw: 1, a: 1, b: 2, c: 3) }.should raise_error(ArgumentError, 'unknown keywords: :a, :b, :c')
     end
 
+    it "raises ArgumentError exception when required keyword argument is not passed" do
+      def m(a:, b:, c:)
+        [a, b, c]
+      end
+
+      -> { m(a: 1, b: 2) }.should raise_error(ArgumentError, /missing keyword: :c/)
+      -> { m() }.should raise_error(ArgumentError, /missing keywords: :a, :b, :c/)
+    end
+
+    it "raises ArgumentError for missing keyword arguments even if there are extra ones" do
+      def m(a:)
+        a
+      end
+
+      -> { m(b: 1) }.should raise_error(ArgumentError, /missing keyword: :a/)
+    end
+
     it "handle * and ** at the same call site" do
       def m(*a)
         a

spec/ruby/language/method_spec.rb

@@ -571,6 +571,13 @@ def m(a:) a end
       end
     end
 
+    evaluate <<-ruby do
+        def m(a:, **kw) [a, kw] end
+      ruby
+
+      -> { m(b: 1) }.should raise_error(ArgumentError)
+    end
+
     evaluate <<-ruby do
         def m(a: 1) a end
       ruby

spec/ruby/language/proc_spec.rb

@@ -237,4 +237,11 @@
       end
     end
   end
+
+  describe "taking |required keyword arguments, **kw| arguments" do
+    it "raises ArgumentError for missing required argument" do
+      p = proc { |a:, **kw| [a, kw] }
+      -> { p.call() }.should raise_error(ArgumentError)
+    end
+  end
 end

spec/tags/language/keyword_arguments_tags.txt

@@ -0,0 +1 @@
+fails:Keyword arguments raises ArgumentError for missing keyword arguments even if there are extra ones

src/main/java/org/truffleruby/core/exception/CoreExceptions.java

@@ -207,8 +207,18 @@ public RubyException argumentErrorInvalidRadix(int radix, Node currentNode) {
     }
 
     @TruffleBoundary
-    public RubyException argumentErrorMissingKeyword(String name, Node currentNode) {
-        return argumentError(StringUtils.format("missing keyword: %s", name), currentNode);
+    public RubyException argumentErrorMissingKeywords(Object[] keys, Node currentNode) {
+        if (keys.length == 1) {
+            return argumentError("missing keyword: " + inspect(keys[0]), currentNode);
+        }
+
+        final String[] names = new String[keys.length];
+
+        for (int i = 0; i < keys.length; i++) {
+            names[i] = inspect(keys[i]);
+        }
+
+        return argumentError("missing keywords: " + String.join(", ", names), currentNode);
     }
 
     @TruffleBoundary

src/main/java/org/truffleruby/language/arguments/MissingKeywordArgumentNode.java

@@ -9,22 +9,77 @@
  */
 package org.truffleruby.language.arguments;
 
+import org.truffleruby.RubyLanguage;
+import org.truffleruby.core.exception.RubyException;
+import org.truffleruby.core.hash.RubyHash;
+import org.truffleruby.core.hash.library.HashStoreLibrary;
+import org.truffleruby.core.symbol.RubySymbol;
 import org.truffleruby.language.RubyContextSourceNode;
 import org.truffleruby.language.control.RaiseException;
 
+import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
+import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.frame.VirtualFrame;
+import org.truffleruby.language.methods.Arity;
 
-public class MissingKeywordArgumentNode extends RubyContextSourceNode {
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
 
-    private final String name;
+public class MissingKeywordArgumentNode extends RubyContextSourceNode {
+    @CompilationFinal(dimensions = 1) private final RubySymbol[] requiredKeywords;
+    @Child private ReadUserKeywordsHashNode readUserKeywordsHashNode;
+    @Child private HashStoreLibrary hashes;
 
-    public MissingKeywordArgumentNode(String name) {
-        this.name = name;
+    public MissingKeywordArgumentNode(RubyLanguage language, Arity arity) {
+        requiredKeywords = requiredKeywordsAsSymbols(language, arity);
+        readUserKeywordsHashNode = new ReadUserKeywordsHashNode();
+        hashes = insert(HashStoreLibrary.createDispatched());
     }
 
     @Override
     public Object execute(VirtualFrame frame) {
-        throw new RaiseException(getContext(), coreExceptions().argumentErrorMissingKeyword(name, this));
+        RubyHash actualKeywords = readUserKeywordsHashNode.execute(frame);
+        throw new RaiseException(getContext(), missingKeywordsError(actualKeywords));
+    }
+
+    private RubySymbol[] requiredKeywordsAsSymbols(RubyLanguage language, Arity arity) {
+        final String[] requiredKeywords = arity.getRequiredKeywordArguments();
+        final RubySymbol[] symbols = new RubySymbol[requiredKeywords.length];
+
+        for (int i = 0; i < requiredKeywords.length; i++) {
+            symbols[i] = language.getSymbol(requiredKeywords[i]);
+        }
+
+        return symbols;
     }
 
+    @TruffleBoundary
+    private RubyException missingKeywordsError(RubyHash actualKeywords) {
+        final Object[] missingKeywords = findMissingKeywordArguments(actualKeywords);
+        return coreExceptions().argumentErrorMissingKeywords(missingKeywords, this);
+    }
+
+    @TruffleBoundary
+    @SuppressWarnings("unchecked")
+    private Object[] findMissingKeywordArguments(RubyHash actualKeywords) {
+        if (actualKeywords == null) {
+            return requiredKeywords;
+        }
+
+        final ArrayList<Object> actualKeywordsAsList = new ArrayList<>();
+        hashes.eachEntry(
+                actualKeywords.store,
+                actualKeywords,
+                (index, key, value, state) -> ((ArrayList<Object>) state).add(key),
+                actualKeywordsAsList);
+
+        final List<RubySymbol> requiredKeywordsAsList = Arrays.asList(requiredKeywords);
+        final List<RubySymbol> missingKeywords = requiredKeywordsAsList.stream()
+                .filter(k -> !actualKeywordsAsList.contains(k))
+                .collect(Collectors.toList());
+
+        return missingKeywords.toArray();
+    }
 }

src/main/java/org/truffleruby/language/arguments/ReadKeywordArgumentNode.java

@@ -9,6 +9,7 @@
  */
 package org.truffleruby.language.arguments;
 
+import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.frame.Frame;
 import org.truffleruby.collections.PEBiFunction;
 import com.oracle.truffle.api.dsl.ImportStatic;
@@ -23,6 +24,8 @@
 import org.truffleruby.language.RubyNode;
 
 import com.oracle.truffle.api.frame.VirtualFrame;
+import org.truffleruby.language.RubyRootNode;
+import org.truffleruby.language.methods.Arity;
 
 /** Read a single keyword argument or execute its default value expression if missing */
 @ImportStatic(HashGuards.class)
@@ -52,7 +55,7 @@ protected ReadKeywordArgumentNode(RubySymbol name, RubyNode defaultValue) {
 
     @Specialization(guards = "hash == null")
     protected Object nullHash(VirtualFrame frame, RubyHash hash) {
-        return defaultValue.execute(frame);
+        return getDefaultValue().execute(frame);
     }
 
     @Specialization(guards = "hash != null", limit = "hashStrategyLimit()")
@@ -72,6 +75,20 @@ public Object accept(Frame frame, Object hash, Object key) {
         // This only works if the library is always cached and does not reach the limit.
         // Since this node is never uncached, and the limit is >= number of strategies, it should hold.
         final VirtualFrame virtualFrame = (VirtualFrame) frame;
-        return defaultValue.execute(virtualFrame);
+        return getDefaultValue().execute(virtualFrame);
+    }
+
+    RubyNode getDefaultValue() {
+        if (defaultValue == null) {
+            // This isn't a true default value - it's a marker to say there isn't one.
+            // This actually makes sense; the semantic action of executing this node is to report an error,
+            // and we do the same thing.
+            CompilerDirectives.transferToInterpreterAndInvalidate();
+            RubyRootNode rootNode = (RubyRootNode) getRootNode();
+            Arity arity = rootNode.getSharedMethodInfo().getArity();
+            defaultValue = insert(new MissingKeywordArgumentNode(getLanguage(), arity));
+        }
+
+        return defaultValue;
     }
 }

src/main/java/org/truffleruby/language/methods/Arity.java

@@ -33,13 +33,14 @@ public final class Arity {
     private final boolean allKeywordsOptional;
     private final boolean hasKeywordsRest;
     private final String[] keywordArguments;
+    private final int requiredKeywordArgumentsCount;
     /** During parsing we cannot know if this Arity object belongs to proc or to lambda. So we calculate the arity
      * number for both cases and provide a ProcType-dependent interface. */
     private final int arityNumber;
     private final int procArityNumber;
 
     public Arity(int preRequired, int optional, boolean hasRest) {
-        this(preRequired, optional, hasRest, 0, NO_KEYWORDS, true, false);
+        this(preRequired, optional, hasRest, 0, NO_KEYWORDS, 0, false);
     }
 
 
@@ -49,14 +50,17 @@ public Arity(
             boolean hasRest,
             int postRequired,
             String[] keywordArguments,
-            boolean allKeywordsOptional,
+            int requiredKeywordArgumentsCount,
             boolean hasKeywordsRest) {
         this.preRequired = preRequired;
         this.optional = optional;
         this.hasRest = hasRest;
         this.postRequired = postRequired;
+        // Required keywords are located at the beginning of the `keywordArguments` array.
+        // So we can specify them with only one `int` field (`requiredKeywordArgumentsCount`).
         this.keywordArguments = keywordArguments;
-        this.allKeywordsOptional = allKeywordsOptional;
+        this.requiredKeywordArgumentsCount = requiredKeywordArgumentsCount;
+        this.allKeywordsOptional = requiredKeywordArgumentsCount == 0;
         this.hasKeywordsRest = hasKeywordsRest;
         this.arityNumber = computeArityNumber(false);
         this.procArityNumber = computeArityNumber(true);
@@ -71,7 +75,7 @@ public Arity withRest(boolean hasRest) {
                 hasRest,
                 postRequired,
                 keywordArguments,
-                allKeywordsOptional,
+                requiredKeywordArgumentsCount,
                 hasKeywordsRest);
     }
 
@@ -82,7 +86,7 @@ public Arity consumingFirstRequired() {
                 hasRest,
                 postRequired,
                 keywordArguments,
-                allKeywordsOptional,
+                requiredKeywordArgumentsCount,
                 hasKeywordsRest);
     }
 
@@ -117,6 +121,10 @@ public boolean hasKeywords() {
         return keywordArguments.length != 0;
     }
 
+    public boolean hasRequiredKeywords() {
+        return !allKeywordsOptional;
+    }
+
     public boolean hasKeywordsRest() {
         return hasKeywordsRest;
     }
@@ -147,6 +155,16 @@ public String[] getKeywordArguments() {
         return keywordArguments;
     }
 
+    public String[] getRequiredKeywordArguments() {
+        final String[] requiredKeywords = new String[requiredKeywordArgumentsCount];
+
+        for (int i = 0; i < requiredKeywords.length; i++) {
+            requiredKeywords[i] = keywordArguments[i];
+        }
+
+        return requiredKeywords;
+    }
+
     public ArgumentDescriptor[] toAnonymousArgumentDescriptors() {
         List<ArgumentDescriptor> descs = new ArrayList<>();
 

src/main/java/org/truffleruby/parser/LoadArgumentsTranslator.java

@@ -27,7 +27,6 @@
 import org.truffleruby.language.arguments.ArrayIsAtLeastAsLargeAsNode;
 import org.truffleruby.language.arguments.CheckNoKeywordArgumentsNode;
 import org.truffleruby.language.arguments.MissingArgumentBehavior;
-import org.truffleruby.language.arguments.MissingKeywordArgumentNode;
 import org.truffleruby.language.arguments.ReadKeywordArgumentNode;
 import org.truffleruby.language.arguments.ReadKeywordRestArgumentNode;
 import org.truffleruby.language.arguments.ReadOptionalArgumentNode;
@@ -273,9 +272,7 @@ public RubyNode visitKeywordArgNode(KeywordArgParseNode node) {
 
         final RubyNode defaultValue;
         if (asgnNode.getValueNode() instanceof RequiredKeywordArgumentValueParseNode) {
-            /* This isn't a true default value - it's a marker to say there isn't one. This actually makes sense; the
-             * semantic action of executing this node is to report an error, and we do the same thing. */
-            defaultValue = new MissingKeywordArgumentNode(name);
+            defaultValue = null;
         } else {
             defaultValue = translateNodeOrNil(sourceSection, asgnNode.getValueNode());
         }

src/main/java/org/truffleruby/parser/ast/ArgsParseNode.java

@@ -34,6 +34,7 @@
 
 package org.truffleruby.parser.ast;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
@@ -125,24 +126,38 @@ public ArgsParseNode(
 
     private Arity createArity() {
         final String[] keywordArguments;
-        boolean allKeywordsOptional = true;
+        final int requiredKeywordArgumentsCount;
 
         if (getKeywordCount() > 0) {
+            final List<String> requiredKeywords = new ArrayList<>();
+            final List<String> optionalKeywords = new ArrayList<>();
+
             final ParseNode[] keywordNodes = getKeywords().children();
             final int keywordsCount = keywordNodes.length;
-            keywordArguments = new String[keywordsCount];
 
             for (int i = 0; i < keywordsCount; i++) {
                 final KeywordArgParseNode kwarg = (KeywordArgParseNode) keywordNodes[i];
                 final AssignableParseNode assignableNode = kwarg.getAssignable();
                 if (!(assignableNode instanceof LocalAsgnParseNode || assignableNode instanceof DAsgnParseNode)) {
                     throw new UnsupportedOperationException("unsupported keyword arg " + kwarg);
                 }
-                keywordArguments[i] = ((INameNode) assignableNode).getName();
-                allKeywordsOptional &= !Helpers.isRequiredKeywordArgumentValueNode(assignableNode);
+
+                final String keyword = ((INameNode) assignableNode).getName();
+                if (Helpers.isRequiredKeywordArgumentValueNode(assignableNode)) {
+                    requiredKeywords.add(keyword);
+                } else {
+                    optionalKeywords.add(keyword);
+                }
             }
+
+            final List<String> keywords = new ArrayList<>(requiredKeywords);
+            keywords.addAll(optionalKeywords);
+
+            keywordArguments = keywords.toArray(String[]::new);
+            requiredKeywordArgumentsCount = requiredKeywords.size();
         } else {
             keywordArguments = Arity.NO_KEYWORDS;
+            requiredKeywordArgumentsCount = 0;
         }
 
         return new Arity(
@@ -151,7 +166,7 @@ private Arity createArity() {
                 hasRestArg(),
                 getPostCount(),
                 keywordArguments,
-                allKeywordsOptional,
+                requiredKeywordArgumentsCount,
                 hasKeyRest());
     }