Fix #1943: Setup keyword args properly for super

* Separate out keyword args from non-keyword args in IRMethod and IRClosure and build a list of <key, value> pairs which are then used to set up a Hash operand (but with a isKWArgsHash field to deal with the keyword rest arg hash specially). * This fix automatically makes everything work. * This is a quick first pass. - JIT needs to deal with the isKWArgsHash field. - The hack of adding the keyword rest arg hash with a dummy symbol could be replaced with a new field in IRClosure and IRMethod. - The boolean in Hash should be removed and we should use a subclassed operand just for this use-case. - Requires more testing. - Requires specs / tests.
jruby · Nov 6, 2014 · e19e3e1 · e19e3e1
1 parent 6d424b4
commit e19e3e1
Showing 4 changed files with 88 additions and 19 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRClosure.java b/core/src/main/java/org/jruby/ir/IRClosure.java
@@ -11,6 +11,7 @@
 import org.jruby.runtime.Arity;
 import org.jruby.runtime.BlockBody;
 import org.jruby.runtime.InterpretedIRBlockBody;
+import org.jruby.util.KeyValuePair;
 import org.objectweb.asm.Handle;
 
 import java.util.ArrayList;
@@ -31,6 +32,7 @@ public class IRClosure extends IRScope {
 
     // Block parameters
     private List<Operand> blockArgs;
+    private List<KeyValuePair<Operand, Operand>> keywordArgs;
 
     /** The parameter names, for Proc#parameters */
     private String[] parameterList;
@@ -79,6 +81,7 @@ protected IRClosure(IRClosure c, IRScope lexicalParent, int closureId, String fu
             this.body = new InterpretedIRBlockBody(this, c.body.arity());
         }
         this.blockArgs = new ArrayList<>();
+        this.keywordArgs = new ArrayList<>();
         this.arity = c.arity;
     }
 
@@ -93,6 +96,7 @@ public IRClosure(IRManager manager, IRScope lexicalParent, int lineNumber, Stati
     public IRClosure(IRManager manager, IRScope lexicalParent, int lineNumber, StaticScope staticScope, Arity arity, int argumentType, String prefix, boolean isBeginEndBlock) {
         this(manager, lexicalParent, lexicalParent.getFileName(), lineNumber, staticScope, prefix);
         this.blockArgs = new ArrayList<>();
+        this.keywordArgs = new ArrayList<>();
         this.argumentType = argumentType;
         this.arity = arity;
         lexicalParent.addClosure(this);
@@ -182,14 +186,33 @@ public boolean isFlipScope() {
     @Override
     public void addInstr(Instr i) {
         // Accumulate block arguments
-        if (i instanceof ReceiveRestArgInstr) blockArgs.add(new Splat(((ReceiveRestArgInstr)i).getResult()));
-        else if (i instanceof ReceiveArgBase) blockArgs.add(((ReceiveArgBase) i).getResult());
+        if (i instanceof ReceiveKeywordRestArgInstr) {
+            // Always add the keyword rest arg to the beginning
+            keywordArgs.add(0, new KeyValuePair<Operand, Operand>(Symbol.KW_REST_ARG_DUMMY, ((ReceiveArgBase) i).getResult()));
+        } else if (i instanceof ReceiveKeywordArgInstr) {
+            ReceiveKeywordArgInstr rkai = (ReceiveKeywordArgInstr)i;
+            keywordArgs.add(new KeyValuePair<Operand, Operand>(new Symbol(rkai.argName), rkai.getResult()));
+        } else if (i instanceof ReceiveRestArgInstr) {
+            blockArgs.add(new Splat(((ReceiveRestArgInstr)i).getResult()));
+        } else if (i instanceof ReceiveArgBase) {
+            blockArgs.add(((ReceiveArgBase) i).getResult());
+        }
 
         super.addInstr(i);
     }
 
     public Operand[] getBlockArgs() {
-        return blockArgs.toArray(new Operand[blockArgs.size()]);
+        if (receivesKeywordArgs()) {
+            int i = 0;
+            Operand[] args = new Operand[blockArgs.size() + 1];
+            for (Operand arg: blockArgs) {
+                args[i++] = arg;
+            }
+            args[i] = new Hash(keywordArgs, true);
+            return args;
+        } else {
+            return blockArgs.toArray(new Operand[blockArgs.size()]);
+        }
     }
 
     public String toStringBody() {

diff --git a/core/src/main/java/org/jruby/ir/IRMethod.java b/core/src/main/java/org/jruby/ir/IRMethod.java
@@ -3,10 +3,15 @@
 import org.jruby.internal.runtime.methods.IRMethodArgs;
 import org.jruby.ir.instructions.Instr;
 import org.jruby.ir.instructions.ReceiveArgBase;
+import org.jruby.ir.instructions.ReceiveKeywordArgInstr;
+import org.jruby.ir.instructions.ReceiveKeywordRestArgInstr;
 import org.jruby.ir.instructions.ReceiveRestArgInstr;
 import org.jruby.ir.operands.LocalVariable;
 import org.jruby.ir.operands.Operand;
+import org.jruby.ir.operands.Symbol;
+import org.jruby.ir.operands.Hash;
 import org.jruby.ir.operands.Splat;
+import org.jruby.util.KeyValuePair;
 import org.jruby.parser.StaticScope;
 
 import java.lang.invoke.MethodType;
@@ -24,6 +29,7 @@ public class IRMethod extends IRScope {
     //
     // Call parameters
     private List<Operand> callArgs;
+    private List<KeyValuePair<Operand, Operand>> keywordArgs;
 
     // Argument description of the form [:req, "a"], [:opt, "b"] ..
     private List<String[]> argDesc;
@@ -39,8 +45,9 @@ public IRMethod(IRManager manager, IRScope lexicalParent, String name,
         super(manager, lexicalParent, name, lexicalParent.getFileName(), lineNumber, staticScope);
 
         this.isInstanceMethod = isInstanceMethod;
-        this.callArgs = new ArrayList<Operand>();
-        this.argDesc = new ArrayList<String[]>();
+        this.callArgs = new ArrayList<>();
+        this.keywordArgs = new ArrayList<>();
+        this.argDesc = new ArrayList<>();
         this.signatures = new HashMap<>();
 
         if (!getManager().isDryRun() && staticScope != null) {
@@ -57,8 +64,17 @@ public IRScopeType getScopeType() {
     @Override
     public void addInstr(Instr i) {
         // Accumulate call arguments
-        if (i instanceof ReceiveRestArgInstr) callArgs.add(new Splat(((ReceiveRestArgInstr)i).getResult(), true));
-        else if (i instanceof ReceiveArgBase) callArgs.add(((ReceiveArgBase) i).getResult());
+        if (i instanceof ReceiveKeywordRestArgInstr) {
+            // Always add the keyword rest arg to the beginning
+            keywordArgs.add(0, new KeyValuePair<Operand, Operand>(Symbol.KW_REST_ARG_DUMMY, ((ReceiveArgBase) i).getResult()));
+        } else if (i instanceof ReceiveKeywordArgInstr) {
+            ReceiveKeywordArgInstr rkai = (ReceiveKeywordArgInstr)i;
+            keywordArgs.add(new KeyValuePair<Operand, Operand>(new Symbol(rkai.argName), rkai.getResult()));
+        } else if (i instanceof ReceiveRestArgInstr) {
+            callArgs.add(new Splat(((ReceiveRestArgInstr)i).getResult(), true));
+        } else if (i instanceof ReceiveArgBase) {
+            callArgs.add(((ReceiveArgBase) i).getResult());
+        }
 
         super.addInstr(i);
     }
@@ -72,7 +88,17 @@ public List<String[]> getArgDesc() {
     }
 
     public Operand[] getCallArgs() {
-        return callArgs.toArray(new Operand[callArgs.size()]);
+        if (receivesKeywordArgs()) {
+            int i = 0;
+            Operand[] args = new Operand[callArgs.size() + 1];
+            for (Operand arg: callArgs) {
+                args[i++] = arg;
+            }
+            args[i] = new Hash(keywordArgs, true);
+            return args;
+        } else {
+            return callArgs.toArray(new Operand[callArgs.size()]);
+        }
     }
 
     @Override

diff --git a/core/src/main/java/org/jruby/ir/operands/Hash.java b/core/src/main/java/org/jruby/ir/operands/Hash.java
@@ -11,6 +11,7 @@
 import org.jruby.util.KeyValuePair;
 
 import java.util.List;
+import java.util.Iterator;
 import java.util.Map;
 
 // Represents a hash { _ =>_, _ => _ .. } in ruby
@@ -21,10 +22,19 @@
 public class Hash extends Operand {
     final public List<KeyValuePair<Operand, Operand>> pairs;
 
-    public Hash(List<KeyValuePair<Operand, Operand>> pairs) {
+    // Is this a hash used to represent a keyword hash to be setup for ZSuper?
+    // SSS FIXME: Quick hack for now - this should probably be done with an overloaded operand.
+    final public boolean isKWArgsHash;
+
+    public Hash(List<KeyValuePair<Operand, Operand>> pairs, boolean isKWArgsHash) {
         super(OperandType.HASH);
 
         this.pairs = pairs;
+        this.isKWArgsHash = isKWArgsHash;
+    }
+
+    public Hash(List<KeyValuePair<Operand, Operand>> pairs) {
+        this(pairs, false);
     }
 
     public boolean isBlank() {
@@ -49,7 +59,7 @@ public Operand getSimplifiedOperand(Map<Operand, Operand> valueMap, boolean forc
                     .getValue().getSimplifiedOperand(valueMap, force)));
         }
 
-        return new Hash(newPairs);
+        return new Hash(newPairs, isKWArgsHash);
     }
 
     /** Append the list of variables used in this operand to the input list */
@@ -71,20 +81,28 @@ public Operand cloneForInlining(CloneInfo ii) {
             newPairs.add(new KeyValuePair(pair.getKey().cloneForInlining(ii), pair.getValue()
                     .cloneForInlining(ii)));
         }
-        return new Hash(newPairs);
+        return new Hash(newPairs, isKWArgsHash);
     }
 
     @Override
-    public Object retrieve(ThreadContext context, IRubyObject self, StaticScope currScope, DynamicScope currDynScope,
-                           Object[] temp) {
+    public Object retrieve(ThreadContext context, IRubyObject self, StaticScope currScope, DynamicScope currDynScope, Object[] temp) {
         Ruby runtime = context.runtime;
-        RubyHash hash = RubyHash.newHash(runtime);
+        RubyHash hash;
+        Iterator<KeyValuePair<Operand, Operand>> it = pairs.iterator();
+
+        if (isKWArgsHash && pairs.get(0).getKey() == Symbol.KW_REST_ARG_DUMMY) {
+            // Dup the rest args hash and use that as the basis for inserting the non-rest args
+            hash = (RubyHash)((RubyHash) pairs.get(0).getValue().retrieve(context, self, currScope, currDynScope, temp)).dup(context);
+            // Skip the first pair
+            it.next();
+        } else {
+            hash = RubyHash.newHash(runtime);
+        }
 
-        for (KeyValuePair<Operand, Operand> pair : pairs) {
-            IRubyObject key = (IRubyObject) pair.getKey().retrieve(context, self, currScope, currDynScope,
-                    temp);
-            IRubyObject value = (IRubyObject) pair.getValue().retrieve(context, self, currScope, currDynScope,
-                    temp);
+        while (it.hasNext()) {
+            KeyValuePair<Operand, Operand> pair = (KeyValuePair<Operand, Operand>) it.next();
+            IRubyObject key = (IRubyObject) pair.getKey().retrieve(context, self, currScope, currDynScope, temp);
+            IRubyObject value = (IRubyObject) pair.getValue().retrieve(context, self, currScope, currDynScope, temp);
 
             hash.fastASetCheckString(runtime, key, value);
         }

diff --git a/core/src/main/java/org/jruby/ir/operands/Symbol.java b/core/src/main/java/org/jruby/ir/operands/Symbol.java
@@ -7,6 +7,8 @@
 import org.jruby.runtime.builtin.IRubyObject;
 
 public class Symbol extends Reference {
+    public static final Symbol KW_REST_ARG_DUMMY = new Symbol("");
+
     public Symbol(String name) {
         super(OperandType.SYMBOL, name);
     }