From c0390a00b5a7614528b8766c2ed1bae0868c8d0a Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 5 Aug 2021 12:34:18 +0900 Subject: [PATCH] [Fix #9980] Fix a false positive for `Style/IdenticalConditionalBranches` Fixes #9980. This PR fixes a false positive for `Style/IdenticalConditionalBranches` when assigning to a variable used in a condition. --- ...or_style_identical_conditional_branches.md | 1 + .../style/identical_conditional_branches.rb | 15 ++++-- .../identical_conditional_branches_spec.rb | 47 +++++++++++++++++++ 3 files changed, 59 insertions(+), 4 deletions(-) create mode 100644 changelog/fix_false_positive_for_style_identical_conditional_branches.md diff --git a/changelog/fix_false_positive_for_style_identical_conditional_branches.md b/changelog/fix_false_positive_for_style_identical_conditional_branches.md new file mode 100644 index 00000000000..9d8a4da0465 --- /dev/null +++ b/changelog/fix_false_positive_for_style_identical_conditional_branches.md @@ -0,0 +1 @@ +* [#9980](https://github.com/rubocop/rubocop/issues/9980): Fix a false positive for `Style/IdenticalConditionalBranches` when assigning to a variable used in a condition. ([@koic][]) diff --git a/lib/rubocop/cop/style/identical_conditional_branches.rb b/lib/rubocop/cop/style/identical_conditional_branches.rb index 0eaf35d3367..60185e7891d 100644 --- a/lib/rubocop/cop/style/identical_conditional_branches.rb +++ b/lib/rubocop/cop/style/identical_conditional_branches.rb @@ -124,14 +124,21 @@ def check_branches(node, branches) return if branches.any?(&:nil?) tails = branches.map { |branch| tail(branch) } - check_expressions(node, tails, :after_condition) if duplicated_expressions?(tails) + check_expressions(node, tails, :after_condition) if duplicated_expressions?(node, tails) heads = branches.map { |branch| head(branch) } - check_expressions(node, heads, :before_condition) if duplicated_expressions?(heads) + check_expressions(node, heads, :before_condition) if duplicated_expressions?(node, heads) end - def duplicated_expressions?(expressions) - expressions.size > 1 && expressions.uniq.one? + def duplicated_expressions?(node, expressions) + unique_expressions = expressions.uniq + return false unless expressions.size >= 1 && unique_expressions.one? + + unique_expression = unique_expressions.first + return true unless unique_expression.assignment? + + lhs = unique_expression.child_nodes.first + node.condition.child_nodes.none? { |n| n.source == lhs.source if n.variable? } end def check_expressions(node, expressions, insert_position) # rubocop:disable Metrics/MethodLength diff --git a/spec/rubocop/cop/style/identical_conditional_branches_spec.rb b/spec/rubocop/cop/style/identical_conditional_branches_spec.rb index 2a9d993727b..434c796f107 100644 --- a/spec/rubocop/cop/style/identical_conditional_branches_spec.rb +++ b/spec/rubocop/cop/style/identical_conditional_branches_spec.rb @@ -96,6 +96,21 @@ end end + context 'on if..else with identical bodies and assigning to a variable used in `if` condition' do + it "doesn't register an offense" do + expect_no_offenses(<<~RUBY) + x = 0 + if x == 0 + x += 1 + foo + else + x += 1 + bar + end + RUBY + end + end + context 'on case with identical bodies' do it 'registers an offense and corrects' do expect_offense(<<~RUBY) @@ -233,6 +248,22 @@ end end + context 'on case..when with identical bodies and assigning to a variable used in `case` condition' do + it "doesn't register an offense" do + expect_no_offenses(<<~RUBY) + x = 0 + case x + when 1 + x += 1 + foo + when 42 + x += 1 + bar + end + RUBY + end + end + context 'when using pattern matching', :ruby27 do context 'on case-match with identical bodies' do it 'registers an offense and corrects' do @@ -259,6 +290,22 @@ do_x RUBY end + + context 'on case..in with identical bodies and assigning to a variable used in `case` condition' do + it "doesn't register an offense" do + expect_no_offenses(<<~RUBY) + x = 0 + case x + in 1 + x += 1 + foo + in 42 + x += 1 + bar + end + RUBY + end + end end context 'when one of the case-match branches is empty' do