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 #630

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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 already implement the new trait for all cases where the custom trait has been implemented so far.

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

see #629 for an alternative approach which however doesn't seem to work (but would be much more concise if it did!)

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 already implement the new trait for all cases where the
custom trait has been implemented so far.

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.
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

lgtm, this is the most reasonable way of adding core::error::Error (the other PR with dyn is a bit weird)

the other concern is when to merge and when to release, since this is quite a big msrv bump. I don't have opinions, would be ok with both doing it now or waiting. Will leave to other hal team members to decide.

@jordens
Copy link
Contributor

jordens commented Sep 11, 2024

Looks good. Is there an example on how that would be used in practice (in a hal impl and then in e.g. a device driver)?

I would now expect device drivers to impl core::error::Error for their errors (using e.g. thiserror) as well with fn source(&self) wanting to call the hal impl's Some(&hal_impl_error.kind() as &dyn Error).
But that ErrorKind result needs to be kept alive making it a bit inconvenient. I guess they'll need to eagerly call and store the ErrorKind on their creation. Playground.

The full solution appears to be ErrorType::Error: Error + core::error::Error.
Demanding that from HALs is breaking but it's not difficult to satisfy.
Without that requirement this PR is hard to make use of.

@MathiasKoch
Copy link

How does the error trait fit with defmt? Is it possible to use it properly while still leveraging the greatness of defmt?

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.

4 participants