Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: jruby/jruby
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: d6e2e8178e93
Choose a base ref
...
head repository: jruby/jruby
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: 4173d5c3c25d
Choose a head ref
  • 3 commits
  • 16 files changed
  • 1 contributor

Commits on Aug 22, 2016

  1. Fix many kwargs issues in methods and procs. This is still a stop-gap

    fix as our instrs for processing kwargs should be made explicit.  As it
    stands we cannot know which argument is or is not a kwarg by just
    checking the last value (although we are getting close to that being
    true).
    
    The unfortunate part of this commit is that many pieces are in-flight
    at once.  The pieces to this commit are:
    
    . frobnicateKwargsArgument now will create a non-kwargs and kwargs
    hash and put non-symbol and symbol entries into the proper hashes.  If
    there are actually both hashes with contents then we will grow the
    args list by one to supply both.  This fixed all known method specs
    and tests.
    . Hackily not entirely depend on signature.arityValue() for whether
    we destructure a single array passed into a proc/lambda.  This is
    more of a form fit in that adding one extra kwargs check fixed literally
    all block kwarg specs/tests sans some optarg cases (which are just
    as broken before this fix).  I would have tried altering the arityValue
    itself but we use this value in tons of locations.  The risk was large
    enough to just do this in two places (in IRBlockBody and IRRuntimeHelpers.java):
       } else if (!getSignature().hasKwargs() && blockArity >= -1 && blockArity <= 1) {
    
    . Removed arg padding on call to a proc.  This was horrifically complicated
    and one of its main purposes was padding for an extra kwargs element.
    Now the frobnicateKwargsArgument will adjust the args array this ended
    making this unneccesary (well maybe.  We still have optarg bugs so
    there is definitely a question of how we pad/adjust arg lists in
    procs when they are in the presence of optargs).
    enebo committed Aug 22, 2016
    Copy the full SHA
    c183943 View commit details
  2. Fixes block execution case where an array passed as single argument s…

    …hould
    
    expand if there are only opt-args and more than one of them.
    enebo committed Aug 22, 2016
    Copy the full SHA
    54f6d77 View commit details
  3. Copy the full SHA
    4173d5c View commit details
22 changes: 2 additions & 20 deletions core/src/main/java/org/jruby/RubyProc.java
Original file line number Diff line number Diff line change
@@ -286,31 +286,13 @@ public static IRubyObject[] prepareArgs(ThreadContext context, Block.Type type,
actual = args.length;
}

// 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().hasKwargs() ?
1 - ((IRBlockBody) blockBody).getSignature().getRequiredKeywordForArityCount() : 0;

// fixed arity > 0 with mismatch needs a new args array
if (isFixed && required > 0 && required+needsKwargs != actual) {
IRubyObject[] newArgs = Arrays.copyOf(args, required+needsKwargs);
if (isFixed && required > 0 && required != actual) {
IRubyObject[] newArgs = Arrays.copyOf(args, required);


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;
Original file line number Diff line number Diff line change
@@ -83,7 +83,7 @@ protected void pre(ThreadContext context, StaticScope staticScope, RubyModule im
public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) {
if (!hasExplicitCallProtocol) return callNoProtocol(context, self, name, args, block);

if (hasKwargs) IRRuntimeHelpers.frobnicateKwargsArgument(context, args, getSignature().required());
if (hasKwargs) args = IRRuntimeHelpers.frobnicateKwargsArgument(context, args, getSignature().required());

return invokeExact(this.variable, context, staticScope, self, args, block, implementationClass, name);
}
@@ -129,7 +129,7 @@ private IRubyObject callNoProtocol(ThreadContext context, IRubyObject self, Stri
RubyModule implementationClass = this.implementationClass;
pre(context, staticScope, implementationClass, self, name, block);

if (hasKwargs) IRRuntimeHelpers.frobnicateKwargsArgument(context, args, getSignature().required());
if (hasKwargs) args = IRRuntimeHelpers.frobnicateKwargsArgument(context, args, getSignature().required());

try {
return invokeExact(this.variable, context, staticScope, self, args, block, implementationClass, name);
6 changes: 3 additions & 3 deletions core/src/main/java/org/jruby/ir/IRBuilder.java
Original file line number Diff line number Diff line change
@@ -616,7 +616,7 @@ protected Operand[] buildCallArgs(Node args) {
switch (args.getNodeType()) {
case ARGSCATNODE:
case ARGSPUSHNODE:
return new Operand[] { new Splat(build(args)) };
return new Operand[] { new Splat(addResultInstr(new BuildSplatInstr(createTemporaryVariable(), build(args), false))) };
case ARRAYNODE: {
Node[] children = ((ListNode) args).children();
int numberOfArgs = children.length;
@@ -629,7 +629,7 @@ protected Operand[] buildCallArgs(Node args) {
return builtArgs;
}
case SPLATNODE:
return new Operand[] { new Splat(buildSplat((SplatNode)args)) };
return new Operand[] { new Splat(addResultInstr(new BuildSplatInstr(createTemporaryVariable(), build(args), false))) };
}

throw new NotCompilableException("Invalid node for call args: " + args.getClass().getSimpleName() + ":" +
@@ -3605,7 +3605,7 @@ public Variable buildSelf() {
}

public Operand buildSplat(SplatNode splatNode) {
return addResultInstr(new BuildSplatInstr(createTemporaryVariable(), build(splatNode.getValue())));
return addResultInstr(new BuildSplatInstr(createTemporaryVariable(), build(splatNode.getValue()), true));
}

public Operand buildStr(StrNode strNode) {
20 changes: 15 additions & 5 deletions core/src/main/java/org/jruby/ir/instructions/BuildSplatInstr.java
Original file line number Diff line number Diff line change
@@ -15,8 +15,17 @@

// Represents a splat value in Ruby code: *array
public class BuildSplatInstr extends OneOperandResultBaseInstr {
public BuildSplatInstr(Variable result, Operand array) {
// Should we dup the resulting splat or not?
private final boolean dup;

public BuildSplatInstr(Variable result, Operand array, boolean dup) {
super(Operation.BUILD_SPLAT, result, array);

this.dup = dup;
}

public boolean getDup() {
return dup;
}

public Operand getArray() {
@@ -25,23 +34,24 @@ public Operand getArray() {

@Override
public Instr clone(CloneInfo ii) {
return new BuildSplatInstr(ii.getRenamedVariable(result), getArray().cloneForInlining(ii));
return new BuildSplatInstr(ii.getRenamedVariable(result), getArray().cloneForInlining(ii), getDup());
}

@Override
public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
return IRRuntimeHelpers.irSplat(context,
(IRubyObject) getArray().retrieve(context, self, currScope, currDynScope, temp));
return IRRuntimeHelpers.splatArray(context,
(IRubyObject) getArray().retrieve(context, self, currScope, currDynScope, temp), getDup());
}

@Override
public void encode(IRWriterEncoder e) {
super.encode(e);
e.encode(getArray());
e.encode(getDup());
}

public static BuildSplatInstr decode(IRReaderDecoder d) {
return new BuildSplatInstr(d.decodeVariable(), d.decodeOperand());
return new BuildSplatInstr(d.decodeVariable(), d.decodeOperand(), d.decodeBoolean());
}

@Override
Original file line number Diff line number Diff line change
@@ -117,7 +117,7 @@ public IRubyObject interpret(ThreadContext context, Block block, IRubyObject sel

// Blocks with explicit call protocol shouldn't do this before args are prepared
if (acceptsKeywordArgument && (block == null || !interpreterContext.hasExplicitCallProtocol())) {
IRRuntimeHelpers.frobnicateKwargsArgument(context, args, interpreterContext.getRequiredArgsCount());
args = IRRuntimeHelpers.frobnicateKwargsArgument(context, args, interpreterContext.getRequiredArgsCount());
}

StaticScope currScope = interpreterContext.getStaticScope();
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ public IRubyObject interpret(ThreadContext context, Block block, IRubyObject sel
int ipc = 0;
Object exception = null;

if (interpreterContext.receivesKeywordArguments()) IRRuntimeHelpers.frobnicateKwargsArgument(context, args, interpreterContext.getRequiredArgsCount());
if (interpreterContext.receivesKeywordArguments()) args = IRRuntimeHelpers.frobnicateKwargsArgument(context, args, interpreterContext.getRequiredArgsCount());

StaticScope currScope = interpreterContext.getStaticScope();
DynamicScope currDynScope = context.getCurrentScope();
100 changes: 77 additions & 23 deletions core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
Original file line number Diff line number Diff line change
@@ -19,6 +19,7 @@
import org.jruby.ir.IRScopeType;
import org.jruby.ir.Interp;
import org.jruby.ir.JIT;
import org.jruby.ir.Tuple;
import org.jruby.ir.operands.IRException;
import org.jruby.ir.operands.Operand;
import org.jruby.ir.operands.Splat;
@@ -37,6 +38,7 @@
import org.jruby.runtime.ivars.VariableAccessor;
import org.jruby.util.ByteList;
import org.jruby.util.DefinedMessage;
import org.jruby.util.RecursiveComparator;
import org.jruby.util.RegexpOptions;
import org.jruby.util.TypeConverter;
import org.jruby.util.log.Logger;
@@ -484,12 +486,14 @@ public static IRubyObject yieldSpecific(ThreadContext context, Block b) {
return b.yieldSpecific(context);
}

public static IRubyObject[] convertValueIntoArgArray(ThreadContext context, IRubyObject value, int blockArity, boolean argIsArray) {
public static IRubyObject[] convertValueIntoArgArray(ThreadContext context, IRubyObject value,
org.jruby.runtime.Signature signature, boolean argIsArray) {
// SSS FIXME: This should not really happen -- so, some places in the runtime library are breaking this contract.
if (argIsArray && !(value instanceof RubyArray)) argIsArray = false;

switch (blockArity) {
case -1 : return argIsArray ? ((RubyArray)value).toJavaArray() : new IRubyObject[] { value };
switch (signature.arityValue()) {
case -1 :
return argIsArray || (signature.opt() > 1 && value instanceof RubyArray) ? ((RubyArray)value).toJavaArray() : new IRubyObject[] { value };
case 0 : return new IRubyObject[] { value };
case 1 : {
if (argIsArray) {
@@ -557,27 +561,56 @@ public static void checkArity(ThreadContext context, StaticScope scope, Object[]
}
}

// Due to our current strategy of destructively processing the kwargs hash we need to dup
// and make sure the copy is not frozen. This has a poor name as encouragement to rewrite
// how we handle kwargs internally :)
public static void frobnicateKwargsArgument(ThreadContext context, IRubyObject[] args, int requiredArgsCount) {
if (args.length <= requiredArgsCount) return; // No kwarg because required args slurp them up.
public static IRubyObject[] frobnicateKwargsArgument(ThreadContext context, IRubyObject[] args, int requiredArgsCount) {
if (args.length <= requiredArgsCount) return args; // No kwarg because required args slurp them up.

RubyHash kwargs = toHash(args[args.length - 1], context);
IRubyObject kwargs = toHash(args[args.length - 1], context);

if (kwargs != null) {
kwargs = (RubyHash) kwargs.dup(context);
kwargs.setFrozen(false);
args[args.length - 1] = kwargs;
if (kwargs.isNil()) { // nil on to_hash is supposed to keep itself as real value so we need to make kwargs hash
IRubyObject[] newArgs = new IRubyObject[args.length + 1];
System.arraycopy(args, 0, newArgs, 0, args.length);
args = newArgs;
args[args.length - 1] = RubyHash.newSmallHash(context.runtime); // opt args
return args;
}

Tuple<RubyHash, RubyHash> hashes = new Tuple<>(RubyHash.newSmallHash(context.runtime), RubyHash.newSmallHash(context.runtime));

// We know toHash makes null, nil, or Hash
((RubyHash) kwargs).visitAll(context, DivvyKeywordsVisitor, hashes);

if (!hashes.b.isEmpty()) { // rest args exists too expand args
IRubyObject[] newArgs = new IRubyObject[args.length + 1];
System.arraycopy(args, 0, newArgs, 0, args.length);
args = newArgs;
args[args.length - 2] = hashes.b; // opt args
}
args[args.length - 1] = hashes.a; // kwargs hash
}

return args;
}

private static RubyHash toHash(IRubyObject lastArg, ThreadContext context) {
private static final RubyHash.VisitorWithState<Tuple<RubyHash, RubyHash>> DivvyKeywordsVisitor = new RubyHash.VisitorWithState<Tuple<RubyHash, RubyHash>>() {
@Override
public void visit(ThreadContext context, RubyHash self, IRubyObject key, IRubyObject value, int index, Tuple<RubyHash, RubyHash> hashes) {
if (key instanceof RubySymbol) {
hashes.a.op_aset(context, key, value);
} else {
hashes.b.op_aset(context, key, value);
}
}
};

private static IRubyObject toHash(IRubyObject lastArg, ThreadContext context) {
if (lastArg instanceof RubyHash) return (RubyHash) lastArg;
if (lastArg.respondsTo("to_hash")) {
if ( context == null ) context = lastArg.getRuntime().getCurrentContext();
lastArg = lastArg.callMethod(context, "to_hash");
if (lastArg instanceof RubyHash) return (RubyHash) lastArg;
if (lastArg.isNil()) return lastArg;
TypeConverter.checkType(context, lastArg, context.runtime.getHash());
return (RubyHash) lastArg;
}
return null;
}
@@ -588,7 +621,11 @@ public static RubyHash extractKwargsHash(Object[] args, int requiredArgsCount, b

Object lastArg = args[args.length - 1];

if (lastArg instanceof IRubyObject) return toHash((IRubyObject) lastArg, null);
if (lastArg instanceof IRubyObject) {
IRubyObject returnValue = toHash((IRubyObject) lastArg, null);
if (returnValue instanceof RubyHash) return (RubyHash) returnValue;
}

return null;
}

@@ -1492,6 +1529,25 @@ else if (true /**RTEST(flag)**/) { // this logic is only used for bare splat, an
return (RubyArray)tmp;
}

/**
* Call to_ary to get Array or die typing. The optionally dup it if specified. Some conditional
* cases in compiler we know we are safe in not-duping. This method is the same impl as MRIs
* splatarray instr in the YARV instruction set.
*/
@JIT @Interp
public static RubyArray splatArray(ThreadContext context, IRubyObject ary, boolean dupArray) {
Ruby runtime = context.runtime;
IRubyObject tmp = TypeConverter.convertToTypeWithCheck19(context, ary, runtime.getArray(), sites(context).to_a_checked);

if (tmp.isNil()) {
tmp = runtime.newArray(ary);
} else if (dupArray) {
tmp = ((RubyArray) tmp).aryDup();
}

return (RubyArray) tmp;
}

public static IRubyObject irToAry(ThreadContext context, IRubyObject value) {
if (!(value instanceof RubyArray)) {
value = RubyArray.aryToAry(value);
@@ -1614,12 +1670,10 @@ private static IRubyObject[] toAry(ThreadContext context, IRubyObject[] args) {
}

private static IRubyObject[] prepareProcArgs(ThreadContext context, Block b, IRubyObject[] args) {
if (args.length == 1) {
int arityValue = b.getBody().getSignature().arityValue();
return IRRuntimeHelpers.convertValueIntoArgArray(context, args[0], arityValue, b.type == Block.Type.NORMAL && args[0] instanceof RubyArray);
} else {
return args;
}
if (args.length != 1) return args;

// Potentially expand single value if it is an array depending on what we are calling.
return IRRuntimeHelpers.convertValueIntoArgArray(context, args[0], b.getBody().getSignature(), b.type == Block.Type.NORMAL && args[0] instanceof RubyArray);
}

private static IRubyObject[] prepareBlockArgsInternal(ThreadContext context, Block block, IRubyObject[] args) {
@@ -1642,7 +1696,7 @@ private static IRubyObject[] prepareBlockArgsInternal(ThreadContext context, Blo
}

int arityValue = sig.arityValue();
if (arityValue >= -1 && arityValue <= 1) {
if (!sig.hasKwargs() && arityValue >= -1 && arityValue <= 1) {
return args;
}

@@ -1759,7 +1813,7 @@ public static IRubyObject[] prepareFixedBlockArgs(ThreadContext context, Block b
public static IRubyObject[] prepareBlockArgs(ThreadContext context, Block block, IRubyObject[] args, boolean usesKwArgs) {
args = prepareBlockArgsInternal(context, block, args);
if (usesKwArgs) {
frobnicateKwargsArgument(context, args, block.getBody().getSignature().required());
args = frobnicateKwargsArgument(context, args, block.getBody().getSignature().required());
}
return args;
}
3 changes: 2 additions & 1 deletion core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
Original file line number Diff line number Diff line change
@@ -1006,7 +1006,8 @@ public void BuildRangeInstr(BuildRangeInstr instr) {
public void BuildSplatInstr(BuildSplatInstr instr) {
jvmMethod().loadContext();
visit(instr.getArray());
jvmMethod().invokeIRHelper("irSplat", sig(RubyArray.class, ThreadContext.class, IRubyObject.class));
jvmAdapter().ldc(instr.getDup());
jvmMethod().invokeIRHelper("splatArray", sig(RubyArray.class, ThreadContext.class, IRubyObject.class, boolean.class));
jvmStoreLocal(instr.getResult());
}

2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/runtime/BlockBody.java
Original file line number Diff line number Diff line change
@@ -267,7 +267,7 @@ public IRubyObject[] prepareArgumentsForCall(ThreadContext context, IRubyObject[
// I thought only procs & lambdas can be called, and blocks are yielded to.
if (args.length == 1) {
// Convert value to arg-array, unwrapping where necessary
args = IRRuntimeHelpers.convertValueIntoArgArray(context, args[0], signature.arityValue(), type == Block.Type.NORMAL && args[0] instanceof RubyArray);
args = IRRuntimeHelpers.convertValueIntoArgArray(context, args[0], signature, type == Block.Type.NORMAL && args[0] instanceof RubyArray);
} else if (getSignature().arityValue() == 1 && !getSignature().restKwargs()) {
// discard excess arguments
args = args.length == 0 ? context.runtime.getSingleNilArray() : new IRubyObject[] { args[0] };
6 changes: 3 additions & 3 deletions core/src/main/java/org/jruby/runtime/IRBlockBody.java
Original file line number Diff line number Diff line change
@@ -84,15 +84,15 @@ public IRubyObject yieldSpecific(ThreadContext context, Block block, IRubyObject
if (canCallDirect()) {
if (arg0 instanceof RubyArray) {
// Unwrap the array arg
args = IRRuntimeHelpers.convertValueIntoArgArray(context, arg0, signature.arityValue(), true);
args = IRRuntimeHelpers.convertValueIntoArgArray(context, arg0, signature, true);
} else {
args = new IRubyObject[] { arg0 };
}
return yieldDirect(context, block, args, null);
} else {
if (arg0 instanceof RubyArray) {
// Unwrap the array arg
args = IRRuntimeHelpers.convertValueIntoArgArray(context, arg0, signature.arityValue(), true);
args = IRRuntimeHelpers.convertValueIntoArgArray(context, arg0, signature, true);

// FIXME: arity error is aginst new args but actual error shows arity of original args.
if (block.type == Block.Type.LAMBDA) signature.checkArity(context.runtime, args);
@@ -170,7 +170,7 @@ public IRubyObject doYield(ThreadContext context, Block block, IRubyObject value
IRubyObject[] args;
if (value == null) { // no args case from BlockBody.yieldSpecific
args = IRubyObject.NULL_ARRAY;
} else if (blockArity >= -1 && blockArity <= 1) {
} else if (!getSignature().hasKwargs() && blockArity >= -1 && blockArity <= 1) {
args = new IRubyObject[] { value };
} else {
args = toAry(context, value);
Loading