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

Add visitor methods to Config in prost-build #816

Open
bsurmanski opened this issue Feb 9, 2023 · 2 comments
Open

Add visitor methods to Config in prost-build #816

bsurmanski opened this issue Feb 9, 2023 · 2 comments

Comments

@bsurmanski
Copy link

The type_attribute, field_attribute is powerful but sometimes constraining. Previously, identifying enums vs messages was impossible due to this (see: #784). There are other customer-specific patterns that can't be turned into '*_attribute' methods.

I believe a type_visitor and field_visitor (and maybe message_visitor, enum_visitor) method should be added to Config. Then customers can do something like

     config.type_visitor("path.to.package", |ty, buf| {
         if(ty.name.endswith("GRPC") {
             buf.push_str("#[serde(rename_all = \"camelCase\")]");
         }
    });

I did some research on implementation details in #586

Thanks!

@LucioFranco
Copy link
Member

This is a really interesting idea! I think its a bit complicated and exposes a lot of internal details so I am not sure if its the right approach. Would be interesting to see a comparison on how other languages handle similar sort of features for their code gen.

@bsurmanski
Copy link
Author

bsurmanski commented Feb 13, 2023

Regarding other languages: in a sense, Prost is 'compiling' (or transpiling) the proto to Rust. The visitor pattern is pretty common in compilers, in general.

In terms of internals: I was just reusing the pattern used in the *_attribute methods. For the lambda input, the DescriptorProto types are already publicly externalized via prost_types. For output, instead, the lambda can be expected to return an Option(String), Some(X) if it wants to append an attribute. So instead, something like

     config.type_visitor("path.to.package", |ty| {
         if(ty.name.endswith("GRPC") {
             return Some("#[serde(rename_all = \"camelCase\")]");
         }
         None
    });

In retrospect, this is much cleaner than passing a buf parameter.

WIth that a _visitor method could potentially be created for each kind of DescriptorProto in prost_types (message, field, enum, file, method, oneof, service).

Perhaps the actual codegen pass could just be added as a set of visitors.

One weakness of the visitor pattern for this is order of visitors might matter. I think i ran into a thing where a Derive(serde::Deserialize) needed to be before serde(rename). And if a codegen pass is a visitor, it would have to be after any type/field annotations.

One potential alternative might be proc_macros. I don't know how that would work, but they take a TokenStream in, and apply arbitrary modifications (vaguely like this).

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

No branches or pull requests

2 participants