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

Columns of parse errors are off by 1 #4896

Closed
nightwing opened this Issue Jan 9, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@nightwing
Copy link
Contributor

commented Jan 9, 2016

They are increased once in espree.js#L368 and second time in eslint.js#L91

@eslintbot

This comment has been minimized.

Copy link

commented Jan 9, 2016

@nightwing Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Jan 9, 2016

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 9, 2016

Can you please provide an example?

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 9, 2016

Also, what version are you using?

@ilyavolodin ilyavolodin added bug core evaluating and removed triage labels Jan 9, 2016

@nightwing

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2016

I see this with latest master checkout, and on http://eslint.org/demo/ with var foo = bar axxxx; red squiggly is displayed under x instead of a

@alberto alberto added accepted and removed evaluating labels Jan 9, 2016

@alberto

This comment has been minimized.

Copy link
Member

commented Jan 9, 2016

Should this be fixed here or in espree so it's compatible with other parsers?

@nzakas nzakas added the blocked label Jan 10, 2016

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 10, 2016

It should be fixed in Espree, though we need to verify that Esprima uses zero-based columns for errors to be sure it's the right change.

@nightwing when reporting bugs, please be sure to include all the information requested, as this issue has barely enough information. We get a lot of bugs, so providing all of what we ask for greatly improves our ability to triage.

@alberto

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

Here is the output from the different parsers for this code:

var a = b c;

espree: 1:12 error Parsing error: Unexpected token c
esprima: 1:12 error Parsing error: Unexpected identifier
babel-eslint: 1:11 error Parsing error: Unexpected token

Notice the correct column number is 11

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

Which Espree version is that?

@alberto

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

3.0.0-alpha-3

I used this packages:

    "babel-eslint": "^5.0.0-beta6",
    "eslint": "^2.0.0-alpha-2",
    "esprima": "^2.7.1"

@alberto alberto self-assigned this Jan 13, 2016

@alberto

This comment has been minimized.

Copy link
Member

commented Jan 14, 2016

What should we do then, given the differences among the parsers?

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 14, 2016

tl;dr Espree is fine, we should just fix ESLint, but not at the spot indicating the original description

Verified that Espree@3 is behaving the same as Espree@2:

$ npm i espree@next
espree@3.0.0-alpha-3 node_modules\espree
├── acorn-jsx@2.0.1
└── acorn@2.7.0

$ node
>
> try { espree.parse("class {}"); } catch (ex) { console.dir(ex) }
{ [SyntaxError: Unexpected token {] index: 6, lineNumber: 1, column: 7 }
undefined
>
(^C again to quit)
>

$ npm i espree@2.x
espree@2.2.5 node_modules\espree

$ node
> var espree = require("espree");
undefined
> try { espree.parse("class {}"); } catch(ex) { console.dir(ex); }
{ [Error: Line 1: Unexpected token {]
  index: 6,
  lineNumber: 1,
  column: 7,
  description: 'Unexpected token {' }
undefined
>

So, there's not an Espree issue here. The fact is that different parsers account for things like whitespace differently when reporting errors. My only concern is that the way Espree works remains consistent with itself and with Esprima.

Looking at the original description, this reference has to do only with an error that occurs from parsing a JSON config and does not affect JavaScript parse errors at all, so that's not relevant.

The relevant part is here, which does appear to add a column to parsing errors. I'm not sure why we're doing that, but it does seem incorrect.

@nzakas nzakas removed the blocked label Jan 14, 2016

@alberto

This comment has been minimized.

Copy link
Member

commented Jan 14, 2016

Yes, the error is at that place.

@btmills btmills closed this in 9c1bafb Jan 15, 2016

btmills added a commit that referenced this issue Jan 15, 2016

Merge pull request #4954 from eslint/issue4896
Fix: Columns of parse errors are off by 1 (fixes #4896)

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.