[Fix #14486] Fix false positives for Style/RedundantParentheses with unary operators and yield
#14489
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
koic
reviewed
Aug 28, 2025
| it_behaves_like 'plausible', '(!x.m arg)' | ||
| it_behaves_like 'plausible', '(!x).y' | ||
| it_behaves_like 'plausible', '(!super arg)' | ||
| it_behaves_like 'plausible', '(!yield arg)' |
There was a problem hiding this comment.
A standalone (!yield x) should be detected. The false positive occurs with logical conditions such as x && (!yield y).
There was a problem hiding this comment.
Yes, that is true. But (!x.m arg) is currently also not an offense, so this is a different issue.
These only get picked up when the argument is also parenthesised like (!x.m(arg))
There was a problem hiding this comment.
Hmm, since (!yield arg) is currently being detected, it should be kept that way.
$ echo '(!yield arg)' | bundle exec rubocop --stdin example.rb -a --only Style/RedundantParentheses
Inspecting 1 file
C
Offenses:
example.rb:1:1: C: [Corrected] Style/RedundantParentheses: Don't use parentheses around a unary operation.
(!yield arg)
^^^^^^^^^^^^
1 file inspected, 1 offense detected, 1 offense corrected
====================
!yield argThere was a problem hiding this comment.
Ok, I'll change it. I have something that seems to work, give me a while to check that this doesn't introduce other unwanted changes.
There was a problem hiding this comment.
I preserved this behaviour. Since it also applies to method calls, I added an additional changelog entry for that and a few tests
|
The |
Earlopain
force-pushed
the
redundant-parens-yield-etc
branch
from
92ed884 to
5e1319e
Compare
|
My bad, fix'd |
Earlopain
changed the title
Style/RedundantParens with unary operators and yieldStyle/RedundantParentheses with unary operators and yield
Earlopain
force-pushed
the
redundant-parens-yield-etc
branch
from
5e1319e to
369f21a
Compare
Earlopain
commented
Aug 28, 2025
| it_behaves_like 'plausible', '+(1.foo)' | ||
| it_behaves_like 'plausible', '-(1.foo.bar)' | ||
| it_behaves_like 'plausible', '+(1.foo.bar)' | ||
| it_behaves_like 'plausible', 'foo(*(bar & baz))' |
There was a problem hiding this comment.
These were not covered by tests and would have suggested removing the parens (which is wrong, it's a splat
There was a problem hiding this comment.
BTW, I ran this change over real-world-rails and real-world-ruby and there are only a few new offenses (~60) and they all seem to make sense (apart from this one here, which I found through that).
Just mentioning since I found this to be a bit of a risky change overall with the change you requested but after checking I can say its rather safe (probably)
koic
reviewed
Aug 30, 2025
Comment on lines
212
to
231
| # No problem removing the parens when it is the only expression | ||
| it_behaves_like 'redundant', '(!x arg)', '!x arg', 'a unary operation' | ||
| it_behaves_like 'redundant', '(!x.m arg)', '!x.m arg', 'a unary operation' | ||
| it_behaves_like 'redundant', '(!super arg)', '!super arg', 'a unary operation' | ||
| it_behaves_like 'redundant', '(!yield arg)', '!yield arg', 'a unary operation' | ||
| it_behaves_like 'redundant', '(!defined? arg)', '!defined? arg', 'a unary operation' | ||
| # Removing the parens leads to semantic differences | ||
| it_behaves_like 'plausible', '(!x arg) && foo' | ||
| it_behaves_like 'plausible', '(!x.m arg) && foo' | ||
| it_behaves_like 'plausible', '(!super arg) && foo' | ||
| it_behaves_like 'plausible', '(!yield arg) && foo' | ||
| it_behaves_like 'plausible', '(!defined? arg) && foo' | ||
| # Removing the parens leads to a syntax error | ||
| it_behaves_like 'plausible', 'foo && (!x arg)' | ||
| it_behaves_like 'plausible', 'foo && (!x.m arg)' | ||
| it_behaves_like 'plausible', 'foo && (!super arg)' | ||
| it_behaves_like 'plausible', 'foo && (!yield arg)' | ||
| it_behaves_like 'plausible', 'foo && (!defined? arg)' |
There was a problem hiding this comment.
That's just my two cents.
Suggested change
| # No problem removing the parens when it is the only expression | |
| it_behaves_like 'redundant', '(!x arg)', '!x arg', 'a unary operation' | |
| it_behaves_like 'redundant', '(!x.m arg)', '!x.m arg', 'a unary operation' | |
| it_behaves_like 'redundant', '(!super arg)', '!super arg', 'a unary operation' | |
| it_behaves_like 'redundant', '(!yield arg)', '!yield arg', 'a unary operation' | |
| it_behaves_like 'redundant', '(!defined? arg)', '!defined? arg', 'a unary operation' | |
| # Removing the parens leads to semantic differences | |
| it_behaves_like 'plausible', '(!x arg) && foo' | |
| it_behaves_like 'plausible', '(!x.m arg) && foo' | |
| it_behaves_like 'plausible', '(!super arg) && foo' | |
| it_behaves_like 'plausible', '(!yield arg) && foo' | |
| it_behaves_like 'plausible', '(!defined? arg) && foo' | |
| # Removing the parens leads to a syntax error | |
| it_behaves_like 'plausible', 'foo && (!x arg)' | |
| it_behaves_like 'plausible', 'foo && (!x.m arg)' | |
| it_behaves_like 'plausible', 'foo && (!super arg)' | |
| it_behaves_like 'plausible', 'foo && (!yield arg)' | |
| it_behaves_like 'plausible', 'foo && (!defined? arg)' | |
| # No problem removing the parens when it is the only expression. | |
| it_behaves_like 'redundant', '(!x arg)', '!x arg', 'a unary operation' | |
| it_behaves_like 'redundant', '(!x.m arg)', '!x.m arg', 'a unary operation' | |
| it_behaves_like 'redundant', '(!super arg)', '!super arg', 'a unary operation' | |
| it_behaves_like 'redundant', '(!yield arg)', '!yield arg', 'a unary operation' | |
| it_behaves_like 'redundant', '(!defined? arg)', '!defined? arg', 'a unary operation' | |
| # Removing the parens leads to semantic differences. | |
| it_behaves_like 'plausible', '(!x arg) && foo' | |
| it_behaves_like 'plausible', '(!x.m arg) && foo' | |
| it_behaves_like 'plausible', '(!super arg) && foo' | |
| it_behaves_like 'plausible', '(!yield arg) && foo' | |
| it_behaves_like 'plausible', '(!defined? arg) && foo' | |
| # Removing the parens leads to a syntax error. | |
| it_behaves_like 'plausible', 'foo && (!x arg)' | |
| it_behaves_like 'plausible', 'foo && (!x.m arg)' | |
| it_behaves_like 'plausible', 'foo && (!super arg)' | |
| it_behaves_like 'plausible', 'foo && (!yield arg)' | |
| it_behaves_like 'plausible', 'foo && (!defined? arg)' |
…es` with unary operators and `yield`, `super`, or `defined?` Removing parens for `defined?` does not result in a syntax error but because of it's low precedence it's better to keep them anyways. For example, in `(defined? foo) && baz`, removing the parens makes it into a single `defined?` invocation. Since code like `(!yield arg)` should continue to register an offense, this also updates normal method calls to do the same. As such, I added a few more testcases for that (+ a changelog entry)
Earlopain
force-pushed
the
redundant-parens-yield-etc
branch
from
369f21a to
381374b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
… and also ,
super/defined?Fixes #14486.
Removing parens for
defined?does not result in a syntax error but because of it's low precedence it's better to keep them anyways. For example, in(defined? foo) && baz, removing the parens makes it into a singledefined?invocationBefore submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.