Skip to content
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.

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

`typescript-estree` should not allow syntax that breaks the ESTree spec #1852

Open
steelbrain opened this issue Apr 6, 2020 · 10 comments
Open

`typescript-estree` should not allow syntax that breaks the ESTree spec #1852

steelbrain opened this issue Apr 6, 2020 · 10 comments

Comments

@steelbrain
Copy link

@steelbrain steelbrain commented Apr 6, 2020

{
  "rules": {
    "no-throw-literal": "error"
  }
}
throw

Just that in a file somewhere

Expected Result

This should be a parse-error and should not be linted on.

Actual Result

Parser creates a ThrowStatement with argument set to null, which breaks rules downstream, see eslint/eslint#13143 for context

Additional Info

[Error - 1:41:32 PM] TypeError: Cannot read property 'type' of null
Occurred while linting [fileName].ts:132
    at Object.couldBeError (eslint/lib/rules/utils/ast-utils.js:1262:22)
    at ThrowStatement (eslint/lib/rules/no-throw-literal.js:38:31)
    at eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (eslint/lib/linter/code-path-analysis/code-path-analyzer.js:634:23)
    at eslint/lib/linter/linter.js:936:32

Versions

package version
@typescript-eslint/parser ^2.13.0
TypeScript ^3.8.2
ESLint 5.16.0
node 12.16.0
npm 6.13.7
@bradzacher
Copy link
Member

@bradzacher bradzacher commented Apr 6, 2020

cc @JamesHenry - what do you think about making the parser less forgiving in these invalid situations to match errors that are thrown by babel-eslint/acorn/espree?

I think typescript-estree is probably fine to parse as is (because typescript parses fine as is), but parser is intended to be used in ESLint, so we should probably match the same errors they throw here?

@bradzacher bradzacher added the bug label Apr 6, 2020
@JamesHenry
Copy link
Member

@JamesHenry JamesHenry commented Apr 6, 2020

@bradzacher are you talking about building checks ourselves or leveraging TS? If it’s the latter IIRC it came up before that almost all of the valuable checks come from semantic diagnostics (even when they seem syntactic in nature) and so if we enable that feedback from TS it forces users to require type information even if they wouldn’t need it for their rules setup

@bradzacher
Copy link
Member

@bradzacher bradzacher commented Apr 6, 2020

Probably the former - building just the checks required to achieve some parity with what other parsers do. That way we can be sure that base rules won't error in weird ways (like this issue).

@steelbrain
Copy link
Author

@steelbrain steelbrain commented Apr 9, 2020

Not sure if I should create a new error for this or post here but

import

should also be a syntax error, otherwise we get

[Error - 8:02:02 PM] TypeError: Cannot read property 'indexOf' of undefined
Occurred while linting file.tsx:1
    at isAbsolute (eslint-plugin-import/lib/core/importType.js:48:15)
    at typeTest (eslint-plugin-import/lib/core/importType.js:116:7)
    at resolveImportType (eslint-plugin-import/lib/core/importType.js:148:10)
    at reportIfMissing (eslint-plugin-import/lib/rules/no-extraneous-dependencies.js:130:32)
    at ImportDeclaration (eslint-plugin-import/lib/rules/no-extraneous-dependencies.js:208:11)
    at eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (eslint/lib/linter/node-event-generator.js:283:22)

@bradzacher bradzacher changed the title Throw without arguments should be a syntax error `typescript-estree` should not allow syntax that breaks the ESTree spec Apr 9, 2020
@bradzacher
Copy link
Member

@bradzacher bradzacher commented Apr 9, 2020

cc @thorn0 - if we were to make typescript-estree more ESTree spec complaint here, and throw issues for weird/incomplete code; would that cause issues for prettier?

@thorn0
Copy link
Contributor

@thorn0 thorn0 commented Apr 9, 2020

No problem. These issues mentioned here are actually not easy to reproduce with Prettier. The standalone throw parses only if it's inside a block and not followed by a semicolon, and I can't reproduce the issue with standalone import at all. I mean it's already a SyntaxError.

@bradzacher bradzacher added breaking change AST and removed bug labels Sep 1, 2020
@bradzacher bradzacher added this to the 5.0.0 milestone Sep 1, 2020
bradzacher added a commit that referenced this issue Nov 12, 2020
Ref: prettier/prettier#9636

This allows consumers to reach into the underlying TS AST in cases where our AST doesn't quite solve the use case.

Motivating example:

We don't (currently) error for code unless TS itself throws an error.
TS is _very_ permissive, but that leads to some weird (and invalid) code we don't error for, and don't include in the AST.
For example - this is _syntactically_ valid in the TS parser, but they emit a _semantic_ error:
```ts
@decorator
enum Foo {
  A = 1
}
```
As it's not a valid place for a decorator - we do not emit its information in the ESTree AST, but as TS treats this as a semantic error - it is parse-time valid.
This creates weird states for consumers wherein they cannot see a decorator exists there, which can cause them to ignore it entirely.
For linting - this isn't a problem. But for something like prettier - this means that they'll delete the decorator at format time!
This is very bad.

Until we implement a solution for #1852 - this will allow consumers to bridge the gap themselves.
bradzacher added a commit that referenced this issue Nov 12, 2020
Ref: prettier/prettier#9636

This allows consumers to reach into the underlying TS AST in cases where our AST doesn't quite solve the use case.

Motivating example:

We don't (currently) error for code unless TS itself throws an error.
TS is _very_ permissive, but that leads to some weird (and invalid) code we don't error for, and don't include in the AST.
For example - this is _syntactically_ valid in the TS parser, but they emit a _semantic_ error:
```ts
@decorator
enum Foo {
  A = 1
}
```
As it's not a valid place for a decorator - we do not emit its information in the ESTree AST, but as TS treats this as a semantic error - it is parse-time valid.
This creates weird states for consumers wherein they cannot see a decorator exists there, which can cause them to ignore it entirely.
For linting - this isn't a problem. But for something like prettier - this means that they'll delete the decorator at format time!
This is very bad.

Until we implement a solution for #1852 - this will allow consumers to bridge the gap themselves.
AriPerkkio added a commit to AriPerkkio/eslint-remote-tester that referenced this issue Dec 17, 2020
@AriPerkkio
Copy link

@AriPerkkio AriPerkkio commented Dec 18, 2020

Hello @bradzacher, I've been testing various ESLint plugins for a while now. This bug/functionality in typescript parser is causing crashes in eslint:all, eslint-plugin-node and eslint-plugin-testing-library.

I'm trying to figure out whether this bug should be handled by all of these community plugins or is this really a bug in typescript parser. There's some conflicting comments out there:

ESLint states that The AST that custom parsers should create is based on ESTree. I think some of these cases do not meet the spec.

So when ever I run into cases caused by typescript-parser generated AST which doesn't meet the spec, should I just ignore these or report them to plugin maintainers?

Crashing rules

Rule: indent

  • Message: Cannot read property 'loc' of undefined Occurred while linting <text>:1
  • Path: Tejas1510/Awesome-Javascript-and-React-Project/college-website/server/src/server/api/email/emailer.js
  • Link
const

function sendEmail() {

try {
TypeError: Cannot read property 'loc' of undefined
Occurred while linting <text>:1
    at Object.VariableDeclaration [as listener] (/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/rules/indent.js:1422:69)
    at /home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/rules/indent.js:1634:55
    at Array.forEach (<anonymous>)
    at Program:exit (/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/rules/indent.js:1634:26)
    at /home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.leaveNode(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/node-event-generator.js:306:14)

Rule: no-manual-cleanup

  • Message: Cannot read property 'match' of undefined Occurred while linting <text>:3
  • Path: wesleyclzns/Perdita/perdita-app/src/componentes/App.js
  • Link
import React from 'react';
import Pidex from './Pidex'
import
function App() {
  return (
    <div className="App">
      <header className="App-header">
       <h1>Pertida Index</h1>
TypeError: Cannot read property 'match' of undefined
Occurred while linting <text>:3
    at ImportDeclaration(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint-plugin-testing-library/rules/no-manual-cleanup.js:47:59)
    at /home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
    at /home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/linter.js:952:32
    at Array.forEach (<anonymous>)

Rule: unable-to-parse-rule-id

  • Message: Cannot read property 'indexOf' of undefined Occurred while linting <text>:3
  • Path: wesleyclzns/Perdita/perdita-app/src/componentes/App.js
  • Link
import React from 'react';
import Pidex from './Pidex'
import
function App() {
  return (
    <div className="App">
      <header className="App-header">
       <h1>Pertida Index</h1>
TypeError: Cannot read property 'indexOf' of undefined
Occurred while linting <text>:3
    at stripImportPathParams(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint-plugin-node/lib/util/strip-import-path-params.js:8:20)
    atExportAllDeclaration,ExportNamedDeclaration,ImportDeclaration,ImportExpression(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint-plugin-node/lib/util/visit-import.js:55:40)
    at /home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode(/home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
    at /home/runner/work/eslint-remote-tester/eslint-remote-tester/ci/node_modules/eslint/lib/linter/linter.js:952:32
@bradzacher
Copy link
Member

@bradzacher bradzacher commented Dec 18, 2020

ESLint's default parser is required to adhere to the ESTree spec because that is the spec they are designed to adhere to.

Custom ESLint parsers are not beholden to the same requirement.
As a custom parser - typescript-eslint is not required to exactly adhere to the ESTree spec.
We do not make the guarantee that we adhere to it - our docs mention that we create an "ESTree-compatible AST", but that's as close as we get to a claim.

And in fact - we definitely do not adhere to it - there's no way for us to!

  • We have new nodes that sit in statement/expression locations that can easily cause crashes on plugins that aren't careful.
  • We also have new nodes which add to places where ESTree previously defined a single node child.
  • We null out properties in certain cases
  • We add all manner of new properties to existing nodes

An example of where we have seen this cause problems: the empty body function expression.
For example, this case especially:

class Foo {
  methodWithoutBody();
//                 ^^ TSEmptyBodyFunctionExpression -> body == null
  methodWithBody() {}
//              ^^^^^ FunctionExpression            -> body == BlockStatement
}

A lot of plugins assume that every MethodDefinition has a FunctionExpression as its .value, and that FunctionExpression has a .body which is a BlockStatement.

But TSEmptyBodyFunctionExpression breaks that convention at all levels - crashing plugins up that just try to do node.value.body.body.

There's also countless problems with stylistic rules and things like function generics, because they assume they don't exist - they don't have handling to traverse the new properties on existing nodes - which means that they create many false positives, or create broken fixes that either just delete generics or create syntactically broken code.

So the question is - what should a plugin do?

99.999999% of the time - fixing this is as simple as adding an if guard.

MethodDefinition(node) {
  if (node.value.body) {
    return;
  }

  // continue as normal
}

And for your cases - they could similarly be solved with simple if checks.
But should they? 🤷 That's up to the plugin themselves as to what they want to handle.

I do want to make our parser more correct, but doing so is not a small task, and it is also a breaking change. It's going to be quite some time before I will be able to tackle this.

@AriPerkkio
Copy link

@AriPerkkio AriPerkkio commented Dec 18, 2020

Thanks for the very detailed answer. I think I'm starting to understand this issue now.

These examples of valid Typescript AST you mentioned and whether plugins should handle those - this is a different issue. I should have specified some examples before for commenting the issue here.

I'm more concerned how the parser outputs AST for typescript which is not valid. Here are some examples tested with astexplorer and typescript playgroud.

import 

function HelloWorld() {}
  • Espree: Unexpected token function
  • TS: String literal expected.(1141)
const

function HelloWorld() { }
  • Espree: Unexpected keyword 'function'
  • TS: Variable declaration list cannot be empty.(1123)
throw

function HelloWorld() {}
  • Illegal newline after throw
  • TS: Line break not permitted here.(1142)

The parser is generating AST for all of these cases. I would expect it to recognize these as syntax errors, or something similar. I'm quite unsure whether third party plugins should be expected to handle these cases. They should however handle cases like TSEmptyBodyFunctionExpression if they are expected to work with typescript parser.

@bradzacher
Copy link
Member

@bradzacher bradzacher commented Dec 18, 2020

The typescript parser is very forgiving. It does its best to figure out what you meant and not throw errors unless it really really has to.

It does this for a number of reasons, but mainly so that use cases that need to be failure tolerant are so (eg syntax highlighters).

It emits errors separately as what it calls diagnostics. These diagnostics are not exceptions - they are just data available as part of the parse.

However, in order to get these diagnostics, you need to use the program.

Before v3, we didn't create a program unless you configured type-aware linting.

So it meant that we had no consistent way to get the syntax errors. So we just didn't emit any...

Now a days we always have some form of a program. This means we could leverage TS to do some basic syntax validation.

But again - emitting errors is a breaking change for the typescript-estree package - meaning it's a breaking change for the entire project.

bradzacher added a commit that referenced this issue Jan 2, 2021
Fixes #2726

This PR is the basis for a big cleanup and reorganisation of the AST.
This first step takes the file `types/src/ts-estree.ts` and splits it up in its entirety.
This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and it was intimidating to look at.

Whilst this PR ultimately creates more LOC, the isolation enables a few things:
- By splitting the AST into its own module, it's isolated so easier to manage / govern
- By splitting each AST node into its own folder we can cleanly document and link to individual node specs
- (in a future PR)
    - We can easily add documentation about the node without cluttering things up
    - Colocate fixtures/snapshots with the node specs to document the cases that we expect a node to show up
    - We can colocate the conversion logic here so that it's easier to validate that the spec and the conversion logic are in sync
        - This will make it much easier to implement and maintain #1852
bradzacher added a commit that referenced this issue Jan 2, 2021
Fixes #2726

This PR is the basis for a big cleanup and reorganisation of the AST.
This first step takes the file `types/src/ts-estree.ts` and splits it up in its entirety.
This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and it was intimidating to look at.

Whilst this PR ultimately creates more LOC, the isolation enables a few things:
- By splitting the AST into its own module, it's isolated so easier to manage / govern
- By splitting each AST node into its own folder we can cleanly document and link to individual node specs
- (in a future PR)
    - We can easily add documentation about the node without cluttering things up
    - Colocate fixtures/snapshots with the node specs to document the cases that we expect a node to show up
    - We can colocate the conversion logic here so that it's easier to validate that the spec and the conversion logic are in sync
        - This will make it much easier to implement and maintain #1852
bradzacher added a commit that referenced this issue Jan 2, 2021
Fixes #2726
Fixes #2912

This PR is the basis for a big cleanup and reorganisation of the AST.
This first step takes the file `types/src/ts-estree.ts` and splits it up in its entirety.
This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and there was no way to isolate/restrict certain things (aside from adding comments).

This PR should ultimately be a no-op - there should be no breaking changes here.
I did fix up a number of types which I found when organising files into their folders.

Whilst this PR ultimately creates more LOC, the isolation enables a few things:
- By splitting the AST into its own module, it's isolated so easier to manage / govern
- By splitting each AST node into its own folder we can cleanly document and link to individual node specs
- By grouping nodes decls by folder, it's easier to inspect the types to validate unions are correct.
    - I found a number of invalid nodes in unions in this PR which have been fixed.
- In a future PR we can:
    - Add lint rule(s) to validate unions are correct (eg ensure all `Expression` types are included in the `Expression` union).
    - Easily add documentation about the node without cluttering things up
    - Colocate fixtures/snapshots with the node specs to document the cases that we expect a node to show up
    - Colocate the conversion logic here so that it's easier to validate that the spec and the conversion logic are in sync
        - This will make it much easier to implement and maintain #1852
bradzacher added a commit that referenced this issue Jan 2, 2021
Fixes #2726
Fixes #2912

This PR is the basis for a big cleanup and reorganisation of the AST.
This first step takes the file `types/src/ts-estree.ts` and splits it up in its entirety.
This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and there was no way to isolate/restrict certain things (aside from adding comments).

This PR should ultimately be a no-op - there should be no breaking changes here.
I did fix up a number of types which I found when organising files into their folders.

Whilst this PR ultimately creates more LOC, the isolation enables a few things:
- By splitting the AST into its own module, it's isolated so easier to manage / govern
- By splitting each AST node into its own folder we can cleanly document and link to individual node specs
- By grouping nodes decls by folder, it's easier to inspect the types to validate unions are correct.
    - I found a number of invalid nodes in unions in this PR which have been fixed.
- In a future PR we can:
    - Add lint rule(s) to validate unions are correct (eg ensure all `Expression` types are included in the `Expression` union).
    - Easily add documentation about the node without cluttering things up
    - Colocate fixtures/snapshots with the node specs to document the cases that we expect a node to show up
    - Colocate the conversion logic here so that it's easier to validate that the spec and the conversion logic are in sync
        - This will make it much easier to implement and maintain #1852
bradzacher added a commit that referenced this issue Jan 2, 2021
Fixes #2726
Fixes #2912

This PR is the basis for a big cleanup and reorganisation of the AST.
This first step takes the file `types/src/ts-estree.ts` and splits it up in its entirety.
This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and there was no way to isolate/restrict certain things (aside from adding comments).

This PR should ultimately be a no-op - there should be no breaking changes here.
I did fix up a number of types which I found when organising files into their folders.

Whilst this PR ultimately creates more LOC, the isolation enables a few things:
- By splitting the AST into its own module, it's isolated so easier to manage / govern
- By splitting each AST node into its own folder we can cleanly document and link to individual node specs
- By grouping nodes decls by folder, it's easier to inspect the types to validate unions are correct.
    - I found a number of invalid nodes in unions in this PR which have been fixed.
- In a future PR we can:
    - Add lint rule(s) to validate unions are correct (eg ensure all `Expression` types are included in the `Expression` union).
    - Easily add documentation about the node without cluttering things up
    - Colocate fixtures/snapshots with the node specs to document the cases that we expect a node to show up
    - Colocate the conversion logic here so that it's easier to validate that the spec and the conversion logic are in sync
        - This will make it much easier to implement and maintain #1852
bradzacher added a commit that referenced this issue Jan 3, 2021
Fixes #2726
Fixes #2912

This PR is the basis for a big cleanup and reorganisation of the AST.
This first step takes the file `types/src/ts-estree.ts` and splits it up in its entirety.
This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and there was no way to isolate/restrict certain things (aside from adding comments).

This PR should ultimately be a no-op - there should be no breaking changes here.
I did fix up a number of types which I found when organising files into their folders.

Whilst this PR ultimately creates more LOC, the isolation enables a few things:
- By splitting the AST into its own module, it's isolated so easier to manage / govern
- By splitting each AST node into its own folder we can cleanly document and link to individual node specs
- By grouping nodes decls by folder, it's easier to inspect the types to validate unions are correct.
    - I found a number of invalid nodes in unions in this PR which have been fixed.
- In a future PR we can:
    - Add lint rule(s) to validate unions are correct (eg ensure all `Expression` types are included in the `Expression` union).
    - Easily add documentation about the node without cluttering things up
    - Colocate fixtures/snapshots with the node specs to document the cases that we expect a node to show up
    - Colocate the conversion logic here so that it's easier to validate that the spec and the conversion logic are in sync
        - This will make it much easier to implement and maintain #1852
bradzacher added a commit that referenced this issue Jan 4, 2021
Fixes #2726
Fixes #2912

This PR is the basis for a big cleanup and reorganisation of the AST.
This first step takes the file `types/src/ts-estree.ts` and splits it up in its entirety.
This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and there was no way to isolate/restrict certain things (aside from adding comments).

This PR should ultimately be a no-op - there should be no breaking changes here.
I did fix up a number of types which I found when organising files into their folders.

Whilst this PR ultimately creates more LOC, the isolation enables a few things:
- By splitting the AST into its own module, it's isolated so easier to manage / govern
- By splitting each AST node into its own folder we can cleanly document and link to individual node specs
- By grouping nodes decls by folder, it's easier to inspect the types to validate unions are correct.
    - I found a number of invalid nodes in unions in this PR which have been fixed.
- In a future PR we can:
    - Add lint rule(s) to validate unions are correct (eg ensure all `Expression` types are included in the `Expression` union).
    - Easily add documentation about the node without cluttering things up
    - Colocate fixtures/snapshots with the node specs to document the cases that we expect a node to show up
    - Colocate the conversion logic here so that it's easier to validate that the spec and the conversion logic are in sync
        - This will make it much easier to implement and maintain #1852
bradzacher added a commit that referenced this issue Jan 4, 2021
Fixes #2726
Fixes #2912

This PR is the basis for a big cleanup and reorganisation of the AST.
This first step takes the file `types/src/ts-estree.ts` and splits it up in its entirety.
This file was a monolith at 1700 lines - meaning it was a pain to organise and manage, and there was no way to isolate/restrict certain things (aside from adding comments).

This PR should ultimately be a no-op - there should be no breaking changes here.
I did fix up a number of types which I found when organising files into their folders.

Whilst this PR ultimately creates more LOC, the isolation enables a few things:
- By splitting the AST into its own module, it's isolated so easier to manage / govern
- By splitting each AST node into its own folder we can cleanly document and link to individual node specs
- By grouping nodes decls by folder, it's easier to inspect the types to validate unions are correct.
    - I found a number of invalid nodes in unions in this PR which have been fixed.
- In a future PR we can:
    - Add lint rule(s) to validate unions are correct (eg ensure all `Expression` types are included in the `Expression` union).
    - Easily add documentation about the node without cluttering things up
    - Colocate fixtures/snapshots with the node specs to document the cases that we expect a node to show up
    - Colocate the conversion logic here so that it's easier to validate that the spec and the conversion logic are in sync
        - This will make it much easier to implement and maintain #1852
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.