Reworking the parent property on AST Nodes

[bradzacher]

Right now the parent property is optional on all AST nodes.
However in reality, it's always defined on all AST nodes, except for Program, where it is missing.

This leads to a bunch of unnecessary ifs/optional chains/"trust me it's right" assertions, all of which "harm" the type safety, or are impossible to get coverage on (leading to istanbul ignore statements):

// this shouldn't happen, checking just in case
/* istanbul ignore if */ if (node.parent) {
  // ...
}

// selector asserts this is correct
const parent = node.parent as TSESTree.VariableDeclaration;

if (node.parent?.type === AST_NODE_TYPES.VariableDeclaration) {
  // ...
}

There is an issue with this, however.
typescript-estree does not assign the parent property; this is done by ESLint as it traverses the AST at lint time.

So there are two solutions I can see:

1. change typescript-estree so that it sets the parent appropriately.
2. create a separate set of types for use within plugins, which correctly type the parent property.

---

As an aside, doing this would unlock a really awesome benefit wherein we could make all the types generic, allowing you to specify the expected parent type ahead of time:

interface BaseNode<TParent extends Node = Node> {
  // ...
  parent: TParent;
}

interface VariableDeclarator<TParent extends Node = Node> extends BaseNode<TParent> {}

interface Program extends BaseNode {
  parent?: never;
}

This would be great for things like return values of functions which explicitly check parent types, and for eslint selectors that explicitly state the tree structure (VariableDeclaration > VariableDeclarator).

[armano2]

Note: Setting up parent within ts-estree can be misleading for projects that do not use eslint.

what do you think about using type augmentation inside eslint-utils/plugin that will extend nodes instead?

import { Node } from '@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree';

declare module '@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree' {
  export interface BaseNode {
    parent: Node;
  }

  export interface Program {
    parent: never;
  }
}

POC

[bradzacher]

Note: Setting up parent within ts-estree can be misleading for projects that do not use eslint.

Hence point (1) - changing typescript-estree so it does.

what do you think about using type augmentation inside eslint-utils/plugin that will extend nodes instead?

Potentially, but the problem with that is that it doesn't help external users of experimental-utils.
We don't want to add documentation to that package saying "add this file to your project so you can use the parent as expected", it should "just work".

If we can make the declaration merging work for the types exported from the experimental-utils package, then we don't need to worry about modifying the types in typescript-estree (though we should still do the generic change I noted, because it would be awesome)

[armano2]

i like idea of using generics for parent, it will be awesome if there will be way to do so from within experimental-utils, but i'm unsure if you can do augmentation that adds generics to interfaces.

[G-Rath]

fwiw it looks like type argumentation no longer works because @typescript-eslint/types is itself using that to add the parent property - so the result is:

All declarations of 'parent' must have identical modifiers.(2687)
Subsequent property declarations must have the same type.  Property 'parent' must be of type 'Node | undefined', but here has type 'Node'.(2717)

playground.

I can't find anyway around this, though would be good if I'm wrong 😅

[bradzacher]

Yeah!

When I sit out the ast-spec package I intentionally did not add the parent property because it's not part of the spec!

It's added here
https://github.com/typescript-eslint/typescript-eslint/blob/c4310b1aac35c7d31b826f0602eca6a5900a09ee/packages/types/src/ts-estree.ts

Thanks for reminding me though - we can make the breaking change in the next major!

[JoshuaKGoldberg]

#5252 is merged into the v6 branch 🥳. Closing this as it'll be resolved in v6.