From aae8223c7076483f1f1641181088790b2f3a66dd Mon Sep 17 00:00:00 2001
From: Jeremy Evans <code@jeremyevans.net>
Date: Thu, 28 May 2020 14:59:11 -0700
Subject: [PATCH] Dup splat array in certain cases where there is a block
 argument

This makes:

```ruby
  args = [1, 2, -> {}]; foo(*args, &args.pop)
```

call `foo` with 1, 2, and the lambda, in addition to passing the
lambda as a block.  This is different from the previous behavior,
which passed the lambda as a block but not as a regular argument,
which goes against the expected left-to-right evaluation order.

This is how Ruby already compiled arguments if using leading
arguments, trailing arguments, or keywords in the same call.

This works by disabling the optimization that skipped duplicating
the array during the splat (splatarray instruction argument
switches from false to true).  In the above example, the splat
call duplicates the array.  I've tested and cases where a
local variable or symbol are used do not duplicate the array,
so I don't expect this to decrease the performance of most Ruby
programs.  However, programs such as:

```ruby
  foo(*args, &bar)
```

could see a decrease in performance, if `bar` is a method call
and not a local variable.

This is not a perfect solution, there are ways to get around
this:

```ruby
  args = Struct.new(:a).new([:x, :y])
  def args.to_a; a; end
  def args.to_proc; a.pop; ->{}; end
  foo(*args, &args)
  # calls foo with 1 argument (:x)
  # not 2 arguments (:x and :y)
```

A perfect solution would require completely disabling the
optimization.

Fixes [Bug #16504]
Fixes [Bug #16500]
---
 compile.c                       |  4 +++-
 spec/ruby/language/send_spec.rb | 36 ++++++++++++++++++++++++---------
 test/ruby/test_call.rb          |  9 +++++++++
 3 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/compile.c b/compile.c
index f522954fb8..6316985b3d 100644
--- a/compile.c
+++ b/compile.c
@@ -5172,12 +5172,12 @@ setup_args(rb_iseq_t *iseq, LINK_ANCHOR *const args, const NODE *argn,
 {
     VALUE ret;
     if (argn && nd_type(argn) == NODE_BLOCK_PASS) {
+        unsigned int dup_rest = 1;
         DECL_ANCHOR(arg_block);
         INIT_ANCHOR(arg_block);
         NO_CHECK(COMPILE(arg_block, "block", argn->nd_body));
 
         *flag |= VM_CALL_ARGS_BLOCKARG;
-        ret = setup_args_core(iseq, args, argn->nd_head, 0, flag, keywords);
 
         if (LIST_INSN_SIZE_ONE(arg_block)) {
             LINK_ELEMENT *elem = FIRST_ELEMENT(arg_block);
@@ -5186,8 +5186,10 @@ setup_args(rb_iseq_t *iseq, LINK_ANCHOR *const args, const NODE *argn,
                 if (iobj->insn_id == BIN(getblockparam)) {
                     iobj->insn_id = BIN(getblockparamproxy);
                 }
+                dup_rest = 0;
             }
         }
+        ret = setup_args_core(iseq, args, argn->nd_head, dup_rest, flag, keywords);
         ADD_SEQ(args, arg_block);
     }
     else {
diff --git a/spec/ruby/language/send_spec.rb b/spec/ruby/language/send_spec.rb
index c56d5e8c26..17381166dc 100644
--- a/spec/ruby/language/send_spec.rb
+++ b/spec/ruby/language/send_spec.rb
@@ -421,18 +421,36 @@ describe "Invoking a method" do
     specs.rest_len(0,*a,4,*5,6,7,*c,-1).should == 11
   end
 
-  it "expands the Array elements from the splat after executing the arguments and block if no other arguments follow the splat" do
-    def self.m(*args, &block)
-      [args, block]
+  ruby_version_is ""..."2.8" do
+    it "expands the Array elements from the splat after executing the arguments and block if no other arguments follow the splat" do
+      def self.m(*args, &block)
+        [args, block]
+      end
+
+      args = [1, nil]
+      m(*args, &args.pop).should == [[1], nil]
+
+      args = [1, nil]
+      order = []
+      m(*(order << :args; args), &(order << :block; args.pop)).should == [[1], nil]
+      order.should == [:args, :block]
     end
+  end
 
-    args = [1, nil]
-    m(*args, &args.pop).should == [[1], nil]
+  ruby_version_is "2.8" do
+    it "expands the Array elements from the splat before applying block argument operations" do
+      def self.m(*args, &block)
+        [args, block]
+      end
 
-    args = [1, nil]
-    order = []
-    m(*(order << :args; args), &(order << :block; args.pop)).should == [[1], nil]
-    order.should == [:args, :block]
+      args = [1, nil]
+      m(*args, &args.pop).should == [[1, nil], nil]
+
+      args = [1, nil]
+      order = []
+      m(*(order << :args; args), &(order << :block; args.pop)).should == [[1, nil], nil]
+      order.should == [:args, :block]
+    end
   end
 
   it "evaluates the splatted arguments before the block if there are other arguments after the splat" do
diff --git a/test/ruby/test_call.rb b/test/ruby/test_call.rb
index 2a1b671cac..67b3a936d4 100644
--- a/test/ruby/test_call.rb
+++ b/test/ruby/test_call.rb
@@ -99,4 +99,13 @@ class TestCall < Test::Unit::TestCase
     ary = [1, 2]
     assert_equal([0, 1, 2, 1], aaa(0, *ary, ary.shift), bug12860)
   end
+
+  def test_call_block_order
+    bug16504 = '[ruby-core:96769] [Bug# 16504]'
+    b = proc{}
+    ary = [1, 2, b]
+    assert_equal([1, 2, b], aaa(*ary, &ary.pop), bug16504)
+    ary = [1, 2, b]
+    assert_equal([0, 1, 2, b], aaa(0, *ary, &ary.pop), bug16504)
+  end
 end