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 #5388] Add new Style/UnlessLogicalOperators
cop.
#9386
Merged
bbatsov
merged 1 commit into
rubocop:master
from
caalberts:cop-style-unless-mixed-conditions
Feb 23, 2021
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
* [#5388](https://github.com/rubocop-hq/rubocop/issues/5388): Add new `Style/UnlessLogicalOperators` cop. ([@caalberts][]) |
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
module Style | ||
# This cop checks for the use of logical operators in an `unless` condition. | ||
# It discourages such code, as the condition becomes more difficult | ||
# to read and understand. | ||
# | ||
# This cop supports two styles: | ||
# - `forbid_mixed_logical_operators` (default) | ||
# - `forbid_logical_operators` | ||
# | ||
# `forbid_mixed_logical_operators` style forbids the use of more than one type | ||
# of logical operators. This makes the `unless` condition easier to read | ||
# because either all conditions need to be met or any condition need to be met | ||
# in order for the expression to be truthy or falsey. | ||
# | ||
# `forbid_logical_operators` style forbids any use of logical operator. | ||
# This makes it even more easy to read the `unless` condition as | ||
# there is only one condition in the expression. | ||
# | ||
# @example EnforcedStyle: forbid_mixed_logical_operators (default) | ||
# # bad | ||
# return unless a || b && c | ||
# return unless a && b || c | ||
# return unless a && b and c | ||
# return unless a || b or c | ||
# return unless a && b or c | ||
# return unless a || b and c | ||
# | ||
# # good | ||
# return unless a && b && c | ||
# return unless a || b || c | ||
# return unless a and b and c | ||
# return unless a or b or c | ||
# return unless a? | ||
# | ||
# @example EnforcedStyle: forbid_logical_operators | ||
# # bad | ||
# return unless a || b | ||
# return unless a && b | ||
# return unless a or b | ||
# return unless a and b | ||
# | ||
# # good | ||
# return unless a | ||
# return unless a? | ||
class UnlessLogicalOperators < Base | ||
include ConfigurableEnforcedStyle | ||
|
||
FORBID_MIXED_LOGICAL_OPERATORS = 'Do not use mixed logical operators in an `unless`.' | ||
FORBID_LOGICAL_OPERATORS = 'Do not use any logical operator in an `unless`.' | ||
|
||
def_node_matcher :or_with_and?, <<~PATTERN | ||
(if (or <`and ...> ) ...) | ||
PATTERN | ||
def_node_matcher :and_with_or?, <<~PATTERN | ||
(if (and <`or ...> ) ...) | ||
PATTERN | ||
def_node_matcher :logical_operator?, <<~PATTERN | ||
(if ({and or} ... ) ...) | ||
PATTERN | ||
|
||
def on_if(node) | ||
return unless node.unless? | ||
|
||
if style == :forbid_mixed_logical_operators && mixed_logical_operator?(node) | ||
add_offense(node, message: FORBID_MIXED_LOGICAL_OPERATORS) | ||
elsif style == :forbid_logical_operators && logical_operator?(node) | ||
add_offense(node, message: FORBID_LOGICAL_OPERATORS) | ||
end | ||
end | ||
|
||
private | ||
|
||
def mixed_logical_operator?(node) | ||
or_with_and?(node) || | ||
and_with_or?(node) || | ||
mixed_precedence_and?(node) || | ||
mixed_precedence_or?(node) | ||
end | ||
|
||
def mixed_precedence_and?(node) | ||
node.source.include?('&&') && node.source.include?('and') | ||
end | ||
|
||
def mixed_precedence_or?(node) | ||
node.source.include?('||') && node.source.include?('or') | ||
end | ||
end | ||
end | ||
end | ||
end |
181 changes: 181 additions & 0 deletions
181
spec/rubocop/cop/style/unless_logical_operators_spec.rb
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,181 @@ | ||
# frozen_string_literal: true | ||
|
||
RSpec.describe RuboCop::Cop::Style::UnlessLogicalOperators, :config do | ||
context 'EnforcedStyle is `forbid_mixed_logical_operators`' do | ||
let(:cop_config) do | ||
{ 'EnforcedStyle' => 'forbid_mixed_logical_operators' } | ||
end | ||
|
||
it 'registers an offense when using `&&` and `||`' do | ||
expect_offense(<<~RUBY) | ||
return unless a && b || c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
|
||
expect_offense(<<~RUBY) | ||
return unless a || b && c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when using `&&` and `and`' do | ||
expect_offense(<<~RUBY) | ||
return unless a && b and c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
|
||
expect_offense(<<~RUBY) | ||
return unless a and b && c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when using `&&` and `or`' do | ||
expect_offense(<<~RUBY) | ||
return unless a && b or c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
|
||
expect_offense(<<~RUBY) | ||
return unless a or b && c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when using `||` and `or`' do | ||
expect_offense(<<~RUBY) | ||
return unless a || b or c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
|
||
expect_offense(<<~RUBY) | ||
return unless a or b || c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when using `||` and `and`' do | ||
expect_offense(<<~RUBY) | ||
return unless a || b and c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
|
||
expect_offense(<<~RUBY) | ||
return unless a and b || c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when using parentheses' do | ||
expect_offense(<<~RUBY) | ||
return unless a || (b && c) || d | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use mixed logical operators in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when using only `&&`s' do | ||
expect_no_offenses(<<~RUBY) | ||
return unless a && b && c | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when using only `||`s' do | ||
expect_no_offenses(<<~RUBY) | ||
return unless a || b || c | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when using only `and`s' do | ||
expect_no_offenses(<<~RUBY) | ||
return unless a and b and c | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when using only `or`s' do | ||
expect_no_offenses(<<~RUBY) | ||
return unless a or b or c | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when using if' do | ||
expect_no_offenses(<<~RUBY) | ||
return if a || b && c || d | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when not used in unless' do | ||
expect_no_offenses(<<~RUBY) | ||
def condition? | ||
a or b && c || d | ||
end | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when not using logical operator' do | ||
expect_no_offenses(<<~RUBY) | ||
return unless a? | ||
RUBY | ||
end | ||
end | ||
|
||
context 'EnforcedStyle is `forbid_logical_operators`' do | ||
let(:cop_config) do | ||
{ 'EnforcedStyle' => 'forbid_logical_operators' } | ||
end | ||
|
||
it 'registers an offense when using only `&&`' do | ||
expect_offense(<<~RUBY) | ||
return unless a && b | ||
^^^^^^^^^^^^^^^^^^^^ Do not use any logical operator in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when using only `||`' do | ||
expect_offense(<<~RUBY) | ||
return unless a || b | ||
^^^^^^^^^^^^^^^^^^^^ Do not use any logical operator in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when using only `and`' do | ||
expect_offense(<<~RUBY) | ||
return unless a and b | ||
^^^^^^^^^^^^^^^^^^^^^ Do not use any logical operator in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when using only `or`' do | ||
expect_offense(<<~RUBY) | ||
return unless a or b | ||
^^^^^^^^^^^^^^^^^^^^ Do not use any logical operator in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when using `&&` followed by ||' do | ||
expect_offense(<<~RUBY) | ||
return unless a && b || c | ||
^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use any logical operator in an `unless`. | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when using if' do | ||
expect_no_offenses(<<~RUBY) | ||
return if a || b | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when not used in unless' do | ||
expect_no_offenses(<<~RUBY) | ||
def condition? | ||
a || b | ||
end | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when not using logical operator' do | ||
expect_no_offenses(<<~RUBY) | ||
return unless a? | ||
RUBY | ||
end | ||
end | ||
end |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there is a better way to distinguish
a && b && c
anda && b and c
.Using node matcher doesn't work because both have the following AST:
Same goes for
a || b || c
anda || b or c
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered using a node matcher for
a and b && c
, using(if (and ... (and ...)) ...)
to match the following AST:However, if we need to check the
node.source
anyway, then it's unnecessary to have the node matcher.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's ok. Technically you don't need to use
include?
, though as you can do an equality check instead (and I guess it'd be faster).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you share an example how we can do an equality check on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just a regular string? How about
==
? :-)