Project

General

Profile

Actions

Feature #15554

open

warn/error passing a block to a method which never use a block

Added by ko1 (Koichi Sasada) about 5 years ago. Updated 5 days ago.

Status:
Open
Target version:
-
[ruby-core:91214]

Description

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?


Related issues 1 (1 open0 closed)

Related to Ruby master - Feature #19979: Allow methods to declare that they don't accept a block via `&nil`OpenActions
Actions #1

Updated by ko1 (Koichi Sasada) about 5 years ago

  • Description updated (diff)

Updated by hsbt (Hiroshi SHIBATA) about 5 years ago

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

Updated by alanwu (Alan Wu) about 5 years ago

Related: Feature #10499

Updated by decuplet (Nikita Shilnikov) about 5 years ago

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

Updated by localhostdotdev (localhost .dev) almost 5 years ago

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

Updated by matthewd (Matthew Draper) almost 5 years ago

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.

Actions #7

Updated by k0kubun (Takashi Kokubun) almost 5 years ago

  • Description updated (diff)
Actions #8

Updated by k0kubun (Takashi Kokubun) almost 5 years ago

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

Updated by nobu (Nobuyoshi Nakada) about 3 years ago

  • Status changed from Open to Closed

Updated by Dan0042 (Daniel DeLorme) 4 months ago

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

Actions #11

Updated by hsbt (Hiroshi SHIBATA) 4 months ago

  • Status changed from Closed to Open

Updated by ko1 (Koichi Sasada) 19 days ago

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'

Updated by ko1 (Koichi Sasada) 19 days ago

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)

Updated by ko1 (Koichi Sasada) 19 days ago

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

Actions #15

Updated by matz (Yukihiro Matsumoto) 19 days ago

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

Updated by Eregon (Benoit Daloze) 19 days ago ยท Edited

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

Updated by Edwing123 (Edwin Garcia) 19 days ago

Regarding the warning message:

+1, these messages are clearer.

Updated by Dan0042 (Daniel DeLorme) 18 days ago

This would be a verbose-only warning right?

Updated by Eregon (Benoit Daloze) 18 days ago

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.

Updated by Dan0042 (Daniel DeLorme) 9 days ago

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?

Actions #21

Updated by k0kubun (Takashi Kokubun) 5 days ago

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

Updated by ko1 (Koichi Sasada) 5 days ago

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

Actions

Also available in: Atom PDF

Like4
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0