Project

General

Profile

Bug #16500

Argument is added to both splat and last &block argument

Added by anatolik (Anatol Pomozov) 9 months ago. Updated 3 months ago.

Status:
Closed
Priority:
Normal
Target version:
-
[ruby-core:96769]

Description

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.


Related issues

Related to Ruby master - Bug #16504: `foo(*args, &args.pop)` should pass all elements of argsClosedjeremyevans0 (Jeremy Evans)Actions

Updated by mame (Yusuke Endoh) 9 months ago

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?

Updated by ioquatix (Samuel Williams) 9 months ago

We cannot reproduce this.

Can you make some script to reproduce this in isolation?

Including Gemfile and Gemfile.lock details.

Updated by ioquatix (Samuel Williams) 9 months ago

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

Updated by ioquatix (Samuel Williams) 9 months ago

  • 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.

Updated by jeremyevans0 (Jeremy Evans) 9 months ago

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.

Updated by mame (Yusuke Endoh) 9 months ago

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.

#8

Updated by mame (Yusuke Endoh) 9 months ago

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

Updated by sawa (Tsuyoshi Sawada) 9 months ago

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

Updated by Eregon (Benoit Daloze) 9 months ago

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.

Updated by Eregon (Benoit Daloze) 9 months ago

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.

#12

Updated by jeremyevans (Jeremy Evans) 3 months ago

  • 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]

Also available in: Atom PDF