Consolidate / clean up exception types used for OS exceptions

[CyberShadow]

https://issues.dlang.org/show_bug.cgi?id=13620

[JakobOvrum]

null and 0 instead of __FILE__ and __LINE__? Was this discussed recently?

[CyberShadow]

No idea what's going on here. Maybe this predates these having proper values as value argument defaults.

[JakobOvrum]

By the looks of it, ErrnoException, FileException, StdioException and even ProcessException were all essentially doing the same thing, so consolidating them into a hiearchy is a no-brainer (the ratio of -/+ in this patch is a testatement to that).

Further, the whole one-exception-type-per-module setup is bonkers anyway, so I'm also for just consolidating them into the same type which this patch also does, but this might be a point of contention. The one-exception-type-per-module setup has supporters, so now would probably be a good time to come out of the woodwork and argue its case. If this setup has any merit, then this patch is a fairly serious silent breaking change.

[jmdavis]

Exception types should be based on what makes sense to catch. So, for instance, if it makes sense to differentiate between exceptions that occurred specifically when operating on a file vs another kind of OS error, then it makes sense to have both FileException and OSException (though FileException should probably be derived from OSException in that case), but if it wouldn't make sense to handle file-related exceptions separately, then having a separate FileException doesn't really buy us anything.

Regardless, I don't think that having an exception type per module really makes sense, and I think that most of us have agreed on that in the past. It's just that we never necessarily agreed on what the exception hierarchy really should look like, and even where we did, no one took the time to implement it. But what exceptions you have is all about needing to catch and handle exceptions differently based on what they're for, not based on which module they're it. Sometimes, that falls along module boundaries, but it often doesn't. I'll probably have to look these over too see whether I agree that they should all be consolidated into a single exception type, but I agree that it at least makes sense to have a common base type for all of the OS-based exceptions. It's just a question of whether having exception types derived for that for specific situations makes sense or not - either based on needing to catch them separately or on there being additional information necessary for a particular exception (and therefore more member variables). But module-based exceptions really doesn't make much sense IMHO.

[CyberShadow]

I've tried to avoid breaking changes as much as was reasonable. Here's the remaining breaking changes:

1. Sometimes, some of the replaced module exception classes were used both to report OS errors, and to report non-OS exceptions (for example: wrong configuration parameters, the environment not satisfying prerequisites, etc.). In the latter case, the code (errno) field was unused.

   I've replaced some instances of the above with specifying a dummy code (e.g. if a path is expected to be a directory but is not, I throw an ErrnoException with ENOTDIR as the code). In other cases I've changed the exception type, which is a breaking change. I guess neither of these solutions are optimal, and which one is better depends on where you stand with regards to correctness vs. backwards-compatibility.

2. StdioException had some funky opCall overloads defined, which I got rid of. They should probably never have been made public, and it's unlikely that there exists user code that uses them, however these were public and even documented.

3. Although the old exception definitions have been replaced with aliases, the changes will affect code that depend on the exception classinfo names (toString etc.) Personally I think this is more "implementation detail" than "specification" land.

4. Non-sensical code such as try { read("unexisting"); } catch (ProcessException e) {} will now catch the exception, because ProcessException is now the same type as FileException.

[JakobOvrum]

DIP33 deserves a mention here. In general I like the suggestions in this DIP, and this PR basically implements the "SystemException, ErrnoException, WinAPIException" part of it. I don't have time to take a closer look at it right now, but a cursory look suggests that there are several issues consolidating it with this PR, notably that it suggests both a FilesystemException and ProcessException, with no suggestion on how they should interact with SystemException...

Of course, redesigning the Phobos exception hiearchy is probably a bit beyond the scope of this PR, so I guess we should focus on forwards-compatibility with any future redesign.

[CyberShadow]

I discovered a breaking change in my own code (yay for dogfooding):

User code might instantiate FileException etc. instances. This is now forbidden because OSException is abstract.

I guess I should change OSException to not be abstract, and add compatibility constructors.

There's still the question of what deprecation path (if any) should be used for the old exception aliases.

[jmdavis]

OSException probably shouldn't be abstract (and definitely shouldn't be abstract if you're aliasing it to an exception type which was concrete previous). However, thinking about this, I don't think that we should get rid of FileException. What I'd really like to see is for it to have a member variable which indicates what went wrong in a system-agnostic manner (e.g. an enum with members like fileNotFound or insufficientPermissions) so that it's possible for a program to know what actually went wrong without knowing system-specific errno codes. FileException would then presumably derive from ErrnoException to give access to the underlying error code, but by having FileException provide a member indicating the error in a system-agnostic manner, it would definitely be more user-friendly IMHO. And if it defaulted to FileError.unknown (or whatever we want to call it) for a constructor that just took a message, then it could be backwards compatible with FileException as it is now (complete with having the errno member, because the base class would have it).

[andralex]

So there are some breaking changes. What are the wins that justify them?

[DmitryOlshansky]

@andralex

Cleaning up the mess with platform-specific exceptions. All of this kind of bobbled up when Windows code was fixed to use GetLastError (and wenforce) instead of wrongly relying on errorno. Then it turned out that FileException was tied to cenforce so we traded one problem for another.

So the proper solution is to have nice, platform independant exceptions.
This is important work to get done before 2.067 release.

[CyberShadow]

FileException would then presumably derive from ErrnoException to give access to the underlying error code,

That's not possible. "File exceptions" can be caused by:

1. Windows API errors
2. C standard library function errors
3. Neither of the above.

but by having FileException provide a member indicating the error in a system-agnostic manner, it would definitely be more user-friendly IMHO.

I recall that this was discussed when DIP33 was proposed. There's some issues with the enum-based approach, such as not being able to catch only certain types of errors as indicated by the enum, and (because there is no subclassing) potentially breaking code that was checking for unknown every time we add an exception type to the enum.

So there are some breaking changes. What are the wins that justify them?

If you mean my last comment, I intend to fix those in my next pass over the code (probably when we decide the deprecation path).

If you mean the minor ones listed in my first comment, I elaborated on the reasons for the change in the linked issue.

[CyberShadow]

As a sidenote, I really wish we could construct the exception's msg lazily. It is not always cheap and is only used when the exception bubbles up to the user.

[jmdavis]

That's not possible. "File exceptions" can be caused by:

Windows API errors
C standard library function errors
Neither of the above.

Well, I think that it's a serious negative if you can't catch a FileException and know that it's a problem with a file. And I'm fine with having FileNotFoundException and InsufficientPermissonsException or whatever instead of on an enum, but if you're going to react to the exception in any sane way other than just throwing your hands up in the air (e.g. try and tell the user what went wrong so that they can correct the problem), then you need to be able to programatically know what went wrong and not just via a human-readable message (particularly since users really shouldn't be seeing exception messages in most cases). And not having that error be handled in a system-agnostic way is not friendly at all. You shouldn't have to care whether you're on Windows or Linux to respond to a file-related error. I've thought for some time that we should overhaul FileException so that it could be sanely used in a cross-platform way but unfortunately never got around to doing anything about it, and this proposal seems to go in the opposite direction and make it so that you don't even know that you're dealing with a file-related problem anymore.

This proposal makes it so that the specific exception type - ErrnoException or WindowsException - aren't even exceptions that you would normally want to catch. They're giving information that you should only have to deal with if you're dealing with something abnormal - i.e. the errno code and whatnot. They're not at all cross-platform - quite the opposite. The result is that it makes it very difficult to react to any of these exceptions meaningfully. Programmers will be forced to catch the ErrnoException and WindowsException types explicitly and then look up what on earth their corresponding error codes mean so that they have a clue what's going on instead of having exception types which are cross-platform and then potentially give you access to the lower-level details if you need them. IMHO, this approach is completely backwards.

I don't know how well we can fix this without breaking code, but IMHO what you're suggesting isn't much better than just making everything throw Exception.

[CyberShadow]

but IMHO what you're suggesting isn't much better than just making everything throw Exception.

This PR fixes implementation, not user-facing design. DIP33 was meant to fix design.

The WindowsException and ErrnoException classes are distinct because their implementation is distinct (in how they interpret the error code).

Users should not catch WindowsException or ErrnoException. All documentation that throws either of those must mention OSException.

I think this is the wrong place to seriously discuss proposals which dramatically change the user-facing design, such as the enum proposal. There are numerous ambiguous questions such as what members the enum should have, or questions such as: "If process creation fails because the indicated file was not found, is it a FileException or a ProcessException? And if we have a FileNotFoundException, it has to be a subclass of FileException even though the failing function was in std.process, not std.file."

Well, I think that it's a serious negative if you can't catch a FileException and know that it's a problem with a file.

I'd like to contest this point of view. OS operations, on files or otherwise, can fail due to a multitude of reasons. The documentation of any API or C call has a list of failure reasons and associated error codes. A FileException by itself is thus of very limited use. Any effective exception recovery for file operations requires wrapping a very small amount of code in a try block (usually a single function call).

I agree that it would be useful to e.g. try deleting a file that might exist, and silently catch an exception indicating the file did not exist, and letting all others propagate. I don't see a good way to do it either.

But I'm sure all this was already discussed along with DIP33.

[DmitryOlshansky]

Time to rebase. We NEED platform-agonstic exceptions in std lib no 2 thoughts about it.
I agree with @CyberShadow on the FileException matter.

[DmitryOlshansky]

I think a I have good idea of 2nd iteration on DIP 33 that uses Interfaces instead of enums, will try to compose sometime soon.

[CyberShadow]

Does anyone have any thoughts on the deprecation path for the old exception classes? It's the only remaining open question regarding the contents of this patch.

TODO:

• Decide on and apply deprecation path for old classes
• Make it possible to throw FileException again, for compatibility
• Revert changes which replaced throwing FileException with no error code
• Add a note in the documentation of WindowsException and ErrnoException to indicate that these exceptions should not be caught, and that OSException should be caught instead
• Fix throw new RangeError("Command line is empty");

[quickfur]

Merge conflicts, please rebase, thanks!

[CyberShadow]

Why? It's not ready to merge even ignoring conflicts.

[quickfur]

Nevermind, got mixed up with another PR.

[jmdavis]

Add a note in the documentation of WindowsException and ErrnoException to indicate that these exceptions should not be caught, and that OSException should be caught instead

If WindowsException and ErrnoException should not be caught, then why have them? Pretty much the entire reason to have a specific exception type is so that you can catch it explicitly, because it means something different from other exception types (including more generic ones), and that affects how you handle the error condition. If the only reason to have them is because their internals are different, then static if should solve that. But if the reason is that they should be treated differently, then why not catch them?

[CyberShadow]

Because the error code changes meaning depending on where it comes from. There needs to be a way to distinguish errno codes from GetLastError ones, whether through inheritance or otherwise.

[DmitryOlshansky]

If WindowsException and ErrnoException should not be caught, then why have them? Pretty much the entire reason to have a specific exception type is so that you can catch it explicitly, because it means something different from other exception types (including more generic ones

This is wrong on "pretty much the entire reason" line. The reason (as usual) to have different types is because the implementation is very different. And the connection between them is that they represent the same kind of thing - operating system exception, so there is little need to catch them explicitly (unless deliberately staying unportable) rather then by portable alias (or super type).

[JakobOvrum]

Ping. Any progress on this?

[quickfur]

ping
It would be nice to get a resolution on this, as several PRs depend on it.

[CyberShadow]

Do we need consensus or can I just fix up the remaining issues myself and someone will merge?

[quickfur]

Well, I just wanted to get the discussion going again, I didn't participate too much in the discussion so I don't know if it's merge-ready, just that it's stopped for too long. :-P

Dmitry did propose on the forum to support catching by interface, then it would address people's concerns about wanting to catch exceptions by meaningful types as opposed to implementation-specific types.

[CyberShadow]

I'm going to have to revisit this subject in more depth, then.

[andralex]

closing for now, @CyberShadow please reopen when you're on this

[CyberShadow]

I would appreciate if someone familiar with Phobos could take over this, currently I would prefer to spend time on improving dlang.org and the forum rather than debating exception hierarchies.