Bug #17197
closedSome Hash methods still have arity 2 instead of 1
Description
Hash#each
was changed recently to have arity of 1.
All other methods of Hash
should behave the same.
Much has been fixed since #14015, but some remains:
# Some methods consistently have arity 2:
{a: 1}.select( &->(_kvp) {} ) # => ArgumentError (wrong number of arguments (given 2, expected 1))
All in all: %i[select keep_if delete_if reject to_h]
have their arity still set at 2.
Updated by marcandre (Marc-Andre Lafortune) almost 4 years ago
Also, as noted by @boblail (Bob Lail), Hash#map
accepts both arity 1 and 2; shouldn't it accept only arity 1?
Updated by Eregon (Benoit Daloze) almost 4 years ago
marcandre (Marc-Andre Lafortune) wrote in #note-1:
Also, as noted by @boblail (Bob Lail),
Hash#map
accepts both arity 1 and 2; shouldn't it accept only arity 1?
Agreed, especially since Hash#each changed to always yield a single argument.
I think it was just the same implementation as for Hash#each that would check the given Proc's arity.
Updated by Eregon (Benoit Daloze) almost 4 years ago
#map
is defined on Enumerable.
So this seems an unintended interaction between
Hash#each using rb_block_pair_yield_optimizable()
:
https://github.com/ruby/ruby/blob/245ed57ddc93901e90388cf479968392299d1067/hash.c#L3158-L3159
and Enumerable#map using rb_block_min_max_arity
and rb_yield_values2
.
https://github.com/ruby/ruby/blob/245ed57ddc93901e90388cf479968392299d1067/enum.c#L547-L587
So as result that optimization seems to still not be transparent and changes semantics.
rb_block_pair_yield_optimizable()
returns false for lambda though, and #map
uses rb_lambda_call()
so not sure what goes wrong there.
Updated by marcandre (Marc-Andre Lafortune) almost 4 years ago
@nobu (Nobuyoshi Nakada) would it be possible to address this before preview-2?
Updated by nobu (Nobuyoshi Nakada) almost 4 years ago
- Related to Bug #12706: Hash#each yields inconsistent number of args added
Updated by mame (Yusuke Endoh) almost 4 years ago
This story is rather different from Hash#each
in #14015.
Hash#each
has yielded an array that has a key and a value since a long time ago:
# 2.7
{ a: 1 }.each {|a| p a } #=> [:a, 1]
BTW, Hash#each
has an optimization to skip array generation if the arity of a given block is 2. However, this optimization had overlooked the case where the block is lambda. This is a bug of optimization. #14015 pointed out this issue, and we fixed it:
# 2.7
{ a: 1 }.each(&->(k, v){ p k }) #=> :a # the bug that #14015 pointed out
# the current master
{ a: 1 }.each(&->(k, v){ p k }) #=> wrong number of arguments (given 1, expected 2)
This is incompatible, but it changes only the case where a lambda block is passed to Hash#each
(which is relatively rare, I think), so I (hopefully) guess it is acceptable, and we have no bug report about this since preview-1.
On the other hand, Hash#select
has yielded two elements since Ruby 1.9.0:
# 2.7
{ a: 1 }.select {|k| p k } #=> :a
# this proposal
{ a: 1 }.select {|k| p k } #=> [:a, 1]
IMO, this is a bug since 1.9.0 because Hash#select
looks like a faster version of Enumerable#select
, so it should behave as possible as like Enumerable#select
. However, the behavior has been longly accepted anyway, and passing a plain (non-lambda) block to Hash#select
is the main use case, so fixing this may have a bigger impact than Hash#each
.
In short: I like the proposal, but the imcompatibility would be much bigger than #14015.
Updated by Dan0042 (Daniel DeLorme) almost 4 years ago
So when you have a bug that is troublesome to fix because it may result in large incompatibility... rather than "just fix it" and wait to see if other people's code explodes, wouldn't the Industry-Standard Best Practiceâ„¢ here be to just add a deprecation warning?
Updated by Eregon (Benoit Daloze) over 3 years ago
@Dan0042 (Daniel DeLorme) How would we warn that case? Check if the block uses arity 1 and warn that it should instead use |k,v|
or |k,|
?
I think we should fix Hash#map
at least, that's also a clear optimization bug like Hash#each
, and it forces other Ruby implementations to replicate the bug.
(e.g., https://github.com/oracle/truffleruby/issues/1944)
Updated by Dan0042 (Daniel DeLorme) over 3 years ago
Eregon (Benoit Daloze) wrote in #note-8:
Check if the block uses arity 1 and warn that it should instead use
|k,v|
or|k,|
?
Yes. It seems obvious, am I missing something? I'm aware that proc{ |k,| }.arity == 1
(imho a bug) but internally it's possible to tell the difference between |k|
and |k,|
otherwise they would have the same behavior.
Updated by mame (Yusuke Endoh) over 3 years ago
Ah, I was wrong.
mame (Yusuke Endoh) wrote in #note-6:
IMO, this is a bug since 1.9.0 because
Hash#select
looks like a faster version ofEnumerable#select
, so it should behave as possible as likeEnumerable#select
.
Hash#select returns a hash instead of an array, so Hash#select
is not a simple faster variant of Enumerable#select
. So it don't necessarily have to follow the arity.
Just for the record: I found one affected use case of Hash#select
with one-arity block in a popular gem:
def self.create(opts = {}, conn = Docker.connection)
query = opts.select {|key| ['name', :name].include?(key) }
It is very difficult to find this kind of cases by gem-codesearch, so I guess there are more cases.
Updated by matz (Yukihiro Matsumoto) over 3 years ago
- Status changed from Open to Rejected
It is caused by a historical reason. I don't think it's worth breaking compatibility, at least for Ruby3.0. Maybe for 3.1?
Matz.
Updated by marcandre (Marc-Andre Lafortune) over 3 years ago
matz (Yukihiro Matsumoto) wrote in #note-11:
It is caused by a historical reason. I don't think it's worth breaking compatibility, at least for Ruby3.0. Maybe for 3.1?
Matz.
Thanks for the reply. Should we add a deprecation warning then?
Updated by matz (Yukihiro Matsumoto) over 3 years ago
@marcandre (Marc-Andre Lafortune) No. Issuing deprecation warnings itself is a declaration of future change. I haven't set my mind for either direction. Actually slightly leaning toward no change.
Matz.
Updated by Dan0042 (Daniel DeLorme) over 3 years ago
It doesn't have to be a deprecation warning though. But I think that in the same sense as the "method redefined" warning indicates a potential problem, hash.select{|x|...}
is likely to have a surprising result, and that's worth a warning.