JamesHenry / typescript-estree

:sparkles: A parser that converts TypeScript source code into an ESTree-compatible form
https://jameshenry.blog
Other
84 stars 13 forks source link

AST missing `start` and `end` values. #19

Open HauptmannEck opened 5 years ago

HauptmannEck commented 5 years ago

What version of TypeScript are you using? 3.1.3 What version of typescript-estree are you using? 2.1.0 (I have not seen any changes in the code that would fix this in v3) What code were you trying to parse?

import * as React from 'react';

const Thing = ( { imgSrc } ) => (
    <img src={imgSrc}
         className="class"
    />
);

What did you expect to happen? Generate AST that matched the ESlint AST so that I could use eslint-plugin-react rules. What actually happened? The AST does not have start or end numerical values so any rule fixes that use them damages the code.

I see there are comments in the alignment tests that allude to this being on purpose:

 * - Remove "start" and "end" values from Babylon nodes to reduce unimportant noise in diffs ("loc" data will still be in
 * each final AST and compared).

The @babel/parse and Espree parsers both have the start and end values on their AST output, so I am curious as to why it was chosen to not have them in this projects AST.

This is the rule that I am seeing breakage on for example: https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/jsx-first-prop-new-line.js#L48

JamesHenry commented 5 years ago

Interestingly, those properties are not actually defined in the ESTree spec: https://github.com/estree/estree/blob/master/es5.md

I noticed esprima doesn't have them, but most of the other popular parsers do. We can probably consider adding them in as it is very non-disruptive, but I would note that it is not necessarily a design goal of this parser to produce ASTs to comply with assumptions in ESlint rules. The goal is to take TypeScript source code and produce an ESTree-compatible AST.

HauptmannEck commented 5 years ago

Luckily as long as the range values exist there is a work around and we can change the code consuming the AST to not use the start/end. Although I noticed range also doesn't seem to be a part of the ESTree spec either.

I don't have an opinion on if the fields should be added or not, just wanted to get the conversation out there. Hopefully this issue can help others in the future understand the cause of their bugs.

mysticatea commented 5 years ago

In fact, ESLint AST spec also doesn't have start and end properties. Espree accidentally introduced those properties when it switched to acorn from esprima. It has been left because removing properties is a heavy operation.

kaicataldo commented 5 years ago

Given that this isn't part of the ESTree spec, is this something we want to do? It sounds like it could just be left to consumers to implement if it's something they need.

JamesHenry commented 5 years ago

Is there any easy way to offer a compatibility layer within typescript-eslint-parser, so that eslint rules don't need to be rewritten?

I imagine post-processing the whole AST is an option, but may be quite slow?

mysticatea commented 5 years ago

ESLint core rules have never used start and end properties :) But I'm not sure plugins...

I think that we can add the verification to check whether rules don't use start/end into RuleTester. But it will be a breaking change.