Project

General

Profile

Feature #15554

Updated by k0kubun (Takashi Kokubun) over 1 year ago

# 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 deprecated. r66772 (commit:9f1fb0a17febc59356d58cef5e98db61a3c03550). 
 Related discussion: [Bug #15539] [Bug#15539] for this issue. 

 ### `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/lib/optparse.rb#L698 

 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-patch-L61 

 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#L81 

 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-patch-L86 

 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? 

Back