Merge pull request #3703 from swcraig/third-option-ternary-parens

[Fix #3451] Add new `require_parentheses_when_complex` style to `Styl…

CHANGELOG.md

@@ -10,6 +10,8 @@
 * `Style/EmptyLiteral` will now auto-correct `Hash.new` when it is the first argument being passed to a method. The arguments will be wrapped with parenthesis. ([@rrosenblum][])
 * [#3713](https://github.com/bbatsov/rubocop/pull/3713): Respect `DisabledByDefault` in parent configs. ([@aroben][])
 * New cop `Rails/EnumUniqueness` checks for duplicate values defined in enum config. ([@olliebennett][])
+* New cop `Rails/EnumUniqueness` checks for duplicate values defined in enum config hash. ([@olliebennett][])
+* [#3451](https://github.com/bbatsov/rubocop/issues/3451): Add new `require_parentheses_when_complex` style to `Style/TernaryParentheses` cop. ([@swcraig][])
 
 ### Changes
 

config/default.yml

@@ -1006,6 +1006,7 @@ Style/TernaryParentheses:
   SupportedStyles:
     - require_parentheses
     - require_no_parentheses
+    - require_parentheses_when_complex
   AllowSafeAssignment: true
 
 Style/TrailingBlankLines:

lib/rubocop/cop/style/ternary_parentheses.rb

@@ -34,57 +34,107 @@ module Style
       #   foo = (bar?) ? a : b
       #   foo = (bar.baz) ? a : b
       #   foo = (bar && baz) ? a : b
+      #
+      # @example
+      #
+      #   EnforcedStyle: require_parentheses_when_complex
+      #
+      #   @bad
+      #   foo = (bar?) ? a : b
+      #   foo = (bar.baz?) ? a : b
+      #   foo = bar && baz ? a : b
+      #
+      #   @good
+      #   foo = bar? ? a : b
+      #   foo = bar.baz ? a : b
+      #   foo = (bar && baz) ? a : b
       class TernaryParentheses < Cop
         include IfNode
         include SafeAssignment
         include ConfigurableEnforcedStyle
 
         MSG = '%s parentheses for ternary conditions.'.freeze
+        MSG_COMPLEX = '%s parentheses for ternary expressions with' \
+          ' complex conditions.'.freeze
 
         def on_if(node)
-          return unless ternary?(node)
-
-          add_offense(node, node.source_range, message) if offense?(node)
+          return unless ternary?(node) && !infinite_loop? && offense?(node)
+          add_offense(node, node.source_range, message(node))
         end
 
         private
 
         def offense?(node)
           condition, = *node
 
-          (require_parentheses? && !parenthesized?(condition) ||
-            !require_parentheses? && parenthesized?(condition)) &&
-            !(safe_assignment?(condition) && safe_assignment_allowed?) &&
-            !infinite_loop?
+          if safe_assignment?(condition)
+            !safe_assignment_allowed?
+          else
+            parens = parenthesized?(condition)
+            case style
+            when :require_parentheses_when_complex
+              complex_condition?(condition) ? !parens : parens
+            else
+              require_parentheses? ? !parens : parens
+            end
+          end
         end
 
         def autocorrect(node)
           condition, = *node
 
-          return nil if !require_parentheses? && (safe_assignment?(condition) ||
+          return nil if parenthesized?(condition) &&
+                        (safe_assignment?(condition) ||
                         unsafe_autocorrect?(condition))
 
           lambda do |corrector|
-            if require_parentheses?
-              corrector.insert_before(condition.source_range, '(')
-              corrector.insert_after(condition.source_range, ')')
-            else
+            if parenthesized?(condition)
               corrector.remove(condition.loc.begin)
               corrector.remove(condition.loc.end)
+            else
+              corrector.insert_before(condition.source_range, '(')
+              corrector.insert_after(condition.source_range, ')')
             end
           end
         end
 
-        def message
-          verb = require_parentheses? ? 'Use' : 'Omit'
+        # If the condition is parenthesized we recurse and check for any
+        # complex expressions within it.
+        def complex_condition?(condition)
+          if condition.type == :begin
+            condition.to_a.any? { |x| complex_condition?(x) }
+          else
+            non_complex_type?(condition) ? false : true
+          end
+        end
+
+        # Anything that is not a variable, constant, or method/.method call
+        # will be counted as a complex expression.
+        def non_complex_type?(condition)
+          condition.variable? || condition.const_type? ||
+            (condition.send_type? && !operator?(condition.method_name)) ||
+            condition.defined_type?
+        end
 
-          format(MSG, verb)
+        def message(node)
+          if require_parentheses_when_complex?
+            condition, = *node
+            omit = parenthesized?(condition) ? 'Only use' : 'Use'
+            format(MSG_COMPLEX, omit)
+          else
+            verb = require_parentheses? ? 'Use' : 'Omit'
+            format(MSG, verb)
+          end
         end
 
         def require_parentheses?
           style == :require_parentheses
         end
 
+        def require_parentheses_when_complex?
+          style == :require_parentheses_when_complex
+        end
+
         def redundant_parentheses_enabled?
           @config.for_cop('RedundantParentheses')['Enabled']
         end
@@ -109,7 +159,6 @@ def unsafe_autocorrect?(condition)
 
         def unparenthesized_method_call?(child)
           argument = method_call_argument(child)
-
           argument && argument !~ /^\(/
         end
 

manual/cops_style.md

@@ -4515,13 +4515,26 @@ foo = (bar?) ? a : b
 foo = (bar.baz) ? a : b
 foo = (bar && baz) ? a : b
 ```
+```ruby
+EnforcedStyle: require_parentheses_when_complex
+
+# bad
+foo = (bar?) ? a : b
+foo = (bar.baz?) ? a : b
+foo = bar && baz ? a : b
+
+# good
+foo = bar? ? a : b
+foo = bar.baz ? a : b
+foo = (bar && baz) ? a : b
+```
 
 ### Important attributes
 
 Attribute | Value
 --- | ---
 EnforcedStyle | require_no_parentheses
-SupportedStyles | require_parentheses, require_no_parentheses
+SupportedStyles | require_parentheses, require_no_parentheses, require_parentheses_when_complex
 AllowSafeAssignment | true
 
 

spec/rubocop/cop/style/ternary_parentheses_spec.rb

@@ -48,6 +48,24 @@
     end
   end
 
+  shared_examples 'safe assignment disabled' do |style|
+    let(:cop_config) do
+      {
+        'EnforcedStyle' => style,
+        'AllowSafeAssignment' => false
+      }
+    end
+
+    it_behaves_like 'code with offense',
+                    'foo = (bar = find_bar) ? a : b'
+
+    it_behaves_like 'code with offense',
+                    'foo = bar = (baz = find_baz) ? a : b'
+
+    it_behaves_like 'code with offense',
+                    'foo = (bar = baz = find_baz) ? a : b'
+  end
+
   context 'when configured to enforce parentheses inclusion' do
     let(:cop_config) { { 'EnforcedStyle' => 'require_parentheses' } }
 
@@ -144,23 +162,7 @@
       it_behaves_like 'code without offense',
                       'foo = (bar = baz = find_baz) ? a : b'
 
-      context 'when safe assignment is disabled' do
-        let(:cop_config) do
-          {
-            'EnforcedStyle' => 'require_no_parentheses',
-            'AllowSafeAssignment' => false
-          }
-        end
-
-        it_behaves_like 'code with offense',
-                        'foo = (bar = find_bar) ? a : b'
-
-        it_behaves_like 'code with offense',
-                        'foo = bar = (baz = find_baz) ? a : b'
-
-        it_behaves_like 'code with offense',
-                        'foo = (bar = baz = find_baz) ? a : b'
-      end
+      it_behaves_like 'safe assignment disabled', 'require_no_parentheses'
     end
 
     context 'with an unparenthesized method call condition' do
@@ -187,6 +189,127 @@
     end
   end
 
+  context 'configured for parentheses on complex and there are parens' do
+    let(:cop_config) do
+      { 'EnforcedStyle' => 'require_parentheses_when_complex' }
+    end
+
+    let(:message) do
+      'Only use parentheses for ternary expressions with complex conditions.'
+    end
+
+    context 'with a simple condition' do
+      it_behaves_like 'code with offense',
+                      'foo = (bar?) ? a : b',
+                      'foo = bar? ? a : b'
+    end
+
+    context 'with a complex condition' do
+      it_behaves_like 'code with offense',
+                      'foo = (bar.baz?) ? a : b',
+                      'foo = bar.baz? ? a : b'
+
+      it_behaves_like 'code without offense',
+                      'foo = (bar && (baz || bar)) ? a : b'
+    end
+
+    context 'with an assignment condition' do
+      it_behaves_like 'code without offense',
+                      'foo = (bar = find_bar) ? a : b'
+
+      it_behaves_like 'code without offense',
+                      'foo = baz = (bar = find_bar) ? a : b'
+
+      it_behaves_like 'code without offense',
+                      'foo = bar = (bar == 1) ? a : b'
+
+      it_behaves_like 'code without offense',
+                      'foo = (bar = baz = find_bar) ? a : b'
+
+      it_behaves_like 'safe assignment disabled',
+                      'require_parentheses_when_complex'
+    end
+
+    context 'with method call condition' do
+      it_behaves_like 'code with offense',
+                      'foo = (defined? bar) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = (baz? bar) ? a : b'
+
+      context 'when calling method on a receiver' do
+        it_behaves_like 'code with offense',
+                        'foo = (baz.foo? bar) ? a : b'
+      end
+    end
+
+    context 'with condition including a range' do
+      it_behaves_like 'code without offense',
+                      '(foo..bar).include?(baz) ? a : b'
+    end
+  end
+
+  context 'configured for parentheses on complex and there are no parens' do
+    let(:cop_config) do
+      { 'EnforcedStyle' => 'require_parentheses_when_complex' }
+    end
+
+    let(:message) do
+      'Use parentheses for ternary expressions with complex conditions.'
+    end
+
+    context 'with complex condition' do
+      it_behaves_like 'code with offense',
+                      'foo = 1 + 1 == 2 ? a : b',
+                      'foo = (1 + 1 == 2) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = bar && baz ? a : b',
+                      'foo = (bar && baz) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = bar && baz || bar ? a : b',
+                      'foo = (bar && baz || bar) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = bar && (baz != bar) ? a : b',
+                      'foo = (bar && (baz != bar)) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = 1 < (bar.baz?) ? a : b',
+                      'foo = (1 < (bar.baz?)) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = 1 <= (bar ** baz) ? a : b',
+                      'foo = (1 <= (bar ** baz)) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = 1 >= bar * baz ? a : b',
+                      'foo = (1 >= bar * baz) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = bar + baz ? a : b',
+                      'foo = (bar + baz) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = bar - baz ? a : b',
+                      'foo = (bar - baz) ? a : b'
+
+      it_behaves_like 'code with offense',
+                      'foo = bar < baz ? a : b',
+                      'foo = (bar < baz) ? a : b'
+    end
+
+    context 'with an assignment condition' do
+      it_behaves_like 'code with offense',
+                      'foo = bar = baz == 1 ? a : b',
+                      'foo = bar = (baz == 1) ? a : b'
+
+      it_behaves_like 'code without offense',
+                      'foo = (bar = baz == 1) ? a : b'
+    end
+  end
+
   context 'when `RedundantParenthesis` would cause an infinite loop' do
     let(:config) do
       RuboCop::Config.new(