Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement error interoperation (RFC 70) #17753

Merged
merged 3 commits into from
Nov 3, 2014
Merged

Conversation

aturon
Copy link
Member

@aturon aturon commented Oct 3, 2014

This PR:

  • Adds the error interoperation traits (Error and FromError) to a new module, std::error, as per RFC 70. Note that this module must live in std in order to refer to String.

    Note that, until multidispatch lands, the FromError trait cannot be
    usefully implemented outside of the blanket impl given here.

  • Incorporates std::error::FromError into the try! macro.

  • Implements Error for most existing error enumerations.

Closes #17747

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@aturon aturon force-pushed the error-interop branch 2 times, most recently from 9b13971 to 4f22561 Compare October 3, 2014 21:47
//! pub trait Error: Send + Any {
//! fn description(&self) -> &str;
//!
//! fn detail(&self) -> Option<&str> { None }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, the return type is Option<&str>, but in the actual definition of the trait, the return type is Option<String>

@aturon
Copy link
Member Author

aturon commented Oct 3, 2014

@japaric Thanks, fixed.

// }

// Note: the definitions below are copied from core::any, and should be unified
// as soon as possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a path forward for unifying these other than allowing &Error to be casted to &Any?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the ideal path; I'm not sure how soon that's likely to happen.

Alternatively, we could probably define macros for this, but given visibility rules that seemed more pain than gain right now.

@aturon
Copy link
Member Author

aturon commented Oct 3, 2014

@alexcrichton fixed, thanks.

@@ -66,6 +64,7 @@
#[doc(no_inline)] pub use cmp::{Ordering, Less, Equal, Greater, Equiv};
#[doc(no_inline)] pub use collections::{Collection, Mutable, Map, MutableMap, MutableSeq};
#[doc(no_inline)] pub use collections::{Set, MutableSet};
#[doc(no_inline)] pub use error::Error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually expect inspection of errors to not fall under "an overwhelming majority of rust code does this", but rather just the tip of the stack is the one to inspect the error. Along those lines, I'm not sure I'd put this in the prelude to get access to the methods.

On the other hand, implementing the trait is always convenient if you don't have to import it (and it avoids the std::error::Error stutter).

The RFC doesn't look like it mentions any modifications to the prelude, so I'm curious, what was your rationale for placing this here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just writing a comment to get your opinion about this. You're right, it wasn't mentioned in the RFC and for that reason alone probably shouldn't happen here.

I also worry about having the name Error show up in the prelude. So maybe an explicit import makes more sense.

(My rationale was just that you'd generally expect methods like description to be available on error types without needing to import a trait, but that's pretty weak sauce.)

@alexcrichton
Copy link
Member

Do you think it may be best to wait for multidispatch to land before landing this? This is a pretty minimal change, so I don't think it's too susceptible to bitrot (ok on that front). I'd be more curious in going ahead and leveraging this trait throughout some examples in the stdlib/compiler/etc to give it a bit of usage and make sure the try! macro works as expected.

@aturon
Copy link
Member Author

aturon commented Oct 3, 2014

@alexcrichton Addressed comments, removed from prelude.

@aturon
Copy link
Member Author

aturon commented Oct 3, 2014

@alexcrichton Sure, we can wait to land this until multidispatch, and I can add tests/examples to docs.

@aturon aturon force-pushed the error-interop branch 2 times, most recently from 4110531 to 5a7810e Compare October 3, 2014 22:28
@aturon
Copy link
Member Author

aturon commented Oct 13, 2014

Just an update: I've tried rolling this out with multidispatch now being snapshotted. However, there are some problems with type inference. @nikomatsakis is looking into it. We can't land this change to try! until that's resolved.

In general, also, the two blanket impls in the RFC conflict (the one that passes through a value unchanged, and the one that boxes it). If we had negative bounds, we could work around that. Instead, if you want to be able to get automatic boxing for a particular concrete error you'll have to write an impl for it.

@nikomatsakis
Copy link
Contributor

cc #18019

@ghost
Copy link

ghost commented Oct 23, 2014

@aturon Is this good to go after the next snapshot?

@alexcrichton
Copy link
Member

cc #18259 for new snapshots

@aturon
Copy link
Member Author

aturon commented Oct 31, 2014

@alexcrichton I've updated with documentation and use of multidispatch. Building fine here. re-r?

bors added a commit that referenced this pull request Oct 31, 2014
This PR:

* Adds the error interoperation traits (`Error` and `FromError`) to a new module, `std::error`, as per [RFC 70](https://github.com/rust-lang/rfcs/blob/master/active/0070-error-chaining.md). Note that this module must live in `std` in order to refer to `String`.

    Note that, until multidispatch lands, the `FromError` trait cannot be
usefully implemented outside of the blanket impl given here.

* Incorporates `std::error::FromError` into the `try!` macro.

* Implements `Error` for most existing error enumerations.

Closes #17747
bors added a commit that referenced this pull request Oct 31, 2014
This PR:

* Adds the error interoperation traits (`Error` and `FromError`) to a new module, `std::error`, as per [RFC 70](https://github.com/rust-lang/rfcs/blob/master/active/0070-error-chaining.md). Note that this module must live in `std` in order to refer to `String`.

    Note that, until multidispatch lands, the `FromError` trait cannot be
usefully implemented outside of the blanket impl given here.

* Incorporates `std::error::FromError` into the `try!` macro.

* Implements `Error` for most existing error enumerations.

Closes #17747
As per [RFC 70](https://github.com/rust-lang/rfcs/blob/master/active/0070-error-chaining.md)

Closes rust-lang#17747

Note that the `error` module must live in `std` in order to refer to `String`.

Note that, until multidispatch lands, the `FromError` trait cannot be
usefully implemented outside of the blanket impl given here.
bors added a commit that referenced this pull request Nov 3, 2014
This PR:

* Adds the error interoperation traits (`Error` and `FromError`) to a new module, `std::error`, as per [RFC 70](https://github.com/rust-lang/rfcs/blob/master/active/0070-error-chaining.md). Note that this module must live in `std` in order to refer to `String`.

    Note that, until multidispatch lands, the `FromError` trait cannot be
usefully implemented outside of the blanket impl given here.

* Incorporates `std::error::FromError` into the `try!` macro.

* Implements `Error` for most existing error enumerations.

Closes #17747
@bors bors closed this Nov 3, 2014
@bors bors merged commit 38e0745 into rust-lang:master Nov 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement error interoperation
7 participants