Revert "Fixes #2511 - Keyword method parameters cannot take a frozen …

…hash. Fixes #2015 - Method keyword arguments are destructive" This reverts commit a8e705c.
jruby · Apr 7, 2015 · 4c86703 · 4c86703
1 parent 8c09f1c
commit 4c86703
Show file tree

Hide file tree

Showing 10 changed files with 34 additions and 80 deletions.
diff --git a/core/src/main/java/org/jruby/compiler/JITCompiler.java b/core/src/main/java/org/jruby/compiler/JITCompiler.java
@@ -302,9 +302,7 @@ public void run() {
                                     PUBLIC_LOOKUP.findStatic(sourceClass, jittedName, signatures.get(-1)),
                                     method.getIRMethod(),
                                     method.getVisibility(),
-                                    method.getImplementationClass(),
-                                    method.getIRMethod().receivesKeywordArgs()));
-
+                                    method.getImplementationClass()));
                 } else {
                     // also specific-arity
                     for (Map.Entry<Integer, MethodType> entry : signatures.entrySet()) {
@@ -317,8 +315,7 @@ public void run() {
                                         entry.getKey(),
                                         method.getIRMethod(),
                                         method.getVisibility(),
-                                        method.getImplementationClass(),
-                                        method.getIRMethod().receivesKeywordArgs()));
+                                        method.getImplementationClass()));
                         break;
                     }
                 }

diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/CompiledIRMetaClassBody.java b/core/src/main/java/org/jruby/internal/runtime/methods/CompiledIRMetaClassBody.java
@@ -2,6 +2,7 @@
 
 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;
@@ -13,15 +14,18 @@
 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, scope.receivesKeywordArgs());
+        super(handle, scope, Visibility.PUBLIC, implementationClass);
 
-        boolean reuseParentDynScope = scope.getFlags().contains(IRFlags.REUSE_PARENT_DYNSCOPE);
-        this.pushNewDynScope = !scope.getFlags().contains(IRFlags.DYNSCOPE_ELIMINATED) && !reuseParentDynScope;
-        this.popDynScope = this.pushNewDynScope || reuseParentDynScope;
+        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;
     }
 
     public String[] getParameterList() {
@@ -52,6 +56,8 @@ protected void pre(ThreadContext context, StaticScope staticScope, RubyModule im
 
     @Override
     public DynamicMethod dup() {
-        return new CompiledIRMetaClassBody(variable, method, implementationClass);
+        CompiledIRMetaClassBody x = new CompiledIRMetaClassBody(variable, method, implementationClass);
+
+        return x;
     }
 }
diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/CompiledIRMethod.java b/core/src/main/java/org/jruby/internal/runtime/methods/CompiledIRMethod.java
@@ -3,7 +3,6 @@
 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;
@@ -19,6 +18,8 @@
 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;
@@ -30,19 +31,15 @@ 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, boolean hasKwargs) {
-        this(variable, null, -1, method, visibility, implementationClass, hasKwargs);
+    public CompiledIRMethod(MethodHandle variable, IRScope method, Visibility visibility, RubyModule implementationClass) {
+        this(variable, null, -1, method, visibility, implementationClass);
     }
 
-    public CompiledIRMethod(MethodHandle variable, MethodHandle specific, int specificArity, IRScope method,
-                            Visibility visibility, RubyModule implementationClass, boolean hasKwargs) {
+    public CompiledIRMethod(MethodHandle variable, MethodHandle specific, int specificArity, IRScope method, Visibility visibility, RubyModule implementationClass) {
         super(implementationClass, visibility, CallConfiguration.FrameNoneScopeNone, method.getName());
         this.variable = variable;
         this.specific = specific;
-        // 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.specificArity = specificArity;
         this.method = method;
         this.method.getStaticScope().determineModule();
         this.arity = calculateArity();
@@ -102,8 +99,6 @@ 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);
@@ -177,8 +172,6 @@ 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 {
@@ -244,7 +237,7 @@ public IRubyObject callNoProtocol(ThreadContext context, IRubyObject self, RubyM
 
     @Override
     public DynamicMethod dup() {
-        return new CompiledIRMethod(variable, specific, specificArity, method, visibility, implementationClass, specificArity == -1);
+        return new CompiledIRMethod(variable, specific, specificArity, method, visibility, implementationClass);
     }
 
     public String getFile() {

diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/MixedModeIRMethod.java b/core/src/main/java/org/jruby/internal/runtime/methods/MixedModeIRMethod.java
@@ -5,7 +5,6 @@
 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;
@@ -109,18 +108,6 @@ 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();
@@ -130,8 +117,6 @@ 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);
@@ -202,9 +187,6 @@ 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);
@@ -240,8 +222,6 @@ 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);
@@ -277,8 +257,6 @@ 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);

diff --git a/core/src/main/java/org/jruby/ir/IRMethod.java b/core/src/main/java/org/jruby/ir/IRMethod.java
@@ -1,8 +1,10 @@
 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;

diff --git a/core/src/main/java/org/jruby/ir/interpreter/InterpreterContext.java b/core/src/main/java/org/jruby/ir/interpreter/InterpreterContext.java
@@ -1,6 +1,8 @@
 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;
@@ -76,10 +78,6 @@ private Instr[] prepareBuildInstructions(List<Instr> instructions) {
         return linearizedInstrArray;
     }
 
-    public int getRequiredArgsCount() {
-        return getStaticScope().getRequiredArgs();
-    }
-
     public IRScope getScope() {
         return scope;
     }

diff --git a/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
@@ -3,7 +3,6 @@
 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;
@@ -114,8 +113,6 @@ 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();

diff --git a/core/src/main/java/org/jruby/ir/interpreter/StartupInterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/StartupInterpreterEngine.java
@@ -39,8 +39,6 @@ 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();

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -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, irScope.receivesKeywordArgs());
+        DynamicMethod method = new CompiledIRMethod(handle, irScope, newVisibility, containingClass);
 
         Helpers.addInstanceMethod(containingClass, rubyName, method, currVisibility, context, runtime);
     }
@@ -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, irScope.receivesKeywordArgs());
+        DynamicMethod method = new CompiledIRMethod(handle, irScope, Visibility.PUBLIC, containingClass);
 
         containingClass.addMethod(rubyName, method);
 
@@ -524,19 +524,6 @@ 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.
@@ -1106,7 +1093,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, false);
+        return new CompiledIRMethod(handle, irModule, Visibility.PUBLIC, newRubyModule);
     }
 
     private static RubyModule newRubyModuleFromIR(ThreadContext context, IRScope irModule, Object rubyContainer) {
@@ -1130,7 +1117,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, false);
+        return new CompiledIRMethod(handle, irClassBody, Visibility.PUBLIC, newRubyClass);
     }
 
     public static RubyModule newRubyClassFromIR(Ruby runtime, IRScope irClassBody, Object superClass, Object container) {
@@ -1177,15 +1164,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, method.receivesKeywordArgs()));
+        rubyClass.addMethod(method.getName(), new CompiledIRMethod(handle, method, Visibility.PUBLIC, rubyClass));
         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, method.receivesKeywordArgs()));
+        rubyClass.addMethod(method.getName(), new CompiledIRMethod(variable, specific, specificArity, method, Visibility.PUBLIC, rubyClass));
         obj.callMethod(context, "singleton_method_added", context.runtime.fastNewSymbol(method.getName()));
     }
 
@@ -1225,7 +1212,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, method.receivesKeywordArgs());
+        DynamicMethod newMethod = new CompiledIRMethod(handle, method, newVisibility, clazz);
 
         Helpers.addInstanceMethod(clazz, method.getName(), newMethod, currVisibility, context, runtime);
     }
@@ -1238,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(variable, specific, specificArity, method, newVisibility, clazz, method.receivesKeywordArgs());
+        DynamicMethod newMethod = new CompiledIRMethod(variable, specific, specificArity, method, newVisibility, clazz);
 
         Helpers.addInstanceMethod(clazz, method.getName(), newMethod, currVisibility, context, runtime);
     }

diff --git a/core/src/main/java/org/jruby/runtime/CompiledIRBlockBody.java b/core/src/main/java/org/jruby/runtime/CompiledIRBlockBody.java
@@ -4,7 +4,9 @@
 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;
 
@@ -15,7 +17,6 @@ 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));
@@ -24,7 +25,6 @@ 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();
@@ -62,8 +62,6 @@ 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) {