Issue #15554 has been updated by byroot (Jean Boussier).
Acceptable for a codebase as big as Rails?
IMO:
- Rails isn't a big code base. Perhaps when compared to the average gem, but not
when compared to the average **application** people work on.
- Adding a warnings that most people will disregard because it throw too many false
positive isn't helping.
- Also consider all dependencies will throw warnings too, so when running this on an
application with a substantial amount of dependencies, this will generate a TON of noise.
I'm not hostile to declare ignored block, I think it's a pretty good idea, and to
be honest it's even a bit weird that block parameters are implicit, when others
aren't. But if that's the argument, then it would make more sense to me to just
always require declaring it, regardless of whether `yield` is used or not.
```ruby
> def foo = yield
> method(:foo).parameters
=> []
```
If you are trying to generate API docs or something along these lines, needed to parse the
source or bytecode to figure out if a `yield` exist is a lot of extra work.
But regardless of what is decided, I think doing it in stages would be preferable. As it
stand I'm pretty happy with this new warnings, and I think if it's to be made more
verbose, it could wait for Ruby 3.5.
----------------------------------------
Feature #15554: warn/error passing a block to a method which never use a block
https://bugs.ruby-lang.org/issues/15554#change-108079
* 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/