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

Remove async-trait as dependency from crate stages #6771

Merged
merged 2 commits into from Feb 26, 2024

Conversation

AbnerZheng
Copy link
Contributor

Refer to #6657
Replace #[async_trait] with ->imp for traits in stages: ExecuteStageTestRunner, UnwindStageTestRunner and StageExt.

@@ -78,7 +82,7 @@ pub(crate) trait UnwindStageTestRunner: StageTestRunner {
provider.commit().expect("failed to commit");
tx.send(result).expect("failed to send result");
});
Box::pin(rx).await.unwrap()
async { rx.await.unwrap() }
Copy link
Member

Choose a reason for hiding this comment

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

The body of the entire function should be wrapped in async move {} to preserve the same semantics. Although I don't think this matters much in this case

pub(crate) trait UnwindStageTestRunner: StageTestRunner {
/// Validate the unwind
fn validate_unwind(&self, input: UnwindInput) -> Result<(), TestRunnerError>;

/// Run [Stage::unwind] and return a receiver for the result.
async fn unwind(&self, input: UnwindInput) -> Result<UnwindOutput, StageError> {
fn unwind(
Copy link
Collaborator

Choose a reason for hiding this comment

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

since this is the impl of the trait, rust should be able to handle async fn here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you're right.
Looks like impl Future + Send is only needed for pub trait, for example,

pub trait StageExt<DB: Database>: Stage<DB> {
/// Utility extension for the `Stage` trait that invokes `Stage::poll_execute_ready`
/// with [poll_fn] context. For more information see [Stage::poll_execute_ready].
fn execute_ready(
&mut self,
input: ExecInput,
) -> impl Future<Output = Result<(), StageError>> + Send {
poll_fn(move |cx| self.poll_execute_ready(cx, input))
}
}

If I use async here, the compiler throws a errors below:

error: use of `async fn` in public traits is discouraged as auto trait bounds cannot be specified
   --> crates/stages/src/stage.rs:253:5
    |
253 |     async fn execute_ready(&mut self, input: ExecInput) -> Result<(), StageError> {
    |     ^^^^^
    |
    = note: you can suppress this lint if you plan to use the trait only in your own code, or do not care about auto traits like `Send` on the `Future`
    = note: `-D async-fn-in-trait` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(async_fn_in_trait)]`
help: you can alternatively desugar to a normal `fn` that returns `impl Future` and add any desired bounds such as `Send`, but these cannot be relaxed without a breaking API change
    |
253 ~     fn execute_ready(&mut self, input: ExecInput) -> impl std::future::Future<Output = Result<(), StageError>> + Send {async {
254 |         poll_fn(|cx| self.poll_execute_ready(cx, input)).await
255 ~     } }
    |

@mattsse mattsse merged commit 92396b3 into paradigmxyz:main Feb 26, 2024
26 of 30 checks passed
@AbnerZheng AbnerZheng deleted the replace-async-trait-in-stages branch February 26, 2024 11:41
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.

None yet

3 participants