summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorYusuke Endoh <mame@ruby-lang.org>2020-03-16 23:03:22 +0900
committerYusuke Endoh <mame@ruby-lang.org>2020-03-16 23:17:12 +0900
commit47141797bed55eb10932c9a722a5132f50d4f3d8 (patch)
treea2934da7ecc862d7746eaf0f504aea16cc35b653
parent4be2a891cce920d2e2c2ece572c66e5aabe98eaa (diff)
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]
-rw-r--r--NEWS.md7
-rw-r--r--hash.c6
-rw-r--r--internal/proc.h1
-rw-r--r--proc.c35
-rw-r--r--spec/ruby/core/hash/shared/each.rb38
-rw-r--r--struct.c2
-rw-r--r--test/ruby/test_hash.rb6
7 files changed, 81 insertions, 14 deletions
diff --git a/NEWS.md b/NEWS.md
index d760de7c11..3fabf7b331 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -121,6 +121,13 @@ Excluding feature bug fixes.
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 5bbb3da5f2..864de0a480 100644
--- a/hash.c
+++ b/hash.c
@@ -3105,7 +3105,7 @@ static VALUE
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)
/* 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)
}
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 864392906b..35bc43a4ee 100644
--- a/internal/proc.h
+++ b/internal/proc.h
@@ -17,6 +17,7 @@ struct rb_iseq_struct; /* in vm_core.h */
/* 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 5ee65a0fee..909a25994b 100644
--- a/proc.c
+++ b/proc.c
@@ -1145,6 +1145,41 @@ block_setup(struct rb_block *block, VALUE block_handler)
}
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 d1f2e5f672..5e88a35445 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
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 79131db2bd..3042d3bed5 100644
--- a/struct.c
+++ b/struct.c
@@ -821,7 +821,7 @@ rb_struct_each_pair(VALUE s)
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 33d1383ed7..cc13e3f6a5 100644
--- a/test/ruby/test_hash.rb
+++ b/test/ruby/test_hash.rb
@@ -1841,4 +1841,10 @@ class TestHash < Test::Unit::TestCase
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