oxc-project / oxc

⚓ A collection of JavaScript tools written in Rust.
https://oxc.rs
MIT License
10.66k stars 392 forks source link

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

Open rzvxa opened 5 days ago

rzvxa commented 5 days ago

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.

https://github.com/oxc-project/oxc/blob/20a7861838dc4a978b1a8d8494056a8c45bc815a/tasks/ast_tools/src/schema/defs.rs#L46-L87

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).

https://github.com/oxc-project/oxc/blob/20a7861838dc4a978b1a8d8494056a8c45bc815a/tasks/ast_tools/src/derives/mod.rs#L94-L102

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:

https://github.com/oxc-project/oxc/blob/20a7861838dc4a978b1a8d8494056a8c45bc815a/crates/oxc_ast_macros/src/lib.rs#L71-L81

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 commented 5 days ago

@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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

@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 commented 5 days ago

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".