Skip to content

fix(ast/estree): Add TSEnumBody to TSEnumDeclaration.body#10017

Merged
overlookmotel merged 14 commits intomainfrom
estree-tsenumbody
Apr 3, 2025
Merged

fix(ast/estree): Add TSEnumBody to TSEnumDeclaration.body#10017
overlookmotel merged 14 commits intomainfrom
estree-tsenumbody

Conversation

@leaysgur
Copy link
Member

@leaysgur leaysgur commented Mar 24, 2025

Part of #9705

This PR fixes AST structure of TSEnumDeclaration from:

{
  "type": "TSEnumDeclaration",
  "id": { /* ... */ },
  "members": [
    {
      "type": "TSEnumMember",
      "id": { /* ... */ },
      "initializer": null
    }
  ],
  "const": false,
  "declare": false
}

to:

{
  "type": "TSEnumDeclaration",
  "id": { /* ... */ },
  "body": {
    "type": "TSEnumBody",
    "members": [
	    {
	      "type": "TSEnumMember",
          "id": { /* ... */ },
	      "initializer": null
	    }
	  ],
  },
  "const": false,
  "declare": false
}

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/ast-spec/src/declaration/TSEnumDeclaration/spec.ts
https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/ast-spec/src/special/TSEnumBody/spec.ts

Historically, we had this node in the past, but it seems to have been deleted in #2509 .

@graphite-app
Copy link
Contributor

graphite-app bot commented Mar 24, 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.

@github-actions github-actions bot added A-ast Area - AST C-bug Category - Bug labels Mar 24, 2025
@leaysgur leaysgur changed the title fix(ast/estree): Add TSEnumBody to TSEnumDeclaration.body fix(ast/estree): Add TSEnumBody to TSEnumDeclaration.body Mar 24, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 24, 2025

CodSpeed Instrumentation Performance Report

Merging #10017 will degrade performances by 3.74%

Comparing estree-tsenumbody (387527b) with main (bde73b5)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 34 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
formatter[antd.js] 7.6 ms 7.9 ms -3.74%
formatter[typescript.js] 7.4 ms 7.2 ms +3.52%

@leaysgur leaysgur marked this pull request as ready for review March 25, 2025 01:47
@leaysgur
Copy link
Member Author

This change causes many tests to pass, but I've just realized that this implementation may not be sufficient...

I believe it might not work for code like this.

enum Foo        { A }
//       ^^^^^^^

I'm not sure how to handle these gaps in serialize step. Do we need to revive the TSEnumBody node in the Rust AST?

@overlookmotel
Copy link
Member

This change causes many tests to pass, but I've just realized that this implementation may not be sufficient...

I believe it might not work for code like this.

enum Foo        { A }
//       ^^^^^^^

I'm not sure how to handle these gaps in serialize step. Do we need to revive the TSEnumBody node in the Rust AST?

Yes, we'll probably need to alter the Rust AST so it includes the Span we need.

But this PR already makes more tests pass, so let's get it merged, and we can then improve on it.

@leaysgur
Copy link
Member Author

leaysgur commented Mar 26, 2025

@overlookmotel Thanks for the review~! I think I've fixed your points.

Yes, we'll probably need to alter the Rust AST so it includes the Span we need.
But this PR already makes more tests pass, so let's get it merged, and we can then improve on it.

🫡 I will create another issue for this later. => #10087

@leaysgur leaysgur requested a review from overlookmotel March 26, 2025 00:57
@overlookmotel overlookmotel merged commit f547d76 into main Apr 3, 2025
28 checks passed
@overlookmotel overlookmotel deleted the estree-tsenumbody branch April 3, 2025 07:42
@overlookmotel
Copy link
Member

Thank you!

overlookmotel pushed a commit that referenced this pull request Apr 9, 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 .

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants