Redefine `ErrorKind::Other` and stop using it in std. by m-ou-se · Pull Request...

Copy link

Member

This comment has been hidden.

FWIW I don't think a crater run will give useful data here; removing Other entirely and running a crater run there might give us the places where people are doing something with it and then go from there manually. But my guess is there's going to be a lot of code that does that...

CDirkx commented 24 days ago •

Regarding how to ensure we don't use Other, maybe it could be implemented as a tidy check. Similar to the "Platform Abstraction Layer" (src/tools/tidy/src/pal.rs), which essentially performs a text search for "#[cfg(target_os = ...)]" etc., we could search for "ErrorKind::Other". This might not catch all cases as it doesn't use type information, but might be good enough for an internal lint for std.

jyn514 commented 24 days ago

You could also add an allow-by-default lint meant only for the standard library.

joshtriplett commented 24 days ago

A crater run won't be definitive, but it'll catch cases where it affects someone's testsuite at least.

Regarding use in the standard library, I think a tidy check for ErrorKind::Other would be a good start for now, and we can always add something more later.

yaahc commented 24 days ago

Some comments:

• I'm not a fan of calling this Unknown and worry that it doesn't accurately describe these kinds of errors
  • The debug impl output might be confusing, Repr::Custom { kind: ErrorKind::Unknown, .. }
  • Users may read this from an unwrap and think this means they encountered a custom error of unknown origin, whatever that means.
• I think we should call this Unclassified instead of Unknown
  • ErrorKind exists to classify errors, and each kind may abstract over multiple underlying source errors
  • More aligned with the stability policy of reserving the right to reclassify errors in the future
  • Alternative names I've considered: Unstable, Internal, None
• Is it accurate to describe these as all being os errors in the documentation and Display impl for ErrorKind?
  • It doesn't seem like all of these are being used to represent os errors
  • What precisely does it mean to be an OS error? Is it the presence of an associated Error code? That seems restrictive / bad, so hopefully not
  • if we want "os error" to mean "errors with error codes" then perhaps we should actually be introducing two new variants, one of which we only use in the various implementations of decode_error_kind
  • if we think it means "any error that originates from an os API" then this seems fine

m-ou-se commented 24 days ago

Yeah the name Unknown is just a bikeshed placeholder. We should put something better there.

I just left the "other os error" message as it was. I only removed "os" from Other because that's now only used for user errors. But maybe it's time to update it, as it's not really accurate. Maybe both should be just "other error".

joshtriplett commented 24 days ago

"Unclassified" seems good to me.

m-ou-se commented 24 days ago •

Unclassified - (of data, documents, etc.) not belonging to a category that is restricted for reasons of security; not secret:

That sounds completely opposite of #[doc(hidden)] #[unstable] ;)

(Not that ErrorKind::TopSecret would be better. ^^)

yaahc commented 24 days ago •

I just left the "other os error" message as it was. I only removed "os" from Other because that's now only used for user errors. But maybe it's time to update it, as it's not really accurate. Maybe both should be just "other error".

Oh, I should have paid more attention to the before part of the diff. If these are up for grabs then I'd probably go with "user error" for ErrorKind::Other and "other error" or "unclassified error" for ErrorKind::Bikeshed. For the second one I'd normally prefer "other error" but I'm worried it will be confusing because there is an ErrorKind::Other that you'd think would be associated with this description. This is making me lean strongly towards wanting to just deprecate ErrorKind::Other and introduce an ErrorKind::User or something to replace it.

joshtriplett commented 24 days ago

@yaahc I think such a deprecation would cause a large amount of churn. I'd rather keep Other for user-specified errors, and just introduce a separate "unclassified OS error code" or similar.

joshtriplett commented 23 days ago

I posted a few minor comments. Otherwise, this LGTM, and I'd be happy to see it FCPed once we settle naming and messages.

joshtriplett commented 20 days ago

In the interests of seeking consensus, I'm proposing this for FCP, while also raising a concern to reflect that we haven't settled on a final name or user-visible messages yet. (That said, the name is permanently unstable, so I think the user-visible messages are more important.)

@rfcbot merge

@rfcbot concern naming-and-messages

rfcbot commented 20 days ago •

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

• naming-and-messages resolved by #85746 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

m-ou-se commented 5 days ago

I'd probably go with "user error" for ErrorKind::Other

"user" can refer to a lot of things. If a program reports a 'user error', I'd expect it's about the user of that program. Not the user of one of the libraries used to make the program.

joshtriplett commented 5 days ago

@rfcbot resolve naming-and-messages

rfcbot commented 5 days ago

This is now entering its final comment period, as per the review above.

joshtriplett commented 5 days ago •

Proposed text for release notes:

std::io::ErrorKind is a #[non_exhaustive] enum that classifies errors into portable categories, such as NotFound or WouldBlock. Rust code that has a std::io::Error can call the kind method to obtain a std::io::ErrorKind and match on that to handle a specific error.

Not all errors are categorized into ErrorKind values; some are left uncategorized and placed in a catch-all variant. In previous versions of Rust, uncategorized errors used ErrorKind::Other; however, user-created std::io::Error values also commonly used ErrorKind::Other. In (this version of Rust), uncategorized errors now use the internal variant ErrorKind::Uncategorized, which we intend to leave hidden and never available for stable Rust code to name explicitly; this leaves ErrorKind::Other exclusively for constructing std::io::Error values that don't come from the standard library or an OS function. This enforces the #[non_exhaustive] nature of ErrorKind. Rust code should never match ErrorKind::Other and expect any particular underlying error code; only match ErrorKind::Other if you're catching a constructed std::io::Error that uses that error kind. Rust code matching on std::io::Error should always use _ for any error kinds it doesn't know about, in which case it can match the underlying error code, or report the error, or bubble it up to calling code.

m-ou-se commented 5 days ago

non-OS-originated

There's plenty of crates that interact with the OS without using std. We should probably just say std instead of OS. (Especially since some of them are directly from std and not from the OS. E.g. "invalid utf-8".)

joshtriplett commented 4 days ago

@m-ou-se Fixed.