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 core::error::Error (dynamic approach) #629

Closed

Conversation

rursprung
Copy link
Contributor

this trait has been stabilised in Rust 1.81.0.

the existing custom Error types cannot be removed / replaced as that'd be a breaking change. for the same reason it's not possible for them to require core::error::Error being implemented.

however, we can add an impl core::error::Error for dyn Error for every custom error type to provide an automatic coverage. HALs can still add an implementation for their specific error type if they wish to add more details (e.g. implement source).
as core::error::Error requires core::fmt::Display to be implemented a simple blanket implementation has been added which prints the Debug output.

existing std feature-gated implementations of std::error::Error have also been moved to core::error::Error and the feature gate removed.

this raises the MSRV to 1.81.0 for most crates, but based on the MSRV policy this should not be an issue.

@rursprung rursprung requested a review from a team as a code owner September 9, 2024 14:03
@rursprung rursprung force-pushed the implement-dyn-core-error branch from 2cac8f5 to 14fa30e Compare September 9, 2024 14:05
@rursprung rursprung changed the title implement core::error::Error implement core::error::Error (dynamic approach) Sep 9, 2024
@rursprung
Copy link
Contributor Author

it seems that this doesn't work as intended, as this doesn't build (tested with i2c::ErrorKind for which an impl Error for ErrorKind exists which in turn has an impl core::error::Error for dyn Error {}):

fn cause_error() -> Result<(), ErrorKind> {
    Err(ErrorKind::Bus)
}

fn foo() -> Result<(), Box<dyn core::error::Error>> {
    cause_error()?;
    Ok(())
}

with #630 (alternative implementation) this works.

i don't know if there's a way to make the approach from this PR here work (in which case HALs wouldn't need to do anything to support core::error::Error except maybe for special cases) or if the other approach has to be used.

@rursprung rursprung force-pushed the implement-dyn-core-error branch from 14fa30e to 7bb7ca3 Compare September 9, 2024 14:22
this trait has been stabilised in Rust 1.81.0.

the existing custom `Error` types cannot be removed / replaced as that'd
be a breaking change. for the same reason it's not possible for them to
require `core::error::Error` being implemented.

however, we can add an `impl core::error::Error for dyn Error` for every
custom error type to provide an automatic coverage. HALs can still add
an implementation for their specific error type if they wish to add more
details (e.g. implement `source`).
as `core::error::Error` requires `core::fmt::Display` to be implemented
a simple blanket implementation has been added which prints the `Debug`
output.

existing `std` feature-gated implementations of `std::error::Error` have
also been moved to `core::error::Error` and the feature gate removed.

this raises the MSRV to 1.81.0 for most crates, but based on the MSRV
policy this should not be an issue.
@rursprung
Copy link
Contributor Author

closed in favour of #630

@rursprung rursprung closed this Sep 24, 2024
@rursprung rursprung deleted the implement-dyn-core-error branch September 24, 2024 20:13
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.

1 participant