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

Fix inherit_variants! macro breaking Rust Analyser #4297

Closed
overlookmotel opened this issue May 7, 2024 · 2 comments
Closed

Fix inherit_variants! macro breaking Rust Analyser #4297

overlookmotel opened this issue May 7, 2024 · 2 comments

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented May 7, 2024

#3115 introduced the inherit_variants! macro which shares variants between enum AST types.

As Boshen pointed out in #3115 (comment), 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:

  • Add #![pre_expand] attr to top of all files.
  • Build script parses all files in crate.
  • Build script expands all macros.
  • Build script serializes the TokenStreams post-expansion for edited files and writes to temp file.
  • pre_expand proc macro ignores its input. It deserializes the TokenStream generated by build script and returns it.

This has the following advantages:

  • pre_expand macro does not parse its input, so doesn't need syn, or any expensive operations.
  • TokenStream includes span info, so it's as close to source maps as can get.
  • "Jump to definition" would take you direct to the original source code pre-expanded.
  • No macro expansion cost at compile time.

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.

  • Build script parses all the files in oxc_ast/src/ast. It outputs a schema of all the AST types to a temp folder.
  • #[inherit_variants] proc macro reads the types schemas which build script created, and uses it to add inherited variants to the enum + generate the rest of the impls e.g. is_statement.

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

  • AST transfer (which already builds such a schema, but at runtime)
  • Building a utility for running queries on types e.g. "which types are larger than 64 bytes?"
  • Building a visualization of the AST's structure and how all the AST types connect/depend on each other.
@Boshen Boshen assigned Boshen and unassigned Boshen May 13, 2024
@Boshen Boshen transferred this issue from oxc-project/oxc May 18, 2024
@Boshen Boshen removed their assignment May 18, 2024
@overlookmotel overlookmotel transferred this issue from oxc-project/backlog Jul 16, 2024
@Boshen
Copy link
Member

Boshen commented Aug 5, 2024

Seems fixed?

image

@overlookmotel
Copy link
Contributor Author

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.

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