[GR-15774] [GR-8022] Block args to block fix.

PullRequest: truffleruby/835

Author: aardvark179

CHANGELOG.md

@@ -12,6 +12,7 @@ Bug fixes:
 * Return a `FFI::Function` correctly for functions returning a callback.
 * Convert to intuitive Ruby exceptions when INVOKE fails (#1690).
 * Implemented `FFI::Pointer#clear` (#1687).
+* Procs will now yield to the block in their declaration context even when called with a block argument (#1657).
 
 Compatibility
 

spec/ruby/core/proc/new_spec.rb

@@ -190,6 +190,17 @@ def some_method
 
       prc.call.should == "hello"
     end
+
+    it "uses the implicit block from an enclosing method when called inside a block" do
+      def some_method
+        proc do |&block|
+          Proc.new
+        end.call { "failing" }
+      end
+      prc = some_method { "hello" }
+
+      prc.call.should == "hello"
+    end
   end
 
   ruby_version_is "2.7" do

spec/ruby/core/proc/shared/call_arguments.rb

@@ -4,4 +4,26 @@
     lambda {|&b| b.send(@method)}.send(@method) {1 + 1}.should == 2
     proc {|&b| b.send(@method)}.send(@method) {1 + 1}.should == 2
   end
+
+  it "yields to the block given at declaration and not to the block argument" do
+    proc_creator = Object.new
+    def proc_creator.create
+      Proc.new do |&b|
+        yield
+      end
+    end
+    a_proc = proc_creator.create { 7 }
+    a_proc.send(@method) { 3 }.should == 7
+  end
+
+  it "can call its block argument declared with a block argument" do
+    proc_creator = Object.new
+    def proc_creator.create(method_name)
+      Proc.new do |&b|
+        yield + b.send(method_name)
+      end
+    end
+    a_proc = proc_creator.create(@method) { 7 }
+    a_proc.call { 3 }.should == 10
+  end
 end

spec/ruby/language/yield_spec.rb

@@ -19,6 +19,10 @@
     it "ignores assignment to the explicit block argument and calls the passed block" do
       @y.ze { 42 }.should == 42
     end
+
+    it "does not pass a named block to the block being yielded to" do
+      @y.z() { |&block| block == nil }.should == true
+    end
   end
 
   describe "taking a single argument" do

spec/tags/core/kernel/block_given_tags.txt

@@ -32,10 +32,12 @@
 import org.truffleruby.language.arguments.ReadCallerFrameNode;
 import org.truffleruby.language.arguments.RubyArguments;
 import org.truffleruby.language.control.RaiseException;
+import org.truffleruby.language.locals.FindDeclarationVariableNodes;
 import org.truffleruby.language.locals.ReadFrameSlotNode;
 import org.truffleruby.language.locals.ReadFrameSlotNodeGen;
 import org.truffleruby.language.locals.WriteFrameSlotNode;
 import org.truffleruby.language.locals.WriteFrameSlotNodeGen;
+import org.truffleruby.language.locals.FindDeclarationVariableNodes.FrameSlotAndDepth;
 import org.truffleruby.language.objects.AllocateObjectNode;
 
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
@@ -125,40 +127,6 @@ public static void insertAncestorFrame(RubyContext context, DynamicObject bindin
         newFrame(binding, newFrameDescriptor(context));
     }
 
-    protected static class FrameSlotAndDepth {
-        private final FrameSlot slot;
-        private final int depth;
-
-        public FrameSlotAndDepth(FrameSlot slot, int depth) {
-            this.slot = slot;
-            this.depth = depth;
-        }
-
-        public FrameSlot getSlot() {
-            return slot;
-        }
-    }
-
-    public static FrameSlotAndDepth findFrameSlotOrNull(String identifier, MaterializedFrame frame) {
-        int depth = 0;
-        while (frame != null) {
-            final FrameSlot frameSlot = frame.getFrameDescriptor().findFrameSlot(identifier);
-            if (frameSlot != null) {
-                return new FrameSlotAndDepth(frameSlot, depth);
-            }
-
-            frame = RubyArguments.getDeclarationFrame(frame);
-            depth++;
-        }
-        return null;
-    }
-
-    public static FrameSlotAndDepth findFrameSlot(String identifier, FrameDescriptor frameDescriptor) {
-        final FrameSlot frameSlot = frameDescriptor.findFrameSlot(identifier);
-        assert frameSlot != null;
-        return new FrameSlotAndDepth(frameSlot, 0);
-    }
-
     public static boolean hiddenVariable(String name) {
         assert !name.isEmpty();
         return name.startsWith("$") || name.charAt(0) == TranslatorEnvironment.TEMP_PREFIX;
@@ -179,7 +147,7 @@ public DynamicObject dup(DynamicObject binding) {
         }
     }
 
-    @ImportStatic(BindingNodes.class)
+    @ImportStatic({ BindingNodes.class, FindDeclarationVariableNodes.class })
     @CoreMethod(names = "local_variable_defined?", required = 1)
     @NodeChild(value = "binding", type = RubyNode.class)
     @NodeChild(value = "name", type = RubyNode.class)
@@ -205,7 +173,7 @@ public boolean localVariableDefinedCached(DynamicObject binding, String name,
         @TruffleBoundary
         @Specialization(guards = "!hiddenVariable(name)")
         public boolean localVariableDefinedUncached(DynamicObject binding, String name) {
-            return findFrameSlotOrNull(name, getFrame(binding)) != null;
+            return FindDeclarationVariableNodes.findFrameSlotOrNull(name, getFrame(binding)) != null;
         }
 
         @TruffleBoundary
@@ -231,31 +199,15 @@ public RubyNode coerceToString(RubyNode name) {
             return NameToJavaStringNodeGen.create(name);
         }
 
-        @Specialization(guards = {
-                "name == cachedName",
-                "!hiddenVariable(cachedName)",
-                "cachedFrameSlot != null",
-                "getFrameDescriptor(binding) == descriptor"
-        }, limit = "getCacheLimit()")
-        public Object localVariableGetCached(DynamicObject binding, String name,
-                @Cached("name") String cachedName,
-                @Cached("getFrameDescriptor(binding)") FrameDescriptor descriptor,
-                @Cached("findFrameSlotOrNull(name, getFrame(binding))") FrameSlotAndDepth cachedFrameSlot,
-                @Cached("createReadNode(cachedFrameSlot)") ReadFrameSlotNode readLocalVariableNode) {
-            final MaterializedFrame frame = RubyArguments.getDeclarationFrame(getFrame(binding), cachedFrameSlot.depth);
-            return readLocalVariableNode.executeRead(frame);
-        }
-
-        @TruffleBoundary
         @Specialization(guards = "!hiddenVariable(name)")
-        public Object localVariableGetUncached(DynamicObject binding, String name) {
+        public Object localVariableGetUncached(DynamicObject binding, String name,
+                @Cached("create(null)") FindDeclarationVariableNodes.FindAndReadDeclarationVariableNode readNode) {
             MaterializedFrame frame = getFrame(binding);
-            FrameSlotAndDepth frameSlot = findFrameSlotOrNull(name, frame);
-            if (frameSlot != null) {
-                return RubyArguments.getDeclarationFrame(frame, frameSlot.depth).getValue(frameSlot.slot);
-            } else {
+            Object result = readNode.execute(frame, name);
+            if (result == null) {
                 throw new RaiseException(getContext(), coreExceptions().nameErrorLocalVariableNotDefined(name, binding, this));
             }
+            return result;
         }
 
         @TruffleBoundary
@@ -264,21 +216,13 @@ public Object localVariableGetLastLine(DynamicObject binding, String name) {
             throw new RaiseException(getContext(), coreExceptions().nameError("Bad local variable name", binding, name, this));
         }
 
-        protected ReadFrameSlotNode createReadNode(FrameSlotAndDepth frameSlot) {
-            if (frameSlot == null) {
-                return null;
-            } else {
-                return ReadFrameSlotNodeGen.create(frameSlot.slot);
-            }
-        }
-
         protected int getCacheLimit() {
             return getContext().getOptions().BINDING_LOCAL_VARIABLE_CACHE;
         }
 
     }
 
-    @ImportStatic(BindingNodes.class)
+    @ImportStatic({ BindingNodes.class, FindDeclarationVariableNodes.class })
     @CoreMethod(names = "local_variable_set", required = 2)
     @NodeChild(value = "binding", type = RubyNode.class)
     @NodeChild(value = "name", type = RubyNode.class)
@@ -326,7 +270,7 @@ public Object localVariableSetNewCached(DynamicObject binding, String name, Obje
         @Specialization(guards = "!hiddenVariable(name)")
         public Object localVariableSetUncached(DynamicObject binding, String name, Object value) {
             MaterializedFrame frame = getFrame(binding);
-            final FrameSlotAndDepth frameSlot = findFrameSlotOrNull(name, frame);
+            final FrameSlotAndDepth frameSlot = FindDeclarationVariableNodes.findFrameSlotOrNull(name, frame);
             final FrameSlot slot;
             if (frameSlot != null) {
                 frame = RubyArguments.getDeclarationFrame(frame, frameSlot.depth);

src/main/java/org/truffleruby/core/binding/BindingNodes.java

@@ -18,8 +18,10 @@
 import org.truffleruby.language.RubyNode;
 import org.truffleruby.language.dispatch.RubyCallNode;
 import org.truffleruby.language.dispatch.RubyCallNodeParameters;
+import org.truffleruby.language.literal.NilLiteralNode;
 import org.truffleruby.language.methods.BlockDefinitionNode;
 import org.truffleruby.language.methods.InternalMethod;
+import org.truffleruby.parser.TranslatorEnvironment;
 
 /**
  * We inline basic operations as it makes little sense to compile them in isolation without the
@@ -138,7 +140,7 @@ private InternalMethod getMethod(DynamicObject module, String name) {
         return method;
     }
 
-    public RubyNode createCallNode(RubyCallNodeParameters callParameters) {
+    public RubyNode createCallNode(RubyCallNodeParameters callParameters, TranslatorEnvironment environment) {
         if (!context.getOptions().BASICOPS_INLINE ||
                 callParameters.isSplatted() || callParameters.isSafeNavigation()) {
             return new RubyCallNode(callParameters);
@@ -171,7 +173,7 @@ public RubyNode createCallNode(RubyCallNodeParameters callParameters) {
                     break;
                 case "block_given?":
                     if (callParameters.isIgnoreVisibility()) {
-                        return InlinedBlockGivenNodeGen.create(context, callParameters, self);
+                        return InlinedBlockGivenNodeGen.create(callParameters, environment, self);
                     }
                     break;
                 case "nil?":

src/main/java/org/truffleruby/core/inlined/CoreMethods.java

@@ -10,9 +10,10 @@
 package org.truffleruby.core.inlined;
 
 import org.truffleruby.RubyContext;
-import org.truffleruby.language.arguments.RubyArguments;
+import org.truffleruby.language.RubyNode;
 import org.truffleruby.language.dispatch.RubyCallNodeParameters;
 import org.truffleruby.language.methods.LookupMethodNode;
+import org.truffleruby.parser.TranslatorEnvironment;
 
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.Specialization;
@@ -22,16 +23,19 @@ public abstract class InlinedBlockGivenNode extends UnaryInlinedOperationNode {
 
     protected static final String METHOD = "block_given?";
 
-    public InlinedBlockGivenNode(RubyContext context, RubyCallNodeParameters callNodeParameters) {
+    @Child protected RubyNode readMethodBlockNode;
+
+    public InlinedBlockGivenNode(RubyCallNodeParameters callNodeParameters,  TranslatorEnvironment environment) {
         super(callNodeParameters);
+        this.readMethodBlockNode = environment.findLocalVarOrNilNode(TranslatorEnvironment.METHOD_BLOCK_NAME, null);
     }
 
     @Specialization(guards = {
             "lookupNode.lookup(frame, self, METHOD) == coreMethods().BLOCK_GIVEN",
     }, assumptions = "assumptions", limit = "1")
     boolean blockGiven(VirtualFrame frame, Object self,
             @Cached("createIgnoreVisibility()") LookupMethodNode lookupNode) {
-        return RubyArguments.getBlock(frame) != null;
+        return readMethodBlockNode.execute(frame) != nil();
     }
 
     @Specialization

src/main/java/org/truffleruby/core/inlined/InlinedBlockGivenNode.java

@@ -109,6 +109,9 @@
 import org.truffleruby.language.globals.ReadGlobalVariableNodeGen;
 import org.truffleruby.language.loader.CodeLoader;
 import org.truffleruby.language.loader.RequireNode;
+import org.truffleruby.language.locals.ReadFrameSlotNode;
+import org.truffleruby.language.locals.ReadFrameSlotNodeGen;
+import org.truffleruby.language.locals.FindDeclarationVariableNodes.FindAndReadDeclarationVariableNode;
 import org.truffleruby.language.methods.DeclarationContext;
 import org.truffleruby.language.methods.InternalMethod;
 import org.truffleruby.language.methods.LookupMethodNode;
@@ -137,6 +140,7 @@
 import org.truffleruby.language.objects.shared.SharedObjects;
 import org.truffleruby.parser.ParserContext;
 import org.truffleruby.parser.RubySource;
+import org.truffleruby.parser.TranslatorEnvironment;
 
 import java.io.File;
 import java.io.PrintStream;
@@ -283,15 +287,15 @@ protected SourceSection getCallerSourceSection() {
     @CoreMethod(names = "block_given?", isModuleFunction = true)
     public abstract static class BlockGivenNode extends CoreMethodArrayArgumentsNode {
 
-        @Child ReadCallerFrameNode callerFrameNode = new ReadCallerFrameNode(CallerFrameAccess.ARGUMENTS);
+        @Child ReadCallerFrameNode callerFrameNode = new ReadCallerFrameNode(CallerFrameAccess.MATERIALIZE);
 
         @Specialization
         public boolean blockGiven(VirtualFrame frame,
+                @Cached("create(nil())") FindAndReadDeclarationVariableNode readNode,
                 @Cached("createBinaryProfile()") ConditionProfile blockProfile) {
-            Frame callerFrame = callerFrameNode.execute(frame);
-            return blockProfile.profile(RubyArguments.getBlock(callerFrame) != null);
+            MaterializedFrame callerFrame = callerFrameNode.execute(frame).materialize();
+            return blockProfile.profile(readNode.execute(callerFrame, TranslatorEnvironment.METHOD_BLOCK_NAME) != nil());
         }
-
     }
 
     @CoreMethod(names = "__callee__", isModuleFunction = true)
@@ -995,11 +999,12 @@ public abstract static class LambdaNode extends CoreMethodArrayArgumentsNode {
 
         @TruffleBoundary
         @Specialization
-        public DynamicObject lambda(NotProvided block) {
-            final Frame parentFrame = getContext().getCallStack().getCallerFrameIgnoringSend(0).getFrame(FrameAccess.READ_ONLY);
-            final DynamicObject parentBlock = RubyArguments.getBlock(parentFrame);
+        public DynamicObject lambda(NotProvided block,
+                                    @Cached("create(nil())") FindAndReadDeclarationVariableNode readNode) {
+            final MaterializedFrame parentFrame = getContext().getCallStack().getCallerFrameIgnoringSend(0).getFrame(FrameAccess.MATERIALIZE).materialize();
+            DynamicObject parentBlock = (DynamicObject) readNode.execute(parentFrame, TranslatorEnvironment.METHOD_BLOCK_NAME);
 
-            if (parentBlock == null) {
+            if (parentBlock == nil()) {
                 throw new RaiseException(getContext(), coreExceptions().argumentError("tried to create Proc object without a block", this));
             } else {
                 warnProcWithoutBlock();