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, and allows YJIT to leave rb_control_frame_t::block_code uninitialized for cfunc frames. [Feature #19777] Co-authored-by: Takashi Kokubun <[email protected]>
XrXr · Sep 8, 2023 · 0307919 · 0307919
1 parent 60ef156
commit 0307919
Show file tree

Hide file tree

Showing 9 changed files with 101 additions and 142 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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.c b/vm.c
@@ -3125,7 +3125,10 @@ rb_execution_context_update(rb_execution_context_t *ec)
             const VALUE *ep = cfp->ep;
             cfp->self = rb_gc_location(cfp->self);
             cfp->iseq = (rb_iseq_t *)rb_gc_location((VALUE)cfp->iseq);
-            cfp->block_code = (void *)rb_gc_location((VALUE)cfp->block_code);
+
+            if (!VM_FRAME_CFRAME_P(cfp)) {
+                cfp->block_code = (void *)rb_gc_location((VALUE)cfp->block_code);
+            }
 
             if (!VM_ENV_LOCAL_P(ep)) {
                 const VALUE *prev_ep = VM_ENV_PREV_EP(ep);
@@ -3174,7 +3177,10 @@ rb_execution_context_mark(const rb_execution_context_t *ec)
             if (VM_FRAME_TYPE(cfp) != VM_FRAME_MAGIC_DUMMY) {
                 rb_gc_mark_movable(cfp->self);
                 rb_gc_mark_movable((VALUE)cfp->iseq);
-                rb_gc_mark_movable((VALUE)cfp->block_code);
+
+                if (!VM_FRAME_CFRAME_P(cfp)) {
+                    rb_gc_mark_movable((VALUE)cfp->block_code);
+                }
 
                 if (!VM_ENV_LOCAL_P(ep)) {
                     const VALUE *prev_ep = VM_ENV_PREV_EP(ep);

diff --git 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
@@ -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
@@ -233,6 +233,9 @@ impl JITState {
     }
 }
 
+/// Used in debug builds for catching use of values that are uninitialized in release builds.
+const UNINIT_POISON: u64 = 0xcccccccc_cccccccc;
+
 use crate::codegen::JCCKinds::*;
 
 #[allow(non_camel_case_types, unused)]
@@ -5194,9 +5197,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
@@ -5232,7 +5232,7 @@ fn gen_push_frame(
     //    .iseq       = <iseq for iseq, 0 for cfunc>,
     //    .self       = recv,
     //    .ep         = <sp - 1>,
-    //    .block_code = 0,
+    //    .block_code = <unset for cfunc, 0 for iseq>,
     // };
     asm.comment("push callee control frame");
 
@@ -5249,7 +5249,15 @@ fn gen_push_frame(
     };
     asm.mov(cfp_opnd(RUBY_OFFSET_CFP_ISEQ), iseq);
     asm.mov(cfp_opnd(RUBY_OFFSET_CFP_SELF), frame.recv);
-    asm.mov(cfp_opnd(RUBY_OFFSET_CFP_BLOCK_CODE), 0.into());
+
+    // cfp->block_code can be left uninitialized, but only for C frames
+    if frame.frame_type & VM_FRAME_MAGIC_CFUNC == VM_FRAME_MAGIC_CFUNC {
+        if cfg!(debug_assertions) {
+            asm.mov(cfp_opnd(RUBY_OFFSET_CFP_BLOCK_CODE), UNINIT_POISON.into());
+        }
+    } else {
+        asm.mov(cfp_opnd(RUBY_OFFSET_CFP_BLOCK_CODE), 0.into());
+    }
 
     // Spill stack temps to let the callee use them (must be done before changing SP)
     asm.spill_temps();
@@ -5476,7 +5484,7 @@ fn gen_send_cfunc(
         recv,
         sp,
         pc: if cfg!(debug_assertions) {
-            Some(!0) // Poison value. Helps to fail fast.
+            Some(UNINIT_POISON)
         } else {
             None     // Leave PC uninitialized as cfuncs shouldn't read it
         },