Parsing: strictly enforce the produced AST matches the spec and enforce most "error recovery" parsing errors #1852
Comments
|
cc @JamesHenry - what do you think about making the I think |
|
@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 |
|
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). |
|
Not sure if I should create a new error for this or post here but should also be a syntax error, otherwise we get |
typescript-estree should not allow syntax that breaks the ESTree spec
|
cc @thorn0 - if we were to make |
|
No problem. These issues mentioned here are actually not easy to reproduce with Prettier. The standalone |
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.
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.
|
Hello @bradzacher, I've been testing various ESLint plugins for a while now. This bug/functionality in typescript parser is causing crashes in 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 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 rulesRule: indent
const
function sendEmail() {
try {Rule: no-manual-cleanup
import React from 'react';
import Pidex from './Pidex'
import
function App() {
return (
<div className="App">
<header className="App-header">
<h1>Pertida Index</h1>Rule: unable-to-parse-rule-id
import React from 'react';
import Pidex from './Pidex'
import
function App() {
return (
<div className="App">
<header className="App-header">
<h1>Pertida Index</h1> |
|
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. And in fact - we definitely do not adhere to it - there's no way for us to!
An example of where we have seen this cause problems: the empty body function expression. class Foo {
methodWithoutBody();
// ^^ TSEmptyBodyFunctionExpression -> body == null
methodWithBody() {}
// ^^^^^ FunctionExpression -> body == BlockStatement
}A lot of plugins assume that every But 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. And for your cases - they could similarly be solved with simple 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. |
|
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() {}
const
function HelloWorld() { }
throw
function HelloWorld() {}
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 |
|
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 should not allow syntax that breaks the ESTree spec
@bradzacher what if we made this an opt-in behavior, so API consumers such as Prettier can opt-into it? I see from as far back as #185 (comment) that |
|
The issue is that because TS is designed as a "forgiving" parser - iirc most true syntax issues are actually reported as semantic issues. Which is a big part of the problem! I think the best way for us to build this is by enforcing invariants during the conversion. For example "if no expression exists for the throw statement, then parser error". We could add a flag to turn off these invariants if consumers want a "bad" ast. |
|
🤔 my concern there is that doing so would add a lot of code to add to the parser. #6247 shows what it's like. Perhaps there's a way to get TypeScript to report these more accurately? |
|
I personally don't mind some logic! I don't mind either way if there's a way to make TS enforce it and we leverage that - the important thing is that our runtime output matches our types. Our AST structure isn't exactly 1:1 so we have some invariants we will want to enforce as we merge things and move things around. |
|
I've gone ahead and made two PRs for comparison:
Filed microsoft/TypeScript#52011 for #6271. |
|
I can't remember if I've mentioned this before, but the thing I'd be testing is what is the performance of That was the API that I implemented the old Code reference: https://github.com/typescript-eslint/typescript-eslint/blob/v4.33.0/packages/eslint-plugin/src/rules/no-unused-vars-experimental.ts#L296 |
> require('@typescript-eslint/typescript-estree').parse('throw')
Uncaught TSError: Expression expected.
<Omited info>
index: 5,
lineNumber: 1,
column: 5
}
> require('@typescript-eslint/typescript-estree').parse('throw\n')
{
type: 'Program',
body: [ { type: 'ThrowStatement', argument: [Object] } ],
sourceType: 'script'
}😄 |
|
@fisker - that's actually a funny TS quirk - not due to us! I think that TS's parser treats the newline as an error recovery signal, and "makes the AST work", Whereas if it sees something on the same line (like EOF, |
|
there are similar issues with |
|
As of v6 we will throw errors for a lot of cases with more being added over time. |
{ "rules": { "no-throw-literal": "error" } }throwJust that in a file somewhere
Expected Result
This should be a parse-error and should not be linted on.
Actual Result
Parser creates a
ThrowStatementwithargumentset to null, which breaks rules downstream, see eslint/eslint#13143 for contextAdditional Info
Versions
@typescript-eslint/parser^2.13.0TypeScript^3.8.2ESLint5.16.0node12.16.0npm6.13.7The text was updated successfully, but these errors were encountered: