typescript-estree icon indicating copy to clipboard operation
typescript-estree copied to clipboard

AST missing `start` and `end` values.

Open HauptmannEck opened this issue 6 years ago • 6 comments

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

HauptmannEck avatar Oct 24 '18 18:10 HauptmannEck

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.

JamesHenry avatar Oct 28 '18 22:10 JamesHenry

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.

HauptmannEck avatar Oct 29 '18 13:10 HauptmannEck

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.

mysticatea avatar Oct 30 '18 05:10 mysticatea

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.

kaicataldo avatar Oct 30 '18 21:10 kaicataldo

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?

JamesHenry avatar Oct 31 '18 01:10 JamesHenry

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.

mysticatea avatar Oct 31 '18 01:10 mysticatea