From e7cfe387d67a4d8a5527df26f9120d9690c3db24 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: Sat, 6 Nov 2021 11:42:29 -0700 Subject: [PATCH] Autocorrect `Stye/RedundantNext` and `Style/RedundantReturn` --- spec/ameba/rule/style/redundant_next_spec.cr | 180 ++++++++----- .../ameba/rule/style/redundant_return_spec.cr | 246 +++++++++++------- src/ameba/ast/util.cr | 12 + src/ameba/rule/style/redundant_next.cr | 10 +- src/ameba/rule/style/redundant_return.cr | 10 +- 5 files changed, 297 insertions(+), 161 deletions(-) diff --git a/spec/ameba/rule/style/redundant_next_spec.cr b/spec/ameba/rule/style/redundant_next_spec.cr index 460d2dca..689894d1 100644 --- a/spec/ameba/rule/style/redundant_next_spec.cr +++ b/spec/ameba/rule/style/redundant_next_spec.cr @@ -5,149 +5,183 @@ module Ameba::Rule::Style describe RedundantNext do it "does not report if there is no redundant next" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL array.map { |x| x + 1 } - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there is redundant next with argument in the block" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL block do |v| next v + 1 + # ^^^^^^^^^^ error: Redundant `next` detected end - ) - subject.catch(s).should_not be_valid + CRYSTAL + + expect_correction source, <<-CRYSTAL + block do |v| + v + 1 + end + CRYSTAL end context "if" do it "doesn't report if there is not redundant next in if branch" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL block do |v| next if v > 10 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there is redundant next in if/else branch" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL block do |a| if a > 0 next a + 1 + # ^^^^^^^^^^ error: Redundant `next` detected else next a + 2 + # ^^^^^^^^^^ error: Redundant `next` detected end end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 2 - s.issues.first.location.to_s.should eq ":3:5" - s.issues.last.location.to_s.should eq ":5:5" + CRYSTAL + + expect_correction source, <<-CRYSTAL + block do |a| + if a > 0 + a + 1 + else + a + 2 + end + end + CRYSTAL end end context "unless" do it "doesn't report if there is no redundant next in unless branch" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL block do |v| next unless v > 10 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there is redundant next in unless/else branch" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL block do |a| unless a > 0 next a + 1 + # ^^^^^^^^^^ error: Redundant `next` detected else next a + 2 + # ^^^^^^^^^^ error: Redundant `next` detected end end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 2 - s.issues.first.location.to_s.should eq ":3:5" - s.issues.last.location.to_s.should eq ":5:5" + CRYSTAL + + expect_correction source, <<-CRYSTAL + block do |a| + unless a > 0 + a + 1 + else + a + 2 + end + end + CRYSTAL end end context "expressions" do it "doesn't report if there is no redundant next in expressions" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL block do |v| a = 1 a + v end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there is redundant next in expressions" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL block do |a| a = 1 next a + # ^^^^^^ error: Redundant `next` detected end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":3:3" + CRYSTAL + + expect_correction source, <<-CRYSTAL + block do |a| + a = 1 + a + end + CRYSTAL end end context "binary-op" do it "doesn't report if there is no redundant next in binary op" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL block do |v| a && v end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there is redundant next in binary op" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL block do |a| a && next a + # ^^^^^^ error: Redundant `next` detected end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":2:8" + CRYSTAL + + expect_correction source, <<-CRYSTAL + block do |a| + a && a + end + CRYSTAL end end context "expception handler" do it "doesn't report if there is no redundant next in exception handler" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL block do |v| v + 1 rescue e next v if v > 0 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there is redundant next in exception handler" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL block do |a| next a + 1 + # ^^^^^^^^^^ error: Redundant `next` detected rescue ArgumentError next a + 2 + # ^^^^^^^^^^ error: Redundant `next` detected rescue Exception a + 2 next a + # ^^^^^^ error: Redundant `next` detected end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 3 - s.issues[0].location.to_s.should eq ":2:3" - s.issues[1].location.to_s.should eq ":4:3" - s.issues[2].location.to_s.should eq ":7:3" + CRYSTAL + + expect_correction source, <<-CRYSTAL + block do |a| + a + 1 + rescue ArgumentError + a + 2 + rescue Exception + a + 2 + a + end + CRYSTAL end end @@ -169,49 +203,51 @@ module Ameba::Rule::Style context "properties" do context "#allow_multi_next=" do it "allows multi next statements by default" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL block do |a, b| next a, b end - ) - subject.catch(s).should be_valid + CRYSTAL end it "allows to configure multi next statements" do - s = Source.new %( - block do |a, b| - next a, b - end - ) rule = Rule::Style::RedundantNext.new rule.allow_multi_next = false - rule.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":2:3" + source = expect_issue rule, <<-CRYSTAL + block do |a, b| + next a, b + # ^^^^^^^^^ error: Redundant `next` detected + end + CRYSTAL + + expect_correction source, <<-CRYSTAL + block do |a, b| + {a, b} + end + CRYSTAL end end context "#allow_empty_next" do it "allows empty next statements by default" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL block do next end - ) - subject.catch(s).should be_valid + CRYSTAL end it "allows to configure empty next statements" do - s = Source.new %( - block do - next - end - ) rule = Rule::Style::RedundantNext.new rule.allow_empty_next = false - rule.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":2:3" + source = expect_issue rule, <<-CRYSTAL + block do + next + # ^^^^ error: Redundant `next` detected + end + CRYSTAL + + expect_no_corrections source end end end diff --git a/spec/ameba/rule/style/redundant_return_spec.cr b/spec/ameba/rule/style/redundant_return_spec.cr index 4638dfc9..6fe207b8 100644 --- a/spec/ameba/rule/style/redundant_return_spec.cr +++ b/spec/ameba/rule/style/redundant_return_spec.cr @@ -5,36 +5,38 @@ module Ameba::Rule::Style describe RedundantReturn do it "does not report if there is no return" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def inc(a) a + 1 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there is redundant return in method body" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def inc(a) return a + 1 + # ^^^^^^^^^^^^ error: Redundant `return` detected end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":2:3" + CRYSTAL + + expect_correction source, <<-CRYSTAL + def inc(a) + a + 1 + end + CRYSTAL end it "doesn't report if it returns tuple literal" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def foo(a) return a, a + 2 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if there are other expressions after control flow" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) case a when true then return true @@ -44,109 +46,141 @@ module Ameba::Rule::Style rescue nil end - ) - subject.catch(s).should be_valid + CRYSTAL end context "if" do it "doesn't report if there is return in if branch" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def inc(a) return a + 1 if a > 0 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there are returns in if/else branch" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def inc(a) do_something(a) if a > 0 return :positive + # ^^^^^^^^^^^^^^^^ error: Redundant `return` detected else return :negative + # ^^^^^^^^^^^^^^^^ error: Redundant `return` detected end end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 2 - s.issues.first.location.to_s.should eq ":4:5" - s.issues.last.location.to_s.should eq ":6:5" + CRYSTAL + + expect_correction source, <<-CRYSTAL + def inc(a) + do_something(a) + if a > 0 + :positive + else + :negative + end + end + CRYSTAL end end context "unless" do it "doesn't report if there is return in unless branch" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def inc(a) return a + 1 unless a > 0 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there are returns in unless/else branch" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def inc(a) do_something(a) unless a < 0 return :positive + # ^^^^^^^^^^^^^^^^ error: Redundant `return` detected else return :negative + # ^^^^^^^^^^^^^^^^ error: Redundant `return` detected end end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 2 - s.issues.first.location.to_s.should eq ":4:5" - s.issues.last.location.to_s.should eq ":6:5" + CRYSTAL + + expect_correction source, <<-CRYSTAL + def inc(a) + do_something(a) + unless a < 0 + :positive + else + :negative + end + end + CRYSTAL end end context "binary op" do it "doesn't report if there is no return in the right binary op node" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def can_create?(a) valid? && a > 0 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there is return in the right binary op node" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def can_create?(a) valid? && return a > 0 + # ^^^^^^^^^^^^ error: Redundant `return` detected end - ) - subject.catch(s).should_not be_valid + CRYSTAL + + expect_correction source, <<-CRYSTAL + def can_create?(a) + valid? && a > 0 + end + CRYSTAL end end context "case" do it "reports if there are returns in whens" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def foo(a) case a when .nil? puts "blah" return nil + # ^^^^^^^^^^ error: Redundant `return` detected when .blank? return "" + # ^^^^^^^^^ error: Redundant `return` detected when true true end end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 2 - s.issues.first.location.to_s.should eq ":5:5" - s.issues.last.location.to_s.should eq ":7:5" + CRYSTAL + + expect_correction source, <<-CRYSTAL + def foo(a) + case a + when .nil? + puts "blah" + nil + when .blank? + "" + when true + true + end + end + CRYSTAL end it "reports if there is return in else" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def foo(a) do_something_with(a) @@ -155,49 +189,76 @@ module Ameba::Rule::Style true else return false + # ^^^^^^^^^^^^ error: Redundant `return` detected end end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":8:5" + CRYSTAL + + expect_correction source, <<-CRYSTAL + def foo(a) + do_something_with(a) + + case a + when true + true + else + false + end + end + CRYSTAL end end context "exception handler" do it "reports if there are returns in body" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def foo(a) return true + # ^^^^^^^^^^^ error: Redundant `return` detected rescue false end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":2:3" + CRYSTAL + + expect_correction source, <<-CRYSTAL + def foo(a) + true + rescue + false + end + CRYSTAL end it "reports if there are returns in rescues" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def foo(a) true rescue ArgumentError return false + # ^^^^^^^^^^^^ error: Redundant `return` detected rescue RuntimeError "" rescue Exception return nil + # ^^^^^^^^^^ error: Redundant `return` detected end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 2 - s.issues.first.location.to_s.should eq ":4:3" - s.issues.last.location.to_s.should eq ":8:3" + CRYSTAL + + expect_correction source, <<-CRYSTAL + def foo(a) + true + rescue ArgumentError + false + rescue RuntimeError + "" + rescue Exception + nil + end + CRYSTAL end it "reports if there are returns in else" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def foo(a) true rescue Exception @@ -205,60 +266,71 @@ module Ameba::Rule::Style else puts "else branch" return :bar + # ^^^^^^^^^^^ error: Redundant `return` detected end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":7:3" + CRYSTAL + + expect_correction source, <<-CRYSTAL + def foo(a) + true + rescue Exception + nil + else + puts "else branch" + :bar + end + CRYSTAL end end context "properties" do context "#allow_multi_return=" do it "allows multi returns by default" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a, b) return a, b end - ) - subject.catch(s).should be_valid + CRYSTAL end it "allows to configure multi returns" do - s = Source.new %( - def method(a, b) - return a, b - end - ) rule = Rule::Style::RedundantReturn.new rule.allow_multi_return = false - rule.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":2:3" + source = expect_issue rule, <<-CRYSTAL + def method(a, b) + return a, b + # ^^^^^^^^^^^ error: Redundant `return` detected + end + CRYSTAL + + expect_correction source, <<-CRYSTAL + def method(a, b) + {a, b} + end + CRYSTAL end end context "#allow_empty_return" do it "allows empty returns by default" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method return end - ) - subject.catch(s).should be_valid + CRYSTAL end it "allows to configure empty returns" do - s = Source.new %( - def method - return - end - ) rule = Rule::Style::RedundantReturn.new rule.allow_empty_return = false - rule.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":2:3" + source = expect_issue rule, <<-CRYSTAL + def method + return + # ^^^^^^ error: Redundant `return` detected + end + CRYSTAL + + expect_no_corrections source end end end diff --git a/src/ameba/ast/util.cr b/src/ameba/ast/util.cr index 5b87608a..0d17d857 100644 --- a/src/ameba/ast/util.cr +++ b/src/ameba/ast/util.cr @@ -158,4 +158,16 @@ module Ameba::AST::Util false end end + + # Returns the exp code of a control expression. + # Wraps implicit tuple literal with curly brackets (e.g. multi-return). + def control_exp_code(source, node : Crystal::ControlExpression) + return unless (exp = node.exp) + return unless (exp_code = node_source(exp, source.lines)) + return exp_code unless exp.is_a?(Crystal::TupleLiteral) && exp_code[0] != '{' + return unless (exp_start = exp.elements.first.location) + return unless (exp_end = exp.end_location) + + "{#{source_between(exp_start, exp_end, source.lines)}}" + end end diff --git a/src/ameba/rule/style/redundant_next.cr b/src/ameba/rule/style/redundant_next.cr index 5decb740..4748b7c1 100644 --- a/src/ameba/rule/style/redundant_next.cr +++ b/src/ameba/rule/style/redundant_next.cr @@ -97,6 +97,8 @@ module Ameba::Rule::Style # AllowEmptyNext: true # ``` class RedundantNext < Base + include AST::Util + properties do description "Reports redundant next expressions" @@ -114,7 +116,13 @@ module Ameba::Rule::Style return if allow_multi_next && node.exp.is_a?(Crystal::TupleLiteral) return if allow_empty_next && (node.exp.nil? || node.exp.not_nil!.nop?) - source.try &.add_issue self, node, MSG + if (exp_code = control_exp_code(source, node)) + issue_for node, MSG do |corrector| + corrector.replace(node, exp_code) + end + else + issue_for node, MSG + end end end end diff --git a/src/ameba/rule/style/redundant_return.cr b/src/ameba/rule/style/redundant_return.cr index e8ec7dc9..32f1c86b 100644 --- a/src/ameba/rule/style/redundant_return.cr +++ b/src/ameba/rule/style/redundant_return.cr @@ -94,6 +94,8 @@ module Ameba::Rule::Style # AllowEmptyReturn: true # ``` class RedundantReturn < Base + include AST::Util + properties do description "Reports redundant return expressions" @@ -111,7 +113,13 @@ module Ameba::Rule::Style return if allow_multi_return && node.exp.is_a?(Crystal::TupleLiteral) return if allow_empty_return && (node.exp.nil? || node.exp.not_nil!.nop?) - source.try &.add_issue self, node, MSG + if (exp_code = control_exp_code(source, node)) + issue_for node, MSG do |corrector| + corrector.replace(node, exp_code) + end + else + issue_for node, MSG + end end end end