diff --git a/spec/ameba/ast/scope_spec.cr b/spec/ameba/ast/scope_spec.cr index d2071676..cfc4f072 100644 --- a/spec/ameba/ast/scope_spec.cr +++ b/spec/ameba/ast/scope_spec.cr @@ -33,6 +33,20 @@ module Ameba::AST end end + describe "#references" do + it "can return an empty list of references" do + scope = Scope.new as_node("") + scope.references.should be_empty + end + + it "allows to add variable references" do + scope = Scope.new as_node("") + nodes = as_nodes "a = 2" + scope.references << Reference.new(nodes.var_nodes.first, scope) + scope.references.size.should eq 1 + end + end + describe "#add_variable" do it "adds a new variable to the scope" do scope = Scope.new as_node("") @@ -85,6 +99,21 @@ module Ameba::AST end end + describe "#spawn_block?" do + it "returns true if a node is a spawn block" do + nodes = as_nodes %( + spawn {} + ) + scope = Scope.new nodes.block_nodes.first + scope.spawn_block?.should be_true + end + + it "returns false otherwise" do + scope = Scope.new as_node "a = 1" + scope.spawn_block?.should be_false + end + end + describe "#macro?" do it "returns true if Crystal::Macro" do nodes = as_nodes %( diff --git a/spec/ameba/ast/variabling/assignment_spec.cr b/spec/ameba/ast/variabling/assignment_spec.cr index 90d0bc5a..7f50f6d1 100644 --- a/spec/ameba/ast/variabling/assignment_spec.cr +++ b/spec/ameba/ast/variabling/assignment_spec.cr @@ -8,14 +8,14 @@ module Ameba::AST describe "#initialize" do it "creates a new assignment with node and var" do - assignment = Assignment.new(node, variable) + assignment = Assignment.new(node, variable, scope) assignment.node.should_not be_nil end end describe "#reference=" do it "creates a new reference" do - assignment = Assignment.new(node, variable) + assignment = Assignment.new(node, variable, scope) assignment.referenced = true assignment.referenced?.should be_true end @@ -23,18 +23,18 @@ module Ameba::AST describe "delegation" do it "delegates locations" do - assignment = Assignment.new(node, variable) + assignment = Assignment.new(node, variable, scope) assignment.location.should eq node.location assignment.end_location.should eq node.end_location end it "delegates to_s" do - assignment = Assignment.new(node, variable) + assignment = Assignment.new(node, variable, scope) assignment.to_s.should eq node.to_s end it "delegates scope" do - assignment = Assignment.new(node, variable) + assignment = Assignment.new(node, variable, scope) assignment.scope.should eq variable.scope end end @@ -52,7 +52,7 @@ module Ameba::AST scope = Scope.new nodes.def_nodes.first variable = Variable.new(nodes.var_nodes.first, scope) - assignment = Assignment.new(nodes.assign_nodes.first, variable) + assignment = Assignment.new(nodes.assign_nodes.first, variable, scope) assignment.branch.should_not be_nil assignment.branch.not_nil!.node.class.should eq Crystal::Expressions end @@ -69,7 +69,7 @@ module Ameba::AST ) scope = Scope.new nodes.def_nodes.first variable = Variable.new(nodes.var_nodes.first, scope) - assignment = Assignment.new(nodes.assign_nodes.first, variable) + assignment = Assignment.new(nodes.assign_nodes.first, variable, scope) assignment.branch.should_not be_nil assignment.branch.not_nil!.node.class.should eq Crystal::Assign end @@ -83,7 +83,7 @@ module Ameba::AST scope = Scope.new nodes.def_nodes.first variable = Variable.new(nodes.var_nodes.first, scope) - assignment = Assignment.new(nodes.assign_nodes.first, variable) + assignment = Assignment.new(nodes.assign_nodes.first, variable, scope) assignment.branch.should be_nil end end @@ -98,7 +98,7 @@ module Ameba::AST scope = Scope.new nodes.def_nodes.first variable = Variable.new(nodes.var_nodes.first, scope) - assignment = Assignment.new(nodes.assign_nodes.first, variable) + assignment = Assignment.new(nodes.assign_nodes.first, variable, scope) assignment.transformed?.should be_false end @@ -110,7 +110,7 @@ module Ameba::AST scope = Scope.new nodes.block_nodes.first variable = Variable.new(nodes.var_nodes.first, scope) - assignment = Assignment.new(nodes.assign_nodes.first, variable) + assignment = Assignment.new(nodes.assign_nodes.first, variable, scope) assignment.transformed?.should be_true end end diff --git a/spec/ameba/ast/variabling/variable_spec.cr b/spec/ameba/ast/variabling/variable_spec.cr index 42c298b6..985074c2 100644 --- a/spec/ameba/ast/variabling/variable_spec.cr +++ b/spec/ameba/ast/variabling/variable_spec.cr @@ -47,14 +47,14 @@ module Ameba::AST it "assigns the variable (creates a new assignment)" do variable = Variable.new(var_node, scope) - variable.assign(assign_node) + variable.assign(assign_node, scope) variable.assignments.any?.should be_true end it "can create multiple assignments" do variable = Variable.new(var_node, scope) - variable.assign(assign_node) - variable.assign(assign_node) + variable.assign(assign_node, scope) + variable.assign(assign_node, scope) variable.assignments.size.should eq 2 end end @@ -62,10 +62,19 @@ module Ameba::AST describe "#reference" do it "references the existed assignment" do variable = Variable.new(var_node, scope) - variable.assign(as_node "foo=1") + variable.assign(as_node("foo=1"), scope) variable.reference(var_node, scope) variable.references.any?.should be_true end + + it "adds a reference to the scope" do + scope = Scope.new as_node "foo = 1" + variable = Variable.new(var_node, scope) + variable.assign(as_node("foo=1"), scope) + variable.reference(var_node, scope) + scope.references.size.should eq 1 + scope.references.first.node.to_s.should eq "foo" + end end describe "#captured_by_block?" 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 new file mode 100644 index 00000000..c1179dc3 --- /dev/null +++ b/spec/ameba/rule/lint/shared_var_in_fiber_spec.cr @@ -0,0 +1,236 @@ +require "../../../spec_helper" + +module Ameba::Rule::Lint + describe SharedVarInFiber do + subject = SharedVarInFiber.new + + it "doesn't report if there is only local shared var in fiber" do + s = Source.new %( + spawn do + i = 1 + puts i + end + + Fiber.yield + ) + subject.catch(s).should be_valid + end + + it "doesn't report if there is only block shared var in fiber" do + s = Source.new %( + 10.times do |i| + spawn do + puts i + end + end + + Fiber.yield + ) + subject.catch(s).should be_valid + end + + it "doesn't report if there a spawn macro is used" do + s = Source.new %( + i = 0 + while i < 10 + spawn puts(i) + i += 1 + end + + Fiber.yield + ) + subject.catch(s).should be_valid + end + + it "reports if there is a shared var in spawn" do + s = Source.new %( + i = 0 + while i < 10 + spawn do + puts(i) + end + i += 1 + end + + Fiber.yield + ) + subject.catch(s).should_not be_valid + end + + it "reports reassigned reference to shared var in spawn" do + s = Source.new %( + channel = Channel(String).new + n = 0 + + while n < 10 + n = n + 1 + spawn do + m = n + channel.send m + end + end + ) + subject.catch(s).should_not be_valid + end + + it "doesn't report reassigned reference to shared var in block" do + s = Source.new %( + channel = Channel(String).new + n = 0 + + while n < 3 + n = n + 1 + m = n + spawn do + channel.send m + end + end + ) + subject.catch(s).should be_valid + end + + it "does not report block is called in a spawn" do + s = Source.new %( + def method(block) + spawn do + block.call(10) + end + end + ) + subject.catch(s).should be_valid + end + + it "reports multiple shared variables in spawn" do + s = Source.new %( + foo, bar, baz = 0, 0, 0 + while foo < 10 + baz += 1 + spawn do + puts foo + puts foo + bar + baz + end + foo += 1 + end + ) + subject.catch(s).should_not be_valid + s.issues.size.should eq 3 + s.issues[0].location.to_s.should eq ":5:10" + s.issues[0].end_location.to_s.should eq ":5:12" + s.issues[0].message.should eq "Shared variable `foo` is used in fiber" + + s.issues[1].location.to_s.should eq ":6:10" + s.issues[1].end_location.to_s.should eq ":6:12" + s.issues[1].message.should eq "Shared variable `foo` is used in fiber" + + s.issues[2].location.to_s.should eq ":6:22" + s.issues[2].end_location.to_s.should eq ":6:24" + s.issues[2].message.should eq "Shared variable `baz` is used in fiber" + end + + it "doesn't report if variable is passed to the proc" do + s = Source.new %( + i = 0 + while i < 10 + proc = ->(x : Int32) do + spawn do + puts(x) + end + end + proc.call(i) + i += 1 + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if a channel is declared in outer scope" do + s = Source.new %( + channel = Channel(Nil).new + spawn { channel.send(nil) } + channel.receive + ) + subject.catch(s).should be_valid + end + + it "doesn't report if there is a loop in spawn" do + s = Source.new %( + channel = Channel(String).new + + spawn do + server = TCPServer.new("0.0.0.0", 8080) + socket = server.accept + while line = socket.gets + channel.send(line) + end + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if a var is mutated in spawn and referenced outside" do + s = Source.new %( + def method + foo = 1 + spawn { foo = 2 } + foo + end + ) + subject.catch(s).should be_valid + end + + it "doesn't report if variable is changed without iterations" do + s = Source.new %( + def foo + i = 0 + i += 1 + spawn { i } + end + ), "source.cr" + + subject.catch(s).should be_valid + end + + it "doesn't report if variable is in a loop inside spawn" do + s = Source.new %( + i = 0 + spawn do + while i < 10 + i += 1 + end + end + ) + + subject.catch(s).should be_valid + end + + it "doesn't report if variable declared inside loop" do + s = Source.new %( + while true + i = 0 + spawn { i += 1 } + end + ) + + subject.catch(s).should be_valid + end + + it "reports rule, location and message" do + s = Source.new %( + i = 0 + while true + i += 1 + spawn { i } + end + ), "source.cr" + + subject.catch(s).should_not be_valid + s.issues.size.should eq 1 + + issue = s.issues.first + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:4:11" + issue.end_location.to_s.should eq "source.cr:4:11" + issue.message.should eq "Shared variable `i` is used in fiber" + end + end +end diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr index c28615b9..34b50451 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/spec/ameba/rule/lint/useless_assign_spec.cr @@ -921,23 +921,6 @@ module Ameba::Rule::Lint 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 - it "doesn't report if assignment is referenced in a macro below" do s = Source.new %( class Foo diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index a8b47073..9cfd23f3 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 all variable references in currency scope + getter references = [] of Reference + # Link to the arguments in current scope getter arguments = [] of Argument @@ -85,7 +88,7 @@ module Ameba::AST # scope.assign_variable(var_name, assign_node) # ``` def assign_variable(name, node) - find_variable(name).try &.assign(node) + find_variable(name).try &.assign(node, self) end # Returns true if current scope represents a block (or proc), @@ -94,6 +97,19 @@ module Ameba::AST node.is_a?(Crystal::Block) || node.is_a?(Crystal::ProcLiteral) end + # Returns true if current scope represents a spawn block, e. g. + # + # ``` + # spawn do + # # ... + # end + # ``` + def spawn_block? + return false unless node.is_a?(Crystal::Block) + call = node.as(Crystal::Block).call + call.try(&.name) == "spawn" + end + # Returns true if currency scope represents a macro. def macro? node.is_a?(Crystal::Macro) diff --git a/src/ameba/ast/variabling/assignment.cr b/src/ameba/ast/variabling/assignment.cr index 77269205..6908572a 100644 --- a/src/ameba/ast/variabling/assignment.cr +++ b/src/ameba/ast/variabling/assignment.cr @@ -16,18 +16,20 @@ module Ameba::AST # Branch of this assignment. getter branch : Branch? + # A scope assignment belongs to + getter scope : Scope + delegate to_s, to: @node delegate location, to: @node delegate end_location, to: @node - delegate scope, to: @variable # Creates a new assignment. # # ``` - # Assignment.new(node, variable) + # Assignment.new(node, variable, scope) # ``` # - def initialize(@node, @variable) + def initialize(@node, @variable, @scope) if scope = @variable.scope @branch = Branch.of(@node, scope) @referenced = true if @variable.special? || diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index c94d67cf..476c25c0 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -46,8 +46,8 @@ module Ameba::AST # variable.assignment.size # => 2 # ``` # - def assign(node) - assignments << Assignment.new(node, self) + def assign(node, scope) + assignments << Assignment.new(node, self, scope) update_assign_reference! end @@ -73,6 +73,7 @@ module Ameba::AST def reference(node : Crystal::Var, scope : Scope) Reference.new(node, scope).tap do |reference| references << reference + scope.references << reference end end diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index 973568bc..22ac35e9 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -6,11 +6,14 @@ module Ameba::AST SUPER_NODE_NAME = "super" RECORD_NODE_NAME = "record" + @scope_queue = [] of Scope + @current_scope : Scope def initialize(@rule, @source) @current_scope = Scope.new(@source.ast) # top level scope @source.ast.accept self + @scope_queue.each { |scope| @rule.test @source, scope.node, scope } end private def on_scope_enter(node) @@ -18,9 +21,9 @@ module Ameba::AST end private def on_scope_end(node) - @rule.test @source, node, @current_scope + @scope_queue << @current_scope - # go up, if this is not a top level scope + # go up if this is not a top level scope if outer_scope = @current_scope.outer_scope @current_scope = outer_scope end diff --git a/src/ameba/rule/lint/shared_var_in_fiber.cr b/src/ameba/rule/lint/shared_var_in_fiber.cr new file mode 100644 index 00000000..b670f657 --- /dev/null +++ b/src/ameba/rule/lint/shared_var_in_fiber.cr @@ -0,0 +1,86 @@ +module Ameba::Rule::Lint + # A rule that disallows using shared variables in fibers. + # + # Using a shared variable in the `spawn` block in most cases + # leads to unexpected behaviour and is undesired. + # + # For example, having this example: + # + # ``` + # n = 0 + # channel = Channel(Int32).new + # + # while n < 3 + # n = n + 1 + # spawn { channel.send n } + # end + # + # 3.times { puts channel.receive } # => # 3, 3, 3 + # ``` + # + # The problem is there is only one shared between fibers variable `i` + # and when `channel.receive` is executed its value is `3`. + # + # To solve this, the code above needs to be rewritten to the following: + # + # ``` + # n = 0 + # channel = Channel(Int32).new + # + # while n < 3 + # n = n + 1 + # m = n + # spawn do { channel.send m } + # end + # + # 3.times { puts channel.receive } # => # 1, 2, 3 + # ``` + # + # This rule is able to find the shared variables between fibers, which are mutated + # during iterations. So it reports the issue on the first sample and passes on + # the second one. + # + # There are also other technics to solve the problem above which are + # [officially documented](https://crystal-lang.org/reference/guides/concurrency.html) + # + # YAML configuration example: + # + # ``` + # Lint/SharedVarInFiber: + # Enabled: true + # ``` + # + struct SharedVarInFiber < Base + properties do + description "Disallows shared variables in fibers." + end + + MSG = "Shared variable `%s` is used in fiber" + + def test(source) + AST::ScopeVisitor.new self, source + end + + def test(source, node, scope : AST::Scope) + return unless scope.spawn_block? + + scope.references.each do |ref| + next if (variable = scope.find_variable(ref.name)).nil? + next if variable.scope == scope || !mutated_in_loop?(variable) + + issue_for ref.node, MSG % variable.name + end + end + + # Variable is mutated in loop if it was declared above the loop and assigned inside. + private def mutated_in_loop?(variable) + declared_in = variable.assignments.first?.try &.branch + + variable.assignments + .reject { |assign| assign.scope.spawn_block? } + .any? do |assign| + assign.branch.try(&.in_loop?) && assign.branch != declared_in + end + end + end +end