fix(ast/estree): fix BindingIdentifier

[hi-ogawa]

spec: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/ast-spec/src/expression/Identifier/spec.ts

I'm not sure whether this can be fixed on struct side BindingIdentifier instead of field side Class.id. BindingIdentifier is used inside flatten BindingPatternKind enum, so changing it might be complicated.

There are probably many instances like this, so I'm just trying to see the pattern first.

[hi-ogawa]

• fix(ast/estree): fix Function.this_param #9913
• fix(ast/extree): fix TSModuleDeclaration.global #9907
• fix(ast/estree): fix BindingIdentifier #9822 👈 (View in Graphite)
• fix(ast/extree): fix Class.implements #9817
• fix(parser)!: rename type_parameters to type_arguments where needed  #9815
• test(ast/estree): update acorn-test262 #9816
• test(ast/estree): ignore field order difference from typescript-eslint #9813
• main

---

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.

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #9822 will not alter performance

_{Comparing 03-17-fix_ast_estree_fix_class.id_ (d69cc34) with main (d3d7d98)}

Summary

✅ 33 untouched benchmarks

[overlookmotel]

Personally I would like to rewrite BindingPattern entirely to get rid of the flattened enum BindingPatternKind. That's the only place we have to flatten an enum in the entire AST, and this oddity complicates our code in various places. Also there's no Span for the entire node including the type annotation.

Something like:

enum BindingPattern {
    BindingIdentifier(Box<'a, BindingIdentifierWithType<'a>>),
    ObjectPattern(Box<'a, ObjectPatternWithType<'a>>),
    ArrayPattern(Box<'a, ArrayPatternWithType<'a>>),
    AssignmentPattern(Box<'a, AssignmentPatternWithType<'a>>),
}

struct BindingIdentifierWithType<'a> {
    pub span: Span,
    pub ident: BindingIdentifier<'a>,
    pub type_annotation: Option<Box<'a, TSTypeAnnotation<'a>>>,
    pub optional: bool,
}

struct ObjectPatternWithType<'a> {
    pub span: Span,
    // `ObjectPattern` is a separate type because it also has a `Span`
    pub pattern: ObjectPattern<'a>,
    pub type_annotation: Option<Box<'a, TSTypeAnnotation<'a>>>,
    pub optional: bool,
}

// Same for `ArrayPatternWithType` and `AssignmentPatternWithType`

This has the downside of requiring more AST types. But the advantage is that we could make illegal syntax impossible to represent in the AST. e.g. the type annotation is not legal here:

[x: Foo] = array;

TS playground

But... problem is that BindingPattern is so commonly used, it'd require tons of changes (including downstream in Rolldown). So we had decided to leave it.

Anyway... TLDR... if BindingPattern has to change to enable TS-ESLint alignment, then doing that would not be out of the question, in my opinion.

[hi-ogawa]

It looks like BindingIdentifier can also do same as IdentifierName and IdentifierReference. The catch is that, when BindingIdentifier is inside BindingPattern, raw json string has duplicate keys for typeAnnotation and optional, but JSON.parse picks up the last (which is the correct value from BindingPattern), so parse result in js world looks correct.

Here is an example:

let x: string = "foo"

$ node napi/parser/example.mjs test.ts
          "id": {
            "type": "Identifier",
            "start": 45,
            "end": 54,
            "name": "x",
            "decorators": [],
            "optional": false,  // 👈
            "typeAnnotation": null,  // 👈
            "typeAnnotation": {  // 👈
              "type": "TSTypeAnnotation",
              "start": 46,
              "end": 54,
              "typeAnnotation": {
                "type": "TSStringKeyword",
                "start": 48,
                "end": 54
              }
            },
            "optional": false  // 👈
          },

I'm not sure if we should concern with this right now or proceed with this until we see issues with this approach.

[overlookmotel]

The duplicate fields in JSON are not ideal, and we'll need to fix at some point. But let's go with it for now, and fix it later once we have achieved alignment of the whole AST.

If it turns out we have to change the shape of BindingPattern anyway, then that'll make it easier to fix.

[overlookmotel]

Merge activity

• Mar 21, 1:44 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Mar 21, 1:44 AM EDT: A user added this pull request to the Graphite merge queue.
• Mar 21, 1:54 AM EDT: A user merged this pull request with the Graphite merge queue.

[ArnaudBarre]

Can this types be generated as non optional?