From c1b4add09427f6b7fa715ce29e9598a41ea292ba 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: Sun, 24 Oct 2021 11:56:01 -0700 Subject: [PATCH 01/23] Add `Source::Corrector` and `Source::Rewriter` --- spec/ameba/source/rewriter_spec.cr | 110 +++++++++++++++++ src/ameba.cr | 1 + src/ameba/issue.cr | 10 +- src/ameba/reportable.cr | 55 +++++++-- src/ameba/rule/base.cr | 4 +- src/ameba/source/corrector.cr | 57 +++++++++ src/ameba/source/rewriter.cr | 132 ++++++++++++++++++++ src/ameba/source/rewriter/action.cr | 181 ++++++++++++++++++++++++++++ 8 files changed, 537 insertions(+), 13 deletions(-) create mode 100644 spec/ameba/source/rewriter_spec.cr create mode 100644 src/ameba/source/corrector.cr create mode 100644 src/ameba/source/rewriter.cr create mode 100644 src/ameba/source/rewriter/action.cr diff --git a/spec/ameba/source/rewriter_spec.cr b/spec/ameba/source/rewriter_spec.cr new file mode 100644 index 00000000..7d97bb9b --- /dev/null +++ b/spec/ameba/source/rewriter_spec.cr @@ -0,0 +1,110 @@ +require "../../spec_helper" + +class Ameba::Source + describe Rewriter do + code = "puts(:hello, :world)" + hello = {5, 11} + comma_space = {11, 13} + world = {13, 19} + + it "can remove" do + rewriter = Rewriter.new(code) + rewriter.remove(*hello) + rewriter.process.should eq "puts(, :world)" + end + + it "can insert before" do + rewriter = Rewriter.new(code) + rewriter.insert_before(*world, "42, ") + rewriter.process.should eq "puts(:hello, 42, :world)" + end + + it "can insert after" do + rewriter = Rewriter.new(code) + rewriter.insert_after(*hello, ", 42") + rewriter.process.should eq "puts(:hello, 42, :world)" + end + + it "can wrap" do + rewriter = Rewriter.new(code) + rewriter.wrap(*hello, '[', ']') + rewriter.process.should eq "puts([:hello], :world)" + end + + it "can replace" do + rewriter = Rewriter.new(code) + rewriter.replace(*hello, ":hi") + rewriter.process.should eq "puts(:hi, :world)" + end + + it "accepts crossing deletions" do + rewriter = Rewriter.new(code) + rewriter.remove(hello[0], comma_space[1]) + rewriter.remove(comma_space[0], world[1]) + rewriter.process.should eq "puts()" + end + + it "accepts multiple actions" do + rewriter = Rewriter.new(code) + rewriter.replace(*comma_space, " => ") + rewriter.wrap(hello[0], world[1], '{', '}') + rewriter.replace(*world, ":everybody") + rewriter.wrap(*world, '[', ']') + rewriter.process.should eq "puts({:hello => [:everybody]})" + end + + it "can wrap the same range" do + rewriter = Rewriter.new(code) + rewriter.wrap(*hello, '(', ')') + rewriter.wrap(*hello, '[', ']') + rewriter.process.should eq "puts([(:hello)], :world)" + end + + it "can insert on empty ranges" do + rewriter = Rewriter.new(code) + rewriter.insert_before(hello[0], '{') + rewriter.replace(hello[0], hello[0], 'x') + rewriter.insert_after(hello[0], '}') + rewriter.insert_before(hello[1], '[') + rewriter.replace(hello[1], hello[1], 'y') + rewriter.insert_after(hello[1], ']') + rewriter.process.should eq "puts({x}:hello[y], :world)" + end + + it "can replace the same range" do + rewriter = Rewriter.new(code) + rewriter.replace(*hello, ":hi") + rewriter.replace(*hello, ":hey") + rewriter.process.should eq "puts(:hey, :world)" + end + + it "can swallow insertions" do + rewriter = Rewriter.new(code) + rewriter.wrap(hello[0] + 1, hello[1], "__", "__") + rewriter.replace(world[0], world[1] - 2, "xx") + rewriter.replace(hello[0], world[1], ":hi") + rewriter.process.should eq "puts(:hi)" + end + + it "rejects out-of-range ranges" do + rewriter = Rewriter.new(code) + expect_raises(IndexError) { rewriter.insert_before(0, 100, "hola") } + end + + it "ignores trivial actions" do + rewriter = Rewriter.new(code) + rewriter.empty?.should be_true + + # This is a trivial wrap + rewriter.wrap(2, 5, "", "") + rewriter.empty?.should be_true + + # This is a trivial deletion + rewriter.remove(2, 2) + rewriter.empty?.should be_true + + rewriter.remove(2, 5) + rewriter.empty?.should be_false + end + end +end diff --git a/src/ameba.cr b/src/ameba.cr index f16c62e6..2a34d0ef 100644 --- a/src/ameba.cr +++ b/src/ameba.cr @@ -2,6 +2,7 @@ require "./ameba/*" require "./ameba/ast/**" require "./ameba/rule/**" require "./ameba/formatter/*" +require "./ameba/source/**" # Ameba's entry module. # diff --git a/src/ameba/issue.cr b/src/ameba/issue.cr index b913eac3..06619686 100644 --- a/src/ameba/issue.cr +++ b/src/ameba/issue.cr @@ -24,12 +24,20 @@ module Ameba delegate :enabled?, :disabled?, to: status - def initialize(@rule, @location, @end_location, @message, status : Status? = nil) + def initialize(@rule, @location, @end_location, @message, status : Status? = nil, @block : (Source::Corrector ->)? = nil) @status = status || Status::Enabled end def syntax? rule.is_a?(Rule::Lint::Syntax) end + + def correctable? + !@block.nil? + end + + def correct(corrector) + @block.try &.call(corrector) + end end end diff --git a/src/ameba/reportable.cr b/src/ameba/reportable.cr index 4632fa91..a85bf28b 100644 --- a/src/ameba/reportable.cr +++ b/src/ameba/reportable.cr @@ -5,41 +5,76 @@ module Ameba getter issues = [] of Issue # Adds a new issue to the list of issues. - def add_issue(rule, location, end_location, message, status : Issue::Status? = nil) : Issue + def add_issue(rule, location, end_location, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil) : Issue status ||= Issue::Status::Disabled if location_disabled?(location, rule) - Issue.new(rule, location, end_location, message, status).tap do |issue| + Issue.new(rule, location, end_location, message, status, block).tap do |issue| issues << issue end end + # :ditto: + def add_issue(rule, location, end_location, message, status : Issue::Status? = nil, &block : Source::Corrector ->) : Issue + add_issue rule, location, end_location, message, status, block + end + # Adds a new issue for Crystal AST *node*. - def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil) : Issue - add_issue rule, node.location, node.end_location, message, status + def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil) : Issue + add_issue rule, node.location, node.end_location, message, status, block + end + + # :ditto: + def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil, &block : Source::Corrector ->) : Issue + add_issue rule, node, message, status, block end # Adds a new issue for Crystal *token*. - def add_issue(rule, token : Crystal::Token, message, status : Issue::Status? = nil) : Issue - add_issue rule, token.location, nil, message, status + def add_issue(rule, token : Crystal::Token, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil) : Issue + add_issue rule, token.location, nil, message, status, block + end + + # :ditto: + def add_issue(rule, token : Crystal::Token, message, status : Issue::Status? = nil, &block : Source::Corrector ->) : Issue + add_issue rule, token, message, status, block end # Adds a new issue for *location* defined by line and column numbers. - def add_issue(rule, location : {Int32, Int32}, message, status : Issue::Status? = nil) : Issue + def add_issue(rule, location : {Int32, Int32}, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil) : Issue location = Crystal::Location.new(path, *location) - add_issue rule, location, nil, message, status + add_issue rule, location, nil, message, status, block + end + + # :ditto: + def add_issue(rule, location : {Int32, Int32}, message, status : Issue::Status? = nil, &block : Source::Corrector ->) : Issue + add_issue rule, location, message, status, block end # Adds a new issue for *location* and *end_location* defined by line and column numbers. - def add_issue(rule, location : {Int32, Int32}, end_location : {Int32, Int32}, message, status : Issue::Status? = nil) : Issue + def add_issue(rule, + location : {Int32, Int32}, + end_location : {Int32, Int32}, + message, + status : Issue::Status? = nil, + block : (Source::Corrector ->)? = nil) : Issue location = Crystal::Location.new(path, *location) end_location = Crystal::Location.new(path, *end_location) - add_issue rule, location, end_location, message, status + add_issue rule, location, end_location, message, status, block + end + + # :ditto: + def add_issue(rule, + location : {Int32, Int32}, + end_location : {Int32, Int32}, + message, + status : Issue::Status? = nil, + &block : Source::Corrector ->) : Issue + add_issue rule, location, end_location, message, status, block end # Returns `true` if the list of not disabled issues is empty, `false` otherwise. diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr index 7777167c..6a3090d8 100644 --- a/src/ameba/rule/base.cr +++ b/src/ameba/rule/base.cr @@ -112,8 +112,8 @@ module Ameba::Rule name.hash end - macro issue_for(*args) - source.add_issue self, {{*args}} + macro issue_for(*args, &block) + source.add_issue self, {{*args}} {{block}} end protected def self.rule_name diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr new file mode 100644 index 00000000..ae3725e7 --- /dev/null +++ b/src/ameba/source/corrector.cr @@ -0,0 +1,57 @@ +require "./rewriter" + +class Ameba::Source + class Corrector + def self.correct(source : Source) + code = source.code + corrector = new(code) + source.issues.each(&.correct(corrector)) + corrected_code = corrector.process + return if code == corrected_code + + corrected_code + end + + @line_sizes : Array(Int32) + + def initialize(code : String) + @rewriter = Rewriter.new(code) + @line_sizes = code.lines(chomp: false).map(&.size) + end + + alias SourceLocation = Crystal::Location | {Int32, Int32} + + def replace(location : SourceLocation, end_location : SourceLocation, content) + @rewriter.replace(loc_to_pos(location), loc_to_pos(end_location) + 1, content) + end + + def wrap(location : SourceLocation, end_location : SourceLocation, insert_before, insert_after) + @rewriter.wrap(loc_to_pos(location), loc_to_pos(end_location) + 1, insert_before, insert_after) + end + + def remove(location : SourceLocation, end_location : SourceLocation) + @rewriter.remove(loc_to_pos(location), loc_to_pos(end_location) + 1) + end + + def insert_before(location : SourceLocation, content) + @rewriter.insert_before(loc_to_pos(location), content) + end + + def insert_after(location : SourceLocation, content) + @rewriter.insert_after(loc_to_pos(location) + 1, content) + end + + private def loc_to_pos(location : SourceLocation) + if location.is_a?(Crystal::Location) + line, column = location.line_number, location.column_number + else + line, column = location + end + @line_sizes[0...line - 1].sum + (column - 1) + end + + def process + @rewriter.process + end + end +end diff --git a/src/ameba/source/rewriter.cr b/src/ameba/source/rewriter.cr new file mode 100644 index 00000000..62b5c65b --- /dev/null +++ b/src/ameba/source/rewriter.cr @@ -0,0 +1,132 @@ +class Ameba::Source + # This class performs the heavy lifting in the source rewriting process. + # It schedules code updates to be performed in the correct order. + # + # For simple cases, the resulting source will be obvious. + # + # Examples for more complex cases follow. Assume these examples are acting on + # the source `puts(:hello, :world)`. The methods `#wrap`, `#remove`, etc. + # receive a range as the first two arguments; for clarity, examples below use + # English sentences and a string of raw code instead. + # + # ## Overlapping deletions: + # + # * remove `:hello, ` + # * remove `, :world` + # + # The overlapping ranges are merged and `:hello, :world` will be removed. + # + # ## Multiple actions at the same end points: + # + # Results will always be independent of the order they were given. + # Exception: rewriting actions done on exactly the same range (covered next). + # + # Example: + # + # * replace `, ` by ` => ` + # * wrap `:hello, :world` with `{` and `}` + # * replace `:world` with `:everybody` + # * wrap `:world` with `[`, `]` + # + # The resulting string will be `puts({:hello => [:everybody]})` + # and this result is independent of the order the instructions were given in. + # + # ## Multiple wraps on same range: + # + # * wrap `:hello` with `(` and `)` + # * wrap `:hello` with `[` and `]` + # + # The wraps are combined in order given and results would be `puts([(:hello)], :world)`. + # + # ## Multiple replacements on same range: + # + # * replace `:hello` by `:hi`, then + # * replace `:hello` by `:hey` + # + # The replacements are made in the order given, so the latter replacement + # supersedes the former and `:hello` will be replaced by `:hey`. + # + # ## Swallowed insertions: + # + # * wrap `world` by `__`, `__` + # * replace `:hello, :world` with `:hi` + # + # A containing replacement will swallow the contained rewriting actions + # and `:hello, :world` will be replaced by `:hi`. + # + # ## Implementation + # + # The updates are organized in a tree, according to the ranges they act on + # (where children are strictly contained by their parent). + class Rewriter + getter code + + def initialize(@code : String) + @action_root = Rewriter::Action.new(0, code.size) + end + + # Returns true if no (non trivial) update has been recorded + def empty? + @action_root.empty? + end + + # Replaces the code of the given range with `content`. + def replace(begin_pos, end_pos, content) + combine(begin_pos, end_pos, replacement: content.to_s) + end + + # Inserts the given strings before and after the given range. + def wrap(begin_pos, end_pos, insert_before, insert_after) + combine(begin_pos, end_pos, insert_before: insert_before.to_s, insert_after: insert_after.to_s) + end + + # Shortcut for `replace(begin_pos, end_pos, "")` + def remove(begin_pos, end_pos) + replace(begin_pos, end_pos, "") + end + + # Shortcut for `wrap(begin_pos, end_pos, content, nil)` + def insert_before(begin_pos, end_pos, content) + wrap(begin_pos, end_pos, content, nil) + end + + # Shortcut for `wrap(begin_pos, end_pos, nil, content)` + def insert_after(begin_pos, end_pos, content) + wrap(begin_pos, end_pos, nil, content) + end + + # Shortcut for `insert_before(pos, pos, content)` + def insert_before(pos, content) + insert_before(pos, pos, content) + end + + # Shortcut for `insert_after(pos, pos, content)` + def insert_after(pos, content) + insert_after(pos, pos, content) + end + + # Applies all scheduled changes and returns modified source as a new string. + def process + String.build do |io| + last_end = 0 + @action_root.ordered_replacements.each do |begin_pos, end_pos, replacement| + io << code[last_end...begin_pos] << replacement + last_end = end_pos + end + io << code[last_end...code.size] + end + end + + protected def combine(begin_pos, end_pos, **attributes) + check_range_validity(begin_pos, end_pos) + action = Rewriter::Action.new(begin_pos, end_pos, **attributes) + @action_root = @action_root.combine(action) + end + + private def check_range_validity(begin_pos, end_pos) + if begin_pos < 0 || end_pos > code.size + raise IndexError.new("The range #{begin_pos}...#{end_pos} is outside the bounds of the source") + end + end + end +end diff --git a/src/ameba/source/rewriter/action.cr b/src/ameba/source/rewriter/action.cr new file mode 100644 index 00000000..c33e607c --- /dev/null +++ b/src/ameba/source/rewriter/action.cr @@ -0,0 +1,181 @@ +class Ameba::Source::Rewriter + # Actions are arranged in a tree and get combined so that: + # - children are strictly contained by their parent + # - siblings all disjoint from one another and ordered + # - only actions with `replacement == nil` may have children + class Action + getter begin_pos : Int32 + getter end_pos : Int32 + getter replacement : String? + getter insert_before : String + getter insert_after : String + protected getter children : Array(Action) + + def initialize(@begin_pos, + @end_pos, + @insert_before = "", + @replacement = nil, + @insert_after = "", + @children = [] of Action) + end + + def combine(action) + return self if action.empty? # Ignore empty action + + do_combine(action) + end + + def empty? + replacement = @replacement + @insert_before.empty? && + @insert_after.empty? && + @children.empty? && + (replacement.nil? || (replacement.empty? && @begin_pos == @end_pos)) + end + + def ordered_replacements + replacement = @replacement + reps = [] of {Int32, Int32, String} + reps << {@begin_pos, @begin_pos, @insert_before} unless @insert_before.empty? + reps << {@begin_pos, @end_pos, replacement} if replacement + reps.concat(@children.flat_map(&.ordered_replacements)) + reps << {@end_pos, @end_pos, @insert_after} unless @insert_after.empty? + reps + end + + def insertion? + replacement = @replacement + !@insert_before.empty? || !@insert_after.empty? || (replacement && !replacement.empty?) + end + + protected def with(*, + begin_pos = @begin_pos, + end_pos = @end_pos, + insert_before = @insert_before, + replacement = @replacement, + insert_after = @insert_after, + children = @children) + children = [] of Action if replacement + self.class.new(begin_pos, end_pos, insert_before, replacement, insert_after, children) + end + + # Assumes *action* is contained within `@begin_pos...@end_pos` and has no children + protected def do_combine(action) + if action.begin_pos == @begin_pos && action.end_pos == @end_pos + merge(action) + else + place_in_hierarchy(action) + end + end + + protected def place_in_hierarchy(action) + family = analyse_hierarchy(action) + sibling_left, sibling_right = family[:sibling_left], family[:sibling_right] + + if (fusible = family[:fusible]) + child = family[:child] + child ||= [] of Action + fuse_deletions(action, fusible, sibling_left + child + sibling_right) + else + extra_sibling = if (parent = family[:parent]) + # action should be a descendant of one of the children + parent.do_combine(action) + elsif (child = family[:child]) + # or it should become the parent of some of the children, + action.with(children: child).combine_children(action.children) + else + # or else it should become an additional child + action + end + self.with(children: sibling_left + [extra_sibling] + sibling_right) + end + end + + # Assumes *more_children* all contained within `@begin_pos...@end_pos` + protected def combine_children(more_children) + more_children.reduce(self) do |parent, new_child| + parent.place_in_hierarchy(new_child) + end + end + + protected def fuse_deletions(action, fusible, other_siblings) + without_fusible = self.with(children: other_siblings) + fusible = [action] + fusible + fused_begin_pos = fusible.map(&.begin_pos).min + fused_end_pos = fusible.map(&.end_pos).max + fused_deletion = action.with(begin_pos: fused_begin_pos, end_pos: fused_end_pos) + without_fusible.do_combine(fused_deletion) + end + + # Similar to `@children.bsearch_index || size` except allows for a starting point + protected def bsearch_child_index(from = 0) + size = @children.size + (from...size).bsearch { |i| yield @children[i] } || size + end + + # Returns the children in a hierarchy with respect to *action*: + # + # - `:sibling_left`, `:sibling_right` (for those that are disjoint from *action*) + # - `:parent` (in case one of our children contains *action*) + # - `:child` (in case *action* strictly contains some of our children) + # - `:fusible` (in case *action* overlaps some children but they can be fused in one deletion) + # + # In case a child has equal range to *action*, it is returned as `:parent` + # + # Reminder: an empty range 1...1 is considered disjoint from 1...10 + protected def analyse_hierarchy(action) # ameba:disable Metrics/CyclomaticComplexity + # left_index is the index of the first child that isn't completely to the left of action + left_index = bsearch_child_index { |child| child.end_pos > action.begin_pos } + # right_index is the index of the first child that is completely on the right of action + start = left_index == 0 ? 0 : left_index - 1 # See "corner case" below for reason of -1 + right_index = bsearch_child_index(start) { |child| child.begin_pos >= action.end_pos } + center = right_index - left_index + case center + when 0 + # All children are disjoint from action, nothing else to do + when -1 + # Corner case: if a child has empty range == action's range + # then it will appear to be both disjoint and to the left of action, + # as well as disjoint and to the right of action. + # Since ranges are equal, we return it as parent + left_index -= 1 # Fix indices, as otherwise this child would be + right_index += 1 # considered as a sibling (both left and right!) + parent = @children[left_index] + else + overlap_left = @children[left_index].begin_pos <=> action.begin_pos + overlap_right = @children[right_index - 1].end_pos <=> action.end_pos + raise "Unable to compare begin pos" if overlap_left.nil? + raise "Unable to compare end pos" if overlap_right.nil? + + # For one child to be the parent of action, we must have: + if center == 1 && overlap_left <= 0 && overlap_right >= 0 + parent = @children[left_index] + else + # Otherwise consider all non disjoint elements (center) to be contained... + contained = @children[left_index...right_index] + fusible = [] of Action + fusible << contained.shift if overlap_left < 0 # ... but check first and last one + fusible << contained.pop if overlap_right > 0 # ... for overlaps + fusible = nil if fusible.empty? + end + end + + { + parent: parent, + sibling_left: @children[0...left_index], + sibling_right: @children[right_index...@children.size], + fusible: fusible, + child: contained, + } + end + + # Assumes *action* has the exact same range and has no children + protected def merge(action) + self.with( + insert_before: "#{action.insert_before}#{insert_before}", + replacement: action.replacement || @replacement, + insert_after: "#{insert_after}#{action.insert_after}", + ).combine_children(action.children) + end + end +end From d3d3ccd7e3d69208cb023cf78f69177f911deccd 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: Sun, 24 Oct 2021 11:56:46 -0700 Subject: [PATCH 02/23] Add `expect_correction` and `expect_no_corrections` --- src/ameba/spec/annotated_source.cr | 3 ++- src/ameba/spec/expect_issue.cr | 38 +++++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/ameba/spec/annotated_source.cr b/src/ameba/spec/annotated_source.cr index 26d5271e..76081fe7 100644 --- a/src/ameba/spec/annotated_source.cr +++ b/src/ameba/spec/annotated_source.cr @@ -15,7 +15,8 @@ class Ameba::Spec::AnnotatedSource def self.parse(annotated_code) lines = [] of String annotations = [] of {Int32, String, String} - annotated_code.each_line do |code_line| + code_lines = annotated_code.split('\n') # must preserve trailing newline + code_lines.each do |code_line| if (annotation_match = ANNOTATION_PATTERN_1.match(code_line)) message_index = annotation_match.end prefix = code_line[0...message_index] diff --git a/src/ameba/spec/expect_issue.cr b/src/ameba/spec/expect_issue.cr index fd8d1871..3571b164 100644 --- a/src/ameba/spec/expect_issue.cr +++ b/src/ameba/spec/expect_issue.cr @@ -47,6 +47,8 @@ require "./util" module Ameba::Spec::ExpectIssue include Spec::Util + class_property source : Source? + def expect_issue(rules : Rule::Base | Enumerable(Rule::Base), annotated_code : String, path = "", @@ -79,6 +81,39 @@ module Ameba::Spec::ExpectIssue end end + def expect_correction(correction, *, file = __FILE__, line = __LINE__) + source = ExpectIssue.source + raise "`expect_correction` must follow `expect_issue`" unless source + + corrected_code = Source::Corrector.correct(source) # TODO: recursive + raise "Use `expect_no_corrections` if the code will not change" unless corrected_code + return if correction == corrected_code + + fail <<-MSG, file, line + Expected correction: + + #{correction} + + Got: + + #{corrected_code} + MSG + end + + def expect_no_corrections(*, file = __FILE__, line = __LINE__) + source = ExpectIssue.source + raise "`expect_no_corrections` must follow `expect_offense`" unless source + + corrected_code = Source::Corrector.correct(source) + return unless corrected_code + + fail <<-MSG, file, line + Expected no corrections, but got: + + #{corrected_code} + MSG + end + def expect_no_issues(rules : Rule::Base | Enumerable(Rule::Base), code : String, path = "", @@ -87,7 +122,7 @@ module Ameba::Spec::ExpectIssue file = __FILE__, line = __LINE__) code = normalize_code(code) if normalize - lines = code.lines + lines = code.split('\n') # must preserve trailing newline actual_annotations = actual_annotations(rules, code, path, lines) unless actual_annotations.to_s == code fail <<-MSG, file, line @@ -100,6 +135,7 @@ module Ameba::Spec::ExpectIssue private def actual_annotations(rules, code, path, lines) source = Source.new(code, path, normalize: false) # already normalized + ExpectIssue.source = source if rules.is_a?(Enumerable) rules.each(&.catch(source)) else From 99e7ccd23b5bb3a9ada2ac3de48dad4008a3c4e6 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: Sun, 24 Oct 2021 11:58:32 -0700 Subject: [PATCH 03/23] Add `--autocorrect` CLI option --- src/ameba/cli/cmd.cr | 7 +++++++ src/ameba/config.cr | 2 ++ src/ameba/formatter/dot_formatter.cr | 10 +++++++++- src/ameba/runner.cr | 15 +++++++++++++++ 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index 821f2845..e6212cde 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -8,6 +8,7 @@ module Ameba::Cli def run(args = ARGV) opts = parse_args args config = Config.load opts.config, opts.colors? + config.autocorrect = opts.autocorrect? if globs = opts.globs config.globs = globs @@ -47,6 +48,7 @@ module Ameba::Cli property? all = false property? colors = true property? without_affected_code = false + property? autocorrect = false end def parse_args(args, opts = Opts.new) @@ -89,6 +91,10 @@ module Ameba::Cli opts.all = true end + parser.on("-a", "--autocorrect", "Autocorrect issues") do + opts.autocorrect = true + end + parser.on("--gen-config", "Generate a configuration file acting as a TODO list") do opts.formatter = :todo @@ -133,6 +139,7 @@ module Ameba::Cli if name = opts.formatter config.formatter = name end + config.formatter.config[:autocorrect] = opts.autocorrect? config.formatter.config[:without_affected_code] = opts.without_affected_code? end diff --git a/src/ameba/config.cr b/src/ameba/config.cr index 381f8725..ba9f6889 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -54,6 +54,8 @@ class Ameba::Config # ``` property excluded : Array(String) + property? autocorrect = false + @rule_groups : Hash(String, Array(Rule::Base)) # Creates a new instance of `Ameba::Config` based on YAML parameters. diff --git a/src/ameba/formatter/dot_formatter.cr b/src/ameba/formatter/dot_formatter.cr index 121ea1cf..91854b6c 100644 --- a/src/ameba/formatter/dot_formatter.cr +++ b/src/ameba/formatter/dot_formatter.cr @@ -36,7 +36,15 @@ module Ameba::Formatter next if issue.disabled? next if (location = issue.location).nil? - output.puts location.colorize(:cyan) + output.print location.colorize(:cyan) + if issue.correctable? + if config[:autocorrect]? + output.print " [Corrected]".colorize(:green) + else + output.print " [Correctable]".colorize(:yellow) + end + end + output.puts output.puts \ "[#{issue.rule.severity.symbol}] " \ "#{issue.rule.name}: " \ diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index 56b06e64..0bc33f08 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -29,6 +29,8 @@ module Ameba # Checks for unneeded disable directives. Always inspects a source last @unneeded_disable_directive_rule : Rule::Base? + private getter? autocorrect : Bool + # Instantiates a runner using a `config`. # # ``` @@ -43,6 +45,7 @@ module Ameba @formatter = config.formatter @severity = config.severity @rules = config.rules.select(&.enabled).reject!(&.special?) + @autocorrect = config.autocorrect? @unneeded_disable_directive_rule = config.rules @@ -50,6 +53,7 @@ module Ameba end protected def initialize(@rules, @sources, @formatter, @severity) + @autocorrect = false end # Performs the inspection. Iterates through all sources and test it using @@ -89,12 +93,16 @@ module Ameba private def run_source(source) @formatter.source_started source + # TODO: run autocorrection recursively. A new `Issue#source` property must + # be added so that `affected_code` will return the code from the old + # source instead of the autocorrected one. if @syntax_rule.catch(source).valid? @rules.each do |rule| next if rule.excluded?(source) rule.test(source) end check_unneeded_directives(source) + autocorrect(source) if autocorrect? end @formatter.source_finished source @@ -135,5 +143,12 @@ module Ameba rule.test(source) end end + + private def autocorrect(source) + corrected_code = Source::Corrector.correct(source) + return unless corrected_code + + File.write(source.path, corrected_code) + end end end From e5fb0526e0ead371d50e9cdb2b14a93fcb0ad868 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, 23 Oct 2021 02:09:04 -0700 Subject: [PATCH 04/23] Autocorrect `Style/LargeNumbers` --- spec/ameba/rule/style/large_numbers_spec.cr | 8 ++++++-- src/ameba/rule/style/large_numbers.cr | 8 +++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/spec/ameba/rule/style/large_numbers_spec.cr b/spec/ameba/rule/style/large_numbers_spec.cr index 87fe95c6..f29884b5 100644 --- a/spec/ameba/rule/style/large_numbers_spec.cr +++ b/spec/ameba/rule/style/large_numbers_spec.cr @@ -9,7 +9,11 @@ module Ameba expect_issue rule, <<-CRYSTAL, number: number number = %{number} - # ^{number} error: Large numbers should be written with underscores: #{expected} + # ^{number} error: Large numbers should be written with underscores. + CRYSTAL + + expect_correction <<-CRYSTAL + number = #{expected} CRYSTAL end end @@ -122,7 +126,7 @@ module Ameba issue.rule.should_not be_nil issue.location.to_s.should eq "source.cr:1:1" issue.end_location.to_s.should eq "source.cr:1:7" - issue.message.should match /1_200_000/ + issue.message.should eq "Large numbers should be written with underscores." end context "properties" do diff --git a/src/ameba/rule/style/large_numbers.cr b/src/ameba/rule/style/large_numbers.cr index 8608abe3..2f72ff1f 100644 --- a/src/ameba/rule/style/large_numbers.cr +++ b/src/ameba/rule/style/large_numbers.cr @@ -28,12 +28,12 @@ module Ameba::Rule::Style # ``` class LargeNumbers < Base properties do - enabled false + enabled true description "Disallows usage of large numbers without underscore" int_min_digits 5 end - MSG = "Large numbers should be written with underscores: %s" + MSG = "Large numbers should be written with underscores." def test(source) Tokenizer.new(source).run do |token| @@ -48,7 +48,9 @@ module Ameba::Rule::Style location.line_number, location.column_number + token.raw.size - 1 ) - issue_for location, end_location, MSG % expected + issue_for location, end_location, MSG do |corrector| + corrector.replace(location, end_location, expected) + end end end end From 573881cb8ae5dcedc5f084e40e9269a89dc84135 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: Sun, 24 Oct 2021 11:58:51 -0700 Subject: [PATCH 05/23] Autocorrect `Layout/TrailingBlankLines` partially --- .../rule/layout/trailing_blank_lines_spec.cr | 22 +++++++++---------- src/ameba/rule/layout/trailing_blank_lines.cr | 8 ++++++- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/spec/ameba/rule/layout/trailing_blank_lines_spec.cr b/spec/ameba/rule/layout/trailing_blank_lines_spec.cr index 4f42f9a6..2b98946a 100644 --- a/spec/ameba/rule/layout/trailing_blank_lines_spec.cr +++ b/spec/ameba/rule/layout/trailing_blank_lines_spec.cr @@ -5,28 +5,26 @@ module Ameba::Rule::Layout describe TrailingBlankLines do it "passes if there is a blank line at the end of a source" do - source = Source.new "a = 1\n", normalize: false - subject.catch(source).should be_valid + expect_no_issues subject, "a = 1\n", normalize: false end it "passes if source is empty" do - source = Source.new "" - subject.catch(source).should be_valid + expect_no_issues subject, "" end it "fails if there is no blank lines at the end" do - source = Source.new "no-blankline" - subject.catch(source).should_not be_valid + expect_issue subject, "no-blankline # error: Trailing newline missing" + expect_correction "no-blankline\n" end it "fails if there more then one blank line at the end of a source" do - source = Source.new "a = 1\n \n", normalize: false - subject.catch(source).should_not be_valid + expect_issue subject, "a = 1\n \n # error: Excessive trailing newline detected", normalize: false + expect_no_corrections end it "fails if last line is not blank" do - source = Source.new "\n\n\n puts 22", normalize: false - subject.catch(source).should_not be_valid + expect_issue subject, "\n\n\n puts 22 # error: Trailing newline missing", normalize: false + expect_correction "\n\n\n puts 22\n" end context "when unnecessary blank line has been detected" do @@ -36,7 +34,7 @@ module Ameba::Rule::Layout issue = source.issues.first issue.rule.should_not be_nil - issue.location.to_s.should eq "source.cr:2:1" + issue.location.to_s.should eq "source.cr:3:1" issue.end_location.should be_nil issue.message.should eq "Excessive trailing newline detected" end @@ -49,7 +47,7 @@ module Ameba::Rule::Layout issue = source.issues.first issue.rule.should_not be_nil - issue.location.to_s.should eq "source.cr:0:1" + issue.location.to_s.should eq "source.cr:1:1" issue.end_location.should be_nil issue.message.should eq "Trailing newline missing" end diff --git a/src/ameba/rule/layout/trailing_blank_lines.cr b/src/ameba/rule/layout/trailing_blank_lines.cr index a133ebb8..63e5327b 100644 --- a/src/ameba/rule/layout/trailing_blank_lines.cr +++ b/src/ameba/rule/layout/trailing_blank_lines.cr @@ -25,7 +25,13 @@ module Ameba::Rule::Layout last_line_empty = last_source_line.empty? if source_lines_size >= 1 && (source_lines.last(2).join.blank? || !last_line_empty) - issue_for({source_lines_size - 1, 1}, last_line_empty ? MSG : MSG_FINAL_NEWLINE) + if last_line_empty + issue_for({source_lines_size, 1}, MSG) + else + issue_for({source_lines_size, 1}, MSG_FINAL_NEWLINE) do |corrector| + corrector.insert_before({source_lines_size + 1, 1}, '\n') + end + end end end end From f39a7a4cd4dd5a0d8ec227424b39702ce363a495 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, 25 Oct 2021 15:09:39 -0700 Subject: [PATCH 06/23] Re-run autocorrect until all correctable issues have been corrected --- spec/ameba/formatter/util_spec.cr | 16 ++++++------- spec/ameba/issue_spec.cr | 15 ++++++++---- spec/ameba/spec/annotated_source_spec.cr | 6 +++-- src/ameba/formatter/dot_formatter.cr | 2 +- src/ameba/formatter/explain_formatter.cr | 7 ++---- src/ameba/formatter/util.cr | 10 ++++++-- src/ameba/issue.cr | 5 +++- src/ameba/reportable.cr | 2 +- src/ameba/runner.cr | 23 +++++++++--------- src/ameba/source.cr | 30 +++++++++++++++++++----- src/ameba/source/corrector.cr | 10 -------- src/ameba/spec/expect_issue.cr | 12 ++++------ 12 files changed, 78 insertions(+), 60 deletions(-) diff --git a/spec/ameba/formatter/util_spec.cr b/spec/ameba/formatter/util_spec.cr index 9087228f..0ddcfb4b 100644 --- a/spec/ameba/formatter/util_spec.cr +++ b/spec/ameba/formatter/util_spec.cr @@ -69,24 +69,24 @@ module Ameba::Formatter describe "#affected_code" do it "returns nil if there is no such a line number" do - source = Source.new %( + code = <<-EOF a = 1 - ) + EOF location = Crystal::Location.new("filename", 2, 1) - subject.affected_code(source, location).should be_nil + subject.affected_code(code, location).should be_nil end it "returns correct line if it is found" do - source = Source.new %( + code = <<-EOF a = 1 - ) + EOF location = Crystal::Location.new("filename", 1, 1) - subject.deansify(subject.affected_code(source, location)) + subject.deansify(subject.affected_code(code, location)) .should eq "> a = 1\n ^\n" end it "returns correct line if it is found" do - source = Source.new <<-EOF + code = <<-EOF # pre:1 # pre:2 # pre:3 @@ -101,7 +101,7 @@ module Ameba::Formatter EOF location = Crystal::Location.new("filename", 6, 1) - subject.deansify(subject.affected_code(source, location, context_lines: 3)) + subject.deansify(subject.affected_code(code, location, context_lines: 3)) .should eq <<-STR > # pre:3 > # pre:4 diff --git a/spec/ameba/issue_spec.cr b/spec/ameba/issue_spec.cr index b6488552..08188eaf 100644 --- a/spec/ameba/issue_spec.cr +++ b/spec/ameba/issue_spec.cr @@ -3,7 +3,8 @@ require "../spec_helper" module Ameba describe Issue do it "accepts rule and message" do - issue = Issue.new rule: DummyRule.new, + issue = Issue.new code: "", + rule: DummyRule.new, location: nil, end_location: nil, message: "Blah", @@ -15,7 +16,8 @@ module Ameba it "accepts location" do location = Crystal::Location.new("path", 3, 2) - issue = Issue.new rule: DummyRule.new, + issue = Issue.new code: "", + rule: DummyRule.new, location: location, end_location: nil, message: "Blah", @@ -27,7 +29,8 @@ module Ameba it "accepts end_location" do location = Crystal::Location.new("path", 3, 2) - issue = Issue.new rule: DummyRule.new, + issue = Issue.new code: "", + rule: DummyRule.new, location: nil, end_location: location, message: "Blah", @@ -38,7 +41,8 @@ module Ameba end it "accepts status" do - issue = Issue.new rule: DummyRule.new, + issue = Issue.new code: "", + rule: DummyRule.new, location: nil, end_location: nil, message: "", @@ -50,7 +54,8 @@ module Ameba end it "sets status to :enabled by default" do - issue = Issue.new rule: DummyRule.new, + issue = Issue.new code: "", + rule: DummyRule.new, location: nil, end_location: nil, message: "" diff --git a/spec/ameba/spec/annotated_source_spec.cr b/spec/ameba/spec/annotated_source_spec.cr index 9f801f46..538f054b 100644 --- a/spec/ameba/spec/annotated_source_spec.cr +++ b/spec/ameba/spec/annotated_source_spec.cr @@ -1,6 +1,7 @@ require "../../spec_helper" -private def dummy_issue(message, +private def dummy_issue(code, + message, position : {Int32, Int32}?, end_position : {Int32, Int32}?, path = "") @@ -9,6 +10,7 @@ private def dummy_issue(message, end_location = Crystal::Location.new(path, *end_position) if end_position Ameba::Issue.new( + code: code, rule: Ameba::DummyRule.new, location: location, end_location: end_location, @@ -25,7 +27,7 @@ private def expect_invalid_location(code, expect_raises Exception, exception_message, file, line do Ameba::Spec::AnnotatedSource.new( lines: code.lines, - issues: [dummy_issue("Message", position, end_position, "path")] + issues: [dummy_issue(code, "Message", position, end_position, "path")] ) end end diff --git a/src/ameba/formatter/dot_formatter.cr b/src/ameba/formatter/dot_formatter.cr index 91854b6c..9f998ecd 100644 --- a/src/ameba/formatter/dot_formatter.cr +++ b/src/ameba/formatter/dot_formatter.cr @@ -50,7 +50,7 @@ module Ameba::Formatter "#{issue.rule.name}: " \ "#{issue.message}".colorize(:red) - if show_affected_code && (code = affected_code(source, location, issue.end_location)) + if show_affected_code && (code = affected_code(issue)) output << code.colorize(:default) end diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr index f8fe87c1..33c8b7d5 100644 --- a/src/ameba/formatter/explain_formatter.cr +++ b/src/ameba/formatter/explain_formatter.cr @@ -38,10 +38,7 @@ module Ameba::Formatter private def explain(source, issue) rule = issue.rule - location, end_location = - issue.location, issue.end_location - - return unless location + return unless (location = issue.location) output_title "ISSUE INFO" output_paragraph [ @@ -49,7 +46,7 @@ module Ameba::Formatter location.to_s.colorize(:cyan).to_s, ] - if affected_code = affected_code(source, location, end_location, context_lines: 3) + if affected_code = affected_code(issue, context_lines: 3) output_title "AFFECTED CODE" output_paragraph affected_code end diff --git a/src/ameba/formatter/util.cr b/src/ameba/formatter/util.cr index d6c411c8..cbf0907c 100644 --- a/src/ameba/formatter/util.cr +++ b/src/ameba/formatter/util.cr @@ -40,8 +40,14 @@ module Ameba::Formatter {pre_context, post_context} end - def affected_code(source, location, end_location = nil, context_lines = 0, max_length = 120, ellipsis = " ...", prompt = "> ") - lines = source.lines + def affected_code(issue : Issue, context_lines = 0, max_length = 120, ellipsis = " ...", prompt = "> ") + return unless (location = issue.location) + + affected_code(issue.code, location, issue.end_location, context_lines, max_length, ellipsis, prompt) + end + + def affected_code(code, location, end_location = nil, context_lines = 0, max_length = 120, ellipsis = " ...", prompt = "> ") + lines = code.split('\n') # must preserve trailing newline lineno, column = location.line_number, location.column_number diff --git a/src/ameba/issue.cr b/src/ameba/issue.cr index 06619686..b6fa55e8 100644 --- a/src/ameba/issue.cr +++ b/src/ameba/issue.cr @@ -6,6 +6,9 @@ module Ameba Disabled end + # The source code that triggered this issue. + getter code : String + # A rule that triggers this issue. getter rule : Rule::Base @@ -24,7 +27,7 @@ module Ameba delegate :enabled?, :disabled?, to: status - def initialize(@rule, @location, @end_location, @message, status : Status? = nil, @block : (Source::Corrector ->)? = nil) + def initialize(@code, @rule, @location, @end_location, @message, status : Status? = nil, @block : (Source::Corrector ->)? = nil) @status = status || Status::Enabled end diff --git a/src/ameba/reportable.cr b/src/ameba/reportable.cr index a85bf28b..ec9e77a5 100644 --- a/src/ameba/reportable.cr +++ b/src/ameba/reportable.cr @@ -9,7 +9,7 @@ module Ameba status ||= Issue::Status::Disabled if location_disabled?(location, rule) - Issue.new(rule, location, end_location, message, status, block).tap do |issue| + Issue.new(code, rule, location, end_location, message, status, block).tap do |issue| issues << issue end end diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index 0bc33f08..b6820e9a 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -93,17 +93,23 @@ module Ameba private def run_source(source) @formatter.source_started source - # TODO: run autocorrection recursively. A new `Issue#source` property must - # be added so that `affected_code` will return the code from the old - # source instead of the autocorrected one. - if @syntax_rule.catch(source).valid? + corrected_issues = [] of Issue + loop do + @syntax_rule.test(source) + break unless source.valid? + @rules.each do |rule| next if rule.excluded?(source) rule.test(source) end check_unneeded_directives(source) - autocorrect(source) if autocorrect? + break unless autocorrect? && source.correct + + corrected_issues += source.issues.select(&.correctable?) + source.issues.clear end + corrected_issues.reverse_each { |issue| source.issues.unshift(issue) } + File.write(source.path, source.code) if corrected_issues.any? @formatter.source_finished source end @@ -143,12 +149,5 @@ module Ameba rule.test(source) end end - - private def autocorrect(source) - corrected_code = Source::Corrector.correct(source) - return unless corrected_code - - File.write(source.path, corrected_code) - end end end diff --git a/src/ameba/source.cr b/src/ameba/source.cr index 68dd2a1c..ccd71af1 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -11,6 +11,9 @@ module Ameba # Crystal code (content of a source file). getter code : String + @lines : Array(String)? + @ast : Crystal::ASTNode? + # Creates a new source by `code` and `path`. # # For example: @@ -22,6 +25,18 @@ module Ameba def initialize(@code, @path = "") end + def correct + corrector = Corrector.new(code) + issues.each(&.correct(corrector)) + corrected_code = corrector.process + return false if code == corrected_code + + @code = corrected_code + @lines = nil + @ast = nil + true + end + # Returns lines of code splitted by new line character. # Since `code` is immutable and can't be changed, this # method caches lines in an instance variable, so calling @@ -33,7 +48,9 @@ module Ameba # source.lines # => ["a = 1", "b = 2"] # ``` # - getter lines : Array(String) { code.split('\n') } + def lines : Array(String) + @lines ||= code.split('\n') + end # Returns AST nodes constructed by `Crystal::Parser`. # @@ -42,11 +59,12 @@ module Ameba # source.ast # ``` # - getter ast : Crystal::ASTNode do - Crystal::Parser.new(code) - .tap(&.wants_doc = true) - .tap(&.filename = path) - .parse + def ast : Crystal::ASTNode + @ast ||= + Crystal::Parser.new(code) + .tap(&.wants_doc = true) + .tap(&.filename = path) + .parse end getter fullpath : String do diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr index ae3725e7..a8d29446 100644 --- a/src/ameba/source/corrector.cr +++ b/src/ameba/source/corrector.cr @@ -2,16 +2,6 @@ require "./rewriter" class Ameba::Source class Corrector - def self.correct(source : Source) - code = source.code - corrector = new(code) - source.issues.each(&.correct(corrector)) - corrected_code = corrector.process - return if code == corrected_code - - corrected_code - end - @line_sizes : Array(Int32) def initialize(code : String) diff --git a/src/ameba/spec/expect_issue.cr b/src/ameba/spec/expect_issue.cr index 3571b164..7e9872ed 100644 --- a/src/ameba/spec/expect_issue.cr +++ b/src/ameba/spec/expect_issue.cr @@ -85,9 +85,8 @@ module Ameba::Spec::ExpectIssue source = ExpectIssue.source raise "`expect_correction` must follow `expect_issue`" unless source - corrected_code = Source::Corrector.correct(source) # TODO: recursive - raise "Use `expect_no_corrections` if the code will not change" unless corrected_code - return if correction == corrected_code + raise "Use `expect_no_corrections` if the code will not change" unless source.correct + return if correction == source.code fail <<-MSG, file, line Expected correction: @@ -96,7 +95,7 @@ module Ameba::Spec::ExpectIssue Got: - #{corrected_code} + #{source.code} MSG end @@ -104,13 +103,12 @@ module Ameba::Spec::ExpectIssue source = ExpectIssue.source raise "`expect_no_corrections` must follow `expect_offense`" unless source - corrected_code = Source::Corrector.correct(source) - return unless corrected_code + return unless source.correct fail <<-MSG, file, line Expected no corrections, but got: - #{corrected_code} + #{source.code} MSG end From 1d5f554e88a875b92a38729a69ecc2a32f6a7ce1 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, 26 Oct 2021 08:17:06 -0700 Subject: [PATCH 07/23] Apply suggestions from code review --- spec/ameba/rule/style/large_numbers_spec.cr | 4 ++-- src/ameba/cli/cmd.cr | 2 +- src/ameba/rule/style/large_numbers.cr | 4 ++-- src/ameba/runner.cr | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/ameba/rule/style/large_numbers_spec.cr b/spec/ameba/rule/style/large_numbers_spec.cr index f29884b5..ff950b2e 100644 --- a/spec/ameba/rule/style/large_numbers_spec.cr +++ b/spec/ameba/rule/style/large_numbers_spec.cr @@ -9,7 +9,7 @@ module Ameba expect_issue rule, <<-CRYSTAL, number: number number = %{number} - # ^{number} error: Large numbers should be written with underscores. + # ^{number} error: Large numbers should be written with underscores: #{expected} CRYSTAL expect_correction <<-CRYSTAL @@ -126,7 +126,7 @@ module Ameba issue.rule.should_not be_nil issue.location.to_s.should eq "source.cr:1:1" issue.end_location.to_s.should eq "source.cr:1:7" - issue.message.should eq "Large numbers should be written with underscores." + issue.message.should match /1_200_000/ end context "properties" do diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index e6212cde..42238e30 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -91,7 +91,7 @@ module Ameba::Cli opts.all = true end - parser.on("-a", "--autocorrect", "Autocorrect issues") do + parser.on("--fix", "Autocorrect issues") do opts.autocorrect = true end diff --git a/src/ameba/rule/style/large_numbers.cr b/src/ameba/rule/style/large_numbers.cr index 2f72ff1f..4c2de7ff 100644 --- a/src/ameba/rule/style/large_numbers.cr +++ b/src/ameba/rule/style/large_numbers.cr @@ -33,7 +33,7 @@ module Ameba::Rule::Style int_min_digits 5 end - MSG = "Large numbers should be written with underscores." + MSG = "Large numbers should be written with underscores: %s" def test(source) Tokenizer.new(source).run do |token| @@ -48,7 +48,7 @@ module Ameba::Rule::Style location.line_number, location.column_number + token.raw.size - 1 ) - issue_for location, end_location, MSG do |corrector| + issue_for location, end_location, MSG % expected do |corrector| corrector.replace(location, end_location, expected) end end diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index b6820e9a..429e7bd5 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -109,7 +109,7 @@ module Ameba source.issues.clear end corrected_issues.reverse_each { |issue| source.issues.unshift(issue) } - File.write(source.path, source.code) if corrected_issues.any? + File.write(source.path, source.code) unless corrected_issues.empty? @formatter.source_finished source end From f87d99a83bf173dbe274b4cb59f32eaf2839adbc 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, 26 Oct 2021 12:01:14 -0700 Subject: [PATCH 08/23] Add `Corrector` method overloads that accept `ASTNode` --- src/ameba/source/corrector.cr | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr index a8d29446..b7dd3e3b 100644 --- a/src/ameba/source/corrector.cr +++ b/src/ameba/source/corrector.cr @@ -40,6 +40,34 @@ class Ameba::Source @line_sizes[0...line - 1].sum + (column - 1) end + def replace(node : Crystal::ASTNode, content) + replace(location(node), end_location(node), content) + end + + def wrap(node : Crystal::ASTNode, insert_before, insert_after) + wrap(location(node), end_location(node), insert_before, insert_after) + end + + def remove(node : Crystal::ASTNode) + remove(location(node), end_location(node)) + end + + def insert_before(node : Crystal::ASTNode, content) + insert_before(location(node), content) + end + + def insert_after(node : Crystal::ASTNode, content) + insert_after(end_location(node), content) + end + + private def location(node : Crystal::ASTNode) + node.location || raise "Missing location" + end + + private def end_location(node : Crystal::ASTNode) + node.end_location || raise "Missing end location" + end + def process @rewriter.process end From e8c0f49cb8438568244b13aeff6ed32f6da15a36 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, 26 Oct 2021 21:07:30 -0700 Subject: [PATCH 09/23] Delete redundant alias `SourceLocation` --- src/ameba/source/corrector.cr | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr index b7dd3e3b..fbb50f8a 100644 --- a/src/ameba/source/corrector.cr +++ b/src/ameba/source/corrector.cr @@ -9,29 +9,27 @@ class Ameba::Source @line_sizes = code.lines(chomp: false).map(&.size) end - alias SourceLocation = Crystal::Location | {Int32, Int32} - - def replace(location : SourceLocation, end_location : SourceLocation, content) + def replace(location, end_location, content) @rewriter.replace(loc_to_pos(location), loc_to_pos(end_location) + 1, content) end - def wrap(location : SourceLocation, end_location : SourceLocation, insert_before, insert_after) + 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 - def remove(location : SourceLocation, end_location : SourceLocation) + def remove(location, end_location) @rewriter.remove(loc_to_pos(location), loc_to_pos(end_location) + 1) end - def insert_before(location : SourceLocation, content) + def insert_before(location, content) @rewriter.insert_before(loc_to_pos(location), content) end - def insert_after(location : SourceLocation, content) + def insert_after(location, content) @rewriter.insert_after(loc_to_pos(location) + 1, content) end - private def loc_to_pos(location : SourceLocation) + private def loc_to_pos(location : Crystal::Location | {Int32, Int32}) if location.is_a?(Crystal::Location) line, column = location.line_number, location.column_number else From 3b11491ceaa75f7513e08d70ecf5d1bb8d5b9308 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, 26 Oct 2021 21:09:32 -0700 Subject: [PATCH 10/23] Add `insert_*` overloads that accept both location and end_location --- src/ameba/source/corrector.cr | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr index fbb50f8a..f4adf1da 100644 --- a/src/ameba/source/corrector.cr +++ b/src/ameba/source/corrector.cr @@ -21,6 +21,14 @@ class Ameba::Source @rewriter.remove(loc_to_pos(location), loc_to_pos(end_location) + 1) end + def insert_before(location, end_location, content) + @rewriter.insert_before(loc_to_pos(location), loc_to_pos(end_location) + 1, content) + end + + def insert_after(location, end_location, content) + @rewriter.insert_after(loc_to_pos(location), loc_to_pos(end_location) + 1, content) + end + def insert_before(location, content) @rewriter.insert_before(loc_to_pos(location), content) end From 16608965f5d2e00e036a0b7bc9afde005d0492a6 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, 26 Oct 2021 12:02:17 -0700 Subject: [PATCH 11/23] Allowed named arguments with `Rule::Base.issue_for` --- src/ameba/rule/base.cr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr index 6a3090d8..0ae7016e 100644 --- a/src/ameba/rule/base.cr +++ b/src/ameba/rule/base.cr @@ -112,8 +112,8 @@ module Ameba::Rule name.hash end - macro issue_for(*args, &block) - source.add_issue self, {{*args}} {{block}} + macro issue_for(*args, **kwargs, &block) + source.add_issue(self, {{*args}}, {{**kwargs}}) {{block}} end protected def self.rule_name From 437584f9dbb834251e95e5ee81c7badf0daa0e7e 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, 26 Oct 2021 13:05:22 -0700 Subject: [PATCH 12/23] Raise error if infinite correction loop --- spec/ameba/runner_spec.cr | 67 ++++++++++++++++++++ spec/spec_helper.cr | 127 ++++++++++++++++++++++++++++++++++++++ src/ameba/runner.cr | 90 ++++++++++++++++++++++++--- 3 files changed, 277 insertions(+), 7 deletions(-) diff --git a/spec/ameba/runner_spec.cr b/spec/ameba/runner_spec.cr index 703f39ee..f5ec70c7 100644 --- a/spec/ameba/runner_spec.cr +++ b/spec/ameba/runner_spec.cr @@ -8,6 +8,13 @@ module Ameba config.update_rule ErrorRule.rule_name, enabled: false config.update_rule PerfRule.rule_name, enabled: false + config.update_rule AtoAA.rule_name, enabled: false + config.update_rule AtoB.rule_name, enabled: false + config.update_rule BtoA.rule_name, enabled: false + config.update_rule BtoC.rule_name, enabled: false + config.update_rule CtoA.rule_name, enabled: false + config.update_rule ClassToModule.rule_name, enabled: false + config.update_rule ModuleToClass.rule_name, enabled: false Runner.new(config) end @@ -54,6 +61,15 @@ module Ameba Runner.new(all_rules, [source], formatter, default_severity).run.success?.should be_true end + it "aborts because of an infinite loop" do + rules = [AtoAA.new] of Rule::Base + source = Source.new "class A; end", "source.cr" + message = "Infinite loop in source.cr caused by Ameba/AtoAA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + context "exception in rule" do it "raises an exception raised in fiber while running a rule" do rule = RaiseRule.new @@ -180,5 +196,56 @@ module Ameba .run.success?.should be_true end end + + describe "#run with rules autocorrecting each other" do + context "with two conflicting rules" do + context "if there is an offense in an inspected file" do + it "aborts because of an infinite loop" do + rules = [AtoB.new, BtoA.new] + source = Source.new "class A; end", "source.cr" + message = "Infinite loop in source.cr caused by Ameba/AtoB -> Ameba/BtoA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + end + + context "if there are multiple offenses in an inspected file" do + it "aborts because of an infinite loop" do + rules = [AtoB.new, BtoA.new] + source = Source.new %( + class A; end + class A_A; end + ), "source.cr" + message = "Infinite loop in source.cr caused by Ameba/AtoB -> Ameba/BtoA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + end + end + + context "with two pairs of conflicting rules" do + it "aborts because of an infinite loop" do + rules = [ClassToModule.new, ModuleToClass.new, AtoB.new, BtoA.new] + source = Source.new "class A_A; end", "source.cr" + message = "Infinite loop in source.cr caused by Ameba/ClassToModule, Ameba/AtoB -> Ameba/ModuleToClass, Ameba/BtoA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + end + + context "with three rule cycle" do + it "aborts because of an infinite loop" do + rules = [AtoB.new, BtoC.new, CtoA.new] + source = Source.new "class A; end", "source.cr" + message = "Infinite loop in source.cr caused by Ameba/AtoB -> Ameba/BtoC -> Ameba/CtoA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + end + end end end diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index c468bed5..aa489fca 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -95,6 +95,133 @@ module Ameba end end + class AtoAA < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("A") + + issue_for(node.name, message: "A to AA") do |corrector| + corrector.replace(node.name, name.sub("A", "AA")) + end + end + end + + class AtoB < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("A") + + issue_for(node.name, message: "A to B") do |corrector| + corrector.replace(node.name, name.tr("A", "B")) + end + end + end + + class BtoA < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("B") + + issue_for(node.name, message: "B to A") do |corrector| + corrector.replace(node.name, name.tr("B", "A")) + end + end + end + + class BtoC < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("B") + + issue_for(node.name, message: "B to C") do |corrector| + corrector.replace(node.name, name.tr("B", "C")) + end + end + end + + class CtoA < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("C") + + issue_for(node.name, message: "C to A") do |corrector| + corrector.replace(node.name, name.tr("C", "A")) + end + end + end + + class ClassToModule < Ameba::Rule::Base + include Ameba::AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef) + return unless (location = node.location) + + end_location = Crystal::Location.new( + location.filename, + location.line_number, + location.column_number + "class".size - 1 + ) + issue_for(location, end_location, message: "class to module") do |corrector| + corrector.replace(location, end_location, "module") + end + end + end + + class ModuleToClass < Ameba::Rule::Base + include Ameba::AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ModuleDef) + return unless (location = node.location) + + end_location = Crystal::Location.new( + location.filename, + location.line_number, + location.column_number + "module".size - 1 + ) + issue_for(location, end_location, message: "module to class") do |corrector| + corrector.replace(location, end_location, "class") + end + end + end + class DummyFormatter < Formatter::BaseFormatter property started_sources : Array(Source)? property finished_sources : Array(Source)? diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index 429e7bd5..c22ad752 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -1,3 +1,5 @@ +require "digest" + module Ameba # Represents a runner for inspecting sources files. # Holds a list of rules to do inspection based on, @@ -11,6 +13,25 @@ module Ameba # ``` # class Runner + # An error indicating that the inspection loop got stuck correcting + # issues back and forth. + class InfiniteCorrectionLoopError < RuntimeError + def initialize(path, issues_by_iteration, loop_start = -1) + root_cause = + issues_by_iteration[loop_start..-1] + .map(&.map(&.rule.name).uniq!.join(", ")) + .join(" -> ") + + message = String.build do |io| + io << "Infinite loop" + io << " in " << path unless path.empty? + io << " caused by " << root_cause + end + + super message + end + end + # A list of rules to do inspection based on. @rules : Array(Rule::Base) @@ -52,8 +73,7 @@ module Ameba .find &.name.==(Rule::Lint::UnneededDisableDirective.rule_name) end - protected def initialize(@rules, @sources, @formatter, @severity) - @autocorrect = false + protected def initialize(@rules, @sources, @formatter, @severity, @autocorrect = false) end # Performs the inspection. Iterates through all sources and test it using @@ -93,8 +113,18 @@ module Ameba private def run_source(source) @formatter.source_started source - corrected_issues = [] of Issue - loop do + # This variable is a 2D array used to track corrected issues after each + # inspection iteration. This is used to output meaningful infinite loop + # error message. + corrected_issues = [] of Array(Issue) + + # When running with --fix, we need to inspect the source until no more + # corrections are made (because automatic corrections can introduce new + # issues). In the normal case the loop is only executed once. + loop_unless_infinite(source, corrected_issues) do + # We have to reprocess the source to pick up any changes. Since a + # change could (theoretically) introduce syntax errors, we break the + # loop if we find any. @syntax_rule.test(source) break unless source.valid? @@ -105,12 +135,18 @@ module Ameba check_unneeded_directives(source) break unless autocorrect? && source.correct - corrected_issues += source.issues.select(&.correctable?) + # The issues that couldn't be corrected will be found again so we + # only keep the corrected ones in order to avoid duplicate reporting. + corrected_issues << source.issues.select(&.correctable?) source.issues.clear end - corrected_issues.reverse_each { |issue| source.issues.unshift(issue) } - File.write(source.path, source.code) unless corrected_issues.empty? + corrected_issues.flatten.reverse_each do |issue| + source.issues.unshift(issue) + end + + File.write(source.path, source.code) unless corrected_issues.empty? + ensure @formatter.source_finished source end @@ -144,6 +180,46 @@ module Ameba end end + private MAX_ITERATIONS = 200 + + private def loop_unless_infinite(source, corrected_issues) + # Keep track of the state of the source. If a rule modifies the source + # and another rule undoes it producing identical source we have an + # infinite loop. + processed_sources = [] of String + + # It is possible for a rule to keep adding indefinitely to a file, + # making it bigger and bigger. If the inspection loop runs for an + # excessively high number of iterations, this is likely happening. + iterations = 0 + + loop do + check_for_infinite_loop(source, corrected_issues, processed_sources) + + if (iterations += 1) > MAX_ITERATIONS + raise InfiniteCorrectionLoopError.new(source.path, corrected_issues) + end + + yield + end + end + + # Check whether a run created source identical to a previous run, which + # means that we definitely have an infinite loop. + private def check_for_infinite_loop(source, corrected_issues, processed_sources) + checksum = Digest::SHA1.hexdigest(source.code) + + if (loop_start = processed_sources.index(checksum)) + raise InfiniteCorrectionLoopError.new( + source.path, + corrected_issues, + loop_start: loop_start + ) + end + + processed_sources << checksum + end + private def check_unneeded_directives(source) if (rule = @unneeded_disable_directive_rule) && rule.enabled rule.test(source) From 749c53527ef6ce4e6262b5fed4fb570bb7d89940 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, 26 Oct 2021 21:03:30 -0700 Subject: [PATCH 13/23] Add documentation --- src/ameba/config.cr | 1 + src/ameba/runner.cr | 1 + src/ameba/source.cr | 2 ++ src/ameba/source/corrector.cr | 15 +++++++++++++++ src/ameba/source/rewriter.cr | 2 +- src/ameba/source/rewriter/action.cr | 1 + 6 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/ameba/config.cr b/src/ameba/config.cr index ba9f6889..91fad3cc 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -54,6 +54,7 @@ class Ameba::Config # ``` property excluded : Array(String) + # Returns true if correctable issues should be autocorrected. property? autocorrect = false @rule_groups : Hash(String, Array(Rule::Base)) diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index c22ad752..3cccff4d 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -50,6 +50,7 @@ module Ameba # Checks for unneeded disable directives. Always inspects a source last @unneeded_disable_directive_rule : Rule::Base? + # Returns true if correctable issues should be autocorrected. private getter? autocorrect : Bool # Instantiates a runner using a `config`. diff --git a/src/ameba/source.cr b/src/ameba/source.cr index ccd71af1..8e27c242 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -25,6 +25,8 @@ module Ameba def initialize(@code, @path = "") end + # Corrects any correctable issues and updates `code`. + # Returns `false` if no issues were corrected. def correct corrector = Corrector.new(code) issues.each(&.correct(corrector)) diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr index f4adf1da..ab6823ef 100644 --- a/src/ameba/source/corrector.cr +++ b/src/ameba/source/corrector.cr @@ -1,6 +1,8 @@ require "./rewriter" class Ameba::Source + # This class takes source code and rewrites it based + # on the different correction actions supplied. class Corrector @line_sizes : Array(Int32) @@ -9,30 +11,37 @@ class Ameba::Source @line_sizes = code.lines(chomp: false).map(&.size) end + # Replaces the code of the given range with *content*. def replace(location, end_location, content) @rewriter.replace(loc_to_pos(location), loc_to_pos(end_location) + 1, 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 + # 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 `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(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 `insert_before(location, location, content)` def insert_before(location, content) @rewriter.insert_before(loc_to_pos(location), content) end + # Shortcut for `insert_after(location, location, content)` def insert_after(location, content) @rewriter.insert_after(loc_to_pos(location) + 1, content) end @@ -46,22 +55,27 @@ class Ameba::Source @line_sizes[0...line - 1].sum + (column - 1) end + # Replaces the code of the given node with *content*. def replace(node : Crystal::ASTNode, content) replace(location(node), end_location(node), content) end + # Inserts the given strings before and after the given node. def wrap(node : Crystal::ASTNode, insert_before, insert_after) wrap(location(node), end_location(node), insert_before, insert_after) end + # Shortcut for `replace(node, "")` def remove(node : Crystal::ASTNode) remove(location(node), end_location(node)) end + # Shortcut for `wrap(node, content, nil)` def insert_before(node : Crystal::ASTNode, content) insert_before(location(node), content) end + # Shortcut for `wrap(node, nil, content)` def insert_after(node : Crystal::ASTNode, content) insert_after(end_location(node), content) end @@ -74,6 +88,7 @@ class Ameba::Source node.end_location || raise "Missing end location" end + # Applies all scheduled changes and returns modified source as a new string. def process @rewriter.process end diff --git a/src/ameba/source/rewriter.cr b/src/ameba/source/rewriter.cr index 62b5c65b..6825ea87 100644 --- a/src/ameba/source/rewriter.cr +++ b/src/ameba/source/rewriter.cr @@ -70,7 +70,7 @@ class Ameba::Source @action_root.empty? end - # Replaces the code of the given range with `content`. + # Replaces the code of the given range with *content*. def replace(begin_pos, end_pos, content) combine(begin_pos, end_pos, replacement: content.to_s) end diff --git a/src/ameba/source/rewriter/action.cr b/src/ameba/source/rewriter/action.cr index c33e607c..df4d1270 100644 --- a/src/ameba/source/rewriter/action.cr +++ b/src/ameba/source/rewriter/action.cr @@ -1,4 +1,5 @@ class Ameba::Source::Rewriter + # :nodoc: # Actions are arranged in a tree and get combined so that: # - children are strictly contained by their parent # - siblings all disjoint from one another and ordered From 8d3b76003ec9078d30aa52263978d9c39d81a8fb 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, 26 Oct 2021 21:46:16 -0700 Subject: [PATCH 14/23] Add autocorrect checks to Flycheck, JSON, and TODO formatters --- src/ameba/formatter/flycheck_formatter.cr | 1 + src/ameba/formatter/json_formatter.cr | 1 + src/ameba/formatter/todo_formatter.cr | 1 + 3 files changed, 3 insertions(+) diff --git a/src/ameba/formatter/flycheck_formatter.cr b/src/ameba/formatter/flycheck_formatter.cr index 50d5de6c..63039b82 100644 --- a/src/ameba/formatter/flycheck_formatter.cr +++ b/src/ameba/formatter/flycheck_formatter.cr @@ -5,6 +5,7 @@ module Ameba::Formatter def source_finished(source : Source) source.issues.each do |e| next if e.disabled? + next if e.correctable? && config[:autocorrect]? if loc = e.location @mutex.synchronize do output.printf "%s:%d:%d: %s: [%s] %s\n", diff --git a/src/ameba/formatter/json_formatter.cr b/src/ameba/formatter/json_formatter.cr index 211dabb9..da16fe1f 100644 --- a/src/ameba/formatter/json_formatter.cr +++ b/src/ameba/formatter/json_formatter.cr @@ -77,6 +77,7 @@ module Ameba::Formatter source.issues.each do |e| next if e.disabled? + next if e.correctable? && config[:autocorrect]? json_source.issues << AsJSON::Issue.new(e.rule.name, e.rule.severity.to_s, e.location, e.end_location, e.message) @result.summary.issues_count += 1 end diff --git a/src/ameba/formatter/todo_formatter.cr b/src/ameba/formatter/todo_formatter.cr index 47314407..8c8524f9 100644 --- a/src/ameba/formatter/todo_formatter.cr +++ b/src/ameba/formatter/todo_formatter.cr @@ -42,6 +42,7 @@ module Ameba::Formatter Hash(Rule::Base, Array(Issue)).new.tap do |h| issues.each do |issue| next if issue.disabled? || issue.rule.is_a?(Rule::Lint::Syntax) + next if issue.correctable? && config[:autocorrect]? (h[issue.rule] ||= Array(Issue).new) << issue end end From 1b6fe40a3b3894b402d63ca2d65345d81eebe266 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, 26 Oct 2021 21:47:39 -0700 Subject: [PATCH 15/23] Add autocorrect checks to `ExplainFormatter` --- src/ameba/cli/cmd.cr | 2 +- src/ameba/formatter/explain_formatter.cr | 17 ++++++++++++++--- src/ameba/runner.cr | 4 ++-- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index 42238e30..e5dcdc6b 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -27,7 +27,7 @@ module Ameba::Cli runner = Ameba.run(config) if location = opts.location_to_explain - runner.explain(location) + runner.explain(location, autocorrect: opts.autocorrect?) else exit 1 unless runner.success? end diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr index 33c8b7d5..e5f7598a 100644 --- a/src/ameba/formatter/explain_formatter.cr +++ b/src/ameba/formatter/explain_formatter.cr @@ -11,6 +11,7 @@ module Ameba::Formatter getter output : IO::FileDescriptor | IO::Memory getter location : Crystal::Location + getter? autocorrect : Bool # Creates a new instance of ExplainFormatter. # Accepts *output* which indicates the io where the explanation will be wrtitten to. @@ -20,7 +21,7 @@ module Ameba::Formatter # ExplainFormatter.new output, # {file: path, line: line_number, column: column_number} # ``` - def initialize(@output, location) + def initialize(@output, location, @autocorrect = false) @location = Crystal::Location.new(location[:file], location[:line], location[:column]) end @@ -40,12 +41,22 @@ module Ameba::Formatter return unless (location = issue.location) - output_title "ISSUE INFO" - output_paragraph [ + issue_info = [ issue.message.colorize(:red).to_s, location.to_s.colorize(:cyan).to_s, ] + if issue.correctable? + if autocorrect? + issue_info << "Corrected".colorize(:green).to_s + else + issue_info << "Correctable".colorize(:yellow).to_s + end + end + + output_title "ISSUE INFO" + output_paragraph issue_info + if affected_code = affected_code(issue, context_lines: 3) output_title "AFFECTED CODE" output_paragraph affected_code diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index 3cccff4d..dd2ca2f7 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -161,8 +161,8 @@ module Ameba # runner.run # runner.explain({file: file, line: l, column: c}) # ``` - def explain(location, output = STDOUT) - Formatter::ExplainFormatter.new(output, location).finished @sources + def explain(location, output = STDOUT, autocorrect = false) + Formatter::ExplainFormatter.new(output, location, autocorrect).finished @sources end # Indicates whether the last inspection successful or not. From d51ef27d545bd81fdac3c288a00b166269d30981 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, 26 Oct 2021 22:21:51 -0700 Subject: [PATCH 16/23] Add `remove_preceding`, `remove_leading`, `remove_trailing` --- src/ameba/source/corrector.cr | 38 +++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr index ab6823ef..43b3d348 100644 --- a/src/ameba/source/corrector.cr +++ b/src/ameba/source/corrector.cr @@ -46,6 +46,25 @@ class Ameba::Source @rewriter.insert_after(loc_to_pos(location) + 1, 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 + + # 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. + def remove_leading(location, end_location, size) + remove(loc_to_pos(location), loc_to_pos(location) + 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. + def remove_trailing(location, end_location, size) + remove(loc_to_pos(end_location) + 1 - size, loc_to_pos(end_location) + 1) + 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 @@ -80,6 +99,25 @@ class Ameba::Source insert_after(end_location(node), content) end + # Removes *size* characters prior to the given node. + def remove_preceding(node : Crystal::ASTNode, size) + remove_preceding(location(node), end_location(node), size) + end + + # Removes *size* characters from the beginning of the given node. + # If *size* is greater than the size of the node, the removed region can + # overrun the end of the node. + def remove_leading(node : Crystal::ASTNode, size) + remove_leading(location(node), end_location(node), size) + end + + # Removes *size* characters from the end of the given node. + # If *size* is greater than the size of the node, the removed region can + # overrun the beginning of the node. + def remove_trailing(node : Crystal::ASTNode, size) + remove_trailing(location(node), end_location(node), size) + end + private def location(node : Crystal::ASTNode) node.location || raise "Missing location" end From b7bb282b9975c7e422d75dd7eddea5f1e8ad3560 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: Wed, 27 Oct 2021 10:08:36 -0700 Subject: [PATCH 17/23] Apply suggestions from code review --- src/ameba/runner.cr | 5 ++--- src/ameba/source.cr | 18 ++++++------------ src/ameba/source/corrector.cr | 2 +- src/ameba/source/rewriter.cr | 4 ++-- src/ameba/source/rewriter/action.cr | 28 +++++++++++++++------------- 5 files changed, 26 insertions(+), 31 deletions(-) diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index dd2ca2f7..87e398c6 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -19,8 +19,7 @@ module Ameba def initialize(path, issues_by_iteration, loop_start = -1) root_cause = issues_by_iteration[loop_start..-1] - .map(&.map(&.rule.name).uniq!.join(", ")) - .join(" -> ") + .join(" -> ", &.map(&.rule.name).uniq!.join(", ")) message = String.build do |io| io << "Infinite loop" @@ -210,7 +209,7 @@ module Ameba private def check_for_infinite_loop(source, corrected_issues, processed_sources) checksum = Digest::SHA1.hexdigest(source.code) - if (loop_start = processed_sources.index(checksum)) + if loop_start = processed_sources.index(checksum) raise InfiniteCorrectionLoopError.new( source.path, corrected_issues, diff --git a/src/ameba/source.cr b/src/ameba/source.cr index 8e27c242..d6e0b97d 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -11,9 +11,6 @@ module Ameba # Crystal code (content of a source file). getter code : String - @lines : Array(String)? - @ast : Crystal::ASTNode? - # Creates a new source by `code` and `path`. # # For example: @@ -50,9 +47,7 @@ module Ameba # source.lines # => ["a = 1", "b = 2"] # ``` # - def lines : Array(String) - @lines ||= code.split('\n') - end + getter lines : Array(String) { code.split('\n') } # Returns AST nodes constructed by `Crystal::Parser`. # @@ -61,12 +56,11 @@ module Ameba # source.ast # ``` # - def ast : Crystal::ASTNode - @ast ||= - Crystal::Parser.new(code) - .tap(&.wants_doc = true) - .tap(&.filename = path) - .parse + getter ast : Crystal::ASTNode do + Crystal::Parser.new(code) + .tap(&.wants_doc = true) + .tap(&.filename = path) + .parse end getter fullpath : String do diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr index 43b3d348..94124786 100644 --- a/src/ameba/source/corrector.cr +++ b/src/ameba/source/corrector.cr @@ -8,7 +8,7 @@ class Ameba::Source def initialize(code : String) @rewriter = Rewriter.new(code) - @line_sizes = code.lines(chomp: false).map(&.size) + @line_sizes = code.each_line(chomp: false).map(&.size).to_a end # Replaces the code of the given range with *content*. diff --git a/src/ameba/source/rewriter.cr b/src/ameba/source/rewriter.cr index 6825ea87..5eda2d6a 100644 --- a/src/ameba/source/rewriter.cr +++ b/src/ameba/source/rewriter.cr @@ -59,9 +59,9 @@ class Ameba::Source # The updates are organized in a tree, according to the ranges they act on # (where children are strictly contained by their parent). class Rewriter - getter code + getter code : String - def initialize(@code : String) + def initialize(@code) @action_root = Rewriter::Action.new(0, code.size) end diff --git a/src/ameba/source/rewriter/action.cr b/src/ameba/source/rewriter/action.cr index df4d1270..456ad33a 100644 --- a/src/ameba/source/rewriter/action.cr +++ b/src/ameba/source/rewriter/action.cr @@ -73,21 +73,23 @@ class Ameba::Source::Rewriter family = analyse_hierarchy(action) sibling_left, sibling_right = family[:sibling_left], family[:sibling_right] - if (fusible = family[:fusible]) + if fusible = family[:fusible] child = family[:child] child ||= [] of Action fuse_deletions(action, fusible, sibling_left + child + sibling_right) else - extra_sibling = if (parent = family[:parent]) - # action should be a descendant of one of the children - parent.do_combine(action) - elsif (child = family[:child]) - # or it should become the parent of some of the children, - action.with(children: child).combine_children(action.children) - else - # or else it should become an additional child - action - end + extra_sibling = + case + when parent = family[:parent] + # action should be a descendant of one of the children + parent.do_combine(action) + when child = family[:child] + # or it should become the parent of some of the children, + action.with(children: child).combine_children(action.children) + else + # or else it should become an additional child + action + end self.with(children: sibling_left + [extra_sibling] + sibling_right) end end @@ -102,8 +104,8 @@ class Ameba::Source::Rewriter protected def fuse_deletions(action, fusible, other_siblings) without_fusible = self.with(children: other_siblings) fusible = [action] + fusible - fused_begin_pos = fusible.map(&.begin_pos).min - fused_end_pos = fusible.map(&.end_pos).max + fused_begin_pos = fusible.min_of(&.begin_pos) + fused_end_pos = fusible.max_of(&.end_pos) fused_deletion = action.with(begin_pos: fused_begin_pos, end_pos: fused_end_pos) without_fusible.do_combine(fused_deletion) end From 73e97ac42ec7e28a68382b1b1eaf191f96e0a497 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: Wed, 27 Oct 2021 11:08:18 -0700 Subject: [PATCH 18/23] Avoid using iterators and throw-away heap allocations --- src/ameba/source/corrector.cr | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr index 94124786..97c46e24 100644 --- a/src/ameba/source/corrector.cr +++ b/src/ameba/source/corrector.cr @@ -4,11 +4,13 @@ class Ameba::Source # This class takes source code and rewrites it based # on the different correction actions supplied. class Corrector - @line_sizes : Array(Int32) + @line_sizes = [] of Int32 def initialize(code : String) + code.each_line(chomp: false) do |line| + @line_sizes << line.size + end @rewriter = Rewriter.new(code) - @line_sizes = code.each_line(chomp: false).map(&.size).to_a end # Replaces the code of the given range with *content*. From 61fc99e1072141ee113461cd60cade6ea83440b9 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: Wed, 27 Oct 2021 11:19:15 -0700 Subject: [PATCH 19/23] Inline the `do_combine` method --- src/ameba/source/rewriter/action.cr | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/ameba/source/rewriter/action.cr b/src/ameba/source/rewriter/action.cr index 456ad33a..23a009ec 100644 --- a/src/ameba/source/rewriter/action.cr +++ b/src/ameba/source/rewriter/action.cr @@ -23,7 +23,11 @@ class Ameba::Source::Rewriter def combine(action) return self if action.empty? # Ignore empty action - do_combine(action) + if action.begin_pos == @begin_pos && action.end_pos == @end_pos + merge(action) + else + place_in_hierarchy(action) + end end def empty? @@ -60,15 +64,6 @@ class Ameba::Source::Rewriter self.class.new(begin_pos, end_pos, insert_before, replacement, insert_after, children) end - # Assumes *action* is contained within `@begin_pos...@end_pos` and has no children - protected def do_combine(action) - if action.begin_pos == @begin_pos && action.end_pos == @end_pos - merge(action) - else - place_in_hierarchy(action) - end - end - protected def place_in_hierarchy(action) family = analyse_hierarchy(action) sibling_left, sibling_right = family[:sibling_left], family[:sibling_right] @@ -82,7 +77,7 @@ class Ameba::Source::Rewriter case when parent = family[:parent] # action should be a descendant of one of the children - parent.do_combine(action) + parent.combine(action) when child = family[:child] # or it should become the parent of some of the children, action.with(children: child).combine_children(action.children) @@ -107,7 +102,7 @@ class Ameba::Source::Rewriter fused_begin_pos = fusible.min_of(&.begin_pos) fused_end_pos = fusible.max_of(&.end_pos) fused_deletion = action.with(begin_pos: fused_begin_pos, end_pos: fused_end_pos) - without_fusible.do_combine(fused_deletion) + without_fusible.combine(fused_deletion) end # Similar to `@children.bsearch_index || size` except allows for a starting point From a40fdee33fa1774552e6e9ee2d46970a2cd457b1 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: Sun, 31 Oct 2021 22:12:21 -0700 Subject: [PATCH 20/23] Revert "Add autocorrect checks to `ExplainFormatter`" This reverts commit 1b6fe40a3b3894b402d63ca2d65345d81eebe266. --- src/ameba/cli/cmd.cr | 2 +- src/ameba/formatter/explain_formatter.cr | 17 +++-------------- src/ameba/runner.cr | 4 ++-- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index e5dcdc6b..42238e30 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -27,7 +27,7 @@ module Ameba::Cli runner = Ameba.run(config) if location = opts.location_to_explain - runner.explain(location, autocorrect: opts.autocorrect?) + runner.explain(location) else exit 1 unless runner.success? end diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr index e5f7598a..33c8b7d5 100644 --- a/src/ameba/formatter/explain_formatter.cr +++ b/src/ameba/formatter/explain_formatter.cr @@ -11,7 +11,6 @@ module Ameba::Formatter getter output : IO::FileDescriptor | IO::Memory getter location : Crystal::Location - getter? autocorrect : Bool # Creates a new instance of ExplainFormatter. # Accepts *output* which indicates the io where the explanation will be wrtitten to. @@ -21,7 +20,7 @@ module Ameba::Formatter # ExplainFormatter.new output, # {file: path, line: line_number, column: column_number} # ``` - def initialize(@output, location, @autocorrect = false) + def initialize(@output, location) @location = Crystal::Location.new(location[:file], location[:line], location[:column]) end @@ -41,22 +40,12 @@ module Ameba::Formatter return unless (location = issue.location) - issue_info = [ + output_title "ISSUE INFO" + output_paragraph [ issue.message.colorize(:red).to_s, location.to_s.colorize(:cyan).to_s, ] - if issue.correctable? - if autocorrect? - issue_info << "Corrected".colorize(:green).to_s - else - issue_info << "Correctable".colorize(:yellow).to_s - end - end - - output_title "ISSUE INFO" - output_paragraph issue_info - if affected_code = affected_code(issue, context_lines: 3) output_title "AFFECTED CODE" output_paragraph affected_code diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index 87e398c6..dcc9f0ec 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -160,8 +160,8 @@ module Ameba # runner.run # runner.explain({file: file, line: l, column: c}) # ``` - def explain(location, output = STDOUT, autocorrect = false) - Formatter::ExplainFormatter.new(output, location, autocorrect).finished @sources + def explain(location, output = STDOUT) + Formatter::ExplainFormatter.new(output, location).finished @sources end # Indicates whether the last inspection successful or not. From 470e41cb7b18a3b3c647a6afb0670eea52c90943 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: Sun, 31 Oct 2021 22:23:14 -0700 Subject: [PATCH 21/23] Raise error if attempting to both explain issue and autocorrect --- src/ameba/cli/cmd.cr | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index 42238e30..0e62027b 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -7,8 +7,15 @@ module Ameba::Cli def run(args = ARGV) opts = parse_args args + location_to_explain = opts.location_to_explain + autocorrect = opts.autocorrect? + + if location_to_explain && autocorrect + raise "Invalid usage: Cannot explain an issue and autocorrect at the same time." + end + config = Config.load opts.config, opts.colors? - config.autocorrect = opts.autocorrect? + config.autocorrect = autocorrect if globs = opts.globs config.globs = globs @@ -26,8 +33,8 @@ module Ameba::Cli runner = Ameba.run(config) - if location = opts.location_to_explain - runner.explain(location) + if location_to_explain + runner.explain(location_to_explain) else exit 1 unless runner.success? end From c2aa2fedb6eafe223be463868bb3212c4fd093db 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: Sun, 31 Oct 2021 22:36:18 -0700 Subject: [PATCH 22/23] Return `source` from `expect_issue` --- .../rule/layout/trailing_blank_lines_spec.cr | 12 +++++------ spec/ameba/rule/style/large_numbers_spec.cr | 4 ++-- src/ameba/spec/expect_issue.cr | 21 +++++++------------ 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/spec/ameba/rule/layout/trailing_blank_lines_spec.cr b/spec/ameba/rule/layout/trailing_blank_lines_spec.cr index 2b98946a..d830127e 100644 --- a/spec/ameba/rule/layout/trailing_blank_lines_spec.cr +++ b/spec/ameba/rule/layout/trailing_blank_lines_spec.cr @@ -13,18 +13,18 @@ module Ameba::Rule::Layout end it "fails if there is no blank lines at the end" do - expect_issue subject, "no-blankline # error: Trailing newline missing" - expect_correction "no-blankline\n" + s = expect_issue subject, "no-blankline # error: Trailing newline missing" + expect_correction s, "no-blankline\n" end it "fails if there more then one blank line at the end of a source" do - expect_issue subject, "a = 1\n \n # error: Excessive trailing newline detected", normalize: false - expect_no_corrections + s = expect_issue subject, "a = 1\n \n # error: Excessive trailing newline detected", normalize: false + expect_no_corrections s end it "fails if last line is not blank" do - expect_issue subject, "\n\n\n puts 22 # error: Trailing newline missing", normalize: false - expect_correction "\n\n\n puts 22\n" + s = expect_issue subject, "\n\n\n puts 22 # error: Trailing newline missing", normalize: false + expect_correction s, "\n\n\n puts 22\n" end context "when unnecessary blank line has been detected" do diff --git a/spec/ameba/rule/style/large_numbers_spec.cr b/spec/ameba/rule/style/large_numbers_spec.cr index ff950b2e..c2a6a962 100644 --- a/spec/ameba/rule/style/large_numbers_spec.cr +++ b/spec/ameba/rule/style/large_numbers_spec.cr @@ -7,12 +7,12 @@ module Ameba it "transforms large number #{number}" do rule = Rule::Style::LargeNumbers.new - expect_issue rule, <<-CRYSTAL, number: number + s = expect_issue rule, <<-CRYSTAL, number: number number = %{number} # ^{number} error: Large numbers should be written with underscores: #{expected} CRYSTAL - expect_correction <<-CRYSTAL + expect_correction s, <<-CRYSTAL number = #{expected} CRYSTAL end diff --git a/src/ameba/spec/expect_issue.cr b/src/ameba/spec/expect_issue.cr index 7e9872ed..1167b8a3 100644 --- a/src/ameba/spec/expect_issue.cr +++ b/src/ameba/spec/expect_issue.cr @@ -47,8 +47,6 @@ require "./util" module Ameba::Spec::ExpectIssue include Spec::Util - class_property source : Source? - def expect_issue(rules : Rule::Base | Enumerable(Rule::Base), annotated_code : String, path = "", @@ -67,7 +65,7 @@ module Ameba::Spec::ExpectIssue raise "Use `report_no_issues` to assert that no issues are found" end - actual_annotations = actual_annotations(rules, code, path, lines) + source, actual_annotations = actual_annotations(rules, code, path, lines) unless actual_annotations == expected_annotations fail <<-MSG, file, line Expected: @@ -79,12 +77,11 @@ module Ameba::Spec::ExpectIssue #{actual_annotations} MSG end + + source end - def expect_correction(correction, *, file = __FILE__, line = __LINE__) - source = ExpectIssue.source - raise "`expect_correction` must follow `expect_issue`" unless source - + def expect_correction(source, correction, *, file = __FILE__, line = __LINE__) raise "Use `expect_no_corrections` if the code will not change" unless source.correct return if correction == source.code @@ -99,10 +96,7 @@ module Ameba::Spec::ExpectIssue MSG end - def expect_no_corrections(*, file = __FILE__, line = __LINE__) - source = ExpectIssue.source - raise "`expect_no_corrections` must follow `expect_offense`" unless source - + def expect_no_corrections(source, *, file = __FILE__, line = __LINE__) return unless source.correct fail <<-MSG, file, line @@ -121,7 +115,7 @@ module Ameba::Spec::ExpectIssue line = __LINE__) code = normalize_code(code) if normalize lines = code.split('\n') # must preserve trailing newline - actual_annotations = actual_annotations(rules, code, path, lines) + _, actual_annotations = actual_annotations(rules, code, path, lines) unless actual_annotations.to_s == code fail <<-MSG, file, line Expected no issues, but got: @@ -133,13 +127,12 @@ module Ameba::Spec::ExpectIssue private def actual_annotations(rules, code, path, lines) source = Source.new(code, path, normalize: false) # already normalized - ExpectIssue.source = source if rules.is_a?(Enumerable) rules.each(&.catch(source)) else rules.catch(source) end - AnnotatedSource.new(lines, source.issues) + {source, AnnotatedSource.new(lines, source.issues)} end private def format_issue(code, **replacements) From 78071722c41f7c106404abee1f21c02758028b2f 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, 1 Nov 2021 08:55:48 -0700 Subject: [PATCH 23/23] Rename `s` to `source` --- spec/ameba/rule/layout/trailing_blank_lines_spec.cr | 12 ++++++------ spec/ameba/rule/style/large_numbers_spec.cr | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/ameba/rule/layout/trailing_blank_lines_spec.cr b/spec/ameba/rule/layout/trailing_blank_lines_spec.cr index d830127e..babe9f05 100644 --- a/spec/ameba/rule/layout/trailing_blank_lines_spec.cr +++ b/spec/ameba/rule/layout/trailing_blank_lines_spec.cr @@ -13,18 +13,18 @@ module Ameba::Rule::Layout end it "fails if there is no blank lines at the end" do - s = expect_issue subject, "no-blankline # error: Trailing newline missing" - expect_correction s, "no-blankline\n" + source = expect_issue subject, "no-blankline # error: Trailing newline missing" + expect_correction source, "no-blankline\n" end it "fails if there more then one blank line at the end of a source" do - s = expect_issue subject, "a = 1\n \n # error: Excessive trailing newline detected", normalize: false - expect_no_corrections s + source = expect_issue subject, "a = 1\n \n # error: Excessive trailing newline detected", normalize: false + expect_no_corrections source end it "fails if last line is not blank" do - s = expect_issue subject, "\n\n\n puts 22 # error: Trailing newline missing", normalize: false - expect_correction s, "\n\n\n puts 22\n" + source = expect_issue subject, "\n\n\n puts 22 # error: Trailing newline missing", normalize: false + expect_correction source, "\n\n\n puts 22\n" end context "when unnecessary blank line has been detected" do diff --git a/spec/ameba/rule/style/large_numbers_spec.cr b/spec/ameba/rule/style/large_numbers_spec.cr index c2a6a962..7397603f 100644 --- a/spec/ameba/rule/style/large_numbers_spec.cr +++ b/spec/ameba/rule/style/large_numbers_spec.cr @@ -7,12 +7,12 @@ module Ameba it "transforms large number #{number}" do rule = Rule::Style::LargeNumbers.new - s = expect_issue rule, <<-CRYSTAL, number: number + source = expect_issue rule, <<-CRYSTAL, number: number number = %{number} # ^{number} error: Large numbers should be written with underscores: #{expected} CRYSTAL - expect_correction s, <<-CRYSTAL + expect_correction source, <<-CRYSTAL number = #{expected} CRYSTAL end