Skip to content

feat(ast)!: Re-introduce TSEnumBody AST node#10284

Merged
overlookmotel merged 4 commits intomainfrom
ast/tsenumbody
Apr 9, 2025
Merged

feat(ast)!: Re-introduce TSEnumBody AST node#10284
overlookmotel merged 4 commits intomainfrom
ast/tsenumbody

Conversation

@leaysgur
Copy link
Member

@leaysgur leaysgur commented Apr 7, 2025

Fixes #10087


Follow up for #10017 .

This PR eliminates the part where nodes were manually generated for JS-land AST.

NOTE: This node was originally present, but was removed at #2509 and is now being reintroduced to get the correct Span position for #9705 .

@github-actions github-actions bot added A-linter Area - Linter A-parser Area - Parser A-semantic Area - Semantic A-ast Area - AST A-transformer Area - Transformer / Transpiler A-codegen Area - Code Generation A-isolated-declarations Isolated Declarations A-formatter Area - Formatter C-enhancement Category - New feature or request labels Apr 7, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Apr 7, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 7, 2025

CodSpeed Instrumentation Performance Report

Merging #10284 will not alter performance

Comparing ast/tsenumbody (135297b) with main (5850a0d)

Summary

✅ 36 untouched benchmarks

@leaysgur leaysgur marked this pull request as ready for review April 7, 2025 08:11
Copy link
Member

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

It would make sense to move the scope from TSEnumDeclaration to TSEnumBody. But let's leave that for a follow-up PR.

Apart from my notes on codegen, all looks good. Thank you!

@leaysgur
Copy link
Member Author

leaysgur commented Apr 8, 2025

It would make sense to move the scope from TSEnumDeclaration to TSEnumBody. But let's leave that for a follow-up PR.

🙆🏻

Thanks as always for your thoughtful review~!

Copy link
Member

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

Looks good to me!

@overlookmotel
Copy link
Member

overlookmotel commented Apr 9, 2025

@Boshen or @Dunqing do you want to take a look? Or merge it?

I've been through it fairly carefully, but I'm not familiar with codegen. It might need a p.add_source_mapping(self.span); somewhere?

@Dunqing
Copy link
Member

Dunqing commented Apr 9, 2025

@Boshen or @Dunqing do you want to take a look? Or merge it?

I've been through it fairly carefully, but I'm not familiar with codegen. It might need a p.add_source_mapping(self.span); somewhere?

Me either!

@github-actions github-actions bot added A-cli Area - CLI A-minifier Area - Minifier A-cfg Area - Control Flow Graph A-ast-tools Area - AST tools A-editor Area - Editor and Language Server labels Apr 9, 2025
@overlookmotel
Copy link
Member

overlookmotel commented Apr 9, 2025

Boshen says no need for p.add_source_mapping(self.span); on TS nodes. We have no use for sourcemaps when printing TS at present.

Have rebased on latest main and fixed conflicts. Will merge once CI passes.

@overlookmotel overlookmotel merged commit 49732ff into main Apr 9, 2025
29 checks passed
@overlookmotel overlookmotel deleted the ast/tsenumbody branch April 9, 2025 11:06
This was referenced Apr 14, 2025
Boshen added a commit that referenced this pull request Jan 16, 2026
Move `scope_id: Cell<Option<ScopeId>>` from `TSEnumDeclaration` to
`TSEnumBody` for better semantic alignment. Since `TSEnumBody` was
introduced as a dedicated type in PR #10284, the scope logically
belongs on the body rather than on the parent declaration node.

Closes #10331

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
graphite-app bot pushed a commit that referenced this pull request Jan 16, 2026
## Summary

- Move `scope_id: Cell<Option<ScopeId>>` from `TSEnumDeclaration` to `TSEnumBody`
- Since `TSEnumBody` was introduced as a dedicated type in PR #10284, the scope logically belongs on the body rather than on the parent declaration node

Closes #10331

## Test plan

- [x] Unit tests pass (`just test`)
- [x] Conformance tests pass (`just conformance`)
- [x] Updated semantic test snapshots reflect scope now on `TSEnumBody`

🤖 Generated with [Claude Code](https://claude.ai/code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST A-ast-tools Area - AST tools A-cfg Area - Control Flow Graph A-cli Area - CLI A-codegen Area - Code Generation A-editor Area - Editor and Language Server A-formatter Area - Formatter A-isolated-declarations Isolated Declarations A-linter Area - Linter A-minifier Area - Minifier A-parser Area - Parser A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ast: TSEnumBody should be revived?

3 participants