From 8322f3aa371d668fde2ff133d592ff654a56ea01 Mon Sep 17 00:00:00 2001 From: Albert Salim Date: Tue, 23 Feb 2021 19:32:16 +0800 Subject: [PATCH] [Fix #5388] Add new `Style/UnlessLogicalOperators` cop (#9386) --- ...add_new_styleunlesslogicaloperators_cop.md | 1 + config/default.yml | 10 + lib/rubocop.rb | 1 + .../cop/style/unless_logical_operators.rb | 94 +++++++++ .../style/unless_logical_operators_spec.rb | 181 ++++++++++++++++++ 5 files changed, 287 insertions(+) create mode 100644 changelog/new_add_new_styleunlesslogicaloperators_cop.md create mode 100644 lib/rubocop/cop/style/unless_logical_operators.rb create mode 100644 spec/rubocop/cop/style/unless_logical_operators_spec.rb diff --git a/changelog/new_add_new_styleunlesslogicaloperators_cop.md b/changelog/new_add_new_styleunlesslogicaloperators_cop.md new file mode 100644 index 00000000000..f375995adee --- /dev/null +++ b/changelog/new_add_new_styleunlesslogicaloperators_cop.md @@ -0,0 +1 @@ +* [#5388](https://github.com/rubocop-hq/rubocop/issues/5388): Add new `Style/UnlessLogicalOperators` cop. ([@caalberts][]) diff --git a/config/default.yml b/config/default.yml index 0d10b4a7184..1799e31e9d2 100644 --- a/config/default.yml +++ b/config/default.yml @@ -4740,6 +4740,16 @@ Style/UnlessElse: Enabled: true VersionAdded: '0.9' +Style/UnlessLogicalOperators: + Description: >- + Checks for use of logical operators in an unless condition. + Enabled: false + VersionAdded: '<>' + EnforcedStyle: forbid_mixed_logical_operators + SupportedStyles: + - forbid_mixed_logical_operators + - forbid_logical_operators + Style/UnpackFirst: Description: >- Checks for accessing the first element of `String#unpack` diff --git a/lib/rubocop.rb b/lib/rubocop.rb index c1891b089a1..c1f1deefec5 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -605,6 +605,7 @@ require_relative 'rubocop/cop/style/trailing_underscore_variable' require_relative 'rubocop/cop/style/trivial_accessors' require_relative 'rubocop/cop/style/unless_else' +require_relative 'rubocop/cop/style/unless_logical_operators' require_relative 'rubocop/cop/style/unpack_first' require_relative 'rubocop/cop/style/variable_interpolation' require_relative 'rubocop/cop/style/when_then' diff --git a/lib/rubocop/cop/style/unless_logical_operators.rb b/lib/rubocop/cop/style/unless_logical_operators.rb new file mode 100644 index 00000000000..59e81ccc3a9 --- /dev/null +++ b/lib/rubocop/cop/style/unless_logical_operators.rb @@ -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 diff --git a/spec/rubocop/cop/style/unless_logical_operators_spec.rb b/spec/rubocop/cop/style/unless_logical_operators_spec.rb new file mode 100644 index 00000000000..3a7c66480fd --- /dev/null +++ b/spec/rubocop/cop/style/unless_logical_operators_spec.rb @@ -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