Feature #15554
closedwarn/error passing a block to a method which never use a block
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) orsuper(...)
- (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 don
t use a block.
So my patch ignores callings by super
.
corner cases¶
There are several cases to use block without (1)-(4) rules.
Proc.new/proc/lambda
without a block
Now it was deprecated in r66772 (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) containsyield
(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?
Updated by hsbt (Hiroshi SHIBATA) almost 6 years ago
I fixed the issues of TestGemRequestSetGemDependencyAPI on upstream repository and merged them at r66904.
Updated by alanwu (Alan Wu) almost 6 years ago
Related: Feature #10499
Updated by decuplet (Nikita Shilnikov) almost 6 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, bind
s 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 Result
s 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)
Updated by localhostdotdev (localhost .dev) over 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) over 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.
Updated by k0kubun (Takashi Kokubun) over 5 years ago
- Description updated (diff)
Updated by k0kubun (Takashi Kokubun) over 5 years ago
- Copied from Feature #10499: Eliminate implicit magic in Proc.new and Kernel#proc added
Updated by nobu (Nobuyoshi Nakada) almost 4 years ago
- Status changed from Open to Closed
Updated by Dan0042 (Daniel DeLorme) about 1 year ago
I think this was closed by mistake and should be re-opened.
Updated by hsbt (Hiroshi SHIBATA) about 1 year ago
- Status changed from Closed to Open
Updated by ko1 (Koichi Sasada) 9 months 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) 9 months 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) 9 months ago
We found issues with this warning system, like: https://github.com/ruby/ruby/commit/8316cb213c
Updated by matz (Yukihiro Matsumoto) 9 months ago
- Related to Feature #19979: Allow methods to declare that they don't accept a block via `&nil` added
Updated by Eregon (Benoit Daloze) 9 months 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 Anonymous 9 months ago
Regarding the warning message:
+1, these messages are clearer.
Updated by Dan0042 (Daniel DeLorme) 9 months ago
This would be a verbose-only warning right?
Updated by Eregon (Benoit Daloze) 9 months 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 months 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?
Updated by k0kubun (Takashi Kokubun) 9 months ago
- Copied from deleted (Feature #10499: Eliminate implicit magic in Proc.new and Kernel#proc)
Updated by ko1 (Koichi Sasada) 9 months ago
Matz accepted to try it with -w
(WOW!) so I'll make a patch.
Updated by hsbt (Hiroshi SHIBATA) 8 months ago
- Status changed from Open to Assigned
Updated by ko1 (Koichi Sasada) 8 months ago
merged, but please give us your feedback.
Updated by ko1 (Koichi Sasada) 8 months ago
- 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 iffoo
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
.
Updated by byroot (Jean Boussier) 8 months ago
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?
Updated by byroot (Jean Boussier) 8 months ago
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.
Updated by byroot (Jean Boussier) 8 months ago
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)
butNilClass#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.
Updated by ko1 (Koichi Sasada) 8 months ago
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
Updated by ko1 (Koichi Sasada) 8 months ago
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.
Updated by byroot (Jean Boussier) 8 months ago
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
Updated by Eregon (Benoit Daloze) 8 months ago · Edited
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.
Updated by byroot (Jean Boussier) 8 months ago
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.
Updated by Eregon (Benoit Daloze) 8 months ago
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).
Updated by byroot (Jean Boussier) 8 months ago
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.
Updated by ko1 (Koichi Sasada) 8 months ago
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.
Updated by akr (Akira Tanaka) 8 months ago
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.
Updated by mame (Yusuke Endoh) 8 months ago
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) }
After one delegation, the block is delegated to fetch_value
method.
def fetch_value(name, &block)
self[name].value(&block)
end
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
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
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.
Updated by jeremyevans0 (Jeremy Evans) 8 months ago
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.
Updated by Eregon (Benoit Daloze) 8 months ago
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.
Updated by Dan0042 (Daniel DeLorme) 8 months ago · Edited
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 becauseblock_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.
Updated by ko1 (Koichi Sasada) 8 months ago
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?
Updated by byroot (Jean Boussier) 8 months ago
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.
Updated by byroot (Jean Boussier) 8 months ago
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.
Updated by Dan0042 (Daniel DeLorme) 8 months ago
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
Updated by byroot (Jean Boussier) 8 months ago
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.
Updated by Dan0042 (Daniel DeLorme) 8 months ago
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?
Updated by byroot (Jean Boussier) 8 months ago
It doesn't matter where perform_later
is invoked. perform_later
do accept a block and will yield to it if provided.
Updated by Dan0042 (Daniel DeLorme) 8 months ago
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?
Updated by Eregon (Benoit Daloze) 8 months ago
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).
Updated by matthewd (Matthew Draper) 8 months ago
Eregon (Benoit Daloze) wrote in #note-40:
With the example from @mame (Yusuke Endoh), I think adding
(&)
or(&on_missing)
forAttribute::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/**)
forAttribute::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].
Updated by byroot (Jean Boussier) 8 months ago
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.
Updated by byroot (Jean Boussier) 7 months ago
@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".
Updated by ko1 (Koichi Sasada) 7 months ago
Thank you, could you compare the relax results on that? or all of your results are false positive?
Updated by byroot (Jean Boussier) 7 months ago
could you compare the relax results on that? or all of your results are false positive?
Yes. It's all false positive.
Updated by Dan0042 (Daniel DeLorme) 7 months ago · Edited
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¢
Updated by byroot (Jean Boussier) 7 months ago
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.
Updated by ko1 (Koichi Sasada) 7 months ago
- 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).
Updated by nobu (Nobuyoshi Nakada) 7 months ago
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'
Updated by Eregon (Benoit Daloze) 7 months ago
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?
Updated by Anonymous 7 months ago
--warn-unused-block
/:warn_unused_block
?
Updated by ko1 (Koichi Sasada) 3 months ago · Edited
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
)
Updated by byroot (Jean Boussier) 3 months ago
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.
Updated by Dan0042 (Daniel DeLorme) 3 months ago · Edited
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?
Updated by matz (Yukihiro Matsumoto) about 2 months ago
I don' like the term "pedantic" either. How about "strict"? I mean --warn-strict-unused-block
or Waning[:strict_unused_block]
.
Matz.
Updated by byroot (Jean Boussier) about 2 months ago
"strict" works I suppose. What we really want to convey is that this group of warnings is more likely to have false positives.
Updated by Dan0042 (Daniel DeLorme) about 2 months ago
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]
?
Updated by Eregon (Benoit Daloze) about 2 months ago
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.
Updated by byroot (Jean Boussier) about 2 months ago
I thought we were looking for a generic group name that can be used for other future warnings that may have false positives.
Updated by Eregon (Benoit Daloze) about 2 months ago
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.
Updated by Dan0042 (Daniel DeLorme) about 2 months ago · Edited
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.)
Updated by ko1 (Koichi Sasada) about 2 months ago
In this ticket, Warning[:strict_unused_block]
is accepted by Matz.
We should discuss :strict
category in different tickets.
Updated by ko1 (Koichi Sasada) 27 days ago
- 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]
Updated by Dan0042 (Daniel DeLorme) 25 days ago
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?