oxc-project / oxc

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

Fix `inherit_variants!` macro breaking Rust Analyser #4297

Closed overlookmotel closed 3 months ago

overlookmotel commented 6 months ago

oxc-project/oxc#3115 introduced the inherit_variants! macro which shares variants between enum AST types.

As Boshen pointed out in https://github.com/oxc-project/oxc/pull/3115#issuecomment-2081362925, it unfortunately broke Rust Analyser's ability to "see" the variants which are inherited.

image

This problem could be solved in a number of ways:

  1. Replace inherit_variants! with #[inherit_variants] proc macro.
  2. Codegen.
  3. Combination of the two.

This also raises some wider questions about how to use code generation in Oxc more generally - some of which covered below.

Option 1: Proc macro

Self-explanatory. Just convert existing macro to proc macro.

Option 2: Codegen

Option 2 would be ideal for compile times, but I don't see how it'd work.

4 possible approaches I can see:

2a: Codegen edits existing files

Codegen edits files in oxc_ast/src/ast directly.

This seems dangerous to me. If a contributor is working on oxc_ast/src/ast/js.rs, and they add an enum variant to one of the enums which is inherited from, the build script codegen would run and overwrite the file, adding the new enum variant to other types which inherit from it. Great! Like an advanced auto-complete.

But... if the contributor has continued editing the file to make other changes, either (1) those changes could be clobbered by the build script or (2) the user's other changes clobber the build script's changes.

2b: Codegen generates all the AST types

We could generate all the AST type definitions from some kind of schema, but personally I don't think this is a good idea. The types are already written as Rust types, and I don't particularly see the point of translating them to e.g. JSON, schema so that a codegen can regenerate the same Rust code we already have.

You have mentioned doing this a couple of times @Boshen - what's your rationale for that?

2c: Codegen creates a "shadow" copy of crate

Build script would create a copy of the entire contents of oxc_ast/src, and make changes to it as required.

oxc_ast/src would be the source code that contributors edit, but the copy output by build script oxc_ast/built is the code which you would actually get when you use oxc_ast.

This does have some advantages:

  1. Does not introduce more proc macros.
  2. Future potential to find a way for build script to "pre-expand" all proc macros. There is then less work to do at compile time - all crates which do not have code changes will not need to be expanded again - essentially caching macro expansion.

However, there's a major DX problem. IDEs' "jump to definition" would take you to the built folder, not src. I am sure people would end up editing the files in built and then be surprised when all the code they wrote is lost when the build script re-runs and overwrites these files.

I don't believe Rust has any equivalent of JS's source maps.

2d: Cache macro expansion output

This is the most "mad science" solution. Maybe can do something along these lines:

This has the following advantages:

But:

  1. Is this even possible?
  2. Outer attrs #![...] on modules are only available on nightly Rust.

Personally I would be very keen on some form of "macro pre-expansion" approach (either 2c or 2d). It would remove the trade-off between using macros and the compile time cost. We could have our macro cake and eat it. But I just don't see how (2c) would work, and (2d) would likely be a lot of effort to make work, if it works at all.

Option 3: Codegen + proc macros.

If pure codegen is not workable (or not workable for now), I think this is best remaining option.

This would be much nicer than the current situation where altering the types also requires altering the macro to keep it in sync.

The types schema could also be useful for other things e.g.:

Boshen commented 3 months ago

Seems fixed?

image
overlookmotel commented 3 months ago

Yes indeed! It does seem to be behaving as desired now. We do still intend to replace inherit_variants! with a more robust version via AST codegen, but great that this major problem has now gone away.