-
Notifications
You must be signed in to change notification settings - Fork 572
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
refactor: make stricter database trait bounds for user ergonomics #1840
base: main
Are you sure you want to change the base?
refactor: make stricter database trait bounds for user ergonomics #1840
Conversation
hmmmm. for some reason it seems not all packages build when running punting to draft |
Ok it seems to be mostly an example that was broken, so I've updated that :) I think this is ready for consideration. CI failure appears to be unrelated |
@@ -10,7 +10,7 @@ use std::sync::Arc; | |||
|
|||
#[auto_impl(&mut, Box)] | |||
pub trait State { | |||
type Error; | |||
type Error: core::error::Error + 'static; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 'static
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case, it's required by the impl :Error for DatabaseComponentError<T, U>
here. The bound could be removed here, but it would require all downstream users to propagate 2 bounds every time they want to use that impl Error for DatabaseComponentError
(which is pretty often in practice)
Given that we don't have any examples of errors borrowing (i.e. all our current databases would meet this bound) it seems pretty reasonble to put it here too. This is just an example lib, and I don't feel strongly about its ergonomics tho
I would consider adding a + 'static
bound to the Error
assoc type in trait Database
tho, for the same reason. We have no clear use case for errors that capture data instead of own it, and it causes a lot of downstream complexity
|
||
/// Wraps a [`BlockHashRef`] to provide a [`BlockHash`] implementation. | ||
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
pub struct WrapBlockHashRef<T: BlockHashRef>(pub T); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, i can't remember it but would this code
impl<T: BlockHashRef> BlockHash for T {
solve all of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was unnecessary anyway so i removed it
@@ -21,7 +21,7 @@ pub use empty_db::{EmptyDB, EmptyDBTyped}; | |||
#[auto_impl(&mut, Box)] | |||
pub trait Database { | |||
/// The database error type. | |||
type Error; | |||
type Error: core::error::Error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider adding a 'static
bound here, as described in https://github.com/bluealloy/revm/pull/1840/files#r1819000225
@@ -52,7 +52,7 @@ pub trait DatabaseCommit { | |||
#[auto_impl(&, &mut, Box, Rc, Arc)] | |||
pub trait DatabaseRef { | |||
/// The database error type. | |||
type Error; | |||
type Error: core::error::Error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider adding a 'static
bound here, as described in https://github.com/bluealloy/revm/pull/1840/files#r1819000225
@@ -13,7 +13,7 @@ use crate::{Database, DatabaseRef}; | |||
/// Use [WrapDatabaseAsync] to provide [Database] implementation for a type that only implements this trait. | |||
pub trait DatabaseAsync { | |||
/// The database error type. | |||
type Error: Send; | |||
type Error: core::error::Error + Send; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider adding a 'static
bound here, as described in https://github.com/bluealloy/revm/pull/1840/files#r1819000225
@@ -48,7 +48,7 @@ pub trait DatabaseAsync { | |||
/// Use [WrapDatabaseAsync] to provide [DatabaseRef] implementation for a type that only implements this trait. | |||
pub trait DatabaseAsyncRef { | |||
/// The database error type. | |||
type Error: Send; | |||
type Error: core::error::Error + Send; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider adding a 'static
bound here, as described in https://github.com/bluealloy/revm/pull/1840/files#r1819000225
a14a377
to
c44063b
Compare
CI failure seems to be related to #1844 |
Two quick ergonomics changes for downstream users. Intended to simplify bounds writing
This is a breaking change, as downstream implementors of these traits may need to be updated
Error
bounds for database errorsAdds an error bound for
Database::Error
andDatabaseRef::Error
, and the async versions. This closes #1674. This prevents propagation of complex bounds for upstream wrapper structs, particularly error types that might wrap EVM output.motivation:
simplify bounds writing for users by ensuring they don't need to manually write out error bounds. This is particulartly a problem when holding an instance of
EVMError<Db>
Before:
After:
Add
Database
as a supertrait ofDatabaseCommit
This does not close a current issue, but I'm happy to make one. This causes some bounds changes in the
database_components
examplesmotivation: simplify bounds writing for users by allowing them to avoid specifying
Database
alternate approach: a
trait CommitableDatabase: Database + DatabaseCommit
with a blanketimpl<T> CommittableDatabase for T where T: Database + DatabaseCommit
Code before:
Code after:
Add
WrapBlockHashRef
to exampleAdd a wrapper struct like
WrapDatabaseRef
toBlockHashRef
to allow relaxing the bounds on the database components in thedatabase_components
exampleDrive-by:
The asyncdb is cfged out when running
cargo c --all-features
. And runningcargo c -p revm-database-interface --features asyncdb
produced compilation errors due to disabled tokio features. I have enabled the necessary features. When running with--workspace
it appears some other package enabled the features, and covered up the dep specification issue