diff --git a/spec/ameba/rule/lint/unused_argument_spec.cr b/spec/ameba/rule/lint/unused_argument_spec.cr index 47be8ce9..427ae569 100644 --- a/spec/ameba/rule/lint/unused_argument_spec.cr +++ b/spec/ameba/rule/lint/unused_argument_spec.cr @@ -6,7 +6,7 @@ module Ameba::Rule::Lint describe UnusedArgument do it "doesn't report if arguments are used" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a, b, c) a + b + c end @@ -16,155 +16,158 @@ module Ameba::Rule::Lint end ->(i : Int32) { i + 1 } - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if method argument is unused" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def method(a, b, c) + # ^ error: Unused argument `c`. If it's necessary, use `_c` as an argument name to indicate that it won't be used. a + b end - ) - subject.catch(s).should_not be_valid - s.issues.first.message.should eq "Unused argument `c`. If it's necessary, use `_c` " \ - "as an argument name to indicate that it won't be used." + CRYSTAL + + expect_correction source, <<-CRYSTAL + def method(a, b, _c) + a + b + end + CRYSTAL end it "reports if block argument is unused" do - s = Source.new %( - [1,2].each_with_index do |a, i| + source = expect_issue subject, <<-CRYSTAL + [1, 2].each_with_index do |a, i| + # ^ error: Unused argument `i`. [...] a end - ) - subject.catch(s).should_not be_valid - s.issues.first.message.should eq "Unused argument `i`. If it's necessary, use `_` " \ - "as an argument name to indicate that it won't be used." + CRYSTAL + + expect_correction source, <<-CRYSTAL + [1, 2].each_with_index do |a, _| + a + end + CRYSTAL end it "reports if proc argument is unused" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL -> (a : Int32, b : String) do + # ^ error: Unused argument `b`. If it's necessary, use `_b` as an argument name to indicate that it won't be used. a = a + 1 end - ) - subject.catch(s).should_not be_valid - s.issues.first.message.should eq "Unused argument `b`. If it's necessary, use `_b` " \ - "as an argument name to indicate that it won't be used." + CRYSTAL + + expect_correction source, <<-CRYSTAL + -> (a : Int32, _b : String) do + a = a + 1 + end + CRYSTAL end it "reports multiple unused args" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def method(a, b, c) + # ^ error: Unused argument `a`. If it's necessary, use `_a` as an argument name to indicate that it won't be used. + # ^ error: Unused argument `b`. If it's necessary, use `_b` as an argument name to indicate that it won't be used. + # ^ error: Unused argument `c`. If it's necessary, use `_c` as an argument name to indicate that it won't be used. nil end - ) - subject.catch(s).should_not be_valid - s.issues[0].message.should eq "Unused argument `a`. If it's necessary, use `_a` " \ - "as an argument name to indicate that it won't be used." - s.issues[1].message.should eq "Unused argument `b`. If it's necessary, use `_b` " \ - "as an argument name to indicate that it won't be used." - s.issues[2].message.should eq "Unused argument `c`. If it's necessary, use `_c` " \ - "as an argument name to indicate that it won't be used." + CRYSTAL + + expect_correction source, <<-CRYSTAL + def method(_a, _b, _c) + nil + end + CRYSTAL end it "doesn't report if it is an instance var argument" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL class A def method(@name) end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if a typed argument is used" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(x : Int32) 3.times do puts x end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if an argument with default value is used" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(x = 1) puts x end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if argument starts with a _" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(_x) end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if it is a block and used" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(&block) block.call end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if block arg is not used" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(&block) end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if unused and there is yield" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(&block) yield 1 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if it's an anonymous block" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(&) yield 1 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if variable is referenced implicitly" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL class Bar < Foo def method(a, b) super end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if arg if referenced in case" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def foo(a) case a when /foo/ end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if enum in a record" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL class Class record Record do enum Enum @@ -172,59 +175,49 @@ module Ameba::Rule::Lint end end end - ) - subject.catch(s).should be_valid + CRYSTAL end context "super" do it "reports if variable is not referenced implicitly by super" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL class Bar < Foo def method(a, b) + # ^ error: Unused argument `b`. If it's necessary, use `_b` as an argument name to indicate that it won't be used. super a end end - ) - subject.catch(s).should_not be_valid - s.issues.first.message.should eq "Unused argument `b`. If it's necessary, use `_b` " \ - "as an argument name to indicate that it won't be used." - end + CRYSTAL - it "reports rule, location and message" do - s = Source.new %( - def method(a) + expect_correction source, <<-CRYSTAL + class Bar < Foo + def method(a, _b) + super a + end end - ), "source.cr" - subject.catch(s).should_not be_valid - issue = s.issues.first - issue.rule.should_not be_nil - issue.message.should eq "Unused argument `a`. If it's necessary, use `_a` " \ - "as an argument name to indicate that it won't be used." - issue.location.to_s.should eq "source.cr:1:12" + CRYSTAL end end context "macro" do it "doesn't report if it is a used macro argument" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL macro my_macro(arg) {% arg %} end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if it is a used macro block argument" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL macro my_macro(&block) {% block %} end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report used macro args with equal names in record" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL record X do macro foo(a, b) {{ a }} + {{ b }} @@ -234,12 +227,11 @@ module Ameba::Rule::Lint {{ a }} + {{ b }} + {{ c }} end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report used args in macro literals" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def print(f : Array(U)) forall U f.size.times do |i| {% if U == Float64 %} @@ -249,65 +241,73 @@ module Ameba::Rule::Lint {% end %} end end - ) - subject.catch(s).should be_valid + CRYSTAL end end context "properties" do describe "#ignore_defs" do it "lets the rule to ignore def scopes if true" do - subject.ignore_defs = true - s = Source.new %( + rule = UnusedArgument.new + rule.ignore_defs = true + + expect_no_issues rule, <<-CRYSTAL def method(a) end - ) - subject.catch(s).should be_valid + CRYSTAL end it "lets the rule not to ignore def scopes if false" do - subject.ignore_defs = false - s = Source.new %( + rule = UnusedArgument.new + rule.ignore_defs = false + + expect_issue rule, <<-CRYSTAL def method(a) + # ^ error: Unused argument `a`. If it's necessary, use `_a` as an argument name to indicate that it won't be used. end - ) - subject.catch(s).should_not be_valid + CRYSTAL end end context "#ignore_blocks" do it "lets the rule to ignore block scopes if true" do - subject.ignore_blocks = true - s = Source.new %( + rule = UnusedArgument.new + rule.ignore_blocks = true + + expect_no_issues rule, <<-CRYSTAL 3.times { |i| puts "yo!" } - ) - subject.catch(s).should be_valid + CRYSTAL end it "lets the rule not to ignore block scopes if false" do - subject.ignore_blocks = false - s = Source.new %( + rule = UnusedArgument.new + rule.ignore_blocks = false + + expect_issue rule, <<-CRYSTAL 3.times { |i| puts "yo!" } - ) - subject.catch(s).should_not be_valid + # ^ error: Unused argument `i`. If it's necessary, use `_` as an argument name to indicate that it won't be used. + CRYSTAL end end context "#ignore_procs" do it "lets the rule to ignore proc scopes if true" do - subject.ignore_procs = true - s = Source.new %( + rule = UnusedArgument.new + rule.ignore_procs = true + + expect_no_issues rule, <<-CRYSTAL ->(a : Int32) {} - ) - subject.catch(s).should be_valid + CRYSTAL end it "lets the rule not to ignore proc scopes if false" do - subject.ignore_procs = false - s = Source.new %( + rule = UnusedArgument.new + rule.ignore_procs = false + + expect_issue rule, <<-CRYSTAL ->(a : Int32) {} - ) - subject.catch(s).should_not be_valid + # ^ error: Unused argument `a`. If it's necessary, use `_a` as an argument name to indicate that it won't be used. + CRYSTAL end end end diff --git a/spec/ameba/rule/lint/unused_block_argument_spec.cr b/spec/ameba/rule/lint/unused_block_argument_spec.cr index adcdc6a1..dbc95b8c 100644 --- a/spec/ameba/rule/lint/unused_block_argument_spec.cr +++ b/spec/ameba/rule/lint/unused_block_argument_spec.cr @@ -5,105 +5,118 @@ module Ameba::Rule::Lint describe UnusedBlockArgument do it "doesn't report if it is an instance var argument" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL class A def initialize(&@callback) end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if anonymous" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a, b, c, &) end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if argument name starts with a `_`" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a, b, c, &_block) end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if it is a block and used" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a, b, c, &block) block.call end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if block arg is not used" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def method(a, b, c, &block) + # ^ error: Unused block argument `block`. [...] end - ) - subject.catch(s).should_not be_valid + CRYSTAL + + expect_correction source, <<-CRYSTAL + def method(a, b, c, &_block) + end + CRYSTAL end it "reports if unused and there is yield" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL def method(a, b, c, &block) + # ^ error: Use `&` as an argument name to indicate that it won't be referenced. 3.times do |i| i.try do yield i end end end - ) - subject.catch(s).should_not be_valid + CRYSTAL + + expect_correction source, <<-CRYSTAL + def method(a, b, c, &) + 3.times do |i| + i.try do + yield i + end + end + end + CRYSTAL end it "doesn't report if anonymous and there is yield" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a, b, c, &) yield 1 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if variable is referenced implicitly" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL class Bar < Foo def method(a, b, c, &block) super end end - ) - subject.catch(s).should be_valid + CRYSTAL end context "super" do it "reports if variable is not referenced implicitly by super" do - s = Source.new %( + source = expect_issue subject, <<-CRYSTAL class Bar < Foo def method(a, b, c, &block) + # ^ error: Unused block argument `block`. [...] super a, b, c end end - ) - subject.catch(s).should_not be_valid - s.issues.first.message.should eq "Unused block argument `block`. If it's necessary, " \ - "use `_block` as an argument name to indicate " \ - "that it won't be used." + CRYSTAL + + expect_correction source, <<-CRYSTAL + class Bar < Foo + def method(a, b, c, &_block) + super a, b, c + end + end + CRYSTAL end end context "macro" do it "doesn't report if it is a used macro block argument" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL macro my_macro(&block) {% block %} end - ) - subject.catch(s).should be_valid + CRYSTAL end end end diff --git a/src/ameba/rule/lint/unused_argument.cr b/src/ameba/rule/lint/unused_argument.cr index c42668a5..864b982f 100644 --- a/src/ameba/rule/lint/unused_argument.cr +++ b/src/ameba/rule/lint/unused_argument.cr @@ -63,7 +63,18 @@ module Ameba::Rule::Lint next if scope.references?(argument.variable) name_suggestion = scope.node.is_a?(Crystal::Block) ? '_' : "_#{argument.name}" - issue_for argument.node, MSG % {argument.name, name_suggestion} + message = MSG % {argument.name, name_suggestion} + + location = argument.node.location + end_location = location.try &.adjust(column_number: argument.name.size - 1) + + if location && end_location + issue_for argument.node, message do |corrector| + corrector.replace(location, end_location, name_suggestion) + end + else + issue_for argument.node, message + end end end end diff --git a/src/ameba/rule/lint/unused_block_argument.cr b/src/ameba/rule/lint/unused_block_argument.cr index 077b9b23..0af82811 100644 --- a/src/ameba/rule/lint/unused_block_argument.cr +++ b/src/ameba/rule/lint/unused_block_argument.cr @@ -31,6 +31,8 @@ module Ameba::Rule::Lint # Enabled: true # ``` class UnusedBlockArgument < Base + include AST::Util + properties do description "Disallows unused block arguments" end @@ -51,11 +53,26 @@ module Ameba::Rule::Lint return if block_arg.anonymous? return if scope.references?(block_arg.variable) + location = block_arg.node.location + end_location = location.try &.adjust(column_number: block_arg.name.size - 1) + if scope.yields? - issue_for block_arg.node, MSG_YIELDED + if location && end_location + issue_for block_arg.node, MSG_YIELDED do |corrector| + corrector.remove(location, end_location) + end + else + issue_for block_arg.node, MSG_YIELDED + end else return if block_arg.ignored? - issue_for block_arg.node, MSG_UNUSED % block_arg.name + if location && end_location + issue_for block_arg.node, MSG_UNUSED % block_arg.name do |corrector| + corrector.insert_before(location, '_') + end + else + issue_for block_arg.node, MSG_UNUSED % block_arg.name + end end end end