show warning for unused block

ko1 · ko1 · commit 9180e33ca3a5 · 2024-04-15T12:08:07.000+09:00
With verbopse mode (-w), the interpreter shows a warning if a block is passed to a method which does not use the given block. Warning on: * the invoked method is written in C * the invoked method is not `initialize` * not invoked with `super` * the first time on the call-site with the invoked method (`obj.foo{}` will be warned once if `foo` is same method) [Feature #15554] `Primitive.attr! :use_block` is introduced to declare that primitive functions (written in C) will use passed block. For minitest, test needs some tweak, so use minitest/minitest@ea9caaf for `test-bundled-gems`.
diff --git a/NEWS.md b/NEWS.md
@@ -111,7 +111,14 @@ See GitHub releases like [GitHub Releases of Logger](https://github.com/ruby/log
 
 ## JIT
 
+## Miscellaneous changes
+
+* Passing a block to a method which doesn't use the passed block will show
+  a warning on verbose mode (`-w`).
+  [[Feature #15554]]
+
 [Feature #13557]: https://bugs.ruby-lang.org/issues/13557
+[Feature #15554]: https://bugs.ruby-lang.org/issues/15554
 [Feature #16495]: https://bugs.ruby-lang.org/issues/16495
 [Feature #18290]: https://bugs.ruby-lang.org/issues/18290
 [Feature #18980]: https://bugs.ruby-lang.org/issues/18980
diff --git a/array.rb b/array.rb
@@ -43,6 +43,8 @@ class Array
   # Related: #each_index, #reverse_each.
   def each
     Primitive.attr! :inline_block
+    Primitive.attr! :use_block
+
     unless defined?(yield)
       return Primitive.cexpr! 'SIZED_ENUMERATOR(self, 0, 0, ary_enum_length)'
     end
diff --git a/compile.c b/compile.c
@@ -2098,6 +2098,7 @@ iseq_set_arguments(rb_iseq_t *iseq, LINK_ANCHOR *const optargs, const NODE *cons
         if (block_id) {
             body->param.block_start = arg_size++;
             body->param.flags.has_block = TRUE;
+            body->param.flags.use_block = 1;
         }
 
         iseq_calc_param_size(iseq);
@@ -5918,6 +5919,7 @@ defined_expr0(rb_iseq_t *iseq, LINK_ANCHOR *const ret,
         ADD_INSN(ret, line_node, putnil);
         ADD_INSN3(ret, line_node, defined, INT2FIX(DEFINED_YIELD), 0,
                   PUSH_VAL(DEFINED_YIELD));
+        ISEQ_BODY(ISEQ_BODY(iseq)->local_iseq)->param.flags.use_block = 1;
         return;
 
       case NODE_BACK_REF:
@@ -8628,6 +8630,9 @@ compile_builtin_attr(rb_iseq_t *iseq, const NODE *node)
         else if (strcmp(RSTRING_PTR(string), "inline_block") == 0) {
             ISEQ_BODY(iseq)->builtin_attrs |= BUILTIN_ATTR_INLINE_BLOCK;
         }
+        else if (strcmp(RSTRING_PTR(string), "use_block") == 0) {
+            ISEQ_BODY(iseq)->param.flags.use_block = 1;
+        }
         else {
             goto unknown_arg;
         }
@@ -9377,6 +9382,8 @@ compile_super(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node, i
     }
     else {
         /* NODE_ZSUPER */
+        ISEQ_BODY(ISEQ_BODY(iseq)->local_iseq)->param.flags.use_block = 1;
+
         int i;
         const rb_iseq_t *liseq = body->local_iseq;
         const struct rb_iseq_constant_body *const local_body = ISEQ_BODY(liseq);
@@ -9510,6 +9517,7 @@ compile_yield(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node, i
 
     ADD_SEQ(ret, args);
     ADD_INSN1(ret, node, invokeblock, new_callinfo(iseq, 0, FIX2INT(argc), flag, keywords, FALSE));
+    ISEQ_BODY(ISEQ_BODY(iseq)->local_iseq)->param.flags.use_block = 1;
 
     if (popped) {
         ADD_INSN(ret, node, pop);
@@ -12935,7 +12943,10 @@ ibf_dump_iseq_each(struct ibf_dump *dump, const rb_iseq_t *iseq)
         (body->param.flags.has_block        << 6) |
         (body->param.flags.ambiguous_param0 << 7) |
         (body->param.flags.accepts_no_kwarg << 8) |
-        (body->param.flags.ruby2_keywords   << 9);
+        (body->param.flags.ruby2_keywords   << 9) |
+        (body->param.flags.anon_rest        << 10) |
+        (body->param.flags.anon_kwrest      << 11) |
+        (body->param.flags.use_block        << 12);
 
 #if IBF_ISEQ_ENABLE_LOCAL_BUFFER
 #  define IBF_BODY_OFFSET(x) (x)
@@ -13151,6 +13162,7 @@ ibf_load_iseq_each(struct ibf_load *load, rb_iseq_t *iseq, ibf_offset_t offset)
     load_body->param.flags.ruby2_keywords = (param_flags >> 9) & 1;
     load_body->param.flags.anon_rest = (param_flags >> 10) & 1;
     load_body->param.flags.anon_kwrest = (param_flags >> 11) & 1;
+    load_body->param.flags.use_block = (param_flags >> 12) & 1;
     load_body->param.size = param_size;
     load_body->param.lead_num = param_lead_num;
     load_body->param.opt_num = param_opt_num;
diff --git a/dir.rb b/dir.rb
@@ -408,6 +408,7 @@ def self.[](*args, base: nil, sort: true)
   #   specifies that patterns may match short names if they exist; Windows only.
   #
   def self.glob(pattern, _flags = 0, flags: _flags, base: nil, sort: true)
+    Primitive.attr! :use_block
     Primitive.dir_s_glob(pattern, flags, base, sort)
   end
 end
diff --git a/gems/bundled_gems b/gems/bundled_gems
@@ -6,7 +6,7 @@
 # - revision: revision in repository-url to test
 #   if `revision` is not given, "v"+`version` or `version` will be used.
 
-minitest        5.22.3  https://github.com/minitest/minitest 287b35d60c8e19c11ba090efc6eeb225325a8520
+minitest        5.22.3  https://github.com/minitest/minitest ea9caafc0754b1d6236a490d59e624b53209734a
 power_assert    2.0.3   https://github.com/ruby/power_assert 84e85124c5014a139af39161d484156cfe87a9ed
 rake            13.2.1  https://github.com/ruby/rake
 test-unit       3.6.2   https://github.com/test-unit/test-unit
diff --git a/iseq.c b/iseq.c
@@ -538,6 +538,11 @@ iseq_location_setup(rb_iseq_t *iseq, VALUE name, VALUE path, VALUE realpath, int
     RB_OBJ_WRITE(iseq, &loc->label, name);
     RB_OBJ_WRITE(iseq, &loc->base_label, name);
     loc->first_lineno = first_lineno;
+
+    if (ISEQ_BODY(iseq)->local_iseq == iseq && strcmp(RSTRING_PTR(name), "initialize") == 0) {
+        ISEQ_BODY(iseq)->param.flags.use_block = 1;
+    }
+
     if (code_location) {
         loc->node_id = node_id;
         loc->code_location = *code_location;
@@ -1011,6 +1016,7 @@ pm_iseq_new_with_opt(pm_scope_node_t *node, VALUE name, VALUE path, VALUE realpa
 {
     rb_iseq_t *iseq = iseq_alloc();
     ISEQ_BODY(iseq)->prism = true;
+    ISEQ_BODY(iseq)->param.flags.use_block = true; // unused block warning is not supported yet
 
     if (!option) option = &COMPILE_OPTION_DEFAULT;
 
diff --git a/lib/optparse.rb b/lib/optparse.rb
@@ -461,7 +461,7 @@ def self.candidate(key, icase = false, pat = nil, &block)
       candidates
     end
 
-    def candidate(key, icase = false, pat = nil)
+    def candidate(key, icase = false, pat = nil, &_)
       Completion.candidate(key, icase, pat, &method(:each))
     end
 
@@ -739,7 +739,7 @@ class RequiredArgument < self
       #
       # Raises an exception if argument is not present.
       #
-      def parse(arg, argv)
+      def parse(arg, argv, &_)
         unless arg
           raise MissingArgument if argv.empty?
           arg = argv.shift
diff --git a/lib/rdoc/context.rb b/lib/rdoc/context.rb
@@ -710,7 +710,7 @@ def display(method_attr) # :nodoc:
   # This method exists to make it easy to work with Context subclasses that
   # aren't part of RDoc.
 
-  def each_ancestor # :nodoc:
+  def each_ancestor(&_) # :nodoc:
   end
 
   ##
diff --git a/pack.rb b/pack.rb
@@ -17,6 +17,7 @@ class String
   #  returns that array.
   #  See {Packed Data}[rdoc-ref:packed_data.rdoc].
   def unpack(fmt, offset: 0)
+    Primitive.attr! :use_block
     Primitive.pack_unpack(fmt, offset)
   end
 
diff --git a/rjit_c.rb b/rjit_c.rb
@@ -1093,6 +1093,7 @@ def C.rb_iseq_constant_body
           ruby2_keywords: [CType::BitField.new(1, 1), 9],
           anon_rest: [CType::BitField.new(1, 2), 10],
           anon_kwrest: [CType::BitField.new(1, 3), 11],
+          use_block: [CType::BitField.new(1, 4), 12],
         ), Primitive.cexpr!("OFFSETOF(((struct rb_iseq_constant_body *)NULL)->param, flags)")],
         size: [CType::Immediate.parse("unsigned int"), Primitive.cexpr!("OFFSETOF(((struct rb_iseq_constant_body *)NULL)->param, size)")],
         lead_num: [CType::Immediate.parse("int"), Primitive.cexpr!("OFFSETOF(((struct rb_iseq_constant_body *)NULL)->param, lead_num)")],
diff --git a/test/ruby/test_method.rb b/test/ruby/test_method.rb
@@ -1623,4 +1623,63 @@ def test_kwarg_eval_memory_leak
       end
     RUBY
   end
+
+  def test_warn_unused_block
+    assert_in_out_err '-w', <<-'RUBY' do |_out, err, _status|
+      def foo = nil
+      foo{}          # warn
+      send(:foo){}   # warn
+      b = Proc.new{}
+      foo(&b)        # warn
+    RUBY
+      assert_equal 3, err.size
+      err = err.join
+      assert_match(/-:2: warning/, err)
+      assert_match(/-:3: warning/, err)
+      assert_match(/-:5: warning/, err)
+    end
+
+    assert_in_out_err '-w', <<-'RUBY' do |_out, err, _status|
+      def foo = nil
+      10.times{foo{}} # warn once
+    RUBY
+      assert_equal 1, err.size
+    end
+
+    assert_in_out_err '-w', <<-'RUBY' do |_out, err, _status|
+      def foo = nil; b = nil
+      foo(&b)       # no warning
+      1.object_id{} # no warning because it is written in C
+
+      class C
+        def initialize
+        end
+      end
+      C.new{} # no warning
+
+    RUBY
+      assert_equal 0, err.size
+    end
+
+    assert_in_out_err '-w', <<-'RUBY' do |_out, err, _status|
+      class C0
+        def foo = nil
+        def bar = nil
+        def baz = nil
+      end
+
+      class C1 < C0
+        def foo = super
+        def bar = super()
+        def baz(&_) = super(&_)
+      end
+
+      C1.new.foo{} # no warning
+      C1.new.bar{} # warning
+      C1.new.baz{} # no warning
+    RUBY
+      assert_equal 1, err.size
+      assert_match(/-:14: warning.+bar/, err.join)
+    end
+  end
 end
diff --git a/test/ruby/test_optimization.rb b/test/ruby/test_optimization.rb
@@ -577,7 +577,7 @@ def test_string_freeze_block
     begin;
       class String
         undef freeze
-        def freeze
+        def freeze(&)
           block_given?
         end
       end
diff --git a/tool/mk_builtin_loader.rb b/tool/mk_builtin_loader.rb
@@ -6,7 +6,7 @@
 
 SUBLIBS = {}
 REQUIRED = {}
-BUILTIN_ATTRS = %w[leaf inline_block]
+BUILTIN_ATTRS = %w[leaf inline_block use_block]
 
 def string_literal(lit, str = [])
   while lit
diff --git a/tool/test/testunit/test_parallel.rb b/tool/test/testunit/test_parallel.rb
@@ -119,7 +119,7 @@ def test_done
 
         result = Marshal.load($1.chomp.unpack1("m"))
         assert_equal(5, result[0])
-        pend "TODO: result[1] returns 17. We should investigate it" do
+        pend "TODO: result[1] returns 17. We should investigate it" do # TODO: misusage of pend (pend doens't use given block)
           assert_equal(12, result[1])
         end
         assert_kind_of(Array,result[2])
diff --git a/trace_point.rb b/trace_point.rb
@@ -94,6 +94,7 @@ class TracePoint
   # Access from other threads is also forbidden.
   #
   def self.new(*events)
+    Primitive.attr! :use_block
     Primitive.tracepoint_new_s(events)
   end
 
@@ -131,6 +132,7 @@ def self.stat
   #	    trace.enabled? #=> true
   #
   def self.trace(*events)
+    Primitive.attr! :use_block
     Primitive.tracepoint_trace_s(events)
   end
 
@@ -196,6 +198,7 @@ def self.trace(*events)
   # out calls by itself from :line handler, otherwise it will call itself infinitely).
   #
   def self.allow_reentry
+    Primitive.attr! :use_block
     Primitive.tracepoint_allow_reentry
   end
 
@@ -258,6 +261,7 @@ def self.allow_reentry
   #    #=> RuntimeError: access from outside
   #
   def enable(target: nil, target_line: nil, target_thread: :default)
+    Primitive.attr! :use_block
     Primitive.tracepoint_enable_m(target, target_line, target_thread)
   end
 
@@ -294,6 +298,7 @@ def enable(target: nil, target_line: nil, target_thread: :default)
   #	trace.disable { p tp.lineno }
   #	#=> RuntimeError: access from outside
   def disable
+    Primitive.attr! :use_block
     Primitive.tracepoint_disable_m
   end
 
diff --git a/vm_backtrace.c b/vm_backtrace.c
@@ -201,8 +201,8 @@ location_lineno_m(VALUE self)
 
 VALUE rb_mod_name0(VALUE klass, bool *permanent);
 
-static VALUE
-gen_method_name(VALUE owner, VALUE name)
+VALUE
+rb_gen_method_name(VALUE owner, VALUE name)
 {
     bool permanent;
     if (RB_TYPE_P(owner, T_CLASS) || RB_TYPE_P(owner, T_MODULE)) {
@@ -235,7 +235,7 @@ calculate_iseq_label(VALUE owner, const rb_iseq_t *iseq)
       case ISEQ_TYPE_MAIN:
         return ISEQ_BODY(iseq)->location.label;
       case ISEQ_TYPE_METHOD:
-        return gen_method_name(owner, ISEQ_BODY(iseq)->location.label);
+        return rb_gen_method_name(owner, ISEQ_BODY(iseq)->location.label);
       case ISEQ_TYPE_BLOCK:
       case ISEQ_TYPE_PLAIN: {
         int level = 0;
@@ -269,7 +269,7 @@ static VALUE
 location_label(rb_backtrace_location_t *loc)
 {
     if (loc->cme && loc->cme->def->type == VM_METHOD_TYPE_CFUNC) {
-        return gen_method_name(loc->cme->owner, rb_id2str(loc->cme->def->original_id));
+        return rb_gen_method_name(loc->cme->owner, rb_id2str(loc->cme->def->original_id));
     }
     else {
         VALUE owner = Qnil;
@@ -457,7 +457,7 @@ location_to_str(rb_backtrace_location_t *loc)
             file = GET_VM()->progname;
             lineno = 0;
         }
-        name = gen_method_name(loc->cme->owner, rb_id2str(loc->cme->def->original_id));
+        name = rb_gen_method_name(loc->cme->owner, rb_id2str(loc->cme->def->original_id));
     }
     else {
         file = rb_iseq_path(loc->iseq);
diff --git a/vm_core.h b/vm_core.h
@@ -418,6 +418,7 @@ struct rb_iseq_constant_body {
             unsigned int ruby2_keywords: 1;
             unsigned int anon_rest: 1;
             unsigned int anon_kwrest: 1;
+            unsigned int use_block: 1;
         } flags;
 
         unsigned int size;
diff --git a/vm_insnhelper.c b/vm_insnhelper.c

Original file line number	Diff line number	Diff line change
`@@ -408,6 +408,7 @@ def self.[](*args, base: nil, sort: true)`
`408`	`408`	`# specifies that patterns may match short names if they exist; Windows only.`
`409`	`409`	`#`
`410`	`410`	`def self.glob(pattern, _flags = 0, flags: _flags, base: nil, sort: true)`
	`411`	`+ Primitive.attr! :use_block`
`411`	`412`	`Primitive.dir_s_glob(pattern, flags, base, sort)`
`412`	`413`	`end`
`413`	`414`	`end`