Bug #16504
closed`foo(*args, &args.pop)` should pass all elements of args
Description
https://bugs.ruby-lang.org/issues/16500?next_issue_id=16499&prev_issue_id=16501#note-7
def foo(*args)
p args
end
# 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)
Updated by mame (Yusuke Endoh) almost 5 years ago
Updated by mame (Yusuke Endoh) almost 5 years ago
- Related to Bug #16500: Argument is added to both splat and last &block argument added
Updated by Eregon (Benoit Daloze) almost 5 years ago
What's the definition of foo
here?
I believe the previous behavior is the block expression gets evaluated before the rest of the arguments.
We should decide and clarify if the block or positional arguments are evaluated first.
If I see foo(*args, &args.pop)
I would expect no duplicated arguments.
I believe that's the previous and more intuitive behavior.
I'd be happy if we can warn it though, because that code is really unreadable.
foo(*args, &args.last)
should be used if wanting to pass the last argument both as block and last positional.
Updated by mame (Yusuke Endoh) almost 5 years ago
Okay, I'll ask matz which is right. But I believe that the 2.6 and current behavior is wrong because Ruby has a principle of left-to-right evaluation. Actually, foo(*ary, ary.pop)
was changed between 2.1 and 2.2; 2.2 and later duplicate the last argument. And what do you think about foo(*ary, 42, &ary.pop)
?
Updated by mame (Yusuke Endoh) almost 5 years ago
- Related to Bug #12860: Splatting an argument does not obey left-to-right execution order added
Updated by mame (Yusuke Endoh) almost 5 years ago
I spent an hour to find the ticket: https://bugs.ruby-lang.org/issues/12860
Updated by Eregon (Benoit Daloze) almost 5 years ago
Agreed left-to-right would be far more consistent.
We just need to be aware that whoever writes foo(*args, &args.pop)
probably expects no duplication (so they would disagree on "bug" probably).
But such code deserves to be clearer IMHO.
Updated by marcandre (Marc-Andre Lafortune) almost 5 years ago
mame (Yusuke Endoh) wrote:
Okay, I'll ask matz which is right. But I believe that the 2.6 and current behavior is wrong because Ruby has a principle of left-to-right evaluation.
For sure current behavior is wrong and left-to-right evaluation must be observed.
Updated by jeremyevans0 (Jeremy Evans) over 4 years ago
I've added a pull request that fixes this issue: https://github.com/ruby/ruby/pull/3157
It's not a perfect fix, but a perfect fix would require disabling a commonly used optimization, resulting in additional array allocations in common cases when using splatting with block passing.
Updated by Eregon (Benoit Daloze) over 4 years ago
Do we have an idea of how much overhead would it be to disable this not-correct optimization?
Arrays are copy-on-write so it seems not so expensive to have an extra array, and also guarantees the callee cannot mutate the original array.
I'd rather we don't compromise semantics because optimizations are too limited.
For the &:symbol
case, we could check if Symbol#to_proc is redefined.
For the &local_var
case, we could check if it's already a Proc.
Updated by mame (Yusuke Endoh) over 4 years ago
jeremyevans0 (Jeremy Evans) wrote in #note-9:
I've added a pull request that fixes this issue: https://github.com/ruby/ruby/pull/3157
Updated by jeremyevans0 (Jeremy Evans) over 4 years ago
mame (Yusuke Endoh) wrote in #note-12:
jeremyevans0 (Jeremy Evans) wrote in #note-9:
I've added a pull request that fixes this issue: https://github.com/ruby/ruby/pull/3157
Sorry, I completely missed that you also had a fix for this (basically the same other than the test added).
Updated by matz (Yukihiro Matsumoto) over 4 years ago
Accepted. We can ignore incompatibility here.
Matz.
Updated by mame (Yusuke Endoh) over 4 years ago
- Assignee set to jeremyevans0 (Jeremy Evans)
jeremyevans0 (Jeremy Evans) wrote in #note-13:
mame (Yusuke Endoh) wrote in #note-12:
jeremyevans0 (Jeremy Evans) wrote in #note-9:
I've added a pull request that fixes this issue: https://github.com/ruby/ruby/pull/3157
Sorry, I completely missed that you also had a fix for this (basically the same other than the test added).
Don't worry. It is good to know that two persons reached the same solution independently. Your patch is better because it includes tests, so could you commit your patch?
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.