[breaking] Make Identifier an "atomic" node

[nicolo-ribaudo]

Identifiers can have different properties totally unrelated to their "Identifierness":

• optional (e.g. in function f(a?) {} with Flow)
• typeAnnotation (e.g. in var a: string; with Flow/TypeScript)
• decorators (e.g. in function f(@dec a) {} with TypeScript)

Every other "atom" (e.g. strings, numbers, ...) don't have child nodes. Additionally, it makes it impossible to get the actual identifier name start/end position, because in foo: string foo isn't represented by anything. Example: AST Explorer.

Also, these properties don't make sense in many places: what does it mean if I add typeAnnotation to prop in obj.prop? Or optional to label in break label;?

I propose creating a new node, AnnotatedPattern (name suggestions are welcome), like this:

defineType("AnnotatedPattern", {
  builder: ["pattern", "typeAnnotation", "optional", "decorators"],
  visitor: ["pattern", "typeAnnotation", "decorators"],
  aliases: ["PatternLike", "LVal"],
  fields: {
    pattern: {
      validate: assertNodeType("PatternLike"),
    },
    typeAnnotation: {
      validate: assertNodeType("TypeAnnotation", "TSTypeAnnotation"),
      optional: true,
    },
    optional: {
      validate: assertValueType("boolean"), // Only valid if pattern is an identifier
      optional: true,
    },
    decorators: {
      validate: chain(
        assertValueType("array"),
        assertEach(assertNodeType("Decorator")),
      ),
      optional: true,
    },
  },
});

AnnotatedPattern would be accepted in function parameters and in variable declarations. typeAnnotation & co would then be removed from destructuring patterns.

[loganfsmyth]

Are function params and variable declarations the only cases where we currently know there are issues?

One alternative would be to move the typeAnnotation field from Identifier to VariableDeclarator, and also add a new FunctionParam node type that would wrap the Identifier and have optional, decorators, and typeAnnotation.

I worry that AnnotatedPattern is too overloaded by having to work in both cases, and having a consistent FunctionParam node would make the AST less polymorphic.

[nicolo-ribaudo]

That would work. I don't think that "would make the AST less polymorphic" is a good argument because it just moves the polymorphism down to the child of FunctionParam, but I prefer your suggestion because of optional and decorators.

Also, TypeScript matches this proposal: https://astexplorer.net/#/gist/8ab88f26fe8714059fdc05d3387d5810/d97b0ed36c10c8101b6dec439cc1171624ec0b62

We also have the TSParameterProperty node, which would probably be merged to FunctionParam.

[bradzacher]

Defining a new node type represents a significant breaking change for us when compared to the base ESTree spec, which can cause us all manner of issues with existing ESLint rules expecting a certain structure. It's not an insurmountable problem, but it's definitely something to consider.

I prefer the idea of hoisting the type annotation up to where it belongs, as opposed to defining a new node type. This is how typescript does it, and it seems to work well:

var foo: string;
// VariableDeclaration:
// - name: Identifier
// - type: StringKeyword

var {bar}: {bar: string};
// VariableDeclaration:
// - name: ObjectBindingPattern
// - type: TypeLiteral

var [bar]: [string];
// VariableDeclaration:
// - name: ArrayBindingPattern
// - type: TupleType

function foo(
  @deco arg?: number,
  // Parameter:
  // - name: Identifier
  // - type: NumberKeyword
  // - optional: true
  // - decorators: [Decorator]
   
  arg2?,
  // Parameter:
  // - name: Identifier
  // - optional: true
) {}

@deco
class Foo {
  // ClassDeclaration
  // - name: Identifier
  // - decorators: [Decorator]

  @deco
  constructor(@deco private a: string) {}
  // Parameter:
  // - name: Identifier
  // - type: StringKeyword
  // - modifiers: [PrivateKeyword]
  // - decorators: [Decorator]

  @deco
  prop: string;
  // PropertyDeclaration:
  // - name: Identifier
  // - type: StringKeyword
  // - decorators: [Decorator]

  @deco
  prop?: string;
  // PropertyDeclaration:
  // - name: Identifier
  // - type: StringKeyword
  // - optional: true
  // - decorators: [Decorator]

  prop?;
  // PropertyDeclaration:
  // - name: Identifier
  // - optional: true
}
  
interface Foo {
  prop: string;
  // PropertySignature:
  // name: Identifier
  // type: StringKeyword
  
  prop?: string;
  // PropertySignature:
  // - name: Identifier
  // - type: StringKeyword
  // - optional: true
}

[loganfsmyth]

Defining a new node type represents a significant breaking change for us when compared to the base ESTree spec

Our AST is not compatible with ESTree already. We have a separate parser mode that we use when parsing for ESLint and that can always keep the existing behavior.

I prefer the idea of hoisting the type annotation up to where it belongs, as opposed to defining a new node type.

The issue at the moment is that, for function params, there is no place to hoist to, we only have Identifier. The Parameter node type in your example is what we're proposing adding. The question is basically whether Parameter should only exist when there is an optional or decorators or typeAnnotation or whether it should be expected in all situations.

[nicolo-ribaudo]

Note that, when the parser plugin estree is enabled, we should keep the old AST anyway.

[nicolo-ribaudo]

I was experimenting with this plugin and found a problem with flow:

let [a: T] = []

where do you suggest to store the type annotation in this case?

[bradzacher]

what in the sam hill is that syntax?!?

It looks like flow completely ignores the type that's provided there:

const [a: number, b: string, c: boolean] = ['a', true, 1];
a.toLocaleLowerCase(); // a === string
b - 1 // b === boolean
c.toFixed(); // c === number

const arr: string[] = [];
const [d: number] = arr;
d.toLocaleLowerCase();

https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVBjOA7AzgFzAG0BDALjGwFcBbAIwFMAnAGjDooKYEtsBzNhgp04cGAxLYAumAC8xAOQkFbfEyoM2ARikBuVCQB0+OABk4GEuPMJmAYRK4GACgCUusMGBgSc2fK5ePlQ6MABaMC1Pb1D-eRExCWxMYzgAMW4ADwYAEzcPLzAMP3lqemZ0LDxCEiYmTjUgohl5Zv0qgmIcijLGJhafOv0c1PNLa0R7Rxd3VCA

I think that's probably some legacy syntax or something that the parser allows "just because"?
You know what - i'll ask the flow team.

[bradzacher]

Quoting response from someone on the flow team:

It's not intentional. You're correct that the annotation is ignored. We should at least fix the parser to error here, and ideally change the AST as well, since the AST representation re-uses a node that supports annotations.

---

Adding a bit more after some discussion:

They're going to discuss this internally and figure out what they want to do.
Right now, as stated, it's currently technically "a parser bug" because the parser reuses the variable decl syntax there.
However, they are considering adding support for it properly, so that it is easier to type destructuring (they would then do a similar thing for object destructuring).

So right now - consider it "valid syntax", as it could either turn into an error, or supported as valid type information.

If they go down the latter route, they don't want to make it an error only to make it valid not long after, for obvious reasons.

[existentialism]

@bradzacher thanks for investigating!

[nicolo-ribaudo]

I was thinking again about this in the context of #14694. Either having a new node as I proposed in the original issue, or something like #9545 (comment), would have probably prevented that problem from happening.

[magic-akari]

Quoting response from someone on the flow team:

It's not intentional. You're correct that the annotation is ignored. We should at least fix the parser to error here, and ideally change the AST as well, since the AST representation re-uses a node that supports annotations.

Adding a bit more after some discussion:

They're going to discuss this internally and figure out what they want to do. Right now, as stated, it's currently technically "a parser bug" because the parser reuses the variable decl syntax there. However, they are considering adding support for it properly, so that it is easier to type destructuring (they would then do a similar thing for object destructuring).

So right now - consider it "valid syntax", as it could either turn into an error, or supported as valid type information.

If they go down the latter route, they don't want to make it an error only to make it valid not long after, for obvious reasons.

Now it is no longer a valid syntax and is reported as an error in flow.js (from v0.176.0 to the latest).
The typeAnnotation syntax of flow.js is very similar to TS. (Are they still going to change it? Please correct me if I'm wrong.)
Maybe it's time to move on to this topic.