From 85a337f986fe6da99c7f8358f790f17b122b3903 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Sat, 13 Jul 2019 12:04:01 -0400 Subject: [PATCH] Kernel#lambda: return forwarded block as non-lambda proc Before this commit, Kernel#lambda can't tell the difference between a directly passed literal block and one passed with an ampersand. A block passed with an ampersand is semantically speaking already a non-lambda proc. When Kernel#lambda receives a non-lambda proc, it should simply return it. Implementation wise, when the VM calls a method with a literal block, it places the code for the block on the calling control frame and passes a pointer (block handler) to the callee. Before this commit, the VM forwards block arguments by simply forwarding the block handler, which leaves the slot for block code unused when a control frame forwards its block argument. I use the vacant space to indicate that a frame has forwarded its block argument and inspect that in Kernel#lambda to detect forwarded blocks. This is a very ad-hoc solution and relies *heavily* on the way block passing works in the VM. However, it's the most self-contained solution I have. [Bug #15620] --- proc.c | 10 +++++++++- test/ruby/test_lambda.rb | 20 ++++++++++++++++++++ vm_args.c | 5 ++++- vm_core.h | 8 +++++++- 4 files changed, 40 insertions(+), 3 deletions(-) diff --git a/proc.c b/proc.c index 54b044f421..b4899d2f1f 100644 --- a/proc.c +++ b/proc.c @@ -792,8 +792,16 @@ proc_new(VALUE klass, int8_t is_lambda, int8_t kernel) break; case block_handler_type_ifunc: - case block_handler_type_iseq: return rb_vm_make_proc_lambda(ec, VM_BH_TO_CAPT_BLOCK(block_handler), klass, is_lambda); + case block_handler_type_iseq: + { + const struct rb_captured_block *captured = VM_BH_TO_CAPT_BLOCK(block_handler); + rb_control_frame_t *last_ruby_cfp = rb_vm_get_ruby_level_next_cfp(ec, cfp); + if (is_lambda && last_ruby_cfp && vm_cfp_forwarded_bh_p(last_ruby_cfp, block_handler)) { + is_lambda = false; + } + return rb_vm_make_proc_lambda(ec, captured, klass, is_lambda); + } } VM_UNREACHABLE(proc_new); return Qnil; diff --git a/test/ruby/test_lambda.rb b/test/ruby/test_lambda.rb index b9412d4540..03b501a6c9 100644 --- a/test/ruby/test_lambda.rb +++ b/test/ruby/test_lambda.rb @@ -74,6 +74,26 @@ class TestLambdaParameters < Test::Unit::TestCase assert_raise(ArgumentError, bug9605) {proc(&plus).call [1,2]} end + def pass_along(&block) + lambda(&block) + end + + def pass_along2(&block) + pass_along(&block) + end + + def test_create_non_lambda_for_proc_one_level + f = pass_along {} + refute_predicate(f, :lambda?, '[Bug #15620]') + assert_nothing_raised(ArgumentError) { f.call(:extra_arg) } + end + + def test_create_non_lambda_for_proc_two_levels + f = pass_along2 {} + refute_predicate(f, :lambda?, '[Bug #15620]') + assert_nothing_raised(ArgumentError) { f.call(:extra_arg) } + end + def test_instance_exec bug12568 = '[ruby-core:76300] [Bug #12568]' assert_nothing_raised(ArgumentError, bug12568) do diff --git a/vm_args.c b/vm_args.c index 47ab8ee66a..444abf9086 100644 --- a/vm_args.c +++ b/vm_args.c @@ -1204,7 +1204,10 @@ vm_caller_setup_arg_block(const rb_execution_context_t *ec, rb_control_frame_t * return VM_BLOCK_HANDLER_NONE; } else if (block_code == rb_block_param_proxy) { - return VM_CF_BLOCK_HANDLER(reg_cfp); + VM_ASSERT(!VM_CFP_IN_HEAP_P(GET_EC(), reg_cfp)); + VALUE handler = VM_CF_BLOCK_HANDLER(reg_cfp); + reg_cfp->block_code = (const void *) handler; + return handler; } else if (SYMBOL_P(block_code) && rb_method_basic_definition_p(rb_cSymbol, idTo_proc)) { const rb_cref_t *cref = vm_env_cref(reg_cfp->ep); diff --git a/vm_core.h b/vm_core.h index cd7bf74b5d..12c3ac3775 100644 --- a/vm_core.h +++ b/vm_core.h @@ -763,7 +763,7 @@ typedef struct rb_control_frame_struct { const rb_iseq_t *iseq; /* cfp[2] */ VALUE self; /* cfp[3] / block[0] */ const VALUE *ep; /* cfp[4] / block[1] */ - const void *block_code; /* cfp[5] / block[2] */ /* iseq or ifunc */ + const void *block_code; /* cfp[5] / block[2] */ /* iseq or ifunc or forwarded block handler */ VALUE *__bp__; /* cfp[6] */ /* outside vm_push_frame, use vm_base_ptr instead. */ #if VM_DEBUG_BP_CHECK @@ -1494,6 +1494,12 @@ vm_block_handler_verify(MAYBE_UNUSED(VALUE block_handler)) (vm_block_handler_type(block_handler), 1)); } +static inline int +vm_cfp_forwarded_bh_p(const rb_control_frame_t *cfp, VALUE block_handler) +{ + return ((VALUE) cfp->block_code) == block_handler; +} + static inline enum rb_block_type vm_block_type(const struct rb_block *block) {