https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112020-09-28T02:30:59ZRuby Issue Tracking SystemRuby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=877652020-09-28T02:30:59Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p>Also, as noted by <a class="user active user-mention" href="https://bugs.ruby-lang.org/users/43766">@boblail (Bob Lail)</a>, <code>Hash#map</code> accepts both arity 1 and 2; shouldn't it accept only arity 1?</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=877672020-09-28T09:13:45ZEregon (Benoit Daloze)
<ul></ul><p>marcandre (Marc-Andre Lafortune) wrote in <a href="#note-1">#note-1</a>:</p>
<blockquote>
<p>Also, as noted by <a class="user active user-mention" href="https://bugs.ruby-lang.org/users/43766">@boblail (Bob Lail)</a>, <code>Hash#map</code> accepts both arity 1 and 2; shouldn't it accept only arity 1?</p>
</blockquote>
<p>Agreed, especially since Hash#each changed to always yield a single argument.</p>
<p>I think it was just the same implementation as for Hash#each that would check the given Proc's arity.</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=877682020-09-28T09:28:29ZEregon (Benoit Daloze)
<ul></ul><p><code>#map</code> is defined on Enumerable.</p>
<p>So this seems an unintended interaction between<br>
Hash#each using <code>rb_block_pair_yield_optimizable()</code>:<br>
<a href="https://github.com/ruby/ruby/blob/245ed57ddc93901e90388cf479968392299d1067/hash.c#L3158-L3159" class="external">https://github.com/ruby/ruby/blob/245ed57ddc93901e90388cf479968392299d1067/hash.c#L3158-L3159</a><br>
and Enumerable#map using <code>rb_block_min_max_arity</code> and <code>rb_yield_values2</code>.<br>
<a href="https://github.com/ruby/ruby/blob/245ed57ddc93901e90388cf479968392299d1067/enum.c#L547-L587" class="external">https://github.com/ruby/ruby/blob/245ed57ddc93901e90388cf479968392299d1067/enum.c#L547-L587</a></p>
<p>So as result that optimization seems to still not be transparent and changes semantics.</p>
<p><code>rb_block_pair_yield_optimizable()</code> returns false for lambda though, and <code>#map</code> uses <code>rb_lambda_call()</code> so not sure what goes wrong there.</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=881392020-10-23T21:08:58Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p><a class="user active user-mention" href="https://bugs.ruby-lang.org/users/4">@nobu (Nobuyoshi Nakada)</a> would it be possible to address this before preview-2?</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=885112020-11-16T05:18:16Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/12706">Bug #12706</a>: Hash#each yields inconsistent number of args</i> added</li></ul> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=885142020-11-16T07:59:22Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>This story is rather different from <code>Hash#each</code> in <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Enumerable & Hash yielding arity (Closed)" href="https://bugs.ruby-lang.org/issues/14015">#14015</a>.</p>
<p><code>Hash#each</code> has yielded an array that has a key and a value since a long time ago:</p>
<pre><code># 2.7
{ a: 1 }.each {|a| p a } #=> [:a, 1]
</code></pre>
<p>BTW, <code>Hash#each</code> 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. <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Enumerable & Hash yielding arity (Closed)" href="https://bugs.ruby-lang.org/issues/14015">#14015</a> pointed out this issue, and we fixed it:</p>
<pre><code># 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)
</code></pre>
<p>This is incompatible, but it changes only the case where a lambda block is passed to <code>Hash#each</code> (which is relatively rare, I think), so I (hopefully) guess it is acceptable, and we have no bug report about this since preview-1.</p>
<p>On the other hand, <code>Hash#select</code> has yielded two elements since Ruby 1.9.0:</p>
<pre><code># 2.7
{ a: 1 }.select {|k| p k } #=> :a
# this proposal
{ a: 1 }.select {|k| p k } #=> [:a, 1]
</code></pre>
<p>IMO, this is a bug since 1.9.0 because <code>Hash#select</code> looks like a faster version of <code>Enumerable#select</code>, so it should behave as possible as like <code>Enumerable#select</code>. However, the behavior has been longly accepted anyway, and passing a plain (non-lambda) block to <code>Hash#select</code> is the main use case, so fixing this may have a bigger impact than <code>Hash#each</code>.</p>
<p>In short: I like the proposal, but the imcompatibility would be much bigger than <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Enumerable & Hash yielding arity (Closed)" href="https://bugs.ruby-lang.org/issues/14015">#14015</a>.</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=885222020-11-16T14:04:42ZDan0042 (Daniel DeLorme)
<ul></ul><p>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?</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=885282020-11-16T22:37:03ZEregon (Benoit Daloze)
<ul></ul><p><a class="user active user-mention" href="https://bugs.ruby-lang.org/users/11019">@Dan0042 (Daniel DeLorme)</a> How would we warn that case? Check if the block uses arity 1 and warn that it should instead use <code>|k,v|</code> or <code>|k,|</code>?</p>
<p>I think we should fix <code>Hash#map</code> at least, that's also a clear optimization bug like <code>Hash#each</code>, and it forces other Ruby implementations to replicate the bug.<br>
(e.g., <a href="https://github.com/oracle/truffleruby/issues/1944" class="external">https://github.com/oracle/truffleruby/issues/1944</a>)</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=885512020-11-17T13:37:03ZDan0042 (Daniel DeLorme)
<ul></ul><p>Eregon (Benoit Daloze) wrote in <a href="#note-8">#note-8</a>:</p>
<blockquote>
<p>Check if the block uses arity 1 and warn that it should instead use <code>|k,v|</code> or <code>|k,|</code>?</p>
</blockquote>
<p>Yes. It seems obvious, am I missing something? I'm aware that <code>proc{ |k,| }.arity == 1</code> (imho a bug) but internally it's possible to tell the difference between <code>|k|</code> and <code>|k,|</code> otherwise they would have the same behavior.</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=885522020-11-17T14:24:19Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>Ah, I was wrong.</p>
<p>mame (Yusuke Endoh) wrote in <a href="#note-6">#note-6</a>:</p>
<blockquote>
<p>IMO, this is a bug since 1.9.0 because <code>Hash#select</code> looks like a faster version of <code>Enumerable#select</code>, so it should behave as possible as like <code>Enumerable#select</code>.</p>
</blockquote>
<p>Hash#select returns a hash instead of an array, so <code>Hash#select</code> is not a simple faster variant of <code>Enumerable#select</code>. So it don't necessarily have to follow the arity.</p>
<p>Just for the record: I found one affected use case of <code>Hash#select</code> with one-arity block in <a href="https://rubygems.org/gems/docker-api" class="external">a popular gem</a>:</p>
<p><a href="https://github.com/swipely/docker-api/blob/1e9b9cc5f0f38dcd54c18189812328bb802d3656/lib/docker/container.rb#L327" class="external">https://github.com/swipely/docker-api/blob/1e9b9cc5f0f38dcd54c18189812328bb802d3656/lib/docker/container.rb#L327</a></p>
<pre><code> def self.create(opts = {}, conn = Docker.connection)
query = opts.select {|key| ['name', :name].include?(key) }
</code></pre>
<p>It is very difficult to find this kind of cases by gem-codesearch, so I guess there are more cases.</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=886242020-11-20T07:34:55Zmatz (Yukihiro Matsumoto)matz@ruby.or.jp
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Rejected</i></li></ul><p>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?</p>
<p>Matz.</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=886292020-11-20T08:25:03Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p>matz (Yukihiro Matsumoto) wrote in <a href="#note-11">#note-11</a>:</p>
<blockquote>
<p>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?</p>
<p>Matz.</p>
</blockquote>
<p>Thanks for the reply. Should we add a deprecation warning then?</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=887672020-11-26T12:33:02Zmatz (Yukihiro Matsumoto)matz@ruby.or.jp
<ul></ul><p><a class="user active user-mention" href="https://bugs.ruby-lang.org/users/182">@marcandre (Marc-Andre Lafortune)</a> 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.</p>
<p>Matz.</p> Ruby master - Bug #17197: Some Hash methods still have arity 2 instead of 1https://bugs.ruby-lang.org/issues/17197?journal_id=888522020-11-30T19:22:20ZDan0042 (Daniel DeLorme)
<ul></ul><p>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, <code>hash.select{|x|...}</code> is likely to have a surprising result, and that's worth a warning.</p>