diff --git a/spec/ameba/ast/variabling/argument_spec.cr b/spec/ameba/ast/variabling/argument_spec.cr new file mode 100644 index 00000000..98952a96 --- /dev/null +++ b/spec/ameba/ast/variabling/argument_spec.cr @@ -0,0 +1,40 @@ +require "../../../spec_helper" + +module Ameba::AST + describe Argument do + arg = Crystal::Arg.new "a" + scope = Scope.new as_node "foo = 1" + variable = Variable.new(Crystal::Var.new("foo"), scope) + + describe "#initialize" do + it "creates a new argument" do + argument = Argument.new(arg, variable) + argument.node.should_not be_nil + end + end + + describe "delegation" do + it "delegates location to node" do + argument = Argument.new(arg, variable) + argument.location.should eq arg.location + end + + it "delegates to_s to node" do + argument = Argument.new(arg, variable) + argument.to_s.should eq arg.to_s + end + end + + describe "#ignored?" do + it "is true if arg starts with _" do + argument = Argument.new(Crystal::Arg.new("_a"), variable) + argument.ignored?.should be_true + end + + it "is false otherwise" do + argument = Argument.new(arg, variable) + argument.ignored?.should be_false + end + end + end +end diff --git a/spec/ameba/rule/unused_argument_spec.cr b/spec/ameba/rule/unused_argument_spec.cr new file mode 100644 index 00000000..2df81e0c --- /dev/null +++ b/spec/ameba/rule/unused_argument_spec.cr @@ -0,0 +1,259 @@ +require "../../spec_helper" + +module Ameba::Rule + subject = UnusedArgument.new + subject.ignore_defs = false + + describe UnusedArgument do + it "doesn't report if arguments are used" do + s = Source.new %( + def method(a, b, c) + a + b + c + end + + 3.times do |i| + i + 1 + end + + ->(i : Int32) { i + 1 } + ) + subject.catch(s).should be_valid + end + + it "reports if method argument is unused" do + s = Source.new %( + def method(a, b, c) + a + b + end + ) + subject.catch(s).should_not be_valid + s.errors.first.message.should eq "Unused argument `c`" + end + + it "reports if block argument is unused" do + s = Source.new %( + [1,2].each_with_index do |a, i| + a + end + ) + subject.catch(s).should_not be_valid + s.errors.first.message.should eq "Unused argument `i`" + end + + it "reports if proc argument is unused" do + s = Source.new %( + -> (a : Int32, b : String) do + a = a + 1 + end + ) + subject.catch(s).should_not be_valid + s.errors.first.message.should eq "Unused argument `b`" + end + + it "reports multiple unused args" do + s = Source.new %( + def method(a, b, c) + nil + end + ) + subject.catch(s).should_not be_valid + s.errors[0].message.should eq "Unused argument `a`" + s.errors[1].message.should eq "Unused argument `b`" + s.errors[2].message.should eq "Unused argument `c`" + end + + it "doesn't report if it is an instance var argument" do + s = Source.new %( + class A + def method(@name) + end + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if a typed argument is used" do + s = Source.new %( + def method(x : Int32) + 3.times do + puts x + end + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if an argument with default value is used" do + s = Source.new %( + def method(x = 1) + puts x + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if argument starts with a _" do + s = Source.new %( + def method(_x) + 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(&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(&block) + end + ) + subject.catch(s).should_not be_valid + end + + it "reports if unused and there is yield" do + s = Source.new %( + def method(&block) + yield 1 + end + ) + subject.catch(s).should_not be_valid + end + + it "doesn't report if variable is referenced implicitly" do + s = Source.new %( + class Bar < Foo + def method(a, b) + 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) + super a + end + end + ) + subject.catch(s).should_not be_valid + s.errors.first.message.should eq "Unused argument `b`" + end + + it "reports rule, location and message" do + s = Source.new %( + def method(a) + end + ), "source.cr" + subject.catch(s).should_not be_valid + error = s.errors.first + error.rule.should_not be_nil + error.message.should eq "Unused argument `a`" + error.location.to_s.should eq "source.cr:2:22" + end + end + + context "macro" do + it "doesn't report if it is a used macro argument" do + s = Source.new %( + macro my_macro(arg) + {% arg %} + end + ) + subject.catch(s).should be_valid + end + + 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 + + it "doesn't report used macro args with equal names in record" do + s = Source.new %( + record X do + macro foo(a, b) + {{a}} + {{b}} + end + + macro bar(a, b, c) + {{a}} + {{b}} + {{c}} + end + end + ) + subject.catch(s).should be_valid + 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 %( + def method(a) + end + ) + subject.catch(s).should be_valid + end + + it "lets the rule not to ignore def scopes if false" do + subject.ignore_defs = false + s = Source.new %( + def method(a) + end + ) + subject.catch(s).should_not be_valid + end + end + + context "#ignore_blocks" do + it "lets the rule to ignore block scopes if true" do + subject.ignore_blocks = true + s = Source.new %( + 3.times { |i| puts "yo!" } + ) + subject.catch(s).should be_valid + end + + it "lets the rule not to ignore block scopes if false" do + subject.ignore_blocks = false + s = Source.new %( + 3.times { |i| puts "yo!" } + ) + subject.catch(s).should_not be_valid + end + end + + context "#ignore_procs" do + it "lets the rule to ignore proc scopes if true" do + subject.ignore_procs = true + s = Source.new %( + ->(a : Int32) {} + ) + subject.catch(s).should be_valid + end + + it "lets the rule not to ignore proc scopes if false" do + subject.ignore_procs = false + s = Source.new %( + ->(a : Int32) {} + ) + subject.catch(s).should_not be_valid + end + end + end + end +end diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 6c8e014a..1a287385 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -7,6 +7,9 @@ module Ameba::AST # Link to local variables getter variables = [] of Variable + # Link to the arguments in current scope + getter arguments = [] of Argument + # Link to the outer scope getter outer_scope : Scope? @@ -40,6 +43,11 @@ module Ameba::AST variables << Variable.new(node, self) end + def add_argument(node) + add_variable Crystal::Var.new(node.name).at(node.location) + arguments << Argument.new(node, variables.last) + end + # Returns variable by its name or nil if it does not exist. # # ``` @@ -77,34 +85,43 @@ module Ameba::AST node.is_a?(Crystal::CStructOrUnionDef) end - # Returns true if current scope references variable, false if not. + # Returns true if current scope (or any of inner scopes) references variable, + # false if not. def references?(variable : Variable) - variable.references.any? { |reference| reference.scope == self } + variable.references.any? do |reference| + reference.scope == self || inner_scopes.any?(&.references? variable) + end end - # Returns arguments of this scope (if any). - def args + # Returns true if current scope is a def, false if not. + def def? + node.is_a? Crystal::Def + end + + # Returns true if var is an argument in current scope, false if not. + def arg?(var) case current_node = node - when Crystal::Block, Crystal::Def then current_node.args - when Crystal::ProcLiteral then current_node.def.args + when Crystal::Def + var.is_a?(Crystal::Arg) && any_arg?(current_node.args, var) + when Crystal::Block + var.is_a?(Crystal::Var) && any_arg?(current_node.args, var) + when Crystal::ProcLiteral + var.is_a?(Crystal::Var) && any_arg?(current_node.def.args, var) else - [] of Crystal::Var + false end end - # Returns true if variable is an argument in current scope, false if not. - def arg?(var : Crystal::Var) - args.any? do |arg| - arg.is_a?(Crystal::Var) && - arg.name == var.name && - arg.location == var.location - end + private def any_arg?(args, var) + args.any? { |arg| arg.name == var.name && arg.location == var.location } end # Returns true if the `node` represents exactly # the same Crystal node as `@node`. def eql?(node) - node == @node && node.location == @node.location + node == @node && + !node.location.nil? && + node.location == @node.location end end end diff --git a/src/ameba/ast/variabling/argument.cr b/src/ameba/ast/variabling/argument.cr new file mode 100644 index 00000000..97151f6e --- /dev/null +++ b/src/ameba/ast/variabling/argument.cr @@ -0,0 +1,48 @@ +module Ameba::AST + # Represents the argument of some node. + # Holds the reference to the variable, thus to scope. + # + # For example, all these vars are arguments: + # + # ``` + # def method(a, b, c = 10, &block) + # 3.times do |i| + # end + # + # ->(x : Int32) {} + # end + # ``` + class Argument + # The actual node. + getter node : Crystal::Var | Crystal::Arg + + # Variable of this argument (may be the same node) + getter variable : Variable + + delegate location, to: @node + delegate to_s, to: @node + + # Creates a new argument. + # + # ``` + # Argument.new(node, variable) + # ``` + def initialize(@node, @variable) + end + + # Returns true if the name starts with '_', false if not. + def ignored? + name.starts_with? '_' + end + + # Name of the argument. + def name + case current_node = node + when Crystal::Var then current_node.name + when Crystal::Arg then current_node.name + else + raise ArgumentError.new "invalid node" + end + end + end +end diff --git a/src/ameba/ast/variabling/reference.cr b/src/ameba/ast/variabling/reference.cr index e85b3225..885234d9 100644 --- a/src/ameba/ast/variabling/reference.cr +++ b/src/ameba/ast/variabling/reference.cr @@ -5,5 +5,6 @@ module Ameba::AST # It behaves like a variable is used to distinguish a # the variable from its reference. class Reference < Variable + property? explicit = true end end diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index f7641b70..1a0d1349 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -105,8 +105,6 @@ module Ameba::AST # 3.times { |i| i + 1 } # ``` def captured_by_block?(scope = @scope) - return false unless scope - scope.inner_scopes.each do |inner_scope| return true if inner_scope.block? && inner_scope.references?(self) return true if captured_by_block?(inner_scope) diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index a878a261..ac9c8d34 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -73,6 +73,11 @@ module Ameba::AST on_scope_enter(node) end + # :nodoc: + def visit(node : Crystal::Macro) + on_scope_enter(node) + end + @current_assign : Crystal::ASTNode? # :nodoc: @@ -94,19 +99,22 @@ module Ameba::AST # :nodoc: def visit(node : Crystal::Arg) - @current_scope.add_variable Crystal::Var.new(node.name).at(node.location) + @current_scope.add_argument node end # :nodoc: def visit(node : Crystal::Var) - if !@current_scope.arg?(node) && (variable = @current_scope.find_variable node.name) - reference = variable.reference node, @current_scope + variable = @current_scope.find_variable node.name + if @current_scope.arg?(node) # node is an argument + @current_scope.add_argument(node) + elsif variable.nil? && @current_assign # node is a variable + @current_scope.add_variable(node) + elsif variable # node is a reference + reference = variable.reference node, @current_scope if @current_assign.is_a?(Crystal::OpAssign) || !reference.target_of?(@current_assign) variable.reference_assignments! end - else - @current_scope.add_variable(node) end end @@ -114,6 +122,17 @@ module Ameba::AST def visit(node : Crystal::MacroLiteral) MacroLiteralVarVisitor.new(node).vars.each { |var| visit(var) } end + + # :nodoc: + def visit(node : Crystal::Call) + return true unless node.name == "super" && node.args.empty? + return true unless (scope = @current_scope).def? + scope.arguments.each do |arg| + variable = arg.variable + variable.reference(variable.node, scope).explicit = false + end + true + end end private class MacroLiteralVarVisitor < Crystal::Visitor diff --git a/src/ameba/rule/unused_argument.cr b/src/ameba/rule/unused_argument.cr new file mode 100644 index 00000000..f4ef6a61 --- /dev/null +++ b/src/ameba/rule/unused_argument.cr @@ -0,0 +1,63 @@ +module Ameba::Rule + # A rule that reports unused arguments. + # For example, this is considered invalid: + # + # ``` + # def method(a, b, c) + # a + b + # end + # ``` + # and should be written as: + # + # ``` + # def method(a, b) + # a + b + # end + # ``` + # + # YAML configuration example: + # + # ``` + # UnusedArgument: + # Enabled: true + # IgnoreDefs: true + # IgnoreBlocks: false + # IgnoreProcs: false + # ``` + # + struct UnusedArgument < Base + properties do + description "Disallows unused arguments" + + ignore_defs true + ignore_blocks false + ignore_procs false + end + + MSG = "Unused argument `%s`" + + def test(source) + AST::ScopeVisitor.new self, source + end + + def test(source, node : Crystal::ProcLiteral, scope : AST::Scope) + ignore_procs || find_unused_arguments source, scope + end + + def test(source, node : Crystal::Block, scope : AST::Scope) + ignore_blocks || find_unused_arguments source, scope + end + + def test(source, node : Crystal::Def, scope : AST::Scope) + ignore_defs || find_unused_arguments source, scope + end + + private def find_unused_arguments(source, scope) + scope.arguments.each do |argument| + next if argument.ignored? || scope.references?(argument.variable) + + source.error self, argument.location, MSG % argument.name + end + end + end +end