Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix double negative conditional bug in NegatedIf and NegatedWhile style rules #3146

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* [#3125](https://github.com/bbatsov/rubocop/issues/3125): Fix `Rails/UniqBeforePluck` to ignore `uniq` with block. ([@tejasbubane][])
* [#3116](https://github.com/bbatsov/rubocop/issues/3116): `Style/SpaceAroundKeyword` allows `&.` method calls after `super` and `yield`. ([@segiddins][])
* [#3131](https://github.com/bbatsov/rubocop/issues/3131): Fix `Style/ZeroLengthPredicate` to ignore `size` and `length` variables. ([@tejasbubane][])
* [#3146](https://github.com/bbatsov/rubocop/pull/3146): Fix `NegatedIf` and `NegatedWhile` to ignore double negations. ([@natalzia-paperless][])

## 0.40.0 (2016-05-09)

Expand Down Expand Up @@ -2168,3 +2169,4 @@
[@tjwp]: https://github.com/tjwp
[@neodelf]: https://github.com/neodelf
[@josh]: https://github.com/josh
[@natalzia-paperless]: https://github.com/natalzia-paperless
10 changes: 6 additions & 4 deletions lib/rubocop/cop/mixin/negative_conditional.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,19 @@ module Cop
# Some common code shared between FavorUnlessOverNegatedIf and
# FavorUntilOverNegatedWhile.
module NegativeConditional
def self.included(mod)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using the included hook really needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a mixin it doesn't have access to def_node_matcher which is used as an easy way to test the conditional for a double negation. Without it we would need to drill into the second level of the condition here which seems like a worse option to me

mod.def_node_matcher :single_negative?, '(send !(send _ :!) :!)'
end

def check_negative_conditional(node)
condition, _body, _rest = *node

# Look at last expression of contents if there are parentheses
# around condition.
condition = condition.children.last while condition.type == :begin
return unless condition.type == :send

_object, method = *condition
return unless method == :! && !(node.loc.respond_to?(:else) &&
node.loc.else)
return unless single_negative?(condition) &&
!(node.loc.respond_to?(:else) && node.loc.else)

add_offense(node, :expression)
end
Expand Down
9 changes: 9 additions & 0 deletions spec/rubocop/cop/style/negated_if_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@
expect(cop.offenses).to be_empty
end

it 'accepts an if where the condition is doubly negated' do
inspect_source(cop,
['if !!condition',
' some_method',
'end',
'some_method if !!condition'])
expect(cop.offenses).to be_empty
end

it 'is not confused by negated elsif' do
inspect_source(cop,
['if test.is_a?(String)',
Expand Down
11 changes: 10 additions & 1 deletion spec/rubocop/cop/style/negated_while_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
expect(cop.offenses.map(&:line)).to eq([1, 4])
end

it 'accepts an while where only part of the condition is negated' do
it 'accepts a while where only part of the condition is negated' do
inspect_source(cop,
['while !a_condition && another_condition',
' some_method',
Expand All @@ -51,6 +51,15 @@
expect(cop.messages).to be_empty
end

it 'accepts a while where the condition is doubly negated' do
inspect_source(cop,
['while !!a_condition',
' some_method',
'end',
'some_method while !!a_condition'])
expect(cop.messages).to be_empty
end

it 'autocorrects by replacing while not with until' do
corrected = autocorrect_source(cop, ['something while !x.even?',
'something while(!x.even?)'])
Expand Down