Add conversions from io:ErrorKind to io::Error

[Mark-Simulacrum]

Filing to help with discussion around the possibility of doing this.

Current changes are clearly backwards incompatible, but I think adding a new function (with a bikeshed on naming) like Error::new_str should be possible (or some other way of specializing the string error message case) to fix #36658.

[rust-highfive]

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

[sfackler]

io::Error's layout was designed to mimimize its size since it is so frequently passed around. I would be somewhat worried about blowing it up from 16 bytes to 40. If the message is cut out we should be okay size wise though.

[Mark-Simulacrum]

Cutting the message out would make us lose information, though, which is why it seems best not to do that; though I'm not sure how often the messages carry meaningful information with them not already included in the ErrorKind.

[sfackler]

I think I'd personally want to hear a very compelling story around why it's crucial to have custom messages and not allocate at the same time before blowing up io::Error's size. The use case described in the related issue wants to avoid allocation because the errors are being generated very frequently, in which case a user isn't going to be looking at the message almost by definition.

[Mark-Simulacrum]

I'll push code sometime soon removing the Cow from the non-allocating repr variant; I agree that it's unlikely an accompanying message would be useful for the case this is intended to be used for.

---

On another note, can someone explain/help me out to see why Travis is failing and how to fix it? I didn't change anything inside Vec, but I see "std/vec/struct.Vec.html:2447: id is not unique: src-3909" in the Travis log during stage2 linkcheck before it fails.

[sfackler]

Dunno what's going on with the travis errors. Seems like it's probably a false alarm?

[Mark-Simulacrum]

I've pushed (force) a couple times already, and it's repeated itself a couple times (same error), so I feel like it might not be.... though I agree it feels spurious.

[bluss]

No reason to have a String or Cow in a non-heap variant. Needs a different solution, for example there is room for an application-specific error identifier (number) in addition to ErrorKind, but it's not going to be easy to use.

[alexcrichton]

I agree with @sfackler that the representation here was chosen pretty precisely to ensure an efficient runtime representation, and we'd likely want some strong numbers to change it much.

In the current iteration, though, I'm not sure I understand what the benefit is over just from_raw_os_error perhaps? That way it seems like you at least get a more descriptive message?

[Mark-Simulacrum]

from_raw_os_error doesn't allow you to specify an error kind directly, which I think is useful.

Adding a u64 or another integer/number type to the new variant will also work (no increase in size); but I'm not sure how useful that is, since it will be library specific and difficult to differentiate between (library A and B could use the same error kind and attached integer for different meanings, which would get confusing).

[alexcrichton]

@rfcbot fcp close

With the particulars around the representation chosen here I personally feel that just being able to construct from a kind isn't quite strong enough motivation to add this here. I could perhaps be persuaded with numbers though!

[rfcbot]

Team member @alexcrichton has proposed to close this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, 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.

[Mark-Simulacrum]

@alexcrichton The issue (#36658) which spurred this PR suggests that heap-allocating errors is significant. Can you document your thoughts on how crates/libstd should be creating stack-allocated errors for the common case (e.g. ErrorKind::WouldBlock)?

Is the intent that crates would use the Error::from_raw_os_error(i32) method? I find that a poor suggestion, since it implies being platform-specific, which AFAIK is something Rust tries to avoid.

[aturon]

I was going to suggest that we could provide a conversion from ErrorKind to raw OS errors, but unfortunately, not every OS can represent every error kind.

However, perhaps we could offer a conversion from ErrorKind to Error, which goes via Os when possible, and falls back to Custom otherwise? I think that would achieve basically the aims of this PR, without any representation changes. The main downside is the fact that it'd sometimes hide an allocation (but only for more obscure errors). If that turns out to be a problem in practice, though, we could always introduce an additional variant later.

[alexcrichton]

@Mark-Simulacrum Unfortunately we still don't have numbers one way or another (beyond microbenchmarks), so the perf wins are still somewhat heresay. Crates can always create a non-allocating version like the standard library does, which is to use from_raw_os_error, yeah.

@aturon that's essentially what this PR does, adding Error::new_kind(kind: ErrorKind). I've always felt like we never really expanded ErrorKind as much as we could, so it's not quite ready for a prominent placement like that. Falling back to what the standard library does, using OS errors, seems reasonable in the interim.

[aturon]

@alexcrichton Right, but I'm saying we could do it without adding another variant. In your comment you mentioned "the particulars of the representation" so I was thinking you were concerned about the representation change.

I also disagree about the prominence of ErrorKind. It's how you figure out anything about errors in a platform-generic way today, and I don't see adding a constructor as any more of a commitment than our current APIs.

[alexcrichton]

Oh we can add new variants so long as they don't inflate the size of the payload. The current PR would suffice for that. I erroneously remembered this as having a new Simple(&'static str) which would inflate the size.

[aturon]

@alexcrichton OK -- so can you elaborate a bit more on your concern here? Having a platform-agnostic, lightweight way to go from an ErrorKind to an Error seems like good functionality to me.

[sfackler]

I'm moderately in favor of adding this as well. The extra variant shouldn't cause any size blowup and I can see the allocation cost mattering in certain rare-ish situations.

[alexcrichton]

I would find it a little odd that we can construct an error with so little information. An error created with only an ErrorKind is virtually useless to ever present to anyone, and only really serves a purpose of some downstream match statement looking at the kind. I suppose if the errors are basically guaranteed to be inspected in that fashion that's ok, so perhaps this is an ok addition?

[Mark-Simulacrum]

I believe the point is that some IO Errors (and ErrorKinds) are usually not intended for presentation to the user; e.g. WouldBlock seems to be mainly a program-internal error message, not something the user would care about.

I can add to the documentation comment (did I write one?) to include that this is intended for library/application-internal error construction that will not be exposed to the end user.

[alexcrichton]

Ok, given that information, let's go the other way instead:

@rfc fcp cancel

[alexcrichton]

@rfcbot fcp cancel

[rfcbot]

@alexcrichton proposal cancelled.

[alexcrichton]

@rfcbot fcp merge

[Mark-Simulacrum]

Just so it's clear, this shouldn't be merged yet--documentation needs to be added. I don't really want to type it out prior to a decision wrt to merging, though.

[sfackler]

Can we land this thing? This PR has been open for 23 days. It really doesn't seem necessary to wait around for another week.

[rfcbot]

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

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, 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.

[Mark-Simulacrum]

Two things that would need to be discussed:

• A name (with_kind?)
• How should we document the intent for this to be used only for library-internal errors?

[alexcrichton]

@sfackler remember that "FCP" for PRs is insta-merge, no need to wait for a week (just need to wait for eyes).

I personally feel this should be called new_kind and then we'd also want to update the docs before landing, yes. We'd just call out explicitly how it's intended for internal errors that are inspected later.

[sfackler]

At least judging from every other T-libs PR, it'll take at least a week to get everyone to check their boxes.

new_kind seems fine, or from_kind.

[alexcrichton]

I'd actually also be fine with just:

impl From<ErrorKind> for Error {
    // ...
}

[aturon]

👍 for From<ErrorKind>.

[brson]

From conversion makes sense to me.

[sfackler]

SGTM

[Mark-Simulacrum]

I'm not sure if this is the right tag here.

[Mark-Simulacrum]

Implemented the From<ErrorKind> for Error as suggested. Ready for another review.

I'm not sure if the documentation comment is sufficiently emphatic about not exposing this "raw" container to users, please let me know if you have better text for it.

[alexcrichton]

Looks good to me!

[rfcbot]

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

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

[alexcrichton]

@bors: r+

[bors]

📌 Commit 99234bb has been approved by alexcrichton

[bors]

⌛ Testing commit 99234bb with merge d2bc30b...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: alexcrichton
Pushing d2bc30b to master...

[carllerche]

Thanks for making this happen @Mark-Simulacrum