Change Errno into a hierarchy of OSError subclasses

[oprypin]

Initial exception hierarchy is:

• OSError (former Errno)
  • OSError::BlockingIO {EAGAIN, EALREADY, EINPROGRESS, EWOULDBLOCK}
  • OSError::FileExists {EEXIST}
  • OSError::FileNotFound {ENOENT}
  • OSError::IsADirectory {EISDIR}
  • OSError::NotADirectory {ENOTDIR}
  • OSError::PermissionError {EACCES, EPERM}
  • ConnectionError
    • BrokenPipe {EPIPE}
    • ConnectionAborted {ECONNABORTED}
    • ConnectionRefused {ECONNREFUSED}
    • ConnectionReset {ECONNRESET}

These are based on what Python does.

The best showcase of improvement is src/ecr/process.cr.

Note that I did not get rid of the inherent dependency on errno/GetLastError, because it's just not viable to predict every possible error from those countless error codes, every time an OS API is called. OSError.create substitutes the exception class with some subclass of OSError, when such a class is defined, or otherwise the generic OSError (just like Errno before) with the error code available. This is also what Python does.

Windows errors are integrated into this; see windows_error.cr.

[ysbaddaden]

This is the kind of change that means to abstract system details and hide them to the developer, but those details are very hard to hide, and in the case of Errno or WinError: impossible to.

For example moving Errno to Crystal::System means that developers musn't use Errno... but developers may need to access Errno in perfectly valid scenarios, for example when writing C bindings that for libraries that will set errno.

Same for WinError, which by-the-way should use the windows naming (WinError) not something else (WindowsError).

If we're abstracting the platform error systems, then mapping WinError -> Errno should be replaced with a mean to raise crystal exceptions.

Last but not least I really don't like the chosen python inspired naming. I'd really suggest to have per namespace exceptions, for example System::Error instead of OSError, or File::Not found, IO::BrokenPipe, ... that is, names that fit better in Crystal land.

[ysbaddaden]

What?! Using Crystal::System::Errno makes no sense: it's not a platform abstraction :/

Using a ... error message is also, huh...

[ysbaddaden]

Furthermore pushing Errno as Crystal::System::Errno means that developers of C bindings will now need to use Crystal::System... but developers musn't.

[oprypin]

Well with the namespace I guess I see what you mean. But Crystal cannot provide access to errno in a cross-platform way because it's a UNIX-specific thing, and at the same time, Crystal::System::Errno is, in fact, a platform abstraction (different POSIX platforms). What do you suggest?
By the way, developers technically don't have to access Crystal::System::Errno, they can just use OSError.create.errno, but they usually shouldn't need to.

[oprypin]

Reworked this. But now accessing errno is a compilation error on non-POSIX.
at least I think so...

[asterite]

I agree with the spirit of this change. Every other high-level programming language (except Ruby) abstracts Errno with meaningful errors (for example Java, C#, Python andGo). Being able to access the value of errno is also good because of the reasons explained by @oprypin . I also remember @waj wanted to get rid of the Errno exception type. So 👍 from me (after reviewing and making the necessary corrections).

[oprypin]

@ysbaddaden, I renamed the exceptions slightly.

by-the-way should use the windows naming (WinError)

Strongly disagree that naming in Windows headers should in any way affect naming in Crystal.

[molovo]

This is a great change, should make life a lot easier. Nice work @oprypin

I do like @ysbaddaden's idea of namespacing the errors, but I don't think they belong directly in the namespace. Perhaps a good convention would be to have an Error namespace which they could live in so that it's immediately obvious from the name what it is. More like File::Error::NotFound, IO::Error::BrokenPipe etc.

[RX14]

I agree with @ysbaddaden that OSError's subclasses should live in the namespaces which raise these errors, instead of the OSError namespace. The best way to design this is to forget about the OS and think about "if my OS and stdlib was written in crystal, how would I design this", then think about how best to map this to crystal and make compromises if needed. People using these exceptions should very rarely need to think that these errors are "os errors", they're just a normal error which is raised from File for example, so it should live in the File namespace.

[oprypin]

@RX14, that last sentence is definitely disjointed from reality. OSError is special in the way that any system call can produce any of them. Scattering them across the standard library will not help anyone, and is a huge source of bikeshedding.

[oprypin]

Please present a concrete agreed upon set of names, and I can accept it
(please no System::Error though)

[RX14]

@oprypin Except not all syscalls produce all errnos? If the same errno has a different enough semantic meaning in two different syscalls it should become 2 different crystal error types. I am in favour of complete abstraction. If people want to think in terms of errnos, they can simply catch OSError and check the errno on that. 99% of people do not want to know or care about syscalls, and so we should design the exception hierarchy entirely without them in mind. I'm sure we'll need to make some compromises, but I want to stick as close to that ideal as possible.

[oprypin]

@RX14, as I've said before, if we are relying on syscalls at all, it's an extreme amount of work trying to predict all possible error codes for every case. There would still probably need to be a fallback error anyway.
This change is simple and non-disruptive. It does not make full abstraction any harder to achieve than before, but is a big improvement by itself. Beggars can't be choosers. I consider this comment not constructive.

[oprypin]

In the next 2 days I will not be able to edit the code of this pull request, but I welcome comments that have a concrete idea of implementation, not just pointing out a problem.

It would be great to get an agreement on the naming of exceptions, so maybe someone can suggest a complete set of names and people can vote on that.

Also tagging @txe -- maybe you have some input.

[oprypin]

If the same errno has a different enough semantic meaning in two different syscalls it should become 2 different crystal error types.

This PR provides a sane default for every possible case without any work needed. If some undesired behavior is detected, it is easy to add a check on a case-by-case basis and manually create a different exception type (and even keep the original errno in it which is nice).

[RX14]

I agree - this is a massive improvement - but i'm always wary of merging imperfect improvements because I'm scared they'll become "good enough" to tag 1.0 and then we'll never get a chance to improve them further. I think that's unreasonable of me. I'll add improving this to the roadmap so we don't forget it before 1.0.

[txe]

I suppose there is a misunderstanding of what @RX14 is trying to say. I'll try to say the same in other words.
There is an idea of unifying of using exceptions across aboard. For example, class Base64 can raise exceptions BadEncoding, BadDecoding and they are not related to errno. Suppose, there are many classes with their exception then It will be good idea to keep these exceptions in the namespace of that classes because only these classes will raise their exceptions.
So, this idea can by applied to class File (in spite of the fact the exceptions have the errno attriute) otherwise a user will feel there is something special in case of File although there is no such thing.
Also, it doesn't mean there is a need to cover all errno.

[konovod]

I think is that syscallls should raise something like OsError, but stdlib methods should try to intercept them and reraise as a File::ENotFound etc - because methods in stdlib will have enough context to identify what exactly this EACCES mean. I'm not expert though, maybe this intermediate level is overkill.

[ysbaddaden]

Please note that I previously proposed a solution to have per-errno exception classes, which was rejected partly because it noticeably impacted the compiler performance, despite however helpful and nicer it was to raise and catch specific errno.

Compiler performance should be verified here too.

@oprypin bloating the global or OSError namespace with lots of exception classes is a bad design. If we don't care about Errno and want to introduce cross platform intelligible namings it MUST be done in a logical API design.

As RX14 suggests, how would we design and name the exception if we didn't make a syscall? Would we introduce a ::FileNotFoundError? Or would we introduce a File::NotFound?

Otherwise, it's just hiding some well-known naming (Errno, WinError) for another naming (OSError), it's introducing a bad API design, ...

That being said, we can't catch and verify each possible errno value after each syscall, this is tedious and will bloat the stdlib, but we can check the most important one(s) for each syscall, even maybe have a generic exception raiser for a namespace, and default to a generic exception for other values.

Last but not least, I wouldn't hide Errno.value (it's cross platform).

[oprypin]

how would we design and name the exception if we didn't make a syscall?

First of all, this theoretically does not happen, but if it does, you reuse the appropriate OSError subclass.

MUST be done in a logical API design.

So what's not logical here? Python devs spent a lot of time designing this hierarchy; it's not a historic leftover.

Last but not least, I wouldn't hide Errno.value (it's cross platform).

It's not really cross-platform. Also it's not hidden by this PR.
Also note that alternative suggestions do, in fact, hide it.

we can check the most important one(s) for each syscall, even maybe have a generic exception raiser for a namespace, and default to a generic exception for other values.

That's what is happening but the namespace is OSError. Can't be anything else because then errno is gone.

is a bad design

introducing a bad API design

Might as well move on to ad hominem. Not sure if those positive votes reflect that though

[straight-shoota]

I hope the last weeks have given some time to look at this discussion with some distance.

@oprypin I don't think @ysbaddaden is in any way aggressive against you. If your proposal was (in part) criticized as "bad design" it was done so with reason. Still, others can have different opinions and I think we should talk about arguments for both.

As I understand it, both approaches have good points and at the current time I couldn't make out a conclusive argument about preferring one over the other.

I think it's worth taking a look at the reasoning behind Pythons exception hierarchy. But it is worth noting that this proposal was based on an existing hierarchy with flaws. They couldn't design the best API they could think of, but had to avoid any radical changes which would break compatibility considerably.

Some general thoughts on naming exceptions:

• Exception classes should have Exception (or Error, see next) in the name to make them easily identifyable. For example OptionParser::InvalidOption could be understood as holding an option with invalid value, IO::Timeout could be a time span. OptionParser::InvalidOptionError and IO::TimeoutError are not much more to write but improve understanding.
• Some classes are named Error and some Exception (e.g. OptionParser::Exception, Key::Error). While it might make sense to use this to differentiate between severities (see for example Differences between Exception and Error (Java)), the naming of Crystal's exception classes doesn't seem to be conclusive at all.
• It can be misleading to have many exception classes named Error in modules (like IO::Error). If, from inside the module, the short path without the module prefix is used to reference this exception class, it becomes ambiguous. You can't simply grep over the code to determine where a certain exception is raised. Everything is fine if for classes named Error the full name is used everywhere.

---

Looking at the current and proposed hierarchies I wonder about how to align things together:

There is IO::Error which - as I understand it - should represent all errors raised from the IO module / raised from IO operations. It has a subclass IO::EOFError - a specific type of error. However, there is also IO::Timeout which is an exception in IO and obviously IO-related, but not a
subtype of IO::Error. Then there is also Socket::Error. While Socket is not part of IO, it would still be reasonable to assume that an error in a socket is an IO error and that rescue IO::Error doesn't ignore malfunctioning sockets.

Many of the error types introduced by this PR classify as IO errors - independent of how they are named. If it is OSError::BlockingIO or IO::BlockingError, it would make sense to rescue it as IO::Error. Yet obviously, all classes for specific OS errors derived from errno/winerror should inherit from one base class OSError (or whatever the name). But they can't simultaneously inherit IO:Error and OSError. And
IO::Error can't inherit from OS::Error because subclasses like IO::EOFError are not OS errors.

Has anyone thought about this? Or am I in some way mistaken?

[oprypin]

A random idea passing by: OSError should be a module

Also: isn't reading the global "last OS error" variable horribly breaking thread safety?

[RX14]

I agree, the current exception hierarchy is badly cobbled together and has very clearly grown over time with little design.

OSError being a module is a good idea. I'd like to standardize on Error vs Exception. The base class is called Exception so perhaps naming all exceptions ending in Exception would be a good idea. On the other hand, perhaps not all exceptions are errors, so naming actual errors Error would be better. Off the top of my head, Error is winning and much more common.

[oprypin]

All actual errors should end with Error -- and that's really all of the exceptions that you normally think about.

Again I really like what Python does. The only non-Error exceptions are special control flow exceptions:

• SystemExit -- exit() is implemented by raising this.
• KeyboardInterrupt -- raised on Ctrl+C signal.
• GeneratorExit and StopIteration -- iterators are implemented by raising this when they run out of items.

But Python also prevents you from accidentally catching those because they derive from BaseException while errors derive from Exception.

[bararchy]

@RX14 @straight-shoota @ysbaddaden ping on this? Would love to get the errorno away

[oprypin]

I think this is basically rejected

[straight-shoota]

While this PR has received mixed feedback, I think everyone agrees on the principal idea to replace the generic Errno/WinError with context-specific error types. But we need to work out the exact design of the exception hierarchy and agree on naming.

This PR is already pretty much cluttered, outdated and probably won't be merged anyway. So, maybe we should take a step back and continue discussion in a new issue about exception design instead of this PR.

But I think we have made some promising progress and can continue this matter on a good basis.

#5566 should maybe be resolved first - or included in a general discussion about Crystal's exception hierarchy?

[oprypin]

woops