From a8f07e2437f074997e21eb9089f953939ab0b366 Mon Sep 17 00:00:00 2001
From: Ary Borenszweig <asterite@gmail.com>
Date: Sun, 15 Dec 2019 23:57:05 -0500
Subject: [PATCH 1/5] Compiler: fix closured instance variable not being
 considered as such

---
 spec/compiler/semantic/closure_spec.cr        | 24 +++++++++++++++++++
 src/compiler/crystal/semantic/ast.cr          |  8 +++++++
 src/compiler/crystal/semantic/bindings.cr     |  4 ++++
 src/compiler/crystal/semantic/main_visitor.cr | 23 ++++++++++++++++--
 4 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/spec/compiler/semantic/closure_spec.cr b/spec/compiler/semantic/closure_spec.cr
index 69f26dec829f..3e3e030987f5 100644
--- a/spec/compiler/semantic/closure_spec.cr
+++ b/spec/compiler/semantic/closure_spec.cr
@@ -524,4 +524,28 @@ describe "Semantic: closure" do
       ),
       "can't send closure to C function (closured vars: x)"
   end
+
+  it "correctly detects previous var as closured (#5609)" do
+    assert_error %(
+      def block(&block)
+        block.call
+      end
+
+      def times
+        yield
+        yield
+      end
+
+      x = 1
+      times do
+        if x.is_a?(Int32)
+          x &+ 2
+        end
+        block do
+          x = "hello"
+        end
+      end
+      ),
+      "undefined method '&+' for String"
+  end
 end
diff --git a/src/compiler/crystal/semantic/ast.cr b/src/compiler/crystal/semantic/ast.cr
index 41819a51e2fd..1b0c1294f030 100644
--- a/src/compiler/crystal/semantic/ast.cr
+++ b/src/compiler/crystal/semantic/ast.cr
@@ -450,9 +450,17 @@ module Crystal
     # Is this metavar assigned a value?
     property? assigned_to = false
 
+    # Local variables associated with this meta variable.
+    # Can be Var or MetaVar.
+    property(local_vars) { [] of ASTNode }
+
     def initialize(@name : String, @type : Type? = nil)
     end
 
+    def local_vars?
+      @local_vars
+    end
+
     # True if this variable belongs to the given context
     # but must be allocated in a closure.
     def closure_in?(context)
diff --git a/src/compiler/crystal/semantic/bindings.cr b/src/compiler/crystal/semantic/bindings.cr
index b9a82928aea4..946fa26bda9f 100644
--- a/src/compiler/crystal/semantic/bindings.cr
+++ b/src/compiler/crystal/semantic/bindings.cr
@@ -164,6 +164,10 @@ module Crystal
       nodes.each &.remove_observer self
     end
 
+    def bound_to?(node : ASTNode)
+      @dependencies.try &.any? &.same?(node)
+    end
+
     def add_observer(observer)
       observers = @observers ||= [] of ASTNode
       observers.push observer
diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr
index 238a353dfe6e..e4407f55d4d6 100644
--- a/src/compiler/crystal/semantic/main_visitor.cr
+++ b/src/compiler/crystal/semantic/main_visitor.cr
@@ -361,7 +361,11 @@ module Crystal
         end
 
         if meta_var.closured?
-          var.bind_to(meta_var)
+          unless var.bound_to?(meta_var)
+            var.bind_to(meta_var)
+          end
+        else
+          meta_var.local_vars << var
         end
 
         node.bind_to(var)
@@ -809,7 +813,11 @@ module Crystal
         simple_var.bind_to(target)
 
         if meta_var.closured?
-          simple_var.bind_to(meta_var)
+          unless simple_var.bound_to? meta_var
+            simple_var.bind_to(meta_var)
+          end
+        else
+          meta_var.local_vars << simple_var
         end
       end
 
@@ -3196,6 +3204,17 @@ module Crystal
           # to the context where the variable is defined
           visitor = self
           while visitor
+            # Grab the meta var in the visitor's scope and bind all
+            # its local vars to it. This is a fix for #5609.
+            meta_var = visitor.meta_vars[var.name]?
+            if meta_var
+              meta_var.local_vars?.try &.each do |local_var|
+                unless local_var.bound_to? local_var
+                  local_var.bind_to meta_var
+                end
+              end
+            end
+
             visitor_context = visitor.closure_context
             break if visitor_context == var_context
 

From 9bf5a973b77755fb24b8b7c62d7851d2f320a147 Mon Sep 17 00:00:00 2001
From: Ary Borenszweig <asterite@gmail.com>
Date: Mon, 16 Dec 2019 00:23:52 -0500
Subject: [PATCH 2/5] Try something...

---
 src/process.cr | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/process.cr b/src/process.cr
index 566e1fae5755..735d42b92051 100644
--- a/src/process.cr
+++ b/src/process.cr
@@ -235,7 +235,7 @@ class Process
   private def stdio_to_fd(stdio : Stdio, for dst_io : IO::FileDescriptor) : IO::FileDescriptor
     case stdio
     when IO::FileDescriptor
-      stdio
+      stdio.as(IO::FileDescriptor)
     when IO
       if dst_io == STDIN
         fork_io, process_io = IO.pipe(read_blocking: true)

From 99a8f3b39bf797cc9258e6f3f85ffa8e364b75fd Mon Sep 17 00:00:00 2001
From: Ary Borenszweig <asterite@gmail.com>
Date: Mon, 16 Dec 2019 05:57:11 -0500
Subject: [PATCH 3/5] Proper use of ensure

---
 spec/std/socket/socket_spec.cr | 37 ++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/spec/std/socket/socket_spec.cr b/spec/std/socket/socket_spec.cr
index 9b9ecb146c60..2b6be2666a5e 100644
--- a/spec/std/socket/socket_spec.cr
+++ b/spec/std/socket/socket_spec.cr
@@ -45,23 +45,30 @@ describe Socket do
   it "sends messages" do
     port = unused_local_port
     server = Socket.tcp(Socket::Family::INET6)
-    server.bind("::1", port)
-    server.listen
-    address = Socket::IPAddress.new("::1", port)
-    spawn do
-      client = server.not_nil!.accept
-      client.gets.should eq "foo"
-      client.puts "bar"
+    begin
+      server.bind("::1", port)
+      server.listen
+      address = Socket::IPAddress.new("::1", port)
+      spawn do
+        client = server.accept
+        begin
+          client.gets.should eq "foo"
+          client.puts "bar"
+        ensure
+          client.close
+        end
+      end
+      socket = Socket.tcp(Socket::Family::INET6)
+      begin
+        socket.connect(address)
+        socket.puts "foo"
+        socket.gets.should eq "bar"
+      ensure
+        socket.close
+      end
     ensure
-      client.try &.close
+      server.close
     end
-    socket = Socket.tcp(Socket::Family::INET6)
-    socket.connect(address)
-    socket.puts "foo"
-    socket.gets.should eq "bar"
-  ensure
-    socket.try &.close
-    server.try &.close
   end
 
   describe "#bind" do

From c871cf1d03dae658371c8a4dd79694806282d84f Mon Sep 17 00:00:00 2001
From: Ary Borenszweig <asterite@gmail.com>
Date: Mon, 16 Dec 2019 14:00:43 -0300
Subject: [PATCH 4/5] Compiler: don't bind the type of a metavar to all vars if
 assigned once

---
 spec/compiler/semantic/closure_spec.cr        | 51 ++++++++++
 src/compiler/crystal/semantic/ast.cr          | 27 ++++-
 src/compiler/crystal/semantic/bindings.cr     |  4 +
 src/compiler/crystal/semantic/main_visitor.cr | 99 ++++++++++---------
 4 files changed, 134 insertions(+), 47 deletions(-)

diff --git a/spec/compiler/semantic/closure_spec.cr b/spec/compiler/semantic/closure_spec.cr
index 3e3e030987f5..ac43a7dbab19 100644
--- a/spec/compiler/semantic/closure_spec.cr
+++ b/spec/compiler/semantic/closure_spec.cr
@@ -548,4 +548,55 @@ describe "Semantic: closure" do
       ),
       "undefined method '&+' for String"
   end
+
+  it "doesn't assign all types to metavar if closured but only assigned to once" do
+    semantic(%(
+      def capture(&block)
+        block
+      end
+
+      x = 1 == 2 ? 1 : nil
+      if x
+        capture do
+          x &+ 1
+        end
+      end
+      ))
+  end
+
+  it "does assign all types to metavar if closured but only assigned to once in a loop" do
+    assert_error %(
+      def capture(&block)
+        block
+      end
+
+      while 1 == 1
+        x = 1 == 2 ? 1 : nil
+        if x
+          capture do
+            x &+ 1
+          end
+        end
+      end
+      ),
+      "undefined method '&+'"
+  end
+
+  it "considered as closure if assigned once but comes from a method arg" do
+    assert_error %(
+      def capture(&block)
+        block
+      end
+
+      def foo(x)
+        capture do
+          x &+ 1
+        end
+        x = 1 == 2 ? 1 : nil
+      end
+
+      foo(1)
+      ),
+      "undefined method '&+'"
+  end
 end
diff --git a/src/compiler/crystal/semantic/ast.cr b/src/compiler/crystal/semantic/ast.cr
index 1b0c1294f030..62da4fe4cb37 100644
--- a/src/compiler/crystal/semantic/ast.cr
+++ b/src/compiler/crystal/semantic/ast.cr
@@ -447,9 +447,15 @@ module Crystal
     # where it wasn't created.
     property? closured = false
 
-    # Is this metavar assigned a value?
+    # Is this var used inside a while loop?
+    property? inside_loop = false
+
+    # Was something assigned to this metavar?
     property? assigned_to = false
 
+    # Was something assigned to this metavar multiple times?
+    property? assigned_to_multiple_times = false
+
     # Local variables associated with this meta variable.
     # Can be Var or MetaVar.
     property(local_vars) { [] of ASTNode }
@@ -457,10 +463,26 @@ module Crystal
     def initialize(@name : String, @type : Type? = nil)
     end
 
+    # Incrase the times something was assigned to this metavar.
+    def increase_assigned_count
+      if assigned_to?
+        @assigned_to_multiple_times = true
+      else
+        @assigned_to = true
+      end
+    end
+
+    # Is this metavar associated with any local vars?
     def local_vars?
       @local_vars
     end
 
+    # Is this var closured and also all local vars related to it
+    # should be bound to it?
+    def closured_multibound?
+      closured? && (assigned_to_multiple_times? || inside_loop?)
+    end
+
     # True if this variable belongs to the given context
     # but must be allocated in a closure.
     def closure_in?(context)
@@ -488,7 +510,8 @@ module Crystal
       end
       io << " (nil-if-read)" if nil_if_read?
       io << " (closured)" if closured?
-      io << " (assigned-to)" if assigned_to?
+      io << " (assign-to)" if assigned_to?
+      io << " (assign-to-multiple-times)" if assigned_to_multiple_times?
       io << " (object id: #{object_id})"
     end
 
diff --git a/src/compiler/crystal/semantic/bindings.cr b/src/compiler/crystal/semantic/bindings.cr
index 946fa26bda9f..ef38117f40bd 100644
--- a/src/compiler/crystal/semantic/bindings.cr
+++ b/src/compiler/crystal/semantic/bindings.cr
@@ -168,6 +168,10 @@ module Crystal
       @dependencies.try &.any? &.same?(node)
     end
 
+    def bind_to_unless_bound(node : ASTNode)
+      bind_to(node) unless bound_to?(node)
+    end
+
     def add_observer(observer)
       observers = @observers ||= [] of ASTNode
       observers.push observer
diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr
index e4407f55d4d6..1f510c5350e6 100644
--- a/src/compiler/crystal/semantic/main_visitor.cr
+++ b/src/compiler/crystal/semantic/main_visitor.cr
@@ -137,6 +137,7 @@ module Crystal
         vars.each do |name, var|
           meta_var = new_meta_var(name)
           meta_var.bind_to(var)
+          meta_var.increase_assigned_count
           meta_vars[name] = meta_var
         end
       end
@@ -350,6 +351,7 @@ module Crystal
 
         meta_var = @meta_vars[node.name]
         check_closured meta_var
+        check_closure_multibound meta_var, var
 
         if var.nil_if_read?
           # Once we know a variable is nil if read we mark it as nilable
@@ -360,14 +362,6 @@ module Crystal
           node.bind_to(@program.nil_var)
         end
 
-        if meta_var.closured?
-          unless var.bound_to?(meta_var)
-            var.bind_to(meta_var)
-          end
-        else
-          meta_var.local_vars << var
-        end
-
         node.bind_to(var)
 
         if needs_type_filters?
@@ -800,9 +794,6 @@ module Crystal
         target.raise ex.message
       end
 
-      meta_var.assigned_to = true
-      check_closured meta_var
-
       simple_var = MetaVar.new(var_name)
 
       # When we assign to a local variable with a fixed type, and it's
@@ -812,13 +803,9 @@ module Crystal
       else
         simple_var.bind_to(target)
 
-        if meta_var.closured?
-          unless simple_var.bound_to? meta_var
-            simple_var.bind_to(meta_var)
-          end
-        else
-          meta_var.local_vars << simple_var
-        end
+        meta_var.increase_assigned_count
+        check_closured meta_var
+        check_closure_multibound(meta_var, simple_var)
       end
 
       @vars[var_name] = simple_var
@@ -886,7 +873,7 @@ module Crystal
         unless scope.has_instance_var_initializer?(var_name)
           meta_var = (@meta_vars[var_name] ||= new_meta_var(var_name))
           meta_var.bind_to value
-          meta_var.assigned_to = true
+          meta_var.increase_assigned_count
 
           simple_var = MetaVar.new(var_name)
           simple_var.bind_to(target)
@@ -2552,7 +2539,7 @@ module Crystal
       case exp = node.exp
       when Var
         meta_var = @meta_vars[exp.name]
-        meta_var.assigned_to = true
+        meta_var.increase_assigned_count
         meta_var
       when InstanceVar
         lookup_instance_var exp
@@ -2708,8 +2695,9 @@ module Crystal
         var = @vars[node_name] = new_meta_var(node_name)
         meta_var = (@meta_vars[node_name] ||= new_meta_var(node_name))
         check_closured(meta_var)
+        check_closure_multibound(meta_var, var)
         meta_var.bind_to(var)
-        meta_var.assigned_to = true
+        meta_var.increase_assigned_count
 
         if types
           unified_type = @program.type_merge(types).not_nil!
@@ -3178,6 +3166,10 @@ module Crystal
     end
 
     def check_closured(var)
+      unless @while_stack.empty?
+        var.inside_loop = true
+      end
+
       return if @typeof_nest > 0
 
       if var.name == "self"
@@ -3187,7 +3179,12 @@ module Crystal
 
       context = current_context
       var_context = var.context
-      if !var_context.same?(context)
+      if var_context.same?(context)
+        var_context = var_context.context if var_context.is_a?(Block)
+        if var.closured?
+          mark_as_closured(var, var_context)
+        end
+      else
         # If the contexts are not the same, it might be that we are in a block
         # inside a method, or a block inside another block. We don't want
         # those cases to closure a variable. So if any context is a block
@@ -3198,31 +3195,32 @@ module Crystal
 
         closured = !context.same?(var_context)
         if closured
-          var.closured = true
-
-          # Go up and mark proc literal defs as closured until we get
-          # to the context where the variable is defined
-          visitor = self
-          while visitor
-            # Grab the meta var in the visitor's scope and bind all
-            # its local vars to it. This is a fix for #5609.
-            meta_var = visitor.meta_vars[var.name]?
-            if meta_var
-              meta_var.local_vars?.try &.each do |local_var|
-                unless local_var.bound_to? local_var
-                  local_var.bind_to meta_var
-                end
-              end
-            end
+          mark_as_closured(var, var_context)
+        end
+      end
+    end
 
-            visitor_context = visitor.closure_context
-            break if visitor_context == var_context
+    def mark_as_closured(var, var_context)
+      var.closured = true
 
-            visitor_context.closure = true if visitor_context.is_a?(Def)
-            visitor = visitor.parent
-          end
+      if var.closured_multibound?
+        # Bind all local vars related to the metavar and bind them
+        # to the metavar. This is a fix for #5609.
+        var.local_vars?.try &.each do |local_var|
+          local_var.bind_to_unless_bound var
         end
       end
+
+      # Go up and mark proc literal defs as closured until we get
+      # to the context where the variable is defined
+      visitor = self
+      while visitor
+        visitor_context = visitor.closure_context
+        break if visitor_context == var_context
+
+        visitor_context.closure = true if visitor_context.is_a?(Def)
+        visitor = visitor.parent
+      end
     end
 
     def check_self_closured
@@ -3354,8 +3352,8 @@ module Crystal
       meta_var = (@meta_vars[name] ||= new_meta_var(name))
       meta_var.bind_to value
       meta_var.bind_to program.nil_var unless meta_var.dependencies.any? &.same?(program.nil_var)
-      meta_var.assigned_to = true
-      check_closured meta_var
+      meta_var.increase_assigned_count
+      check_closured(meta_var)
 
       @vars[name] = meta_var
       meta_var
@@ -3431,6 +3429,17 @@ module Crystal
       nil_exp
     end
 
+    # If the metavar is a closure multibound then bind var
+    # to it. Otherwise add it to the local vars so that they could
+    # be bound later on.
+    def check_closure_multibound(meta_var, var)
+      if meta_var.closured_multibound?
+        var.bind_to_unless_bound(meta_var)
+      else
+        meta_var.local_vars << var
+      end
+    end
+
     def visit(node : When | Unless | Until | MacroLiteral | OpAssign)
       raise "BUG: #{node.class_desc} node '#{node}' (#{node.location}) should have been eliminated in normalize"
     end

From 7bbf9e653ec7a8439a447dd01b743287fd839bf0 Mon Sep 17 00:00:00 2001
From: Ary Borenszweig <asterite@gmail.com>
Date: Mon, 16 Dec 2019 14:01:13 -0300
Subject: [PATCH 5/5] Revert std changes

---
 spec/std/socket/socket_spec.cr | 37 ++++++++++++++--------------------
 src/process.cr                 |  2 +-
 2 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/spec/std/socket/socket_spec.cr b/spec/std/socket/socket_spec.cr
index 2b6be2666a5e..9b9ecb146c60 100644
--- a/spec/std/socket/socket_spec.cr
+++ b/spec/std/socket/socket_spec.cr
@@ -45,30 +45,23 @@ describe Socket do
   it "sends messages" do
     port = unused_local_port
     server = Socket.tcp(Socket::Family::INET6)
-    begin
-      server.bind("::1", port)
-      server.listen
-      address = Socket::IPAddress.new("::1", port)
-      spawn do
-        client = server.accept
-        begin
-          client.gets.should eq "foo"
-          client.puts "bar"
-        ensure
-          client.close
-        end
-      end
-      socket = Socket.tcp(Socket::Family::INET6)
-      begin
-        socket.connect(address)
-        socket.puts "foo"
-        socket.gets.should eq "bar"
-      ensure
-        socket.close
-      end
+    server.bind("::1", port)
+    server.listen
+    address = Socket::IPAddress.new("::1", port)
+    spawn do
+      client = server.not_nil!.accept
+      client.gets.should eq "foo"
+      client.puts "bar"
     ensure
-      server.close
+      client.try &.close
     end
+    socket = Socket.tcp(Socket::Family::INET6)
+    socket.connect(address)
+    socket.puts "foo"
+    socket.gets.should eq "bar"
+  ensure
+    socket.try &.close
+    server.try &.close
   end
 
   describe "#bind" do
diff --git a/src/process.cr b/src/process.cr
index 735d42b92051..566e1fae5755 100644
--- a/src/process.cr
+++ b/src/process.cr
@@ -235,7 +235,7 @@ class Process
   private def stdio_to_fd(stdio : Stdio, for dst_io : IO::FileDescriptor) : IO::FileDescriptor
     case stdio
     when IO::FileDescriptor
-      stdio.as(IO::FileDescriptor)
+      stdio
     when IO
       if dst_io == STDIN
         fork_io, process_io = IO.pipe(read_blocking: true)