Improved exception hierarchy/types

[Blacksmoke16]

Discussion

• What aspect of the language would you like to see improved?

The types and hierarchy of exception classes.

• What are the reasons?

Currently Crystal exception types are essentially all direct children of Exception. This makes it a bit challenging to rescue "groups" of related exceptions in a more general fashion. Similarly there could be room to differentiate between something that could be gracefully handled versus something that could not.

In the end having these higher level more generic exception types gives shard creators more flexibility in describing how their custom exceptions should be handled. I.e. automatically handled with a more general type, or something on its own.

• Include practical examples to illustrate your points.

For example being able to rescue/check for a LogicError type that could use a different form of logging/messaging since it should directly result in a change in your code. This may also help with an implementation of Ruby's throw? :shrug:

• Optionally add one (or more) proposals to improve the current situation.

Other langs handle the latter point via having a Throwable (Raisable in our case), then Exception and Error subtypes. From here exception types are grouped into related categories. I don't have anything concrete as of now, but using other lang's hierarchy as a base and figuring out what makes the most sense to include would be a good start.

• Ruby: https://blog.appsignal.com/2018/04/10/rescuing-exceptions-in-ruby.html
• PHP: https://php.watch/articles/PHP-exception-hierarchy
• Java: https://rollbar.com/blog/java-exceptions-hierarchy-explained/
• Python: https://docs.python.org/3/library/exceptions.html#exception-hierarchy

[asterite]

Duplicate of https://github.com/crystal-lang/crystal/pull/8990?

[straight-shoota]

@asterite Not really a duplicate, but maybe a restart? Thanks for digging that up though. I was sure there was some more previous discussions about this, but couldn't find that one.

[Blacksmoke16]

I think there's deff a path forward without Raiseable that would be simpler, backward compatible, and still provide the majority of benefits, especially since there's not really a need for a higher level Exception type at the moment anyway.

For example, maybe something like this:

Exception
├── DivisionByZeroError
├── Base64::Error
├── Channel::ClosedError
├── Crystal::ELF::Error
├── Enumerable::EmptyError
├── InvalidByteSequenceError
├── Path::Error
├── Time::Error (maybe)
│   ├── Time::FloatingTimeConversionError
│   ├── Time::Format::Error
│   ├── Time::Location::InvalidLocationNameError
│   ├── Time::Location::InvalidTZDataError
│   └── Time::Location::InvalidTimezoneOffsetError
├── IO::Error
│   ├── File::Error
│   │   ├── File::AccessDeniedError
│   │   ├── File::AlreadyExistsError
│   │   ├── File::NotFoundError
│   │   └── File::BadPatternError (This currently inherits `Exception`?)
│   ├── IO::EOFError
│   └── IO::TimeoutError
├── RuntimeError
│   ├── OSError (new, deprecate `RuntimeError#os_error` in favor of this type)
│   ├── KeyError
│   ├── IndexError
│   └── OverflowError
└── LogicError (new)
    ├── ArgumentError
    ├── TypeCastError
    ├── NotImplementedError
    ├── NilAssertionError
    └── DomainError (maybe)

Wasn't really sure what to do with those random ones towards the top, so just kept them as direct children, which maybe means they should really be Error types? :shrug:. Also noticed File::BadPatternError doesn't inherit from File::Error?

Time::Error could make sense, but I could also see some of those being put under LogicError. I like the idea of having a dedicated LogicError type to catch things that should result in code changes. Based on Python, I thought it also made sense to have a dedicated OSError type to differentiate between a more general runtime error versus one from an underlying OS. However, could just keep current SystemError concept as that's a bit more flexible.

DomainError was something PHP had, not really sold if its needed but i included it anyway. Idea being it represents a value that does not fit in the "defined data domain", which is a bit more specific than LogicError.

EDIT: DomainError is probably just ArgumentError, so no need for that I think.

NOTE: I just threw this together, mainly for example/demonstration purposes

[straight-shoota]

Thanks for starting this. I think there is a lot room for improvements exceptions in Crystal. So this is a very extensive topic. I'm listing a few talking points that I feel should be addressed at some point. Some may not be very clear, those mainly serve as a memo for myself 🙈

• Organizational structure of stdlib exception hierarchy. In many places it feels like this has grown to the current state and needs some top-down management to create a reasonable tree.
  • Re-arrange structure, introduce new error types, maybe merge some together (either entirely or under a common hiearchy).
  • ArgumentError is a kind of catch-all solution but tells very little. I'd like to reduce its usage in general and/or redefine it to make more sense.
  • introduce MathError/ArithmeticError (https://github.com/crystal-lang/crystal/pull/11230#discussion_r762426524)
• Standardize exception naming and patterns for the Crystal ecosystem
  • #5566
  • Error class in namespaces vs. more descriptive names
  • Base class as a common parent for all errors in a namespace vs. possibility to individually inherit from different base errors
• Consider a top-level error type that's above the default type matched by an unrestricted rescue (#8990)
• Define the status of assertion errors such as NilAssertionError and TypeCastError - should they be considered critical or acceptable in application code (#8990)
• Docs: Strucutured documentation for a method's errors (#8353)
• Compiler: Tracing which exceptions can be raised from a piece of code (#7154, #8353)
• Consider alternatives for communicating error state that don't involve raising but offer more context than a nilable result (#3396)

[Blacksmoke16]

Has there been any discussion around allowing exceptions to be rescued based on an included module? This would make exceptions a bit more flexible by allowing users to more easily rescue groups of related exceptions, while still having them be reusable via a parent level Exception type.

E.g.

module MyApp::Exception; end

class MyApp::Exception::SomeError < ArgumentError
  include MyApp::Exception
end

begin
  # ...
rescue ex : MyApp::Exception # Rescue all exceptions within `MyApp`
end

begin 
  # ...
rescue ex : ArgumentError # Rescue all forms of `ArgumentError`
end

A workaround for this is to use an alias, but that ultimately gets reduced to Exception so won't really work.

https://play.crystal-lang.org/#/r/cj10

[straight-shoota]

I'm wondering about the purpose of such exception grouping. What would be the purpose for another level of hierarchy on top of class inheritance?

Your example shows a namespace/app specific exception grouping. Why would I want to rescue exceptions from a specific namespace if they're semantically unrelated? If they'd be semantically related, they would be in a common hierarchy tree.

[Blacksmoke16]

Why would I want to rescue exceptions from a specific namespace if they're semantically unrelated?

My thinking is they're logically related since they're defined in the same namespace of a singular project. One use case I can think of is within related libraries, say A and B. Library A wraps library B in that it provides extra functionality on top of a more basic implementation. Library B has a portion that has user defined code within it. Library A may want to rescue all exceptions from library B such that it can better handle/report the error (e.g. incorrect usages of A/B features) while allowing user code exceptions to bubble up.

Granted this is pretty abstract, but is there an actual reason for not supporting it? It actually seems Ruby allows this fwiw.

[HertzDevil]

For reference, the current hierarchy is like this: (generated from crystal tool hierarchy -E Exception src/docs_main.cr)

Exception
├── ArgumentError
├── Base64::Error
├── CSV::Error
|   └── CSV::MalformedCSVError
├── Channel::ClosedError
├── Compress::Deflate::Error
├── Compress::Gzip::Error
├── Compress::Zip::Error
├── Compress::Zlib::Error
├── Crypto::Bcrypt::Error
├── Crystal::ELF::Error
├── Crystal::Error
|   └── Crystal::CodeError
|       └── Crystal::SyntaxException
├── Digest::FinalizedError
├── DivisionByZeroError
├── Enumerable::EmptyError
├── Enumerable::NotFoundError
├── File::BadPatternError
├── HTTP::FormData::Error
├── HTTP::Server::ClientError
├── INI::ParseException
├── IO::Error
|   ├── File::Error
|   |   ├── File::AccessDeniedError
|   |   ├── File::AlreadyExistsError
|   |   └── File::NotFoundError
|   ├── IO::EOFError
|   ├── IO::TimeoutError
|   └── Socket::Error
|       ├── Socket::Addrinfo::Error
|       ├── Socket::BindError
|       └── Socket::ConnectError
├── IndexError
├── InvalidBigDecimalException
├── InvalidByteSequenceError
├── JSON::Error
|   └── JSON::ParseException
|       └── JSON::SerializableError
├── KeyError
├── MIME::Error
├── MIME::Multipart::Error
├── NilAssertionError
├── NotImplementedError
├── OAuth2::Error
├── OAuth::Error
├── OpenSSL::Error
|   ├── OpenSSL::Cipher::Error
|   ├── OpenSSL::Digest::Error
|   |   └── OpenSSL::Digest::UnsupportedError
|   └── OpenSSL::SSL::Error
├── OptionParser::Exception
|   ├── OptionParser::InvalidOption
|   └── OptionParser::MissingOption
├── OverflowError
├── Path::Error
├── RuntimeError
├── Spec::SpecError
|   ├── Spec::AssertionFailed
|   ├── Spec::ExamplePending
|   └── Spec::NestingSpecError
├── System::Group::NotFoundError
├── System::User::NotFoundError
├── Time::FloatingTimeConversionError
├── Time::Format::Error
├── Time::Location::InvalidLocationNameError
├── Time::Location::InvalidTZDataError
├── Time::Location::InvalidTimezoneOffsetError
├── TypeCastError
├── URI::Error
├── UUID::Error
├── XML::Error
└── YAML::Error
    └── YAML::ParseException

[m-o-e]

Somewhat related:

I think it would also be nice if stdlib could refrain from raising String (raise "...") where possible and be specific instead (raise FooError.new("...")).

I recently bumped into this when writing a pooled HTTP client where I have to handle one of those anonymous Exceptions.

My handler-code has to rely on the text message, which doesn't feel right:

rescue ex
  if ex.message == "This HTTP::Client cannot be reconnected"
    # ...
  end

I think it should rather be:

rescue ex : HTTP::Client::CanNotBeReconnectedError
  # ...
end

Currently there appear to be ~200 places where String is raised: grep -r --exclude 'src/compiler/*' "raise \"" src | grep -Ev '{{|%'

Imho that's fine in the compiler and inside macros but not for Exceptions that may need to be handled by the user/developer at runtime.

[HertzDevil]

I don't think raising a more specific exception type would break anything, but what about raising a less specific, or unrelated type? Let's say JSON::SerializableError is no longer a subclass of JSON::ParseException (#11858). How do we implement this at all in a transitionable manner?

[Blacksmoke16]

With https://github.com/crystal-lang/crystal/issues/14552 we have some extra options when it comes to setting up the hierarchy as we would no longer be limited to inheritance. E.g. we have the ability to treat some of these as interface mixins vs requiring them to be dedicated types. This would allow having "exception" types that can be recused but not directly raised themselves. Not exactly sure how this could/would fit into things, but is at least another possibility in our toolbox.

In the shorter term, thoughts on starting things off by:

• Introduce some new exception types: LogicError, Time::Error and ArithmeticError
• Move ArgumentError, ArithmeticError, TypeCastError, NotImplementedError, and NilAssertionError as children of LogicError
• Move DivisionByZeroError to be a child of ArithmeticError
  • OverflowError would ideally be there too, but that would be breaking since its currently a child of RuntimeError. Module exception type could work around this but :shrug:
• Move Time::* as children of Time::Error

[Blacksmoke16]

After dwelling on this for a while I'm now unsure if LogicError really adds anything. The idea of it is good in theory but is it really worth having in the stdlib vs just adding it as a custom exception type as needed? I been viewing it as something that could happen during development to indicate something that needs to be fixed. So wouldn't really expect it to happen in production. There's also the argument that given how somewhat general it is you could say it's basically the same as ::Exception :shrug:. Open to other's opinions on it, but at this point I don't really think it matters much.