More ErrorKinds for common errnos

[ijackson]

From the commit message of the main commit here (as revised):

There are a number of IO error situations which it would be very
useful for Rust code to be able to recognise without having to resort
to OS-specific code.  Taking some Unix examples, `ENOTEMPTY` and
`EXDEV` have obvious recovery strategies.  Recently I was surprised to
discover that `ENOSPC` came out as `ErrorKind::Other`.

Since I am familiar with Unix I reviwed the list of errno values in
  https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

Here, I add those that most clearly seem to be needed.

@CraftSpider provided information about Windows, and references, which
I have tried to take into account.

This has to be insta-stable because we can't sensibly have a different
set of ErrorKinds depending on a std feature flag.

I have *not* added these to the mapping tables for any operating
systems other than Unix and Windows.  I hope that it is OK to add them
now for Unix and Windows now, and maybe add them to other OS's mapping
tables as and when someone on that OS is able to consider the
situation.

I adopted the general principle that it was usually a bad idea to map
two distinct error values to the same Rust error code.  I notice that
this principle is already violated in the case of `EACCES` and
`EPERM`, which both map to `PermissionDenied`.  I think this was
probably a mistake but it would be quite hard to change now, so I
don't propose to do anything about that.

However, for Windows, there are sometimes different error codes for
identical situations.  Eg there are WSA* versions of some error
codes as well as ERROR_* ones.  Also Windows seems to have a great
many more erorr codes.  I don't know precisely what best practice
would be for Windows.

Errno values I wasn't sure about so *haven't* included:

EMFILE ENFILE ENOBUFS ENOLCK:

  These are all fairly Unix-specific resource exhaustion situations.
  In practice it seemed not very likely to me that anyone would want
  to handle these differently to `Other`.

ENOMEM ERANGE EDOM EOVERFLOW

  Normally these don't get exposed to the Rust callers I hope.  They
  don't tend to come out of filesystem APIs.

EILSEQ

  Hopefully Rust libraries open files in binary mode and do the
  converstion in Rust.  So Rust code ought not to be exposed to
  EILSEQ.

EIO

  The range of things that could cause this is troublesome.  I found
  it difficult to describe.  I do think it would be useful to add this
  at some point, because EIO on a filesystem operation is much more
  serious than most other errors.

ENETDOWN

  I wasn't sure if this was useful or, indeed, if any modern systems
  use it.

ENOEXEC

  It is not clear to me how a Rust program could respond to this.  It
  seems rather niche.

EPROTO ENETRESET ENODATA ENOMSG ENOPROTOOPT ENOSR ENOSTR ETIME
ENOTRECOVERABLE EOWNERDEAD EBADMSG EPROTONOSUPPORT EPROTOTYPE EIDRM

  These are network or STREAMS related errors which I have never in
  my own Unix programming found the need to do anything with.  I think
  someone who understands these better should be the one to try to
  find good Rust names and descriptions for them.

ENOTTY ENXIO ENODEV EOPNOTSUPP ESRCH EALREADY ECANCELED ECHILD
EINPROGRESS

  These are very hard to get unless you're already doing something
  very Unix-specific, in which case the raw_os_error interface is
  probably more suitable than relying on the Rust ErrorKind mapping.

EFAULT EBADF

  These would seem to be the result of application UB.

(omitted errnos are discussed below, especially in https://github.com//pull/79965#issuecomment-810468334)

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[ijackson]

@rustbot modify labels +T-libs +A-io

[Mark-Simulacrum]

r? @m-ou-se

Probably needs a T-libs decision, maybe discussion.

[nagisa]

A similar change to Window's decode_error_kind should happen as part of this PR.

[m-ou-se]

We've discussed this PR in the library team meeting a few days ago, and we generally felt positive about this change. We discussed two concerns:

1. This might break code matching specifically on ErrorKind::Other. However, as the documentation already mentions this, this is acceptable.
2. It might be too Unix-specific. We'd like to see how this works out for the other platforms (especially Windows). Some of these might have a name or description that is too specific for Unix and doesn't fit the equivalent Windows error. Or it might be the case that two related new ErrorKinds are represented as the same error code on Windows, which would make it tricky to map them properly. We'd like to see an implementation for Windows as well, to make sure this won't cause any problems there later.

[ijackson]

2. It might be too Unix-specific. We'd like to see how this works out for the other platforms (especially Windows). Some of these might have a name or description that is too specific for Unix and doesn't fit the equivalent Windows error. [...] We'd like to see an implementation for Windows as well, to make sure this won't cause any problems there later.

Thanks for looking at this. That concern makes sense to me. However, I know very little about Windows and am certainly not able to do this. Is there some way I could request help from a Rustacean who is an expert in the Windows APIs, to do the Windows part of this ? Taking one of the sentences out from your comment:

Or it might be the case that two related new ErrorKinds are represented as the same error code on Windows, which would make it tricky to map them properly.

It seems to me that it would be best to have distinct `ErrorKind` values whenever an important platform makes a distinction between two of its own error numbers. The downside of that is that portable code might have to check for two errors in some circumstances, but with the alternative approach of squashing two errors into one, code that knows more about what's going on, and is doing something a bit subtle, would have to resort to a non-portable API to be correct. But the converse concern, which I quoted earlier, is definitely valid. And it certianly does make sense to consider the names and descriptions in the light of Windows.

1. This might break code matching specifically on ErrorKind::Other. However, as the documentation already mentions this, this is acceptable.

Right. Thanks, Ian.

…

-- Ian Jackson <[email protected]> These opinions are my own. Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

[m-ou-se]

Is there some way I could request help from a Rustacean who is an expert in the Windows APIs, to do the Windows part of this ?

You could try asking on Zulip (in #t-libs or #t-compiler/windows) first. If that doesn't work, we can ping a few Windows experts on GitHub here.

It seems to me that it would be best to have distinct ErrorKind values whenever an important platform makes a distinction between two of its own error numbers.

Yes, maybe. But that's easier to see once we have some concrete error for which this is the case.

[CraftSpider]

I'm not a master of Windows IO, but I took some time to go through and link the system error codes that look like they match the new error types in this PR. If desired, I could split this information into some other location for easier reading.

[ijackson]

@CraftSpider Thank you for that! I have replied to a couple of your comments where I think things are not 100% settled, but I think I should be able to make a plausible Windows implementation out of your suggestions. It will still want a review by someone with Windows expertise.

[ijackson]

(After we've decided on the set of Windows error value numbers needed, I'll fish them out of the docs links @CraftSpider helpfully provided and add them to Windows's c.rs.)

[ijackson]

Thanks everone who helped work on this!

@ChrisDenton, you mentioned some Windows error codes which might usefully have ErrorKinds. Now that this batch is merged, you might want to revisit that question. Feel free to tag me in your MR - I love thinking about error handling :-).

[ijackson]

Also, @zackw you had a list in #79965 (comment) which would be worth revisiting now, if you felt like it. FYI I don't expect to have time to pick that up soon...

[TheBlueMatt]

This appears to have broken our CI on beta builds now (which assert errors match expected ones after filesystem operations), though I guess that was expected?

[the8472]

Breakage should only happen if you're matching against ErrorKind::Other, which is discouraged

[TheBlueMatt]

if you're matching against ErrorKind::Other

Indeed, it was an assertion checking exactly that.

which is discouraged

Looks like that was added pretty recently (middle of last year). The code in our tests appears to predate that warning pretty substantially. It seems strange that downstream projects would break after a pretty recent warning, or is this intended to land in a future edition and not current ones?

[the8472]

It was implicit before. The enum docs have been saying this for 7 years:

/// This list is intended to grow over time and it is not recommended to
/// exhaustively match against it.

And Other said this:

/// Any I/O error not part of this list.

So logically if the list is expanded then new items may cover things that were previously "not part of this list", i.e. Other. The change last year only made it more explicit.

[TheBlueMatt]

It was implicit before.

Hmm, to some extent I suppose that makes sense. It still seems incredibly strange that this breakage is "expected" given the rustc commitment of running, in-the-wild code not suddenly breaking due to rustc/libstd changes, especially when there is no compile warning anywhere to indicate this.

There's a much more important related issue here, though - I don't even know how I'd address the issue here even if I wanted to. We are some code that checks the error, which seems like a reasonable thing to want. If we want to support new rustc, it seems the only possible way to do so is to never check the error, because otherwise we'd end up bumping the MSRV to get the new variants to check against?

[the8472]

When checking IO errors you have several options:

match io_thing() {
   Ok(_)  => println!("success"),
   Err(error) if error.kind() == ErrorKind::NotFound =>
      println!("someone deleted a file? incorrect path? ask the user to check if he passed the right one"),
   #[cfg(unix)]
   Err(error) if error.raw_os_error() == Some(libc::EOPNOTSUPP) =>
      println!("some obscure problem with network filesystems? tell user to not use NFS"),
   Err(_) => println!("general error of unknown type, we don't know how to handle this!")
}

Matching against specific ErrorKind only makes sense if you know the meaning you're trying to match against, e.g. because you know how to handle this particular error. Other has no meaning so a wildcard pattern should be used instead.

[TheBlueMatt]

When checking IO errors you have several options:

That doesn't address the issue at all. Lets say we have a test (which we do) which wants to check that the error was due to trying to access a directory as a file. We could manually look up libc errors (yuck, platform specific!), or we could match the new error type (bump MSRV to beta), or we could check Other, ensuring its not one of the common error types, which we did.

Sure, its obviously not ideal, but the docs were not super explicit about this being invalid code (they're now a bit clearer), and more importantly rustc did nothing to warn us that what we wrote wasn't just not-ideal (it was a test, after all), but that it was in fact invalid code that exhibits undefined behavior (in the could-break sense, not the C could-eat-your-cat sense). This feels not just unergonomic but a borderline violation of the rustc stability guarantees. I have to admit I'm incredibly dissapointed the response to this is "who cares" not "alright, lets make sure there's a rustc warning for a few releases then we can revisit" or some other reasonably cautious approach to addressing undefined behavior in rust stdlib.

[the8472]

We could manually look up libc errors (yuck, platform specific!), or we could match the new error type (bump MSRV to beta), or we could check Other, ensuring its not one of the common error types, which we did.

Well, what you're really checking for is "not any of the currently known variants" on a #[non_exhaustive] enum. There is no good way to express this other than

match test_error() {
  ErrorKind::Foo | ErrorKind::Bar | ErrorKind::Baz | ....  => panic!("these are the ones we know and it shouldn't be any of them"),
  _ => return Ok(())
}

importantly rustc did nothing to warn us that what we wrote wasn't just not-ideal

I'm not sure if this is possible. Error is a user-constructable type, so there are cases where you know that an error really should have that value. So linting against that kind of use may lead to too many false positives. Perhaps the documentation could be improved further.

Anyway, since you consider this a bigger issue than I personally do (I'm not speaking for all of rust!) I think it would make sense to open an issue to bring this to broader attention rather than discussing this in an already landed PR.

[ijackson]

Well, what you're really checking for is "not any of the currently known variants" on a #[non_exhaustive] enum. There is no good way to express this other than

match test_error() {
  ErrorKind::Foo | ErrorKind::Bar | ErrorKind::Baz | ....  => panic!("these are the ones we know and it shouldn't be any of them"),
  _ => return Ok(())
}

@TheBlueMatt I think that ^ something like that is precisely what you need to be doing. Because, what you are trying to express is "not one of the ErrorKind variants which existed when I read the docs and wrote this test". (As opposed to "not one of the ErrorKind variants in the current std".)

Short of some madness involving version numbers, I think your best option is to replace your matches against Other with a new function fn is_known_errorkind(ErrorKind) -> bool, containing a match statement cut and pasted from the current stable non-Other ErrorKinds in the docs. That will express precisely what you mean.

That function can't live in stdlib for the same reason as matching against Other doesn't work: you need it to depend on when you wrote the code, freezing your own local ErrorKind list. When you want to match new kinds you can update the function and fix the resulting test failures. Crucially, on your schedule rather than ours.

Anyway, since you consider this a bigger issue than I personally do (I'm not speaking for all of rust!) I think it would make sense to open an issue to bring this to broader attention rather than discussing this in an already landed PR.

@TheBlueMatt I think ^ that is a good suggestion if you want to pursue this. In any case thanks for the feedback and see my other reply #86442 (comment)

[TheBlueMatt]

On Jul 29, 2021, at 17:09, Ian Jackson ***@***.***> wrote: Anyway, since you consider this a bigger issue than I personally do (I'm not speaking for all of rust!) I think it would make sense to open an issue to bring this to broader attention rather than discussing this in an already landed PR. @TheBlueMatt I think ^ that is a good suggestion if you want to pursue this. In any case thanks for the feedback and see my other reply #86442 (comment

Thanks for the feedback. In my specific case it’s probably not even worth fighting with things, we can just remove the test and move on. I figured I’d flag it largely because I’d have assumed this clearly fits the rustc stability guarantee, though obviously I’m aware there are tradeoffs to be had here. I’ll continue on the other thread with the broader issue since for our specific test this doesn’t matter really.

[pthariensflame]

@XAMPPRocky Was this missed for relnotes?

[SimonSapin]

The new variants still have #[unstable] attributes and are tracked here: #86442

[kornelski]

std uses file_name in functions, rather than filename. It'd be odd to break correspondence between snake_case and CamelCase, so I think this should be FileNameTooLong to match. Same goes for InvalidFileName.