Merge pull request #135 from crystal-ameba/feat/shared-var-in-spawn

New rule: SharedVarInSpawn

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 %(

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

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, scope)
-        variable.assign(assign_node)
+        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

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

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

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)

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? ||

src/ameba/ast/variabling/variable.cr

@@ -46,8 +46,8 @@ module Ameba::AST
     # variable.assignment.size # => 2
     # ```
     #
-    def assign(node)
+    def assign(node, scope)
-      assignments << Assignment.new(node, self)
+      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
 

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

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