Bug #16500
closedArgument is added to both splat and last &block argument
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.
Updated by mame (Yusuke Endoh) almost 5 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) almost 5 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) almost 5 years ago
https://github.com/ruby-grape/grape/blob/d58dc0ab7a0b51625217deedd8110d1030be7cf7/lib/grape/middleware/stack.rb#L80-L84 is probably responsible for the failure.
Updated by ioquatix (Samuel Williams) almost 5 years ago
- Status changed from Open to Rejected
- Assignee set to ioquatix (Samuel Williams)
Updated by ioquatix (Samuel Williams) almost 5 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) almost 5 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) almost 5 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.
- 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.
Updated by mame (Yusuke Endoh) almost 5 years ago
- Related to Bug #16504: `foo(*args, &args.pop)` should pass all elements of args added
Updated by sawa (Tsuyoshi Sawada) almost 5 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) almost 5 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) almost 5 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.
Updated by jeremyevans (Jeremy Evans) over 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.