From d7795c0d7d5bd6f3009907309883a68ab485bb1e 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: Mon, 19 Dec 2022 06:27:20 -0800 Subject: [PATCH 1/2] Add autocorrect for `Style/UnlessElse` --- spec/ameba/rule/style/unless_else_spec.cr | 10 +++- spec/ameba/source_spec.cr | 17 ++++++ src/ameba/rule/style/unless_else.cr | 25 ++++++++- src/ameba/source.cr | 8 +++ src/ameba/source/corrector.cr | 64 +++++++++++++++++++++++ 5 files changed, 122 insertions(+), 2 deletions(-) diff --git a/spec/ameba/rule/style/unless_else_spec.cr b/spec/ameba/rule/style/unless_else_spec.cr index cf1c065d..f79575a2 100644 --- a/spec/ameba/rule/style/unless_else_spec.cr +++ b/spec/ameba/rule/style/unless_else_spec.cr @@ -13,7 +13,7 @@ module Ameba::Rule::Style end it "fails if unless has else" do - expect_issue subject, <<-CRYSTAL + source = expect_issue subject, <<-CRYSTAL unless something # ^^^^^^^^^^^^^^ error: Favour if over unless with else :one @@ -21,6 +21,14 @@ module Ameba::Rule::Style :two end CRYSTAL + + expect_correction source, <<-CRYSTAL + if something + :two + else + :one + end + CRYSTAL end it "reports rule, pos and message" do diff --git a/spec/ameba/source_spec.cr b/spec/ameba/source_spec.cr index fd4781fa..e50b499b 100644 --- a/spec/ameba/source_spec.cr +++ b/spec/ameba/source_spec.cr @@ -46,5 +46,22 @@ module Ameba s.matches_path?("new_source.cr").should be_false end end + + describe "#pos" do + it "works" do + s = Source.new <<-EOS + foo + bar + fizz + buzz + EOS + loc = Crystal::Location.new("", 2, 1) + end_loc = Crystal::Location.new("", 3, 4) + s.code[s.pos(loc)...s.pos(end_loc, end: true)].should eq <<-EOS + bar + fizz + EOS + end + end end end diff --git a/src/ameba/rule/style/unless_else.cr b/src/ameba/rule/style/unless_else.cr index cdb4b966..20fa0e08 100644 --- a/src/ameba/rule/style/unless_else.cr +++ b/src/ameba/rule/style/unless_else.cr @@ -50,7 +50,30 @@ module Ameba::Rule::Style MSG = "Favour if over unless with else" def test(source, node : Crystal::Unless) - issue_for node, MSG unless node.else.nop? + return if node.else.nop? + return unless location = node.location + return unless cond_end_location = node.cond.end_location + return unless else_location = node.else_location + return unless end_location = node.end_location + + issue_for node, MSG do |corrector| + keyword_begin_pos = source.pos(location) + keyword_end_pos = keyword_begin_pos + {{ "unless".size }} + keyword_range = keyword_begin_pos...keyword_end_pos + + cond_end_pos = source.pos(cond_end_location, end: true) + else_begin_pos = source.pos(else_location) + body_range = cond_end_pos...else_begin_pos + + else_end_pos = else_begin_pos + {{ "else".size }} + end_end_pos = source.pos(end_location, end: true) + end_begin_pos = end_end_pos - {{ "end".size }} + else_range = else_end_pos...end_begin_pos + + corrector.replace(keyword_range, "if") + corrector.replace(body_range, source.code[else_range]) + corrector.replace(else_range, source.code[body_range]) + end end end end diff --git a/src/ameba/source.cr b/src/ameba/source.cr index 38ce9b82..03aa3ac3 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -75,5 +75,13 @@ module Ameba def matches_path?(filepath) path.in?(filepath, File.expand_path(filepath)) end + + # Converts an AST location to a string position. + def pos(location : Crystal::Location, end end_pos = false) : Int32 + line, column = location.line_number, location.column_number + pos = lines[0...line - 1].sum(&.size) + line + column - 2 + pos += 1 if end_pos + pos + end end end diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr index 646594be..33e722d2 100644 --- a/src/ameba/source/corrector.cr +++ b/src/ameba/source/corrector.cr @@ -18,41 +18,92 @@ class Ameba::Source @rewriter.replace(loc_to_pos(location), loc_to_pos(end_location) + 1, content) end + # :ditto: + def replace(range : Range(Int32, Int32), content) + begin_pos, end_pos = range.begin, range.end + end_pos -= 1 unless range.excludes_end? + @rewriter.replace(begin_pos, end_pos, content) + end + # Inserts the given strings before and after the given range. def wrap(location, end_location, insert_before, insert_after) @rewriter.wrap(loc_to_pos(location), loc_to_pos(end_location) + 1, insert_before, insert_after) end + # :ditto: + def wrap(range : Range(Int32, Int32), insert_before, insert_after) + begin_pos, end_pos = range.begin, range.end + end_pos -= 1 unless range.excludes_end? + @rewriter.wrap(begin_pos, end_pos, insert_before, insert_after) + end + # Shortcut for `replace(location, end_location, "")` def remove(location, end_location) @rewriter.remove(loc_to_pos(location), loc_to_pos(end_location) + 1) end + # Shortcut for `replace(range, "")` + def remove(range : Range(Int32, Int32)) + begin_pos, end_pos = range.begin, range.end + end_pos -= 1 unless range.excludes_end? + @rewriter.remove(begin_pos, end_pos) + end + # Shortcut for `wrap(location, end_location, content, nil)` def insert_before(location, end_location, content) @rewriter.insert_before(loc_to_pos(location), loc_to_pos(end_location) + 1, content) end + # Shortcut for `wrap(range, content, nil)` + def insert_before(range : Range(Int32, Int32), content) + begin_pos, end_pos = range.begin, range.end + end_pos -= 1 unless range.excludes_end? + @rewriter.insert_before(begin_pos, end_pos, content) + end + # Shortcut for `wrap(location, end_location, nil, content)` def insert_after(location, end_location, content) @rewriter.insert_after(loc_to_pos(location), loc_to_pos(end_location) + 1, content) end + # Shortcut for `wrap(range, nil, content)` + def insert_after(range : Range(Int32, Int32), content) + begin_pos, end_pos = range.begin, range.end + end_pos -= 1 unless range.excludes_end? + @rewriter.insert_after(begin_pos, end_pos, content) + end + # Shortcut for `insert_before(location, location, content)` def insert_before(location, content) @rewriter.insert_before(loc_to_pos(location), content) end + # Shortcut for `insert_before(pos.., content)` + def insert_before(pos : Int32, content) + @rewriter.insert_before(pos, content) + end + # Shortcut for `insert_after(location, location, content)` def insert_after(location, content) @rewriter.insert_after(loc_to_pos(location) + 1, content) end + # Shortcut for `insert_after(...pos, content)` + def insert_after(pos : Int32, content) + @rewriter.insert_after(pos, content) + end + # Removes *size* characters prior to the source range. def remove_preceding(location, end_location, size) @rewriter.remove(loc_to_pos(location) - size, loc_to_pos(location)) end + # :ditto: + def remove_preceding(range : Range(Int32, Int32), size) + begin_pos = range.begin + @rewriter.remove(begin_pos - size, begin_pos) + end + # Removes *size* characters from the beginning of the given range. # If *size* is greater than the size of the range, the removed region can # overrun the end of the range. @@ -60,6 +111,12 @@ class Ameba::Source @rewriter.remove(loc_to_pos(location), loc_to_pos(location) + size) end + # :ditto: + def remove_leading(range : Range(Int32, Int32), size) + begin_pos = range.begin + @rewriter.remove(begin_pos, begin_pos + size) + end + # Removes *size* characters from the end of the given range. # If *size* is greater than the size of the range, the removed region can # overrun the beginning of the range. @@ -67,6 +124,13 @@ class Ameba::Source @rewriter.remove(loc_to_pos(end_location) + 1 - size, loc_to_pos(end_location) + 1) end + # :ditto: + def remove_trailing(range : Range(Int32, Int32), size) + end_pos = range.end + end_pos -= 1 unless range.excludes_end? + @rewriter.remove(end_pos - size, end_pos) + end + private def loc_to_pos(location : Crystal::Location | {Int32, Int32}) if location.is_a?(Crystal::Location) line, column = location.line_number, location.column_number From 2cedd72b0961feaf83eecbb29caacbb5dc437cb2 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Thu, 22 Dec 2022 19:18:27 +0100 Subject: [PATCH 2/2] Apply review sugggestions --- spec/ameba/source_spec.cr | 51 ++++++++++++++++------------- src/ameba/rule/style/unless_else.cr | 14 +++++--- 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/spec/ameba/source_spec.cr b/spec/ameba/source_spec.cr index e50b499b..8c8b1a1b 100644 --- a/spec/ameba/source_spec.cr +++ b/spec/ameba/source_spec.cr @@ -4,63 +4,70 @@ module Ameba describe Source do describe ".new" do it "allows to create a source by code and path" do - s = Source.new("code", "path") - s.path.should eq "path" - s.code.should eq "code" - s.lines.should eq ["code"] + source = Source.new "code", "path" + source.path.should eq "path" + source.code.should eq "code" + source.lines.should eq ["code"] end end describe "#fullpath" do it "returns a relative path of the source" do - s = Source.new "", "./source_spec.cr" - s.fullpath.should contain "source_spec.cr" + source = Source.new "", "./source_spec.cr" + source.fullpath.should contain "source_spec.cr" end it "returns fullpath if path is blank" do - s = Source.new "", "" - s.fullpath.should_not be_nil + source = Source.new "", "" + source.fullpath.should_not be_nil end end describe "#spec?" do it "returns true if the source is a spec file" do - s = Source.new "", "./source_spec.cr" - s.spec?.should be_true + source = Source.new "", "./source_spec.cr" + source.spec?.should be_true end it "returns false if the source is not a spec file" do - s = Source.new "", "./source.cr" - s.spec?.should be_false + source = Source.new "", "./source.cr" + source.spec?.should be_false end end describe "#matches_path?" do it "returns true if source's path is matched" do - s = Source.new "", "source.cr" - s.matches_path?("source.cr").should be_true + source = Source.new "", "source.cr" + source.matches_path?("source.cr").should be_true end it "returns false if source's path is not matched" do - s = Source.new "", "source.cr" - s.matches_path?("new_source.cr").should be_false + source = Source.new "", "source.cr" + source.matches_path?("new_source.cr").should be_false end end describe "#pos" do it "works" do - s = Source.new <<-EOS + source = Source.new <<-CRYSTAL foo bar fizz buzz - EOS - loc = Crystal::Location.new("", 2, 1) - end_loc = Crystal::Location.new("", 3, 4) - s.code[s.pos(loc)...s.pos(end_loc, end: true)].should eq <<-EOS + CRYSTAL + + location = Crystal::Location.new("", 2, 1) + end_location = Crystal::Location.new("", 3, 4) + + range = Range.new( + source.pos(location), + source.pos(end_location, end: true), + exclusive: true + ) + source.code[range].should eq <<-CRYSTAL bar fizz - EOS + CRYSTAL end end end diff --git a/src/ameba/rule/style/unless_else.cr b/src/ameba/rule/style/unless_else.cr index 20fa0e08..42f9cfb9 100644 --- a/src/ameba/rule/style/unless_else.cr +++ b/src/ameba/rule/style/unless_else.cr @@ -51,10 +51,16 @@ module Ameba::Rule::Style def test(source, node : Crystal::Unless) return if node.else.nop? - return unless location = node.location - return unless cond_end_location = node.cond.end_location - return unless else_location = node.else_location - return unless end_location = node.end_location + + location = node.location + cond_end_location = node.cond.end_location + else_location = node.else_location + end_location = node.end_location + + unless location && cond_end_location && else_location && end_location + issue_for node, MSG + return + end issue_for node, MSG do |corrector| keyword_begin_pos = source.pos(location)