show warning for unused block

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
ea9caafc07
for `test-bundled-gems`.

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

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

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;

dir.rb

@@ -408,6 +408,7 @@ class Dir
   #   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

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

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;
 

lib/optparse.rb

@@ -461,7 +461,7 @@ class OptionParser
       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 OptionParser
       #
       # 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

lib/rdoc/context.rb

@@ -710,7 +710,7 @@ class RDoc::Context < RDoc::CodeObject
   # 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
 
   ##

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
 

rjit_c.rb

@@ -1093,6 +1093,7 @@ module RubyVM::RJIT # :nodoc: all
           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)")],

test/ruby/test_method.rb

@@ -1623,4 +1623,63 @@ class TestMethod < Test::Unit::TestCase
       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

test/ruby/test_optimization.rb

@@ -577,7 +577,7 @@ class TestRubyOptimization < Test::Unit::TestCase
     begin;
       class String
         undef freeze
-        def freeze
+        def freeze(&)
           block_given?
         end
       end

tool/mk_builtin_loader.rb

@@ -6,7 +6,7 @@ require_relative 'ruby_vm/helpers/c_escape'
 
 SUBLIBS = {}
 REQUIRED = {}
-BUILTIN_ATTRS = %w[leaf inline_block]
+BUILTIN_ATTRS = %w[leaf inline_block use_block]
 
 def string_literal(lit, str = [])
   while lit

tool/test/testunit/test_parallel.rb

@@ -119,7 +119,7 @@ module TestParallel
 
         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])

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 @@ class TracePoint
   #	    trace.enabled? #=> true
   #
   def self.trace(*events)
+    Primitive.attr! :use_block
     Primitive.tracepoint_trace_s(events)
   end
 
@@ -196,6 +198,7 @@ class TracePoint
   # 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 @@ class TracePoint
   #    #=> 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 @@ class TracePoint
   #	trace.disable { p tp.lineno }
   #	#=> RuntimeError: access from outside
   def disable
+    Primitive.attr! :use_block
     Primitive.tracepoint_disable_m
   end
 

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 @@ retry:
       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);

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;

vm_insnhelper.c

@@ -2966,6 +2966,57 @@ vm_call_single_noarg_leaf_builtin(rb_execution_context_t *ec, rb_control_frame_t
     return builtin_invoker0(ec, calling->recv, NULL, (rb_insn_func_t)bf->func_ptr);
 }
 
+VALUE rb_gen_method_name(VALUE owner, VALUE name); // in vm_backtrace.c
+
+static void
+warn_unused_block(const rb_callable_method_entry_t *cme, const rb_iseq_t *iseq, void *pc)
+{
+    static st_table *dup_check_table = NULL;
+
+    st_data_t key = 0;
+    union {
+        VALUE v;
+        unsigned char b[SIZEOF_VALUE];
+    } k1 = {
+        .v = (VALUE)pc,
+    }, k2 = {
+        .v = (VALUE)cme->def,
+    };
+
+    // make unique key from pc and me->def pointer
+    for (int i=0; i<SIZEOF_VALUE; i++) {
+        // fprintf(stderr, "k1:%3d k2:%3d\n", k1.b[i], k2.b[SIZEOF_VALUE-1-i]);
+        key |= (st_data_t)(k1.b[i] ^ k2.b[SIZEOF_VALUE-1-i]) << (8 * i);
+    }
+
+    if (0) {
+        fprintf(stderr, "SIZEOF_VALUE:%d\n", SIZEOF_VALUE);
+        fprintf(stderr, "pc:%p def:%p\n", pc, cme->def);
+        fprintf(stderr, "key:%p\n", (void *)key);
+    }
+
+    if (!dup_check_table) {
+        dup_check_table = st_init_numtable();
+    }
+
+    // duplication check
+    if (st_insert(dup_check_table, key, 1)) {
+        // already shown
+    }
+    else {
+        VALUE m_loc = rb_method_entry_location((const rb_method_entry_t *)cme);
+        VALUE name = rb_gen_method_name(cme->defined_class, ISEQ_BODY(iseq)->location.base_label);
+
+        if (!NIL_P(m_loc)) {
+            rb_warning("the passed block for '%"PRIsVALUE"' defined at %"PRIsVALUE":%"PRIsVALUE" may be ignored",
+                       name, RARRAY_AREF(m_loc, 0), RARRAY_AREF(m_loc, 1));
+        }
+        else {
+            rb_warning("the block may be ignored because '%"PRIsVALUE"' does not use a block", name);
+        }
+    }
+}
+
 static inline int
 vm_callee_setup_arg(rb_execution_context_t *ec, struct rb_calling_info *calling,
                     const rb_iseq_t *iseq, VALUE *argv, int param_size, int local_size)
@@ -2974,6 +3025,12 @@ vm_callee_setup_arg(rb_execution_context_t *ec, struct rb_calling_info *calling,
     const struct rb_callcache *cc = calling->cc;
     bool cacheable_ci = vm_ci_markable(ci);
 
+    if (UNLIKELY(!ISEQ_BODY(iseq)->param.flags.use_block &&
+                 calling->block_handler != VM_BLOCK_HANDLER_NONE &&
+                 !(vm_ci_flag(calling->cd->ci) & VM_CALL_SUPER))) {
+        warn_unused_block(vm_cc_cme(cc), iseq, (void *)ec->cfp->pc);
+    }
+
     if (LIKELY(!(vm_ci_flag(ci) & VM_CALL_KW_SPLAT))) {
         if (LIKELY(rb_simple_iseq_p(iseq))) {
             rb_control_frame_t *cfp = ec->cfp;