Crystal: Better guidelines to Exception raising from STD libs

[bararchy]

I got this error:

Unexpected end of http response Unexpected end of http response (Exception)
0x557dd7c46885: exec at /usr/lib/crystal/http/client.cr 491:24
0x557dd7c8ecfe: ??? at /usr/lib/crystal/http/client.cr 585:5
0x557dd7bbf1d7: ??? at /usr/lib/crystal/fiber.cr 255:3
0x0: ??? at ??

Which originates from here: https://github.com/crystal-lang/crystal/blob/master/src/http/client.cr#L497

This is troublesome because I have no way of specifically capture this exception unless I rescue Exception, I propose that each module (at least) will have it's own Exception class, so that a raise like this should be

raise HTTP::Exception.new(message)

So it can be handled correctly.

[bew]

I think that raise "a string" is too permissive, it raises an Exception. And when you try to catch it, you can't specify what to rescue, you need to rescue virtually ALL errors.

Maybe the raise(String) method should be removed from the stdlib, so that you need to specify the Exception class to raise.

If we go a bit further, the Exception class could be made abstract, so that you need to create & specify your own exception class.

[bararchy]

@bew Even though I tend to agree with you on this point, I feel it's to harsh to close raise "string"
I use it quite alot in small scripts that should not be used by other shards, or where I know what is going on.

When making Shards andor working on STD libs, I think it should be consider a non-approval for merge unless you handle your own Exceptions, so that not all of Crystal core will raise generic "Exception" class.

I think removing Exception totally is also too harsh, but maybe somehow discourage it ? (compiler warning ? )

[RX14]

We could fail CI if grep 'raise +"' -R src.

[bararchy]

@RX14 That's a great idea

[bew]

RX14 I think this has a few edge cases hard to detect:

• custom raise methods like:

class MyClass
  class Error < Exception
  end

  def raise(msg) # custom raise
    ::raise Error.new msg
  end

  def foo
    raise "message for my custom raise" # <-- false positive
  end
end

• custom raise with receiver, this used a lot in the compiler, e.g: https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/semantic/top_level_visitor.cr#L61

The latter is "easy" to detect, but the first one is not (even if that a bad idea to do that)

[bararchy]

So, should we move with @RX14 suggestion to fail CI on raise "string" ?

[straight-shoota]

While I generally support using explicit error types, I don't think this simple grep is a good idea. It would result in a lot of false positives because of local method overwrites. You'd need to look at the AST for that.

ameba could probably do that, so maybe it would be an idea to explore if it could be applied to the stlib/compiler repo.

Apart from that, it should be relatively easy to start manually enhancing blank raise calls with a specific error type, even without an automatic check. Although it would probably be worth discussing changes to the exception hierarchy first (continuation of #5559, #5003, need a RFC for that).

[bararchy]

Well, It seems to be a "too big" a topic to actually discuss in this single issue, maybe smaller PRs with specific raise code changes will be better.