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

Support for #[cfg_attr(cond, generate_derive(x, y, z))] #5738

Open
rzvxa opened this issue Sep 12, 2024 · 5 comments
Open

Support for #[cfg_attr(cond, generate_derive(x, y, z))] #5738

rzvxa opened this issue Sep 12, 2024 · 5 comments
Labels
C-enhancement Category - New feature or request good first issue Experience Level - Good for newcomers

Comments

@rzvxa
Copy link
Collaborator

rzvxa commented Sep 12, 2024

We should parse the cfg_attr attributes in our oxc_ast_tools/oxc_ast_macros and use them to find conditional generate_derive usages.

This should be implemented in the same way that generate_derive works. We need to change the generate_derives field in these 2 types so it'd be Vec<(Option</* conditions */ TokenStream>, /* trait name */ String)> or something similar.

pub struct StructDef {
pub id: TypeId,
pub name: String,
#[serde(skip)]
pub visitable: bool,
pub fields: Vec<FieldDef>,
#[serde(skip)]
pub has_lifetime: bool,
pub size_64: usize,
pub align_64: usize,
pub offsets_64: Option<Vec<usize>>,
pub size_32: usize,
pub align_32: usize,
pub offsets_32: Option<Vec<usize>>,
#[serde(skip)]
pub generated_derives: Vec<String>,
#[serde(skip)]
pub markers: OuterMarkers,
#[serde(skip)]
pub module_path: String,
}
#[derive(Debug, Serialize)]
#[serde(tag = "type", rename = "enum", rename_all = "camelCase")]
pub struct EnumDef {
pub id: TypeId,
pub name: String,
pub visitable: bool,
pub variants: Vec<VariantDef>,
/// For `@inherits` inherited enum variants
pub inherits: Vec<InheritDef>,
pub has_lifetime: bool,
pub size_64: usize,
pub align_64: usize,
pub offsets_64: Option<Vec<usize>>,
pub size_32: usize,
pub align_32: usize,
pub offsets_32: Option<Vec<usize>>,
pub generated_derives: Vec<String>,
#[serde(skip)]
pub module_path: String,
}

After parsing these derives we just have to adjust this part to add the preceding #[cfg(condition)] to the output stream of each derive(if there is a condition).

let module_path = def.module_path();
let krate = module_path.split("::").next().unwrap();
if !acc.contains_key(krate) {
acc.insert(krate, Default::default());
}
let streams = acc.get_mut(krate).expect("We checked this right above!");
streams.0.insert(module_path);
streams.1.push(stream);
acc

With these 2 changes, we should be done with the oxc_ast_tools part of it.

As for the oxc_ast_macros, We want to process them similarly to the generate_derive here:

let assertion =
attrs.iter().filter(|attr| attr.path().is_ident("generate_derive")).flat_map(parse).map(
|derive| {
let (abs_derive, generics) = abs_trait(&derive);
quote! {{
// NOTE: these are wrapped in a scope to avoid the need for unique identifiers.
trait AssertionTrait: #abs_derive #generics {}
impl<T: #derive #generics> AssertionTrait for T {}
}}
},
);

The only difference is that it should output assertions behind a feature flag.

* use just ast command to run ast tools on your local environment.
@rzvxa rzvxa added C-enhancement Category - New feature or request good first issue Experience Level - Good for newcomers labels Sep 12, 2024
@rzvxa
Copy link
Collaborator Author

rzvxa commented Sep 12, 2024

@Boshen marked this as a good first issue since I've almost explained the whole process.

Implementing this should allow us to start feature-gating some of our generated derives. All of those feature-gating tasks should also be good first issues since they are in fact trivial compared to this one.

Please feel free to remove the good first issue label if you think otherwise Captain.

@overlookmotel
Copy link
Collaborator

Personally I prefer #[cfg_attr(feature = "foo", generate_derive(bar, baz, qux))], because it's familiar syntax and so clearer what it does, rather than another "mysterious" attr.

@rzvxa Do you really think parsing the few extra tokens will make a real difference to the #[ast] macro?

Even if it does, we have the beginnings of viable ideas for pre-compiling the macro output in ast_tools, which would make the macro execution cheap, no matter how much input/output there is. Even if we don't have time for that in near future, if we feel confident we'll get there in the end, I think worth taking that into account.

That said, I don't feel super-strongly about this. Happy to go with #[cfg_generate_derive] if you prefer @rzvxa.

@rzvxa
Copy link
Collaborator Author

rzvxa commented Sep 12, 2024

Do you really think parsing the few extra tokens will make a real difference to the #[ast] macro?

TBH, I didn't time the builds to see how much of an impact it has. However, Since the syn attribute's meta contains the token stream of the arguments we have to parse them explicitly where it didn't before so in theory it does more work. But as always compiler might optimize it down or we don't see the effect because we don't get called too many times(a couple hundred times ain't much).

Maybe we should do it with cfg_attr just to keep it Rust-like; Even if it means a bit slower build(realistically it wouldn't be too bad, Might get worse with time as we use it more).

@rzvxa
Copy link
Collaborator Author

rzvxa commented Sep 12, 2024

@overlookmotel Can you make the call on this one? I'll update the description and title according to your answer.

I don't have a strong preference between these two, Treat cfg_generate_derive as more of a suggestion than a definitive solution.

@overlookmotel
Copy link
Collaborator

I don't have a hugely strong feeling either. If I had to choose, I'd go for cfg_attr for the reason that it's less "mysterious".

@rzvxa rzvxa changed the title Introduce cfg_generate_derive attribute Support for #[cfg_attr(cond, generate_derive(x, y, z))] Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category - New feature or request good first issue Experience Level - Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants