Fixed remaining mri kwargs issues sans parsing bug (did not untag bec…

…ause of some JIT failures with kwargs)
jruby · Apr 9, 2015 · af2e4db · af2e4db
1 parent 971b87c
commit af2e4db
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 19 deletions.
diff --git a/core/src/main/java/org/jruby/RubyProc.java b/core/src/main/java/org/jruby/RubyProc.java
@@ -260,27 +260,41 @@ public static IRubyObject[] prepareArgs(ThreadContext context, Block.Type type,
         boolean isFixed = arity.isFixed();
         int required = arity.required();
         int actual = args.length;
+        boolean restKwargs = blockBody instanceof IRBlockBody && ((IRBlockBody) blockBody).getSignature().kwargs();
 
+        // FIXME: This is a hot mess.  restkwargs factors into destructing a single element array as well.  I just weaved it into this logic.
         // for procs and blocks, single array passed to multi-arg must be spread
-        if (arity != Arity.ONE_ARGUMENT &&  required != 0 &&
-                (isFixed || arity != Arity.OPTIONAL) &&
+        if ((arity != Arity.ONE_ARGUMENT &&  required != 0 && (isFixed || arity != Arity.OPTIONAL) || restKwargs) &&
                 actual == 1 && args[0].respondsTo("to_ary")) {
             args = args[0].convertToArray().toJavaArray();
             actual = args.length;
         }
 
-        // FIXME: This code is horrible so I named it poorly.  We add 1 if we have kwargs so we do not accidentally
-        // chomp it off of args array.  Much of this logic needs to be encapsulated as part of our transition from
-        // Arity to Signature.
-        int fudge = blockBody instanceof IRBlockBody && ((IRBlockBody) blockBody).getSignature().kwargs() ? 1 : 0;
+        // FIXME: NOTE IN THE BLOCKCAPALYPSE: I think we only care if there is any kwargs syntax at all and if so it is +1
+        // argument.  This ended up more complex because required() on signature adds +1 is required kwargs.  I suspect
+        // required() is used for two purposes and the +1 might be useful in some other way so I made it all work and
+        // after this we should clean this up (IRBlockBody and BlockBody are also messing with args[] so that should
+        // be part of this cleanup.
+
+        // We add one to our fill and adjust number of incoming args code when there are kwargs.  We subtract one
+        // if it happens to be requiredkwargs since required gets a +1.  This is horrible :)
+        int needsKwargs = blockBody instanceof IRBlockBody && ((IRBlockBody) blockBody).getSignature().kwargs() ?
+                1 - ((IRBlockBody) blockBody).getSignature().getRequiredKeywordCount() : 0;
 
         // fixed arity > 0 with mismatch needs a new args array
-        if (isFixed && required > 0 && required+fudge != actual) {
-            IRubyObject[] newArgs = Arrays.copyOf(args, required);
+        if (isFixed && required > 0 && required+needsKwargs != actual) {
+            IRubyObject[] newArgs = Arrays.copyOf(args, required+needsKwargs);
+
 
-            // pad with nil
-            if (required > actual) {
+            if (required > actual) {                      // Not enough required args pad.
                 Helpers.fillNil(newArgs, actual, required, context.runtime);
+                // ENEBO: what if we need kwargs here?
+            } else if (needsKwargs != 0) {
+                if (args.length < required+needsKwargs) { // Not enough args and we need an empty {} for kwargs processing.
+                    newArgs[newArgs.length - 1] = RubyHash.newHash(context.runtime);
+                } else {                                  // We have more args than we need and kwargs is always the last arg.
+                    newArgs[newArgs.length - 1] = args[args.length - 1];
+                }
             }
 
             args = newArgs;

diff --git a/core/src/main/java/org/jruby/runtime/IRBlockBody.java b/core/src/main/java/org/jruby/runtime/IRBlockBody.java
@@ -183,7 +183,7 @@ public IRubyObject[] prepareArgumentsForCall(ThreadContext context, IRubyObject[
             if (args.length == 1) {
                 // Convert value to arg-array, unwrapping where necessary
                 args = IRRuntimeHelpers.convertValueIntoArgArray(context, args[0], arity, (type == Type.NORMAL) && (args[0] instanceof RubyArray));
-            } else if (arity().getValue() == 1) {
+            } else if (arity().getValue() == 1 && !getSignature().restKwargs()) {
                // discard excess arguments
                 args = (args.length == 0) ? context.runtime.getSingleNilArray() : new IRubyObject[] { args[0] };
             }

diff --git a/core/src/main/java/org/jruby/runtime/Signature.java b/core/src/main/java/org/jruby/runtime/Signature.java
@@ -36,7 +36,9 @@ public enum Rest { NONE, NORM, ANON, STAR }
     private final boolean kwargs;
     private final Arity arity;
 
+    // FIXME: three booleans for kwargs?  Even if we keep these we should make it cheaper to get these values from Iter/friends
     private boolean requiredKwargs;
+    private boolean restKwargs;
 
     public Signature(int pre, int opt, int post, Rest rest, boolean kwargs) {
         this.pre = pre;
@@ -56,13 +58,14 @@ public Signature(int pre, int opt, int post, Rest rest, boolean kwargs) {
         }
     }
 
-    public Signature(int pre, int opt, int post, Rest rest, boolean kwargs, boolean requiredKwargs) {
+    public Signature(int pre, int opt, int post, Rest rest, boolean kwargs, boolean requiredKwargs, boolean restKwargs) {
         this.pre = pre;
         this.opt = opt;
         this.post = post;
         this.rest = rest;
         this.kwargs = kwargs;
         this.requiredKwargs = requiredKwargs;
+        this.restKwargs = restKwargs;
 
         // NOTE: Some logic to *assign* variables still uses Arity, which treats Rest.ANON (the
         //       |a,| form) as a rest arg for destructuring purposes. However ANON does *not*
@@ -75,9 +78,12 @@ public Signature(int pre, int opt, int post, Rest rest, boolean kwargs, boolean
         }
     }
 
-    private int getRequiredKeywordCount() {
-        if (requiredKwargs) return 1;
-        return 0;
+    public int getRequiredKeywordCount() {
+        return requiredKwargs ? 1 : 0;
+    }
+
+    public boolean restKwargs() {
+        return restKwargs;
     }
 
     public int pre() { return pre; }
@@ -129,7 +135,7 @@ public static Signature from(int pre, int opt, int post, Rest rest, boolean kwar
         return new Signature(pre, opt, post, rest, kwargs);
     }
 
-    public static Signature from(int pre, int opt, int post, Rest rest, boolean kwargs, boolean requiredKwargs) {
+    public static Signature from(int pre, int opt, int post, Rest rest, boolean kwargs, boolean requiredKwargs, boolean restKwargs) {
         if (opt == 0 && post == 0 && !kwargs) {
             switch (pre) {
                 case 0:
@@ -166,7 +172,7 @@ public static Signature from(int pre, int opt, int post, Rest rest, boolean kwar
                     break;
             }
         }
-        return new Signature(pre, opt, post, rest, kwargs, requiredKwargs);
+        return new Signature(pre, opt, post, rest, kwargs, requiredKwargs, restKwargs);
     }
 
     public static Signature from(IterNode iter) {
@@ -185,7 +191,8 @@ public static Signature from(IterNode iter) {
             rest = restFromArg(restArg);
         }
 
-       return Signature.from(args.getPreCount(), args.getOptionalArgsCount(), args.getPostCount(), rest, args.hasKwargs(), hasRequiredKeywordArg(args));
+       return Signature.from(args.getPreCount(), args.getOptionalArgsCount(), args.getPostCount(), rest,
+               args.hasKwargs(), hasRequiredKeywordArg(args), args.hasKeyRest());
     }
 
     private static boolean hasRequiredKeywordArg(ArgsNode args) {
@@ -201,7 +208,6 @@ private static boolean hasRequiredKeywordArg(ArgsNode args) {
         return false;
     }
 
-
     private static boolean isRequiredKeywordArgumentValueNode(Node asgnNode) {
         return asgnNode.childNodes().get(0) instanceof RequiredKeywordArgumentValueNode;
     }