search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.76k stars 12.77k forks source link

Tracking Issue for io_error_more #86442

Open ijackson opened 3 years ago

ijackson commented 3 years ago

Feature gate: #![feature(io_error_more)]

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

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

Unresolved Questions

TheBlueMatt commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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.

https://github.com/rust-lang/rust/pull/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 commented 3 years ago

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

See-also discussion ending at https://github.com/rust-lang/rust/pull/79965#issuecomment-889382116, 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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: https://github.com/rust-lang/rust/pull/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

rbtcollins commented 3 years ago

So in 1.55 this has broken our test suite for remove_dir_all by leaking into stable rather than being a nightly only feature.

It seems very poor form to return an enum variant to code which cannot match against it because the compiler considers it a nightly only feature. See https://github.com/XAMPPRocky/remove_dir_all/pull/33 : the failing jobs are failing in stable because the change isn't present there, but putting the change into stable code results in:

error[E0658]: use of unstable library feature 'io_error_more'
  --> src/portable.rs:83:15
   |
83 |             &[io::ErrorKind::NotADirectory, io::ErrorKind::Other],
   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |

To be really clear, my beef isn't with the breakage around Other, its that the new status quo means neither the old code nor the new code works. I'd like to see that remedied ASAP, since we can't run tests at all on stable today. Ironically, the tests are one @ijackson wrote ;)

dtolnay commented 3 years ago

@rbtcollins, to reiterate https://github.com/rust-lang/rust/issues/86442#issuecomment-889480955, nothing is supposed to be comparing equality to ErrorKind::Other. Code that does this is wrong.

If the error kind is not one of the stable specific variants (non-Other), the correct way for stable code to match it is _ and new unstable variants have no bearing on that as they would continue to match _.

rbtcollins commented 3 years ago

@dtolnay Ok, sure, we can fix this, probably by matching every other error type to exclude them but the definition has clearly shifted from 1.54 and before, and this leaves a pretty poor feeling here: https://blog.rust-lang.org/2014/10/30/Stability.html made a huge impression on me, and many others, and it seems like there was debate here, but the conclusion was to modify existing behaviour, rather than allowing edition based opt-in.

The 1.54 docs were: 

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

Errors that are Other now may move to a different or a new ErrorKind variant in the future.

This is very different to "must use _ to match" which implies must exhaustively match every other Kind to be moderately sure the desired error has occurred - which I don't look forward to doing.

rbtcollins commented 3 years ago

Whats the right way to have a discussion about the ergonomics here? 85746 seems like a really awkward interface : theres no way that I know of to ergnomically say 'match only unstable entries'. So when one is interested in a concrete error, that is currently behind an unstable feature flag, the choices are to either accept 'any random error', or list all-known-errors as failure cases.

Mark-Simulacrum commented 3 years ago

If your code previously just checked that the error was ErrorKind::Other, without a more detailed assert (e.g., via the os error code), deleting that test seems like the right path forward, presuming you don't want to add platform-specific code to make the test actually confirm a specific error.

I think the test you cited is a little different -- it looks like it was intending to check that the error in question is not e.g. a permissions error or similar. There it seems like it makes more sense to either move the test toward using raw_os_error and checking a specific error code (of course, platform specific) or asserting that the test is not of some collection of particular variants (e.g., if the goal is to make sure the failure is not due to permissions errors...?).

The only real change after #85746 is that it's harder for someone to assert "this is an error std didn't know enough about to differentiate", but that doesn't seem like a particularly useful assertion for tests to make -- it doesn't test a specific error, only that it's not one of a very small limited subset.

dtolnay commented 3 years ago

Whats the right way to have a discussion about the ergonomics here?

The right way is to counterpropose some alternative way by which new error kinds can be accommodated as operating systems and standard library implementations evolve over time.

rbtcollins commented 3 years ago

@Mark-Simulacrum Yes, we can either take on platform specific handling, or exclude all the known variants that this isn't; and previously we could do that in a pithy fashion by matching on Other exactly.

@dtolnay obviously ;) I meant - do I carry on here, or file a new bug, or create an RFC :- I'm not sure of the most effective way to engage.

Putting some principles forward, it should be possible to both do positive assertions - error is specifically X that we know, including some X that the stdlib hasn't yet categorised. And it should be possible for existing uncategorised things to be categorised better over time.

We can either consider better over time to be at most once per edition, or up to multiple times per edition.

At most once per edition is easy to reason about; there are no strong commitments that nothing will be incompatible between editions. It might be nice to do it more often though.

lets consider the dimensions we're dealing with.

The OS adds variants without coordinating with us, so any enumeration we have will be incomplete. We need to emit those when they happen though, which gives rise to Other and now Unknown.

Callers of APIs will rarely want 234 different code paths, so matching some set of cases and then a wildcard is common. non-exhaustive can be used to require this.

Some callers will be looking for those as-yet uncategorised cases, so they would like some way to match on those. If the uncategorised cases are in a hidden/unstable variant, thats impossible. If those cases are in an accessible variant, recategorising them will be an API break, requiring either callers to accept this is as API skew over time, or for us to only do this at most once per edition. Or recategorising has to be done by some specialisation within the uncategorised bucket.

So one possibility occurs to me:

Have an IoErrorKind::Uncategorised(UnstableCategory)

UnstableCategory is an enum which grows over an edition, and the contents moved to IoErrorKind each edition.

#[non_exhaustive]
enum UnstableCategory {
    Uncategorised,
   ...
}

Then consuming code that wants to match a currently uncategorised error:

match kind {
    IoErrorKind::Uncategorised(_) => { println!("expected case"); }
    _ => { panic!("unexpected error occured"); }
}

If a point release of Rust categorises that error - lets say NotADirectory, then the following happens:

#[non_exhaustive]
enum UnstableCategory {
    Uncategorised,
    NotADirectory,
   ...
}

And the user side code doesn't break. The user can opt in to tighter matching if they wish, either with some build.rs that sets a feature based on the compiler version, or a MSRV.

match kind {
    IoErrorKind::Uncategorised(UnstableCategory::NotADirectory) => { println!("expected case"); }
    _ => { panic!("unexpected error occured"); }
}

How is this different to the status quo? All stable Kinds are outside the space matched on, so trying to do this at the IoErrorKind layer would fail: it would bring in all known kinds, and matching just the placeholder for uncategorised kinds fails if we try to categorise directly by moving things outside of that cat4egory.

joshtriplett commented 3 years ago

error is specifically X that we know, including some X that the stdlib hasn't yet categorised

Allowing code to match "something the standard library hasn't yet categorized" would set that code up to be broken when the standard library does start categorizing it in the future.

And it should be possible for existing uncategorised things to be categorised better over time.

That's exactly the problem: we need to make sure that existing code doesn't break when we add new categorization.

We did that by ensuring the only way to match "unknown error" is with a wildcard (_), so that when the standard library adds new categorization your code doesn't break.

We were faced with a three-way choice: never categorize any additional errors (not an option), or require people to change their code every single time we added a new categorization, or require people to change their code once if they're currently using ErrorKind::Other. And the documentation already warned against matching ErrorKind::Other in that way.

At most once per edition is easy to reason about; there are no strong commitments that nothing will be incompatible between editions. It might be nice to do it more often though.

In addition to the issue of doing it more often, we also can't tie this to an edition. io::ErrorKind can't be a different type in different editions, for a variety of reasons but in particular because one Rust program can include code from multiple editions.

Also, the change you're proposing of adding something like ErrorKind::Uncategorized is effectively what we did, and it would cause exactly the same issue with any code currently matching ErrorKind::Other.

Here's another way to look at the change:

rbtcollins commented 3 years ago

@joshtriplett

Allowing code to match "something the standard library hasn't yet categorized" would set that code up to be broken when the standard library does start categorizing it in the future.

I specifically proposed a mechanism that wouldn't break the code (within the same edition) if the stdlib started categorising it - but only sustainable for a relatively short period, because eventually the number of newly categorised errors will be large enough that people will want to match on the specific sub-variant.

re: needing the same errorkind across editions - ah, thats a constraint I hadn't internalised. If I can think of a way forward I'll suggest it, but thats a pretty hard obstacle.

Yes, I understand what has been done so far. It has removed a useful capability that existed before. I find it frustrating that most people seem to blithely assert that everything is Just Fine and that the new situation is better - only @Mark-Simulacrum has actually picked up on the impact so far.

joshtriplett commented 3 years ago

@rbtcollins I appreciate that the mechanism you're suggesting wouldn't necessarily break code when categorization was added (though people would have to change code to match if the categorization was moved to the top level alongside other variants). But the mechanism you're suggesting would break code when the mechanism itself was introduced, in exactly the same way that was already done. With the approach that was implemented, code that matched ErrorKind::Other has to match _ instead; with the approach you're suggesting, code that matched ErrorKind::Other would have to match ErrorKind::Uncategorised(_) instead. Either way, existing code that uses ErrorKind::Other would break. The approach we used was to allow the introduction of new variants in the future without any further breakage.

And to be clear, I'm not suggesting this is an ideal solution or that it has zero impact, just that it seemed like the best of the solutions we had available.

jplatte commented 2 years ago

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

PR has been merged for a while ;)

YJDoc2 commented 2 years ago

~Hey, I was wondering if these extra error kinds are added in std lib for stable or not?~

~I am using rustc 1.56.1 and trying to create a file in a directory (using fs::OpenOptions) gives me ReadOnlyFilesystem error,~ ~(which is expected as the fs is supposed to be readonly,) but if I try error.kind() == std::io::ErrorKind::ReadOnlyFilesystem~ ~I get compiler error use of unstable library feature.~

~Is there a way I can check if the error I get is of this specific kind without using nightly?~

~The way I understood rust release process I thought unstable features were nightly -> beta -> stabilized , but in this case it~ ~seems half of this is in stable and half is still unstable? Apologies if I misunderstood anything.~

~To elaborate, my current setup is~

if kind == ro_fs{
// do something
}else{
// do something else
}

~I am compiling with rustc v 1.56.1, for target x86_64-unknown-linux-gnu, with rustflags -C target-feature=+crt-static on Linux OS.~

Thanks :)

EDIT/UPDATE : on reading the discussion above once again, I figured out I can use the same way that TheBlueMatt has used and use raw_os_error, so that might work in my case. Thank you.

rbtcollins commented 2 years ago

@joshtriplett well, I'm finally getting time to do any rust, and the first thing is fixing up the still broken tests in remove_dir_all, and that means figuring this out. 6 months later and its still not usable on stable, so I'll need to use raw os errors indefinitely.

I still maintain that this is a stability break, no matter how desirable the feature. https://blog.rust-lang.org/2014/10/30/Stability.html

Code that worked, that was written with the API of the time, now doesn't work. We've pushed work onto our users.

We should update the stability guarantee I think - it was inspirational, but we're not living up to it.

/end whinge

ijackson commented 2 years ago

@rbtcollins I think you should use raw OS errors, yes.

I think this is an opportunity to plug my blog post about this issue where I go into some detail about what the problems are, why there aren't easy answers, and what to do if you're trying to fix things.

In #90706 I proposed to add a link to my blog post. That was felt to be inappropriate. Instead in #94273 a very abbreviated version of my recommendation was added. I still think this is a complicated and difficult issue and it is a shame that the official documentation still doesn't really reference a thorough explanation for people in @rbtcollins's situation, who are quite understandably frustrated.

rbtcollins commented 2 years ago

I wonder if adding to that prose a comment about raw errors would be useful?

I think there are three cases: One knows the error, and the version of rust at the time the code is written knows it: match on exactly that error.

One knows the error, and the version of rust at the time the code is written does not know it: use raw os errors and platform specific code to match on exactly that error.

One doesn't know the exact error: match on the inverted set of all the errors you know it cannot be (and if some of those are not known to rust, perhaps do so with raw os errors).

Xuanwo commented 2 years ago

Personally, I want this feature to get stabilized.

There is no way for a lib to construct an error like IsADirectory with error context in stable rust as described in https://github.com/datafuselabs/opendal/pull/221.

superatomic commented 2 years ago

I am running into some similar problems. It seems that std::fs::read_to_string can potentially return an Err<std::io::Error> with ErrorKind::IsADirectory as its .kind().

I can use a _ wildcard match to handle it and display a generic error (eg. eprintln!("{:?} IO Error", kind)), but even though doing that will print "IsADirectory IO Error" to stderr on stable, I can't actually match against it on stable.

archer884 commented 2 years ago

That this feature exists is becoming annoying in my everyday work, because my editor will suggest that I use (for instance) InvalidFilename and then turn around and tell me I'm not allowed to use InvalidFilename because the feature isn't stabilized. I rarely express the opinion that an unstable feature is somehow harmful but, in this case, I'm starting to feel it's time to "act" or get off the pot, so to speak.

memoryruins commented 2 years ago

@archer884 considering there are other unstable APIs in std, that seems more like an issue for editor plugins rather than motivation to stabilize or remove this nightly feature. Ideally completions would hint if something is unstable or hide them entirely (either on stable toolchain or configured to hide). There are open issues to improve this situation in rust-analyzer and intellij-rust, such as https://github.com/rust-lang/rust-analyzer/issues/3020 and https://github.com/intellij-rust/intellij-rust/issues/4439.

superatomic commented 2 years ago

@archer884 considering there are other unstable APIs in std, that seems more like an issue for editor plugins rather than motivation to stabilize or remove this nightly feature. Ideally completions would hint if something is unstable or hide them entirely (either on stable toolchain or configured to hide). There are open issues to improve this situation in rust-analyzer and intellij-rust, such as rust-lang/rust-analyzer#3020 and intellij-rust/intellij-rust#4439.

As I mentioned here, this affects my Rust code itself, independent of my IDE. Functions such as std::fs::read_to_string are able to return errors (on stable) that are a part of the io_error_more unstable API, but they can't be directly matched against (except with a wildcard match) because they are unstable. But I can call methods like .to_string(), which work on the unstable variants. (eg. err_object.kind().to_string() can output "is a directory", even though ErrorKind::IsADirectory is unstable)

Proof of concept using evcxr_repl:

>> // The root directory `/` is always not a file. This example may not work on Windows.
.. if let Err(e) = std::fs::read_to_string("/") {
..     println!("{}", e.kind().to_string()); // `e.kind()` will be a `std::io::ErrorKind::IsADirectory`
.. }
is a directory
()
>> std::io::ErrorKind::IsADirectory
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 
use of unstable library feature 'io_error_more'

I'm sure that you know way more about Rust then I do, but to me it seems weird that even if #![feature(io_error_more)] is not enabled it can still exist in a limbo state where it can't be referenced directly but can be used.

memoryruins commented 2 years ago

I'm sure that you know way more about Rust then I do, but to me it seems weird that even if #![feature(io_error_more)] is not enabled it can still exist in a limbo state where it can't be referenced directly but can be used.

@superatomic apologies if I wasn't clear. I was addressing a general (not unique to this feature) issue about editor suggestions of unstable parts in std, but I wasn't implying what should be done with #![feature(io_error_more)]. As you and others note, there are issues specific to this feature that motivate actions such as stabilization.

lucacasonato commented 2 years ago

I also want to vouch for stabilization as soon as possible. The existence of these unstable enum variants would be fine, if the creation of them was also "unstable only". Right now the unstable variants can be created in the internals of std in stable, without being able to create these same variants externally in stable. It makes mocking APIs, testing error handling and various other things very challenging.

We are going to need to implement some rather elaborate io::Error wrapping systems in the Deno project now to be able to manually create io::Error-likes for the unstable ErrorKind variants.

lucacasonato commented 2 years ago

Ooh, this is a fun workaround for creating unstable ErrorKinds, in stable: std::io::Error::from_raw_os_error(libc::EISDIR). Very hacky :D

Considering the ErrorKind can already be created (and matched via the stringified form) in stable, this may aswell be stable already. Updating the behavior here would likely break a considerable amount of existing applications which may unintentionally rely on these unstable error kinds already.

m-ou-se commented 2 years ago

It seems like most of the recent comments are only about IsADirectory. Since that makes clear it's a useful and wanted error kind, it might make sense to send a PR for stabilizing that one specifically. Then that doesn't have to be blocked on the other variants being stabilized.

The existence of these unstable enum variants would be fine, if the creation of them was also "unstable only".

Any uncategorized error is unstable in the same way, regardless of whether its ErrorKind is ErrorKind::Uncategorized or a specific unstable one like ErrorKind::IsADirectory. That we internally already (experimentally) split off IsADirectory from Uncategorized shouldn't make any difference for Rust programs, as the result is the same: An io::Error with an unmatchable ErrorKind. (Which is exactly the intention, since we haven't categorized them yet, no one should match on them, as that would break once we do put them in a stable category/kind.)

lucacasonato commented 2 years ago

It seems like most of the recent comments are only about IsADirectory. Since that makes clear it's a useful and wanted error kind, it might make sense to send a PR for stabilizing that one specifically. Then that doesn't have to be blocked on the other variants being stabilized.

That'd be great. We care mostly about IsADirectory, NotADirectory, and FilesystemLoop for our use cases right now.

superatomic commented 2 years ago

It seems like most of the recent comments are only about IsADirectory. Since that makes clear it's a useful and wanted error kind, it might make sense to send a PR for stabilizing that one specifically. Then that doesn't have to be blocked on the other variants being stabilized.

I support stabilizing IsADirectory, but I highly suggest that we stabilize NotADirectory at the same time as they are opposites of each other. (like @lucacasonato said)

It might also be worth stabilizing DirectoryNotEmpty as well, but it doesn't appear as important and is only partially related (it's the only other directory-related error, but it doesn't deal with the existence of directories).

superatomic commented 2 years ago

The existence of these unstable enum variants would be fine, if the creation of them was also "unstable only".

Any uncategorized error is unstable in the same way, regardless of whether its ErrorKind is ErrorKind::Uncategorized or a specific unstable one like ErrorKind::IsADirectory. That we internally already (experimentally) split off IsADirectory from Uncategorized shouldn't make any difference for Rust programs, as the result is the same: An io::Error with an unmatchable ErrorKind. (Which is exactly the intention, since we haven't categorized them yet, no one should match on them, as that would break once we do put them in a stable category/kind.)

Is there some way to prevent or warn about the use of ErrorKind::Other and ErrorKind::Uncategorized? Perhaps a Clippy lint like @ijackson was talking about might make sense.

Returning unstable variants is still not ideal even if we should treat them like ErrorKind::Other in stable code and just use wildcards. I think the biggest potential problem is that returning unstable varients implies that they are stable (i.e. matching ErrorKind::Other is bad form, but still possible). It might not be ideal to change which variant is returned depending on whether #![feature(io_error_more)] is used, but it might be worth only splitting errors off from ErrorKind::Uncategorized when io_error_more is enabled, instead of always.

m-ou-se commented 2 years ago

Is there some way to prevent or warn about the use of ErrorKind::Other and ErrorKind::Uncategorized?

Using Uncategorized already gives an error:

error[E0658]: use of unstable library feature 'io_error_uncategorized'
 --> src/main.rs:2:13
  |
2 |     std::io::ErrorKind::Uncategorized;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Or do you mean something else?

I think the biggest potential problem is that returning unstable varients implies that they are stable

I'm not sure I understand. ErrorKind is #[non_exhaustive], so you'll always have to take into account that it might have more variants than you can't match on. Whether some of those have an unstable/experimental name or not shouldn't make any difference.

change which variant is returned depending on whether #![feature(io_error_more)] is used

What would happen if an (indirect) dependency enables that feature, but your own crate does not? (Or the other way around?) Especially if that dependency ends up creating an Error or ErrorKind to your crate. An entire Rust program (which can be a combination of many crates) uses a single copy of std, so we can't make std behave differently per crate.

Xuanwo commented 2 years ago

I prefer to stabilize the following error kind:

The reasons are listed as follows:

They are normal

Any call to remove_dir or create_dir could trigger them unless we check metadata every time we try to call std::fs functions.

They are actionable

Developers have a strong need to check and match them.

Think about we are developing command-line tools. We may want to output friendly messages if users' input path is a directory.

References:

m-ou-se commented 2 years ago

The only thing I want to know before stabilizing any of them, is if they make sense on all/most platforms. E.g. ideally, trying to remove a non-empty directory would result in the same DirectoryNotEmpty error kind on all platforms.

This doesn't necessarily have to be a strict requirement. There might be cases where one OS simply does not have errors that are specific enough. But in those cases we should be extra careful, and wonder if we're making our error kinds too specific.

Xuanwo commented 2 years ago

The only thing I want to know before stabilizing any of them, is if they make sense on all/most platforms. E.g. ideally, trying to remove a non-empty directory would result in the same DirectoryNotEmpty error kind on all platforms.

Nice catch. The three errors are obvious on posix platforms like linux, darwin, and bsd.

Windows also have some compatibility with the UNIX family of operating systems, which is the subset of errno.h.

References: https://docs.microsoft.com/en-us/cpp/c-runtime-library/errno-constants?view=msvc-170

superatomic commented 2 years ago

Is there some way to prevent or warn about the use of ErrorKind::Other and ErrorKind::Uncategorized?

Using Uncategorized already gives an error:

error[E0658]: use of unstable library feature 'io_error_uncategorized'
 --> src/main.rs:2:13
  |
2 |     std::io::ErrorKind::Uncategorized;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Or do you mean something else?

Oops, I didn't check to see if it already issued a warning. ErrorKind::Uncategorized is not a problem.

Should ErrorKind::Other behave in the same way, though? Currently, Rust doesn't warn about using it, even when it used in a pattern match.

I think the biggest potential problem is that returning unstable variants implies that they are stable

I'm not sure I understand. ErrorKind is #[non_exhaustive], so you'll always have to take into account that it might have more variants than you can't match on. Whether some of those have an unstable/experimental name or not shouldn't make any difference.

Sorry, I should have been clearer about what I was talking about. If a programmer understands that the unstable ErrorKind enum variants should be treated as ErrorKind::Other and only matched via wildcards, there isn't a problem.

However, because these variants are given specific names and descriptions, and return based on certain contexts, it gives off the appearance of being usable.

Basically, it's not immediately obvious that the unstable variants should be ignored and simply wildcard matched.

change which variant is returned depending on whether #![feature(io_error_more)] is used

What would happen if an (indirect) dependency enables that feature, but your own crate does not? (Or the other way around?) Especially if that dependency ends up creating an Error or ErrorKind to your crate. An entire Rust program (which can be a combination of many crates) uses a single copy of std, so we can't make std behave differently per crate.

I didn't know that std worked like that. I agree, it shouldn't return different variants. Sorry!

ClementNerma commented 2 years ago

This issue is really starting to bug me as it breaks some existing code and, as others have said, prevents from fixing it.

I have a specific scenario where I need to move a file to a different filesystem, the only reliable cross-platform way I've found to do it is to fs::rename and check the resulting error.

Now this doesn't work because the ErrorKind:CrossesDevice variant has been introduced. I don't like silent breakages like this one but I understand how they are necessary.

The problem I have is that the code can't be fixed, I now need to check for this specific error variant but I can't because the feature is unstable and won't compile by definition on stable. So my program doesn't work anymore and I don't have any reliable way to make it work without using some huge hacks that may completely break in the future.

I don't know what solution could be brought - I thought about adding a function that would convert the new variants to the old one, but it would of course be unstable for a bit of time so that wouldn't solve the problem at all.

Do you have any idea of improvements to solve this problem?

Xuanwo commented 2 years ago

First of all, as advised by @m-ou-se, we are on the way to stabilizing part of io_error_more instead, like NotADirectory and DirectoryNotEmpty. This can prevent us kept blocking without any progress. And we will not stabilize ErrorKind:CrossesDevice for now. (Please correct me if I understand wrongly).

Then, for this problem:

I need to move a file to a different filesystem, the only reliable cross-platform way I've found to do it is to fs::rename and check the resulting error.

Which variant do you check before?

As described in docs,

Errors from the standard library that do not fall under any of the I/O error kinds cannot be matched on, and will only match a wildcard (_) pattern. New ErrorKinds might be added in the future for some of those.

To me, the clearest way is falling back to copy&delete (just like mv does) while meeting errors except NotFound, PermissionDenied, and others already known (and stable) error variants.

Currently, Rust doesn't warn about using it, even when it used in a pattern match.

Maybe it's worth adding a Clippy rule for matching ErrorKind::Other.

Proposed new lint at https://github.com/rust-lang/rust-clippy/issues/9004

ClementNerma commented 2 years ago

Nice answer, thank you.

May I ask why ErrorKind isn't marked as #[non_exhaustive] given new variants can be added anytime and ErrorKind::Other shouldn't be matched against?

Xuanwo commented 2 years ago

May I ask why ErrorKind isn't marked as #[non_exhaustive] given new variants can be added anytime and ErrorKind::Other shouldn't be matched against?

ErrorKind does marked as #[non_exhaustive]:

image

Reference: https://doc.rust-lang.org/std/io/enum.ErrorKind.html

ClementNerma commented 2 years ago

Oh yes sorry, I confused #[non_exhaustive] and having a non-matchable variant. Nevermind.

ijackson commented 2 years ago

This doesn't necessarily have to be a strict requirement. There might be cases where one OS simply does not have errors that are specific enough. But in those cases we should be extra careful, and wonder if we're making our error kinds too specific.

Highly relevant to this concern is the discussion starting here: https://github.com/rust-lang/rust/pull/79965#issuecomment-832771472

In particular, I would like to (re)propose the principles I set out here https://github.com/rust-lang/rust/pull/79965#issuecomment-833422225

The only thing I want to know before stabilizing any of them, is if they make sense on all/most platforms. E.g. ideally, trying to remove a non-empty directory would result in the same DirectoryNotEmpty error kind on all platforms.

I think what you mean is whether there are platforms where you can try to remove a non-empty directory, and get an error because it was non-empty, and not be able to tell that that was the reason ?

I think if there are any such operating systems then they ought to either not implement these parts of std, or ought to be at a low support tier where it is expected that Rust programmers might have to do platform-specific work.

The alternative is to say that all Rust programmers even on popular and sensible operating systems must apply platform-specific workarounds (involving raw OS error numbers), or write unnatural code, or be exposed to toctou races.