feat(formatter): correct ast_nodes generator #12864
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
A-ast-tools
This was referenced Aug 7, 2025
Merged
Member
Author
CodSpeed Instrumentation Performance ReportMerging #12864 will not alter performanceComparing Summary
|
Dunqing
force-pushed
the
08-07-feat_formatter_correct_formatting_for_trystatement
branch
from
a5bd6b4 to
dd5283c
Compare
Dunqing
force-pushed
the
08-07-feat_formatter_correct_ast_nodes_generator
branch
from
30dcb9e to
de8558f
Compare
Member
Merge activity
|
Dunqing
force-pushed
the
08-07-feat_formatter_correct_formatting_for_trystatement
branch
from
dd5283c to
76de684
Compare
Dunqing
force-pushed
the
08-07-feat_formatter_correct_ast_nodes_generator
branch
from
de8558f to
40affe9
Compare
graphite-app
bot
force-pushed
the
08-07-feat_formatter_correct_formatting_for_trystatement
branch
from
76de684 to
56ecd0a
Compare
graphite-app
bot
force-pushed
the
08-07-feat_formatter_correct_ast_nodes_generator
branch
from
40affe9 to
00f736a
Compare
graphite-app
bot
force-pushed
the
08-07-feat_formatter_correct_formatting_for_trystatement
branch
from
56ecd0a to
ce27298
Compare
graphite-app
bot
force-pushed
the
08-07-feat_formatter_correct_ast_nodes_generator
branch
from
00f736a to
8316ceb
Compare
graphite-app
bot
force-pushed
the
08-07-feat_formatter_correct_formatting_for_trystatement
branch
from
ce27298 to
248fd94
Compare
graphite-app
bot
force-pushed
the
08-07-feat_formatter_correct_ast_nodes_generator
branch
from
8316ceb to
bfacd66
Compare
Base automatically changed from
08-07-feat_formatter_correct_formatting_for_trystatement
to
main
August 7, 2025 15:17
graphite-app
bot
deleted the
08-07-feat_formatter_correct_ast_nodes_generator
branch
This was referenced Aug 9, 2025
graphite-app bot
pushed a commit
that referenced
this pull request
Aug 9, 2025
Follow-on after #12864. Pure refactor, does not alter generated code. Shorten code by using `Schema::type_by_name` and `EnumDef::all_variants` methods. `EnumDef::all_variants` iterates over all variants of an enum, including inherited variants. As well as being shorter, it also correctly handles multiple layers of inheritance e.g. if `X` inherits variants from `Y`, and `Y` inherits variants from `Z`. In the case of `Statement`, inheritance is only 1 level deep, so it makes no difference - but it might if we change the AST later on.
graphite-app bot
pushed a commit
that referenced
this pull request
Aug 9, 2025
…`TypeId`s (#12930) Follow-on after #12864. Pure refactor - does not alter generated code. There's a couple of things going on in this PR. Firstly, perf: * Prefer comparing `TypeId`s over string comparison, because it's cheaper. * Compile a single `Vec` of all types which have no following node at the start (including those listed in `AST_NODE_WITHOUT_FOLLOWING_NODE_LIST`), rather than checking 2 `Vec`s each time in `generate_struct_impls`. Secondly, the code previously was relying on the fact that almost all of `Statement`s variants have the same name as the types those variants contain e.g.: ```rs pub enum Statement<'a> { BlockStatement(Box<'a, BlockStatement<'a>>), // ... } ``` But that the names match is a bit of a co-incidence. This PR makes it so we don't rely on that co-incidence, and instead makes the "no following node" list from the actual *types* of the variants. This change reveals a side-effect of the previous behavior which may or may not be intentional. The only variants of `Statement` where the name of the variant and name of the *type* of the variant don't match are `Function` and `Class`. To maintain the same output as before, I've added an exclude list `AST_NODE_WITH_FOLLOWING_NODE_LIST` containing these 2 types. Presumably the reason why these 2 need an exclusion is because `Function` and `Class` can be either a `Statement` or an `Expression`. I don't know if this may be problematic and might need logic elsewhere to handle them differently, depending on the context? Note: `TypeDef::innermost_type` is to get the `TypeId` of the inner type of a variant (e.g. `BlockStatement`), rather than the type which is the actual enum variant (e.g. `Box<BlockStatement>`). @Dunqing I don't know the formatter at all, so these changes may be unhelpful. Feel free to close or modify this PR if so.
taearls
pushed a commit
to taearls/oxc
that referenced
this pull request
Aug 12, 2025
taearls
pushed a commit
to taearls/oxc
that referenced
this pull request
Aug 12, 2025
Follow-on after oxc-project#12864. Pure refactor, does not alter generated code. Shorten code by using `Schema::type_by_name` and `EnumDef::all_variants` methods. `EnumDef::all_variants` iterates over all variants of an enum, including inherited variants. As well as being shorter, it also correctly handles multiple layers of inheritance e.g. if `X` inherits variants from `Y`, and `Y` inherits variants from `Z`. In the case of `Statement`, inheritance is only 1 level deep, so it makes no difference - but it might if we change the AST later on.
taearls
pushed a commit
to taearls/oxc
that referenced
this pull request
Aug 12, 2025
…`TypeId`s (oxc-project#12930) Follow-on after oxc-project#12864. Pure refactor - does not alter generated code. There's a couple of things going on in this PR. Firstly, perf: * Prefer comparing `TypeId`s over string comparison, because it's cheaper. * Compile a single `Vec` of all types which have no following node at the start (including those listed in `AST_NODE_WITHOUT_FOLLOWING_NODE_LIST`), rather than checking 2 `Vec`s each time in `generate_struct_impls`. Secondly, the code previously was relying on the fact that almost all of `Statement`s variants have the same name as the types those variants contain e.g.: ```rs pub enum Statement<'a> { BlockStatement(Box<'a, BlockStatement<'a>>), // ... } ``` But that the names match is a bit of a co-incidence. This PR makes it so we don't rely on that co-incidence, and instead makes the "no following node" list from the actual *types* of the variants. This change reveals a side-effect of the previous behavior which may or may not be intentional. The only variants of `Statement` where the name of the variant and name of the *type* of the variant don't match are `Function` and `Class`. To maintain the same output as before, I've added an exclude list `AST_NODE_WITH_FOLLOWING_NODE_LIST` containing these 2 types. Presumably the reason why these 2 need an exclusion is because `Function` and `Class` can be either a `Statement` or an `Expression`. I don't know if this may be problematic and might need logic elsewhere to handle them differently, depending on the context? Note: `TypeDef::innermost_type` is to get the `TypeId` of the inner type of a variant (e.g. `BlockStatement`), rather than the type which is the actual enum variant (e.g. `Box<BlockStatement>`). @Dunqing I don't know the formatter at all, so these changes may be unhelpful. Feel free to close or modify this PR if so.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.