From 31b2f6127a6ce7bd6641c5a7967a1a9ef6885fa8 Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Fri, 5 Aug 2022 13:30:19 -0400 Subject: [PATCH] [Fix #10648] Allow `Style/TernaryParentheses` to take priority over `Style/RedundantParentheses`. `Style/TernaryParentheses` allows RuboCop to require ternary conditions are wrapped in parentheses, but this behaviour doesn't take effect when `Style/RedundantParentheses` is also enabled, in order to prevent an infinite loop. However, it makes more sense for `TernaryParentheses` to take priority here and have `RedudantParentheses` allow the parentheses instead of creating an infinite autocorrection loop. This change moves the infinite loop protection into `RedundantParentheses` instead. --- ...e_allow_styleternaryparentheses_to_take.md | 1 + .../cop/style/redundant_parentheses.rb | 16 ++++- lib/rubocop/cop/style/ternary_parentheses.rb | 14 +--- spec/rubocop/cli/autocorrect_spec.rb | 26 +++++++ .../cop/style/redundant_parentheses_spec.rb | 69 ++++++++++++++++--- .../cop/style/ternary_parentheses_spec.rb | 25 ------- 6 files changed, 101 insertions(+), 50 deletions(-) create mode 100644 changelog/change_allow_styleternaryparentheses_to_take.md diff --git a/changelog/change_allow_styleternaryparentheses_to_take.md b/changelog/change_allow_styleternaryparentheses_to_take.md new file mode 100644 index 00000000000..35ee5c328c7 --- /dev/null +++ b/changelog/change_allow_styleternaryparentheses_to_take.md @@ -0,0 +1 @@ +* [#10648](https://github.com/rubocop/rubocop/issues/10648): Allow `Style/TernaryParentheses` to take priority over `Style/RedundantParentheses` when parentheses are enforced. ([@dvandersluis][]) diff --git a/lib/rubocop/cop/style/redundant_parentheses.rb b/lib/rubocop/cop/style/redundant_parentheses.rb index e73dd570808..8551c69e0be 100644 --- a/lib/rubocop/cop/style/redundant_parentheses.rb +++ b/lib/rubocop/cop/style/redundant_parentheses.rb @@ -58,7 +58,8 @@ def allowed_expression?(node) allowed_ancestor?(node) || allowed_method_call?(node) || allowed_array_or_hash_element?(node) || - allowed_multiple_expression?(node) + allowed_multiple_expression?(node) || + allowed_ternary?(node) end def allowed_ancestor?(node) @@ -80,6 +81,19 @@ def allowed_multiple_expression?(node) !ancestor.begin_type? && !ancestor.def_type? && !ancestor.block_type? end + def allowed_ternary?(node) + return unless node&.parent&.if_type? + + node.parent.ternary? && ternary_parentheses_required? + end + + def ternary_parentheses_required? + config = @config.for_cop('Style/TernaryParentheses') + allowed_styles = %w[require_parentheses require_parentheses_when_complex] + + config.fetch('Enabled') && allowed_styles.include?(config['EnforcedStyle']) + end + def like_method_argument_parentheses?(node) node.send_type? && node.arguments.one? && !node.arithmetic_operation? && node.first_argument.begin_type? diff --git a/lib/rubocop/cop/style/ternary_parentheses.rb b/lib/rubocop/cop/style/ternary_parentheses.rb index 78ee224df36..b34cfd66185 100644 --- a/lib/rubocop/cop/style/ternary_parentheses.rb +++ b/lib/rubocop/cop/style/ternary_parentheses.rb @@ -71,7 +71,7 @@ def on_if(node) return if only_closing_parenthesis_is_last_line?(condition) return if condition_as_parenthesized_one_line_pattern_matching?(condition) - return unless node.ternary? && !infinite_loop? && offense?(node) + return unless node.ternary? && offense?(node) message = message(node) @@ -166,22 +166,10 @@ def require_parentheses_when_complex? style == :require_parentheses_when_complex end - def redundant_parentheses_enabled? - @config.for_cop('Style/RedundantParentheses').fetch('Enabled') - end - def parenthesized?(node) node.begin_type? end - # When this cop is configured to enforce parentheses and the - # `RedundantParentheses` cop is enabled, it will cause an infinite loop - # as they compete to add and remove the parentheses respectively. - def infinite_loop? - (require_parentheses? || require_parentheses_when_complex?) && - redundant_parentheses_enabled? - end - def unsafe_autocorrect?(condition) condition.children.any? do |child| unparenthesized_method_call?(child) || below_ternary_precedence?(child) diff --git a/spec/rubocop/cli/autocorrect_spec.rb b/spec/rubocop/cli/autocorrect_spec.rb index 129286a86ad..e415b196817 100644 --- a/spec/rubocop/cli/autocorrect_spec.rb +++ b/spec/rubocop/cli/autocorrect_spec.rb @@ -2640,4 +2640,30 @@ module Foo#{trailing_whitespace} ] RUBY end + + it 'properly autocorrects when `Style/TernaryParentheses` requires parentheses ' \ + 'that `Style/RedundantParentheses` would otherwise remove' do + source_file = Pathname('example.rb') + create_file(source_file, <<~RUBY) + foo ? bar : baz + RUBY + + create_file('.rubocop.yml', <<~YAML) + Style/TernaryParentheses: + EnforcedStyle: require_parentheses + Style/RedundantParentheses: + Enabled: true + YAML + + status = cli.run( + %w[--autocorrect-all --only] << %w[ + Style/TernaryParentheses + Style/RedundantParentheses + ].join(',') + ) + expect(status).to eq(0) + expect(source_file.read).to eq(<<~RUBY) + (foo) ? bar : baz + RUBY + end end diff --git a/spec/rubocop/cop/style/redundant_parentheses_spec.rb b/spec/rubocop/cop/style/redundant_parentheses_spec.rb index 98606dc489b..a0827e41b91 100644 --- a/spec/rubocop/cop/style/redundant_parentheses_spec.rb +++ b/spec/rubocop/cop/style/redundant_parentheses_spec.rb @@ -57,18 +57,65 @@ it_behaves_like 'redundant', '(retry)', 'retry', 'a keyword' it_behaves_like 'redundant', '(self)', 'self', 'a keyword' - it 'registers an offense for parens around constant ternary condition' do - expect_offense(<<~RUBY) - (X) ? Y : N - ^^^ Don't use parentheses around a constant. - (X)? Y : N - ^^^ Don't use parentheses around a constant. - RUBY + context 'ternaries' do + let(:other_cops) do + { + 'Style/TernaryParentheses' => { + 'Enabled' => ternary_parentheses_enabled, + 'EnforcedStyle' => ternary_parentheses_enforced_style + } + } + end + let(:ternary_parentheses_enabled) { true } + let(:ternary_parentheses_enforced_style) { nil } + + context 'when Style/TernaryParentheses is not enabled' do + let(:ternary_parentheses_enabled) { false } + + it 'registers an offense for parens around constant ternary condition' do + expect_offense(<<~RUBY) + (X) ? Y : N + ^^^ Don't use parentheses around a constant. + (X)? Y : N + ^^^ Don't use parentheses around a constant. + RUBY + + expect_correction(<<~RUBY) + X ? Y : N + X ? Y : N + RUBY + end + end - expect_correction(<<~RUBY) - X ? Y : N - X ? Y : N - RUBY + context 'when Style/TernaryParentheses has EnforcedStyle: require_no_parentheses' do + let(:ternary_parentheses_enforced_style) { 'require_no_parentheses' } + + it 'registers an offense for parens around ternary condition' do + expect_offense(<<~RUBY) + (X) ? Y : N + ^^^ Don't use parentheses around a constant. + (X)? Y : N + ^^^ Don't use parentheses around a constant. + RUBY + + expect_correction(<<~RUBY) + X ? Y : N + X ? Y : N + RUBY + end + end + + context 'when Style/TernaryParentheses has EnforcedStyle: require_parentheses' do + let(:ternary_parentheses_enforced_style) { 'require_parentheses' } + + it_behaves_like 'plausible', '(X) ? Y : N' + end + + context 'when Style/TernaryParentheses has EnforcedStyle: require_parentheses_when_complex' do + let(:ternary_parentheses_enforced_style) { 'require_parentheses_when_complex' } + + it_behaves_like 'plausible', '(X) ? Y : N' + end end it_behaves_like 'keyword with return value', 'break' diff --git a/spec/rubocop/cop/style/ternary_parentheses_spec.rb b/spec/rubocop/cop/style/ternary_parentheses_spec.rb index 999cc37e443..0dda1e8971f 100644 --- a/spec/rubocop/cop/style/ternary_parentheses_spec.rb +++ b/spec/rubocop/cop/style/ternary_parentheses_spec.rb @@ -2,11 +2,6 @@ RSpec.describe RuboCop::Cop::Style::TernaryParentheses, :config do let(:redundant_parens_enabled) { false } - let(:other_cops) do - { - 'Style/RedundantParentheses' => { 'Enabled' => redundant_parens_enabled } - } - end shared_examples 'safe assignment disabled' do |style, message| let(:cop_config) { { 'EnforcedStyle' => style, 'AllowSafeAssignment' => false } } @@ -898,24 +893,4 @@ end end end - - context 'when `RedundantParenthesis` would cause an infinite loop' do - let(:redundant_parens_enabled) { true } - - context 'when `EnforcedStyle: require_parentheses`' do - let(:cop_config) { { 'EnforcedStyle' => 'require_parentheses' } } - - it 'accepts' do - expect_no_offenses('foo = bar? ? a : b') - end - end - - context 'when `EnforcedStyle: require_parentheses_when_complex`' do - let(:cop_config) { { 'EnforcedStyle' => 'require_parentheses_when_complex' } } - - it 'accepts' do - expect_no_offenses('!condition.nil? ? foo : bar') - end - end - end end