Remove normalize parameter from expect_issue (#249)

* Add `normalize` parameter to `expect_correction`

* Convert Style/IsAFilter spec

* Revert "Add `normalize` parameter to `expect_correction`"

This reverts commit 4b67e4b900.

* Remove `normalize` parameter from `expect_issue`

* Require indentation if multiple issues on a single line

* Update `Style/IsAFilter` spec

* Update `ExpectIssue` documentation

* Add missing `expect_no_corrections`

* Use carets and space with issues at column 1 or 2

* Update `expect_issue` docs

spec/ameba/rule/layout/trailing_blank_lines_spec.cr

@@ -5,7 +5,7 @@ module Ameba::Rule::Layout
 
   describe TrailingBlankLines do
     it "passes if there is a blank line at the end of a source" do
-      expect_no_issues subject, "a = 1\n", normalize: false
+      expect_no_issues subject, "a = 1\n"
     end
 
     it "passes if source is empty" do
@@ -18,12 +18,12 @@ module Ameba::Rule::Layout
     end
 
     it "fails if there more then one blank line at the end of a source" do
-      source = expect_issue subject, "a = 1\n \n # error: Excessive trailing newline detected", normalize: false
+      source = expect_issue subject, "a = 1\n \n # error: Excessive trailing newline detected"
       expect_no_corrections source
     end
 
     it "fails if last line is not blank" do
-      source = expect_issue subject, "\n\n\n puts 22 # error: Trailing newline missing", normalize: false
+      source = expect_issue subject, "\n\n\n puts 22 # error: Trailing newline missing"
       expect_correction source, "\n\n\n puts 22\n"
     end
 

spec/ameba/rule/lint/debugger_statement_spec.cr

@@ -5,7 +5,7 @@ module Ameba::Rule::Lint
 
   describe DebuggerStatement do
     it "passes if there is no debugger statement" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         "this is not a debugger statement"
         s = "debugger"
 
@@ -18,16 +18,18 @@ module Ameba::Rule::Lint
           end
         end
         A.new.debugger
-      )
+        CRYSTAL
     end
 
     it "fails if there is a debugger statement" do
-      expect_issue subject, %(
+      source = expect_issue subject, <<-CRYSTAL
         a = 2
         debugger
-        # ^{} error: Possible forgotten debugger statement detected
+        # ^^^^^^ error: Possible forgotten debugger statement detected
         a = a + 1
-      )
+        CRYSTAL
+
+      expect_no_corrections source
     end
 
     it "reports rule, pos and message" do

spec/ameba/rule/lint/duplicated_require_spec.cr

@@ -5,20 +5,22 @@ module Ameba::Rule::Lint
 
   describe DuplicatedRequire do
     it "passes if there are no duplicated requires" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         require "math"
         require "big"
         require "big/big_decimal"
-      )
+        CRYSTAL
     end
 
     it "reports if there are a duplicated requires" do
-      expect_issue subject, %(
+      source = expect_issue subject, <<-CRYSTAL
         require "big"
         require "math"
         require "big"
         # ^{} error: Duplicated require of `big`
-      )
+        CRYSTAL
+
+      expect_no_corrections source
     end
 
     it "reports rule, pos and message" do

spec/ameba/rule/lint/shared_var_in_fiber_spec.cr

@@ -5,18 +5,18 @@ module Ameba::Rule::Lint
     subject = SharedVarInFiber.new
 
     it "doesn't report if there is only local shared var in fiber" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         spawn do
           i = 1
           puts i
         end
 
         Fiber.yield
-      )
+        CRYSTAL
     end
 
     it "doesn't report if there is only block shared var in fiber" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         10.times do |i|
           spawn do
             puts i
@@ -24,11 +24,11 @@ module Ameba::Rule::Lint
         end
 
         Fiber.yield
-      )
+        CRYSTAL
     end
 
     it "doesn't report if there a spawn macro is used" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         i = 0
         while i < 10
           spawn puts(i)
@@ -36,11 +36,11 @@ module Ameba::Rule::Lint
         end
 
         Fiber.yield
-      )
+        CRYSTAL
     end
 
     it "reports if there is a shared var in spawn" do
-      expect_issue subject, %(
+      source = expect_issue subject, <<-CRYSTAL
         i = 0
         while i < 10
           spawn do
@@ -51,11 +51,13 @@ module Ameba::Rule::Lint
         end
 
         Fiber.yield
-      )
+        CRYSTAL
+
+      expect_no_corrections source
     end
 
     it "reports reassigned reference to shared var in spawn" do
-      expect_issue subject, %(
+      source = expect_issue subject, <<-CRYSTAL
         channel = Channel(String).new
         n = 0
 
@@ -67,11 +69,13 @@ module Ameba::Rule::Lint
             channel.send m
           end
         end
-      )
+        CRYSTAL
+
+      expect_no_corrections source
     end
 
     it "doesn't report reassigned reference to shared var in block" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         channel = Channel(String).new
         n = 0
 
@@ -82,21 +86,21 @@ module Ameba::Rule::Lint
             channel.send m
           end
         end
-      )
+        CRYSTAL
     end
 
     it "does not report block is called in a spawn" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         def method(block)
           spawn do
             block.call(10)
           end
         end
-      )
+        CRYSTAL
     end
 
     it "reports multiple shared variables in spawn" do
-      expect_issue subject, %(
+      source = expect_issue subject, <<-CRYSTAL
         foo, bar, baz = 0, 0, 0
         while foo < 10
           baz += 1
@@ -109,11 +113,13 @@ module Ameba::Rule::Lint
           end
           foo += 1
         end
-      )
+        CRYSTAL
+
+      expect_no_corrections source
     end
 
     it "doesn't report if variable is passed to the proc" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         i = 0
         while i < 10
           proc = ->(x : Int32) do
@@ -124,19 +130,19 @@ module Ameba::Rule::Lint
           proc.call(i)
           i += 1
         end
-      )
+        CRYSTAL
     end
 
     it "doesn't report if a channel is declared in outer scope" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         channel = Channel(Nil).new
         spawn { channel.send(nil) }
         channel.receive
-      )
+        CRYSTAL
     end
 
     it "doesn't report if there is a loop in spawn" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         channel = Channel(String).new
 
         spawn do
@@ -146,47 +152,47 @@ module Ameba::Rule::Lint
             channel.send(line)
           end
         end
-      )
+        CRYSTAL
     end
 
     it "doesn't report if a var is mutated in spawn and referenced outside" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         def method
           foo = 1
           spawn { foo = 2 }
           foo
         end
-      )
+        CRYSTAL
     end
 
     it "doesn't report if variable is changed without iterations" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         def foo
           i = 0
           i += 1
           spawn { i }
         end
-      )
+        CRYSTAL
     end
 
     it "doesn't report if variable is in a loop inside spawn" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         i = 0
         spawn do
           while i < 10
             i += 1
           end
         end
-      )
+        CRYSTAL
     end
 
     it "doesn't report if variable declared inside loop" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         while true
           i = 0
           spawn { i += 1 }
         end
-      )
+        CRYSTAL
     end
 
     it "reports rule, location and message" do

spec/ameba/rule/performance/any_after_filter_spec.cr

@@ -5,66 +5,74 @@ module Ameba::Rule::Performance
 
   describe AnyAfterFilter do
     it "passes if there is no potential performance improvements" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         [1, 2, 3].select { |e| e > 1 }.any?(&.zero?)
         [1, 2, 3].reject { |e| e > 1 }.any?(&.zero?)
         [1, 2, 3].select { |e| e > 1 }
         [1, 2, 3].reject { |e| e > 1 }
         [1, 2, 3].any? { |e| e > 1 }
-      )
+        CRYSTAL
     end
 
     it "reports if there is select followed by any? without a block" do
-      expect_issue subject, %(
+      source = expect_issue subject, <<-CRYSTAL
         [1, 2, 3].select { |e| e > 2 }.any?
                 # ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Use `any? {...}` instead of `select {...}.any?`
-      )
+        CRYSTAL
+
+      expect_no_corrections source
     end
 
     it "does not report if source is a spec" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL, "source_spec.cr"
         [1, 2, 3].select { |e| e > 2 }.any?
-      ), "source_spec.cr"
+        CRYSTAL
     end
 
     it "reports if there is reject followed by any? without a block" do
-      expect_issue subject, %(
+      source = expect_issue subject, <<-CRYSTAL
         [1, 2, 3].reject { |e| e > 2 }.any?
                 # ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Use `any? {...}` instead of `reject {...}.any?`
-      )
+        CRYSTAL
+
+      expect_no_corrections source
     end
 
     it "does not report if any? calls contains a block" do
-      expect_no_issues subject, %(
+      expect_no_issues subject, <<-CRYSTAL
         [1, 2, 3].select { |e| e > 2 }.any?(&.zero?)
         [1, 2, 3].reject { |e| e > 2 }.any?(&.zero?)
-      )
+        CRYSTAL
     end
 
     context "properties" do
       it "allows to configure object_call_names" do
         rule = Rule::Performance::AnyAfterFilter.new
         rule.filter_names = %w(select)
-        expect_no_issues rule, %(
+        expect_no_issues rule, <<-CRYSTAL
           [1, 2, 3].reject { |e| e > 2 }.any?
-        )
+          CRYSTAL
       end
     end
 
     context "macro" do
       it "reports in macro scope" do
-        expect_issue subject, %(
+        source = expect_issue subject, <<-CRYSTAL
           {{ [1, 2, 3].reject { |e| e > 2  }.any? }}
                      # ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Use `any? {...}` instead of `reject {...}.any?`
-        )
+          CRYSTAL
+
+        expect_no_corrections source
       end
     end
 
     it "reports rule, pos and message" do
-      expect_issue subject, %(
+      source = expect_issue subject, <<-CRYSTAL
         [1, 2, 3].reject { |e| e > 2 }.any?
                 # ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Use `any? {...}` instead of `reject {...}.any?`
-      )
+        CRYSTAL
+
+      expect_no_corrections source
     end
   end
 end

spec/ameba/rule/style/is_a_filter_spec.cr

@@ -5,53 +5,57 @@ module Ameba::Rule::Style
 
   describe IsAFilter do
     it "passes if there is no potential performance improvements" do
-      source = Source.new %(
+      expect_no_issues subject, <<-CRYSTAL
         [1, 2, nil].select(Int32)
         [1, 2, nil].reject(Nil)
-      )
-      subject.catch(source).should be_valid
+        CRYSTAL
     end
 
     it "reports if there is .is_a? call within select" do
-      source = Source.new %(
+      source = expect_issue subject, <<-CRYSTAL
         [1, 2, nil].select(&.is_a?(Int32))
-      )
-      subject.catch(source).should_not be_valid
+                  # ^^^^^^^^^^^^^^^^^^^^^^ error: Use `select(Int32)` instead of `select {...}`
+        CRYSTAL
+
+      expect_correction source, <<-CRYSTAL
+        [1, 2, nil].select(Int32)
+        CRYSTAL
     end
 
     it "reports if there is .nil? call within reject" do
-      source = Source.new %(
+      source = expect_issue subject, <<-CRYSTAL
         [1, 2, nil].reject(&.nil?)
-      )
-      subject.catch(source).should_not be_valid
+                  # ^^^^^^^^^^^^^^ error: Use `reject(Nil)` instead of `reject {...}`
+        CRYSTAL
+
+      expect_correction source, <<-CRYSTAL
+        [1, 2, nil].reject(Nil)
+        CRYSTAL
     end
 
     it "does not report if there .is_a? call within block with multiple arguments" do
-      source = Source.new %(
+      expect_no_issues subject, <<-CRYSTAL
         t.all? { |_, v| v.is_a?(String) }
         t.all? { |foo, bar| foo.is_a?(String) }
         t.all? { |foo, bar| bar.is_a?(String) }
-      )
-      subject.catch(source).should be_valid
+        CRYSTAL
     end
 
     context "properties" do
       it "allows to configure filter_names" do
-        source = Source.new %(
-          [1, 2, nil].reject(&.nil?)
-        )
         rule = IsAFilter.new
         rule.filter_names = %w(select)
-        rule.catch(source).should be_valid
+        expect_no_issues rule, <<-CRYSTAL
+          [1, 2, nil].reject(&.nil?)
+          CRYSTAL
       end
     end
 
     context "macro" do
       it "doesn't report in macro scope" do
-        source = Source.new %(
+        expect_no_issues subject, <<-CRYSTAL
           {{ [1, 2, nil].reject(&.nil?) }}
-        )
-        subject.catch(source).should be_valid
+          CRYSTAL
       end
     end
 

src/ameba/rule/style/is_a_filter.cr

@@ -44,7 +44,9 @@ module Ameba::Rule::Style
       filter_names : Array(String) = %w(select reject any? all? none? one?)
     end
 
-    MSG = "Use `%s(%s)` instead of `%s {...}`"
+    MSG = "Use `%s` instead of `%s`"
+    NEW = "%s(%s)"
+    OLD = "%s {...}"
 
     def test(source)
       AST::NodeVisitor.new self, source, skip: [
@@ -57,6 +59,7 @@ module Ameba::Rule::Style
 
     def test(source, node : Crystal::Call)
       return unless node.name.in?(filter_names)
+      return unless (filter_location = node.name_location)
       return unless (block = node.block)
       return unless (body = block.body).is_a?(Crystal::IsA)
       return unless (path = body.const).is_a?(Crystal::Path)
@@ -77,8 +80,16 @@ module Ameba::Rule::Style
         end
       end
 
-      issue_for node.name_location, end_location,
-        MSG % {node.name, name, node.name}
+      old = OLD % node.name
+      new = NEW % {node.name, name}
+      msg = MSG % {new, old}
+      if end_location
+        issue_for(filter_location, end_location, msg) do |corrector|
+          corrector.replace(filter_location, end_location, new)
+        end
+      else
+        issue_for(filter_location, nil, msg)
+      end
     end
   end
 end

src/ameba/spec/annotated_source.cr

@@ -123,7 +123,8 @@ class Ameba::Spec::AnnotatedSource
                  " " * indent_count
                end
       caret_count = column_length(line, column, end_line, end_column)
-      carets = if indent_count < 0 || caret_count <= 0
+      caret_count += indent_count if indent_count < 0
+      carets = if caret_count <= 0
                  "^{}"
                else
                  "^" * caret_count

src/ameba/spec/expect_issue.cr

@@ -1,6 +1,8 @@
 require "./annotated_source"
 require "./util"
 
+# Mixin for `expect_issue` and `expect_no_issues`
+#
 # This mixin makes it easier to specify strict issue expectations
 # in a declarative and visual fashion. Just type out the code that
 # should generate an issue, annotate code by writing '^'s
@@ -11,51 +13,96 @@ require "./util"
 #
 # Usage:
 #
-#     expect_issue subject, %(
-#       def foo
-#         a do
-#           b
-#         end.c
-#       # ^^^^^ error: Avoid chaining a method call on a do...end block.
-#       end
-#     )
+#     expect_issue subject, <<-CRYSTAL
+#       a do
+#         b
+#       end.c
+#       # ^^^ error: Avoid chaining a method call on a do...end block.
+#       CRYSTAL
 #
 # Equivalent assertion without `expect_issue`:
 #
-#     source = Source.new %(
-#       def foo
-#         a do
-#           b
-#         end.c
-#       end
-#     ), "source.cr"
+#     source = Source.new <<-CRYSTAL, "source.cr"
+#       a do
+#         b
+#       end.c
+#       CRYSTAL
 #     subject.catch(source).should_not be_valid
 #     source.issues.size.should be(1)
 #
 #     issue = source.issues.first
-#     issue.location.to_s.should eq "source.cr:4:3"
-#     issue.end_location.to_s.should eq "source.cr:4:7"
+#     issue.location.to_s.should eq "source.cr:3:1"
+#     issue.end_location.to_s.should eq "source.cr:3:5"
 #     issue.message.should eq(
 #       "Avoid chaining a method call on a do...end block."
 #     )
 #
+# Autocorrection can be tested using `expect_correction` after
+# `expect_issue`.
+#
+#     source = expect_issue subject, <<-CRYSTAL
+#       x % 2 == 0
+#       # ^^^^^^^^ error: Replace with `Int#even?`.
+#       CRYSTAL
+#
+#     expect_correction source, <<-CRYSTAL
+#       x.even?
+#       CRYSTAL
+#
 # If you do not want to specify an issue then use the
 # companion method `expect_no_issues`. This method is a much
 # simpler assertion since it just inspects the code and checks
 # that there were no issues. The `expect_issue` method has
 # to do more work by parsing out lines that contain carets.
+#
+# If the code produces an issue that could not be auto-corrected, you can
+# use `expect_no_corrections` after `expect_issue`.
+#
+#     source = expect_issue subject, <<-CRYSTAL
+#       a do
+#         b
+#       end.c
+#       # ^^^ error: Avoid chaining a method call on a do...end block.
+#       CRYSTAL
+#
+#     expect_no_corrections source
+#
+# If your code has variables of different lengths, you can use `%{foo}`,
+# `^{foo}`, and `_{foo}` to format your template; you can also abbreviate
+# issue messages with `[...]`:
+#
+#     %w[raise fail].each do |keyword|
+#       expect_issue subject, <<-CRYSTAL, keyword: keyword
+#         %{keyword} Exception.new(msg)
+#         # ^{keyword}^^^^^^^^^^^^^^^^^ error: Redundant `Exception.new` [...]
+#         CRYSTAL
+#
+#     %w[has_one has_many].each do |type|
+#       expect_issue subject, <<-CRYSTAL, type: type
+#         class Book
+#           %{type} :chapter, foreign_key: "book_id"
+#           _{type}         # ^^^^^^^^^^^^^^^^^^^^^^ error: Specifying the default [...]
+#         end
+#         CRYSTAL
+#     end
+#
+# If you need to specify an issue on a blank line, use the empty `^{}` marker:
+#
+#     expect_issue subject, <<-CRYSTAL
+#
+#       # ^{} error: Missing frozen string literal comment.
+#       puts 1
+#       CRYSTAL
 module Ameba::Spec::ExpectIssue
   include Spec::Util
 
   def expect_issue(rules : Rule::Base | Enumerable(Rule::Base),
                    annotated_code : String,
                    path = "",
-                   normalize = true,
                    *,
                    file = __FILE__,
                    line = __LINE__,
                    **replacements)
-    annotated_code = normalize_code(annotated_code) if normalize
     annotated_code = format_issue(annotated_code, **replacements)
     expected_annotations = AnnotatedSource.parse(annotated_code)
     lines = expected_annotations.lines
@@ -109,11 +156,9 @@ module Ameba::Spec::ExpectIssue
   def expect_no_issues(rules : Rule::Base | Enumerable(Rule::Base),
                        code : String,
                        path = "",
-                       normalize = true,
                        *,
                        file = __FILE__,
                        line = __LINE__)
-    code = normalize_code(code) if normalize
     lines = code.split('\n') # must preserve trailing newline
     _, actual_annotations = actual_annotations(rules, code, path, lines)
     unless actual_annotations.to_s == code
@@ -126,7 +171,7 @@ module Ameba::Spec::ExpectIssue
   end
 
   private def actual_annotations(rules, code, path, lines)
-    source = Source.new(code, path, normalize: false) # already normalized
+    source = Source.new(code, path, normalize: false)
     if rules.is_a?(Enumerable)
       rules.each(&.catch(source))
     else