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

Move ::std::error to ::core::error #33149

Closed

Conversation

thepowersgang
Copy link
Contributor

The idea of this is to move the Error trait into libcore, where it can be used by more code, particularly no_std code.

It however has a few possibly breaking changes in it. The first is that the downcast methods were moved from being on Error (the unsized type) to being on Box<Error>. This changes the UFCS paths, but doesn't change the method-call usage.

Secondly, the Error trait has been marked with #[fundamental] to allow the From<&str> for Box<Error> impls to work. When raised on IRC, the tentative conclusion was that being an error is not an incidental aspect of a type, so this is acceptable.

Feel free to reject this PR if it breaks existing code, it's more of a "nice to have" feature.

- Moved entire module to libcore
- Apply `#[fundamental]` to the `Error` trait [breaking-change]
- Move Box downcast methods to methods on `Box<Error>` [breaking-change]
@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@alexcrichton
Copy link
Member

The libs team has been very hesitant to make any more traits #[fundamental] because it's unclear if that functionality will ever be stabilized. We would like to have very few traits marked as such, and arguably Error isn't actually a fundamental trait because many crates can often forget to implement it and then add it in a later commit.

It is kinda unfortunate that Error isn't in libcore, but unless we can do it without #[fundamental] I would not personally want to move it at this time.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 22, 2016
@bors
Copy link
Contributor

bors commented May 8, 2016

☔ The latest upstream changes (presumably #33494) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

The libs team discussed this during triage the other day and the conclusion was that we don't want to do this. The Error trait is not actually a fundamental trait as implementations are often forgotten and added after the fact, and this change would make those changes breaking changes.

We can perhaps do this eventually by informing coherence that specifically some times (e.g. those in libcore) won't implement Error ever, but that's unfortunately a feature for another day.

Thanks though for the PR @thepowersgang!

@tarcieri
Copy link
Contributor

@alexcrichton any chance your opinion has changed on this in the past year? This seems like something of a blocker for using things like error-chain in no_std environments:

rust-lang-deprecated/error-chain#157

My solution there was to vendor all of libstd's error.rs into error-chain for use in no_std contexts, which really feels to me like the wrong answer, and that the "right" one is moving std::error back into libcore.

@arielb1
Copy link
Contributor

arielb1 commented Jun 12, 2017

The libcore PR was rejected because it made Error fundamental, which was a breaking change.

I think that can be bypassed with this hack (a combination of what I'll call the "exotic closure" trait and "predicate" trait):

in libcore:

#[doc(hidden)]
#[unstable(feature="libstd_implementation_detail")]
pub trait ErrorOrStrMapper {
    type Output;
    fn from_error<E: Error>(self, e: E) -> Self::Output;
    fn from_str(self, s: &str) -> Self::Output;
}

#[doc(hidden)]
#[unstable(feature="libstd_implementation_detail")]
trait ErrorOrStr {
    fn convert<F: ErrorOrStrMapper>(self, f: F) -> F::Output;
}

impl<E: Error> ErrorOrStr for E {
    fn to_error<F: ErrorOrStrMapper>(self, f: F) -> F::Output {
        f.from_error(self)
    }
}

// ok because libcore knows that `! &'a str : Error`
impl<'a> ErrorOrStr for &'a str {
    fn to_error<F: ErrorOrStrMapper>(self, f: F) -> F::Output {
        f.from_str(self)
    }
}

in liballoc (From<&str> for Box<Error> must be implemented there, because all the types are available in liballoc, so StringError must use Box<str>):

struct StringError(Box<str>);

struct BoxErrorMapper;
impl ErrorOrStrMapper for BoxErrorMapper {
    type Output = Box<Error + Send + Sync>;
    fn from_error<E: Error>(self, e: E) -> Self::Output {
        Box::new(e)
    }
    fn from_str(self, s: &str) -> Self::Output {
        // I'm not sure on that `From` - we might want to return an
        // OomError here if we fail to allocate enough memory.
        Box::new(StringError(From::from(s)))
    }
}

impl<E: ErrorOrStr> From<E> for Box<Error + Send + Sync> {
    fn from(err: E) -> Self {
        err.to_error(BoxErrorMapper)
    }
}

impl<E: ErrorOrStr> From<E> for Box<Error> {
    fn from(err: E) -> Self {
        err.to_error(BoxErrorMapper)
    }
}

impl From<Box<str>> for Box<Error + Send + Sync> {
    fn from(s: Box<str>) -> Self {
        Box::new(StringError(s))
    }
}

// ok because String is local, so we know `!String: ErrorOrStr`
impl From<Box<str>> for Box<Error> {
    fn from(s: String) -> Self {
        Box::new(StringError(s))
    }
}

in libcollections:

// ok because String is local, so we know `!String: ErrorOrStr`
impl From<String> for Box<Error + Send + Sync> {
    fn from(s: String) -> Self {
        Self::from(s.into_boxed_str())
    }
}

// ok because String is local, so we know `!String: ErrorOrStr`
impl From<String> for Box<Error> {
    fn from(s: String) -> Self {
        Self::from(s.into_boxed_str())
    }
}

The downside is that the docs will have the From impl as E: ErrorOrStr rather than separate E: &str and E: Error.

@Pratyush
Copy link
Contributor

Pratyush commented Jul 13, 2017

This is probably relevant here: https://internals.rust-lang.org/t/crate-evaluation-for-2017-06-27-error-chain/5362/25

In short, there are plans to move std::error::Error into alloc.

@jbowens
Copy link

jbowens commented Nov 20, 2017

😢

@tarcieri
Copy link
Contributor

@withoutboats' failure crate and the Fail trait is probably the way forward here, as the entire crate has been written with no_std support in mind:

https://github.com/withoutboats/failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants