eslint / espree

An Esprima-compatible JavaScript parser
BSD 2-Clause "Simplified" License
2.26k stars 189 forks source link

Align program range/start/loc with other parsers #488

Closed fisker closed 3 years ago

fisker commented 3 years ago
require('@babel/parser').parse('   /**/   a /**/      ', {ranges: true}).program

// {start:0, end: 22, range: [0, 22]}
require('acorn').parse('   /**/   a /**/      ', {ranges: true})
// {start:0, end: 22, range: [0, 22]}
require('meriyah').parse('   /**/   a /**/      ', {ranges: true})

// {start:0, end: 22, range: [0, 22]}
require('@typescript-eslint/typescript-estree').parse('   /**/   a /**/      ', {range: true})
// {range: [10, 22]}

(I think they are starting from 10 only to align with espree)

require('espree').parse('   /**/   a /**/      ', {range: true, loc: true})
// {range: [10, 11], start: 0, end: 22}  ({start: 10, end: 11} on master branch)

It doesn't make sense at all, if there is no statement, just comments and whitespace, what should it be?(currently we count from 0)

Image this, I have a function to check comment is inside node, isInsideNode(comment, node), If only comments, it's true, if I wrote a letter or number anywhere, it's false.

mdjermanovic commented 3 years ago

This is intentional:

https://github.com/eslint/espree/blob/dffb7aa72f2cd23d99e36e6c7c1a76c73ff08f16/lib/espree.js#L155-L161

require('@typescript-eslint/typescript-estree').parse('   /**/   a /**/      ', {range: true})
// {range: [10, 22]}

(I think they are starting from 10 only to align with espree)

Is @babel/eslint-parser also aligned with espree? If all major parsers used with ESLint are producing the same result, then we probably shouldn't change that.

fisker commented 3 years ago

Is @babel/eslint-parser also aligned with espree?

Yes, they do, notice '@typescript-eslint/typescript-estree' only aligned start position. [10, 22] vs [10, 11].

we probably shouldn't change that.

What about my other points in the topic? This isInsideNode is a real-world case, we check comments for Program in the Prettier project, this is how I found this difference (we used .start before, and use .range now).


Indeed, the parsers used with ESLint aligned with espree, but now we changed Program.start/Program.end, so they need to fix that, why not fix the range instead of fixing the .start and .end.

mdjermanovic commented 3 years ago

I checked all parsers on https://astexplorer.net/ with /**/ a /**/ and /**/ /**/.

For the /**/ a /**/ example, esprima, flow and recast produce the same Program.range[0] as espree and all *-eslint parsers.

Granted, espree was aligned with esprima, and *-eslint parsers are aligned with espree, but range[0]/range[1]/start/end values for same code examples are generally very different between the parsers. It seems there is no consensus on how this should work, so I'm not sure if there's a strong enough reason to make a breaking change for all ESLint plugins and parsers now.

@eslint/eslint-tsc thoughts about this?

Indeed, the parsers used with ESLint aligned with espree, but now we changed Program.start/Program.end, so they need to fix that, why not fix the range instead of fixing the .start and .end.

start and end are not required for eslint-compatible parsers (for example, @typescript-eslint/parser does not produce these properties). Rules should never use start/end, so there is no need to align this with espree.

btmills commented 3 years ago

Are there scenarios where end users would observe incorrect behavior because of these differences? If not, I'd leave it alone.

fisker commented 3 years ago

Yes,

we check comments for Program in the Prettier project.

btmills commented 3 years ago

Thanks, I wasn't sure if that was an end user issue or just something you noticed internally. Is your proposal "Program.range should be [0, sourceCode.length] regardless of leading or trailing whitespace or comments"? Is Program the only node we'd need to change?

fisker commented 3 years ago

I wasn't sure if that was an end user issue or just something you noticed internally.

Prettier is a user of this espree, I'm maintainer from Prettier project, and I was the one bring espree to Prettier.

Is your proposal "Program.range should be [0, sourceCode.length] regardless of leading or trailing whitespace or comments"?

I just want align espree with other parsers, as I wrote except flow-parser and *-eslint parser(they aligned with espree), only espree not counting from 0.

Is Program the only node we'd need to change?

Yes, the only one.

btmills commented 3 years ago

To help us assess the impact of this change, what functionality in Prettier is currently broken when using espree because of this inconsistency? An example or issue link would help. I'm not familiar with Prettier's internals, so I apologize if the impact should be obvious and I'm coming across a bit dense here.

nzakas commented 3 years ago

My point of view:

  1. Espree is the canonical implementation of an ESLint parser. If other parsers want to be compatible with ESLint, they should align their output to Espree, not the other way around. Babel cannot work directly with ESLint, nor can Mariyah or Acorn.
  2. The only reason start and end are output from Espree is because Acorn produces them and it is prohibitively expensive to remove them (I know because I tried). ESLint intentionally does not rely on these properties for maximum compatibility.
  3. The change to the start and end properties is of no consequence as we just made the values align with those in the range to avoid confusion. This change should not affect the ESLint ecosystem at all.
  4. If the only node causing problems for Prettier is Program, why can’t you just use 0 in place of range[0]? It seems like the root node is a special case anyway, and if you know the value you want to use, and the value is constant, it shouldn’t matter what value Espree produces.
fisker commented 3 years ago

Thanks for the explanation.

I thought this is a good chance to bring this up because we are about to release a new version with breaking changes, I understand your concerns, I can accept if you decide not to accept.

Thanks to all of you!