Rescued exceptions from class variable assignment swallow output

[jwoertink]

Running this program will compile fine, but it won't raise or puts anything in the rescue block.

class Test
  begin
  @@x : Int32 | Nil = raise “err”
  rescue
    raise "This never gets called"
    puts "neither does this"
  end
end

I expected this to raise the "This never gets called" part, or at the very least, run the puts. I didn't find any other similar issues, but I had trouble coming up with a title lol 😅

Crystal 0.35.0 (2020-06-09)

LLVM: 10.0.0
Default target: x86_64-apple-macosx

[jhass]

We should find a way to disallow such code. The "right" way to write this is

@@x : Int32 | Nil =  begin
    raise “err”
  rescue
    raise "This never gets called"
    puts "neither does this"
  end

[waj]

Seems like this is not directly related with exceptions. For example:

class Test
  if true
    @@x : Int32 | Nil = 1
  end

  def self.x
    @@x
  end
end

pp Test.x # => nil

And even with instance variables:

class Test
  if true
    @x : Int32 | Nil = 1
  end
end

pp Test.new # => #<Test:0x103788fe0 @x=nil>

[asterite]

My thoughts about this: we should completely disallow top-level code that's outside of clases. It's something we copied from Ruby but doesn't translate well to Crystal.

[asterite]

This:

class Test
  begin
  @@x : Int32 | Nil = raise “err”
  rescue
    raise "This never gets called"
    puts "neither does this"
  end
end

Is equivalent to this (as seen by the compiler):

class Test
  @@x : Int32 | Nil = raise “err”


  begin
  rescue
    raise "This never gets called"
    puts "neither does this"
  end
end

That is, instance/class variable declaration has a different scope/visibility than other top-level code.

Then this:

class Test
  if true
    @x : Int32 | Nil = 1
  end
end

Is seen by the compiler like this:

class Test
    @x : Int32 | Nil = 1

  if true
  end
end

That is, those declarations/initializations are not considered top-level code, they are considered attached to the class in a separate place.

[waj]

If that were the case @x would be initialized to 1, but instead is left as nil. A simple solution might be disable instance or class variable declaration that is not directly under the class?

[waj]

Currently the compiler raises the error "can't use instance variables at the top level" for this code:

class Test
  if true
    @x = 1
  end
end

I think it should raise the same error.

[jwoertink]

I would be ok with getting a compile-time error telling me to not use the code in this way. This just came from this example:

@@x : Int32 | Nil = some_method_that_may_or_may_not_raise

def self.some_method_that_may_or_may_not_raise
  nil || raise "oops"
end

It's all generated from in a macro, so I was trying to figure out a way to wrap that so if the user's method raised an exception, I could catch that and do something else. I'll roll with what @jhass suggested. That's an easy fix for my end. Thanks for the help!

[paulcsmith]

How does the code @jwoertink look like when expanded? To me that looks like it should work as is because it isn’t wrapped in a begin and the method seems to be called like a normal method. I get why the other ones should be disallowed, but this last one seems different

Maybe I’m just not understanding though :P

[asterite]

I think there are several bugs here so that's why it's hard to explain what's going on. Theres #8862, it also seems that when you wrap a class variable initialization in begin/end then it's not executed right away.

My advice: don't do this :-) . In the future I'd like to remove top-level code from classes if possible. I think it's the cleanest approach. I also see code in the wild that looks like this:

module MyModule
  OptionParser.new ...
  puts "Here's my main code!"
end

I think that's not nice or easy to understand, especially when trying to mix class variables or when some of those calls can actually be macros that define instance variables or methods. It's just not how other statically typed and compiled languages work, and we should avoid copying Ruby here.

(of course it depends on what others want about this "feature", but I consider this an anti-feature...)

[paulcsmith]

@asterite Yeah I agree with that example as well that it is a bit confusing and would be better wrapped in a method that is called from elsewhere to start things up. I'm not sure how to redo this example Jeremy posted:

@@x : Int32 | Nil = some_method_that_may_or_may_not_raise

def self.some_method_that_may_or_may_not_raise
  nil || raise "oops"
end

How would you set up a class variable with a default value that is generated from a method? I'm probably just being dense here but I don't get what the alternative is 😂

[asterite]

How would you set up a class variable with a default value that is generated from a method?

Require a class method in the subclass and use that?

[paulcsmith]

@asterite I'm just being dumb today and still don't get it. I made a play snippet with what I understood and it raises the same error as using the parent class: https://play.crystal-lang.org/#/r/9bkc

EDIT: Never mind. Looks like Jeremy fixed it already: https://github.com/luckyframework/habitat/pull/51/files#diff-a82a4bbf3e7cdad8547a42db8e22426aR195-R201

@@{{ decl.var }} : {{decl.type}} | Nil {% if has_default %} = begin
            {{ decl.value }}
          rescue
            # This will cause a MissingSettingError to be raised
            nil
          end

[jwoertink]

Yup. @paulcsmith I had got the idea from @jhass above. Doing @@x = begin... should be fine, but doing begin @@x... should throw an error.