Bug #16500: Argument is added to both splat and last &block argument

• Status: Closed
• Assignee: ioquatix (Samuel Williams)
• Target version: -
• ruby -v: 2.7.0
• Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN

[anatolik (Anatol Pomozov)]

Here is a followup for a ruby2.7 issue discussed here https://gitlab.com/groups/gitlab-org/-/epics/2380

I run gitlab with ruby2.7. gitlab/lib/api/api_guard.rb calls Rack's use method:

use Rack::OAuth2::Server::Resource::Bearer, 'The API' do |request|
  # The authenticator only fetches the raw token string

  # Must yield access token to store it in the env
  request.access_token
end

The use method looks like:

def use(middleware, *args, &block)
  if @map
    mapping, @map = @map, nil
      @use << proc { |app| generate_map app, mapping }
  end
  @use << proc { |app| middleware.new(app, *args, &block) }
end

For some reason, a Proc object was set to &block and added to args. It sounds wrong. A Proc should only be set to &block, and args should contain only one argument.

[mame (Yusuke Endoh)]

Thank you for the report! I cannot reproduce the issue by a simple config.ru:

require "rack/oauth2"

use Rack::OAuth2::Server::Resource::Bearer, 'The API' do |request|
  request.access_token
end

Adding p args into the definition of Rack's use shows only ["The API"]. So I'd like to try it with the source code of gitlab. I have no idea at all about gitlab, so could you tell me how to reproduce it?

[ioquatix (Samuel Williams)]

We cannot reproduce this.

Can you make some script to reproduce this in isolation?

Including Gemfile and Gemfile.lock details.

[ioquatix (Samuel Williams)]

https://github.com/ruby-grape/grape/blob/d58dc0ab7a0b51625217deedd8110d1030be7cf7/lib/grape/middleware/stack.rb#L80-L84 is probably responsible for the failure.

[ioquatix (Samuel Williams)]

• Status changed from Open to Rejected
• Assignee set to ioquatix (Samuel Williams)

https://github.com/ruby-grape/grape/issues/1967

[ioquatix (Samuel Williams)]

• Status changed from Rejected to Open

On Ruby 2.7.0:

irb(main):020:-> x = [1, 2, ->{}]; puts(*x, &x.pop)
1
2
=> nil
irb(main):021:-> x = [1, 2, ->{}]; puts(*x, &x.last)
1
2
#<Proc:0x0000562763a56398 (irb):21 (lambda)>
=> nil

This seems like buggy behaviour related to the original issue.

[jeremyevans0 (Jeremy Evans)]

ioquatix (Samuel Williams) wrote:

On Ruby 2.7.0:

irb(main):020:-> x = [1, 2, ->{}]; puts(*x, &x.pop)
1
2
=> nil
irb(main):021:-> x = [1, 2, ->{}]; puts(*x, &x.last)
1
2
#<Proc:0x0000562763a56398 (irb):21 (lambda)>
=> nil

This seems like buggy behaviour related to the original issue.

I'm not sure if that behavior is buggy. puts ignores a passed block, so the behavior seems expected.

def a(*args, &b)
  p [args, b]
end
x = [1, 2, ->{}]; a(*x, &x.pop)
# => [[1, 2], #<Proc:0x00001100c1362518 (irb):4 (lambda)>]
x = [1, 2, ->{}]; a(*x, &x.last)
# => [[1, 2, #<Proc:0x000011004f2b0ba0 (irb):5 (lambda)>], #<Proc:0x000011004f2b0ba0 (irb):5 (lambda)>]

Same results with Ruby 1.9.

[mame (Yusuke Endoh)]

Ruby 2.7 partially changed the behavior. Coped from my comment: https://github.com/ruby-grape/grape/issues/1967#issuecomment-573366122

# in 2.6 or before
args = [1, 2, -> {}]; foo(   *args, &args.pop) #=> passes [1, 2] (bug; [1, 2, ->{}] is expected)
args = [1, 2, -> {}]; foo(0, *args, &args.pop) #=> passes [0, 1, 2] (bug; [0, 1, 2, ->{}] is expected)

# in 2.7
args = [1, 2, -> {}]; foo(   *args, &args.pop) #=> passes [1, 2] (bug; [1, 2, ->{}] is expected)
args = [1, 2, -> {}]; foo(0, *args, &args.pop) #=> passes [0, 1, 2, ->{}] (good)

So, there are two issues.

• The issue that this ticket says is caused by a behavior change of 2.7, and ruby-grape's change will fix the issue https://github.com/ruby-grape/grape/commit/dec3e1ff5dbf3215a714565e62b12bd2ef6b0ddb
• The behavior of args = [1, 2, -> {}]; foo( *args, &args.pop) should pass [1, 2, ->{}], and we should fix the bug on the ruby interpreter.

[mame (Yusuke Endoh)]

• Related to Bug #16504: `foo(*args, &args.pop)` should pass all elements of args added

[sawa (Tsuyoshi Sawada)]

• Subject changed from Argument added both to splat and last &block argument to Argument is added to both splat and last &block argument
• Description updated (diff)

[Eregon (Benoit Daloze)]

mame (Yusuke Endoh) wrote:

• The behavior of args = [1, 2, -> {}]; foo( *args, &args.pop) should pass [1, 2, ->{}], and we should fix the bug on the ruby interpreter.

Why is that better than the previous behavior?
Changing that will be more incompatible.

[Eregon (Benoit Daloze)]

FWIW the 2.6 behavior is detailed in ruby/spec:
https://github.com/ruby/spec/blob/84d606aa8e85a8fef6521b3402dae612d04288c4/language/send_spec.rb#L424-L445
From commit
https://github.com/ruby/spec/commit/01992ab93dd893d9e8bf79db9f5ff7d250952097
which shows RubyGems used to rely on the 2.6 behavior.

What I want to say is existing code relies on 2.6 behavior.
It's probably good to always evaluate positional arguments before the block argument for consistency, but it's an incompatible change.

• Status changed from Open to Closed

Applied in changeset git|aae8223c7076483f1f1641181088790b2f3a66dd.

---

Dup splat array in certain cases where there is a block argument

This makes:

  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:

  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:

  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]