Bug #17197: Some Hash methods still have arity 2 instead of 1

• Status: Rejected
• Assignee: nobu (Nobuyoshi Nakada)
• Target version: -
• ruby -v: ruby 3.0.0dev (2020-09-26T17:38:39Z master 950614b088)
• Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN

[marcandre (Marc-Andre Lafortune)]

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.

[marcandre (Marc-Andre Lafortune)]

Also, as noted by @boblail (Bob Lail), Hash#map accepts both arity 1 and 2; shouldn't it accept only arity 1?

[Eregon (Benoit Daloze)]

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.

[Eregon (Benoit Daloze)]

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

[marcandre (Marc-Andre Lafortune)]

@nobu (Nobuyoshi Nakada) would it be possible to address this before preview-2?

[nobu (Nobuyoshi Nakada)]

• Related to Bug #12706: Hash#each yields inconsistent number of args added

[mame (Yusuke Endoh)]

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.

[Dan0042 (Daniel DeLorme)]

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?

[Eregon (Benoit Daloze)]

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

[Dan0042 (Daniel DeLorme)]

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.

[mame (Yusuke Endoh)]

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 of Enumerable#select, so it should behave as possible as like Enumerable#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:

https://github.com/swipely/docker-api/blob/1e9b9cc5f0f38dcd54c18189812328bb802d3656/lib/docker/container.rb#L327

  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.

[matz (Yukihiro Matsumoto)]

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

[marcandre (Marc-Andre Lafortune)]

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?

[matz (Yukihiro Matsumoto)]

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

[Dan0042 (Daniel DeLorme)]

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.