diff --git a/spec/ameba/rule/layout/trailing_blank_lines_spec.cr b/spec/ameba/rule/layout/trailing_blank_lines_spec.cr index babe9f05..028ab496 100644 --- a/spec/ameba/rule/layout/trailing_blank_lines_spec.cr +++ b/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 diff --git a/spec/ameba/rule/lint/debugger_statement_spec.cr b/spec/ameba/rule/lint/debugger_statement_spec.cr index 084b25ba..2f67e351 100644 --- a/spec/ameba/rule/lint/debugger_statement_spec.cr +++ b/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 diff --git a/spec/ameba/rule/lint/duplicated_require_spec.cr b/spec/ameba/rule/lint/duplicated_require_spec.cr index 2c0dd45c..45d7961e 100644 --- a/spec/ameba/rule/lint/duplicated_require_spec.cr +++ b/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 diff --git a/spec/ameba/rule/lint/shared_var_in_fiber_spec.cr b/spec/ameba/rule/lint/shared_var_in_fiber_spec.cr index 6981ee8f..221845c2 100644 --- a/spec/ameba/rule/lint/shared_var_in_fiber_spec.cr +++ b/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 diff --git a/spec/ameba/rule/performance/any_after_filter_spec.cr b/spec/ameba/rule/performance/any_after_filter_spec.cr index c53e9f59..f95f395a 100644 --- a/spec/ameba/rule/performance/any_after_filter_spec.cr +++ b/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 diff --git a/spec/ameba/rule/style/is_a_filter_spec.cr b/spec/ameba/rule/style/is_a_filter_spec.cr index a45a96ed..ff53cfcb 100644 --- a/spec/ameba/rule/style/is_a_filter_spec.cr +++ b/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 diff --git a/src/ameba/rule/style/is_a_filter.cr b/src/ameba/rule/style/is_a_filter.cr index 6dd4a777..31867d4e 100644 --- a/src/ameba/rule/style/is_a_filter.cr +++ b/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 diff --git a/src/ameba/spec/annotated_source.cr b/src/ameba/spec/annotated_source.cr index 76081fe7..7a33d3df 100644 --- a/src/ameba/spec/annotated_source.cr +++ b/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 diff --git a/src/ameba/spec/expect_issue.cr b/src/ameba/spec/expect_issue.cr index 1167b8a3..20eecf69 100644 --- a/src/ameba/spec/expect_issue.cr +++ b/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