From 39ee3e22bd3d071c1c283b6b8dbd1af413342fb1 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Thu, 7 Sep 2023 14:56:07 -0400 Subject: [PATCH] Make Kernel#lambda raise when given non-literal block Previously, Kernel#lambda returned a non-lambda proc when given a non-literal block and issued a warning under the `:deprecated` category. With this change, Kernel#lambda will always return a lambda proc, if it returns without raising. Due to interactions with block passing optimizations, we previously had two separate code paths for detecting whether Kernel#lambda got a literal block. This change allows us to remove one path, the hack done with rb_control_frame_t::block_code introduced in 85a337f for supporting situations where Kernel#lambda returned a non-lambda proc. [Feature #19777] Co-authored-by: Takashi Kokubun --- proc.c | 49 +++++++++---------- spec/ruby/core/kernel/lambda_spec.rb | 72 ++++++++++++++++------------ spec/ruby/core/proc/lambda_spec.rb | 8 ++-- test/ruby/test_lambda.rb | 26 ---------- test/ruby/test_proc.rb | 47 ++++-------------- vm_args.c | 5 +- vm_core.h | 6 --- yjit/src/codegen.rs | 3 -- 8 files changed, 79 insertions(+), 137 deletions(-) diff --git a/proc.c b/proc.c index 9fd24a8fd4e561..0f4735b589b8b0 100644 --- a/proc.c +++ b/proc.c @@ -793,16 +793,8 @@ proc_new(VALUE klass, int8_t is_lambda) break; case block_handler_type_ifunc: - 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); - } + return rb_vm_make_proc_lambda(ec, VM_BH_TO_CAPT_BLOCK(block_handler), klass, is_lambda); } VM_UNREACHABLE(proc_new); return Qnil; @@ -857,31 +849,34 @@ rb_block_lambda(void) } static void -f_lambda_warn(void) +f_lambda_filter_non_literal(void) { rb_control_frame_t *cfp = GET_EC()->cfp; VALUE block_handler = rb_vm_frame_block_handler(cfp); - if (block_handler != VM_BLOCK_HANDLER_NONE) { - switch (vm_block_handler_type(block_handler)) { - case block_handler_type_iseq: - if (RUBY_VM_PREVIOUS_CONTROL_FRAME(cfp)->ep == VM_BH_TO_ISEQ_BLOCK(block_handler)->ep) { - return; - } - break; - case block_handler_type_symbol: + if (block_handler == VM_BLOCK_HANDLER_NONE) { + // no block erorr raised else where + return; + } + + switch (vm_block_handler_type(block_handler)) { + case block_handler_type_iseq: + if (RUBY_VM_PREVIOUS_CONTROL_FRAME(cfp)->ep == VM_BH_TO_ISEQ_BLOCK(block_handler)->ep) { return; - case block_handler_type_proc: - if (rb_proc_lambda_p(VM_BH_TO_PROC(block_handler))) { - return; - } - break; - case block_handler_type_ifunc: - break; } + break; + case block_handler_type_symbol: + return; + case block_handler_type_proc: + if (rb_proc_lambda_p(VM_BH_TO_PROC(block_handler))) { + return; + } + break; + case block_handler_type_ifunc: + break; } - rb_warn_deprecated("lambda without a literal block", "the proc without lambda"); + rb_raise(rb_eArgError, "the lambda method requires a literal block"); } /* @@ -895,7 +890,7 @@ f_lambda_warn(void) static VALUE f_lambda(VALUE _) { - f_lambda_warn(); + f_lambda_filter_non_literal(); return rb_block_lambda(); } diff --git a/spec/ruby/core/kernel/lambda_spec.rb b/spec/ruby/core/kernel/lambda_spec.rb index 2f7abc749cb739..565536ac0d6f5e 100644 --- a/spec/ruby/core/kernel/lambda_spec.rb +++ b/spec/ruby/core/kernel/lambda_spec.rb @@ -26,42 +26,44 @@ l.lambda?.should be_true end - it "creates a lambda-style Proc if given a literal block via Kernel.public_send" do - suppress_warning do - l = Kernel.public_send(:lambda) { 42 } - l.lambda?.should be_true + ruby_version_is ""..."3.3" do + it "creates a lambda-style Proc if given a literal block via Kernel.public_send" do + suppress_warning do + l = Kernel.public_send(:lambda) { 42 } + l.lambda?.should be_true + end end - end - it "returns the passed Proc if given an existing Proc" do - some_proc = proc {} - l = suppress_warning {lambda(&some_proc)} - l.should equal(some_proc) - l.lambda?.should be_false - end + it "returns the passed Proc if given an existing Proc" do + some_proc = proc {} + l = suppress_warning {lambda(&some_proc)} + l.should equal(some_proc) + l.lambda?.should be_false + end - it "creates a lambda-style Proc when called with zsuper" do - suppress_warning do - l = KernelSpecs::LambdaSpecs::ForwardBlockWithZSuper.new.lambda { 42 } - l.lambda?.should be_true - l.call.should == 42 + it "creates a lambda-style Proc when called with zsuper" do + suppress_warning do + l = KernelSpecs::LambdaSpecs::ForwardBlockWithZSuper.new.lambda { 42 } + l.lambda?.should be_true + l.call.should == 42 - lambda { l.call(:extra) }.should raise_error(ArgumentError) + lambda { l.call(:extra) }.should raise_error(ArgumentError) + end end - end - it "returns the passed Proc if given an existing Proc through super" do - some_proc = proc { } - l = KernelSpecs::LambdaSpecs::SuperAmpersand.new.lambda(&some_proc) - l.should equal(some_proc) - l.lambda?.should be_false - end + it "returns the passed Proc if given an existing Proc through super" do + some_proc = proc { } + l = KernelSpecs::LambdaSpecs::SuperAmpersand.new.lambda(&some_proc) + l.should equal(some_proc) + l.lambda?.should be_false + end - it "does not create lambda-style Procs when captured with #method" do - kernel_lambda = method(:lambda) - l = suppress_warning {kernel_lambda.call { 42 }} - l.lambda?.should be_false - l.call(:extra).should == 42 + it "does not create lambda-style Procs when captured with #method" do + kernel_lambda = method(:lambda) + l = suppress_warning {kernel_lambda.call { 42 }} + l.lambda?.should be_false + l.call(:extra).should == 42 + end end it "checks the arity of the call when no args are specified" do @@ -137,8 +139,16 @@ def ret end context "when called without a literal block" do - it "warns when proc isn't a lambda" do - -> { lambda(&proc{}) }.should complain("#{__FILE__}:#{__LINE__}: warning: lambda without a literal block is deprecated; use the proc without lambda instead\n") + ruby_version_is ""..."3.3" do + it "warns when proc isn't a lambda" do + -> { lambda(&proc{}) }.should complain("#{__FILE__}:#{__LINE__}: warning: lambda without a literal block is deprecated; use the proc without lambda instead\n") + end + end + + ruby_version_is "3.3" do + it "raises when proc isn't a lambda" do + -> { lambda(&proc{}) }.should raise_error(ArgumentError, /the lambda method requires a literal block/) + end end it "doesn't warn when proc is lambda" do diff --git a/spec/ruby/core/proc/lambda_spec.rb b/spec/ruby/core/proc/lambda_spec.rb index b2d3f503502463..5c3c38fc2a64c1 100644 --- a/spec/ruby/core/proc/lambda_spec.rb +++ b/spec/ruby/core/proc/lambda_spec.rb @@ -14,9 +14,11 @@ Proc.new {}.lambda?.should be_false end - it "is preserved when passing a Proc with & to the lambda keyword" do - suppress_warning {lambda(&->{})}.lambda?.should be_true - suppress_warning {lambda(&proc{})}.lambda?.should be_false + ruby_version_is ""..."3.3" do + it "is preserved when passing a Proc with & to the lambda keyword" do + suppress_warning {lambda(&->{})}.lambda?.should be_true + suppress_warning {lambda(&proc{})}.lambda?.should be_false + end end it "is preserved when passing a Proc with & to the proc keyword" do diff --git a/test/ruby/test_lambda.rb b/test/ruby/test_lambda.rb index 9949fab8c76498..7738034240497d 100644 --- a/test/ruby/test_lambda.rb +++ b/test/ruby/test_lambda.rb @@ -177,32 +177,6 @@ def test_proc_inside_lambda_toplevel RUBY 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 - prev_warning, Warning[:deprecated] = Warning[:deprecated], false - f = pass_along {} - refute_predicate(f, :lambda?, '[Bug #15620]') - assert_nothing_raised(ArgumentError) { f.call(:extra_arg) } - ensure - Warning[:deprecated] = prev_warning - end - - def test_create_non_lambda_for_proc_two_levels - prev_warning, Warning[:deprecated] = Warning[:deprecated], false - f = pass_along2 {} - refute_predicate(f, :lambda?, '[Bug #15620]') - assert_nothing_raised(ArgumentError) { f.call(:extra_arg) } - ensure - Warning[:deprecated] = prev_warning - end - def test_instance_exec bug12568 = '[ruby-core:76300] [Bug #12568]' assert_nothing_raised(ArgumentError, bug12568) do diff --git a/test/ruby/test_proc.rb b/test/ruby/test_proc.rb index 2b3590d4d0627d..4245a7b7857c17 100644 --- a/test/ruby/test_proc.rb +++ b/test/ruby/test_proc.rb @@ -289,7 +289,6 @@ def test_lambda? assert_equal(false, l.lambda?) assert_equal(false, l.curry.lambda?, '[ruby-core:24127]') assert_equal(false, proc(&l).lambda?) - assert_equal(false, assert_deprecated_warning {lambda(&l)}.lambda?) assert_equal(false, Proc.new(&l).lambda?) l = lambda {} assert_equal(true, l.lambda?) @@ -299,47 +298,21 @@ def test_lambda? assert_equal(true, Proc.new(&l).lambda?) end - def self.helper_test_warn_lamda_with_passed_block &b + def helper_test_warn_lambda_with_passed_block &b lambda(&b) end - def self.def_lambda_warning name, warn - define_method(name, proc do - prev = Warning[:deprecated] - assert_warn warn do - Warning[:deprecated] = true - yield - end - ensure - Warning[:deprecated] = prev - end) - end - - def_lambda_warning 'test_lambda_warning_normal', '' do - lambda{} - end - - def_lambda_warning 'test_lambda_warning_pass_lambda', '' do - b = lambda{} - lambda(&b) - end - - def_lambda_warning 'test_lambda_warning_pass_symbol_proc', '' do - lambda(&:to_s) - end - - def_lambda_warning 'test_lambda_warning_pass_proc', /deprecated/ do - b = proc{} - lambda(&b) - end - - def_lambda_warning 'test_lambda_warning_pass_block', /deprecated/ do - helper_test_warn_lamda_with_passed_block{} + def test_lambda_warning_pass_proc + assert_raise(ArgumentError) do + b = proc{} + lambda(&b) + end end - def_lambda_warning 'test_lambda_warning_pass_block_symbol_proc', '' do - # Symbol#to_proc returns lambda - helper_test_warn_lamda_with_passed_block(&:to_s) + def test_lambda_warning_pass_block + assert_raise(ArgumentError) do + helper_test_warn_lambda_with_passed_block{} + end end def test_curry_ski_fib diff --git a/vm_args.c b/vm_args.c index a02e08d7fc2433..882966e4328361 100644 --- a/vm_args.c +++ b/vm_args.c @@ -885,10 +885,7 @@ 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) { - 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; + return VM_CF_BLOCK_HANDLER(reg_cfp); } 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 25629d3789491a..f3e5a04f72d568 100644 --- a/vm_core.h +++ b/vm_core.h @@ -1550,12 +1550,6 @@ 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) { diff --git a/yjit/src/codegen.rs b/yjit/src/codegen.rs index 69b1c52c9ca739..3ae519bc812a24 100644 --- a/yjit/src/codegen.rs +++ b/yjit/src/codegen.rs @@ -5194,9 +5194,6 @@ fn gen_push_frame( let block_handler = asm.load( Opnd::mem(64, ep_opnd, SIZEOF_VALUE_I32 * VM_ENV_DATA_INDEX_SPECVAL) ); - - asm.store(Opnd::mem(64, CFP, RUBY_OFFSET_CFP_BLOCK_CODE), block_handler); - block_handler } BlockHandler::AlreadySet => 0.into(), // unused