std: Stabilize last bits of io::Error

[alexcrichton]

This commit stabilizes a few remaining bits of the io::Error type:

• The Error::new method is now stable. The last detail parameter was removed
  and the second desc parameter was generalized to E: Into<Box<Error>> to
  allow creating an I/O error from any form of error. Currently there is no form
  of downcasting, but this will be added in time.
• An implementation of From<&str> for Box<Error> was added to liballoc to
  allow construction of errors from raw strings.
• The Error::raw_os_error method was stabilized as-is.
• Trait impls for Clone, Eq, and PartialEq were removed from Error as it
  is not possible to use them with trait objects.

This is a breaking change due to the modification of the new method as well as
the removal of the trait implementations for the Error type.

[breaking-change]

[rust-highfive]

r? @pcwalton

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

[alexcrichton]

r? @aturon
cc @sfackler

[aturon]

cc @seanmonstar

[sfackler]

👍

[aturon]

think you wanted stable here

[alexcrichton]

So I do!

[alexcrichton]

Ah this is also a breaking change as io::Error is no longer Sync.

[aturon]

r=me modulo nits

[alexcrichton]

@bors: r=aturon ac77392

[seanmonstar]

\o/

[lilyball]

Trait impls for Clone, Eq, and PartialEq were removed from Error as it
is not possible to use them with trait objects.

This is rather unfortunate. I was relying on Clone in my API because I needed my API to return the same error every time once an error happens once (it writes structured output, and a failure to write any part of it invalidates the structure, so I can't let clients just ignore the error and keep writing). My solution was to hold onto the error and return a .clone() of it for all API calls, but I can't do that anymore.

The only solution I can see is to change my API to start returning Result<(),Rc<io::Error>> but that's kind of awkward. Alternatively I can just give up on always returning an error from all subsequent calls and just hope clients don't try and continue writing after an error occurs, but I don't want to do that if I don't have to.

Is there no way that io::Error can require the Clone bound on the custom Error values? Perhaps with a trait CloneableError: Error + Clone {}? Alternatively, it could use an Rc instead of a Box for holding the custom error. This doesn't recover the Eq and PartialEq bounds but I'm fine with that (comparing errors for equality doesn't seem very useful, you generally want to compare the error kinds instead).

[alexcrichton]

This is rather unfortunate.

Yeah I agree that this had more fallout than I initially realized. There may be a few possible routes for your API, however. In general I'd recommend avoiding Rc<T> as it limits the Send/Sync usefulness of the API and also is somewhat less idiomatic.

• If you're the one generating the io::Error being returned (e.g. it didn't originate elsewhere), then you could store a factory to create more when needed.
• You could store the ErrorKind + description and make sure that it always returns the same ErrorKind on future calls (but it would be a different underlying object).
• You could store the original error itself and just hand out copies of the kind/description instead of only storing the kind/description.

Unfortunately cloning Box<Error> would require a separate trait which didn't return Self (which isn't object-safe), but instead returns Box<Error>. Otherwise we would have definitely added Clone as a supertrait to Error!

[lilyball]

@alexcrichton

Unfortunately cloning Box<Error> would require a separate trait which didn't return Self (which isn't object-safe)

Ah right, I forgot about that. Why do we have that limitation? I get why a trait that uses Self for any argument would not work, but it seems like it should be feasible to support methods that merely return Self by having them return Box<Trait> (see below).

In general I'd recommend avoiding Rc<T> as it limits the Send/Sync usefulness of the API and also is somewhat less idiomatic.

It could use Arc then.

At the moment I'm thinking the best solution is actually just to make io::Error use Arc<Error + Send + Sync> instead of Box<Error + Send>. This would allow Clone to work again, without requiring all the clients of io::Error to have to change their API if they need cloneability. I think it's reasonable to assume that most io::Error values are going to be one of the built-in kinds and thus won't incur any overhead from the reference-counting, and when custom errors are involved, error handling is not generally a hot path anyway so the overhead shouldn't be noticeable.

As for your other suggestions, I'm not generating the io::Error, I'm piping it through from my underlying Write (my type is a wrapper around a Write that writes out structured data; specifically, a particular XML format). I could try and copy out the ErrorKind + description, but beyond being awkward, that would be completely unable to handle custom errors. I could fall back to using my own custom error as a placeholder if the error was a custom error (after returning the original error the first time), but that's not very nice.

---

Some tangential musing about the returning-Self limitation:

I would think this could be supported for functions that return Self (but don't take Self for any argument). A wrapper function would be needed that calls the original function and wraps up the return value in a Box<Trait>, but that should be easy to generate at the same time as the trait dispatch table is generated. This should even work for things like Box<Trait+Send> by merely coercing the type from Box<Trait> as the in-memory representation of Box<Trait> and Box<Trait+Send> is presumably identical (since right now only marker traits (which have no methods) can be used as additional bounds on a trait object) and therefore the value can just be coerced.

Although in the future if we do support Box<Trait+OtherTrait> even that could be made to work, perhaps by having the wrapper function harvest the trait dispatch table pointer from the original trait object (assuming Rust doesn't just simply generate a combined trait dispatch table for that combination of traits, which would mean it could just generate a new wrapper that returns Box<Trait+OtherTrait> directly; harvesting the trait dispatch table pointer would only be necessary if it re-uses the existing trait dispatch table (which would generate a Box<Trait> instead of a Box<Trait+OtherTrait>).

All of this assumes, of course, that we continue to not have "real" subtyping. If we did, this wouldn't work as a a Box<Trait> that holds a subtype value would not necessarily have the same trait dispatch table as a newly-created Self value of the base type (e.g. if the base type is what implements the trait), unless we had a restriction that any Self-returning methods on a base type must return the same concrete type it was called on (e.g. the subtype if it was called on a subtype value). But if we ever come to that, we could just restrict subtype-able types from being turned into trait objects if the trait has a Self-returning method (and relying on some sort of final declaration to disallow subtyping on otherwise-subtypeable types that want to work like this).

[lilyball]

I came up with a workaround, which is to create my own type that contains an Arc<io::Error> and uses that to implement the Error, Display, and Debug traits such that it's indistinguishable from the original. But this fails because io::Error is not Sync (by virtue of its Box<Error+Send> not being Sync) so the resulting Arc<io::Error> is not Send, which means I can't pass the whole shebang back into io::Error::new(). Filed as #24049.

[alexcrichton]

I find the use case of putting io::Error in an Arc in one form or another pretty compelling, so it may be best to continue discussion on #24049 as it seems like one of the better methods forward!