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

feat: support async_trait-less generation #1697

Closed
wants to merge 1 commit into from

Conversation

jrandolf
Copy link

@jrandolf jrandolf commented May 9, 2024

Motivation

async_trait is not necessary for most use-cases. This PR provides the option for users.

Solution

Providing an option to generate async traits without async_trait.

Fixed: #1651

@jrandolf jrandolf force-pushed the master branch 2 times, most recently from 03921c4 to 9b03433 Compare May 9, 2024 04:41
@jrandolf jrandolf force-pushed the master branch 3 times, most recently from 4e862a5 to 4ea070c Compare May 24, 2024 20:29
@jrandolf
Copy link
Author

@LucioFranco Note that the current CI doesn't test all feature combinations. We should use cargo hack --feature-powerset test to ensure all features are covered.

Cargo.toml Outdated Show resolved Hide resolved
codegen/Cargo.toml Outdated Show resolved Hide resolved
async fn #name(#self_param, request: tonic::Request<#req_message>)
-> std::result::Result<tonic::Response<#res_message>, tonic::Status> {
Err(tonic::Status::unimplemented("Not yet implemented"))
#[cfg(not(feature = "async_trait"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a lot of code duplication on top of what's already there. Given that the only difference is whether the method is prefixed with async or not and wrapping of the return type in impl std::future::Future<Output = ..>, can we simplify?

Copy link
Author

Choose a reason for hiding this comment

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

Decomposing a method to such an extent may not be advisable. It represents a premature optimization that could lead to developer confusion and complications. Understanding the interdependencies among the components within the decomposition requires extra caution, potentially increasing the risk of errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not a satisfying answer to me. I don't think the term "premature optimization" applies here, since there's no performance impact to what I'm suggesting (indeed, the performance might well be worse).

I'm suggesting that the increased code duplication makes this code harder to maintain (for example, carefully reviewing these changes is a pain in the ass), and asking you whether that issue could be mitigated.

Copy link
Author

Choose a reason for hiding this comment

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

The changes have already been made. I was just expressing an opinion.

Copy link
Author

Choose a reason for hiding this comment

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

@djc Gentle ping. Could you please take a look?

@jrandolf jrandolf force-pushed the master branch 2 times, most recently from 2ec5abd to a34b259 Compare May 25, 2024 18:33
@jrandolf jrandolf requested a review from djc May 25, 2024 18:38
@LucioFranco
Copy link
Member

I think we need to generate the code as fn foo() -> impl Future + Send to allow sendable futures. We also need to check if the generated functions will work with the Arc since that also needs Sync. I would like to move in this direction but I don't feel like there is a huge rush. I would like to see some more tests as well checking all the permutations of sendiness/syncness so that we know this will work for users.

@jrandolf jrandolf closed this by deleting the head repository Sep 10, 2024
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.

Introduce a build option to not add #[async_trait] on generated service.
3 participants