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

der: add Error::(set_)source; remove Clone + Copy #1328

Merged
merged 1 commit into from
Mar 2, 2024
Merged

Conversation

tarcieri
Copy link
Member

When the std feature is enabled, adds support for propagating an arbitrary error source which can be set using Error::set_source.

The main use case is passing back an arbitrary error value in the ErrorKind::Value use case, to handle passing back what specifically was wrong with a given value in a DER-encoded document.

This entails removing the Clone + Copy bounds since the error is stored in a Box, which can't be Copy and can't reflect a Clone bound. Fortunately, nothing in this repo ever seems to clone an Error so it's not really an issue.

Also adds Error::source impls to the downstream error types in pkcs1, pkcs5, pkcs8, sec1, and spki.

When the `std` feature is enabled, adds support for propagating an
arbitrary error source which can be set using `Error::set_source`.

The main use case is passing back an arbitrary error value in the
`ErrorKind::Value` use case, to handle passing back what specifically
was wrong with a given value in a DER-encoded document.

This entails removing the `Clone` + `Copy` bounds since the error is
stored in a `Box`, which can't be `Copy` and can't reflect a `Clone`
bound. Fortunately, nothing in this repo ever seems to clone an `Error`
so it's not really an issue.

Also adds `Error::source` impls to the downstream error types in
`pkcs1`, `pkcs5`, `pkcs8`, `sec1`, and `spki`.
@turbocool3r
Copy link
Contributor

I like this approach, but I still think this serde-like way I suggested in #1055 might be the way to go if there is a need to make error processing zero-cost.

One might use a pattern when a few decoders are tested in sequence and the process is only continued when certain internal errors occur (similar to trying to decode something as different DER types) which might be less efficient with this dynamic approach. Dependency on std also prevents some use cases like this crate of mine which supports no_std (for reasons) and would benefit from custom errors. Personally I think if there might be a breaking change it's worth doing one which allows to use more patterns.

I can update the PR if you think it's worth seeing how well the other approach works with the current code.

@tarcieri tarcieri merged commit 7784f2d into master Mar 2, 2024
106 checks passed
@tarcieri tarcieri deleted the der/error-source branch March 2, 2024 20:29
tarcieri added a commit that referenced this pull request Mar 28, 2024
This reverts commit 7784f2d.

This ended up complicating downstream error types, particularly in
`no_std` contexts.

It's also not really necessary now that #1055 has landed.
tarcieri added a commit that referenced this pull request Mar 28, 2024
…" (#1371)

This reverts commit 7784f2d.

This ended up complicating downstream error types, particularly in
`no_std` contexts.

It's also not really necessary now that #1055 has landed.
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.

2 participants