feat(ast_codegen, span): process SourceType through ast_codegen.

[rzvxa]

No description provided.

[rzvxa]

• feat(ast_codegen, span): process SourceType through ast_codegen. #4696 👈
• feat(ast_codegen): process AST-related syntax types. #4694
• refactor(ast_codegen): disallow implicit enum discriminants on #[ast] types. #4690
• refactor(ast_codegen): move away from RType in generators. #4682
• refactor(syntax): add explicit enum discriminants to AST related types. #4691
• refactor(ast): add explicit enum discriminants. #4689
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rzvxa and the rest of your teammates on Graphite

[rzvxa]

@overlookmotel this stack should be ready for review.

[codspeed-hq]

CodSpeed Performance Report

Merging #4696 will not alter performance

_{Comparing 08-06-feat_ast_codegen_span_process_sourcetype_through_ast_codegen (125c5fd) with main (3f3cb62)}

Summary

✅ 29 untouched benchmarks

[overlookmotel]

Great to have these types included in schema.

Personally I wouldn't have split the types into a separate file. Is the rationale for that to give the codegen less code to chew through looking for items with an #[ast] attr? If so, I wouldn't worry too much about that - the file is not so large, and it's no big deal if the codegen isn't quite as quick as it could be.

It's annoying not to be able to assert that field offsets are correct because the fields are private. Perhaps at some point we should add a generated folder to oxc_syntax and oxc_span crates, and generate the assertions there so they can access the private fields. We'd need to do that for CloneIn and Serialize traits if we want to codegen their impls for SourceType.

[overlookmotel]

One last thing: Since oxc_span crate now depends on oxc_ast_macros, should we put #[ast] on Span as well?

[rzvxa]

Great to have these types included in schema.

Personally I wouldn't have split the types into a separate file. Is the rationale for that to give the codegen less code to chew through looking for items with an #[ast] attr? If so, I wouldn't worry too much about that - the file is not so large, and it's no big deal if the codegen isn't quite as quick as it could be.

I intentionally panic on unrelated items in the source read by codegen. It isn't for performance purposes, I don't care for performance in the codegen.

It lowers the clutter in the rust definition files, I would even suggest going as far as changing the file extension to something like XXX.d.rs. Otherwise, we have no way of reasoning about what is and isn't included in the codegen(unless you read the source code).

It's annoying not to be able to assert that field offsets are correct because the fields are private. Perhaps at some point we should add a generated folder to oxc_syntax and oxc_span crates, and generate the assertions there so they can access the private fields. We'd need to do that for CloneIn and Serialize traits if we want to codegen their impls for SourceType.

Yeah, Currently I'm working on the generate_derive marker and the CloneIn trait. One limitation of our current approach is that we output one file per generator. That means we get only one output derive_clone_in.rs file.

We used to have multiple outputs from a single generator but the implementation was no good. We need a way to have one generator per crate or one output per type and fold it into crate/module boundaries.

We might not even need such a thing. In general, I prefer we don't invest time on cross-crate compilation, We shouldn't expand codegen far out of AST. Otherwise, we start generating code all over the place and bloat the project.

I can extract the pre-expanded derives as its own thing if we see a reasonable improvement over vanilla derives.

[rzvxa]

One last thing: Since oxc_span crate now depends on oxc_ast_macros, should we put #[ast] on Span as well?

Sure.

[graphite-app]

Merge activity

• Aug 6, 1:40 PM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Aug 6, 1:41 PM EDT: overlookmotel added this pull request to the Graphite merge queue.
• Aug 6, 1:46 PM EDT: overlookmotel merged this pull request with the Graphite merge queue.

[overlookmotel]

Otherwise, we have no way of reasoning about what is and isn't included in the codegen(unless you read the source code).

Personally, I think it's fine to just have #[ast] to inform the reader what it being altered by the codegen. I also like your idea of using #[generate_derive(CloneIn)] - again, that makes it explicit that some code is being generated for the type, rather than it just happening invisibly.

I'll make a PR to add #[ast] to Span.

[rzvxa]

Otherwise, we have no way of reasoning about what is and isn't included in the codegen(unless you read the source code).

Personally, I think it's fine to just have #[ast] to inform the reader what it being altered by the codegen. I also like your idea of using #[generate_derive(CloneIn)] - again, that makes it explicit that some code is being generated for the type, rather than it just happening invisibly.

The problem is when you use #[ast] on something expecting it to work similarly to all places you've seen this marker, But it doesn't; The #[ast] itself isn't the whole picture. we don't know if it is even included in the codegen or not as long as we haven't checked the main file of the codegen.

In the #[ast] macro we can panic if the file isn't already processed by the codegen, which just complicates the compilation, However, it is possible to use it to raise compile errors on illegal usage.

It means when you add #[ast] to a new type you can't build the crate as long as you haven't run the just ast command.

I'll make a PR to add #[ast] to Span.

Thank you!

[overlookmotel]

In the #[ast] macro we can panic if the file isn't already processed by the codegen, which just complicates the compilation, However, it is possible to use it to raise compile errors on illegal usage.

I think this would be a good idea. Opened oxc-project/backlog#124 to discuss.