Skip to content

Commit

Permalink
Fix #1943: Setup keyword args properly for super
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@subbuss
subbuss committed Nov 6, 2014
1 parent 6d424b4 commit e19e3e1
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 19 deletions.
29 changes: 26 additions & 3 deletions core/src/main/java/org/jruby/ir/IRClosure.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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);
Expand Down Expand Up @@ -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() {
Expand Down
36 changes: 31 additions & 5 deletions core/src/main/java/org/jruby/ir/IRMethod.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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) {
Expand All @@ -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);
}
Expand All @@ -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
Expand Down
40 changes: 29 additions & 11 deletions core/src/main/java/org/jruby/ir/operands/Hash.java
View file
Expand Up @@ -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
Expand All @@ -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() {
Expand All @@ -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 */
Expand All @@ -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);
}
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/org/jruby/ir/operands/Symbol.java
View file
Expand Up @@ -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);
}
Expand Down

0 comments on commit e19e3e1

Please sign in to comment.