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

feat(ast_tools): add dedicated Derive trait. #5278

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

rzvxa
Copy link
Contributor

@rzvxa rzvxa commented Aug 27, 2024

In an effort toward the implementation of #5256, this PR allows us to have a separately generated "derive" file for each crate.
This also eliminates a bunch of boilerplate when writing new "derive" generators and generally makes it more approachable.

@github-actions github-actions bot added A-ast Area - AST A-ast-tools Area - AST tools labels Aug 27, 2024
Copy link

codspeed-hq bot commented Aug 27, 2024

CodSpeed Performance Report

Merging #5278 will not alter performance

Comparing 08-28-feat_ast_tools_add_dedicated_derive_trait (68a1c01) with main (be1a6d4)

Summary

✅ 28 untouched benchmarks

Copy link

graphite-app bot commented Aug 27, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from c1aaa07 to 941c2f5 Compare August 27, 2024 22:55
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch 2 times, most recently from 530fbc2 to 2da2ff8 Compare August 28, 2024 19:51
@rzvxa rzvxa marked this pull request as ready for review August 28, 2024 19:56
@rzvxa rzvxa requested a review from overlookmotel August 28, 2024 19:56
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from 2da2ff8 to 19d8106 Compare August 28, 2024 20:52
@rzvxa rzvxa changed the base branch from main to 08-28-feat_regular_expression_implement_display_for_regularexpression_type August 28, 2024 20:52
@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from facde65 to daa5001 Compare August 29, 2024 08:33
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from 19d8106 to c70e979 Compare August 29, 2024 08:33
@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from daa5001 to 9c5363f Compare August 29, 2024 08:42
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch 3 times, most recently from 7919ebb to b590a23 Compare August 29, 2024 16:02
@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from 1e1e0e6 to 7367de6 Compare August 29, 2024 16:30
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch 2 times, most recently from 041aa22 to dc5e509 Compare August 29, 2024 16:40
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Aug 30, 2024 — with Graphite App
@overlookmotel
Copy link
Contributor

Just to say sorry I've not got to reviewing this yet. Have been going hell-for-leather on the transformer and semantic. Hope I can get to it this evening.

Copy link
Contributor

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really good. It simplifies writing a new #[generate_derive] a lot. I can imagine us writing a bunch more in future - it's a really fast way to build something out which would take days and days to do by hand. The comments I've made are largely nits.

tasks/ast_tools/src/codegen.rs Outdated Show resolved Hide resolved
tasks/ast_tools/src/derives/mod.rs Show resolved Hide resolved
tasks/ast_tools/src/derives/mod.rs Show resolved Hide resolved
tasks/ast_tools/src/derives/mod.rs Show resolved Hide resolved
tasks/ast_tools/src/generators/ast_builder.rs Outdated Show resolved Hide resolved
tasks/ast_tools/src/generators/ast_builder.rs Outdated Show resolved Hide resolved
@overlookmotel
Copy link
Contributor

overlookmotel commented Aug 30, 2024

One broader thing...

The one thing that could confuse people is visibility. We're used to code generated by #[derive] sitting next to the struct/enum it's derived on, and having access to everything you can "see" from that position. Obviously #[generated_derive] doesn't work like that.

I don't think much we can do about that, but we should document that you have to make structs/enums/fields pub(crate) if you're using #[generated_derive], so people at least understand how to use it.

Or (and very possibly this is a stupid idea), could we make it work like #[derive]? e.g.:

  • Generate 1 file of generated code for each source file (not one for each crate).
  • Introduce a #[derive_generator] macro.

Then you use #[generated_derive] as follows:

// src/foo.rs
#[derive_generator]
use oxc_allocator::CloneIn;

#[generated_derive(CloneIn)]
struct Foo { // Not pub
    blah: u64 // Not pub
}

#[derive_generator] would expand to:

include!("generated/derive_clonein_foo.rs");

Then the generated code has access to everything in the source file without having to make anything pub (still wouldn't work for a struct which is defined inside a private module {} block, but that's a niche case).

This approach has pluses and minuses... maybe it solves one problem but creates another!

@overlookmotel
Copy link
Contributor

overlookmotel commented Aug 30, 2024

One other thing. At some point, maybe we should consider breaking #[generated_derive] out into a stand-alone thing, separate from #[ast]. We may well want to derive traits on types which are not part of the AST.

And... maybe this is completely bananas, but with this Derive trait, is there anything stopping us defining the derives in the source crates along with the traits they generate code for? e.g.:

// oxc_allocator/src/clone_in.rs

#[cfg(feature = "generate")]
use oxc_ast_tools::{Derive, TypeDef, LateCtx, TokenStream};

pub trait CloneIn {
    type Cloned;
    fn clone_in(&self, allocator: &'new_alloc Allocator) -> Self::Cloned;
}

#[cfg(feature = "generate")]
impl Derive for CloneIn {
    fn derive(&mut self, def: &TypeDef, _: &LateCtx) -> TokenStream {
        match &def {
            TypeDef::Enum(it) => derive_enum(it),
            TypeDef::Struct(it) => derive_struct(it),
        }
    }
}

// etc...

It's always been impossible to define your #[derive] macro in same crate as the trait it derives, but that's just due to a quirk of how Rust's proc macros work. I don't think we're constrained in the same way.

@overlookmotel overlookmotel removed the 0-merge Merge with Graphite Merge Queue label Aug 30, 2024
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch 2 times, most recently from 2c3dbe4 to bcde7b1 Compare September 1, 2024 16:21
@overlookmotel overlookmotel added 0-merge Merge with Graphite Merge Queue and removed 0-merge Merge with Graphite Merge Queue labels Sep 2, 2024
@oxc-project oxc-project deleted a comment from graphite-app bot Sep 2, 2024
@overlookmotel
Copy link
Contributor

Since #5304 is blocked at present, would you like to split this and #5280 into a separate stack, so we can get them merged now?

Concerning my idea of using include!() to generate code in situ, I guess the other option is one we've discussed before - the codegen passing a serialized TokenStream to the macro to insert. Maybe that's a better solution (when we can get it working) - though I do like that with generated code you have it committed to the repo and it's visible, rather than being hidden away behind a macro expansion.

@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from bcde7b1 to 241b549 Compare September 2, 2024 22:39
@rzvxa
Copy link
Contributor Author

rzvxa commented Sep 2, 2024

Since #5304 is blocked at present, would you like to split this and #5280 into a separate stack, so we can get them merged now?

Let's not do that, I don't want to mess up the Graphite stack. The down stack should be ready to merge. I'll request a review after running actions.


What I like about include is the fact that we can have our code outside of the src directory(I'm 90% sure, Haven't tested it), This would allow us to put these stuff in a hidden directory so it isn't accessible from our code base in any other way. If we want to hide away the process then having these files in the generated directory might be more confusing than helpful. It's not like accessing oxc_crate_name/.generated_derives/ is hard.

It is worth having an issue tracking this idea in the backlog. We have to do a quick test to make sure it's practical. There might be some quirks with how crates work.

Regarding the latter idea, My personal take is having derives in a separate project is "cleaner" than having them with a conditional feature flag and bloating our core projects with bootstrapping flags. We could use a third person pitching in, Technically speaking it is possible to implement I'm not sure if it would result in a better DX.

@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from b92d023 to 7c5d4b2 Compare September 2, 2024 23:01
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from 241b549 to 641d9e6 Compare September 2, 2024 23:02
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from 641d9e6 to 4ea0d1f Compare September 3, 2024 01:51
@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from 1d0bc62 to 3aaf2bf Compare September 3, 2024 02:12
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from f9a67d8 to 0be2ab4 Compare September 3, 2024 02:12
@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from 3aaf2bf to a5ba8b5 Compare September 3, 2024 02:15
@rzvxa rzvxa force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from 0be2ab4 to d7b3aaf Compare September 3, 2024 02:15
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Sep 3, 2024 — with Graphite App
@Boshen Boshen changed the base branch from 08-28-feat_regular_expression_implement_display_for_regularexpression_type to graphite-base/5278 September 3, 2024 02:20
@Boshen Boshen changed the base branch from graphite-base/5278 to main September 3, 2024 02:29
Boshen pushed a commit that referenced this pull request Sep 3, 2024
In an effort toward the implementation of #5256, this PR allows us to have a separately generated "derive" file for each crate.
This also eliminates a bunch of boilerplate when writing new "derive" generators and generally makes it more approachable.
@Boshen Boshen force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from d7b3aaf to aba6e88 Compare September 3, 2024 02:30
In an effort toward the implementation of #5256, this PR allows us to have a separately generated "derive" file for each crate.
This also eliminates a bunch of boilerplate when writing new "derive" generators and generally makes it more approachable.
@Boshen Boshen force-pushed the 08-28-feat_ast_tools_add_dedicated_derive_trait branch from aba6e88 to 68a1c01 Compare September 3, 2024 02:36
@graphite-app graphite-app bot merged commit 68a1c01 into main Sep 3, 2024
23 checks passed
@graphite-app graphite-app bot deleted the 08-28-feat_ast_tools_add_dedicated_derive_trait branch September 3, 2024 02:46
@overlookmotel
Copy link
Contributor

Regarding the latter idea, My personal take is having derives in a separate project is "cleaner" than having them with a conditional feature flag and bloating our core projects with bootstrapping flags. We could use a third person pitching in, Technically speaking it is possible to implement I'm not sure if it would result in a better DX.

I suppose the reason I thought of this is I recently added another generated file to oxc_traverse: #5119

This generator is pretty simple and lightweight, and it felt like little generators like this would sit quite naturally in the crate in which the generated code sits, so you have the generator and generated code next to each other.

I can imagine writing many more of these (but in Rust, not JS!), and the codebase ending up peppered with them. At that point oxc_ast_tools could become a bit of a monolith.

I'm not 100% pushing that we should do this, just explaining where I'm coming from.

This was referenced Sep 5, 2024
Boshen added a commit that referenced this pull request Sep 6, 2024
## [0.27.0] - 2024-09-06

- bd820f9 semantic: [**BREAKING**] Remove
`SymbolTable::get_symbol_id_from_name` and
`SymbolTable::get_scope_id_from_name` (#5480) (overlookmotel)

- cba93f5 ast: [**BREAKING**] Add `ThisExpression` variants to
`JSXElementName` and `JSXMemberExpressionObject` (#5466) (overlookmotel)

- 87c5df2 ast: [**BREAKING**] Rename `Expression::without_parentheses`
(#5448) (overlookmotel)

### Features

- e8bdd12 allocator: Add `AsMut` impl for `Box` (#5515) (overlookmotel)
- 90facd3 ast: Add `ContentHash` trait; remove noop `Hash`
implementation from `Span` (#5451) (rzvxa)
- 23285f4 ast: Add `ContentEq` trait. (#5427) (rzvxa)
- 59abf27 ast, parser: Add `oxc_regular_expression` types to the parser
and AST. (#5256) (rzvxa)
- 68a1c01 ast_tools: Add dedicated `Derive` trait. (#5278) (rzvxa)
- c782916 codegen: Print `type_parameters` in `TaggedTemplateExpression`
(#5438) (Dunqing)
- 4cb63fe index: Impl rayon related to trait for IndexVec (#5421)
(IWANABETHATGUY)
- ba4b68c minifier: Remove parenthesized expression for dce (#5439)
(Boshen)
- ed8ab6d oxc: Conditional expose `oxc_cfg` in `oxc` crate (#5524)
(IWANABETHATGUY)
- 91b39c4 oxc_diagnostic: Impl DerefMut for OxcDiagnostic (#5474)
(IWANABETHATGUY)
- 10279f5 parser: Add syntax error for hyphen in `JSXMemberExpression`
`<Foo.bar-baz />` (#5440) (Boshen)
- 0f50b1e semantic: Check for initializers in ambient
`VariableDeclaration`s (#5463) (DonIsaac)
- 62f7fff semantic: Check for non-declared, non-abstract object
accessors without bodies (#5461) (DonIsaac)
- 5407143 semantic: Check for non-declared, non-abstract class accessors
without bodies (#5460) (DonIsaac)
- 052e94c semantic: Check for parameter properties in constructor
overloads (#5459) (DonIsaac)
- 32d4bbb transformer: Add `TransformOptions::enable_all` method (#5495)
(Boshen)
- c59d8b3 transformer: Support all /regex/ to `new RegExp` transforms
(#5387) (Dunqing)
- cedf7a4 xtask: Impl `as_ast_kind` method for each variant (#5491)
(IWANABETHATGUY)

### Bug Fixes

- 0df1d9d ast, codegen, linter: Panics in fixers. (#5431) (rzvxa)
- fce549e diagnostics: Ignore `Interrupted` and `BrokenPipe` errors
while printing (#5526) (Boshen)
- ea7a52f napi/transform: Fix test (Boshen)
- 9b984b3 regex: Panic on displaying surrogated `UnicodeEscape`
characters. (#5469) (rzvxa)
- 88b7ddb regular_expression: Handle unterminated character class
(#5523) (leaysgur)
- 7a797ac semantic: Incorrect reference when `MemberExpression` used in
`TSPropertySignature` (#5525) (Dunqing)
- d8b9909 semantic: `IdentifierReference` within `TSPropertySignature`
cannot reference type-only import binding (#5441) (Dunqing)
- 8f9627d transformer: RegExp transform do not transform invalid regexps
(#5494) (overlookmotel)
- 2060efc transformer: RegExp transform don't transform all RegExps
(#5486) (overlookmotel)
- cfe5497 transformer: Do not create double reference in JSX transform
(#5414) (overlookmotel)
- 0617249 transformer/nullish-coalescing-operator: Incorrect reference
flags (#5408) (Dunqing)
- 0eb32a6 traverse: Invalid variable name generated by
`generate_uid_based_on_node` (#5407) (Dunqing)- b96bea4 Add back
lifetime (#5507) (IWANABETHATGUY)

### Performance

- bfabd8f syntax: Further optimize `is_identifier_name` (#5426)
(overlookmotel)
- aeda84f syntax: Optimize `is_identifier_name` (#5425) (overlookmotel)
- ed8937e transformer: Memoize rope instance (#5518) (Dunqing)
- bfab091 transformer: Store needed options only on `RegExp` (#5484)
(overlookmotel)
- b4765af transformer: Pre-calculate if unsupported patterns in RegExp
transform (#5483) (overlookmotel)
- 182ab91 transformer: Pre-calculate unsupported flags in RegExp
transform (#5482) (overlookmotel)

### Documentation

- 64db1b4 ast: Clarify docs for `RegExpPattern` (#5497) (overlookmotel)
- 3f204a9 span: Update docs about `ContentEq` `Vec` comparison speed
(#5478) (overlookmotel)- 00511fd Use `oxc_index` instead of `index_vec`
in doc comments (#5423) (IWANABETHATGUY)

### Refactor

- 9f6e0ed ast: Simplify `ContentEq` trait definition. (#5468) (rzvxa)
- a43e951 ast: Use loop instead of recursion (#5447) (overlookmotel)
- 2224cc4 ast: Renumber `JSXMemberExpressionObject` discriminants
(#5464) (overlookmotel)
- a952c47 ast: Use loop not recursion (#5449) (overlookmotel)
- d9d7e7c ast: Remove `IdentifierName` from `TSThisParameter` (#5327)
(overlookmotel)
- ccc8a27 ast, ast_tools: Use full method path for generated derives
trait calls. (#5462) (rzvxa)
- fdb8857 linter: Use "parsed pattern" in `no_div_regex` rule. (#5417)
(rzvxa)
- e7bd49d regular_expression: Correct typo (#5429) (overlookmotel)
- e4ed41d semantic: Change the reference flag to `ReferenceFlags::Type`
if it is used within a `TSTypeQuery` (#5444) (Dunqing)
- 94a6ac6 span: Use `Hasher` from `std` (#5476) (overlookmotel)
- b47aca0 syntax: Use `generate_derive` for `CloneIn` in types outside
of `oxc_ast` crate. (#5280) (rzvxa)
- a96866d transformer: Re-order imports (#5499) (overlookmotel)
- 6abde0a transformer: Clarify match in RegExp transform (#5498)
(overlookmotel)
- 09c522a transformer: RegExp transform report pattern parsing errors
(#5496) (overlookmotel)
- dd19823 transformer: RegExp transform do not take ownership of
`Pattern` then reallocate it (#5492) (overlookmotel)
- 2514cc9 transformer/react: Move all entry points to implementation of
Traverse trait (#5473) (Dunqing)
- c984219 transformer/typescript: Move all entry points to
implementation of Traverse trait (#5422) (Dunqing)

### Styling

- 2a43fa4 linter: Introduce the writing style from PR #5491 and reduce
the if nesting (#5512) (dalaoshu)

### Testing

- 340b535 linter/no-unused-vars: Arrow functions in tagged templates
(#5510) (Don Isaac)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-ast Area - AST A-ast-tools Area - AST tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants