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

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.

[byroot (Jean Boussier)]

So I went over Rails warnings after the last patch, I may have missed some, but there is now about 5 unique warnings left:

/rails/activejob/lib/active_job/enqueuing.rb:69: warning: the block passed to 'job_or_instantiate' defined at /rails/activejob/lib/active_job/enqueuing.rb:78 may be ignored

This one is the one I mentioned above with .... It's technically correct, I'll likely fix it in Rails.

/rails/actionpack/test/controller/resources_test.rb:592: warning: the block passed to 'ResourcesTest#assert_singleton_named_routes_for' defined at /rails/actionpack/test/controller/resources_test.rb:1308 may be ignored

Totally legit, allowed to find a couple assertions that were ignored in the test suite.

/rails/activesupport/lib/active_support/callbacks.rb:130: warning: the block passed to 'FilterTest::NonYieldingAroundFilterController#non_yielding_action' defined at /rails/actionpack/test/controller/filters_test.rb:517 may be ignored

non_yielding_action is essentially a stub, so technically a false positive, but I think it's acceptable.

/rails/activerecord/test/cases/serialized_attribute_test.rb:505: warning: the block passed to 'attribute' defined at /rails/activemodel/lib/active_model/attribute_registration.rb:12 may be ignored

Legit, uncovered a problem in a test.

/usr/local/lib/ruby/gems/3.4.0+0/gems/actionview-7.1.3.2/lib/action_view/base.rb:264: warning: the block passed to '_inline_template___4411181644816080575_6440' defined at inline template:0 may be ignored

Semi-legit, the Rails "template" interface allow for a block, so we always pass one, but not all implementation use it.

My overall impression after the latest change:

• It is much much less noisy
• I don't know how many true positive are now silenced, but the interesting thing is that the noise reduction allowed me to find 2-3 bugs I couldn't before because it was hidden in the noise.
• There is still things that aren't strictly false positives, but that are somewhat intended, but I think it's acceptable.

[Dan0042 (Daniel DeLorme)]

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

So I went over Rails warnings after the last patch, I may have missed some, but there is now about 5 unique warnings left:

/rails/activejob/lib/active_job/enqueuing.rb:69: warning: the block passed to 'job_or_instantiate' defined at /rails/activejob/lib/active_job/enqueuing.rb:78 may be ignored

This one is the one I mentioned above with .... It's technically correct, I'll likely fix it in Rails.

Sorry, I still don't get this. Is this the fix? https://github.com/Shopify/rails/commit/640c7c751fa2b5d3d2e634685fbf0807c639a2ca#diff-702c0e36deefbad2dd430f4b34573a7de9d2ee2e78c3bad62552772b5612e099
enqueuing.rb:78 is a comment line so it feels like I'm looking at the wrong code.
Without knowing where the block was defined it's a bit hard to say anything, but it looks to me like job_or_instantiate should be

        def job_or_instantiate(arg, ...) # :doc:
          arg.is_a?(self) ? arg : new(arg, ...)
        end

[byroot (Jean Boussier)]

Is this the fix?

Yes.

Without knowing where the block was defined it's a bit hard to say anything

I pasted the code above. But in short job_or_instantiate doesn't take a block, but it's just more convenient to forward everything with ... because we have to deal with ruby2_keywords here.

but it looks to me like job_or_instantiate should be

No, because new shouldn't receive a block. A job argument must be serializable, and blocks can't be serialized.

[Dan0042 (Daniel DeLorme)]

I pasted the code above.

But you didn't paste the part where perform_later is invoked, that's what I meant. If someone calls perform_later{ something } then that block should be used somewhere, no?

[byroot (Jean Boussier)]

It doesn't matter where perform_later is invoked. perform_later do accept a block and will yield to it if provided.

[Dan0042 (Daniel DeLorme)]

Thank you for your patience, I finally understand.

---

On a separate note,
based on https://github.com/Shopify/rails/commit/640c7c751fa2b5d3d2e634685fbf0807c639a2ca
it seems that many of the false positives are due to stub methods.

I think this heuristic would work really well to reduce false positives:

• If a method accepts positional/keyword arguments and ignores them, it's ok to ignore the block argument as well

So the following methods would not warn even if called with a block:

def test1(*) = nil
def test2(**) = nil
def test3(...) = nil
def test4(value) = nil #possible?

[Eregon (Benoit Daloze)]

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

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

I was thinking about that too, to make the duck typing cases opt-in via env var/CLI arg.
It's not ideal because quite hidden but it seems better than not having at all.
I think we should keep that strict mode option (or make it default).

[matthewd (Matthew Draper)]

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

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.

FWIW, as an Active Record maintainer, I agree. The code is currently structured as it is, using the block, specifically because it's aesthetically convenient to "hide" the rarely-needed argument in a place that other overriding methods won't need to know it's present... but I think it's more "clever cheat" than "valuable design that should be preserved, warning-free".

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

job_or_instantiate doesn't accept a block. But for simplicity we forward all the arguments.

I think this one is interesting because it's basically a variant of the super case.

I'd still hope that both ... and zsuper could be friendly to existing code, and warn on truly-questionable code, by having them note that the block they're implicitly forwarding has already been consumed [by the calling method also using yield etc] -- meaning that a non-block-expecting method should not warn/fail on that block argument, even though it would if given an explicitly supplied block [or an implicit forward that has not been consumed by any earlier caller].

[byroot (Jean Boussier)]

it's basically a variant of the super case.

I never thought about it this way, but I think it does make sense yes.

could you compare the results on rails tests?

@ko1 (Koichi Sasada) sorry didn't answer yet. I'll try to do that Monday.

[byroot (Jean Boussier)]

@ko1 (Koichi Sasada) here's the result (I ran it on the 5 most important sub components)

### Active Record

activemodel/lib/active_model/attribute.rb:63: warning: the block passed to 'ActiveModel::Type::Value#serializable?' defined at activemodel/lib/active_model/type/value.rb:28 may be ignored
activerecord/lib/arel/collectors/composite.rb:34: warning: the block passed to 'Arel::Collectors::Bind#add_binds' defined at activerecord/lib/arel/collectors/bind.rb:21 may be ignored
activerecord/lib/arel/visitors/to_sql.rb:352: warning: the block passed to 'Arel::Collectors::SubstituteBinds#add_binds' defined at activerecord/lib/arel/collectors/substitute_binds.rb:23 may be ignored
activerecord/lib/active_record/associations/association_scope.rb:148: warning: the block passed to 'ActiveRecord::Associations::AssociationScope::ReflectionProxy#all_includes' defined at activerecord/lib/active_record/associations/association_scope.rb:109 may be ignored
activemodel/lib/active_model/attribute_set/builder.rb:43: warning: the block passed to 'ActiveModel::Attribute#value' defined at activemodel/lib/active_model/attribute.rb:41 may be ignored
activemodel/lib/active_model/attribute_set.rb:51: warning: the block passed to 'ActiveModel::Attribute#value' defined at activemodel/lib/active_model/attribute.rb:41 may be ignored
activerecord/test/cases/transactions_test.rb:158: warning: the block passed to 'ActiveRecord::ConnectionAdapters::NullTransaction#after_rollback' defined at activerecord/lib/active_record/connection_adapters/abstract/transaction.rb:121 may be ignored
activerecord/lib/arel.rb:70: warning: the block passed to 'Arel::Nodes::Node#fetch_attribute' defined at activerecord/lib/arel/nodes/node.rb:155 may be ignored
activerecord/lib/arel/visitors/to_sql.rb:757: warning: the block passed to 'Arel::Collectors::SubstituteBinds#add_bind' defined at activerecord/lib/arel/collectors/substitute_binds.rb:18 may be ignored
activerecord/lib/arel/visitors/to_sql.rb:757: warning: the block passed to 'ActiveRecord::StatementCache::PartialQueryCollector#add_bind' defined at activerecord/lib/active_record/statement_cache.rb:77 may be ignored
activerecord/lib/arel/visitors/to_sql.rb:352: warning: the block passed to 'ActiveRecord::StatementCache::PartialQueryCollector#add_binds' defined at activerecord/lib/active_record/statement_cache.rb:83 may be ignored
activerecord/lib/active_record/migration/default_strategy.rb:10: warning: the block passed to 'drop_table' defined at activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb:531 may be ignored
activerecord/lib/active_record/querying.rb:23: warning: the block passed to 'with' defined at activerecord/lib/active_record/relation/query_methods.rb:475 may be ignored
/opt/rubies/3.4-dev-04-19/lib/ruby/3.4.0+0/delegate.rb:84: warning: the block passed to 'WeakRef#__getobj__' defined at /opt/rubies/3.4-dev-04-19/lib/ruby/3.4.0+0/weakref.rb:44 may be ignored
activerecord/lib/arel/visitors/to_sql.rb:782: warning: the block passed to 'Arel::Collectors::SubstituteBinds#add_binds' defined at activerecord/lib/arel/collectors/substitute_binds.rb:23 may be ignored
/opt/rubies/3.4-dev-04-19/lib/ruby/3.4.0+0/delegate.rb:349: warning: the block passed to 'ActiveModel::Type::Value#serializable?' defined at activemodel/lib/active_model/type/value.rb:28 may be ignored
activerecord/lib/arel/nodes/grouping.rb:7: warning: the block passed to 'Arel::Nodes::SqlLiteral#fetch_attribute' defined at activerecord/lib/arel/nodes/sql_literal.rb:22 may be ignored
activerecord/lib/active_record/migration/default_strategy.rb:10: warning: the block passed to 'drop_join_table' defined at activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb:418 may be ignored

### Active Model

activemodel/lib/active_model/attribute_set/builder.rb:55: warning: the block passed to 'ActiveModel::Attribute#value' defined at activemodel/lib/active_model/attribute.rb:41 may be ignored

### Action View

actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored
activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored
activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored
activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored
activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored
activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/test/template/template_test.rb:198: warning: the block passed to 'TestERBTemplate#render' defined at actionview/test/template/template_test.rb:65 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored
activerecord/lib/arel/collectors/composite.rb:28: warning: the block passed to 'Arel::Collectors::Bind#add_bind' defined at activerecord/lib/arel/collectors/bind.rb:16 may be ignored
activerecord/lib/arel/collectors/composite.rb:34: warning: the block passed to 'Arel::Collectors::Bind#add_binds' defined at activerecord/lib/arel/collectors/bind.rb:21 may be ignored
activerecord/lib/arel/collectors/composite.rb:34: warning: the block passed to 'Arel::Collectors::Bind#add_binds' defined at activerecord/lib/arel/collectors/bind.rb:21 may be ignored
activerecord/lib/arel/collectors/composite.rb:34: warning: the block passed to 'Arel::Collectors::Bind#add_binds' defined at activerecord/lib/arel/collectors/bind.rb:21 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored

### Action Pack

actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Text#render' defined at actionview/lib/action_view/template/text.rb:23 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::Renderable#render' defined at actionview/lib/action_view/template/renderable.rb:15 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::HTML#render' defined at actionview/lib/action_view/template/html.rb:24 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::RawFile#render' defined at actionview/lib/action_view/template/raw_file.rb:20 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::HTML#render' defined at actionview/lib/action_view/template/html.rb:24 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::HTML#render' defined at actionview/lib/action_view/template/html.rb:24 may be ignored
actionview/lib/action_view/renderer/template_renderer.rb:66: warning: the block passed to 'ActionView::Template::HTML#render' defined at actionview/lib/action_view/template/html.rb:24 may be ignored

As expected, it's all false positive caused by "duck-typing".

[ko1 (Koichi Sasada)]

Thank you, could you compare the relax results on that? or all of your results are false positive?

[byroot (Jean Boussier)]

could you compare the relax results on that? or all of your results are false positive?

Yes. It's all false positive.

[Dan0042 (Daniel DeLorme)]

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

@ko1 (Koichi Sasada) here's the result (I ran it on the 5 most important sub components)

I think it's worth mentioning there's 67 warnings in that output but "only" 25 uniques. Acceptable for a codebase as big as Rails?

In order to understand the false positives I had a look at the "add_bind" warnings. And at first I thought the ignored block was actually a bug! It took me half an hour of looking through the code to finally understand what it did and why it was ok to ignore the block. So while it's a false positive, this mirrors my experience in #note-45 that making the arguments more explicit would really help code clarity. my 2¢

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

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

[ko1 (Koichi Sasada)]

• Status changed from Closed to Assigned

BTW on our application I found the following issue on RSpec tests on strict mode.

it ... do
  subject do ... end  # this subject ignores the block
end

This kind of issue can not be checked with current relax mode (because subject{} works outside of it block).

I feel this kind of information is useful if developers have (1) effective warning filtering mechanism and (2) a tool to list the suspicious methods by analyzing the acquired warnings.

For (1), at least application authors doesn't need gem's warning.
For (2), false positive will be ignored easily (as https://bugs.ruby-lang.org/issues/15554#note-57).

Also I'm afraid that if we introduce strict mode option (like environment variable), people can believe duck typing methods should make explicit for accepting block or not. It is what Matz does not like (now).

[nobu (Nobuyoshi Nakada)]

I want the message to be separated for caller and callee, to tag jump.

Current:

/home/runner/.rubies/ruby-head/lib/ruby/gems/3.4.0+0/gems/test-unit-3.6.2/lib/test/unit/fixture.rb:284: warning: the block passed to 'priority_setup' defined at /home/runner/.rubies/ruby-head/lib/ruby/gems/3.4.0+0/gems/test-unit-3.6.2/lib/test/unit/priority.rb:183 may be ignored

Such as:

/home/runner/.rubies/ruby-head/lib/ruby/gems/3.4.0+0/gems/test-unit-3.6.2/lib/test/unit/fixture.rb:284: warning: the block may be ignored
/home/runner/.rubies/ruby-head/lib/ruby/gems/3.4.0+0/gems/test-unit-3.6.2/lib/test/unit/priority.rb:183: info: passed to 'priority_setup'

[Eregon (Benoit Daloze)]

We are discussing how to make opt-in.
Because relaxed mode can miss cases (e.g. for methods with a common name), and some newer codebases/some Rubyists might want to use the stricter check (e.g. it enhances readability, ignored blocks by duck typing are ignored explicitly).

One idea is command-line flag: --strict-unused-block-warning / --pedantic-warnings ?

Or it could be a new warning category, e.g. :pedantic, disabled by default (similar to performance category).
Maybe the name should be more specific? (e.g. :strict_unused_block). Or not?

Thoughts on this?

--warn-unused-block/:warn_unused_block?

[ko1 (Koichi Sasada)]

As @Eregon (Benoit Daloze) mentioned on #60 we need to discuss how to enable strict mode.
https://hackmd.io/CoLraFp_QrqyHBcv3g8bVg?view#Feature-15554-warnerror-passing-a-block-to-a-method-which-never-use-a-block-ko1

Summary of proposed ways:

• Environment variable (RUBY_WARN_UNUSED_BLOCK_STRICTLY for example)
• ruby command option
  • --strict-unused-block-warning
  • --pedantic-warning
  • --warn-unused-block
• Warning category option like Warning[:performance] = true
  • Warning[:pedantic]
  • Special category for this option: Warning[:strict_unused_block]
  • (and they are enabled on command line option like: ruby -W:pedantic)

[byroot (Jean Boussier)]

Warning category option like

I think this is the better option as it build on top of an existing interface.

And probably some existing warnings could be moved into that category. The way I see it, a pedantic category would be for all the warnings that have have chance to produce false positives. My hope is to be able to run my test suites with $VERBOSE = true and turn warnings into errors without having to filter any of them.

[Dan0042 (Daniel DeLorme)]

This is a bit off-topic, but I've often thought it would be nice to have multiple "levels" of warnings. So we could have the fine-grained Warning[:strict_unused_block] which, if unset, defaults to the coarse-grained Warning[:strict] which, if unset, defaults to $VERBOSE.

BTW "pedantic" might sound funny but it's both derogatory and undescriptive, and used here in a different sense to the -pedantic option of gcc. Maybe something like "potential_bug" would be better?

[matz (Yukihiro Matsumoto)]

I don' like the term "pedantic" either. How about "strict"? I mean --warn-strict-unused-block or Waning[:strict_unused_block].

Matz.

[byroot (Jean Boussier)]

"strict" works I suppose. What we really want to convey is that this group of warnings is more likely to have false positives.

[Dan0042 (Daniel DeLorme)]

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

"strict" works I suppose. What we really want to convey is that this group of warnings is more likely to have false positives.

Warning[:strict_unused_block]
Warning[:possible_unused_block]
Warning[:potential_unused_block]
Warning[:probable_unused_block]
?

[Eregon (Benoit Daloze)]

I think Warning[:strict_unused_block] is good.

What we really want to convey is that this group of warnings is more likely to have false positives.

Yes, but also it removes the not-fully-precise heuristic of "if any method with the same name but not necessarily the same class accepts a block, ignore all block warnings for calls to a method named like that".
I.e., it checks all methods/calls, not just some of them.
So "strict" seems a good fit, it does the proper checks, without using not-fully-precise heuristics.

[byroot (Jean Boussier)]

I thought we were looking for a generic group name that can be used for other future warnings that may have false positives.

[Eregon (Benoit Daloze)]

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

I thought we were looking for a generic group name that can be used for other future warnings that may have false positives.

I think it's necessary to be able to enable/disable specific ones, so I don't think there is much value for a generic group name.
It could always be added later though.

[Dan0042 (Daniel DeLorme)]

generic group name that can be used for other future warnings

I think it's necessary to be able to enable/disable specific ones

I agree with both of the above; they are both desirable features to have in a warning system.
At the same time let's keep in mind that the existing categories are pretty generic: deprecated, experimental

How about this:
Warning[:strict] can be turned on by command line flag -W:strict for any warnings that may have false positives.

And to disable specific ones, use the warning gem. (TBH I would like to see this functionality integrated into Ruby, but that's a different topic.)

[ko1 (Koichi Sasada)]

In this ticket, Warning[:strict_unused_block] is accepted by Matz.

We should discuss :strict category in different tickets.

[ko1 (Koichi Sasada)]

• Status changed from Assigned to Closed

Applied in changeset git|ab7ab9e4508c24b998703824aa9576fb2e092065.

---

Warning[:strict_unused_block]

to show unused block warning strictly.

class C
  def f = nil
end

class D
  def f = yield
end

[C.new, D.new].each{|obj| obj.f{}}

In this case, D#f accepts a block. However C#f doesn't
accept a block. There are some cases passing a block with
obj.f{} where obj is C or D. To avoid warnings on
such cases, "unused block warning" will be warned only if
there is not same name which accepts a block.
On the above example, C.new.f{} doesn't show any warnings
because there is a same name D#f which accepts a block.

We call this default behavior as "relax mode".

strict_unused_block new warning category changes from
"relax mode" to "strict mode", we don't check same name
methods and C.new.f{} will be warned.

[Feature #15554]

[Dan0042 (Daniel DeLorme)]

Now that strict_unused_block has been added as a warning category, I have to ask what is a "warning category" exactly? Normally I would consider a "category" to be a grouping of similar items. All warnings related to performance go into the "performance" category, etc. But this new "category" only toggles a single warning. It's not even used for the "given block not used" warnings in array.c

Are we going to gradually shift to one category per warning message? That's closer to the idea of #issue brought up in the context of Exceptions in #20024, not really a "category". But it's not something I would oppose; currently we build strings for warning messages then discard them after a regexp match... that's a weird way of doing things.

So what is supposed to be my mental model of "warning categories" now?