From: "Dan0042 (Daniel DeLorme) via ruby-core" <ruby-core@...>
Date: 2024-04-19T00:53:33+00:00
Subject: [ruby-core:117602] [Ruby master Feature#15554] warn/error passing a block to a method which never use a block

Issue #15554 has been updated by Dan0042 (Daniel DeLorme).


byroot (Jean Boussier) wrote in #note-26:
> So I'd expect `block_given?` to qualify as "using the block". But I suppose it's tricky because `block_given?` is just a method like any other for the compiler?

I would also expect `block_given?` to qualify as "using the block", and I don't think there's any need to overthink it because it's just a method. If `block_given?` is present, do not show the warning. In the unlikely case that `block_given?` was actually a redefined method, then the warning is not displayed even if it should, and that's not really a serious issue.


byroot (Jean Boussier) wrote in #note-28:
> - It generated some false positive because of `...` delegation for methods that don't accept a block.

I don't understand that one. `...` delegation doesn't "create" a block that is then passed down the chain. If you have `foo(1,2,3)` without a block that is delegated via `...` to `def bar(x,y,z) = x+y+z`  then it shouldn't print a warning. But if you have `foo(1,2,3){ }` that is delegated via `...` to that same `bar` then it *should* print a warning, right?


ko1 (Koichi Sasada) wrote in #note-29:
> Matz hesitates to force to put `&_` (or other tricks) for duck typing methods

I don't understand this duck typing argument. Normally, duck typing requires a method to have the same name AND arguments (or at least a compatible signature).
```ruby
a = Object.new
def a.foo(x,y) = nil

b = Object.new
def b.foo(x) = nil

(rand<0.5 ? a : b).foo(1,2) #raises error 50% of the time; different arity = not duck-compatible
```
It's commonly understood that in order to have `a` and `b` be duck-compatible, their `foo` method must accept the same arguments. So if you want `b` to ignore an argument you'd have to define it like this: `def b.foo(x, dummy=nil) = nil`

Blocks are a type of argument, so the same should apply; in order for `ActiveModel::Attribute#value` and `ActiveModel::Attribute::Uninitialized#value` to be duck-compatible, they should both take a block or neither take a block. The fact that it hasn't been the case until now is more like a quirk of the language that has resulted in many facepalm bugs. `ActiveModel::Attribute#value` should be defined with `&` to indicate that this method receives a block, and ignores it.

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

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