Skip to content

refactor(ast/estree): re-order fields in visitation order#11362

Merged
graphite-app[bot] merged 1 commit intomainfrom
05-28-refactor_ast_estree_re-order_fields_in_visitation_order
May 29, 2025
Merged

refactor(ast/estree): re-order fields in visitation order#11362
graphite-app[bot] merged 1 commit intomainfrom
05-28-refactor_ast_estree_re-order_fields_in_visitation_order

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented May 28, 2025

Re-order fields in ESTree and TS-ESTree ASTs to be in visitation order (which is the order in which they're defined in Rust types).

This does not align with either Acorn or TS-ESLint, but for the reasons discussed in #9705 (comment), I feel this isn't a concern. The field order doesn't matter much per se, only that it's consistent, and this change has other benefits - it will make it simpler to build a visitor which visits fields in the correct order, and allows adding other parsers to acorn-test262 in order to produce test cases for decorators.

Visitation order aligns with TS-ESLint except for the following types:

  • ExportSpecifier
  • TSImportType
  • TSIndexedAccessType
  • TSMethodSignature
  • TSPropertySignature
  • TSTypePredicate

In these 6 cases, TS-ESLint's visitorKeys has the wrong visitation order, because it does not visit fields in source order. I will open an issue and hopefully they can fix.

This PR also bumps acorn-test262 to include oxc-project/estree-conformance#37 which re-orders the fields in fixture snapshots to match the new field order here.

Copy link
Member Author

overlookmotel commented May 28, 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.

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

@github-actions github-actions bot added A-ast Area - AST C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels May 28, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented May 28, 2025

CodSpeed Instrumentation Performance Report

Merging #11362 will create unknown performance changes

Comparing 05-28-refactor_ast_estree_re-order_fields_in_visitation_order (12690a1) with 05-28-refactor_ast_tools_estree_order_type_and_span_fields_first_by_default (1d1ebd6)

Summary

🆕 38 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 codegen[RadixUIAdoptionSection.jsx] N/A 132.2 µs N/A
🆕 codegen[binder.ts] N/A 4.8 ms N/A
🆕 codegen[cal.com.tsx] N/A 38.9 ms N/A
🆕 codegen[react.development.js] N/A 2.2 ms N/A
🆕 formatter[RadixUIAdoptionSection.jsx] N/A 298.7 µs N/A
🆕 formatter[binder.ts] N/A 18.9 ms N/A
🆕 formatter[cal.com.tsx] N/A 142.7 ms N/A
🆕 formatter[react.development.js] N/A 9.2 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 21.1 µs N/A
🆕 lexer[binder.ts] N/A 929.3 µs N/A
🆕 lexer[cal.com.tsx] N/A 5.8 ms N/A
🆕 lexer[react.development.js] N/A 383.6 µs N/A
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.8 ms N/A
🆕 linter[binder.ts] N/A 148.6 ms N/A
🆕 linter[cal.com.tsx] N/A 1.3 s N/A
🆕 linter[react.development.js] N/A 54.4 ms N/A
🆕 mangler[RadixUIAdoptionSection.jsx] N/A 14.2 µs N/A
🆕 mangler[binder.ts] N/A 811.2 µs N/A
🆕 mangler[cal.com.tsx] N/A 3 ms N/A
🆕 mangler[react.development.js] N/A 282.7 µs N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@overlookmotel overlookmotel marked this pull request as ready for review May 28, 2025 22:26
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label May 29, 2025
Copy link
Member

Boshen commented May 29, 2025

Merge activity

Re-order fields in ESTree and TS-ESTree ASTs to be in visitation order (which is the order in which they're defined in Rust types).

This does not align with either Acorn or TS-ESLint, but for the reasons discussed in #9705 (comment), I feel this isn't a concern. The field order doesn't matter much per se, only that it's consistent, and this change has other benefits - it will make it simpler to build a visitor which visits fields in the correct order, and allows adding other parsers to `acorn-test262` in order to produce test cases for decorators.

Visitation order aligns with TS-ESLint except for the following types:

* `ExportSpecifier`
* `TSImportType`
* `TSIndexedAccessType`
* `TSMethodSignature`
* `TSPropertySignature`
* `TSTypePredicate`

In these 6 cases, TS-ESLint's `visitorKeys` has the wrong visitation order, because it does not visit fields in source order. I will open an issue and hopefully they can fix.

This PR also bumps `acorn-test262` to include oxc-project/estree-conformance#37 which re-orders the fields in fixture snapshots to match the new field order here.
@graphite-app graphite-app bot force-pushed the 05-28-refactor_ast_tools_estree_order_type_and_span_fields_first_by_default branch from 5e1cb06 to 1d1ebd6 Compare May 29, 2025 02:21
@graphite-app graphite-app bot force-pushed the 05-28-refactor_ast_estree_re-order_fields_in_visitation_order branch from aaf25cb to 12690a1 Compare May 29, 2025 02:22
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label May 29, 2025
Base automatically changed from 05-28-refactor_ast_tools_estree_order_type_and_span_fields_first_by_default to main May 29, 2025 02:28
@graphite-app graphite-app bot merged commit 12690a1 into main May 29, 2025
25 checks passed
@graphite-app graphite-app bot deleted the 05-28-refactor_ast_estree_re-order_fields_in_visitation_order branch May 29, 2025 02:28
overlookmotel pushed a commit that referenced this pull request Jun 2, 2025
Part of #9705 , follow up #11362 

This change will generate the same AST shapes in all locations, regardless of `visitor-keys`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants