[GR-17173] [GR-17194] Fix ReadCallerFrameNode to still optimize for multiple callers and fix $~ in grep and grep_v.

PullRequest: truffleruby/955

Author: eregon

CHANGELOG.md

@@ -11,6 +11,7 @@ Bug fixes:
 * Allow `Time#strftime` to be called with binary format strings.
 * Do not modify the argument passed to `IO#write` when the encoding does not match (#1714).
 * Use the class where the method was defined to check if an `UnboundMethod` can be used for `#define_method` (#1710).
+* Fixed setting `$~` for `Enumerable` and `Enumerator::Lazy`'s `#grep` and `#grep_v`.
 
 Compatibility:
 

spec/ruby/core/enumerable/grep_spec.rb

@@ -29,6 +29,28 @@ class EnumerableSpecGrep2; def ===(obj); /^ca/ =~ obj; end; end
     ary.grep(/a(b)a/) { $1 }.should == ["b", "b"]
   end
 
+  it "sets $~ in the block" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].grep(/b/) { |e|
+      e.should == "abc"
+      $&.should == "b"
+    }
+
+    # Set by the failed match of "def"
+    $~.should == nil
+  end
+
+  it "sets $~ to the last match when given no block" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].grep(/b/).should == ["abc"]
+
+    # Set by the failed match of "def"
+    $~.should == nil
+
+    ["abc", "def"].grep(/e/)
+    $&.should == "e"
+  end
+
   describe "with a block" do
     before :each do
       @numerous = EnumerableSpecs::Numerous.new(*(0..9).to_a)

spec/ruby/core/enumerable/grep_v_spec.rb

@@ -9,6 +9,28 @@ def (@odd_matcher = BasicObject.new).===(obj)
     end
   end
 
+  it "sets $~ in the block" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].grep_v(/e/) { |e|
+      e.should == "abc"
+      $~.should == nil
+    }
+
+    # Set by the match of "def"
+    $&.should == "e"
+  end
+
+  it "sets $~ to the last match when given no block" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].grep_v(/e/).should == ["abc"]
+
+    # Set by the match of "def"
+    $&.should == "e"
+
+    ["abc", "def"].grep_v(/b/)
+    $&.should == nil
+  end
+
   describe "without block" do
     it "returns an Array of matched elements" do
       @numerous.grep_v(@odd_matcher).should == [0, 2, 4, 6, 8]

spec/ruby/core/enumerator/lazy/grep_spec.rb

@@ -33,6 +33,39 @@
     Enumerator::Lazy.new(Object.new, 100) {}.grep(Object).size.should == nil
   end
 
+  it "sets $~ in the block" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].lazy.grep(/b/) { |e|
+      e.should == "abc"
+      $&.should == "b"
+    }.force
+
+    # Set by the failed match of "def"
+    $~.should == nil
+  end
+
+  it "sets $~ in the next block with each" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].lazy.grep(/b/).each { |e|
+      e.should == "abc"
+      $&.should == "b"
+    }
+
+    # Set by the failed match of "def"
+    $~.should == nil
+  end
+
+  it "sets $~ in the next block with map" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].lazy.grep(/b/).map { |e|
+      e.should == "abc"
+      $&.should == "b"
+    }.force
+
+    # Set by the failed match of "def"
+    $~.should == nil
+  end
+
   describe "when the returned lazy enumerator is evaluated by Enumerable#first" do
     it "stops after specified times when not given a block" do
       (0..Float::INFINITY).lazy.grep(Integer).first(3).should == [0, 1, 2]

spec/ruby/core/enumerator/lazy/grep_v_spec.rb

@@ -31,6 +31,39 @@
     Enumerator::Lazy.new(Object.new, 100) {}.grep_v(Object).size.should == nil
   end
 
+  it "sets $~ in the block" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].lazy.grep_v(/e/) { |e|
+      e.should == "abc"
+      $~.should == nil
+    }.force
+
+    # Set by the match of "def"
+    $&.should == "e"
+  end
+
+  it "sets $~ in the next block with each" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].lazy.grep_v(/e/).each { |e|
+      e.should == "abc"
+      $~.should == nil
+    }
+
+    # Set by the match of "def"
+    $&.should == "e"
+  end
+
+  it "sets $~ in the next block with map" do
+    "z" =~ /z/ # Reset $~
+    ["abc", "def"].lazy.grep_v(/e/).map { |e|
+      e.should == "abc"
+      $~.should == nil
+    }.force
+
+    # Set by the match of "def"
+    $&.should == "e"
+  end
+
   describe "when the returned lazy enumerator is evaluated by Enumerable#first" do
     it "stops after specified times when not given a block" do
       (0..Float::INFINITY).lazy.grep_v(3..5).first(3).should == [0, 1, 2]

src/main/java/org/truffleruby/language/arguments/ReadCallerFrameNode.java

@@ -25,43 +25,49 @@
 public class ReadCallerFrameNode extends RubyBaseNode {
 
     private final ConditionProfile callerFrameProfile = ConditionProfile.createBinaryProfile();
-    @CompilationFinal private volatile boolean firstCall = true;
+    @CompilationFinal private volatile boolean deoptWhenNotPassedCallerFrame = true;
 
     public static ReadCallerFrameNode create() {
         return new ReadCallerFrameNode();
     }
 
     public MaterializedFrame execute(VirtualFrame frame) {
-        // Avoid polluting the profile for the first call which has to use getCallerFrame()
-        if (firstCall) {
-            CompilerDirectives.transferToInterpreterAndInvalidate();
-            firstCall = false;
-            notifyCallerToSendFrame();
-            return getCallerFrame();
-        }
-
         final MaterializedFrame callerFrame = RubyArguments.getCallerFrame(frame);
 
         if (callerFrameProfile.profile(callerFrame != null)) {
             return callerFrame;
         } else {
+            // Every time the caller of the method using ReadCallerFrameNode changes,
+            // we need to notify the caller's CachedDispatchNode to pass us the frame next time.
+            if (deoptWhenNotPassedCallerFrame) {
+                // Invalidate because deoptWhenNotPassedCallerFrame might change and require recompilation
+                CompilerDirectives.transferToInterpreterAndInvalidate();
+            }
             return getCallerFrame();
         }
     }
 
-    private void notifyCallerToSendFrame() {
+    @TruffleBoundary
+    private MaterializedFrame getCallerFrame() {
+        if (!notifyCallerToSendFrame()) {
+            // If we fail to notify the call node (e.g., because it is a UncachedDispatchNode which is not handled yet),
+            // we don't want to deoptimize this CallTarget on every call.
+            deoptWhenNotPassedCallerFrame = false;
+        }
+        return getContext().getCallStack().getCallerFrameIgnoringSend().getFrame(FrameInstance.FrameAccess.MATERIALIZE).materialize();
+    }
+
+    private boolean notifyCallerToSendFrame() {
         final Node callerNode = getContext().getCallStack().getCallerNode(0, false);
         if (callerNode instanceof DirectCallNode) {
             final Node parent = callerNode.getParent();
             if (parent instanceof CachedDispatchNode) {
                 ((CachedDispatchNode) parent).startSendingOwnFrame();
+                return true;
             }
         }
-    }
 
-    @TruffleBoundary
-    private MaterializedFrame getCallerFrame() {
-        return getContext().getCallStack().getCallerFrameIgnoringSend().getFrame(FrameInstance.FrameAccess.MATERIALIZE).materialize();
+        return false;
     }
 
 }

src/main/ruby/truffleruby/core/enumerable.rb

@@ -357,8 +357,9 @@ def grep(pattern, &block)
     if block_given?
       each do
         o = Truffle.single_block_arg
-        if pattern === o
-          Truffle::RegexpOperations.set_last_match($~, block.binding)
+        matches = pattern === o
+        Truffle::RegexpOperations.set_last_match($~, block.binding)
+        if matches
           ary << yield(o)
         end
       end
@@ -376,25 +377,27 @@ def grep(pattern, &block)
     ary
   end
 
-  def grep_v(pattern)
+  def grep_v(pattern, &block)
     ary = []
 
     if block_given?
       each do
         o = Truffle.single_block_arg
-        unless pattern === o
-          # Regexp.set_block_last_match # TODO BJF Aug 1, 2016 Investigate for removal
+        matches = pattern === o
+        Truffle::RegexpOperations.set_last_match($~, block.binding)
+        unless matches
           ary << yield(o)
         end
       end
     else
       each do
         o = Truffle.single_block_arg
         unless pattern === o
-          # Regexp.set_block_last_match # TODO BJF Aug 1, 2016 Investigate for removal
           ary << o
         end
       end
+
+      Truffle::RegexpOperations.set_last_match($~, Truffle.invoke_primitive(:caller_binding))
     end
 
     ary

src/main/ruby/truffleruby/core/enumerator.rb

@@ -391,44 +391,35 @@ def reject
     end
 
     def grep(pattern, &block)
-      if block_given?
-        Lazy.new(self, nil) do |yielder, *args|
-          val = args.length >= 2 ? args : args.first
-          if pattern === val
-            Truffle::RegexpOperations.set_last_match($~, block.binding)
+      binding = block ? block.binding : Truffle.invoke_primitive(:caller_binding)
+
+      Lazy.new(self, nil) do |yielder, *args|
+        val = args.length >= 2 ? args : args.first
+        matches = pattern === val
+        Truffle::RegexpOperations.set_last_match($~, binding)
+
+        if matches
+          if block
             yielder.yield yield(val)
-          end
-        end
-      else
-        lazy = Lazy.new(self, nil) do |yielder, *args|
-          val = args.length >= 2 ? args : args.first
-          if pattern === val
+          else
             yielder.yield val
           end
-
-          Truffle::RegexpOperations.set_last_match($~, Truffle.invoke_primitive(:caller_binding))
         end
-
-        Truffle::RegexpOperations.set_last_match($~, Truffle.invoke_primitive(:caller_binding))
-
-        lazy
       end
     end
 
-    def grep_v(pattern)
-      if block_given?
-        Lazy.new(self, nil) do |yielder, *args|
-          val = args.length >= 2 ? args : args.first
-          unless pattern === val
-            # Regexp.set_block_last_match # TODO BJF Aug 2, 2016 Investigate for removal
+    def grep_v(pattern, &block)
+      binding = block ? block.binding : Truffle.invoke_primitive(:caller_binding)
+
+      Lazy.new(self, nil) do |yielder, *args|
+        val = args.length >= 2 ? args : args.first
+        matches = pattern === val
+        Truffle::RegexpOperations.set_last_match($~, binding)
+
+        unless matches
+          if block
             yielder.yield yield(val)
-          end
-        end
-      else
-        Lazy.new(self, nil) do |yielder, *args|
-          val = args.length >= 2 ? args : args.first
-          unless pattern === val
-            # Regexp.set_block_last_match # TODO BJF Aug 2, 2016 Investigate for removal
+          else
             yielder.yield val
           end
         end