[Fix #10325] Enhance Style/RedundantCondition for the case that variable assignments in each branch

changelog/fix_enhance_style_redundantcondition.md

@@ -0,0 +1 @@
+* [#10325](https://github.com/rubocop/rubocop/issues/10325): Enhance `Style/RedundantCondition` by considering the case that variable assignments in each branch. ([@nobuyo][])

lib/rubocop/cop/style/redundant_condition.rb

@@ -73,13 +73,12 @@ def range_of_offense(node)
         end
 
         def offense?(node)
-          condition, if_branch, else_branch = *node
+          _condition, _if_branch, else_branch = *node
 
           return false if use_if_branch?(else_branch) || use_hash_key_assignment?(else_branch)
 
-          condition == if_branch && !node.elsif? && (
-            node.ternary? || !else_branch.instance_of?(AST::Node) || else_branch.single_line?
-          )
+          synonymous_condition_and_branch?(node) && !node.elsif? &&
+            (node.ternary? || !else_branch.instance_of?(AST::Node) || else_branch.single_line?)
         end
 
         def use_if_branch?(else_branch)
@@ -90,16 +89,87 @@ def use_hash_key_assignment?(else_branch)
           else_branch&.send_type? && else_branch&.method?(:[]=)
         end
 
+        def synonymous_condition_and_branch?(node)
+          condition, if_branch, _else_branch = *node
+          # e.g.
+          #   if var
+          #     var
+          #   else
+          #     'foo'
+          #   end
+          return true if condition == if_branch
+
+          # e.g.
+          #   if foo
+          #     @value = foo
+          #   else
+          #     @value = another_value?
+          #   end
+          return true if branches_have_assignment?(node) && condition == if_branch.expression
+
+          # e.g.
+          #   if foo
+          #     test.value = foo
+          #   else
+          #     test.value = another_value?
+          #   end
+          branches_have_method?(node) && condition == if_branch.first_argument
+        end
+
+        def branches_have_assignment?(node)
+          _condition, if_branch, else_branch = *node
+
+          return false unless if_branch && else_branch
+
+          asgn_type?(if_branch) && (if_branch_variable_name = if_branch.name) &&
+            asgn_type?(else_branch) && (else_branch_variable_name = else_branch.name) &&
+            if_branch_variable_name == else_branch_variable_name
+        end
+
+        def asgn_type?(node)
+          node.lvasgn_type? || node.ivasgn_type? || node.cvasgn_type? || node.gvasgn_type?
+        end
+
+        def branches_have_method?(node)
+          _condition, if_branch, else_branch = *node
+
+          return false unless if_branch && else_branch
+
+          if_branch.send_type? && if_branch.arguments.count == 1 &&
+            else_branch.send_type? && else_branch.arguments.count == 1 &&
+            if_branch.method?(else_branch.method_name)
+        end
+
         def else_source(else_branch)
-          if require_parentheses?(else_branch)
+          if branches_have_method?(else_branch.parent)
+            else_source_if_has_method(else_branch)
+          elsif require_parentheses?(else_branch)
             "(#{else_branch.source})"
           elsif without_argument_parentheses_method?(else_branch)
             "#{else_branch.method_name}(#{else_branch.arguments.map(&:source).join(', ')})"
+          elsif branches_have_assignment?(else_branch.parent)
+            else_source_if_has_assignment(else_branch)
           else
             else_branch.source
           end
         end
 
+        def else_source_if_has_method(else_branch)
+          if require_parentheses?(else_branch.first_argument)
+            "(#{else_branch.first_argument.source})"
+          else
+            else_branch.first_argument.source
+          end
+        end
+
+        def else_source_if_has_assignment(else_branch)
+          if require_parentheses?(else_branch.expression)
+            "(#{else_branch.expression.source})"
+          else
+            else_branch.expression.source
+          end
+        end
+
         def make_ternary_form(node)
           _condition, if_branch, else_branch = *node
           ternary_form = [if_branch.source, else_source(else_branch)].join(' || ')
@@ -127,8 +197,8 @@ def require_parentheses?(node)
         end
 
         def without_argument_parentheses_method?(node)
-          node.send_type? &&
-            !node.arguments.empty? && !node.parenthesized? && !node.operator_method?
+          node.send_type? && !node.arguments.empty? &&
+            !node.parenthesized? && !node.operator_method? && !node.assignment_method?
         end
       end
     end

spec/rubocop/cop/style/redundant_condition_spec.rb

@@ -227,6 +227,100 @@
           foo || (1..2)
         RUBY
       end
+
+      it 'registers an offense and corrects when defined inside method and the branches contains assignment' do
+        expect_offense(<<~RUBY)
+          def test
+            if foo
+            ^^^^^^ Use double pipes `||` instead.
+              @value = foo
+            else
+              @value = 'bar'
+            end
+          end
+        RUBY
+
+        expect_correction(<<~RUBY)
+          def test
+            @value = foo || 'bar'
+          end
+        RUBY
+      end
+
+      it 'registers an offense and corrects when the branches contains assignment' do
+        expect_offense(<<~RUBY)
+          if foo
+          ^^^^^^ Use double pipes `||` instead.
+            @value = foo
+          else
+            @value = 'bar'
+          end
+        RUBY
+
+        expect_correction(<<~RUBY)
+          @value = foo || 'bar'
+        RUBY
+      end
+
+      it 'registers an offense and corrects when the branches contains assignment method' do
+        expect_offense(<<~RUBY)
+          if foo
+          ^^^^^^ Use double pipes `||` instead.
+            test.bar = foo
+          else
+            test.bar = 'baz'
+          end
+        RUBY
+
+        expect_correction(<<~RUBY)
+          test.bar = foo || 'baz'
+        RUBY
+      end
+
+      it 'registers an offense and corrects when the branches contains 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?
+            @value = foo
+          else
+            @value = 'bar'
+          end
+        RUBY
+      end
+
+      it 'does not register offenses when the branches contains assignment but target not matched' do
+        expect_no_offenses(<<~RUBY)
+          if foo
+            @foo = foo
+          else
+            @baz = 'quux'
+          end
+        RUBY
+      end
+
+      it 'does not register offenses when using `nil?` and the branches contains method which has multiple arguments' do
+        expect_no_offenses(<<~RUBY)
+          if foo.nil?
+            test.bar foo, bar
+          else
+            test.bar = 'baz', 'quux'
+          end
+        RUBY
+      end
     end
   end