Fixes #4319. JRuby can not interpret keyword argument when placed aft…

…er positional argument in block Fixes #2485. proc with extra args incorrectly binds wrong post args ReceivePostReqdArgInstr now records whether we use rest args and how many opt args are processed. Using these two new variables this instruction will now count forward in cases where we have too many arguments passed in (this only occurs for procs). IRBuilder has been changed to pass Signature for setting up pre and post arg instructions. RubyProc will take kwargs into consideration when re-processing the incoming argument list. I am hoping to delete this code in a future commit.
jruby · Dec 2, 2016 · 4e4935e · 4e4935e
1 parent ecc1fa8
commit 4e4935e
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 29 deletions.
diff --git a/core/src/main/java/org/jruby/RubyProc.java b/core/src/main/java/org/jruby/RubyProc.java
@@ -268,11 +268,11 @@ public static IRubyObject[] prepareArgs(ThreadContext context, Block.Type type,
         boolean isFixed = signature.isFixed();
         int required = signature.required();
         int actual = args.length;
-        boolean restKwargs = blockBody instanceof IRBlockBody && ((IRBlockBody) blockBody).getSignature().hasKwargs();
+        boolean kwargs = blockBody instanceof IRBlockBody && signature.hasKwargs();
 
         // 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 ((signature != Signature.ONE_ARGUMENT &&  required != 0 && (isFixed || signature != Signature.OPTIONAL) || restKwargs) &&
+        if ((signature != Signature.ONE_ARGUMENT &&  required != 0 && (isFixed || signature != Signature.OPTIONAL) || kwargs) &&
                 actual == 1 && args[0].respondsTo("to_ary")) {
             IRubyObject newAry = Helpers.aryToAry(args[0]);
 
@@ -287,15 +287,11 @@ public static IRubyObject[] prepareArgs(ThreadContext context, Block.Type type,
             actual = args.length;
         }
 
+        // FIXME: This pruning only seems to be needed for any?/all? specs.
         // fixed arity > 0 with mismatch needs a new args array
-        if (isFixed && required > 0 && required != actual) {
-            IRubyObject[] newArgs = ArraySupport.newCopy(args, required);
-
-            if (required > actual) { // Not enough required args pad.
-                Helpers.fillNil(newArgs, actual, required, context.runtime);
-            }
-
-            args = newArgs;
+        int kwargsHackRequired = required + (kwargs ? 1 : 0);
+        if (isFixed && required > 0 && kwargsHackRequired != actual) {
+            args = ArraySupport.newCopy(args, kwargsHackRequired);
         }
 
         return args;

diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -1979,9 +1979,11 @@ protected LocalVariable getArgVariable(String name, int depth) {
         return scope instanceof IRFor ? getLocalVariable(name, depth) : getNewLocalVariable(name, 0);
     }
 
-    private void addArgReceiveInstr(Variable v, int argIndex, boolean post, int numPreReqd, int numPostRead) {
+    private void addArgReceiveInstr(Variable v, int argIndex, Signature signature) {
+        boolean post = signature != null;
+
         if (post) {
-            addInstr(new ReceivePostReqdArgInstr(v, argIndex, numPreReqd, numPostRead));
+            addInstr(new ReceivePostReqdArgInstr(v, argIndex, signature.pre(), signature.opt(), signature.hasRest(), signature.post()));
         } else {
             addInstr(new ReceivePreReqdArgInstr(v, argIndex));
         }
@@ -2002,20 +2004,20 @@ private Variable argumentResult(String name) {
         }
     }
 
-    public void receiveRequiredArg(Node node, int argIndex, boolean post, int numPreReqd, int numPostRead) {
+    public void receiveRequiredArg(Node node, int argIndex, Signature signature) {
         switch (node.getNodeType()) {
             case ARGUMENTNODE: {
                 String argName = ((ArgumentNode)node).getName();
 
                 if (scope instanceof IRMethod) addArgumentDescription(ArgumentType.req, argName);
 
-                addArgReceiveInstr(argumentResult(argName), argIndex, post, numPreReqd, numPostRead);
+                addArgReceiveInstr(argumentResult(argName), argIndex, signature);
                 break;
             }
             case MULTIPLEASGNNODE: {
                 MultipleAsgnNode childNode = (MultipleAsgnNode) node;
                 Variable v = createTemporaryVariable();
-                addArgReceiveInstr(v, argIndex, post, numPreReqd, numPostRead);
+                addArgReceiveInstr(v, argIndex, signature);
                 if (scope instanceof IRMethod) addArgumentDescription(ArgumentType.anonreq, null);
                 Variable tmp = createTemporaryVariable();
                 addInstr(new ToAryInstr(tmp, v));
@@ -2054,7 +2056,7 @@ protected void receiveNonBlockArgs(final ArgsNode argsNode) {
         Node[] args = argsNode.getArgs();
         int preCount = signature.pre();
         for (int i = 0; i < preCount; i++, argIndex++) {
-            receiveRequiredArg(args[i], argIndex, false, -1, -1);
+            receiveRequiredArg(args[i], argIndex, null);
         }
 
         // Fixup opt/rest
@@ -2106,7 +2108,7 @@ protected void receiveNonBlockArgs(final ArgsNode argsNode) {
         int postCount = argsNode.getPostCount();
         int postIndex = argsNode.getPostIndex();
         for (int i = 0; i < postCount; i++) {
-            receiveRequiredArg(args[postIndex + i], i, true, signature.pre(), postCount);
+            receiveRequiredArg(args[postIndex + i], i, signature);
         }
     }
 

diff --git a/core/src/main/java/org/jruby/ir/instructions/ReceivePostReqdArgInstr.java b/core/src/main/java/org/jruby/ir/instructions/ReceivePostReqdArgInstr.java
@@ -23,12 +23,20 @@ public class ReceivePostReqdArgInstr extends ReceiveArgBase implements FixedArit
     /** The method/block parameter list has these many required parameters before opt+rest args*/
     public final int preReqdArgsCount;
 
+    /** The method/block parameter list has a maximum of this many optional arguments*/
+    public final int optArgsCount;
+
+    /** Does this method/block accept a rest argument */
+    public final boolean restArg;
+
     /** The method/block parameter list has these many required parameters after opt+rest args*/
     public final int postReqdArgsCount;
 
-    public ReceivePostReqdArgInstr(Variable result, int argIndex, int preReqdArgsCount, int postReqdArgsCount) {
+    public ReceivePostReqdArgInstr(Variable result, int argIndex, int preReqdArgsCount, int optArgCount, boolean restArg, int postReqdArgsCount) {
         super(Operation.RECV_POST_REQD_ARG, result, argIndex);
         this.preReqdArgsCount = preReqdArgsCount;
+        this.optArgsCount = optArgCount;
+        this.restArg = restArg;
         this.postReqdArgsCount = postReqdArgsCount;
     }
 
@@ -39,7 +47,9 @@ public String[] toStringNonOperandArgs() {
 
     @Override
     public Instr clone(CloneInfo info) {
-        if (info instanceof SimpleCloneInfo) return new ReceivePostReqdArgInstr(info.getRenamedVariable(result), argIndex, preReqdArgsCount, postReqdArgsCount);
+        if (info instanceof SimpleCloneInfo) {
+            return new ReceivePostReqdArgInstr(info.getRenamedVariable(result), argIndex, preReqdArgsCount, optArgsCount, restArg, postReqdArgsCount);
+        }
 
         InlineCloneInfo ii = (InlineCloneInfo) info;
 
@@ -64,15 +74,18 @@ public void encode(IRWriterEncoder e) {
         super.encode(e);
         e.encode(getArgIndex());
         e.encode(preReqdArgsCount);
+        e.encode(optArgsCount);
+        e.encode(restArg);
         e.encode(postReqdArgsCount);
     }
 
     public static ReceivePostReqdArgInstr decode(IRReaderDecoder d) {
-        return new ReceivePostReqdArgInstr(d.decodeVariable(), d.decodeInt(), d.decodeInt(), d.decodeInt());
+        return new ReceivePostReqdArgInstr(d.decodeVariable(), d.decodeInt(), d.decodeInt(), d.decodeInt(), d.decodeBoolean(), d.decodeInt());
     }
 
     public IRubyObject receivePostReqdArg(ThreadContext context, IRubyObject[] args, boolean acceptsKeywordArgument) {
-        return IRRuntimeHelpers.receivePostReqdArg(context, args, preReqdArgsCount, postReqdArgsCount, argIndex, acceptsKeywordArgument);
+        return IRRuntimeHelpers.receivePostReqdArg(context, args, preReqdArgsCount, optArgsCount, restArg,
+                postReqdArgsCount, argIndex, acceptsKeywordArgument);
     }
 
     @Override

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -897,14 +897,33 @@ private static IRubyObject constructRestArg(ThreadContext context, IRubyObject[]
         return RubyArray.newArrayMayCopy(context.runtime, args, argIndex, remainingArguments);
     }
 
-    @JIT
-    public static IRubyObject receivePostReqdArg(ThreadContext context, IRubyObject[] args, int preReqdArgsCount, int postReqdArgsCount, int argIndex, boolean acceptsKeywordArgument) {
-        boolean kwargs = extractKwargsHash(args, preReqdArgsCount + postReqdArgsCount, acceptsKeywordArgument) != null;
+    @JIT @Interp
+    public static IRubyObject receivePostReqdArg(ThreadContext context, IRubyObject[] args, int pre,
+                                                 int opt, boolean rest, int post,
+                                                 int argIndex, boolean acceptsKeywordArgument) {
+        int required = pre + post;
+        // FIXME: Once we extract kwargs from rest of args processing we can delete this extract and n calc.
+        boolean kwargs = extractKwargsHash(args, required, acceptsKeywordArgument) != null;
         int n = kwargs ? args.length - 1 : args.length;
-        int remaining = n - preReqdArgsCount;
-        if (remaining <= argIndex) return context.nil;
+        int remaining = n - pre;       // we know we have received all pre args by post receives.
+
+        if (remaining < post) {        // less args available than post args need
+            if (pre + argIndex >= n) { // argument is past end of arg list
+                return context.nil;
+            } else {
+                return args[pre + argIndex];
+            }
+        }
 
-        return (remaining > postReqdArgsCount) ? args[n - postReqdArgsCount + argIndex] : args[preReqdArgsCount + argIndex];
+        // At this point we know we have enough arguments left for post without worrying about AIOOBE.
+
+        if (rest) {                      // we can read from back since we will take all args we can get.
+            return args[n - post + argIndex];
+        } else if (n > required + opt) { // we filled all opt so we can read from front (and avoid excess args case from proc).
+            return args[pre + opt + argIndex];
+        } else {                         // partial opts filled in too few args so we can read from end.
+            return args[n - post + argIndex];
+        }
     }
 
     public static IRubyObject receiveOptArg(IRubyObject[] args, int requiredArgs, int preArgs, int argIndex, boolean acceptsKeywordArgument) {

diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -1761,10 +1761,12 @@ public void ReceivePostReqdArgInstr(ReceivePostReqdArgInstr instr) {
         jvmMethod().loadContext();
         jvmMethod().loadArgs();
         jvmAdapter().pushInt(instr.preReqdArgsCount);
+        jvmAdapter().pushInt(instr.optArgsCount);
+        jvmAdapter().pushBoolean(instr.restArg);
         jvmAdapter().pushInt(instr.postReqdArgsCount);
         jvmAdapter().pushInt(instr.getArgIndex());
         jvmAdapter().ldc(jvm.methodData().scope.receivesKeywordArgs());
-        jvmMethod().invokeIRHelper("receivePostReqdArg", sig(IRubyObject.class, ThreadContext.class, IRubyObject[].class, int.class, int.class, int.class, boolean.class));
+        jvmMethod().invokeIRHelper("receivePostReqdArg", sig(IRubyObject.class, ThreadContext.class, IRubyObject[].class, int.class, int.class, boolean.class, int.class, int.class, boolean.class));
         jvmStoreLocal(instr.getResult());
     }
 

diff --git a/spec/tags/ruby/language/block_tags.txt b/spec/tags/ruby/language/block_tags.txt
@@ -1,2 +1,2 @@
 fails:A block yielded a single Array assigns elements to optional arguments
-fails:A block yielded a single Array calls #to_hash on the last element when there are more arguments than parameters
+