[ruby-core:117586] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block

["mame (Yusuke Endoh) via ruby-core" <ruby-core@ml.ruby-lang.org>]

Issue #15554 has been updated by mame (Yusuke Endoh).


I was also in the camp that `(&)` should be just added, but after analyzing one warning of rails at the dev meeting, I am now a little less sure. I will try to explain my feelings.

---

This call passes a block.

```ruby
read_attribute(attr_name) { |n| missing_attribute(n, caller) }
```
https://github.com/rails/rails/blob/6f0d1ad14b92b9f5906e44740fce8b4f1c7075dc/activerecord/lib/active_record/attribute_methods.rb#L413

After [one delegation](https://github.com/rails/rails/blob/6f0d1ad14b92b9f5906e44740fce8b4f1c7075dc/activerecord/lib/active_record/attribute_methods/read.rb#L29), the block is delegated to `fetch_value` method.

```ruby
def fetch_value(name, &block)
  self[name].value(&block)
end
```
https://github.com/rails/rails/blob/6f0d1ad14b92b9f5906e44740fce8b4f1c7075dc/activemodel/lib/active_model/attribute_set.rb#L50

Here, `self[name]` is usually an instance of `ActiveModel::Attribute`, so the block is passed to `ActiveModel::Attribute#value`. But it does not accept a block.

```
def value
  # `defined?` is cheaper than `||=` when we get back falsy values
  @value = type_cast(value_before_type_cast) unless defined?(@value)
  @value
end
```
https://github.com/rails/rails/blob/6f0d1ad14b92b9f5906e44740fce8b4f1c7075dc/activemodel/lib/active_model/attribute.rb#L41

So the warning is issued.

I think the reason why the block is passed is because `self[name]` is sometimes an instance of `ActiveModel::Attribute::Uninitialized`, whose `#value` method accepts a block.

```ruby
def value
  if block_given?
    yield name
  end
end
```
https://github.com/rails/rails/blob/6f0d1ad14b92b9f5906e44740fce8b4f1c7075dc/activemodel/lib/active_model/attribute.rb#L249-L253

---

It is trivial how to stop the warning. Just replace `def value` with `def value(&)`. The problem itself was pointed out by @ko1 with an abstract code example in https://bugs.ruby-lang.org/issues/15554#note-29. I had thought it was okay to add `(&)`. However, when I saw the above example in rails, I was less sure.

If Ruby requires a change in the code, I hope that change itself is an improvement of the code, such as, the code becomes more beautiful, shorter, more concise, clearer, etc. For example, this warning tends to require many test mock methods to accept `(&)` explicitly. I don't mind this, because I feel that it benefits the test code itself. It clarifies the test to explicitly state that the mock method would accept the block. Also, the fact that the mock method just discards a block may indicate a lack of test case.

However, I cannot justify adding `(&)` to the current definition of `ActiveModel::Attribute#value`. The code is concise enough, and there is no flaw. Just for consistency with the special case `ActiveModel::Attribute::Uninitialized#value`, it is not desirable to pollute the code of the major case `ActiveModel::Attribute#value`.

Of course, I understand the benefit of "easier to find bugs in other code", but I am not comfortable sacrificing innocent and beautiful code for that.

----------------------------------------
Feature #15554: warn/error passing a block to a method which never use a block
https://bugs.ruby-lang.org/issues/15554#change-107986

* 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/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?




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/