ruby-changes:60427
From: Yusuke <ko1@a...>
Date: Mon, 16 Mar 2020 23:17:43 +0900 (JST)
Subject: [ruby-changes:60427] 47141797be (master): hash.c: Do not use the fast path (rb_yield_values) for lambda blocks
https://git.ruby-lang.org/ruby.git/commit/?id=47141797be From 47141797bed55eb10932c9a722a5132f50d4f3d8 Mon Sep 17 00:00:00 2001 From: Yusuke Endoh <mame@r...> Date: Mon, 16 Mar 2020 23:03:22 +0900 Subject: hash.c: Do not use the fast path (rb_yield_values) for lambda blocks As a semantics, Hash#each yields a 2-element array (pairs of keys and values). So, `{ a: 1 }.each(&->(k, v) { })` should raise an exception due to lambda's arity check. However, the optimization that avoids Array allocation by using rb_yield_values for blocks whose arity is more than 1 (introduced at b9d29603375d17c3d1d609d9662f50beaec61fa1 and some commits), seemed to overlook the lambda case, and wrongly allowed the code above to work. This change experimentally attempts to make it strict; now the code above raises an ArgumentError. This is an incompatible change; if the compatibility issue is bigger than our expectation, it may be reverted (until Ruby 3.0 release). [Bug #12706] diff --git a/NEWS.md b/NEWS.md index d760de7..3fabf7b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -121,6 +121,13 @@ Excluding feature bug fixes. https://github.com/ruby/ruby/blob/trunk/NEWS.md#L121 your plan to https://github.com/ruby/xmlrpc or https://github.com/ruby/net-telnet. +* EXPERIMENTAL: Hash#each consistently yields a 2-element array [[Bug #12706]] + + * Now `{ a: 1 }.each(&->(k, v) { })` raises an ArgumentError + due to lambda's arity check. + * This is experimental; if it brings a big incompatibility issue, + it may be reverted until 2.8/3.0 release. + ## Stdlib compatibility issues Excluding feature bug fixes. diff --git a/hash.c b/hash.c index 5bbb3da..864de0a 100644 --- a/hash.c +++ b/hash.c @@ -3105,7 +3105,7 @@ static VALUE https://github.com/ruby/ruby/blob/trunk/hash.c#L3105 rb_hash_each_pair(VALUE hash) { RETURN_SIZED_ENUMERATOR(hash, 0, 0, hash_enum_size); - if (rb_block_arity() > 1) + if (rb_block_pair_yield_optimizable()) rb_hash_foreach(hash, each_pair_i_fast, 0); else rb_hash_foreach(hash, each_pair_i, 0); @@ -4446,7 +4446,7 @@ rb_hash_any_p(int argc, VALUE *argv, VALUE hash) https://github.com/ruby/ruby/blob/trunk/hash.c#L4446 /* yields pairs, never false */ return Qtrue; } - if (rb_block_arity() > 1) + if (rb_block_pair_yield_optimizable()) rb_hash_foreach(hash, any_p_i_fast, (VALUE)args); else rb_hash_foreach(hash, any_p_i, (VALUE)args); @@ -5500,7 +5500,7 @@ env_each_pair(VALUE ehash) https://github.com/ruby/ruby/blob/trunk/hash.c#L5500 } FREE_ENVIRON(environ); - if (rb_block_arity() > 1) { + if (rb_block_pair_yield_optimizable()) { for (i=0; i<RARRAY_LEN(ary); i+=2) { rb_yield_values(2, RARRAY_AREF(ary, i), RARRAY_AREF(ary, i+1)); } diff --git a/internal/proc.h b/internal/proc.h index 8643929..35bc43a 100644 --- a/internal/proc.h +++ b/internal/proc.h @@ -17,6 +17,7 @@ struct rb_iseq_struct; /* in vm_core.h */ https://github.com/ruby/ruby/blob/trunk/internal/proc.h#L17 /* proc.c */ VALUE rb_proc_location(VALUE self); st_index_t rb_hash_proc(st_index_t hash, VALUE proc); +int rb_block_pair_yield_optimizable(void); int rb_block_arity(void); int rb_block_min_max_arity(int *max); VALUE rb_block_to_s(VALUE self, const struct rb_block *block, const char *additional_info); diff --git a/proc.c b/proc.c index 5ee65a0..909a259 100644 --- a/proc.c +++ b/proc.c @@ -1145,6 +1145,41 @@ block_setup(struct rb_block *block, VALUE block_handler) https://github.com/ruby/ruby/blob/trunk/proc.c#L1145 } int +rb_block_pair_yield_optimizable(void) +{ + int min, max; + const rb_execution_context_t *ec = GET_EC(); + rb_control_frame_t *cfp = ec->cfp; + VALUE block_handler = rb_vm_frame_block_handler(cfp); + struct rb_block block; + + if (block_handler == VM_BLOCK_HANDLER_NONE) { + rb_raise(rb_eArgError, "no block given"); + } + + block_setup(&block, block_handler); + min = rb_vm_block_min_max_arity(&block, &max); + + switch (vm_block_type(&block)) { + case block_handler_type_symbol: + return 0; + + case block_handler_type_proc: + { + VALUE procval = block_handler; + rb_proc_t *proc; + GetProcPtr(procval, proc); + if (proc->is_lambda) return 0; + if (min != max) return 0; + return min > 1; + } + + default: + return min > 1; + } +} + +int rb_block_arity(void) { int min, max; diff --git a/spec/ruby/core/hash/shared/each.rb b/spec/ruby/core/hash/shared/each.rb index d1f2e5f..5e88a35 100644 --- a/spec/ruby/core/hash/shared/each.rb +++ b/spec/ruby/core/hash/shared/each.rb @@ -21,19 +21,37 @@ describe :hash_each, shared: true do https://github.com/ruby/ruby/blob/trunk/spec/ruby/core/hash/shared/each.rb#L21 ary.sort.should == ["a", "b", "c"] end - it "yields 2 values and not an Array of 2 elements when given a callable of arity 2" do - obj = Object.new - def obj.foo(key, value) - ScratchPad << key << value + ruby_version_is ""..."2.8" do + it "yields 2 values and not an Array of 2 elements when given a callable of arity 2" do + obj = Object.new + def obj.foo(key, value) + ScratchPad << key << value + end + + ScratchPad.record([]) + { "a" => 1 }.send(@method, &obj.method(:foo)) + ScratchPad.recorded.should == ["a", 1] + + ScratchPad.record([]) + { "a" => 1 }.send(@method, &-> key, value { ScratchPad << key << value }) + ScratchPad.recorded.should == ["a", 1] end + end - ScratchPad.record([]) - { "a" => 1 }.send(@method, &obj.method(:foo)) - ScratchPad.recorded.should == ["a", 1] + ruby_version_is "2.8" do + it "yields an Array of 2 elements when given a callable of arity 2" do + obj = Object.new + def obj.foo(key, value) + end + + -> { + { "a" => 1 }.send(@method, &obj.method(:foo)) + }.should raise_error(ArgumentError) - ScratchPad.record([]) - { "a" => 1 }.send(@method, &-> key, value { ScratchPad << key << value }) - ScratchPad.recorded.should == ["a", 1] + -> { + { "a" => 1 }.send(@method, &-> key, value { }) + }.should raise_error(ArgumentError) + end end it "uses the same order as keys() and values()" do diff --git a/struct.c b/struct.c index 79131db..3042d3b 100644 --- a/struct.c +++ b/struct.c @@ -821,7 +821,7 @@ rb_struct_each_pair(VALUE s) https://github.com/ruby/ruby/blob/trunk/struct.c#L821 RETURN_SIZED_ENUMERATOR(s, 0, 0, struct_enum_size); members = rb_struct_members(s); - if (rb_block_arity() > 1) { + if (rb_block_pair_yield_optimizable()) { for (i=0; i<RSTRUCT_LEN(s); i++) { VALUE key = rb_ary_entry(members, i); VALUE value = RSTRUCT_GET(s, i); diff --git a/test/ruby/test_hash.rb b/test/ruby/test_hash.rb index 33d1383..cc13e3f 100644 --- a/test/ruby/test_hash.rb +++ b/test/ruby/test_hash.rb @@ -1841,4 +1841,10 @@ class TestHash < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_hash.rb#L1841 h[obj2] = true assert_equal true, h[obj] end + + def test_bug_12706 + assert_raise(ArgumentError) do + {a: 1}.each(&->(k, v) {}) + end + end end -- cgit v0.10.2 -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/