Standardize system error codes for File::Error

[straight-shoota]

We're interpreting error codes from the operating system all over the file and process implementations, as well as when instantiating File::Error.
This patch merges them together so we always map the same set of error codes to specific error classes (not found, access denied etc.).

Resolves #15905
Closes #15902

[HertzDevil]

Could these be plain methods instead? I'd prefer not having to use Arrays of unions for this

[straight-shoota]

thought: I'm not entirely sure about these OS_ERRORS arrays. It good that they are strongly connected to the respective error classes.
But there are some issues, for example searching in an array is not ideal for performance. For example, splatting the array into a when expression might be better. But it's also less ergonomical.
The WinError values could never match on unix targets, which could be another approach for improval.

I have considered alternative, but it's unclear whether they would be better:

• A class method (.os_error?(error)) that returns true if the parameter matches one of the error codes. This is a less declarative approach. But might be more efficient?
• Group predicate methods on Errno and WinError. Would be easy to implement, but more detached from the error classes. We could consider using the same name as the error classes, though (e.g. #not_found_error?, #access_denied_error?).

[HertzDevil]

The class method body could be a case-expression generated from such an array with a macro, but at that point it isn't much different from not using a macro.

[Sija]

Why arrays and not tuples? Tuples have better performance and can be splatted into when expressions.

[straight-shoota]

Arrays can also be splatted into when expressions. There's no reason for these arrays to exist at runtime. All use cases could be splatted at compile time.
It's just not very convenient to do so because we would need to wrap the entire case in {% begin %}...{% end %}.

I'll rewrite into class methods to see how that looks.

[ysbaddaden]

I don't like this. We shall keep the errno / winerror checks buried under Crystal::System.

• it's leaking system specifics out of crystal/system;
• it creates an indirection when we want to know which errno or winerror will lead to an exception;
• it's pushing target-specifics constants to every target, so POSIX targets have to check errno against all the winerror that will never match (the opposite for windows).

[ysbaddaden]

I like the explicit exceptions. I just don't like the #os_error? methods.

[ysbaddaden]

Said differently: each system implementation decides what exception to raise, it shouldn't have to ask each exception "hey, should I raise you?"

[straight-shoota]

Neither Errno nor WinError nor File::Error are in Crystal::System. So it feels like this doesn't need to belong there either.

We could consider moving these methods into Crystal::System. That breaks the direct connection to the error types. But I suppose it's not essential. I'm fine either way.

I suppose we could exclude checks against error codes that are impossible on the target system.
This would only mean putting the WinErrror codes behind {% if flag?(:win32) %} because some Windows API functions use errno, so on Windows we need all.

[straight-shoota]

The question is not "should I raise you?" but "I got this error, does it fit the error class you describe?". And then proceed with either raising that error (which File::Error.from_os_error handles) or reacting to that error condition in a specific way.

[HertzDevil]

If we want to eliminate the cross-type checks unless the argument is indeed a union between the system error code types, maybe one of these would work:

class File::Error
  def self.os_error?(error : Errno | WinError | WasiError)
    false
  end
end

class File::AlreadyExistsError < File::Error
  def self.os_error?(error : Errno)
    error == Errno::EEXIST
  end

  def self.os_error?(error : WinError)
    error.in?(
      WinError::ERROR_ALREADY_EXISTS,
      WinError::ERROR_FILE_EXISTS,
    )
  end
end

enum Errno
  def os_error_for?(ex : File::AlreadyExistsError.class)
    self == EEXIST
  end

  def os_error_for?(ex : File::Error.class)
    false
  end
end

enum WinError
  def os_error_for?(ex : File::AlreadyExistsError.class)
    self.in?(
      ERROR_ALREADY_EXISTS,
      ERROR_FILE_EXISTS,
    )
  end

  def os_error_for?(ex : File::Error.class)
    false
  end
end

[ysbaddaden]

Sorry, I'm trying my best, but the more I look into the PR the less I understand how replacing a few errno checks after a syscall with an out-of-scope method call is supposed to be better. For example 🫤

[straight-shoota]

The main problem is that the current checks are incomplete. #15905 lists which error codes should usually be interpreted as "file not found". Currently, we're missing out on some of them in practically every instance. For example, the referenced example is missing ENOTDIR and ENAMETOOLONG.
On Windows we're generally even missing more than on Unix because Windows has more different codes.

The first step to fix that would be to add all the missing error codes in all the places where they're missing.
In system/win32 we're currently checking for error codes relating to File::NotFoundError in four different places. Each time we'd need to compare against ten different error codes, as identified in #15902.

That's a lot of duplication checking exactly the same set of values all over the place. And the individual error check conditions would be big and ugly to read. So IMO giving this common behaviour a name and extracting them into a shared helper is a good idea.

How exactly we implement that is up for debate.

I'm dismissing my review to not block the PR, but I'm still not fond of these indirections; direct errno/winerror checks after a syscall keep things linked together, and there aren't so many of them that we really need to refactor.