Skip to content

Commit

Permalink
Fixes #2511 - Keyword method parameters cannot take a frozen hash. Fixes
Browse files Browse the repository at this point in the history 
 #2015 - Method keyword arguments are destructive
  • Loading branch information
@enebo
enebo committed Apr 7, 2015
1 parent 6cff710 commit a8e705c
Show file tree
Hide file tree
Showing 10 changed files with 80 additions and 34 deletions.
7 changes: 5 additions & 2 deletions core/src/main/java/org/jruby/compiler/JITCompiler.java
View file
Expand Up @@ -302,7 +302,9 @@ public void run() {
PUBLIC_LOOKUP.findStatic(sourceClass, jittedName, signatures.get(-1)),
method.getIRMethod(),
method.getVisibility(),
method.getImplementationClass()));
method.getImplementationClass(),
method.getIRMethod().receivesKeywordArgs()));

} else {
// also specific-arity
for (Map.Entry<Integer, MethodType> entry : signatures.entrySet()) {
Expand All @@ -315,7 +317,8 @@ public void run() {
entry.getKey(),
method.getIRMethod(),
method.getVisibility(),
method.getImplementationClass()));
method.getImplementationClass(),
method.getIRMethod().receivesKeywordArgs()));
break;
}
}
Expand Down
16 changes: 5 additions & 11 deletions core/src/main/java/org/jruby/internal/runtime/methods/CompiledIRMetaClassBody.java
View file
Expand Up @@ -2,7 +2,6 @@

import org.jruby.RubyModule;
import org.jruby.ir.IRFlags;
import org.jruby.ir.IRMetaClassBody;
import org.jruby.ir.IRScope;
import org.jruby.parser.StaticScope;
import org.jruby.runtime.Block;
Expand All @@ -14,18 +13,15 @@
import java.lang.invoke.MethodHandle;

public class CompiledIRMetaClassBody extends CompiledIRMethod {
private final IRMetaClassBody irMetaClassBody;
private final boolean reuseParentDynScope;
private final boolean pushNewDynScope;
private final boolean popDynScope;

public CompiledIRMetaClassBody(MethodHandle handle, IRScope scope, RubyModule implementationClass) {
super(handle, scope, Visibility.PUBLIC, implementationClass);
super(handle, scope, Visibility.PUBLIC, implementationClass, scope.receivesKeywordArgs());

irMetaClassBody = (IRMetaClassBody)scope;
this.reuseParentDynScope = scope.getFlags().contains(IRFlags.REUSE_PARENT_DYNSCOPE);
this.pushNewDynScope = !scope.getFlags().contains(IRFlags.DYNSCOPE_ELIMINATED) && !this.reuseParentDynScope;
this.popDynScope = this.pushNewDynScope || this.reuseParentDynScope;
boolean reuseParentDynScope = scope.getFlags().contains(IRFlags.REUSE_PARENT_DYNSCOPE);
this.pushNewDynScope = !scope.getFlags().contains(IRFlags.DYNSCOPE_ELIMINATED) && !reuseParentDynScope;
this.popDynScope = this.pushNewDynScope || reuseParentDynScope;
}

public String[] getParameterList() {
Expand Down Expand Up @@ -56,8 +52,6 @@ protected void pre(ThreadContext context, StaticScope staticScope, RubyModule im

@Override
public DynamicMethod dup() {
CompiledIRMetaClassBody x = new CompiledIRMetaClassBody(variable, method, implementationClass);

return x;
return new CompiledIRMetaClassBody(variable, method, implementationClass);
}
}
21 changes: 14 additions & 7 deletions core/src/main/java/org/jruby/internal/runtime/methods/CompiledIRMethod.java
View file
Expand Up @@ -3,6 +3,7 @@
import org.jruby.RubyModule;
import org.jruby.ir.IRMethod;
import org.jruby.ir.IRScope;
import org.jruby.ir.runtime.IRRuntimeHelpers;
import org.jruby.parser.StaticScope;
import org.jruby.runtime.Arity;
import org.jruby.runtime.Block;
Expand All @@ -18,8 +19,6 @@
import org.jruby.runtime.Helpers;

public class CompiledIRMethod extends JavaMethod implements MethodArgs2, PositionAware {
private static final Logger LOG = LoggerFactory.getLogger("CompiledIRMethod");

protected final MethodHandle variable;

protected final MethodHandle specific;
Expand All @@ -31,15 +30,19 @@ public class CompiledIRMethod extends JavaMethod implements MethodArgs2, Positio
private final StaticScope staticScope;
private final boolean hasExplicitCallProtocol;

public CompiledIRMethod(MethodHandle variable, IRScope method, Visibility visibility, RubyModule implementationClass) {
this(variable, null, -1, method, visibility, implementationClass);
public CompiledIRMethod(MethodHandle variable, IRScope method, Visibility visibility,
RubyModule implementationClass, boolean hasKwargs) {
this(variable, null, -1, method, visibility, implementationClass, hasKwargs);
}

public CompiledIRMethod(MethodHandle variable, MethodHandle specific, int specificArity, IRScope method, Visibility visibility, RubyModule implementationClass) {
public CompiledIRMethod(MethodHandle variable, MethodHandle specific, int specificArity, IRScope method,
Visibility visibility, RubyModule implementationClass, boolean hasKwargs) {
super(implementationClass, visibility, CallConfiguration.FrameNoneScopeNone, method.getName());
this.variable = variable;
this.specific = specific;
this.specificArity = specificArity;
// deopt unboxing if we have to process kwargs hash (although this really has nothing to do with arg
// unboxing -- it was a simple path to hacking this in).
this.specificArity = hasKwargs ? -1 : specificArity;
this.method = method;
this.method.getStaticScope().determineModule();
this.arity = calculateArity();
Expand Down Expand Up @@ -99,6 +102,8 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz
try {
if (!hasExplicitCallProtocol) return callNoProtocol(context, self, clazz, name, args, block);

if (specificArity == -1) IRRuntimeHelpers.frobnicateKwargsArgument(context, arity.required(), args);

return (IRubyObject)this.variable.invokeExact(context, staticScope, self, args, block, implementationClass, name);
} catch (Throwable t) {
Helpers.throwException(t);
Expand Down Expand Up @@ -172,6 +177,8 @@ private IRubyObject callNoProtocol(ThreadContext context, IRubyObject self, Ruby
RubyModule implementationClass = this.implementationClass;
pre(context, staticScope, implementationClass, self, name, block);

if (specificArity == -1) IRRuntimeHelpers.frobnicateKwargsArgument(context, arity.required(), args);

try {
return (IRubyObject)this.variable.invokeExact(context, staticScope, self, args, block, implementationClass, name);
} finally {
Expand Down Expand Up @@ -237,7 +244,7 @@ public IRubyObject callNoProtocol(ThreadContext context, IRubyObject self, RubyM

@Override
public DynamicMethod dup() {
return new CompiledIRMethod(variable, specific, specificArity, method, visibility, implementationClass);
return new CompiledIRMethod(variable, specific, specificArity, method, visibility, implementationClass, specificArity == -1);
}

public String getFile() {
Expand Down
22 changes: 22 additions & 0 deletions core/src/main/java/org/jruby/internal/runtime/methods/MixedModeIRMethod.java
View file
Expand Up @@ -5,6 +5,7 @@
import org.jruby.MetaClass;
import org.jruby.Ruby;
import org.jruby.RubyClass;
import org.jruby.RubyHash;
import org.jruby.RubyModule;
import org.jruby.ir.*;
import org.jruby.ir.interpreter.InterpreterContext;
Expand Down Expand Up @@ -108,6 +109,18 @@ public InterpreterContext ensureInstrsReady() {
return method.getInterpreterContext();
}

protected IRubyObject frobnicateKwarg(ThreadContext context, int requiredArguments, int argCount, IRubyObject arg) {
if (argCount > requiredArguments && arg != null && arg instanceof RubyHash) {
RubyHash kwargs = (RubyHash) arg;
kwargs = (RubyHash) kwargs.dup(context);
kwargs.setFrozen(false);
return kwargs;
}

return arg;
}


@Override
public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) {
if (IRRuntimeHelpers.isDebug()) doDebug();
Expand All @@ -117,6 +130,8 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz
DynamicMethod jittedMethod = box.actualMethod;

if (jittedMethod != null) {
InterpreterContext ic = method.getInterpreterContext();
if (ic.receivesKeywordArguments()) IRRuntimeHelpers.frobnicateKwargsArgument(context, ic.getRequiredArgsCount(), args);
return jittedMethod.call(context, self, clazz, name, args, block);
} else {
return INTERPRET_METHOD(context, ensureInstrsReady(), getImplementationClass().getMethodLocation(), self, name, args, block);
Expand Down Expand Up @@ -187,6 +202,9 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz
DynamicMethod jittedMethod = box.actualMethod;

if (jittedMethod != null) {
InterpreterContext ic = method.getInterpreterContext();
if (ic.receivesKeywordArguments()) arg0 = frobnicateKwarg(context, ic.getRequiredArgsCount(), 1, arg0);

return jittedMethod.call(context, self, clazz, name, arg0, block);
} else {
return INTERPRET_METHOD(context, ensureInstrsReady(), getImplementationClass().getMethodLocation(), self, name, arg0, block);
Expand Down Expand Up @@ -222,6 +240,8 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz
DynamicMethod jittedMethod = box.actualMethod;

if (jittedMethod != null) {
InterpreterContext ic = method.getInterpreterContext();
if (ic.receivesKeywordArguments()) arg1 = frobnicateKwarg(context, ic.getRequiredArgsCount(), 2, arg1);
return jittedMethod.call(context, self, clazz, name, arg0, arg1, block);
} else {
return INTERPRET_METHOD(context, ensureInstrsReady(), getImplementationClass().getMethodLocation(), self, name, arg0, arg1, block);
Expand Down Expand Up @@ -257,6 +277,8 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz
DynamicMethod jittedMethod = box.actualMethod;

if (jittedMethod != null) {
InterpreterContext ic = method.getInterpreterContext();
if (ic.receivesKeywordArguments()) arg2 = frobnicateKwarg(context, ic.getRequiredArgsCount(), 2, arg2);
return jittedMethod.call(context, self, clazz, name, arg0, arg1, arg2, block);
} else {
return INTERPRET_METHOD(context, ensureInstrsReady(), getImplementationClass().getMethodLocation(), self, name, arg0, arg1, arg2, block);
Expand Down
2 changes: 0 additions & 2 deletions core/src/main/java/org/jruby/ir/IRMethod.java
View file
@@ -1,10 +1,8 @@
package org.jruby.ir;

import java.lang.invoke.MethodType;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.jruby.ast.MethodDefNode;
import org.jruby.ir.interpreter.InterpreterContext;
Expand Down
6 changes: 4 additions & 2 deletions core/src/main/java/org/jruby/ir/interpreter/InterpreterContext.java
View file
@@ -1,8 +1,6 @@
package org.jruby.ir.interpreter;

import java.util.Collection;
import java.util.List;
import org.jruby.ir.IRClosure;
import org.jruby.ir.IRFlags;
import org.jruby.ir.IRMetaClassBody;
import org.jruby.ir.IRMethod;
Expand Down Expand Up @@ -78,6 +76,10 @@ private Instr[] prepareBuildInstructions(List<Instr> instructions) {
return linearizedInstrArray;
}

public int getRequiredArgsCount() {
return getStaticScope().getRequiredArgs();
}

public IRScope getScope() {
return scope;
}
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
View file
Expand Up @@ -3,6 +3,7 @@
import org.jruby.RubyBoolean;
import org.jruby.RubyFixnum;
import org.jruby.RubyFloat;
import org.jruby.RubyHash;
import org.jruby.RubyModule;
import org.jruby.common.IRubyWarnings;
import org.jruby.exceptions.Unrescuable;
Expand Down Expand Up @@ -113,6 +114,8 @@ public IRubyObject interpret(ThreadContext context, IRubyObject self,
int ipc = 0;
Object exception = null;

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

StaticScope currScope = interpreterContext.getStaticScope();
DynamicScope currDynScope = context.getCurrentScope();
IRScope scope = currScope.getIRScope();
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/org/jruby/ir/interpreter/StartupInterpreterEngine.java
View file
Expand Up @@ -39,6 +39,8 @@ public IRubyObject interpret(ThreadContext context, IRubyObject self,
int ipc = 0;
Object exception = null;

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

StaticScope currScope = interpreterContext.getStaticScope();
DynamicScope currDynScope = context.getCurrentScope();
IRScope scope = currScope.getIRScope();
Expand Down
29 changes: 21 additions & 8 deletions core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
View file
Expand Up @@ -246,7 +246,7 @@ public static void defCompiledIRMethod(ThreadContext context, MethodHandle handl
Visibility currVisibility = context.getCurrentVisibility();
Visibility newVisibility = Helpers.performNormalMethodChecksAndDetermineVisibility(runtime, containingClass, rubyName, currVisibility);

DynamicMethod method = new CompiledIRMethod(handle, irScope, newVisibility, containingClass);
DynamicMethod method = new CompiledIRMethod(handle, irScope, newVisibility, containingClass, irScope.receivesKeywordArgs());

Helpers.addInstanceMethod(containingClass, rubyName, method, currVisibility, context, runtime);
}
Expand All @@ -263,7 +263,7 @@ public static void defCompiledIRClassMethod(ThreadContext context, IRubyObject o

RubyClass containingClass = obj.getSingletonClass();

DynamicMethod method = new CompiledIRMethod(handle, irScope, Visibility.PUBLIC, containingClass);
DynamicMethod method = new CompiledIRMethod(handle, irScope, Visibility.PUBLIC, containingClass, irScope.receivesKeywordArgs());

containingClass.addMethod(rubyName, method);

Expand Down Expand Up @@ -524,6 +524,19 @@ public static void checkArity(ThreadContext context, Object[] args, int required
}
}

// 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, int requiredArguments, IRubyObject[] args) {
RubyHash kwargs = IRRuntimeHelpers.extractKwargsHash(args, requiredArguments, true);

if (kwargs != null) {
kwargs = (RubyHash) kwargs.dup(context);
kwargs.setFrozen(false);
args[args.length - 1] = kwargs;
}
}

public static RubyHash extractKwargsHash(Object[] args, int requiredArgsCount, boolean receivesKwargs) {
if (!receivesKwargs) return null;
if (args.length <= requiredArgsCount) return null; // No kwarg because required args slurp them up.
Expand Down Expand Up @@ -1093,7 +1106,7 @@ public static DynamicMethod newInterpretedModuleBody(ThreadContext context, IRSc
@JIT
public static DynamicMethod newCompiledModuleBody(ThreadContext context, MethodHandle handle, IRScope irModule, Object rubyContainer) {
RubyModule newRubyModule = newRubyModuleFromIR(context, irModule, rubyContainer);
return new CompiledIRMethod(handle, irModule, Visibility.PUBLIC, newRubyModule);
return new CompiledIRMethod(handle, irModule, Visibility.PUBLIC, newRubyModule, false);
}

private static RubyModule newRubyModuleFromIR(ThreadContext context, IRScope irModule, Object rubyContainer) {
Expand All @@ -1117,7 +1130,7 @@ public static DynamicMethod newInterpretedClassBody(ThreadContext context, IRSco
public static DynamicMethod newCompiledClassBody(ThreadContext context, MethodHandle handle, IRScope irClassBody, Object container, Object superClass) {
RubyModule newRubyClass = newRubyClassFromIR(context.runtime, irClassBody, superClass, container);

return new CompiledIRMethod(handle, irClassBody, Visibility.PUBLIC, newRubyClass);
return new CompiledIRMethod(handle, irClassBody, Visibility.PUBLIC, newRubyClass, false);
}

public static RubyModule newRubyClassFromIR(Ruby runtime, IRScope irClassBody, Object superClass, Object container) {
Expand Down Expand Up @@ -1164,15 +1177,15 @@ public static void defInterpretedClassMethod(ThreadContext context, IRScope meth
public static void defCompiledClassMethod(ThreadContext context, MethodHandle handle, IRScope method, IRubyObject obj) {
RubyClass rubyClass = checkClassForDef(context, method, obj);

rubyClass.addMethod(method.getName(), new CompiledIRMethod(handle, method, Visibility.PUBLIC, rubyClass));
rubyClass.addMethod(method.getName(), new CompiledIRMethod(handle, method, Visibility.PUBLIC, rubyClass, method.receivesKeywordArgs()));
obj.callMethod(context, "singleton_method_added", context.runtime.fastNewSymbol(method.getName()));
}

@JIT
public static void defCompiledClassMethod(ThreadContext context, MethodHandle variable, MethodHandle specific, int specificArity, IRScope method, IRubyObject obj) {
RubyClass rubyClass = checkClassForDef(context, method, obj);

rubyClass.addMethod(method.getName(), new CompiledIRMethod(variable, specific, specificArity, method, Visibility.PUBLIC, rubyClass));
rubyClass.addMethod(method.getName(), new CompiledIRMethod(variable, specific, specificArity, method, Visibility.PUBLIC, rubyClass, method.receivesKeywordArgs()));
obj.callMethod(context, "singleton_method_added", context.runtime.fastNewSymbol(method.getName()));
}

Expand Down Expand Up @@ -1212,7 +1225,7 @@ public static void defCompiledInstanceMethod(ThreadContext context, MethodHandle
Visibility currVisibility = context.getCurrentVisibility();
Visibility newVisibility = Helpers.performNormalMethodChecksAndDetermineVisibility(runtime, clazz, method.getName(), currVisibility);

DynamicMethod newMethod = new CompiledIRMethod(handle, method, newVisibility, clazz);
DynamicMethod newMethod = new CompiledIRMethod(handle, method, newVisibility, clazz, method.receivesKeywordArgs());

Helpers.addInstanceMethod(clazz, method.getName(), newMethod, currVisibility, context, runtime);
}
Expand All @@ -1225,7 +1238,7 @@ public static void defCompiledInstanceMethod(ThreadContext context, MethodHandle
Visibility currVisibility = context.getCurrentVisibility();
Visibility newVisibility = Helpers.performNormalMethodChecksAndDetermineVisibility(runtime, clazz, method.getName(), currVisibility);

DynamicMethod newMethod = new CompiledIRMethod(variable, specific, specificArity, method, newVisibility, clazz);
DynamicMethod newMethod = new CompiledIRMethod(variable, specific, specificArity, method, newVisibility, clazz, method.receivesKeywordArgs());

Helpers.addInstanceMethod(clazz, method.getName(), newMethod, currVisibility, context, runtime);
}
Expand Down
6 changes: 4 additions & 2 deletions core/src/main/java/org/jruby/runtime/CompiledIRBlockBody.java
View file
Expand Up @@ -4,9 +4,7 @@
import org.jruby.ir.IRClosure;
import org.jruby.ir.IRFlags;
import org.jruby.ir.IRScope;
import org.jruby.ir.representations.CFG;
import org.jruby.ir.runtime.IRRuntimeHelpers;
import org.jruby.parser.StaticScope;
import org.jruby.runtime.Block.Type;
import org.jruby.runtime.builtin.IRubyObject;

Expand All @@ -17,6 +15,7 @@ public class CompiledIRBlockBody extends IRBlockBody {
protected final MethodHandle handle;
protected boolean pushScope;
protected boolean reuseParentScope;
protected boolean usesKwargs;

public CompiledIRBlockBody(MethodHandle handle, IRScope closure, long encodedSignature) {
super(closure.getStaticScope(), ((IRClosure)closure).getParameterList(), closure.getFileName(), closure.getLineNumber(), Signature.decode(encodedSignature));
Expand All @@ -25,6 +24,7 @@ public CompiledIRBlockBody(MethodHandle handle, IRScope closure, long encodedSig
// FIXME: duplicated from InterpreterContext
this.reuseParentScope = closure.getFlags().contains(IRFlags.REUSE_PARENT_DYNSCOPE);
this.pushScope = !closure.getFlags().contains(IRFlags.DYNSCOPE_ELIMINATED) && !this.reuseParentScope;
this.usesKwargs = closure.receivesKeywordArgs();

// Done in the interpreter (WrappedIRClosure) but we do it here
closure.getStaticScope().determineModule();
Expand Down Expand Up @@ -62,6 +62,8 @@ protected IRubyObject commonYieldPath(ThreadContext context, IRubyObject[] args,
}
this.evalType.set(EvalType.NONE);

if (usesKwargs) IRRuntimeHelpers.frobnicateKwargsArgument(context, arity.required(), args);

try {
return (IRubyObject)handle.invokeExact(context, getStaticScope(), self, args, block, binding.getMethod(), type);
} catch (Throwable t) {
Expand Down

0 comments on commit a8e705c

Please sign in to comment.