Skip to content

Reworking the parent property on AST Nodes #1417

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
bradzacher opened this issue Jan 8, 2020 · 6 comments · Fixed by #5252
Closed

Reworking the parent property on AST Nodes #1417

bradzacher opened this issue Jan 8, 2020 · 6 comments · Fixed by #5252
Labels
accepting prs Go ahead, send a pull request that resolves this issue breaking change This change will require a new major version to be released enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Milestone
6.0.0

Comments

@bradzacher
Copy link
Member

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).
@bradzacher bradzacher added enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree breaking change This change will require a new major version to be released labels Jan 8, 2020
@armano2
Copy link
Collaborator

armano2 commented Jan 9, 2020 •
edited
Loading

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
Copy link
Member Author

bradzacher commented Jan 9, 2020 •
edited
Loading

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
Copy link
Collaborator

armano2 commented Jan 9, 2020 •
edited
Loading

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 G-Rath mentioned this issue Aug 2, 2020
Merged
@G-Rath G-Rath mentioned this issue Mar 16, 2021
Merged
@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue May 22, 2022
Closed
@G-Rath
Copy link
Contributor

G-Rath commented May 27, 2022

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
Copy link
Member Author

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!

@bradzacher bradzacher added this to the 6.0.0 milestone May 27, 2022
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Jun 26, 2022
Merged
3 tasks
@JoshuaKGoldberg
Copy link
Member

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue breaking change This change will require a new major version to be released enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Milestone
6.0.0
Development

Successfully merging a pull request may close this issue.

feat: made BaseNode.parent non-optional JoshuaKGoldberg/typescript-eslint
4 participants
@armano2 @G-Rath @JoshuaKGoldberg @bradzacher