Tracking Issue for io_error_more

[ijackson]

View all comments

Feature gate: #![feature(io_error_more)]

This is a tracking issue for many io::ErrorKind variants being added in #79965.

• FilesystemLoop ([discussion] ErrorKind::FilesystemLoop from io_error_more #130188)
• and a bunch more that have all been stabilized

The additions were motivated by a perusal of Unix standards, but corresponding mappings for Windows were included in that PR.

Public API

std::io::ErrorKind::Foo for many Foo. I will paste a list here when the MR is merged :-).

Steps / History

• Implementation: More ErrorKinds for common errnos #79965
• Final commenting period (FCP)
• Partial Stabilization - Stabilize most of io_error_more #128316
• Stabilization PR

Unresolved Questions

• What is our general policy about ErrorKind variants ? It may not be necessary to resolve this wider question in order to stabilise this particular set of ErrorKinds.

[TheBlueMatt]

What is our general policy about ErrorKind variants ?

Just a bit of user feedback, this PR landing in beta broke our CI as we were asserting certain io Errors. I dunno if you can really call it a "language-breaking" change, but it did seem surprising that our CI would fail on beta due to rust lib changes.

[SimonSapin]

https://doc.rust-lang.org/std/path/struct.Path.html#method.is_dir documents:

This is a convenience function that coerces errors to false. If you want to check errors, call fs::metadata and handle its Result. Then call fs::Metadata::is_dir if it was Ok.

I’ve implemented this by coercing ErrorKind::NotFound to false and propagating other errors. However some of my tests fail in cases where ErrorKind::NotADirectory is (or would be) returned, showing that it should also be coerced to false.

This and the previous comment show that these variants are not like other unstable features that crates who don’t opt-in can pretend don’t exist, they can already be present in errors returned by the standard library. I’d like @rust-lang/libs to consider stabilizing them soon.

---

@TheBlueMatt, was your code that broke expecting ErrorKind::Other or does this also affect values that previously were, well, other than Other?

[TheBlueMatt]

@TheBlueMatt, was your code that broke expecting ErrorKind::Other or does this also affect values that previously were, well, other than Other?

Yes, it was checking that an error was Other. Still, that seems like a good enough reason that this can't be stabilized as-is? Its not unreasonably that some non-test code could be checking for Other for one reason or another, no?

[joshtriplett]

@TheBlueMatt Other has been documented for some time as not something you should directly check for:

Errors that are Other now may move to a different or a new ErrorKind variant in the future. It is not recommended to match an error against Other and to expect any additional characteristics, e.g., a specific Error::raw_os_error return value.

#85746 is the change that made the standard library stop returning Other, which was a prerequisite for adding new ErrorKind values.

We do want to stabilize these new variants soon. But in the meantime, if you have code that matches Other and checks a specific underlying error code, you should be able to change that to match _ and check the specific error code.

[TheBlueMatt]

@TheBlueMatt Other has been documented for some time as not something you should directly check for:

See-also discussion ending at #79965 (comment), but the specific documentation that isn't only lightly implying this being an issue was added middle of last year (which is much more recent than our test code here). Indeed, it was implied earlier, but for those of us who weren't thinking really deeply about the implications of the struct-level docs it didn't come across entirely.

you should be able to change that to match _ and check the specific error code.

Right, we'd have to do even more platform-specific stuff, which I'm a bit hesitant to do, I guess we can just drop our tests, but I do worry about other code that may exist that isn't pure-tests from other projects.

I'm honestly somewhat surprised this is moving forward without some attempts at a rustc warning better communicating to users that the code they have written that compiles and runs is actually undefined behavior (not in the C sense of may-eat-your-cat, but in the may-change-in-the-future, behavior is not guaranteed sense). I understand that may not be practical to hit in all cases, but I imagine at least some direct match .. Other or if == Other things could likely be detected. In general, I am very surprised this doesn't fall under rustc's stability guarantees around running, operational, functional code in the wild not being broken by future versions of rustc.

[ijackson]

Hi, @TheBlueMatt

As the author of one of the MRs which actually broke your test, I would like to say that I'm sorry about that. Unfortunately, risking breakage like yours seemed like the best option. I know it sucks when you're on the wrong side of a tradeoff like this - especially when it suddenly breaks your stuff.

You are right to say that it would have been better to have arranged to warn about this. But doing so in rustc is not currently straightforward. ErrorKind::Other had to be a stable variant, because users of the stdlib need to be able to construct errors with that kind. But we do still need to be able to add variants, rather than having this enum frozen for all time.

Perhaps if we had thought of it sooner, a clippy lint would have made sense. But we didn't have one. (And maybe that wouldn't have helped you; not everyone runs clippy. I know I generally don't, despite feeling guilty about that...)

But given where we were, our sensible options were risking this kind of breakage, or postponing the addition of new ErrorKinds (perhaps for some considerable time) while we did more preparation (eg trying to warn people).

My view is that the new ErrorKinds are very sorely needed. For example, just this week I was writing a program that needs to read configuration from filesystem objects which might be files, or directories. The IsADirectory error from File::open makes that more convenient to do portably and avoids having to ever discard an ErrorKind::Other on the basis of complex reasoning about the filesystem state.

Whether avoiding a substantial delay to new ErrorKinds was worth risking this kind of breakage seems like something reasonable people can disagree on. As others have said, one justification is that the documentation has always said that ErrorKind might grow new variants, and to my mind the implication that those new variants would probably appear instead of Other is obvious. I guess that is cold comfort to you.

Another mitigation is that even when this causes breakage, it is likely to be in tests (as in your case) - ie, the risk of bad code being deployed as a result is considerably lower than that of a test failure.

Having said all that: given this is only in beta, I think we do have the option to back this out ? If we did that, how long would we wait ? Who would write the missing clippy lint or whatever ?

Personally I'm torn. Rust's care for stability is one of its great attractions to me and I regret having undermined that for some of Rust's users. On the other had the previous ErrorKind enum was IMO seriously deficient, and the change in my MR is precisely the kind of change which the docs have always explicitly said may happen (even if all the consequences have only been explicitly spelled out more recently).

I want to reply separately about what alternative idiom I think your tests should probably use.

[joshtriplett]

On Thu, Jul 29, 2021 at 12:22:18PM -0700, Matt Corallo wrote: > you should be able to change that to match _ and check the specific error code. Right, we'd have to do even more platform-specific stuff, which I'm a bit hesitant to do,

If you're not checking for any specific error code, then I don't think it makes sense to check for `Other` at all. That would *inherently* break if a new ErrorKind variant were ever added. `ErrorKind` is `#[non_exhaustive]` precisely to allow adding new error types; as a result, asserting "this is an error type that doesn't have a more specific enum variant" is inherently fragile and non-forwards-compatible.

I guess we can just drop our tests, but I do worry about other code that may exist that isn't pure-tests from other projects.

It seems inherently unreliable to match `Other` for anything other than "some error I don't know about", given the above. Code doing that in a match without a `_` branch would already get a warning about `#[non_exhaustive]`. So the only way to have this happen would be an `==` or `if let` or similar, or an assert. We could add a clippy lint against asserts that match `Other` without an error code. That might have some false-positives, for code that actually uses Other for something other than an unhandled errno value, but it might still be viable as a clippy lint.

[TheBlueMatt]

On Jul 29, 2021, at 17:01, Ian Jackson ***@***.***> wrote:  Hi, @TheBlueMatt As the author of the MR which actually broke your test, I would like to say that I'm sorry about that. Unfortunately, risking breakage like yours seemed like the best option. I know it sucks when you're on the wrong side of a tradeoff like this - especially when it suddenly breaks your stuff.

First of all, it’s software, things happen, in our case it was just some minor check in a test, no sweat!

You are right to say that it would have been better to have arranged to warn about this. But doing so in rustc is not currently straightforward. ErrorKind::Other had to be a stable variant, because users of the stdlib need to be able to construct errors with that kind. But we do still need to be able to add variants, rather than having this enum frozen for all time. Perhaps if we had thought of it sooner, a clippy lint would have made sense. But we didn't have one. (And maybe that wouldn't have helped you; not everyone runs clippy. I know I generally don't, despite feeling guilty about that...)

Indeed, sadly because clippy requires rustup I generally can’t use it anyway. :(

But given where we were, our sensible options were risking this kind of breakage, or postponing the addition of new ErrorKinds (perhaps for some considerable time) while we did more preparation (eg trying to warn people).

I do wonder if it’s practical to do this in an edition? The next edition isn’t shaping up to be that far away, and adding this kind is breaking thing seems like a great candidate for such a thing! I’m not suggesting waiting for editions for every new variant, but doing the first new additions in an edition with more linting may avoid a lot of this.

My view is that the new ErrorKinds are very sorely needed. For example, just this week I was writing a program that needs to read configuration from filesystem objects which might be files, or directories. The IsADirectory error from File::open makes that more convenient to do portably and avoids having to ever discard an ErrorKind::Other on the basis of complex reasoning about the filesystem state.

Makes sense, there’s quite a tradeoff here, but I do really worry about stability of rust here, and the guarantees around upstream not breaking existing code seemed stronger than they appear to be now. I suppose I just weigh stability differently.

Whether avoiding a substantial delay to new ErrorKinds was worth risking this kind of breakage seems like something reasonable people can disagree on. As others have said, one justification is that the documentation has always said that ErrorKind might grow new variants, and to my mind the implication that those new variants would probably appear instead of Other is obvious. I guess that is cold comfort to you.

Partially. I agree that’s what it implies, but the explicit mention was only added very recently, and for those of us not super deep in the libstd world I guess it didn’t come across until now.

Another mitigation is that even when this causes breakage, it is likely to be in tests (as in your case) - ie, the risk of bad code being deployed as a result is considerably lower than that of a test failure. Having said all that: given this is only in beta, I think we do have the option to back this out ? If we did that, how long would we wait ? Who would write the missing clippy lint or whatever ?

I would think at least simple checks could happen in rustc and not clippy. Eg matching/comparing equality on a non-exhaustive enum with an “other” variant seems like something that rustc could warn on, though I admit I’m not on any way familiar with the rustc codebase, so feel free to tell me I’m wrong.

…

Personally I'm torn. Rust's care for stability is one of its great attractions to me and I regret having undermined that for some of Rust's users. On the other had the previous ErrorKind enum was IMO seriously deficient, and the change in my MR is precisely the kind of change which the docs have always explicitly said may happen (even if all the consequences have only been explicitly spelled out more recently). I want to reply separately about what alternative idiom I think your tests should probably use. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[joshtriplett]

@TheBlueMatt Unfortunately, it isn't possible for us to make library changes like this in an edition. Editions can affect how code is compiled, but not the underlying behavior of the standard library, partly because you can have code from Rust 2015, 2018, and 2021 all compiled into one binary and sharing the same standard library.

For what it's worth, we absolutely value stability as critically important. In this case, we made a careful tradeoff, knowing that some code out there (especially test code) might be affected. We did consider making these alternatives insta-stable, but that still would have broken your test case because the enum variant would no longer match. We also considered "never extend ErrorKind again" as a potential alternative, but we felt that because ErrorKind is #[non_exhaustive] and because we don't expect non-test code to typically match on Other in this fashion, we thought it would be reasonably safe to make this change.

One other note: #85746 (which is the change that actually affected your test) is targeted at 1.55, and has a relnotes tag; we're expecting to have a detailed release note explaining the situation for people. A release note does not ameliorate the breakage, but it should help communicate the nature and rationale and tradeoffs of the change.

[TheBlueMatt]

In this case, we made a careful tradeoff, knowing that some code out there (especially test code) might be affected. We did consider making these alternatives insta-stable, but that still would have broken your test case because the enum variant would no longer match. We also considered "never extend ErrorKind again" as a potential alternative, but we felt that because ErrorKind is #[non_exhaustive] and because we don't expect non-test code to typically match on Other in this fashion, we thought it would be reasonably safe to make this change.

I'm curious if extending the #[non_exhaustive] API in rustc to warn around matching Other variants was considered? eg adding a tag to #[non_exhaustive] to certain variants which indicates "generate a warning if this is matched on or compared against" at compile-time so as to make this exact language pattern better supported. I understand this is maybe part of the goal of #85746, but it does feel like an oversight in the expressiveness of the #[non_exhaustive], as mentioned in the description of #85746.

[ijackson]

I'm curious if extending the #[non_exhaustive] API in rustc to warn around matching Other variants was considered? eg adding a tag to #[non_exhaustive] to certain variants ...

No, we didn't think of that.

I understand this is maybe part of the goal of #85746, but it does feel like an oversight in the expressiveness of the #[non_exhaustive], as mentioned in the description of #85746.

Now that #85746 has landed, I don't think this situation can arise again with ErrorKind. In the future, new ErrorKinds may indeed be added, for new kinds of underlying OS errors. But when that happens, the new kind will appear instead of Unknown - not instead of Other. Unknown is permanently unstable so cannot be matched in tests (without turning on an unstable feature). So it would not be possible to write, on stable, a test which will break in the future.

I don't know if there are other enums in the stdlib with similar properties to ErrorKind. I can't think of any offhand.

[TheBlueMatt]

No, we didn't think of that.

I do wonder if it may make sense as a language feature in the future also for non-stdlib users. It doesn't feel strange to think that some crates may want a similar pattern. I admit the 85746 approach may make more sense to them, but there is no way to non-stabilize a particular enum variant outside of std, as far as I understand.

Unknown is permanently unstable so cannot be matched in tests (without turning on an unstable feature). So it would not be possible to write, on stable, a test which will break in the future.

Ah, maybe I partially misunderstood the way 85746 operates.