Align JS-side AST with standard for TypeScript

[overlookmotel]

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.

[overlookmotel]

cc @ArnaudBarre @hi-ogawa

[ArnaudBarre]

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]

#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:

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

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

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

[hi-ogawa]

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

[ArnaudBarre]

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]

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]

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

• https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/ast-spec/tests/util/serializers/Node.ts
• https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/ast-spec/src/declaration/ClassDeclaration/fixtures/extends/snapshots/1-TSESTree-AST.shot
• https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/ast-spec/src/declaration/ClassDeclaration/fixtures/extends/snapshots/3-Babel-AST.shot

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]

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]

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.

[overlookmotel]

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]

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]

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.