diff --git a/spec/ameba/rule/lint/unused_argument_spec.cr b/spec/ameba/rule/lint/unused_argument_spec.cr index 125ff5f8..47be8ce9 100644 --- a/spec/ameba/rule/lint/unused_argument_spec.cr +++ b/spec/ameba/rule/lint/unused_argument_spec.cr @@ -115,21 +115,21 @@ module Ameba::Rule::Lint subject.catch(s).should be_valid end - it "reports if block arg is not used" do + it "doesn't report if block arg is not used" do s = Source.new %( def method(&block) end ) - subject.catch(s).should_not be_valid + subject.catch(s).should be_valid end - it "reports if unused and there is yield" do + it "doesn't report if unused and there is yield" do s = Source.new %( def method(&block) yield 1 end ) - subject.catch(s).should_not be_valid + subject.catch(s).should be_valid end it "doesn't report if it's an anonymous block" do diff --git a/spec/ameba/rule/lint/unused_block_argument_spec.cr b/spec/ameba/rule/lint/unused_block_argument_spec.cr new file mode 100644 index 00000000..adcdc6a1 --- /dev/null +++ b/spec/ameba/rule/lint/unused_block_argument_spec.cr @@ -0,0 +1,110 @@ +require "../../../spec_helper" + +module Ameba::Rule::Lint + subject = UnusedBlockArgument.new + + describe UnusedBlockArgument do + it "doesn't report if it is an instance var argument" do + s = Source.new %( + class A + def initialize(&@callback) + end + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if anonymous" do + s = Source.new %( + def method(a, b, c, &) + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if argument name starts with a `_`" do + s = Source.new %( + def method(a, b, c, &_block) + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if it is a block and used" do + s = Source.new %( + def method(a, b, c, &block) + block.call + end + ) + subject.catch(s).should be_valid + end + + it "reports if block arg is not used" do + s = Source.new %( + def method(a, b, c, &block) + end + ) + subject.catch(s).should_not be_valid + end + + it "reports if unused and there is yield" do + s = Source.new %( + def method(a, b, c, &block) + 3.times do |i| + i.try do + yield i + end + end + end + ) + subject.catch(s).should_not be_valid + end + + it "doesn't report if anonymous and there is yield" do + s = Source.new %( + def method(a, b, c, &) + yield 1 + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if variable is referenced implicitly" do + s = Source.new %( + class Bar < Foo + def method(a, b, c, &block) + super + end + end + ) + subject.catch(s).should be_valid + end + + context "super" do + it "reports if variable is not referenced implicitly by super" do + s = Source.new %( + class Bar < Foo + def method(a, b, c, &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." + end + end + + context "macro" do + it "doesn't report if it is a used macro block argument" do + s = Source.new %( + macro my_macro(&block) + {% block %} + end + ) + subject.catch(s).should be_valid + end + end + end +end diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index e8f763d5..ba0459c3 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -4,6 +4,9 @@ module Ameba::AST # Represents a context of the local variable visibility. # This is where the local variables belong to. class Scope + # Whether the scope yields. + setter yields = false + # Link to local variables getter variables = [] of Variable @@ -143,6 +146,14 @@ module Ameba::AST end || variable.used_in_macro? end + # Returns `true` if current scope (or any of inner scopes) yields, + # `false` otherwise. + def yields?(check_inner_scopes = true) + return true if @yields + return inner_scopes.any?(&.yields?) if check_inner_scopes + false + end + # Returns `true` if current scope is a def, `false` otherwise. def def? node.is_a?(Crystal::Def) diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index be369b2c..1dec9af8 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -19,8 +19,8 @@ module Ameba::AST MacroFor, } - SUPER_NODE_NAME = "super" - RECORD_NODE_NAME = "record" + SPECIAL_NODE_NAMES = %w[super previous_def] + RECORD_NODE_NAME = "record" @scope_queue = [] of Scope @current_scope : Scope @@ -64,6 +64,11 @@ module Ameba::AST end {% end %} + # :nodoc: + def visit(node : Crystal::Yield) + @current_scope.yields = true + end + # :nodoc: def visit(node : Crystal::Def) node.name == "->" || on_scope_enter(node) @@ -134,7 +139,7 @@ module Ameba::AST def visit(node : Crystal::Call) case when @current_scope.def? - if node.name == SUPER_NODE_NAME && node.args.empty? + if node.name.in?(SPECIAL_NODE_NAMES) && node.args.empty? @current_scope.arguments.each do |arg| variable = arg.variable variable.reference(variable.node, @current_scope).explicit = false diff --git a/src/ameba/rule/lint/unused_argument.cr b/src/ameba/rule/lint/unused_argument.cr index 66879d27..c42668a5 100644 --- a/src/ameba/rule/lint/unused_argument.cr +++ b/src/ameba/rule/lint/unused_argument.cr @@ -50,6 +50,10 @@ module Ameba::Rule::Lint end def test(source, node : Crystal::Def, scope : AST::Scope) + # `Lint/UnusedBlockArgument` rule covers this case explicitly + if block_arg = node.block_arg + scope.arguments.reject!(&.node.== block_arg) + end ignore_defs? || find_unused_arguments source, scope end diff --git a/src/ameba/rule/lint/unused_block_argument.cr b/src/ameba/rule/lint/unused_block_argument.cr new file mode 100644 index 00000000..077b9b23 --- /dev/null +++ b/src/ameba/rule/lint/unused_block_argument.cr @@ -0,0 +1,62 @@ +module Ameba::Rule::Lint + # A rule that reports unused block arguments. + # For example, this is considered invalid: + # + # ``` + # def foo(a, b, &block) + # a + b + # end + # + # def bar(&block) + # yield 42 + # end + # ``` + # + # and should be written as: + # + # ``` + # def foo(a, b, &_block) + # a + b + # end + # + # def bar(&) + # yield 42 + # end + # ``` + # + # YAML configuration example: + # + # ``` + # Lint/UnusedBlockArgument: + # Enabled: true + # ``` + class UnusedBlockArgument < Base + properties do + description "Disallows unused block arguments" + end + + MSG_UNUSED = "Unused block argument `%1$s`. If it's necessary, use `_%1$s` " \ + "as an argument name to indicate that it won't be used." + + MSG_YIELDED = "Use `&` as an argument name to indicate that it won't be referenced." + + def test(source) + AST::ScopeVisitor.new self, source + end + + def test(source, node : Crystal::Def, scope : AST::Scope) + return unless block_arg = node.block_arg + return unless block_arg = scope.arguments.find(&.node.== block_arg) + + return if block_arg.anonymous? + return if scope.references?(block_arg.variable) + + if scope.yields? + issue_for block_arg.node, MSG_YIELDED + else + return if block_arg.ignored? + issue_for block_arg.node, MSG_UNUSED % block_arg.name + end + end + end +end diff --git a/src/ameba/rule/style/type_names.cr b/src/ameba/rule/style/type_names.cr index ebaf23e5..40d24697 100644 --- a/src/ameba/rule/style/type_names.cr +++ b/src/ameba/rule/style/type_names.cr @@ -58,16 +58,12 @@ module Ameba::Rule::Style MSG = "Type name should be camelcased: %s, but it was %s" - private def check_node(source, node) + def test(source, node : Crystal::Alias | Crystal::ClassDef | Crystal::ModuleDef | Crystal::LibDef | Crystal::EnumDef) name = node.name.to_s expected = name.camelcase return if name == expected issue_for node, MSG % {expected, name} end - - def test(source, node : Crystal::Alias | Crystal::ClassDef | Crystal::ModuleDef | Crystal::LibDef | Crystal::EnumDef) - check_node(source, node) - end end end diff --git a/src/ameba/rule/style/variable_names.cr b/src/ameba/rule/style/variable_names.cr index c3cded64..e2ae8edf 100644 --- a/src/ameba/rule/style/variable_names.cr +++ b/src/ameba/rule/style/variable_names.cr @@ -29,18 +29,16 @@ module Ameba::Rule::Style MSG = "Var name should be underscore-cased: %s, not %s" - private def check_node(source, node) - return if (expected = node.name.underscore) == node.name - - issue_for node, MSG % {expected, node.name} - end - def test(source : Source) VarVisitor.new self, source end def test(source, node : Crystal::Var | Crystal::InstanceVar | Crystal::ClassVar) - check_node source, node + name = node.name.to_s + expected = name.underscore + return if name == expected + + issue_for node, MSG % {expected, name} end private class VarVisitor < AST::NodeVisitor