Crystal: Instance variable initialized inside a begin-rescue

[straight-shoota]

class Foo
 @foo : String

  def initialize
    begin
      @foo = ""
    rescue exc
      raise exc
    end
  end
end

Foo.new

This code results in the following (full) compiler error:

Error in line 13: instantiating 'Foo:Class#new()'

instance variable '@foo' of Foo must be String, not Nil

Error: instance variable '@foo' is initialized inside a begin-rescue, so it can potentially be left uninitialized if an exception is raised and rescued

The message instance variable '@foo' of Foo must be String, not Nil is partly confusing because it is never set to nil.

I think this should compile. The instance variable actually cannot be left uninitialized because the rescue block raises again.

Related to #4149

[ysbaddaden]

No. raise could be a method, and have nothing to do with the global raise. The compiler doesn't know about that when typing instance variables. This would require a full analysis of methods which isn't possible at this stage.

Compiler ain't that smart.

[straight-shoota]

@ysbaddaden Change the constructor body to use a intermediary local var and it compiles:

begin
  foo = "foo"
rescue exc
  raise exc
end
@foo = foo

If raise was a regular method and wouldn't raise an exception, then this shouldn't work either because foo could be nil. It seems the compiler is smart enough to figure this out. Isn't this what NoReturn type is for?

[bew]

@ysbaddaden can't the compiler detect the NoReturn instead ?
As raise return type is NoReturn the compiler can know that it can't go further the raise call.

[asterite]

The compiler could do this. But eventually (I think) the compiler will be dumbed-down a lot so nobody has to guess how it should work, or trace stuff in their heads to understand what a program does.

[RX14]

@ysbaddaden This isn't related to type inference of the ivar (that's already explicitly specified), this is about type checking.

[straight-shoota]

@asterite An equivalent example without instance variables is this:

begin
  foo = "foo"
rescue exc
  raise exc
end
pp typeof(foo) # => String

The compiler knows that foo can't be nil after the block, despite the rescue because the rescue branch is NoReturn.

This should equally apply to instance variables. As far as I can see this is just a bug in the ivar initialization check that registers the begin-rescue but does not take into account that it actually won't return from the rescue branch.

[asterite]

This is tricky to fix because of https://github.com/crystal-lang/crystal/issues/3706

Basically, the rescue can use self and other instance vars and not realize they were not initialized.

[straight-shoota]

@asterite In my example, no ivars or self are used in the rescue block. If so, the same rules should apply as for local vars.

This diff should fix the immediate issue:

--- a/src/compiler/crystal/semantic/main_visitor.cr
+++ b/src/compiler/crystal/semantic/main_visitor.cr
@@ -2656,20 +2656,8 @@ module Crystal
       @exception_handler_vars = nil

       if node.rescues || node.else
-        # Any variable introduced in the begin block is possibly nil
-        # in the rescue blocks because we can't know if an exception
-        # 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
@@ -2702,6 +2690,22 @@ module Crystal
           # Otherwise, merge all types that resulted from all rescue/else blocks
           merge_rescue_vars exception_handler_vars, all_rescue_vars

+          # Any variable introduced in the begin block is possibly nil
+          # in the rescue blocks because we can't know if an exception
+          # 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
+            end
+          end
+
           # And then accept the ensure part
           with_block_kind :ensure do
             node.ensure.try &.accept self

But it doesn't account for an the case when an ivar or self are used in the rescue block:

begin
  @foo = "foo"
rescue exc
  raise @foo
end

I'm pretty sure this can be tracked, I just don't know how to start :)

[asterite]

Right, that last snippet is the problematic one, and I don't know where to start either (the code is too complex and any change will surely break something else). But this issue can be side-tracked, so it's not that important to fix right now.

[asterite]

I think, in the first snippet in this issue, if you assign it to a local variable, and then after the exception handler you assign that to the instance variable, it works, which makes this issue even less of a problem.