Fixed #3706: rescue in initialize can lead to uninitialized instance …

…variables
crystal-lang · Dec 15, 2016 · 02d4db3 · luislavena · Dec 15, 2016 · asterite
1 parent 366aec3
commit 02d4db3
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 47 deletions.
diff --git a/spec/compiler/semantic/exception_spec.cr b/spec/compiler/semantic/exception_spec.cr
@@ -355,52 +355,6 @@ describe "Semantic: exception" do
       )) { int32 }
   end
 
-  it "doesn't type instance variable if initialized inside begin and rescue raises" do
-    assert_type(%(
-      require "prelude"
-
-      class Foo
-        def initialize
-          begin
-            @bar = 1
-          rescue
-            raise "OH NO"
-          end
-        end
-
-        def bar
-          @bar
-        end
-      end
-
-      foo = Foo.new
-      foo.bar
-      )) { int32 }
-  end
-
-  it "doesn't type instance variable if initialized inside begin and in rescue" do
-    assert_type(%(
-      require "prelude"
-
-      class Foo
-        def initialize
-          begin
-            @bar = 1
-          rescue
-            @bar = 2
-          end
-        end
-
-        def bar
-          @bar
-        end
-      end
-
-      foo = Foo.new
-      foo.bar
-      )) { int32 }
-  end
-
   it "correctly types #1988" do
     assert_type(%(
       begin
@@ -445,4 +399,34 @@ describe "Semantic: exception" do
       var
       )) { union_of(int32, string) }
   end
+
+  it "marks instance variable as nilable if assigned inside rescue inside initialize" do
+    assert_error %(
+      require "prelude"
+
+      def some_method
+        if rand < 0.999999999
+          raise "OH NO"
+        else
+          2
+        end
+      end
+
+      class Coco < Exception
+        def initialize(@x : Foo)
+        end
+      end
+
+      class Foo
+        def initialize
+          @x = 1
+        rescue
+          raise Coco.new(self)
+        end
+      end
+
+      Foo.new
+      ),
+      "instance variable '@x' of Foo must be Int32, not Nil"
+  end
 end
diff --git a/src/compiler/crystal/semantic/exception.cr b/src/compiler/crystal/semantic/exception.cr
@@ -232,6 +232,8 @@ module Crystal
         io << colorize("instance variable '#{nil_reason.name}' was used before it was initialized in one of the 'initialize' methods, rendering it nilable").bold
       when :used_self_before_initialized
         io << colorize("'self' was used before initializing instance variable '#{nil_reason.name}', rendering it nilable").bold
+      when :initialized_in_rescue
+        io << colorize("instance variable '#{nil_reason.name}' is initialized inside a begin-rescue, so it can potentially be left uninitialized if an exception is raised and rescued").bold
       end
     end
 

diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr
@@ -2490,6 +2490,15 @@ module Crystal
         # was raised before assigning any of the vars.
         exception_handler_vars.each do |name, var|
           unless before_body_vars[name]?
+            # Instance variables inside the body must be marked as nil
+            if name.starts_with?('@')
+              ivar = scope.lookup_instance_var(name)
+              unless ivar.type.includes_type?(@program.nil_var)
+                ivar.nil_reason = NilReason.new(name, :initialized_in_rescue)
+                ivar.bind_to @program.nil_var
+              end
+            end
+
             var.nil_if_read = true
           end
         end
@@ -2549,7 +2558,7 @@ module Crystal
         end
 
         # However, those previous variables can't be nil afterwards:
-        # if an exception was raised then we won't running the code
+        # if an exception was raised then we won't be running the code
         # after the ensure clause, so variables don't matter. But if
         # an exception was not raised then all variables were declared
         # successfully.