From 9180e33ca3a5886fec3f9e0a2f48072b55914e65 Mon Sep 17 00:00:00 2001 From: Koichi Sasada Date: Wed, 27 Mar 2024 07:29:38 +0900 Subject: [PATCH] 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 https://github.com/minitest/minitest/commit/ea9caafc0754b1d6236a490d59e624b53209734a for `test-bundled-gems`. --- NEWS.md | 7 ++++ array.rb | 2 + compile.c | 14 ++++++- dir.rb | 1 + gems/bundled_gems | 2 +- iseq.c | 6 +++ lib/optparse.rb | 4 +- lib/rdoc/context.rb | 2 +- pack.rb | 1 + rjit_c.rb | 1 + test/ruby/test_method.rb | 59 +++++++++++++++++++++++++++++ test/ruby/test_optimization.rb | 2 +- tool/mk_builtin_loader.rb | 2 +- tool/test/testunit/test_parallel.rb | 2 +- trace_point.rb | 5 +++ vm_backtrace.c | 10 ++--- vm_core.h | 1 + vm_insnhelper.c | 57 ++++++++++++++++++++++++++++ 18 files changed, 165 insertions(+), 13 deletions(-) diff --git a/NEWS.md b/NEWS.md index bdb4fa1eb1..ff08c4e566 100644 --- 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 index 8e809b35c9..f63ff00056 100644 --- 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 index 3fb1566ad4..2a7bdc2c99 100644 --- 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 index 632c49eee9..42b475ab2c 100644 --- a/dir.rb +++ b/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 diff --git a/gems/bundled_gems b/gems/bundled_gems index 842fbe0ce3..ad979c1c5c 100644 --- 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 index 3659dab2f5..4d4006777e 100644 --- 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 index 76ff38aca3..1dec3fa124 100644 --- a/lib/optparse.rb +++ b/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 diff --git a/lib/rdoc/context.rb b/lib/rdoc/context.rb index c6edfb473c..c688d562c3 100644 --- a/lib/rdoc/context.rb +++ b/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 ## diff --git a/pack.rb b/pack.rb index d505eaee35..b6e29c3eab 100644 --- 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 index 0ba33b40bf..b9a5bb0b55 100644 --- a/rjit_c.rb +++ b/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)")], diff --git a/test/ruby/test_method.rb b/test/ruby/test_method.rb index ec06f4c50a..67b3e03e10 100644 --- a/test/ruby/test_method.rb +++ b/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 diff --git a/test/ruby/test_optimization.rb b/test/ruby/test_optimization.rb index 70b6bde6ed..44ec615405 100644 --- a/test/ruby/test_optimization.rb +++ b/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 diff --git a/tool/mk_builtin_loader.rb b/tool/mk_builtin_loader.rb index 07c8291249..4abd497f0e 100644 --- a/tool/mk_builtin_loader.rb +++ b/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 diff --git a/tool/test/testunit/test_parallel.rb b/tool/test/testunit/test_parallel.rb index f79c3a1d80..6882fd6c5f 100644 --- a/tool/test/testunit/test_parallel.rb +++ b/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]) diff --git a/trace_point.rb b/trace_point.rb index e61b4b6802..b136e9b399 100644 --- 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 @@ 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 diff --git a/vm_backtrace.c b/vm_backtrace.c index 6f4379ad23..3fe816930d 100644 --- 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 @@ 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); diff --git a/vm_core.h b/vm_core.h index 2a9d5f906f..57d90b343f 100644 --- 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 index fe038e8618..68a74a3e33 100644 --- a/vm_insnhelper.c +++ b/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; idef); + 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;