Feature #15554: warn/error passing a block to a method which never use a block

• Status: Closed
• Assignee: matz (Yukihiro Matsumoto)
• Target version: -

[ko1 (Koichi Sasada)]

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 doesnt warn warning even if this method doesn't use a block. The rule (3) can pass blocks easily and there are many methods dont 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 (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?

[ko1 (Koichi Sasada)]

• Description updated (diff)

[hsbt (Hiroshi SHIBATA)]

I fixed the issues of TestGemRequestSetGemDependencyAPI on upstream repository and merged them at r66904.

[alanwu (Alan Wu)]

Related: Feature #10499

[decuplet (Nikita Shilnikov)]

I have a nice example where I use calls like super { ... } even if the super method doesn't yield a block. From my understanding, this behavior won't be broken by the changes, but I still want to add it to the context.
I'm the author of the dry-monads gem, it defines, you might have guessed, a bunch of monads. For instance, there is the Result monad which represents possibly-unsuccessful computation. A typical use case:

def call(params)
  validate(params).bind { |values|
    create_account(values[:account]).bind { |account|
      create_owner(account, values[:owner]).fmap { |owner|
        [account, owner]
      }
    }
  }
end

Here validate, create_account, and create_owner all return a Result value, binds compose the results together. That's rather ugly, that's why the gem also adds so-called do notation (the name has stolen from Haskell), it's a mixin you add to a class which prepends every method you define and passes a block which in order tries to unwrap a Result value. Better see the code and read the comments:

# this line also prepends the current class with a dynamically created module
include Dry::Monads::Do

# this method will be overridden in the prepended module
def call(params)
  # yield will halt the execution if validate returns Failure(...)
  # or unwraps the result if it returns Success(...) so that it'll
  # be assigned to values. This way we avoid the chain of .bind calls
  values = yield validate(params)
  account = yield create_account(values[:account])
  owner = yield create_owner(account, values[:owner])

  Success([account, owner])
end

Obviously, the do version is way cleaner yet has the same semantics. Dry::Monads::Do wraps all the methods as they are defined so that you don't need to worry whether a method unwraps Results or not. If it doesn't, then it just doesn't call the block, that simple. Also Dry::Monads::Do is smart enough to discard its block if another block is given to a method:

include Dry::Monads::Result::Mixin
include Dry::Monads::Do

def foo
  bar { 5 }
  baz # baz will return 6, see below
end

def bar
  # here the block comes from foo, not from Do
  yield + 1
end

def baz
  # no block from foo given, Do in action
  yield Success(6)
end

Now here's the problem.

include Dry::Monads::Do

def call
  foo
end

def foo
  # foo gets the block from Do but doesn't use it
  5
end

In the last example, I have no means to detect that foo won't use the block. I could analyze parameters, I guess it would be slower, but if a method uses yield I just cannot detect it without analyzing the source code, that'd be dead slow and not reliable either.

See what Do does here and there for more details.

I should add that dry-monads is quite popular, it's not only used by me :) (see it on rubygems https://rubygems.org/gems/dry-monads)

---

Gem's docs

[localhostdotdev (localhost .dev)]

To detect if a block is used, binding would also need to be detected, e.g.: def b(arg); arg.eval("yield"); end; def a; b(binding); end

[matthewd (Matthew Draper)]

This is great! Ignored blocks can be very confusing.

A method called by super doesnt warn warning even if this method doesn't use a block.
The rule (3) can pass blocks easily and there are many methods dont use a block.

So my patch ignores callings by super.

Would it be possible for a method called by super to only ignore an unexpected block if the calling method (that contained super) also contained something that explicitly uses/consumes the block (&b / yield)?

I think that would still allow existing uses where a method consumes a block itself, and then (accidentally) implicitly passes it to a super that doesn't want it -- but that it would also keep the warning in the other situation, where an entire super-chain does not use the block.

Otherwise I am worried that super becomes a bad thing to use, because it completely disables this new safety feature.

[k0kubun (Takashi Kokubun)]

• Description updated (diff)

[k0kubun (Takashi Kokubun)]

• Copied from Feature #10499: Eliminate implicit magic in Proc.new and Kernel#proc added

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

[Dan0042 (Daniel DeLorme)]

I think this was closed by mistake and should be re-opened.

[hsbt (Hiroshi SHIBATA)]

• Status changed from Closed to Open

[ko1 (Koichi Sasada)]

Retry it on current master 2024.

Summary:

• false positive on several cases:
  • intentional cases for duck typing
  • making mock cases they want to ignore arguments
• found a few issues by warnings

---

https://gist.github.com/ko1/804a0034fb0ec0bdcb67f492932dc976
is a patch to make unused block warning.

---

on make test-all

I need to add some patches to remove warnings:
https://gist.github.com/ko1/ee47ff7ba1eb8d2beaee74f4124a348d
Most of cases they are mock/wrapper methods for tests.

---

on make test-spec

I need to add some patches to remove warnings:
https://gist.github.com/ko1/ba24938ee810793d52e462961056190c
Most of cases they are mock/wrapper methods for tests.

We got several issues on cgi. Passing blocks for CGI related methods can be bugs (not checked details).
https://gist.github.com/ko1/3aafaf775e77074108bb0058ab47cd10

I found a test that the passed block will be ignored, but I'm not sure it is needed.

  it "ignores the supplied block" do
    -> { ObjectSpace.garbage_collect {} }.should_not raise_error
  end

The warning is the following:

/home/ko1/ruby/src/trunk/spec/ruby/core/objectspace/garbage_collect_spec.rb:14: warning: a block is given for a block unused method 'ObjectSpace.garbage_collect'

[ko1 (Koichi Sasada)]

Duck typing case:

class C1
  def foo = nil
end

class C2
  def foo = yield
end

[C1.new, C2.new].each{ _1.foo{ ... } }

RDoc's each_ancestor method(s) is.

---

Mock cases:

diff --git a/spec/ruby/security/cve_2019_8322_spec.rb b/spec/ruby/security/cve_2019_8322_spec.rb
index b70d78c033..7a25b68268 100644
--- a/spec/ruby/security/cve_2019_8322_spec.rb
+++ b/spec/ruby/security/cve_2019_8322_spec.rb
@@ -8,7 +8,7 @@
 describe "CVE-2019-8322 is resisted by" do
   it "sanitising owner names" do
     command = Gem::Commands::OwnerCommand.new
-    def command.rubygems_api_request(*args)
+    def command.rubygems_api_request(*args, &_)
       Struct.new(:body).new("---\n- email: \"\e]2;nyan\a\"\n  handle: handle\n  id: id\n")
     end
     def command.with_response(response)

In this case, it provides command.rubygems_api_request method for mock which ignores the given block.
I think this kind of code can be many places.
(using ... is one solutuon)

[ko1 (Koichi Sasada)]

We found issues with this warning system, like: https://github.com/ruby/ruby/commit/8316cb213c

[matz (Yukihiro Matsumoto)]

• Related to Feature #19979: Allow methods to declare that they don't accept a block via `&nil` added

[Eregon (Benoit Daloze)]

These results look encouraging to me.
The number of "false positives" seems reasonable.
Also they are not really false positives, because they are cases where a block is given and ignored by a method, but with the same call site also potentially using that block when calling another method.

It seems useful to have some way to suppress this warning at a call site for cases where it's not possible to modify the called method immediately (e.g. because it's in some other gem and so need a fix + release of that gem).
It's probably very rare though, as it would need to be such an intentional case for duck typing (same call site calls 2 methods, one accepts a block, one does not).
So maybe it's fine to not have such a way because rare enough.
Best way to know seems to experiment by merging this change.

Not sure what form to suppress the warning would be best.
I can think of a magic comment (e.g. ignore block warnings for all call sites between unused_block_warning: false and unused_block_warning: true).
Or it could be a way to mark a Method/UnboundMethod object (would mark the callee as "accepting a block"). But it feels quite wrong to mutate another gem's methods like that.

I found a test that the passed block will be ignored, but I'm not sure it is needed.

It's fine to remove that spec.

Regarding the warning message:

file.rb:line: warning: a block is given for a block unused method 'ObjectSpace.garbage_collect'

I think this would be better:

file.rb:line: warning: the passed block is ignored because 'ObjectSpace.garbage_collect' does not accept a block

and for a block given to a Proc:

file.rb:line: warning: the passed block is ignored because #<Proc:0x... file:line> does not accept a block

[Edwing123 (Edwin Garcia)]

Regarding the warning message:

+1, these messages are clearer.

[Dan0042 (Daniel DeLorme)]

This would be a verbose-only warning right?

[Eregon (Benoit Daloze)]

One concern if it's verbose-only is it won't help for typical mistakes with blocks like:

p foo do
  body
end

It would need to warn by default to address that.

[Dan0042 (Daniel DeLorme)]

Eregon (Benoit Daloze) wrote in #note-19:

It would need to warn by default to address that.

Or you need to use $VERBOSE, or it could be a new category like Warning[:strict]. But it only indicates a possible problem, not definitely a problem. At the very least I'd make it a verbose warning during 1-2 years so that gems have the time to update. When gems output warnings and there's no fix available, it can be quite annoying, as experienced during the 2.7 migration. Even more so if those warnings are false positives.

ko1 (Koichi Sasada) wrote in #note-14:

We found issues with this warning system, like: https://github.com/ruby/ruby/commit/8316cb213c

This is fairly conclusive evidence that the warning is helpful. Will no one add it to the next dev meeting agenda?

[k0kubun (Takashi Kokubun)]

• Copied from deleted (Feature #10499: Eliminate implicit magic in Proc.new and Kernel#proc)

[ko1 (Koichi Sasada)]

Matz accepted to try it with -w (WOW!) so I'll make a patch.

[hsbt (Hiroshi SHIBATA)]

• Status changed from Open to Assigned

[ko1 (Koichi Sasada)]

merged, but please give us your feedback.

[ko1 (Koichi Sasada)]

• Status changed from Assigned to Closed

Applied in changeset git|9180e33ca3a5886fec3f9e0a2f48072b55914e65.

---

show warning for unused block

With verbopse mode (-w), the interpreter shows a warning if
a block is passed to a method which does not use the given block.

Warning on:

• the invoked method is written in C
• the invoked method is not initialize
• not invoked with super
• the first time on the call-site with the invoked method
  (obj.foo{} will be warned once if foo is same method)

[Feature #15554]

Primitive.attr! :use_block is introduced to declare that primitive
functions (written in C) will use passed block.

For minitest, test needs some tweak, so use
https://github.com/minitest/minitest/commit/ea9caafc0754b1d6236a490d59e624b53209734a
for test-bundled-gems.

[byroot (Jean Boussier)]

I'm yet to process the warnings uncovered in our app, but to share an early one I think is relevant, from statsd-instrument:

      def distribution(key, value = UNSPECIFIED, sample_rate: nil, tags: nil, no_prefix: false, &block)
        check_block_or_numeric_value(value, &block)
        check_tags_and_sample_rate(sample_rate, tags)

        super
      end

      private

      def check_block_or_numeric_value(value)
        if block_given?
          raise ArgumentError, "The value argument should not be set when providing a block" unless value == UNSPECIFIED
        else
          raise ArgumentError, "The value argument should be a number, got #{value.inspect}" unless value.is_a?(Numeric)
        end
      end

This is easy to workaround by declaring the block I guess. 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?

[byroot (Jean Boussier)]

Also to include a positive note, it's definitely pointing at valuable things, e.g. from our test suite:

    SOME_LIST.excluding(things) do |behaviour|
      test "#... snip #{behaviour.serialize}" do
        # ...snip
      end
    end

Here the author clearly wanted to write SOME_LIST.excluding(things).each do |behaviour|, but the mistake silently did nothing and this code is essentially dead by accident.

[byroot (Jean Boussier)]

I worked on clearing these warnings from the Rails test suite: https://github.com/Shopify/rails/commit/640c7c751fa2b5d3d2e634685fbf0807c639a2ca

• It caught one big mistake in the test suite: https://github.com/Shopify/rails/commit/640c7c751fa2b5d3d2e634685fbf0807c639a2ca#diff-412bab7e684dc6250e9b25ee6e8252245f8bbef553279102acbee4d856edb264L335
• It generated a few dozen false positives because of duck-typing (e.g. Object#try(arg, &block) but NilClass#try(arg).
• It generated some false positive because of ... delegation for methods that don't accept a block.

So mixed results I guess. I'm extremely happy it caught this mistake, but the false positive rate is still a bit too high. ... should definitely be ignored.

[ko1 (Koichi Sasada)]

Matz hesitates to force to put &_ (or other tricks) for duck typing methods, so I try to skip warning if a foo uses block, any other foo doesn't warn this warning even if it doesn't use a block.

          class C0
            def f = yield
          end

          class C1 < C0
            def f = nil
          end

          [C0, C1].f{ block } # do not warn on both calls

https://github.com/ruby/ruby/pull/10559

[ko1 (Koichi Sasada)]

byroot (Jean Boussier) wrote in #note-28:

• It generated some false positive because of ... delegation for methods that don't accept a block.

for examoles?

class C
  def bar = yield
  def foo(...)
    bar(...)
  end
end

C.new.foo{}

doesn't show warning.

[byroot (Jean Boussier)]

doesn't show warning.

You are right, I got confused a bit my a method_missing. Please ignore that remark about ....

On another note, these warnings found another small issue in the Rails test suite: https://github.com/rails/rails/pull/51585/files

[Eregon (Benoit Daloze)]

ko1 (Koichi Sasada) wrote in #note-30:

doesn't show warning.

I think https://github.com/ruby/ruby/pull/10559 is warning too little, I commented there.
From my understanding it will not warn if there exists any method of the same name which accepts a block (even if that method is never called).
That sounds like it would miss tons of potential erroneous cases.

I think duck typing cases should be warned, they are potential bugs, it's weird one definition uses the block and another does not, it could very well be that the other definition should use it and it's a bug.
Adding &_ seems good to acknowledge the very rare case of this method might receive a block and ignores it purposefully.

[byroot (Jean Boussier)]

I think https://github.com/ruby/ruby/pull/10559 is warning too little, I commented there.

There is definitely a very fine line to walk here between false positives and false negatives.

Some of these warnings are very valuable, as demonstrated by the uncovered issues, but if there is too many false positives, the community won't use it.

Currently there is a small move toward running library test suites with warnings enabled, I'd hate if this would stop because of one warning that is too noisy.

I'm not sure if grouping all methods of the same name is the best thing to do or not, but just putting it out there that IMO, at least for a first version, we should likely prioritize reducing false positives over false negatives.

[Eregon (Benoit Daloze)]

IIUC @ko1's PR, for example the issue of https://bugs.ruby-lang.org/issues/15554#note-27 wouldn't be found if there is a single def excluding(*args, &block) defined anywhere in the loaded code (which does not seem unlikely).

[byroot (Jean Boussier)]

Yes I understand that too. And this will certainly cause a few false negative, but should also remove a lot more false positive.

I don't know if this is the best solution, but based on the warnings I saw in Rails test suite, I think it's worth a try. This feature clearly is still in flight, we have months to fine tune it.

[ko1 (Koichi Sasada)]

I think https://github.com/ruby/ruby/pull/10559 is warning too little, I commented there.

Yes. It will reduce many cases and it only warns only a few cases.

I think duck typing cases should be warned, they are potential bugs, it's weird one definition uses the block and another does not, it could very well be that the other definition should use it and it's a bug.
Adding &_ seems good to acknowledge the very rare case of this method might receive a block and ignores it purposefully.

I also understand the opinion.

quote from my comment:

"Matz hesitates to force to put &_ (or other tricks) for duck typing methods"

This is "Ruby"'s language design discussion.
So please persuade matz.

[akr (Akira Tanaka)]

I encouraged the idea of filtering out the warnings by method name at yesterday's meeting.

There were several ideas:
(1) force to add &_ for many methods (matz don't like)
(2) filter warnings by method name (discussed and implemented now)
(3) revert this warning and hope a type checker will generate this warning more precisely. (assuming &_ information is described in type signatures)

I encouraged the filtering because I guess the filtering reduces
most of the false positives (useless warning emissions).

I guess the filtering is better than reverting this warning.
A type checker for Ruby is not popular yet.

[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.

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, the block is delegated to fetch_value method.

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.

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 (Koichi Sasada) 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.

[jeremyevans0 (Jeremy Evans)]

One approach that reduces false positives (invalid warnings) and potentially increases false negatives (missing warnings) would be to only warn for literal blocks and not passed blocks:

def foo
end

def bar(&) = foo(&) # no warning

bar{}
foo{} # warning

Currently, this generates two warnings:

-:4: warning: the block passed to 'Object#foo' defined at -:1 may be ignored
-:7: warning: the block passed to 'Object#foo' defined at -:1 may be ignored

I propose the first warning be eliminated. In the above example, it is always an error, but in real world code, such as when foo is called indirectly using send, or when the foo method is defined differently for different objects, it may not be an error.

Passing a literal block to a method that doesn't accept a block is almost always an error. Passing an existing block to a method that doesn't accept it may or may not be an error. I think it's better to only warn in cases where we are fairly sure it is an error.

[Eregon (Benoit Daloze)]

With the example from @mame (Yusuke Endoh), 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.

I understand the other arguments too, notably it does not make the code more beautiful but I think it makes the code clearer.
This is clearly to some amount subjective.

I suspect it will be very difficult to change matz's opinion on this, at least this feature should still detect some ignored blocks bugs.

One more idea is we could emit a different warning message for the cases that would be excluded by the global method name ignore.
Then people could easily filter these specific warnings (and keep other block ignored warnings enabled) if they don't want to address that, but would otherwise be able to see all ignored block warnings and address them.

[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).

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 communicate that this method receives a block, and ignores it.

[ko1 (Koichi Sasada)]

I made strict mode option for trial (will remove soon)
https://github.com/ruby/ruby/pull/10578

@byroot (Jean Boussier) could you compare the results on rails tests?

[byroot (Jean Boussier)]

I don't understand that one. ...

As I said previously I made a mistake on that one. But here's the code that cause this warning:

def perform_later(...)
  job = job_or_instantiate(...)
  enqueue_result = job.enqueue

  yield job if block_given?

  enqueue_result
end

job_or_instantiate doesn't accept a block. But for simplicity we forward all the arguments. To fix the warnings I'd need to do:

def perform_later(*, **)
  job = job_or_instantiate(*, **)
  enqueue_result = job.enqueue

  yield job if block_given?

  enqueue_result
end

@byroot (Jean Boussier) (Jean Boussier) could you compare the results on rails tests?

I'll compile a list later today. But I can already says I checked https://buildkite.com/rails/rails-nightly yesterday and the number of false positive was way down.