Project

General

Profile

Actions

Bug #16500

closed

Argument is added to both splat and last &block argument

Added by anatolik (Anatol Pomozov) about 4 years ago. Updated almost 4 years ago.

Status:
Closed
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 1 (0 open1 closed)

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

Updated by mame (Yusuke Endoh) about 4 years 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) about 4 years 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) about 4 years ago

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

Updated by ioquatix (Samuel Williams) about 4 years 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) about 4 years 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) about 4 years 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.

Actions #8

Updated by mame (Yusuke Endoh) about 4 years ago

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

Updated by sawa (Tsuyoshi Sawada) about 4 years ago

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

Updated by Eregon (Benoit Daloze) about 4 years 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) about 4 years 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.

Actions #12

Updated by jeremyevans (Jeremy Evans) almost 4 years 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]

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0