Align JS-side AST with standard for TypeScript #9705

overlookmotel · 2025-03-12T07:54:42Z

Our AST on JS side (via oxc-parser NPM package) is now aligned with ESTree standard for JS syntax. We have conformance tests which ensure our AST is exactly the same as Acorn's AST for all Test262 test cases which Acorn can parse.

However, we do not yet guarantee that our AST for TypeScript types is aligned with any particular standard.

TS-ESLint's parser seems like the obvious target.

Achieving alignment

Recently there have been various PRs to align parts of the AST with TS-ESLint.

In my opinion, it'd be preferable if we took a more systematic approach, using a conformance suite. This will ensure:

We don't miss anything, and can confidently assert our complete alignment with TS-ESLint.
We don't break anything in future.
While making changes to align one type, we don't inadvertently break another.

This methodology proved successful while working on ESTree comatibility. Once we had the conformance suite working, we got to 99% passing within a few days!

I suggest we adopt same approach as I've outlined in #9703, with the target snapshots generated in and committed to acorn-test262 repo. We can:

Use all the test cases in https://github.com/microsoft/TypeScript/tree/main/tests/cases/conformance and https://github.com/microsoft/TypeScript/tree/main/tests/cases/compiler.
Parse all these cases with TS-ESLint parser, convert the resulting ASTs to JSON, and commit those JSON files to the repo.

How

Wherever possible, it's preferable to use the DSL (domain specific language) provided by #[estree] attributes on AST types to control how the AST gets converted e.g. #[estree(flatten)], #[estree(rename = "...")], #[estree(append_to = "...")].

We can also use custom converters #[estree(via = ...)] where necessary, but the less we have to resort to that, the better.

Conflicts between ESTree and TS-ESLint

It's possible that we'll run into cases where Acorn and TS-ESLint produce a different AST for some JS syntax. If so, that'll be problematic. Ideally we want a single AST shape which is compatible with both ESTree and TS-ESLint.

If we do find such cases, I suggest that we just skip them initially. Once we have everything else passing, we can see how broad a problem this is, and figure out how best to deal with it.

Raw transfer

I suggest that in the inital alignment effort, we don't worry about raw transfer.

Let's only adapt the AST that Oxc generates as JSON via ESTree trait. This will be simpler to do, and will get us to the finish line faster.

Only once we have all the tests passing, we can then work on getting the AST generated by raw transfer aligned too. This is generally much easier once you have a working ESTree impl to translate from.

JSX

See #9703. I suggest we do JSX first before TS.

The text was updated successfully, but these errors were encountered:

overlookmotel · 2025-03-12T07:54:58Z

cc @ArnaudBarre @hi-ogawa

ArnaudBarre · 2025-03-12T08:24:35Z

I think you should target TS-ESLint for anything non supported by acorn, this will simplify the setup. JSX is small enough so that it will probably not conflict, but today most JSX code is TSX so IMO I think you can avoid the intermediary step.
Also TS-ESLint adopt JS features faster than acorn (the these two small diff I found, on top of acorn not yet handling decorators or import assertions) so IMO you should only target one popular parser (Babel would be a valid choice also)

overlookmotel · 2025-03-14T09:12:59Z

#9774 implements a TS conformance suite.

Continuing the conversation from that PR about current state of our conformance...

@hi-ogawa said:

Current result Positive Passed: 32/10725 (0.30%) 😢

Oh dear! Well, we had a similar result when we started on ESTree conformance, and within a few days we were at 99%. Maybe it'll be the same this time, inshallah.

But...

maybe keys are ordered alphabetically

In Literal nodes for numbers, TS-ESLint has the raw field before value. That also matches the pattern of alphabetical order that you noticed.

The order of keys doesn't really matter. What does matter is that it's consistent, to make operating on the objects in JS code efficient, as consistent object shapes allow JS engines to monomorphize functions that operate on those objects.

The other advantage of testing for correct field order is that conformance tests can compare the JSON produced by our serializer directly to the snapshotted JSON, with a simple string comparison. This is much faster than using serde to parse both JSONs to serde::Values, and comparing those structures. Before I made the switch to direct JSON string comparison, the conformance tests were impractically slow to run on CI.

I can see 2 possible ways forward:

Alter conformance tester to not be sensitive to field order, at least for now. We could disable testing TS conformance on CI, while we work on getting the AST aligned, and then deal with field order later.
Alter the AST in acorn-test262 before serializing to JSON. We'd re-order fields to be same order as ESTree (Acorn), with additional TS fields after that. To avoid having to add loads of #[estree(field_order(...))] attributes to AST types, we could alter the codegen in oxc_ast_tools to automatically put #[ts] fields last.

Perhaps (1) is a good place to start, and then we do (2) later on?

typescript-eslint parser uses undefined instead of null for optional ts-specific field

That's problematic. We could match that behavior in raw transfer without any perf hit, but obviously we can't when transferring the AST via JSON. We'd have to add a "fixup" pass on JS side to convert null to undefined. I imagine that'd be costly.

I think we need to keep the JSON-based transfer method alive for the foreseeable future. Raw transfer is experimental, and quite possible it has show-stopper bugs. We've already found it doesn't work on Node 20 or Bun, and we've not got it working on WASM yet.

I would also be loathe to remove empty TS-specific fields entirely in JS-side AST. That'd be a big perf hit for the consumer, as it'd result in variable object shapes.

I'd suggest the best starting point on this one is to modify the AST in acorn-test262 to convert all undefineds to nulls, prior to converting to JSON.

But does null vs undefined matter anyway? Will it affect downstream tools if we just go with null?

overlookmotel · 2025-03-14T09:22:05Z

@hi-ogawa Do you have write access to acorn-estree repo? If so, feel free to make changes there if required and merge your own PRs. I'm going to be away from keyboard until Monday, and don't want to block your progress if you're working during that time.

Part of #9705 Requires oxc-project/acorn-test262#6 Current result `Positive Passed: 32/10725 (0.30%)` 😢 What I observed from a few diffs I looked at: - typescript-eslint parser's field ordering is not a super set of acorn (maybe keys are ordered alphabetically) - example: `ClassDeclaration` - typescript-eslint https://ast-explorer.dev/#eNolyDEKgDAMBdCrlD/rBTyIOGQJJUOlpqUpgpTe3YjrG8jYcPLNFluqHQuqQ3+q/LCK5aSfR/eY2SwcYUxSp+I0SEMgSLx4l2apKGFzyNzFOoF0Yr7njiIU - abstract, body, declare, decorators, id, implements, superClass, superTypeArguments, typeParameters, - acorn https://ast-explorer.dev/#eNolyTEKwzAMRuGrmH/OCXKOUjpoEaqGFMcyllMIxnePQrbH9wYyVvz4zy5tqx0LagCLtRIt0ZLZPX3SmHSTBQ0qKRFUdn5r880KYQ3I3NU7YXm+29FEX2fVZ+/2PXI0lYl5ATuZJ1U= - id, superClass, body - typescript-eslint parser uses `undefined` instead of `null` for optional ts-specific field - example: `ClassDeclaration.typeParameters` - https://ast-explorer.dev/#eNolyDEKgDAMBdCrlD/rBTyIOGQJJUOlpqUpgpTe3YjrG8jYcPLNFluqHQuqQ3+q/LCK5aSfR/eY2SwcYUxSp+I0SEMgSLx4l2apKGFzyNzFOoF0Yr7njiIU - spec https://github.com/typescript-eslint/typescript-eslint/blob/2224f12ff05b59c5a459291fe93fc8099221089d/packages/ast-spec/src/base/ClassBase.ts#L62

hi-ogawa · 2025-03-14T09:28:18Z

@overlookmotel Yes, I saw a green merge button myself, so I should be able to push it. 👍

ArnaudBarre · 2025-03-14T09:29:45Z

You will have pre-processing to do anyway to convert TS-ESLint location to start/end. You could also do the null/undefined change during that process.
Personally for now I've done the diff in pure JS, to also have false, null, undefined and [] be considered equivalent, handle bigint non json serializable and don't throw on nullable keys from one or the other.

overlookmotel · 2025-03-14T09:47:47Z

Just to clarify one point:

Alter conformance tester to not be sensitive to field order, at least for now.

I mean alter conformance tester for TypeScript only. We have succeeded in getting field order aligned with Acorn (and I made some PRs on Acorn acornjs/acorn#1347 acornjs/acorn#1346 to make Acorn align with us!), so would be ideal not to accidentally regress that.

armano2 · 2025-03-16T18:32:31Z

AST of ts-eslint should be 100% aligned with estree (extended), but there is sometimes differences when new js features are added (stage 3) and there is small period of time where AST may differ
ts-eslint also does alignment check with @babel/eslint to ensure that consistency

But does null vs undefined matter anyway? Will it affect downstream tools if we just go with null?

yes to my knowledge there is few cases where null has actual meaning in estree but there is technically no difference in null | undefined as far as estree is concerned outside of semantics and depending what parser you use acron/babel/ts-eslint/esprima "nullish" value may be different

https://github.com/estree/estree/blob/master/es2015.md#expressions

eg.

[,2]
/// elements: [null, Literal]

https://typescript-eslint.io/play/#ts=5.7.3&showAST=es&fileType=.tsx&code=MYewdgzgLgBAHjAvDA2gGgEwF0g&eslintrc=N4KABGBEBOCuA2BTAzpAXGYBfEWg&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

additionally ts-eslint adds empty array's and defaults "flags" to false and eg. babel does not always do it for ts specific fields

overlookmotel · 2025-03-17T02:44:57Z

To avoid having to add loads of #[estree(field_order(...))] attributes to AST types, we could alter the codegen in oxc_ast_tools to automatically put #[ts] fields last.

I'm going to do this part now.

hi-ogawa · 2025-03-17T03:32:40Z

2. Alter the AST in acorn-test262 before serializing to JSON. We'd re-order fields to be same order as ESTree (Acorn), with additional TS fields after that.

I'm thinking about this, but to reorder this, we need to know acorn's order in js script of acorn-test262. Is there any easy-to-use format of estree spec providing this data? Either generating our own from ast tools or somehow extracting it from markdown https://github.com/estree/estree or dts https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/estree/index.d.ts?

For now I went with 2st option of normalizing oxc output #9813.

#9813) Part of #9705 This doesn't magically improve passes, but it makes it easier to digest mismatch diff. <details><summary>tasks/coverage/acorn-test262-diff/typescript/tests/cases/compiler/2dArrays.diff</summary> ```diff @@ -20,12 +20,12 @@ "type": "Identifier", "start": 6, "end": 10, - "decorators": [], - "name": "Cell", - "optional": false + "name": "Cell" }, - "implements": [], - "superClass": null + "implements": null, + "superClass": null, + "superTypeParameters": null, + "typeParameters": null }, { "type": "ClassDeclaration", @@ -41,6 +41,7 @@ "type": "PropertyDefinition", "start": 33, "end": 49, + "accessibility": null, "computed": false, "declare": false, "decorators": [], @@ -49,9 +50,7 @@ "type": "Identifier", "start": 33, "end": 39, - "decorators": [], - "name": "isSunk", - "optional": false + "name": "isSunk" }, "optional": false, "override": false, @@ -77,12 +76,12 @@ "type": "Identifier", "start": 22, "end": 26, - "decorators": [], - "name": "Ship", - "optional": false + "name": "Ship" }, - "implements": [], - "superClass": null + "implements": null, + "superClass": null, + "superTypeParameters": null, + "typeParameters": null }, { "type": "ClassDeclaration", @@ -98,6 +97,7 @@ "type": "PropertyDefinition", "start": 71, "end": 85, + "accessibility": null, "computed": false, "declare": false, "decorators": [], @@ -106,9 +106,7 @@ "type": "Identifier", "start": 71, "end": 76, - "decorators": [], - "name": "ships", - "optional": false + "name": "ships" }, "optional": false, "override": false, @@ -130,10 +128,9 @@ "type": "Identifier", "start": 78, "end": 82, - "decorators": [], - "name": "Ship", - "optional": false - } + "name": "Ship" + }, + "typeParameters": null } } }, @@ -143,6 +140,7 @@ "type": "PropertyDefinition", "start": 90, "end": 104, + "accessibility": null, "computed": false, "declare": false, "decorators": [], @@ -151,9 +149,7 @@ "type": "Identifier", "start": 90, "end": 95, - "decorators": [], - "name": "cells", - "optional": false + "name": "cells" }, "optional": false, "override": false, @@ -175,10 +171,9 @@ "type": "Identifier", "start": 97, "end": 101, - "decorators": [], - "name": "Cell", - "optional": false - } + "name": "Cell" + }, + "typeParameters": null } } }, @@ -195,9 +190,7 @@ "type": "Identifier", "start": 118, "end": 130, - "decorators": [], - "name": "allShipsSunk", - "optional": false + "name": "allShipsSunk" }, "kind": "method", "optional": false, @@ -245,18 +238,14 @@ "type": "Identifier", "start": 191, "end": 194, - "decorators": [], - "name": "val", - "optional": false + "name": "val" }, "optional": false, "property": { "type": "Identifier", "start": 195, "end": 201, - "decorators": [], - "name": "isSunk", - "optional": false + "name": "isSunk" } } } @@ -271,11 +260,18 @@ "type": "Identifier", "start": 177, "end": 180, + "accessibility": null, "decorators": [], "name": "val", - "optional": false + "optional": false, + "override": false, + "readonly": false, + "typeAnnotation": null } - ] + ], + "returnType": null, + "thisParam": null, + "typeParameters": null } ], "callee": { @@ -298,9 +294,7 @@ "type": "Identifier", "start": 155, "end": 160, - "decorators": [], - "name": "ships", - "optional": false + "name": "ships" } }, "optional": false, @@ -308,12 +302,11 @@ "type": "Identifier", "start": 161, "end": 166, - "decorators": [], - "name": "every", - "optional": false + "name": "every" } }, - "optional": false + "optional": false, + "typeParameters": null } } ] @@ -322,7 +315,10 @@ "expression": false, "generator": false, "id": null, - "params": [] + "params": [], + "returnType": null, + "thisParam": null, + "typeParameters": null } } ] @@ -333,12 +329,12 @@ "type": "Identifier", "start": 59, "end": 64, - "decorators": [], - "name": "Board", - "optional": false + "name": "Board" }, - "implements": [], - "superClass": null + "implements": null, + "superClass": null, + "superTypeParameters": null, + "typeParameters": null } ], "sourceType": "script", ``` </details> It slows down a bit, but not so much?

overlookmotel · 2025-03-17T10:44:06Z

To avoid having to add loads of #[estree(field_order(...))] attributes to AST types, we could alter the codegen in oxc_ast_tools to automatically put #[ts] fields last.

This is now done. It actually affected very few types. #9820

For now I went with 2st option of normalizing oxc output #9813.

OK great. Yes, probably easiest to get content aligned first before we worry about field order.

Maybe the slow speed I noticed previously was from generating diffs for mismatches, rather than the serde deserialization.

I'm thinking about this, but to reorder this, we need to know acorn's order in js script of acorn-test262. Is there any easy-to-use format of estree spec providing this data? Either generating our own from ast tools or somehow extracting it from markdown https://github.com/estree/estree or dts https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/estree/index.d.ts?

Yes. I've written something to output a list of types with their field order, based on Oxc's AST schema: overlookmotel/estree-field-orders branch + oxc-project/acorn-test262#11

We can do that whenever we're ready.

Right now it's not perfect, because there are some types which exist in our AST which don't exist in TS-ESLint's (and probably vice-versa).

overlookmotel · 2025-03-17T10:53:32Z

Thanks for the feedback.

AST of ts-eslint should be 100% aligned with estree (extended), but there is sometimes differences when new js features are added (stage 3) and there is small period of time where AST may differ ts-eslint also does alignment check with @babel/eslint to ensure that consistency

This is great to hear. Hopefully we won't have trouble with the ESTree-compatible and TSESLint-compatible ASTs diverging from each other (apart from, obviously, the TS AST having more fields e.g. typeAnnotation, and additional types for those fields).

yes to my knowledge there is few cases where null has actual meaning in estree but there is technically no difference in null | undefined as far as estree is concerned outside of semantics and depending what parser you use acron/babel/ts-eslint/esprima "nullish" value may be different

https://github.com/estree/estree/blob/master/es2015.md#expressions

eg.

[,2]
/// elements: [null, Literal]

We do handle that one correctly.

I think our best starting point is to treat null and undefined as equivalent ("undefined" meaning a field that exists and has the value undefined, as opposed to a field which isn't present at all).

additionally ts-eslint adds empty array's and defaults "flags" to false and eg. babel does not always do it for ts specific fields

Interesting. Babel is a whole other thing, though. Babel's AST is quite different from ESTree, and we're not seeking to replicate its AST (though we might offer that as an option later on, if there's a demand for it).

hi-ogawa · 2025-03-18T03:01:39Z

If anyone is interested in helping out this, here is my workflow to understand estree mismatches:

# this runs full estree conformance and updates estree_xxx.snap
# but doesn't generate diff
cargo run -p oxc_coverage -- estree

# SAVE_DIFF=true to save diff files in tasks/coverage/acorn-test262-diff
# saving many files is too slow, so using --filter is recommended
SAVE_DIFF=true cargo run -p oxc_coverage -- estree --filter compiler/2dArray

# also you can use `just example parser` for any file to dump ast output
just example parser --estree test.ts

And this is an example diff, which shows Identifier and thisParam is off currently.

tasks/coverage/acorn-test262-diff/typescript/tests/cases/compiler/2dArrays.diff

@@ -20,10 +20,7 @@
         "type": "Identifier",
         "start": 6,
         "end": 10,
-        "decorators": [],
-        "name": "Cell",
-        "optional": false,
-        "typeAnnotation": null
+        "name": "Cell"
       },
       "implements": [],
       "superClass": null,
@@ -53,10 +50,7 @@
               "type": "Identifier",
               "start": 33,
               "end": 39,
-              "decorators": [],
-              "name": "isSunk",
-              "optional": false,
-              "typeAnnotation": null
+              "name": "isSunk"
             },
             "optional": false,
             "override": false,
@@ -82,10 +76,7 @@
         "type": "Identifier",
         "start": 22,
         "end": 26,
-        "decorators": [],
-        "name": "Ship",
-        "optional": false,
-        "typeAnnotation": null
+        "name": "Ship"
       },
       "implements": [],
       "superClass": null,
@@ -115,10 +106,7 @@
               "type": "Identifier",
               "start": 71,
               "end": 76,
-              "decorators": [],
-              "name": "ships",
-              "optional": false,
-              "typeAnnotation": null
+              "name": "ships"
             },
             "optional": false,
             "override": false,
@@ -141,10 +129,7 @@
                     "type": "Identifier",
                     "start": 78,
                     "end": 82,
-                    "decorators": [],
-                    "name": "Ship",
-                    "optional": false,
-                    "typeAnnotation": null
+                    "name": "Ship"
                   }
                 }
               }
@@ -164,10 +149,7 @@
               "type": "Identifier",
               "start": 90,
               "end": 95,
-              "decorators": [],
-              "name": "cells",
-              "optional": false,
-              "typeAnnotation": null
+              "name": "cells"
             },
             "optional": false,
             "override": false,
@@ -190,10 +172,7 @@
                     "type": "Identifier",
                     "start": 97,
                     "end": 101,
-                    "decorators": [],
-                    "name": "Cell",
-                    "optional": false,
-                    "typeAnnotation": null
+                    "name": "Cell"
                   }
                 }
               }
@@ -211,10 +190,7 @@
               "type": "Identifier",
               "start": 118,
               "end": 130,
-              "decorators": [],
-              "name": "allShipsSunk",
-              "optional": false,
-              "typeAnnotation": null
+              "name": "allShipsSunk"
             },
             "kind": "method",
             "optional": false,
@@ -262,20 +238,14 @@
                                     "type": "Identifier",
                                     "start": 191,
                                     "end": 194,
-                                    "decorators": [],
-                                    "name": "val",
-                                    "optional": false,
-                                    "typeAnnotation": null
+                                    "name": "val"
                                   },
                                   "optional": false,
                                   "property": {
                                     "type": "Identifier",
                                     "start": 195,
                                     "end": 201,
-                                    "decorators": [],
-                                    "name": "isSunk",
-                                    "optional": false,
-                                    "typeAnnotation": null
+                                    "name": "isSunk"
                                   }
                                 }
                               }
@@ -290,13 +260,17 @@
                               "type": "Identifier",
                               "start": 177,
                               "end": 180,
+                              "accessibility": null,
                               "decorators": [],
                               "name": "val",
                               "optional": false,
+                              "override": false,
+                              "readonly": false,
                               "typeAnnotation": null
                             }
                           ],
                           "returnType": null,
+                          "thisParam": null,
                           "typeParameters": null
                         }
                       ],
@@ -320,10 +294,7 @@
                             "type": "Identifier",
                             "start": 155,
                             "end": 160,
-                            "decorators": [],
-                            "name": "ships",
-                            "optional": false,
-                            "typeAnnotation": null
+                            "name": "ships"
                           }
                         },
                         "optional": false,
@@ -331,10 +302,7 @@
                           "type": "Identifier",
                           "start": 161,
                           "end": 166,
-                          "decorators": [],
-                          "name": "every",
-                          "optional": false,
-                          "typeAnnotation": null
+                          "name": "every"
                         }
                       },
                       "optional": false,
@@ -349,6 +317,7 @@
               "id": null,
               "params": [],
               "returnType": null,
+              "thisParam": null,
               "typeParameters": null
             }
           }
@@ -360,10 +329,7 @@
         "type": "Identifier",
         "start": 59,
         "end": 64,
-        "decorators": [],
-        "name": "Board",
-        "optional": false,
-        "typeAnnotation": null
+        "name": "Board"
       },
       "implements": [],
       "superClass": null,

Original ts source file is found in tasks/coverage/typescript/tests/cases/compiler/2dArrays.ts and a reference estree output is found in tasks/coverage/acorn-test262/test-typescript/tests/cases/compiler/2dArrays.ts.md.

Part of #9705, fixes #10578. Continuing to treat the following code as an error: ```ts // conformance/jsx/jsxParsingError1.tsx <div className={class1, class2}/> ``` also enables the following code to be parsed: ```ts // conformance/jsx/jsxReactTestSuite.tsx <Comp {...(z = { a: "1" }, z)} /> ``` by deferring the check to the semantic checker.

leaysgur · 2025-05-01T08:30:27Z

Now #10711 and #10739 are merged, #9667 is the last mismatch...! 🎉

overlookmotel · 2025-05-02T16:36:26Z

Amazing! It's required a herculean effort by many people to get us this far.

I'll tackle field order in next few days, and convert the conformance tester to compare ASTs JSON-to-JSON, which I'm afraid will probably throw up more errors. I suspect in places we're generating JSON like:

{
    "type": "Identifier",
    "name": "foo",
    "optional": false,
    "typeAnnotation": null,
    "optional": true,
    "typeAnnotation": null,
}

But hopefully they won't be too hard to fix.

...and then I'll try to fix #9667.

leaysgur · 2025-05-07T04:24:22Z

Just a note about field order..., even for the simplest code 1; (just a single Literal), acorn and TS-ESLint seem to have different order.

Does this mean that we need estree(field_order) for JS and TS separately?

overlookmotel · 2025-05-07T16:33:53Z

My thinking is that both Acorn and TS-ESLint's field orders are pretty arbitrary. TS-ESLint has fields in alphabetical order (but with some random exceptions to that rule!). Acorn's field ordering seems to follow little logic at all.

The field order isn't important per se. What is important is that field order is consistent within our AST for nodes with the same type (i.e. BindingIdentifier and IdentifierReference both create Identifier nodes with fields in same order), because consistent object "shapes" produces better runtime performance for the consumer (and probably makes JSON.parse faster too).

So I now think that my insistence on aligning field order with Acorn was misguided.

I'm planning on taking a different approach now: Order fields in visitation order, based on @typescript-eslint/visitor-keys. I think this will make for a more human-readable AST, and will make it simpler to build a visitor which visits fields in the correct order. I'm working on that now.

So, in short, our field order won't match either Acorn or TS-ESLint. But in my opinion, that's OK!

I also intend to move start and end fields (and range / loc once we support them) to be the last in every type.

That's less ideal from a human-readability perspective, but on positive side will allow removing the extra AST traversal to update spans to UTF-16. Updating spans can happen during serialization/raw deserialization pass instead. Removing the extra pass will give us a perf boost.

But that requires the span fields to be last, as the most efficient way to do UTF-8 -> UTF-16 conversion is in ascending order of source position, in which case you can't calculate end until you've visited the node's children. Because JSON is constructed as a text stream, it's difficult to write "end": ??? first, and then later on rewind and replace it with the correct value. It'd get even more tricky once we also support loc.

If anyone thinks this is unacceptable, please speak now!

Side note: @leaysgur I see you archived https://github.com/leaysgur/oxc_estree_ts-ast-diff-viewer. Would you mind bringing it back? I used it to help with #10874 after updating to latest TS-ESLint. And field ordering will no doubt throw up more problems, which I'd like to use your excellent tool to help fix!

…AST (#10874) Part of #9705. Recent changes in TS-ESLint made `TSMappedType`'s `optional` field default to `false` instead of `null`. Update our TS-ESTree AST to match it.

ArnaudBarre · 2025-05-07T17:51:56Z

The number of people that look up AST directly from the output is very low so I don't think this is an issue. AST explorer can re-roder fields and put them after type when displayed. As long as you put start before end this would still be better than TS-ESLint 😬

leaysgur · 2025-05-08T05:49:45Z

So, in short, our field order won't match either Acorn or TS-ESLint. But in my opinion, that's OK!
I also intend to move start and end fields (and range / loc once we support them) to be the last in every type.

I also think both directions make sense.

https://github.com/leaysgur/oxc_estree_ts-ast-diff-viewer

No problem at all! — I’ve restored it.
Since most mismatches had been resolved, I assumed its role was complete. 😅

If you have any requests, feel free to let me know~!

leaysgur · 2025-05-12T07:41:13Z

I added 2 scripts to leaysgur/oxc_estree_ts-ast-diff-viewer repo. (no view, just scripts)

One is for:

What is important is that field order is consistent within our AST for nodes with the same type (i.e. BindingIdentifier and IdentifierReference both create Identifier nodes with fields in same order),

I have made a PR for this, and no other problems seemed to be found. ✨

The other one compares with the order of their visitor-keys.

According to this script, it seems that there are mismatches in the following 32 nodes.

- `AccessorProperty`
  - theirs: [ "decorators", "key", "typeAnnotation", "value" ]
  - ours: [ "key", "typeAnnotation", "value", "decorators" ]
- `ArrayPattern`
  - theirs: [ "decorators", "elements", "typeAnnotation" ]
  - ours: [ "elements", "decorators", "typeAnnotation" ]
- `ArrowFunctionExpression`
  - theirs: [ "typeParameters", "params", "returnType", "body" ]
  - ours: [ "params", "body", "typeParameters", "returnType" ]
- `AssignmentPattern`
  - theirs: [ "decorators", "left", "right", "typeAnnotation" ]
  - ours: [ "left", "right", "decorators", "typeAnnotation" ]
- `CallExpression`
  - theirs: [ "callee", "typeArguments", "arguments" ]
  - ours: [ "callee", "arguments", "typeArguments" ]
- `ClassDeclaration`
  - theirs: [ "decorators", "id", "typeParameters", "superClass", "superTypeArguments", "implements", "body" ]
  - ours: [ "id", "superClass", "body", "decorators", "typeParameters", "superTypeArguments", "implements" ]
- `ClassExpression`
  - theirs: [ "decorators", "id", "typeParameters", "superClass", "superTypeArguments", "implements", "body" ]
  - ours: [ "id", "superClass", "body", "decorators", "typeParameters", "superTypeArguments", "implements" ]
- `ExportSpecifier`
  - theirs: [ "exported", "local" ]
  - ours: [ "local", "exported" ]
- `FunctionDeclaration`
  - theirs: [ "id", "typeParameters", "params", "returnType", "body" ]
  - ours: [ "id", "params", "body", "typeParameters", "returnType" ]
- `FunctionExpression`
  - theirs: [ "id", "typeParameters", "params", "returnType", "body" ]
  - ours: [ "id", "params", "body", "typeParameters", "returnType" ]
- `JSXElement`
  - theirs: [ "openingElement", "children", "closingElement" ]
  - ours: [ "openingElement", "closingElement", "children" ]
- `JSXFragment`
  - theirs: [ "openingFragment", "children", "closingFragment" ]
  - ours: [ "openingFragment", "closingFragment", "children" ]
- `JSXOpeningElement`
  - theirs: [ "name", "typeArguments", "attributes" ]
  - ours: [ "attributes", "name", "typeArguments" ]
- `LabeledStatement`
  - theirs: [ "label", "body" ]
  - ours: [ "body", "label" ]
- `MethodDefinition`
  - theirs: [ "decorators", "key", "value" ]
  - ours: [ "key", "value", "decorators" ]
- `NewExpression`
  - theirs: [ "callee", "typeArguments", "arguments" ]
  - ours: [ "callee", "arguments", "typeArguments" ]
- `ObjectPattern`
  - theirs: [ "decorators", "properties", "typeAnnotation" ]
  - ours: [ "properties", "decorators", "typeAnnotation" ]
- `PropertyDefinition`
  - theirs: [ "decorators", "key", "typeAnnotation", "value" ]
  - ours: [ "key", "value", "decorators", "typeAnnotation" ]
- `RestElement`
  - theirs: [ "decorators", "argument", "typeAnnotation" ]
  - ours: [ "argument", "decorators", "typeAnnotation" ]
- `SwitchCase`
  - theirs: [ "test", "consequent" ]
  - ours: [ "consequent", "test" ]
- `TaggedTemplateExpression`
  - theirs: [ "tag", "typeArguments", "quasi" ]
  - ours: [ "tag", "quasi", "typeArguments" ]
- `TemplateLiteral`
  - theirs: [ "quasis", "expressions" ]
  - ours: [ "expressions", "quasis" ]
- `TSAbstractAccessorProperty`
  - theirs: [ "decorators", "key", "typeAnnotation" ]
  - ours: [ "key", "typeAnnotation", "decorators" ]
- `TSAbstractPropertyDefinition`
  - theirs: [ "decorators", "key", "typeAnnotation" ]
  - ours: [ "key", "decorators", "typeAnnotation" ]
- `TSDeclareFunction`
  - theirs: [ "id", "typeParameters", "params", "returnType", "body" ]
  - ours: [ "id", "params", "body", "typeParameters", "returnType" ]
- `TSEmptyBodyFunctionExpression`
  - theirs: [ "id", "typeParameters", "params", "returnType" ]
  - ours: [ "id", "params", "typeParameters", "returnType" ]
- `TSImportType`
  - theirs: [ "argument", "qualifier", "typeArguments", "options" ]
  - ours: [ "argument", "options", "qualifier", "typeArguments" ]
- `TSIndexedAccessType`
  - theirs: [ "indexType", "objectType" ]
  - ours: [ "objectType", "indexType" ]
- `TSMappedType`
  - theirs: [ "key", "constraint", "nameType", "typeAnnotation" ]
  - ours: [ "nameType", "typeAnnotation", "key", "constraint" ]
- `TSMethodSignature`
  - theirs: [ "typeParameters", "key", "params", "returnType" ]
  - ours: [ "key", "typeParameters", "params", "returnType" ]
- `TSPropertySignature`
  - theirs: [ "typeAnnotation", "key" ]
  - ours: [ "key", "typeAnnotation" ]
- `TSTypePredicate`
  - theirs: [ "typeAnnotation", "parameterName" ]
  - ours: [ "parameterName", "typeAnnotation" ]

NOTE:

They said keys are arranged in the order they appeared in the code, but in some cases this was not the case
Acorn(jsx) also seems to have a different order 🆚 visitor-keys

💁🏻 UPDATE: These diffs are now outdated.

Part of #9705 With this PR, all AST nodes now have a consistent key order for each node type. (Checked by https://github.com/leaysgur/oxc_estree_ts-ast-diff-viewer/blob/main/scripts/verify-key-order-oxc.js) Except for: - `Literal`: `regex` and `bigint` keys - `TSModuleDeclaration`: `body` key is missing when `declare module "jq";`

overlookmotel · 2025-05-12T19:37:38Z

They said keys are arranged in the order they appeared in the code, but in some cases this was not the case

This comment is a really good find.

I have found 6 types where TS-ESLint's visitor keys are not in source code order:

ExportSpecifier
TSImportType
TSIndexedAccessType
TSMethodSignature
TSPropertySignature
TSTypePredicate

I'm going to open an issue on TS-ESLint to fix them, and the fact that they have a comment saying "important: should be sorted in the order that they appear in the source code" will be really helpful in arguing the case.

Unfortunately ExportSpecifier is also in ESTree and Espree has that one in wrong order too, so TS-ESLint won't be able to change that one without diverging from Espree.

I'm still working on making sure there aren't any more cases before I open that issue, though. Am using #10887 to make sure that we have all our fields in the Rust types in source code order first! Currently we have a bug somewhere which I'm still trying to hunt down.

armano2 · 2025-05-14T23:11:45Z

This comment is a really good find.

eslint doesn't really care about order of properties in objects, and instead uses predefined visitor keys

eslint visitor keys can be found in https://github.com/eslint/js/blob/main/packages/eslint-visitor-keys/lib/visitor-keys.js and ts-eslint just extends it with additional values

visitor-keys are used by traverser that is essentially a recursive loop that base on type checks in what order and if fields should be "visitable"

ref:

traverser: https://github.com/estools/estraverse
visitor-keys: https://github.com/eslint/js/blob/main/packages/eslint-visitor-keys/lib/visitor-keys.js

They said keys are arranged in the order they appeared in the code, but in some cases this was not the case

this comment refers to visitable fields, and not all fields can/should be visited,
only fields whose child is Node should be visited, eg. loc, range, type is not on this list

Note

The iteration order for objects follows a certain set of rules since ES2015, but it does not (always) follow the insertion order.

overlookmotel · 2025-05-28T22:30:18Z

Finally got fields re-ordered properly (#11362). Fields are now in visitation order.

Note: I've not moved start + end to be last yet.

Next step is to test our output for TS-ESTree via JSON-to-JSON comparison. As mentioned above, this will probably reveal more problems, but hopefully not too tricky to fix.

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/acorn-test262#37 which re-orders the fields in fixture snapshots to match the new field order here.

Part of #9705 , follow up #11362 This change will generate the same AST shapes in all locations, regardless of `visitor-keys`.

overlookmotel · 2025-06-02T20:59:12Z

#11450 makes the conformance tester check that field orders are correct in TS-ESTree AST. After #11385 and #11449, all the tests pass.

We do still have some problems where we're generating invalid JSON which includes repeated fields e.g. { optional: true, optional: false, ... }, and the conformance tester is intentionally ignoring those problems for now. That will be trickier to solve, and it sort of works as is, so I've left that for now.

We are now in a position to add tests for TS-ESTree to the testing suite for raw transfer (and to fix any bugs that reveals).

leaysgur · 2025-06-04T01:10:38Z

I don't think there is any new information, but I've done a little research and will make some notes...

{ optional: true, optional: false, ... }

There seemed to be a conflict between flatten and add_field. As you said, it seemed to require some ingenuity to resolve it, so I've left it too. 🙃

for raw transfer (and to fix any bugs that reveals).

I tried the script rawTransfer: true vs rawTransfer: false at hand and did not find a mismatch. 🎉

overlookmotel · 2025-06-04T08:40:49Z

#11476 adds tests for raw transfer on TS-ESTree ASTs. It revealed only 1 tiny mismatch, fixed in #11463. We're all good!

The only problem still remaining with raw transfer is that the errors are missing a codeframe property. I'll fix that today.

overlookmotel · 2025-06-04T13:07:29Z

#11481 adds codeframe property to errors.

All we have left now is the problem of repeated properties in JSON (or, in raw transfer, pointlessly assigned to twice).

There seemed to be a conflict between flatten and add_field. As you said, it seemed to require some ingenuity to resolve it, so I've left it too. 🙃

I'm not sure at this point if this is a blocker to a JS-side AST visitor with lazy (raw) deserialization or not. Hopefully not, as it'll probably be a real pain to fix.

I think it's just BindingPattern that has this problem, but fixing it will likely require changing the Rust types BindingPattern and BindingPatternKind, which will break a lot of code. Nonetheless, in my opinion it'll be worthwhile, because (a) BindingPattern is a nexus of various kinds of mess at the moment, and (b) we can probably reduce its size at same time, and it's an extremely common element in ASTs, so that will be a measurable memory saving.

I just would prefer not to get into it right now, unless it's unavoidable.

overlookmotel · 2025-06-04T19:16:53Z

I've opened a separate issue about BindingPattern (#11489). As that's the only remaining problem here, I think we can finally close this epic issue!

Just to say enormous thanks to everyone who has helped with this effort. Big shout out in particular to @leaysgur and @therewillbecode. It's fantastic that we finally have a spec-compliant AST on JS side, and that it's already paying off with Prettier adopting Oxc.

Boshen · 2025-06-05T01:43:36Z

Thank you all for the hard work on such a big project, which spanned over many months.

overlookmotel added A-ast Area - AST C-enhancement Category - New feature or request labels Mar 12, 2025

This was referenced Mar 14, 2025

feat: add typescript-eslint estree of typescript tests oxc-project/acorn-test262#6

Merged

test(ast/estree): add typescript-eslint estree coverage #9774

Merged

This was referenced Mar 17, 2025

test(ast/estree): ignore field order difference from typescript-eslint #9813

Merged

fix: replace undefined with null in typescript eslint ast oxc-project/acorn-test262#8

Merged

Boshen added the E-Help Wanted Experience level - For the experienced collaborators label Mar 18, 2025

This was referenced Mar 20, 2025

fix(ast/estree): Align BindingIdentifier output with TS-ESTree #9928

Closed

fix(ast/estree): Fix FormalParameter #9954

Merged

overlookmotel mentioned this issue May 4, 2025

oxc-parse with experimentalRawTransfer issue for TSTypeReference #10782

Closed

This was referenced May 7, 2025

fix(ast/estree): fix optional field of TSMappedType in TS-ESTree AST #10874

Merged

fix(ast/estree): remove TSImportTypeOptions custom serializer #10873

Merged

leaysgur mentioned this issue May 12, 2025

fix(estree): Ensure the same key order for AssignmentPattern #10953

Merged

overlookmotel mentioned this issue May 17, 2025

Add decorators in ESTree for JS files #10921

Closed

overlookmotel mentioned this issue May 28, 2025

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

Merged

leaysgur mentioned this issue May 30, 2025

fix(estree): Use consistent field order in serialization #11385

Merged

overlookmotel pushed a commit that referenced this issue Jun 2, 2025

fix(estree): Use consistent field order in serialization (#11385)

ec4fc83

Part of #9705 , follow up #11362 This change will generate the same AST shapes in all locations, regardless of `visitor-keys`.

overlookmotel mentioned this issue Jun 4, 2025

Refactor BindingPattern to remove weirdness #11489

Open

overlookmotel closed this as completed Jun 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Align JS-side AST with standard for TypeScript #9705

Align JS-side AST with standard for TypeScript #9705

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Align JS-side AST with standard for TypeScript #9705

Align JS-side AST with standard for TypeScript #9705

Comments

Uh oh!

Achieving alignment

How

Conflicts between ESTree and TS-ESLint

Raw transfer

JSX

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

D6F3 Uh oh!

Uh oh!

Uh oh!

Uh oh!