Add downcasting to std::error::Error #24793

aturon · 2015-04-24T23:16:29Z

This commit brings the Error trait in line with the Error interoperation
RFC by adding downcasting,
which has long been intended. This change means that for any Error
trait objects that are 'static, you can downcast to concrete error
types.

To make this work, it is necessary for Error to inherit from
Reflect (which is currently used to mark concrete types as "permitted
for reflection, aka downcasting"). This is a breaking change: it means
that impls like

impl<T> Error for MyErrorType<T> { ... }

must change to

impl<T: Reflect> Error for MyErrorType<T> { ... }

This commit furthermore marks Reflect as stable, since we are already
essentially committed to it via Any. Note that in the future, if we
determine that the parametricity aspects of Reflect are not needed, we
can deprecate the trait and provide a blanket implementation for it
for all types (rather than by using OIBIT), which would allow all
mentions of Reflect to be dropped over time. So there is not a strong
commitment here.

[breaking-change]

r? @alexcrichton

alexcrichton · 2015-04-25T00:08:09Z

@bors: r+ e912b2a p=1

Nominating for a cherry-pick into beta as well

bors · 2015-04-25T02:45:22Z

⌛ Testing commit e912b2a with merge 0ffa676...

bors · 2015-04-25T03:43:30Z

💔 Test failed - auto-linux-64-opt

reem · 2015-04-25T05:28:05Z

@aturon is it really necessary to mark Reflect stable here? One could just as easily use Any there.

EDIT: Note that the deprecation path you describe wouldn't be forwards compatible, since downstream users could add negative implementations of Reflect to their own types if we expose it.

reem · 2015-04-25T05:28:58Z

src/libstd/error.rs

+    #[unstable(feature = "core",
+               reason = "unclear whether to commit to this public implementation detail")]
+    fn type_id(&self) -> TypeId where Self: 'static {
+        TypeId::of::<&'static Self>()


why not TypeId::of::<Self>? TypeId supports T: ?Sized

reem · 2015-04-25T05:30:21Z

Should we also add by-value downcasting from Box<Error> to Box<T>?

aturon · 2015-04-25T06:12:41Z

@reem The need for Reflect stabilization has to do with the fact that Any requires 'static, which is a constraint we do not wish to impose on Error. (It makes good sense for Any, since the trait is only usable with 'static values anyway, but Error of course has more uses than downcasting.) So the two are not equivalent.

re: forward compatibility of the deprecation path -- good point. This will require blanket impls and OIBIT to coexist. But that seems plausible (cc @nikomatsakis).

I suppose the main alternative to this change, which we discussed on IRC, would be to wait until upcasting it working and then ask people to use e.g. Box<Error + Any> when they want downcasting, rather than baking it in to the error type. As we discussed, this increases flexibility, but makes it much easier to lock APIs out of the ability to downcast, and then be stuck. Since downcasting is a fairly central part of Error, I'm still inclined toward this change.

FYI: I also updated with your other suggestions. Thanks!

@alexcrichton @nikomatsakis thoughts?

reem · 2015-04-25T06:23:28Z

src/libstd/error.rs

+    #[doc(hidden)]
+    #[unstable(feature = "core",
+               reason = "unclear whether to commit to this public implementation detail")]
+    fn type_id(&self) -> TypeId where Self: 'static {


If you move the Reflect bound to a method-level where clause here, then we can use exclusively Any and avoid stabilizing Reflect.

Hmm, actually this will prevent downcasting with Box<Error>, rather than Box<Error + Reflect>, which iirc isn't even allowed.

Drat, it was an interesting idea!

Thought about this some more, and we can get away without stabilizing Reflect with just a bit less flexibility - instead of implementing Error for T: Reflect in e.g. TryLockError, just implement for T: Any. It's slightly less flexible, but it's backwards compatible to generalize the impls when/if we do stabilize Reflect directly.

@reem Ah yes, I think at some point in the iterations on this topic I confused myself into thinking this would hurt more than it does: you can still have non-'static generic error types, they will just fail to be Error for the time being. That seems plausible, and I agree that not stabilizing Reflect is a good idea.

pnkfelix · 2015-04-26T09:57:42Z

triage: beta-nominated

Because @alexcrichton said so above. (Not r+'ing though since @aturon is awaiting feedback )

glaebhoerl · 2015-04-27T18:00:47Z

@aturon Back in the RFC thread you appeared to agree with me that dynamically typed error-downcasting is an "anti-pattern" (one should use enums). Has something changed?

alexcrichton · 2015-04-27T23:35:40Z

@aturon I think after some more discussion we agreed on not stabilizing Reflect, leaving it as a super trait, and just using Any as a bound everywhere, right? I would personally prefer to avoid Error + Any in trait objects as if it's a pillar of Error that you can do downcasting then it'd be unfortunate to forget the Any.

@glaebhoerl

The addition of downcasting here is one of the pillars of io::Error as it's the bread-and-butter error type for all of I/O (e.g. it's on io::{Write, Read} etc). As it is such a central error type it needs the ability to carry custom payloads to be applicable for everyone using the I/O traits. As a result in this case we're adding downcasting.

reem · 2015-04-27T23:39:02Z

There's also the matter that you can't actually do Error + Any. Any is kind
of useless except as a bound (because of Reflect) or a trait to use when
you want only downcasting (from any type).
On Mon, Apr 27, 2015 at 7:35 PM Alex Crichton notifications@github.com
wrote:

@aturon https://github.com/aturon I think after some more discussion we
agreed on not stabilizing Reflect, leaving it as a super trait, and just
using Any as a bound everywhere, right? I would personally prefer to
avoid Error + Any in trait objects as if it's a pillar of Error that you

can do downcasting then it'd be unfortunate to forget the Any.

@glaebhoerl https://github.com/glaebhoerl

The addition of downcasting here is one of the pillars of io::Error as
it's the bread-and-butter error type for all of I/O (e.g. it's on io::{Write,
Read} etc). As it is such a central error type it needs the ability to
carry custom payloads to be applicable for everyone using the I/O
traits. As a result in this case we're adding downcasting.

—
Reply to this email directly or view it on GitHub
#24793 (comment).

aturon · 2015-04-28T17:55:56Z

@glaebhoerl Continuing along the lines of what @alexcrichton said: it's still the case that enums are the recommended, conventional way to provide concrete error types, and should be preferred to downcasting when practical.

That said, there are places where downcasting is appropriate. One important example that @alexcrichton is the io::Error type. During IO reform, we initially proposed for various traits like Read/Write have an associated error type, but in practice we found this to come at a massive cost in ergonomics, and instead reverted to a single concrete error type. But it's still sometimes desirable to pass a custom error through the traits. To enable this, the error type provides a "custom" variant that is a boxed Error, and is intended for downcasting.

Now, I've long maintained that such use cases are important, but why build downcasting into the Error trait itself? In general, we often have to make tough decisions about traits that are primarily used for trait objects (rather than generic bounds): that's a one-way abstraction, so if one API gives you a Box<Error> but downcasting was separate (say, you'd have to have Box<ReflectError>), then you're stuck -- you would not be able to downcast, and it would be a breaking change to alter the type. Similar problems arise with marker traits (Box<Error + Send>). So in these cases, we have to think carefully about the anticipated uses of trait objects, and try to make a good decision about the defaults, to decrease the likelihood of painting your APIs into a corner.

With all of that said, one of the primary use cases of Error for trait objects is indeed downcasting. (The other being extracting textual messages/cause chains.) Baking it in this way ensures that all uses of the Error trait are ready to handle this use case. This doesn't impose a burden on implementors of the trait, but it might indeed suggest that downcasting is more "blessed" than we mean it to be. We'll have to counter this with strong conventions documentation.

pnkfelix · 2015-04-30T21:16:58Z

triage: beta-accepted

glaebhoerl · 2015-04-30T21:58:26Z

@aturon Thanks for explaining! An associated error type was indeed the alternative solution I had in mind, but if that was tried and proved untenable, then I guess that's that. I'm mostly concerned because dynamically downcasting exception types happens to be the idiomatic error handling solution in a large number of very popular languages, and it would be very easy to see that become the done thing in Rust as well if we're not watchful. I guess we'll have to make sure the documentation is clear about this. :)

aturon · 2015-04-30T22:03:07Z

@alexcrichton @reem Sorry for the delay here, been in intense meeting mode this week.

I've pushed an update which leaves Reflect unstable, and customizes the error message to suggest bounding by Any. re-r?

reem · 2015-04-30T22:04:53Z

src/libcore/any.rs

@@ -222,7 +220,7 @@ impl TypeId {
    /// Returns the `TypeId` of the type this generic function has been
    /// instantiated with
    #[stable(feature = "rust1", since = "1.0.0")]
-    pub fn of<T: ?Sized + Any>() -> TypeId {
+    pub fn of<T: ?Sized + Reflect + 'static>() -> TypeId {


Better to just use Any here, the docs will be clearer.

You have to use Reflect here to allow Error to implement downcasting without inheriting from Any.

alexcrichton · 2015-04-30T22:11:50Z

r=me with a few final nits, thanks @aturon!

reem · 2015-04-30T22:17:33Z

src/libstd/error.rs

+
+#[unstable(feature = "error_downcast", reason = "recently added")]
+/// An extension trait for downcasting from boxed Errors
+pub trait BoxErrorExt {


Can't we have a method on Error which takes self: Box?

Ah, we can indeed! I sometimes forget we still have that special treatment of Box.

This commit brings the `Error` trait in line with the [Error interoperation RFC](rust-lang/rfcs#201) by adding downcasting, which has long been intended. This change means that for any `Error` trait objects that are `'static`, you can downcast to concrete error types. To make this work, it is necessary for `Error` to inherit from `Reflect` (which is currently used to mark concrete types as "permitted for reflection, aka downcasting"). This is a breaking change: it means that impls like ```rust impl<T> Error for MyErrorType<T> { ... } ``` must change to something like ```rust impl<T: Reflect> Error for MyErrorType<T> { ... } ``` except that `Reflect` is currently unstable (and should remain so for the time being). For now, code can instead bound by `Any`: ```rust impl<T: Any> Error for MyErrorType<T> { ... } ``` which *is* stable and has `Reflect` as a super trait. The downside is that this imposes a `'static` constraint, but that only constrains *when* `Error` is implemented -- it does not actually constrain the types that can implement `Error`. [breaking-change]

aturon · 2015-05-01T01:21:17Z

@reem: FYI, I've incorporated most of your suggestions, except for those that are not possible.

aturon · 2015-05-01T01:21:37Z

@bors: r=alexcrichton p=10

Prioritizing due to breakage.

bors · 2015-05-01T01:21:38Z

📌 Commit a576262 has been approved by alexcrichton

bors · 2015-05-01T04:22:54Z

⌛ Testing commit a576262 with merge 5c710b5...

@alexcrichton

This commit brings the `Error` trait in line with the [Error interoperation RFC](rust-lang/rfcs#201) by adding downcasting, which has long been intended. This change means that for any `Error` trait objects that are `'static`, you can downcast to concrete error types. To make this work, it is necessary for `Error` to inherit from `Reflect` (which is currently used to mark concrete types as "permitted for reflection, aka downcasting"). This is a breaking change: it means that impls like ```rust impl<T> Error for MyErrorType<T> { ... } ``` must change to ```rust impl<T: Reflect> Error for MyErrorType<T> { ... } ``` This commit furthermore marks `Reflect` as stable, since we are already essentially committed to it via `Any`. Note that in the future, if we determine that the parametricity aspects of `Reflect` are not needed, we can deprecate the trait and provide a blanket implementation for it for *all* types (rather than by using OIBIT), which would allow all mentions of `Reflect` to be dropped over time. So there is not a strong commitment here. [breaking-change] r? @alexcrichton

bors · 2015-05-01T07:05:26Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

rust-highfive assigned alexcrichton Apr 24, 2015

reem reviewed Apr 25, 2015
View reviewed changes

aturon force-pushed the io-error-any branch from e912b2a to 75d173f Compare April 25, 2015 05:59

reem reviewed Apr 25, 2015
View reviewed changes

rust-highfive added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 26, 2015

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 30, 2015

aturon force-pushed the io-error-any branch 2 times, most recently from a010bf6 to 6436123 Compare April 30, 2015 22:01

reem reviewed Apr 30, 2015
View reviewed changes

aturon force-pushed the io-error-any branch from 6436123 to 7a8ece2 Compare May 1, 2015 01:18

aturon force-pushed the io-error-any branch from 7a8ece2 to a576262 Compare May 1, 2015 01:20

bors merged commit a576262 into rust-lang:master May 1, 2015

alexcrichton mentioned this pull request May 5, 2015

Cherry-pick std::error downcasting to beta #25121

Merged

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 5, 2015

SimonSapin mentioned this pull request Feb 10, 2019

Stabilize std::error::Error::type_id #58048

Merged

SimonSapin mentioned this pull request May 17, 2019

Tracking issue for stabilizing Error::type_id #60784

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add downcasting to std::error::Error #24793

Add downcasting to std::error::Error #24793

aturon commented Apr 24, 2015

alexcrichton commented Apr 25, 2015

bors commented Apr 25, 2015

bors commented Apr 25, 2015

reem commented Apr 25, 2015

reem Apr 25, 2015

reem commented Apr 25, 2015

aturon commented Apr 25, 2015

reem Apr 25, 2015

reem Apr 25, 2015

aturon Apr 25, 2015

reem Apr 26, 2015

aturon Apr 27, 2015

pnkfelix commented Apr 26, 2015

glaebhoerl commented Apr 27, 2015

alexcrichton commented Apr 27, 2015

reem commented Apr 27, 2015

can do downcasting then it'd be unfortunate to forget the Any.

aturon commented Apr 28, 2015

pnkfelix commented Apr 30, 2015

glaebhoerl commented Apr 30, 2015

aturon commented Apr 30, 2015

reem Apr 30, 2015

aturon May 1, 2015

alexcrichton commented Apr 30, 2015

reem Apr 30, 2015

aturon May 1, 2015

aturon commented May 1, 2015

aturon commented May 1, 2015

bors commented May 1, 2015

bors commented May 1, 2015

bors commented May 1, 2015

Add downcasting to std::error::Error #24793

Add downcasting to std::error::Error #24793

Conversation

aturon commented Apr 24, 2015

alexcrichton commented Apr 25, 2015

bors commented Apr 25, 2015

bors commented Apr 25, 2015

reem commented Apr 25, 2015

Choose a reason for hiding this comment

reem commented Apr 25, 2015

aturon commented Apr 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Apr 26, 2015

glaebhoerl commented Apr 27, 2015

alexcrichton commented Apr 27, 2015

reem commented Apr 27, 2015

can do downcasting then it'd be unfortunate to forget the Any.

aturon commented Apr 28, 2015

pnkfelix commented Apr 30, 2015

glaebhoerl commented Apr 30, 2015

aturon commented Apr 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Apr 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aturon commented May 1, 2015

aturon commented May 1, 2015

bors commented May 1, 2015

bors commented May 1, 2015

bors commented May 1, 2015