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.

Sign up for GitHub

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

chore(build): Refactor server trait gen to dedup some code #2143

Closed
wants to merge 2 commits into from
Closed

chore(build): Refactor server trait gen to dedup some code #2143

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1022521
chore(build): Refactor server trait gen to dedup some code
yotamofek Jan 2, 2025
4050502
Refactor a bit more
yotamofek Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 35 additions & 67 deletions tonic-build/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,91 +257,59 @@ fn generate_trait_methods<T: Service>(
quote!(&self)
};

let method = match (
method.client_streaming(),
method.server_streaming(),
generate_default_stubs,
) {
Comment on lines -260 to -264
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to keep the current styles for the conditions about whether it is client streaming or server streaming or not. This refactoring is well designed to avoid reimplementing logic, but at the same time, I feel that it has become more difficult to grasp the outline of the processing from the perspective of code generation. Taking the trade-off into consideration, how about introducing body_or_semicolon to the conditional branch of generate_default_stubs to reduce the number of branches?

Copy link
Contributor Author

@yotamofek yotamofek Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I agree, refactored a bit more to look more like the older code. There's a bit of duplication here but I think it's a reasonable trade-off because it makes the branching easier to follow. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I tried to say is that using switch with client streaming and/or server streaming, and then in the each branch using body_or_semicolon variable to handle the default stub option.

And, I would like to write partial_sig and return_ty inline directly without making them variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean. I don't really see a point in doing something like match (method.client_streaming(), method.server_streaming()) since those two settings don't impact one another.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If proper separation seems difficult, it seems likely that refactoring aimed at introducing further complexity will also be difficult.

(false, false, true) => {
quote! {
#method_doc
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"))
}
}
}
(false, false, false) => {
quote! {
#method_doc
async fn #name(#self_param, request: tonic::Request<#req_message>)
-> std::result::Result<tonic::Response<#res_message>, tonic::Status>;
}
}
(true, false, true) => {
quote! {
#method_doc
async fn #name(#self_param, request: tonic::Request<tonic::Streaming<#req_message>>)
-> std::result::Result<tonic::Response<#res_message>, tonic::Status> {
Err(tonic::Status::unimplemented("Not yet implemented"))
}
}
}
(true, false, false) => {
quote! {
#method_doc
async fn #name(#self_param, request: tonic::Request<tonic::Streaming<#req_message>>)
-> std::result::Result<tonic::Response<#res_message>, tonic::Status>;
}
let req_param_type = if method.client_streaming() {
quote!(tonic::Request<tonic::Streaming<#req_message>>)
} else {
quote!(tonic::Request<#req_message>)
};

let partial_sig = quote! {
#method_doc
async fn #name(#self_param, request: #req_param_type)
};

let default_body = quote! {
{
Err(tonic::Status::unimplemented("Not yet implemented"))
}
(false, true, true) => {
};

let result = |ok| quote!(std::result::Result<#ok, tonic::Status>);
let response_result = |message| result(quote!(tonic::Response<#message>));

let method = match (method.server_streaming(), generate_default_stubs) {
(false, true) => {
let return_ty = response_result(res_message);
quote! {
#method_doc
async fn #name(#self_param, request: tonic::Request<#req_message>)
-> std::result::Result<tonic::Response<BoxStream<#res_message>>, tonic::Status> {
Err(tonic::Status::unimplemented("Not yet implemented"))
}
#partial_sig -> #return_ty #default_body
}
}
(false, true, false) => {
let stream = quote::format_ident!("{}Stream", method.identifier());
let stream_doc = generate_doc_comment(format!(
" Server streaming response type for the {} method.",
method.identifier()
));

(false, false) => {
let return_ty = response_result(res_message);
quote! {
#stream_doc
type #stream: tonic::codegen::tokio_stream::Stream<Item = std::result::Result<#res_message, tonic::Status>> + std::marker::Send + 'static;

#method_doc
async fn #name(#self_param, request: tonic::Request<#req_message>)
-> std::result::Result<tonic::Response<Self::#stream>, tonic::Status>;
#partial_sig -> #return_ty;
}
}
(true, true, true) => {
(true, true) => {
let return_ty = response_result(quote!(BoxStream<#res_message>));
quote! {
#method_doc
async fn #name(#self_param, request: tonic::Request<tonic::Streaming<#req_message>>)
-> std::result::Result<tonic::Response<BoxStream<#res_message>>, tonic::Status> {
Err(tonic::Status::unimplemented("Not yet implemented"))
}
#partial_sig -> #return_ty #default_body
}
}
(true, true, false) => {
(true, false) => {
let stream = quote::format_ident!("{}Stream", method.identifier());
let stream_doc = generate_doc_comment(format!(
" Server streaming response type for the {} method.",
method.identifier()
));

let stream_item_ty = result(res_message);
let stream_ty = quote!(tonic::codegen::tokio_stream::Stream<Item = #stream_item_ty> + std::marker::Send + 'static);
let return_ty = response_result(quote!(Self::#stream));
quote! {
#stream_doc
type #stream: tonic::codegen::tokio_stream::Stream<Item = std::result::Result<#res_message, tonic::Status>> + std::marker::Send + 'static;
type #stream: #stream_ty;

#method_doc
async fn #name(#self_param, request: tonic::Request<tonic::Streaming<#req_message>>)
-> std::result::Result<tonic::Response<Self::#stream>, tonic::Status>;
#partial_sig -> #return_ty;
}
}
};
Expand Down
Loading