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

IntoError trait #307

Closed
wants to merge 1 commit into from
Closed

IntoError trait #307

wants to merge 1 commit into from

Conversation

stepancheg
Copy link
Contributor

Consider this scenario. There's another error type which should be constructable from either standard errors or anyhow::Error. So ? operator can be used to return results with new error type.

Example of such error is SharedError from buck2 project:

https://github.com/facebook/buck2/blob/126049599e9d1b56c25c5fba7e6127294719dff0/app/buck2_common/src/result.rs#L46

It is not possible to implement such generic conversion in Rust.

For example, this approach is compilation error:

impl<E: Into<anyhow::Error>> From<E> for SharedError {
    fn from(err: E) -> Self { ... }
}

   = note: conflicting implementation in crate `core`:
           - impl<T> From<T> for T;

This approach is different compilation error:

impl<E: std::error::Error> From<E> for SharedError {
    fn from(err: E) -> Self { ...  }
}

impl From<anyhow::Error> for SharedError {
    fn from(err: anyhow::Error) -> Self { ... }
}

   = note: upstream crates may add a new impl of trait `std::error::Error` for type `anyhow::Error` in future versions

IntoError crate implemented inside anyhow crate allows implementation:

impl<E: anyhow::IntoError> From<E> for SharedError {
    fn from(value: E) -> Self { ... }
}

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

impl<E: Into<anyhow::Error>> From<E> for SharedError {
    fn from(_: E) -> Self { ... }
}
impl<E: anyhow::IntoError> From<E> for SharedError {
    fn from(_: E) -> Self { ... }
}

Do you have a complete example where one of these is incoherent ("conflicting implementations") and the other is fine?

@stepancheg
Copy link
Contributor Author

Sorry, I missed important bit of first example. The important part is that I also need From<SharedError> for anyhow::Error.

Two examples which do not work. First example is where SharedError does not implement std::error::Error:

use std::fmt::{Debug, Display, Formatter};

#[derive(Debug)]
struct SharedError {}

impl<E: Into<anyhow::Error>> From<E> for SharedError {
    fn from(value: E) -> Self {
        SharedError {}
    }
}

impl From<SharedError> for anyhow::Error {
    fn from(value: SharedError) -> Self {
        todo!()
    }
}

  = note: conflicting implementation in crate `core`:
          - impl<T> From<T> for T;

Another example is where SharedError implements std::error::Error:

use std::fmt::{Debug, Display, Formatter};

#[derive(Debug)]
struct SharedError {}

impl Display for SharedError {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        todo!()
    }
}

impl std::error::Error for SharedError {}

impl<E: Into<anyhow::Error>> From<E> for SharedError {
    fn from(value: E) -> Self {
        SharedError {}
    }
}

   = note: conflicting implementation in crate `core`:
           - impl<T> From<T> for T;

both do not compile.

Working example is included in this PR.

Consider this scenario. There's another error type which should be
constructable from either standard errors or `anyhow::Error`.
So `?` operator can be used to return results with new error type.

Example of such error is `SharedError` from buck2 project:

https://github.com/facebook/buck2/blob/126049599e9d1b56c25c5fba7e6127294719dff0/app/buck2_common/src/result.rs#L46

It is not possible to implement such generic conversion in Rust.

For example, this approach is compilation error:

```
impl<E: Into<anyhow::Error>> From<E> for SharedError {
    fn from(err: E) -> Self { ... }
}

   = note: conflicting implementation in crate `core`:
           - impl<T> From<T> for T;
```

This approach is different compilation error:

```
impl<E: std::error::Error> From<E> for SharedError {
    fn from(err: E) -> Self { ...  }
}

impl From<anyhow::Error> for SharedError {
    fn from(err: anyhow::Error) -> Self { ... }
}

   = note: upstream crates may add a new impl of trait `std::error::Error` for type `anyhow::Error` in future versions
```

`IntoError` crate implemented inside `anyhow` crate allows
implementation:

```
impl<E: anyhow::IntoError> From<E> for SharedError {
    fn from(value: E) -> Self { ... }
}
```
@JakobDegen
Copy link

Thought about this a bit, I do think the trick shown here is a fully sufficient alternative to this PR. Not sure if there's a downside I'm not seeing, but it's at least not obvious to me what advantage this would still bring

@dtolnay
Copy link
Owner

dtolnay commented Sep 27, 2023

If Jakob's solution is adequate for your use case, that's great and I'd like to close this PR.

Otherwise I am on board with adding a conversion trait. My remaining hesitations are:

  • Judging by the fact that nobody else has raised a need for this in 4 years, putting a new trait into the crate root would seem more prominently featured than it deserves, I think. Is there a submodule that could work? anyhow::convert::IntoError

  • Whether this needs to be a sealed trait. What is the intended semantics of someone implementing this trait in a downstream crate other than via a std::error::Error + Send + Sync + 'static impl.

  • Interaction with anyhow!(…) macro. It seems likely that people would expect anyhow!(e) to allow converting anything that implements IntoError. It already handles a more general set of allowed types than just impl std::error::Error + Send + Sync + 'static, namely Box<dyn Error + Send + Sync> and impl Display. To what extent does that need to be reconciled somehow with this trait.

  • The documentation needs to be clearer about the exact niche situation where somebody would need to look at this trait. I see a lot of opportunity for this trait to be misleading to people whose use case isn't the thing it is invented for. Maybe even to the extent that it might deserve a more explicit trait name.

@stepancheg
Copy link
Contributor Author

Jakob took over what I wanted to do.

I'll close this PR, since it is controversial, and nobody else requested it.

@stepancheg stepancheg closed this Oct 26, 2023
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.

None yet

3 participants