Improve AST with respect to estree

[Boshen]

This is not exhaustive but should cover all 'types' of differences to resolve:

• Part of the AST is still based on the "import assertion" spec and should be updated. I want to tackle this I think it's a good next for me to do.
• setProp(node, "typeArguments", node.typeParameters) is not a mistake, it's because TSESLint deprecated typeParameters in multiple nodes for typeArguments but keep both for now for backward compatibility. I'm not sure what is the right move for OXC here. For ref: fix: rename typeParameters to typeArguments where needed typescript-eslint/typescript-eslint#5384
• Sometimes node are missing boolean, like prefix: true on UnaryExpression. I don't yet know how to do that in serde
• StaticMemberExpression, ComputedMemberExpression & PrivateFieldExpression need to be serialized to MemberExpression with few changes
• StringLiteral, BooleanLiteral, ... need to be serialized as Literal with raw value set. For BigInt, because it's not in the JSON spec, it would still requires some work on the JS side but this can be done as part of the reviver function when desrializing.
• modifiers is not an ESTree concept. So anytime there is this prop in OXC it probably means you need to set one or two boolean on the equivalent node. Examples are VariableDeclaration.declare, ClassExpression.abstract, TSEnumDeclaration.const. My take here is that it will make the OXC AST more clean and easy to explore to have named booleans on nodes instead of a comment to say which one are 'allowed'. But I don't know the implication of this.
• Some types that are nullable in OXC default to empty array in ESTree, like TSInterfaceDeclaration.extends, ClassExpression.implements
• FunctionBody is a BlockStatement in ESTree
• FormalParameters is closer to ESTree now, but should be inlined directly into <node>.params in multiple nodes
• TSMappedTypeModifierOperator is boolean | '-' | '+' | undefined in TSESTree, I think my conversion is incomplete at the moment
• UsingDeclaration is a VariableDeclaration (Not done in my wrapper)
• Not sure yet, but the structure of hashbang, comments & directives needs some updates

Some changes could be seen as rename only, but as soon we touch the name of the nodes the type generation became complex to handle that why I didn't push more PRs for that for now.

For info the TSUnionType hack is really prettier specific.

But the biggest blocker to make my repo public is the utf16 conversion. The time to print some large files goes in seconds with the dump implementation, and because I saw you plan to solve that I don't want to optimize for it.

Some good news is that my repo successfully gave the same output of prettier on ~500 TSX files, ~8k dts files & 10k files inside my node_modules. This is obviously a very biased dataset, but still promising!

Originally posted by @ArnaudBarre in #2463 (comment)

[Boshen]

modifiers is not an ESTree concept. So anytime there is this prop in OXC it probably means you need to set one or two boolean on the equivalent node. Examples are VariableDeclaration.declare, ClassExpression.abstract, TSEnumDeclaration.const. My take here is that it will make the OXC AST more clean and easy to explore to have named booleans on nodes instead of a comment to say which one are 'allowed'. But I don't know the implication of this.

The "modifiers" concept was ported from a contributor from a long time ago. We may need to reevaluate this decision and make larger changes. The underlying problem is how recoverable our parsing should be.

[sapphi-red]

• ImportDeclaration.specifiers should be [] instead of null for import "foo" (playground) (fix(ast)!: always return Array<ImportDeclarationSpecifier> for ImportDeclaration.specifiers #8560)
• uses ObjectProperty/BindingProperty instead of Property (playground)
• CatchClause.param should be Identifier instead of CatchParameter (playground)

[overlookmotel]

To surface all the obscure differences between our JS-side AST and ESTree, I think we ideally need conformance tests. Initially we'll have tons of failures, but we can chip away at them until there are none. I suggest:

• Parse all Test262 fixtures with Acorn and write the resulting ASTs as JSON files.
• Run Oxc parser on same fixtures and serialize to JSON with serde.
• Conformance runner compare the two, and generate snapshot files showing which fixtures fail.

The Acorn AST JSON files will take time to compute and will be many many files. We could:

• Create a new repo with the code to run Acorn.
• Check in the resulting JSON files to that repo.
• Link that repo into main Oxc repo as a git submodule (same as we do with Test262, Babel, etc fixtures).

Then the conformance tests could be written purely in Rust, and we only need to run Acorn once, not every time conformance runs.

That'll only cover us for JS syntax. I guess we could do similar for TS ASTs using fixtures from TypeScript repo and using typescript-eslint's parser to generate the JSON files. But personally I think we should concentrate on the JS AST first.

Does that sound like a good idea? If so, does anyone have an interest in building the infrastructure?

[ArnaudBarre]

I think that a tool like this would be useful for interfacing with other tools that manipulate source code (my initial goal that I still want to pursue at some point is having a fast prettier parser). And for me source code is TS code and I don't know anyone still thinking that people write mainly JS (expect ESLint). So I think the infra should target TS code in priority, but I don't have the bandwidth to do it so maybe Test262 is better than nothing.

[overlookmotel]

Just to clarify: The target is very much TS. But as TS is a superset of JS, in my opinion it'd be easier if we tackle JS first, and then go on to TS.

I am also keen to make sure that there aren't any places where JS and TS ASTs diverge from each other (aside from TS AST adding new types, and new properties to existing JS types). i.e. I want to make sure that we can satisfy both ESTree and TS AST compatibility with a single AST shape. If we can't, that will inform how the serializer needs to be designed (which is a work in progress - #6347).

[overlookmotel]

We are now at 99.88% on the conformance tests vs Acorn for JS syntax. What's left is:

Two oddities:

• language/expressions/dynamic-import/import-attributes/2nd-param-await-ident.js - fixed in test(ast/estree): bump acorn-test262 submodule #9047.
• language/expressions/template-literal/tv-line-terminator-sequence.js - fixed in fix(lexer): fix decoding lone \r in template literals #9066.

And 2 groups of edge cases which we currently need to accept failure on:

• (fn) = function() {} and similar. Probably we ignore these - see parser: missing ParenthesizedExpression for identifier on left hand side of assignment expression #9029.
• unexpected end of hex escape from serde_json due to lone surrogates in strings. We have to ignore these for now - see codegen: string surrogates printed incorrectly #3526 (comment).

With apologies, I do intend to destroy these fine results shortly by making the conformance test runner stricter - not ignoring extraneous fields, and requiring fields to be in the right order as well in the JSON. But all the test failures we'll get from those changes should be easy to fix.

Main next step, once the last few tests listed above are fixed, will be to go on to TS and JSX.

[overlookmotel]

Looks like language/expressions/template-literal/tv-line-terminator-sequence.js fail is a bug in the lexer. I'll look into it tomorrow.

[Boshen]

Consider this issue done when acorn estree compat is complete. We'll create another tracking issue for typescript eslint estree, but the core team will not spend time on it for the time being.

[overlookmotel]

All JS tests now passing except the small number which fail due to #3526 and #9029. #9073 filters out those test cases, which leaves 100% pass!

Next step: Make the tester stricter to make sure no extraneous fields in our AST which aren't related to TS, and to ensure fields are in the right order (important for usability and consistent object "shapes" for JS engines). This will no doubt cause many of the passing tests to fail again, but they should be fairly quick to fix.

I'm going to check what Rollup (rollup/parseAst) does for TS and JSX - whether it guarantees compatibility with any particular standard. If it doesn't then, as Boshen says, we can leave TS/JSX compatibility until later.

[overlookmotel]

Looks like Rollup's parseAst does aim to conform to typescript-eslint:

For encoded non-JavaScript AST nodes like TypeScript or JSX, we try to follow
the format of typescript-eslint, which can be derived from their playground
https://typescript-eslint.io/play/#showAST=es&fileType=.tsx
For JSX, see also https://github.com/facebook/jsx/blob/main/AST.md

https://github.com/rollup/rollup/blob/4b8745922d37d8325197d5a6613ffbf231163c7d/scripts/ast-types.js#L23-L26

@Boshen I assume this means that we do need to make our JS-side AST conform to typescript-eslint after all?

On the positive side, given how swiftly we've been able to achieve ESTree compatibility, and that our TS types are broadly similar to typescript-eslint's AST anyway, I doubt this would be a huge task.

[hi-ogawa]

I don't think rollup/parseAst supports typescript. The code comment you referenced is likely stale or meant for something different. SWC syntax seems mostly fixed here https://github.com/rollup/rollup/blob/4b8745922d37d8325197d5a6613ffbf231163c7d/rust/parse_ast/src/lib.rs#L23-L30 and I confirmed ts syntax gives a parse error https://stackblitz.com/edit/vitejs-vite-ohgd7eku?file=repro.js

For jsx support, that's fairly recent addition in rollup https://github.com/rollup/rollup/releases/tag/v4.24.0, so I'd expect the downstream usage is very small or maybe none. For Vite ecosystem specifically, estree without jsx and typescript should suffice as far as I know.

[overlookmotel]

OK great!

I do think we should support JSX, since Rollup does. Thankfully it'll be pretty simple as there's very few JSX types.

I'll open a separate issue for that.

[overlookmotel]

Re-opening this issue for the remaining work on JS syntax - correct property order, and ensuring no extraneous properties in the AST (except those related to TS).

[overlookmotel]

#9285 ticks off those last 2. After that's merged, JS-side AST is 100% ESTree compatible. The new stricter conformance tester flushed out a few bugs, which are now fixed. And the new JSON serializer is 25% faster than serde.

All that remains is to decide how to plumb it into the NAPI/WASM bindings.

[Boshen]

Oxc v0.53.0 now returns an estree for direct consumption in JavaScript through napi and wasm APIs, no more ugly work arounds.

[cernymatej]

@Boshen is there a way to access the information about whether an identifier is a reference one or not, like it was possible before? (in javascript oxc-parser) Or is it only estree from now on?

[Boshen]

@Boshen is there a way to access the information about whether an identifier is a reference one or not, like it was possible before? (in javascript oxc-parser) Or is it only estree from now on?

Nope, it's estree, people wants it, it has less information than our Rust AST, sadly we can't go back now.

[cernymatej]

I see. Thank you!
Would you consider adding additional OXC-specific information to the nodes so that they maintain ESTree compatibility while preserving useful details? Or do you think that wouldn’t be worth exploring?

[overlookmotel]

@cernymatej Sorry for belated response. We may consider in the future providing a way to access this information on JS side. But I'm afraid that's likely a way off. ESTree compatibility was the primary goal, as it's what's needed for interop with other tools.

[cernymatej]

Great, thank you for the info! 🙂
At least the identifier type (reference, binding,…) would be a big improvement!