From 30d860838b19afa495ff2da2543cfa105b143800 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?fn=20=E2=8C=83=20=E2=8C=A5?= <70830482+FnControlOption@users.noreply.github.com> Date: Tue, 16 Nov 2021 23:57:42 -0800 Subject: [PATCH] Add simple autocorrect for `Style/GuardClause` --- ...mple_autocorrect_for_style_guard_clause.md | 1 + lib/rubocop/cop/style/guard_clause.rb | 83 ++++++++--- spec/rubocop/cli/autocorrect_spec.rb | 4 +- spec/rubocop/cli/options_spec.rb | 4 +- spec/rubocop/cop/style/guard_clause_spec.rb | 129 ++++++++++++++++++ 5 files changed, 197 insertions(+), 24 deletions(-) create mode 100644 changelog/new_add_simple_autocorrect_for_style_guard_clause.md diff --git a/changelog/new_add_simple_autocorrect_for_style_guard_clause.md b/changelog/new_add_simple_autocorrect_for_style_guard_clause.md new file mode 100644 index 00000000000..180b96f5bd6 --- /dev/null +++ b/changelog/new_add_simple_autocorrect_for_style_guard_clause.md @@ -0,0 +1 @@ +* Add simple autocorrect for `Style/GuardClause`. ([@FnControlOption][]) diff --git a/lib/rubocop/cop/style/guard_clause.rb b/lib/rubocop/cop/style/guard_clause.rb index 796453c7501..80065970a29 100644 --- a/lib/rubocop/cop/style/guard_clause.rb +++ b/lib/rubocop/cop/style/guard_clause.rb @@ -10,6 +10,9 @@ module Style # one of `return`, `break`, `next`, `raise`, or `fail` is used # in the body of the conditional expression. # + # NOTE: Autocorrect works in most cases except with if-else statements + # that contain logical operators such as `foo || raise('exception')` + # # @example # # bad # def test @@ -90,6 +93,7 @@ module Style # end # class GuardClause < Base + extend AutoCorrector include MinBodyLength include StatementModifier @@ -101,40 +105,49 @@ def on_def(node) return unless body - if body.if_type? - check_ending_if(body) - elsif body.begin_type? - final_expression = body.children.last - check_ending_if(final_expression) if final_expression&.if_type? - end + check_ending_body(body) end alias on_defs on_def def on_if(node) return if accepted_form?(node) - guard_clause_in_if = node.if_branch&.guard_clause? - guard_clause_in_else = node.else_branch&.guard_clause? - guard_clause = guard_clause_in_if || guard_clause_in_else - return unless guard_clause + if (guard_clause = node.if_branch&.guard_clause?) + kw = node.loc.keyword.source + guard = :if + elsif (guard_clause = node.else_branch&.guard_clause?) + kw = node.inverse_keyword + guard = :else + else + return + end - kw = if guard_clause_in_if - node.loc.keyword.source - else - node.inverse_keyword - end + guard = nil if and_or_guard_clause?(guard_clause) - register_offense(node, guard_clause_source(guard_clause), kw) + register_offense(node, guard_clause_source(guard_clause), kw, guard) end private + def check_ending_body(body) + return if body.nil? + + if body.if_type? + check_ending_if(body) + elsif body.begin_type? + final_expression = body.children.last + check_ending_if(final_expression) if final_expression&.if_type? + end + end + def check_ending_if(node) return if accepted_form?(node, ending: true) || !min_body_length?(node) return if allowed_consecutive_conditionals? && consecutive_conditionals?(node.parent, node) register_offense(node, 'return', node.inverse_keyword) + + check_ending_body(node.if_branch) end def consecutive_conditionals?(parent, node) @@ -145,28 +158,56 @@ def consecutive_conditionals?(parent, node) end end - def register_offense(node, scope_exiting_keyword, conditional_keyword) + def register_offense(node, scope_exiting_keyword, conditional_keyword, guard = nil) condition, = node.node_parts example = [scope_exiting_keyword, conditional_keyword, condition.source].join(' ') if too_long_for_single_line?(node, example) return if trivial?(node) example = "#{conditional_keyword} #{condition.source}; #{scope_exiting_keyword}; end" + replacement = <<~RUBY.chomp + #{conditional_keyword} #{condition.source} + #{scope_exiting_keyword} + end + RUBY end - add_offense(node.loc.keyword, message: format(MSG, example: example)) + add_offense(node.loc.keyword, message: format(MSG, example: example)) do |corrector| + next if node.else? && guard.nil? + + autocorrect(corrector, node, condition, replacement || example, guard) + end end - def guard_clause_source(guard_clause) - parent = guard_clause.parent + def autocorrect(corrector, node, condition, replacement, guard) + corrector.replace(node.loc.keyword.join(condition.loc.expression), replacement) + corrector.remove(node.loc.end) + return unless node.else? + + corrector.remove(node.loc.else) + corrector.remove(branch_to_remove(node, guard)) + end + + def branch_to_remove(node, guard) + case guard + when :if then node.if_branch + when :else then node.else_branch + end + end - if parent.and_type? || parent.or_type? + def guard_clause_source(guard_clause) + if and_or_guard_clause?(guard_clause) guard_clause.parent.source else guard_clause.source end end + def and_or_guard_clause?(guard_clause) + parent = guard_clause.parent + parent.and_type? || parent.or_type? + end + def too_long_for_single_line?(node, example) max = max_line_length max && node.source_range.column + example.length > max diff --git a/spec/rubocop/cli/autocorrect_spec.rb b/spec/rubocop/cli/autocorrect_spec.rb index 886890a2792..60e44c4d75c 100644 --- a/spec/rubocop/cli/autocorrect_spec.rb +++ b/spec/rubocop/cli/autocorrect_spec.rb @@ -1179,7 +1179,9 @@ class Goo def something first call do_other 'things' - more_work if other > 34 + return unless other > 34 + + more_work end end end diff --git a/spec/rubocop/cli/options_spec.rb b/spec/rubocop/cli/options_spec.rb index 6f7dd08e686..4872d324f28 100644 --- a/spec/rubocop/cli/options_spec.rb +++ b/spec/rubocop/cli/options_spec.rb @@ -1577,7 +1577,7 @@ def badName 'Use snake_case for method names.', 'def badName', ' ^^^^^^^', - 'example3.rb:2:3: C: Style/GuardClause: ' \ + 'example3.rb:2:3: C: [Correctable] Style/GuardClause: ' \ 'Use a guard clause (return unless something) instead of ' \ 'wrapping the code inside a conditional expression.', ' if something', @@ -1592,7 +1592,7 @@ def badName ' end', ' ^^^', '', - '3 files inspected, 15 offenses detected, 12 offenses autocorrectable', + '3 files inspected, 15 offenses detected, 13 offenses autocorrectable', '' ].join("\n")) end diff --git a/spec/rubocop/cop/style/guard_clause_spec.rb b/spec/rubocop/cop/style/guard_clause_spec.rb index 022308b5f5e..6eec5a357e5 100644 --- a/spec/rubocop/cop/style/guard_clause_spec.rb +++ b/spec/rubocop/cop/style/guard_clause_spec.rb @@ -28,6 +28,20 @@ def func end end RUBY + + expect_correction(<<~RUBY) + def func + return unless something + #{body} + #{trailing_whitespace} + end + + def func + return if something + #{body} + #{trailing_whitespace} + end + RUBY end it 'reports an offense if method body ends with if / unless without else' do @@ -48,6 +62,22 @@ def func end end RUBY + + expect_correction(<<~RUBY) + def func + test + return unless something + #{body} + #{trailing_whitespace} + end + + def func + test + return if something + #{body} + #{trailing_whitespace} + end + RUBY end end @@ -115,6 +145,8 @@ def func end end RUBY + + expect_no_corrections end it 'registers an offense when using `|| raise` in `else` branch' do @@ -128,6 +160,8 @@ def func end end RUBY + + expect_no_corrections end it 'registers an offense when using `and return` in `then` branch' do @@ -141,6 +175,8 @@ def func end end RUBY + + expect_no_corrections end it 'registers an offense when using `and return` in `else` branch' do @@ -154,6 +190,8 @@ def func end end RUBY + + expect_no_corrections end it 'accepts a method which body does not end with if / unless' do @@ -225,6 +263,20 @@ def func end end RUBY + + expect_correction(<<~RUBY) + def func + return unless something + work + #{trailing_whitespace} + end + + def func + return if something + work + #{trailing_whitespace} + end + RUBY end end @@ -336,6 +388,14 @@ def func puts "hello" end RUBY + + expect_correction(<<~RUBY) + #{kw} if something + #{trailing_whitespace} + + puts "hello" + + RUBY end it "registers an error with #{kw} in the else branch" do @@ -347,6 +407,14 @@ def func #{kw} end RUBY + + expect_correction(<<~RUBY) + #{kw} unless something + puts "hello" + + #{trailing_whitespace} + + RUBY end it "doesn't register an error if condition has multiple lines" do @@ -403,6 +471,15 @@ def func "blah blah blah" end RUBY + + expect_correction(<<~RUBY) + #{kw} if something + #{trailing_whitespace} + + puts "hello" \\ + "blah blah blah" + + RUBY end end @@ -427,11 +504,24 @@ def test if something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line ^^ Use a guard clause (`unless something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line; return; end`) instead of wrapping the code inside a conditional expression. if something_else + ^^ Use a guard clause (`return unless something_else`) instead of wrapping the code inside a conditional expression. work end end end RUBY + + expect_correction(<<~RUBY) + def test + unless something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line + return + end + return unless something_else + work + #{trailing_whitespace} + #{trailing_whitespace} + end + RUBY end end @@ -446,6 +536,17 @@ def test end end RUBY + + expect_correction(<<~RUBY) + def test + unless something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line + return + end + work + more_work + #{trailing_whitespace} + end + RUBY end end end @@ -463,6 +564,14 @@ def test end end RUBY + + expect_correction(<<~RUBY) + def test + return unless something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line + work + #{trailing_whitespace} + end + RUBY end end @@ -483,6 +592,16 @@ def test end end RUBY + + expect_correction(<<~RUBY) + module CopTest + def test + return unless something + work + #{trailing_whitespace} + end + end + RUBY end it 'registers an offense for singleton methods' do @@ -496,6 +615,16 @@ def self.test end end RUBY + + expect_correction(<<~RUBY) + module CopTest + def self.test + return unless something && something_else + work + #{trailing_whitespace} + end + end + RUBY end end end