Skip to content

Commit

Permalink
Prevent false positiveness cause by macro literals
Browse files Browse the repository at this point in the history
  • Loading branch information
veelenga committed May 12, 2018
1 parent 6579c8f commit 4154327
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 25 deletions.
31 changes: 31 additions & 0 deletions spec/ameba/rule/useless_assign_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -820,6 +820,37 @@ module Ameba::Rule
)
subject.catch(s).should be_valid
end

it "doesn't report if assignment is referenced in macro def" do
s = Source.new %(
macro macro_call
puts x
end
def foo
x = 1
macro_call
end
)
subject.catch(s).should be_valid
end

it "reports if assignment is referenced in macro def in a different scope" do
s = Source.new %(
class Foo
def foo
x = 1
end
end
class Bar
macro macro_call
puts x
end
end
)
subject.catch(s).should_not be_valid
end
end
end
end
3 changes: 3 additions & 0 deletions src/ameba/ast/scope.cr
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ module Ameba::AST
# The actual AST node that represents a current scope.
getter node : Crystal::ASTNode

# Macro literals in current scope
getter macro_literals = [] of Crystal::MacroLiteral

delegate to_s, to: node
delegate location, to: node

Expand Down
12 changes: 11 additions & 1 deletion src/ameba/ast/variabling/variable.cr
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ module Ameba::AST
end
end

# Returns true if the current assignment is referenced in
# Returns true if the current var is referenced in
# in the block. For example this variable is captured
# by block:
#
Expand All @@ -113,6 +113,16 @@ module Ameba::AST
false
end

# Returns true if current variable potentially referenced in a macro literal,
# false if not.
def used_in_macro?(scope = @scope)
scope.inner_scopes.each do |inner_scope|
return true if inner_scope.macro_literals.any? { |literal| literal.value.includes?(name) }
end
return true if (outer_scope = scope.outer_scope) && used_in_macro?(outer_scope)
false
end

# Returns true if the variable is a target (on the left) of the assignment,
# false otherwise.
def target_of?(assign)
Expand Down
24 changes: 1 addition & 23 deletions src/ameba/ast/visitors/scope_visitor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ module Ameba::AST

# :nodoc:
def visit(node : Crystal::MacroLiteral)
MacroLiteralVarVisitor.new(node).vars.each { |var| visit(var) }
@current_scope.macro_literals << node
end

# :nodoc:
Expand All @@ -134,26 +134,4 @@ module Ameba::AST
true
end
end

private class MacroLiteralVarVisitor < Crystal::Visitor
getter vars = [] of Crystal::Var

def initialize(literal)
Crystal::Parser.new(literal.value).parse.accept self
rescue
nil
end

def visit(node : Crystal::ASTNode)
true
end

def visit(node : Crystal::Var)
vars << node
end

def visit(node : Crystal::Call)
vars << Crystal::Var.new(node.name).at(node.location)
end
end
end
2 changes: 1 addition & 1 deletion src/ameba/rule/useless_assign.cr
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ module Ameba::Rule

def test(source, node, scope : AST::Scope)
scope.variables.each do |var|
next if var.captured_by_block?
next if var.captured_by_block? || var.used_in_macro?

var.assignments.each do |assign|
next if assign.referenced?
Expand Down

0 comments on commit 4154327

Please sign in to comment.