From ea7f73f17b2bbcd6dc9344c9ab5940f92f87312e Mon Sep 17 00:00:00 2001 From: nobuyo Date: Sat, 7 May 2022 23:47:23 +0900 Subject: [PATCH] [Fix #10607] Fix autocorrect for `Style/RedundantCondition` when there are parenthesized method calls in each branch --- ...ondition_with_parenthesized_method_call.md | 1 + lib/rubocop/cop/style/redundant_condition.rb | 29 ++++++++++-- .../cop/style/redundant_condition_spec.rb | 47 +++++++++++++++++++ 3 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 changelog/fix_autocorrect_for_style_redundant_condition_with_parenthesized_method_call.md diff --git a/changelog/fix_autocorrect_for_style_redundant_condition_with_parenthesized_method_call.md b/changelog/fix_autocorrect_for_style_redundant_condition_with_parenthesized_method_call.md new file mode 100644 index 00000000000..816ca81f835 --- /dev/null +++ b/changelog/fix_autocorrect_for_style_redundant_condition_with_parenthesized_method_call.md @@ -0,0 +1 @@ +* [#10607](https://github.com/rubocop/rubocop/issues/10607): Fix autocorrect for `Style/RedundantCondition` when there are parenthesized method calls in each branch. ([@nobuyo][]) diff --git a/lib/rubocop/cop/style/redundant_condition.rb b/lib/rubocop/cop/style/redundant_condition.rb index a3ca00156cd..33f1a1cec30 100644 --- a/lib/rubocop/cop/style/redundant_condition.rb +++ b/lib/rubocop/cop/style/redundant_condition.rb @@ -44,9 +44,9 @@ def on_if(node) message = message(node) add_offense(range_of_offense(node), message: message) do |corrector| - if node.ternary? + if node.ternary? && !branches_have_method?(node) correct_ternary(corrector, node) - elsif node.modifier_form? || !node.else_branch + elsif redudant_condition?(node) corrector.replace(node, node.if_branch.source) else corrected = make_ternary_form(node) @@ -59,7 +59,7 @@ def on_if(node) private def message(node) - if node.modifier_form? || !node.else_branch + if redudant_condition?(node) REDUNDANT_CONDITION else MSG @@ -68,6 +68,7 @@ def message(node) def range_of_offense(node) return node.loc.expression unless node.ternary? + return node.loc.expression if node.ternary? && branches_have_method?(node) range_between(node.loc.question.begin_pos, node.loc.colon.end_pos) end @@ -81,6 +82,10 @@ def offense?(node) (node.ternary? || !else_branch.instance_of?(AST::Node) || else_branch.single_line?) end + def redudant_condition?(node) + node.modifier_form? || !node.else_branch + end + def use_if_branch?(else_branch) else_branch&.if_type? end @@ -89,6 +94,10 @@ def use_hash_key_assignment?(else_branch) else_branch&.send_type? && else_branch&.method?(:[]=) end + def use_hash_key_access?(node) + node.send_type? && node.method?(:[]) + end + def synonymous_condition_and_branch?(node) condition, if_branch, _else_branch = *node # e.g. @@ -113,7 +122,8 @@ def synonymous_condition_and_branch?(node) # else # test.value = another_value? # end - branches_have_method?(node) && condition == if_branch.first_argument + branches_have_method?(node) && condition == if_branch.first_argument && + !use_hash_key_access?(if_branch) end def branches_have_assignment?(node) @@ -140,6 +150,14 @@ def branches_have_method?(node) if_branch.method?(else_branch.method_name) end + def if_source(if_branch) + if branches_have_method?(if_branch.parent) && if_branch.parenthesized? + if_branch.source.delete_suffix(')') + else + if_branch.source + end + end + def else_source(else_branch) if branches_have_method?(else_branch.parent) else_source_if_has_method(else_branch) @@ -176,7 +194,8 @@ def else_source_if_has_assignment(else_branch) def make_ternary_form(node) _condition, if_branch, else_branch = *node - ternary_form = [if_branch.source, else_source(else_branch)].join(' || ') + ternary_form = [if_source(if_branch), else_source(else_branch)].join(' || ') + ternary_form += ')' if branches_have_method?(node) && if_branch.parenthesized? if node.parent&.send_type? "(#{ternary_form})" diff --git a/spec/rubocop/cop/style/redundant_condition_spec.rb b/spec/rubocop/cop/style/redundant_condition_spec.rb index 88332d42e69..4a802ec72f4 100644 --- a/spec/rubocop/cop/style/redundant_condition_spec.rb +++ b/spec/rubocop/cop/style/redundant_condition_spec.rb @@ -322,6 +322,21 @@ def test RUBY end + it 'registers an offense and corrects when the branches contains parenthesized method call' do + expect_offense(<<~RUBY) + if foo + ^^^^^^ Use double pipes `||` instead. + bar(foo) + else + bar(1..2) + end + RUBY + + expect_correction(<<~RUBY) + bar(foo || (1..2)) + RUBY + end + it 'does not register offenses when using `nil?` and the branches contains assignment' do expect_no_offenses(<<~RUBY) if foo.nil? @@ -351,6 +366,16 @@ def test end RUBY end + + it 'does not register offenses when the branches contains hash key access' do + expect_no_offenses(<<~RUBY) + if foo + bar[foo] + else + bar[1] + end + RUBY + end end end @@ -426,6 +451,28 @@ def test RUBY end + it 'registers an offense and corrects with ternary expression and the branches contains parenthesized method call' do + expect_offense(<<~RUBY) + foo ? bar(foo) : bar(quux) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Use double pipes `||` instead. + RUBY + + expect_correction(<<~RUBY) + bar(foo || quux) + RUBY + end + + it 'registers an offense and corrects with ternary expression and the branches contains chained parenthesized method call' do + expect_offense(<<~RUBY) + foo ? foo(foo).bar(foo) : foo(foo).bar(quux) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use double pipes `||` instead. + RUBY + + expect_correction(<<~RUBY) + foo(foo).bar(foo || quux) + RUBY + end + it 'registers an offense and corrects when the else branch contains `rescue`' do expect_offense(<<~RUBY) if a