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

Feature Request: Add Debug bound to Error types for Database trait #1674

Open
prestwich opened this issue Aug 1, 2024 · 1 comment · May be fixed by #1840
Open

Feature Request: Add Debug bound to Error types for Database trait #1674

prestwich opened this issue Aug 1, 2024 · 1 comment · May be fixed by #1840
Labels
feature New feature or lib ability

Comments

@prestwich
Copy link
Contributor

prestwich commented Aug 1, 2024

Currently downstream errors containing Db::Error have to add manual bounds for Debug if they want any printing strategy. However, Debug is derived or impld for all current errors used in revm. This would be a breaking change, however it is unlikely that any downstream implementors have non-Debug error types (I checked the foundry fork DB and they're already Debug)

This affects EVMError<Db::Error> because it derives Debug and therefore has no Debug impl unless the inner Db::Error has a Debug bound

affected traits:

  • Database
  • DatabaseRef
  • State
  • StateRef
  • BlockHash
  • BlockHashRef

What I want to do:

// broken code
#[derive(Debug, thiserror::Error)]
pub enum Error<Db> 
where
    Db: Database
{
    // results in an error 
    // `EvmError<Db::Error>` cannot be formatted using `{:?}` because it doesn't implement `Debug`
    #[error("{0:?}"]
    Evm(EVMError<Db::Error>),
    ...
}

What I have to do instead:

#[derive(Debug, thiserror::Error)]
pub enum Error<Db> 
where
    Db: Database
    // this bound holds for all current Database implementors, but must be manually written by consumers
    Db::Error: Debug,
{
    #[error("{0:?}"]
    Evm(EVMError<Db::Error>),
    ...
}

the same is true of std::error::Error, however:

  • this approach does not dovetail with the crates extern crate alloc as std strategy for no_std support
  • it would require an intermediary trait ErrBound to handle changing the bound for std/no_std
  • there would need to be a cfg(feature = "std") implementation of std::error::Error for DatabaseComponentError

so while adding a std::error::Error it seems like supporting a bound of std::error::Error would be much more invasive, while adding Debug is non-invasive

@rakita
Copy link
Member

rakita commented Aug 2, 2024

This seems reasonable

We are currently in the process of bigger refactor on Generics here: #1672
So I will plan to add those at a similar time.

@rakita rakita added the feature New feature or lib ability label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or lib ability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants