- ruby-core - ml.ruby-lang.org

[ruby-core:117663] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by Dan0042 (Daniel DeLorme) 23 Apr '24

23 Apr '24

Issue #15554 has been updated by Dan0042 (Daniel DeLorme). byroot (Jean Boussier) wrote in #note-53: > @ko1 here's the result (I ran it on the 5 most important sub components) I think it's worth mentioning there's 67 warnings in that output but "only" 25 uniques. Acceptable for a codebase as big as Rails? In order to understand the false positives I had a look at the "add_bind" warnings. And at first I thought the ignored block was actually a bug! It took me half an hour of looking through the code to finally understand what it did and why it was ok to ignore the block. So while it's a false positive, this mirrors my experience in #note-45 that making the arguments more explicit would really help understanding of the code. my 2¢ ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-108071 * Author: ko1 (Koichi Sasada) * Status: Closed * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117655] [Ruby master Feature#6648] Provide a standard API for retrieving all command-line flags passed to Ruby
by kddnewton (Kevin Newton) 23 Apr '24

23 Apr '24

Issue #6648 has been updated by kddnewton (Kevin Newton). As another note, this would be useful within CRuby itself. Right now there are lots of tests that run `assert_in_out_err`, which in turn calls `EnvUtil.invoke_ruby`. `EnvUtil.invoke_ruby` does not pass along some command-line options like RJIT, YJIT, Prism, etc. So there appear to be some tests that are being run in the CRuby CI that aren't testing what they should be testing. ---------------------------------------- Feature #6648: Provide a standard API for retrieving all command-line flags passed to Ruby https://bugs.ruby-lang.org/issues/6648#change-108065 * Author: headius (Charles Nutter) * Status: Assigned * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- Currently there are no standard mechanisms to get the flags passed to the currently running Ruby implementation. The available mechanisms are not ideal: * Scanning globals and hoping they have not been tweaked to new settings * Using external wrappers to launch Ruby * ??? Inability to get the full set of command-line flags, including flags passed to the VM itself (and probably VM-specific) makes it impossible to launch subprocess Ruby instances with the same settings. A real world example of this is "((%bundle exec%))" when called with a command line that sets various flags, a la ((%jruby -Xsome.vm.setting --1.9 -S bundle exec%)). None of these flags can propagate to the subprocess, so odd behaviors result. The only option is to put the flags into an env var (((|JRUBY_OPTS|)) or ((|RUBYOPT|))) but this breaks the flow of calling a simple command line. JRuby provides mechanisms to get all its command line options, but they require calling Java APIs from Ruby's API set. Rubinius provides its own API for accessing comand-line options, but I do not know if it includes VM-level flags as well as standard Ruby flags. I know there is a (({RubyVM})) namespace in the 2.0 line. If that namespace is intended to be general-purpose for VM-level features, it would be a good host for this API. Something like... ``` class << RubyVM def vm_args; end # returns array of command line args *not* passed to the target script def script; end # returns the script being executed...though this overlaps with $0 def script_args; end # returns args passed to the script...though this overlaps with ARGV, but that is perhaps warranted since ARGV can be modified (i.e. you probably want the original args) end ``` -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117651] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by byroot (Jean Boussier) 23 Apr '24

23 Apr '24

Issue #15554 has been updated by byroot (Jean Boussier). > could you compare the relax results on that? or all of your results are false positive? Yes. It's all false positive. ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-108061 * Author: ko1 (Koichi Sasada) * Status: Closed * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117648] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by ko1 (Koichi Sasada) 23 Apr '24

23 Apr '24

Issue #15554 has been updated by ko1 (Koichi Sasada). Thank you, could you compare the relax results on that? or all of your results are false positive? ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-108058 * Author: ko1 (Koichi Sasada) * Status: Closed * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117643] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by byroot (Jean Boussier) 22 Apr '24

22 Apr '24

Issue #15554 has been updated by byroot (Jean Boussier). @ko1 here's the result (I ran it on the 5 most important sub components) ``` ### Active Record activemodel/lib/active_model/attribute.rb:63: warning: the block passed to 'ActiveModel::Type::Value#serializable?' defined at activemodel/lib/active_model/type/value.rb:28 may be ignored activerecord/lib/arel/collectors/composite.rb:34: warning: the block passed to 'Arel::Collectors::Bind#add_binds' defined at activerecord/lib/arel/collectors/bind.rb:21 may be ignored activerecord/lib/arel/visitors/to_sql.rb:352: warning: the block passed to 'Arel::Collectors::SubstituteBinds#add_binds' defined at activerecord/lib/arel/collectors/substitute_binds.rb:23 may be ignored activerecord/lib/active_record/associations/association_scope.rb:148: warning: the block passed to 'ActiveRecord::Associations::AssociationScope::ReflectionProxy#all_includes' defined at activerecord/lib/active_record/associations/association_scope.rb:109 may be ignored activemodel/lib/active_model/attribute_set/builder.rb:43: warning: the block passed to 'ActiveModel::Attribute#value' defined at activemodel/lib/active_model/attribute.rb:41 may be ignored activemodel/lib/active_model/attribute_set.rb:51: warning: the block passed to 'ActiveModel::Attribute#value' defined at activemodel/lib/active_model/attribute.rb:41 may be ignored activerecord/test/cases/transactions_test.rb:158: warning: the block passed to 'ActiveRecord::ConnectionAdapters::NullTransaction#after_rollback' defined at activerecord/lib/active_record/connection_adapters/abstract/transaction.rb:121 may be ignored activerecord/lib/arel.rb:70: warning: the block passed to 'Arel::Nodes::Node#fetch_attribute' defined at activerecord/lib/arel/nodes/node.rb:155 may be ignored activerecord/lib/arel/visitors/to_sql.rb:757: warning: the block passed to 'Arel::Collectors::SubstituteBinds#add_bind' defined at activerecord/lib/arel/collectors/substitute_binds.rb:18 may be ignored activerecord/lib/arel/visitors/to_sql.rb:757: warning: the block passed to 'ActiveRecord::StatementCache::PartialQueryCollector#add_bind' defined at activerecord/lib/active_record/statement_cache.rb:77 may be ignored activerecord/lib/arel/visitors/to_sql.rb:352: warning: the block passed to 'ActiveRecord::StatementCache::PartialQueryCollector#add_binds' defined at activerecord/lib/active_record/statement_cache.rb:83 may be ignored activerecord/lib/active_record/migration/default_strategy.rb:10: warning: the block passed to 'drop_table' defined at activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb:531 may be ignored activerecord/lib/active_record/querying.rb:23: warning: the block passed to 'with' defined at activerecord/lib/active_record/relation/query_methods.rb:475 may be ignored /opt/rubies/3.4-dev-04-19/lib/ruby/3.4.0+0/delegate.rb:84: warning: the block passed to 'WeakRef#__getobj__' defined at /opt/rubies/3.4-dev-04-19/lib/ruby/3.4.0+0/weakref.rb:44 may be ignored activerecord/lib/arel/visitors/to_sql.rb:782: warning: the block passed to 'Arel::Collectors::SubstituteBinds#add_binds' defined at activerecord/lib/arel/collectors/substitute_binds.rb:23 may be ignored /opt/rubies/3.4-dev-04-19/lib/ruby/3.4.0+0/delegate.rb:349: warning: the block passed to 'ActiveModel::Type::Value#serializable?' defined at activemodel/lib/active_model/type/value.rb:28 may be ignored activerecord/lib/arel/nodes/grouping.rb:7: warning: the block passed to 'Arel::Nodes::SqlLiteral#fetch_attribute' defined at activerecord/lib/arel/nodes/sql_literal.rb:22 may be ignored activerecord/lib/active_record/migration/default_strategy.rb:10: warning: the block passed to 'drop_join_table' defined at activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb:418 may be ignored ### Active Model activemodel/lib/active_model/attribute_set/builder.rb:55: warning: the block passed to 'ActiveModel::Attribute#value' defined at activemodel/lib/active_model/attribute.rb:41 may be ignored ### Action View actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/test/template/template_test.rb:198: warning: the block passed to 'TestERBTemplate#render' defined at actionview/test/template/template_test.rb:65 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored activerecord/lib/arel/collectors/composite.rb:34: warning: the block passed to 'Arel::Collectors::Bind#add_binds' defined at activerecord/lib/arel/collectors/bind.rb:21 may be ignored activerecord/lib/arel/collectors/composite.rb:34: warning: the block passed to 'Arel::Collectors::Bind#add_binds' defined at activerecord/lib/arel/collectors/bind.rb:21 may be ignored activerecord/lib/arel/collectors/composite.rb:34: warning: the block passed to 'Arel::Collectors::Bind#add_binds' defined at activerecord/lib/arel/collectors/bind.rb:21 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored ### Action Pack actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::HTML#render' defined at actionview/lib/action_view/template/html.rb:24 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::HTML#render' defined at actionview/lib/action_view/template/html.rb:24 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::HTML#render' defined at actionview/lib/action_view/template/html.rb:24 may be ignored actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::HTML#render' defined at actionview/lib/action_view/template/html.rb:24 may be ignored ``` As expected, it's all false positive caused by "duck-typing". ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-108054 * Author: ko1 (Koichi Sasada) * Status: Closed * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117640] [Ruby master Bug#20442] Printing class variable inconsistent when using nested hashes
by k776 (Kieran Pilkington) 22 Apr '24

22 Apr '24

Issue #20442 has been reported by k776 (Kieran Pilkington). ---------------------------------------- Bug #20442: Printing class variable inconsistent when using nested hashes https://bugs.ruby-lang.org/issues/20442 * Author: k776 (Kieran Pilkington) * Status: Open * ruby -v: 3.3.0 * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- See attached test.rb. I have a class with a class variable. The idea is simple, increment the value and then print out the resulting class var. ``` Mock1 0 1 Mock2 {} {"a"=>1} Mock3 {} {"a"=>1} Mock4 {} {} ``` Mock1 through Mock3 are working as expected. But Mock4 does not, it outputs a blank hash, rather than the expected: `{"a"=>{"a"=>1}}` The value is being set. If I replace the print method with `@val['a']['a']` is outputs the number 1 as expected. ---Files-------------------------------- test.rb (820 Bytes) -- https://bugs.ruby-lang.org/

2 1

[ruby-core:117631] [Ruby master Misc#20441] Should passing keyword args to method_name(*) be an error?
by ozydingo (Andrew Schwartz) 21 Apr '24

21 Apr '24

Issue #20441 has been reported by ozydingo (Andrew Schwartz). ---------------------------------------- Misc #20441: Should passing keyword args to method_name(*) be an error? https://bugs.ruby-lang.org/issues/20441 * Author: ozydingo (Andrew Schwartz) * Status: Open ---------------------------------------- In the following method: ```rb def foo(*) super end ``` it is apparently the intended ruby 3 behavior to pass keyword args as a positional Hash to `super`. I believe this is confusing and can lead to hidden and hard-to-discover bugs (e.g. #20440). Since `*` is meant to only represent positional args, should it be an ArgumentError to pass keyword args at all to this method? Similar to how it is an error to pass positions args to `bar(**)`. -- https://bugs.ruby-lang.org/

2 4

[ruby-core:117624] [Ruby master Bug#20440] `super` from child class passing keyword arg as Hash if in a method with passthrough args called from base class
by ozydingo (Andrew Schwartz) 21 Apr '24

21 Apr '24

Issue #20440 has been reported by ozydingo (Andrew Schwartz). ---------------------------------------- Bug #20440: `super` from child class passing keyword arg as Hash if in a method with passthrough args called from base class https://bugs.ruby-lang.org/issues/20440 * Author: ozydingo (Andrew Schwartz) * Status: Open * ruby -v: 3.3.0 * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- Apologies for the verbose title, but that's the specific set of conditions that AFAICT are required to reproduce the bug! Here's the simplest setup I can reproduce: ```rb class Base def foo(*args, x: 1) puts "Base: calling foo with args: #{args}, x: #{x}" end def foo!(x: 1) puts "Base: calling foo! with x: #{x}" foo(x: x) end end class Child < Base def foo(*) puts "Child: calling foo" super end end ``` When I call `Child.new.foo!`, I expect it to call the base class method `foo!`, which will use the default keyword arg `x: 1`; then the child method `foo` with `x: 1`, and finally the base method `foo` with `x: 1`. However, this is not what I observe: ```rb Child.new.foo! Base: calling foo! with x: 1 Child: calling foo Base: calling foo with args: [{:x=>1}], x: 1 ``` So when the child `foo` method called `super`, it passed not only `x: 1` as a keyword arg, but *also* `{x: 1}` as a Hash positional arg to the super method. This is breaking my upgrade to Ruby 3.0 as I have a similar setup but without the `*args` param, this I am getting the error "wrong number of arguments (given 1, expected 0)". -- https://bugs.ruby-lang.org/

4 6

[ruby-core:117623] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by byroot (Jean Boussier) 20 Apr '24

20 Apr '24

Issue #15554 has been updated by byroot (Jean Boussier). > it's basically a variant of the super case. I never thought about it this way, but I think it does make sense yes. > could you compare the results on rails tests? @ko1 sorry didn't answer yet. I'll try to do that Monday. ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-108031 * Author: ko1 (Koichi Sasada) * Status: Closed * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:117622] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block
by matthewd (Matthew Draper) 20 Apr '24

20 Apr '24

Issue #15554 has been updated by matthewd (Matthew Draper). Eregon (Benoit Daloze) wrote in #note-40: > With the example from @mame, I think adding `(&)` or `(&on_missing)` for `Attribute::Attribute#value` would be good to clarify the value method can receive a block, and purposefully ignores it. > To me it sounds weird that there are method overrides for the same method with "different" signatures. > In fact if it was a positional or keyword argument instead of a block argument, one would already need to add e.g. `def value(arg = nil/*/kw: nil/**)` for `Attribute::Attribute#value`. > So I think it's good to treat a block argument closer to positional or keyword arguments in this aspect. FWIW, as an Active Record maintainer, I agree. The code is currently structured as it is, using the block, specifically because it's aesthetically convenient to "hide" the rarely-needed argument in a place that other overriding methods won't need to know it's present... but I think it's more "clever cheat" than "valuable design that should be preserved, warning-free". byroot (Jean Boussier) wrote in #note-43: > `job_or_instantiate` doesn't accept a block. But for simplicity we forward all the arguments. I think this one is interesting because it's basically a variant of the `super` case. I'd still hope that both `...` and zsuper could be friendly to existing code, _and_ warn on truly-questionable code, by having them note that the block they're implicitly forwarding has already been consumed [by the calling method also using `yield` etc] -- meaning that a non-block-expecting method should _not_ warn/fail on that block argument, even though it would if given an explicitly supplied block [or an implicit forward that has not been consumed by any earlier caller]. ---------------------------------------- Feature #15554: warn/error passing a block to a method which never use a block https://bugs.ruby-lang.org/issues/15554#change-108030 * Author: ko1 (Koichi Sasada) * Status: Closed * Assignee: matz (Yukihiro Matsumoto) ---------------------------------------- # Abstract Warn or raise an ArgumentError if block is passed to a method which does not use a block. In other words, detect "block user methods" implicitly and only "block user methods" can accept a block. # Background Sometimes, we pass a block to a method which ignores the passed block accidentally. ``` def my_open(name) open(name) end # user hopes it works as Kernel#open which invokes a block with opened file. my_open(name){|f| important_work_with f } # but simply ignored... ``` To solve this issue, this feature request propose showing warnings or raising an exception on such case. Last developer's meeting, matz proposed `&nil` which declares this method never receive a block. It is explicit, but it is tough to add this `&nil` parameter declaration to all of methods (do you want to add it to `def []=(i, e, &nil)`?). (I agree `&nil` is valuable on some situations) # Spec ## Define "use a block" methods We need to define which method accepts a block and which method does not. * (1) method has a block parameter (`&b`) * (2) method body has `yield' * (3) method body has `super` (ZSUPER in internal terminology) or `super(...)` * (4) method body has singleton method (optional) (1) and (2) is very clear. I need to explain about (3) and (4). (3). `super` (ZSUPER) passes all parameters as arguments. So there is no surprise that which can accept `block`. However `super(...)` also passes a block if no explicit block passing (like `super(){}` or `super(&b)`) are written. I'm not sure we need to continue this strange specification, but to keep compatibility depending this spec, I add this rule. (4). surprisingly, the following code invoke a block: ``` def foo class << Object.new yield end end foo{ p :ok } #=> :ok ``` I'm also not sure we need to keep this spec, but to allow this spec, I added (4) rule. Strictly speaking, it is not required, but we don't keep the link from singleton class ISeq to lexical parent iseq now, so I added it. ## Exceptional cases A method called by `super` doesn`t warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods don`t use a block. So my patch ignores callings by `super`. ## corner cases There are several cases to use block without (1)-(4) rules. ### `Proc.new/proc/lambda` without a block Now it was deprecated in r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). Related discussion: [Bug #15539] ### `block_given?` `block_given?` expects block, but I believe we use it with `yield` or a block parameter. If you know the usecase without them, please tell us. ### `yield` in `eval` We can't know `yield` (or (3), (4) rule) in an `eval` evaluating string at calling time. ``` def foo eval('yield`) end foo{} # at calling time, # we can't know the method foo can accept a block or not. ``` So I added a warning to use `yield` in `eval` like that: `test.rb:4: warning: use yield in eval will not be supported in Ruby 3.` Workaround is use a block parameter explicitly. ``` def foo &b eval('b.call') end foo{ p :ok } ``` # Implementation Strategy is: * [compile time] introduce `iseq::has_yield` field and check it if the iseq (or child iseq) contains `yield` (or something) * [calling time] if block is given, check `iseq::has_yield` flag and show warning (or raise an exception) https://gist.github.com/ko1/c9148ad0224bf5befa3cc76ed2220c0b On this patch, now it raises an error to make it easy to detect. It is easy to switch to show the warning. # Evaluation and discussion I tried to avoid ruby's tests. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786 Here is a patch. There are several patterns to avoid warnings. ## tests for `block_given?`, `Proc.new` (and similar) without block Add a dummy block parameter. It is test-specific issue. ## empty `each` Some tests add `each` methods do not `yield`, like: `def each; end`. Maybe test-specific issue, and adding a dummy block parameter. ## Subtyping / duck typing https://github.com/ruby/ruby/blob/c01a5ee85e2d6a7128cccafb143bfa694284ca87/… This `parse` method doesn't use `yield`, but other sub-type's `parse` methods use. ## `super` with `new` method https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… This method override `Class#new` method and introduce a hook with block (yield a block in this hook code). https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package/tar_writer.rb#… In this method, call `super` and it also passing a block. However, called `initialize` doesn't use a block. ## Change robustness This change reduce robustness for API change. `Delegator` requires to support `__getobj__` for client classes. Now `__getobj__` should accept block but most of `__getobj__` clients do not call given block. https://github.com/ruby/ruby/blob/trunk/lib/delegate.rb#L80 This is because of delegator.rb's API change. https://gist.github.com/ko1/37483e7940cdc4390bf8eb0001883786#file-tests-pat… Nobu says calling block is not required (ignoring a block is no problem) so it is not a bug for delegator client classes. ## Found issues. ``` [ 2945/20449] Rinda::TestRingServer#test_do_reply = 0.00 s 1) Error: Rinda::TestRingServer#test_do_reply: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:635:in `test_do_reply' [ 2946/20449] Rinda::TestRingServer#test_do_reply_local = 0.00 s 2) Error: Rinda::TestRingServer#test_do_reply_local: ArgumentError: passing block to the method "with_timeout" (defined at /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:787) is never used. /home/ko1/src/ruby/trunk/test/rinda/test_rinda.rb:657:in `test_do_reply_local' [10024/20449] TestGemRequestSetGemDependencyAPI#test_platform_mswin = 0.01 s 3) Error: TestGemRequestSetGemDependencyAPI#test_platform_mswin: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:655:in `test_platform_mswin' [10025/20449] TestGemRequestSetGemDependencyAPI#test_platforms = 0.01 s 4) Error: TestGemRequestSetGemDependencyAPI#test_platforms: ArgumentError: passing block to the method "util_set_arch" (defined at /home/ko1/src/ruby/trunk/lib/rubygems/test_case.rb:1053) is never used. /home/ko1/src/ruby/trunk/test/rubygems/test_gem_request_set_gem_dependency_api.rb:711:in `test_platforms' ``` These 4 detection show the problem. `with_timeout` method (used in Rinda test) and `util_set_arch` method (used in Rubygems test) simply ignore the given block. So these tests are simply ignored. I reported them. (https://github.com/rubygems/rubygems/issues/2601) ## raise an error or show a warning? At least, Ruby 2.7 should show warning for this kind of violation with `-w`. How about for Ruby3? -- https://bugs.ruby-lang.org/

1 0