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

Transition to quote + syn for code generation. #1026

Open
wants to merge 62 commits into
base: master
Choose a base branch
from

Conversation

gibbz00
Copy link
Contributor

@gibbz00 gibbz00 commented Apr 13, 2024

Apil 13

Throwing this out here ASAP as a draft so that others can become aware of it. Will most definitely clash with PRs such as #1019.

Built upon #1020. (rebased)

Will create a proper write-up once things become presentable.

Update (14 April)

Post quote + syn POC made separation of concerns a lot clearer, which lead to a bunch of module splitting. Might be worth trying to send for these as separate PRs, which this draft then builds upon. Ex. placing unescape_c_escape_string and its unit tests in a separate module.

Update (21 April)

Think this is ready to be reviewed now.

Goals:

  1. Lay a groundwork for enabling codegen plugin/modularity.
  2. Make it easier to reason and debug about the generated code output

Assessment:

  1. Still plenty left to be done. Refrained from making larger API changes before input from others, in addition to the experience gained from trying to add JSON mapping.

  2. Similar to outcome of 1, but here an example of what has changed:

Example:

Before
self.buf.push_str(&format!(
    "impl {}::Name for {} {{\n",
    self.config.prost_path.as_deref().unwrap_or("::prost"),
    to_upper_camel(message_name)
));
self.depth += 1;

self.buf.push_str(&format!(
    "const NAME: &'static str = \"{}\";\n",
    message_name,
));
self.buf.push_str(&format!(
    "const PACKAGE: &'static str = \"{}\";\n",
    self.package,
));

let prost_path = self.config.prost_path.as_deref().unwrap_or("::prost");
let string_path = format!("{prost_path}::alloc::string::String");

let full_name = format!(
    "{}{}{}{}{message_name}",
    self.package.trim_matches('.'),
    if self.package.is_empty() { "" } else { "." },
    self.type_path.join("."),
    if self.type_path.is_empty() { "" } else { "." },
);
let domain_name = self
    .config
    .type_name_domains
    .get_first(fq_message_name)
    .map_or("", |name| name.as_str());

self.buf.push_str(&format!(
    r#"fn full_name() -> {string_path} {{ "{full_name}".into() }}"#,
));

self.buf.push_str(&format!(
    r#"fn type_url() -> {string_path} {{ "{domain_name}/{full_name}".into() }}"#,
));

self.depth -= 1;
self.buf.push_str("}\n");
After
let name_path = self.prost_type_path("Name");
let message_name_syn = message_name.parse_syn::<syn::Type>();
let package_name = &self.package;
let string_path = self.prost_type_path("alloc::string::String");
let fully_qualified_name =
    FullyQualifiedName::new(&self.package, &self.type_path, message_name);
let domain_name = self
    .config
    .type_name_domains
    .get_first(fq_message_name.as_ref())
    .map_or("", |name| name.as_str());

let fq_name_str = fully_qualified_name.as_ref().trim_start_matches('.');
let type_url = format!("{}/{}", domain_name, fq_name_str);

quote! {
    impl #name_path for #message_name_syn {
        const NAME: &'static str = #message_name;
        const PACKAGE: &'static str = #package_name;

        fn full_name() -> #string_path { #fq_name_str.into() }
        fn type_url() -> #string_path { #type_url.into() }
    }
}
Other examples
Some(quote! {
    #(#documentation)*
    #(#(#type_attributes)*)*
    #(#(#message_attributes)*)*
    #[allow(clippy::derive_partial_eq_without_eq)]
    #[derive(Clone, PartialEq, #prost_path)]
    #maybe_skip_debug
    pub struct #ident {
        #(#resolved_fields,)*
        #(#resolved_oneof_fields,)*
    }

    #nested

    #maybe_type_name
})
quote! {
    #(#documentation)*
    #maybe_deprecated
    #[prost(#field_type_attr, #maybe_label #maybe_boxed tag=#field_number_string, #maybe_default)]
    #field_attributes
    pub #field_identifier: #field_type
}

One takeaway it that this PR removes manual indentation handling, delegating it to the format feature.

Possible future "improvements" which lead to breaking changes.
PR splitting

Waiting for #1029 and #1030 to get merged before rebasing this one on top of them. Unsure how much else there's to split up.

Goal is to hopefully improve code generation logic readability.
Feels weird adding so much manual formatting and offer a auto format
option. Users not using the format option would still be receiving
formatted output, albeit not done as well as `prettyplease` would do.
* Removes the need a bunch of invariant runtime assertions.

* Adds `pretty_assertions` dev dependency for simplied test failure
  debugging.
* scoped and in order of usage
Also moves code_generator.rs to code_generator/mod.rs
@gibbz00 gibbz00 marked this pull request as ready for review April 21, 2024 08:54
@gibbz00 gibbz00 changed the title WIP: Transition to quote + syn for code generation. Transition to quote + syn for code generation. Apr 21, 2024
@caspermeijn
Copy link
Collaborator

Unsure how much else there's to split up.

I suggest the following to make reviewing easier:

  • Split off "this PR removes manual indentation handling, delegating it to the format feature." That is a significant change that could be merged separately.
  • You have made multiple PRs to move code from code_generator.rs to separate modules, however the diff this PR is still a big red file and big green files. So I can't compare the two versions. Maybe a rebase will solve that.

@gibbz00
Copy link
Contributor Author

gibbz00 commented May 29, 2024

Hi, thank you for starting to look into this pr 😊

I'll see what I can do during the weekend.

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

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

  • You have done some work to reorganize code_generator into modules. I think that work is not yet merged into this branch. As the diff in Github is still not useful
  • It seems FullyQualifiedName and pretty_assertions can be easily split into separate PRs

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.

2 participants