From 5ce8bb9d83b31b7ff8863649d359209a471461da Mon Sep 17 00:00:00 2001 From: ydah <13041216+ydah@users.noreply.github.com> Date: Fri, 11 Mar 2022 16:04:49 +0900 Subject: [PATCH] [Fix rubocop#10436] Fix an error for `Style/SoleNestedConditional` raises exception when inspecting `if ... end if ...` --- ..._an_error_style_sole_nested_conditional.md | 1 + .../cop/style/sole_nested_conditional.rb | 62 +++++++-- .../cop/style/sole_nested_conditional_spec.rb | 122 +++++++++++++++++- 3 files changed, 172 insertions(+), 13 deletions(-) create mode 100644 changelog/fix_an_error_style_sole_nested_conditional.md diff --git a/changelog/fix_an_error_style_sole_nested_conditional.md b/changelog/fix_an_error_style_sole_nested_conditional.md new file mode 100644 index 00000000000..aa206c9e875 --- /dev/null +++ b/changelog/fix_an_error_style_sole_nested_conditional.md @@ -0,0 +1 @@ +* [#10447](https://github.com/rubocop/rubocop/pull/10447): Fix an error for `Style/SoleNestedConditional` raises exception when inspecting `if ... end if ...`. ([@ydah][]) diff --git a/lib/rubocop/cop/style/sole_nested_conditional.rb b/lib/rubocop/cop/style/sole_nested_conditional.rb index 10c6b3369b5..2991a021128 100644 --- a/lib/rubocop/cop/style/sole_nested_conditional.rb +++ b/lib/rubocop/cop/style/sole_nested_conditional.rb @@ -15,6 +15,11 @@ module Style # end # end # + # # bad + # if condition_b + # do_something + # end if condition_a + # # # good # if condition_a && condition_b # do_something @@ -26,12 +31,21 @@ module Style # do_something if condition_b # end # + # # bad + # if condition_b + # do_something + # end if condition_a + # # @example AllowModifier: true # # good # if condition_a # do_something if condition_b # end # + # # good + # if condition_b + # do_something + # end if condition_a class SoleNestedConditional < Base include RangeHelp extend AutoCorrector @@ -47,7 +61,7 @@ def on_if(node) if_branch = node.if_branch return if use_variable_assignment_in_condition?(node.condition, if_branch) - return unless offending_branch?(if_branch) + return unless offending_branch?(node, if_branch) message = format(MSG, conditional_type: node.keyword) add_offense(if_branch.loc.keyword, message: message) do |corrector| @@ -72,13 +86,13 @@ def assigned_variables(condition) end end - def offending_branch?(branch) + def offending_branch?(node, branch) return false unless branch branch.if_type? && !branch.else? && !branch.ternary? && - !(branch.modifier_form? && allow_modifier?) + !((node.modifier_form? || branch.modifier_form?) && allow_modifier?) end def autocorrect(corrector, node, if_branch) @@ -86,6 +100,14 @@ def autocorrect(corrector, node, if_branch) corrector.wrap(node.condition, '(', ')') end + if outer_condition_modify_form?(node, if_branch) + autocorrect_outer_condition_modify_form(corrector, node, if_branch) + else + autocorrect_outer_condition_basic(corrector, node, if_branch) + end + end + + def autocorrect_outer_condition_basic(corrector, node, if_branch) correct_from_unless_to_if(corrector, node) if node.unless? and_operator = if_branch.unless? ? ' && !' : ' && ' @@ -97,11 +119,17 @@ def autocorrect(corrector, node, if_branch) end end - def correct_from_unless_to_if(corrector, node) + def autocorrect_outer_condition_modify_form(corrector, node, if_branch) + correct_from_unless_to_if(corrector, if_branch, is_modify_form: true) if if_branch.unless? + correct_for_outer_condition_modify_form_style(corrector, node, if_branch) + end + + def correct_from_unless_to_if(corrector, node, is_modify_form: false) corrector.replace(node.loc.keyword, 'if') condition = node.condition - if condition.send_type? && condition.comparison_method? && !condition.parenthesized? + if (condition.send_type? && condition.comparison_method? && !condition.parenthesized?) || + (is_modify_form && wrap_condition?(condition)) corrector.wrap(node.condition, '!(', ')') else corrector.insert_before(node.condition, '!') @@ -113,7 +141,7 @@ def correct_for_guard_condition_style(corrector, node, if_branch, and_operator) correct_outer_condition(corrector, outer_condition) condition = if_branch.condition - corrector.insert_after(outer_condition, replacement_condition(and_operator, condition)) + corrector.insert_after(outer_condition, "#{and_operator}#{replace_condition(condition)}") range = range_between(if_branch.loc.keyword.begin_pos, condition.source_range.end_pos) corrector.remove(range_with_surrounding_space(range: range, newlines: false)) @@ -129,6 +157,16 @@ def correct_for_basic_condition_style(corrector, node, if_branch, and_operator) corrector.wrap(if_branch.condition, '(', ')') if wrap_condition?(if_branch.condition) end + def correct_for_outer_condition_modify_form_style(corrector, node, if_branch) + condition = if_branch.condition + corrector.insert_before(condition, + "#{'!' if node.unless?}#{replace_condition(node.condition)} && ") + + corrector.remove(node.condition.loc.expression) + corrector.remove(range_with_surrounding_space(range: node.loc.keyword, newlines: false)) + corrector.replace(if_branch.loc.keyword, 'if') + end + def correct_for_comment(corrector, node, if_branch) return if config.for_cop('Style/IfUnlessModifier')['Enabled'] @@ -165,17 +203,17 @@ def wrap_condition?(node) (node.send_type? && node.arguments.any? && !node.parenthesized?) end - def replacement_condition(and_operator, condition) - if wrap_condition?(condition) - "#{and_operator}(#{condition.source})" - else - "#{and_operator}#{condition.source}" - end + def replace_condition(condition) + wrap_condition?(condition) ? "(#{condition.source})" : condition.source end def allow_modifier? cop_config['AllowModifier'] end + + def outer_condition_modify_form?(node, if_branch) + node.condition.loc.expression.begin_pos > if_branch.condition.loc.expression.begin_pos + end end end end diff --git a/spec/rubocop/cop/style/sole_nested_conditional_spec.rb b/spec/rubocop/cop/style/sole_nested_conditional_spec.rb index afa42f5ba64..d3906651812 100644 --- a/spec/rubocop/cop/style/sole_nested_conditional_spec.rb +++ b/spec/rubocop/cop/style/sole_nested_conditional_spec.rb @@ -409,6 +409,126 @@ def foo RUBY end + it 'registers an offense and corrects when `if` foo do_something end `if` bar' do + expect_offense(<<~RUBY) + if foo + ^^ Consider merging nested conditions into outer `if` conditions. + do_something + end if bar + RUBY + + expect_correction(<<~RUBY) + if bar && foo + do_something + end + RUBY + end + + it 'registers an offense and corrects when `if` foo do_something end `unless` bar' do + expect_offense(<<~RUBY) + if foo + ^^ Consider merging nested conditions into outer `unless` conditions. + do_something + end unless bar + RUBY + + expect_correction(<<~RUBY) + if !bar && foo + do_something + end + RUBY + end + + it 'registers an offense and corrects when `unless` foo do_something end `if` bar' do + expect_offense(<<~RUBY) + unless foo + ^^^^^^ Consider merging nested conditions into outer `if` conditions. + do_something + end if bar + RUBY + + expect_correction(<<~RUBY) + if bar && !foo + do_something + end + RUBY + end + + it 'registers an offense and corrects when `if` foo do_something end `if` bar && baz' do + expect_offense(<<~RUBY) + if foo + ^^ Consider merging nested conditions into outer `if` conditions. + do_something + end if bar && baz + RUBY + + expect_correction(<<~RUBY) + if (bar && baz) && foo + do_something + end + RUBY + end + + it 'registers an offense and corrects when `if` foo && bar do_something end `if` baz' do + expect_offense(<<~RUBY) + if foo && bar + ^^ Consider merging nested conditions into outer `if` conditions. + do_something + end if baz + RUBY + + expect_correction(<<~RUBY) + if baz && foo && bar + do_something + end + RUBY + end + + it 'registers an offense and corrects when `if` foo do_something end `unless` bar && baz' do + expect_offense(<<~RUBY) + if foo + ^^ Consider merging nested conditions into outer `unless` conditions. + do_something + end unless bar && baz + RUBY + + expect_correction(<<~RUBY) + if !(bar && baz) && foo + do_something + end + RUBY + end + + it 'registers an offense and corrects when `if` foo && bar do_something end `unless` baz' do + expect_offense(<<~RUBY) + if foo && bar + ^^ Consider merging nested conditions into outer `unless` conditions. + do_something + end unless baz + RUBY + + expect_correction(<<~RUBY) + if !baz && foo && bar + do_something + end + RUBY + end + + it 'registers an offense and corrects when `unless` foo && bar do_something end `if` baz' do + expect_offense(<<~RUBY) + unless foo && bar + ^^^^^^ Consider merging nested conditions into outer `if` conditions. + do_something + end if baz + RUBY + + expect_correction(<<~RUBY) + if baz && !(foo && bar) + do_something + end + RUBY + end + it 'does not register an offense when using nested ternary within conditional' do expect_no_offenses(<<~RUBY) if foo @@ -557,7 +677,7 @@ def foo expect_no_offenses(<<~RUBY) if foo do_something if bar - end + end if baz RUBY end end