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

Crash in comma-spacing and comma-style on array literal with empty element #1492

Closed
btmills opened this issue Nov 24, 2014 · 17 comments

Comments

Projects
None yet
3 participants
@btmills
Copy link
Member

commented Nov 24, 2014

On latest master, adding the (theoretically) valid test case [, 0] to tests/lib/rules/comma-spacing.js causes it to fail with the following stack trace:

  1) comma-spacing [, 0]:
     TypeError: Cannot read property 'range' of null
      at EventEmitter.module.exports.api.getTokenBefore (/Users/brandon/code/eslint/eslint/lib/eslint.js:841:31)
      at RuleContext.(anonymous function) [as getTokenBefore] (/Users/brandon/code/eslint/eslint/lib/rule-context.js:69:33)
      at /Users/brandon/code/eslint/eslint/lib/rules/comma-spacing.js:75:43
      at Array.forEach (native)
      at validateCommaSpacing (/Users/brandon/code/eslint/eslint/lib/rules/comma-spacing.js:74:19)
      at EventEmitter.ArrayExpression (/Users/brandon/code/eslint/eslint/lib/rules/comma-spacing.js:132:13)
      at EventEmitter.emit (events.js:95:17)
      at Controller.controller.traverse.enter (/Users/brandon/code/eslint/eslint/lib/eslint.js:638:25)
      at Controller.__execute (/Users/brandon/code/eslint/eslint/node_modules/estraverse/estraverse.js:318:31)
      at Controller.traverse (/Users/brandon/code/eslint/eslint/node_modules/estraverse/estraverse.js:394:28)
      at EventEmitter.module.exports.api.verify (/Users/brandon/code/eslint/eslint/lib/eslint.js:631:24)
      at testValidTemplate (/Users/brandon/code/eslint/eslint-tester/lib/eslint-tester.js:91:35)
      at Context.<anonymous> (/Users/brandon/code/eslint/eslint-tester/lib/eslint-tester.js:150:25)
      at Test.Runnable.run (/Users/brandon/code/eslint/eslint/node_modules/mocha/lib/runnable.js:211:32)
      at Runner.runTest (/Users/brandon/code/eslint/eslint/node_modules/mocha/lib/runner.js:358:10)
      at /Users/brandon/code/eslint/eslint/node_modules/mocha/lib/runner.js:404:12
      at next (/Users/brandon/code/eslint/eslint/node_modules/mocha/lib/runner.js:284:14)
      at /Users/brandon/code/eslint/eslint/node_modules/mocha/lib/runner.js:293:7
      at next (/Users/brandon/code/eslint/eslint/node_modules/mocha/lib/runner.js:237:23)
      at Object._onImmediate (/Users/brandon/code/eslint/eslint/node_modules/mocha/lib/runner.js:261:5)
      at processImmediate [as _immediateCallback] (timers.js:345:15)

Similar to #1418, lib/rules/comma-spacing.js line 75 forgets that arrays can have empty elements.

There is a similar incorrect assumption in comma-style, which causes another crash:

/Users/brandon/code/eslint/eslint/lib/token-store.js:87
        return tokens[starts[node.range[0]] - (skip || 0) - 1];
                                 ^
TypeError: Cannot read property 'range' of null
    at EventEmitter.api.getTokenBefore (/Users/brandon/code/eslint/eslint/lib/token-store.js:87:34)
    at RuleContext.(anonymous function) [as getTokenBefore] (/Users/brandon/code/eslint/eslint/lib/rule-context.js:70:33)
    at /Users/brandon/code/eslint/eslint/lib/rules/comma-style.js:32:43
    at Array.forEach (native)
    at EventEmitter.validateComma (/Users/brandon/code/eslint/eslint/lib/rules/comma-style.js:31:19)
    at EventEmitter.emit (events.js:117:20)
    at Controller.controller.traverse.enter (/Users/brandon/code/eslint/eslint/lib/eslint.js:635:25)
    at Controller.__execute (/Users/brandon/code/eslint/eslint/node_modules/estraverse/estraverse.js:395:31)
    at Controller.traverse (/Users/brandon/code/eslint/eslint/node_modules/estraverse/estraverse.js:493:28)
    at EventEmitter.module.exports.api.verify (/Users/brandon/code/eslint/eslint/lib/eslint.js:628:24)

Edit: Add comma-style crash to issue description

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 24, 2014

Is this similar or the same? I think it's the same issue.

@btmills

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2014

It's the same erroneous assumption in a different rule, and it can be triggered by slightly different test cases. Though the stack trace starts in lib/eslint.js, the issue itself is a contract violation in lib/rules/comma-spacing.js when it calls getTokenBefore with a null element of an array literal.

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 24, 2014

Ah, for it.

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2014

I'm getting the same error in no-multi-spaces.js:163. It looks like a number of rules have the same erroneous assumption.

@btmills

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2014

@ljharb are you using v0.9.2 or latest master? #1507 should have fixed that for master (was #1418).

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2014

I'm using v0.9.2. If it's fixed in master, could we get a publish?

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 27, 2014

@ljharb can you verify that master is working for you? I've been trying to hold off on doing another publish until we get some semblance of ES6/JSX support working.

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2014

It is not. On the latest master, I get:

TypeError: Cannot read property 'range' of null
at EventEmitter.module.exports.api.getTokenBefore (…/eslint/lib/eslint.js:837:31)
    at RuleContext.(anonymous function) [as getTokenBefore] (…/eslint/lib/rule-context.js:69:33)
    at checkList (…/eslint/lib/rules/no-multi-spaces.js:141:36)
    at EventEmitter.ArrayExpression (…/eslint/lib/rules/no-multi-spaces.js:163:13)
    at EventEmitter.emit (events.js:117:20)
    at Controller.controller.traverse.enter (…/eslint/lib/eslint.js:634:25)
    at Controller.__execute (…/eslint/node_modules/estraverse/estraverse.js:318:31)
    at Controller.traverse (…/eslint/node_modules/estraverse/estraverse.js:394:28)
    at EventEmitter.module.exports.api.verify (…/eslint/lib/eslint.js:627:24)
    at processFile (…/eslint/lib/cli-engine.js:131:27)
    at …/eslint/lib/cli-engine.js:226:30
@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

Different rule is throwing the error than the one originally reported. That's progress, at least.

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

Reopened #1418 to track.

@nzakas nzakas closed this Nov 28, 2014

@btmills

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2014

I think these are two separate issues dealing with the same assumption in two different rules. This issue is for comma-spacing, and #1418 is for no-multi-spaces. What @ljharb is reporting is in no-multi-spaces, and the location hasn't changed since the original report. I'll address that back in #1418 shortly, but to the best of my knowledge, this issue in comma-spacing has not been addressed by any PR, and the test still fails. I can confirm that later tonight, but I don't believe this should be closed yet.

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

Thanks for the clarification. Reopened.

@btmills

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2014

I investigated this further by creating a config file that enabled every single rule, then running it on michaelficarra/everything.js. This discovered one more crash based on the same incorrect assumption in comma-style:

/Users/brandon/code/eslint/eslint/lib/token-store.js:87
        return tokens[starts[node.range[0]] - (skip || 0) - 1];
                                 ^
TypeError: Cannot read property 'range' of null
    at EventEmitter.api.getTokenBefore (/Users/brandon/code/eslint/eslint/lib/token-store.js:87:34)
    at RuleContext.(anonymous function) [as getTokenBefore] (/Users/brandon/code/eslint/eslint/lib/rule-context.js:70:33)
    at /Users/brandon/code/eslint/eslint/lib/rules/comma-style.js:32:43
    at Array.forEach (native)
    at EventEmitter.validateComma (/Users/brandon/code/eslint/eslint/lib/rules/comma-style.js:31:19)
    at EventEmitter.emit (events.js:117:20)
    at Controller.controller.traverse.enter (/Users/brandon/code/eslint/eslint/lib/eslint.js:635:25)
    at Controller.__execute (/Users/brandon/code/eslint/eslint/node_modules/estraverse/estraverse.js:395:31)
    at Controller.traverse (/Users/brandon/code/eslint/eslint/node_modules/estraverse/estraverse.js:493:28)
    at EventEmitter.module.exports.api.verify (/Users/brandon/code/eslint/eslint/lib/eslint.js:628:24)

Since the two implementations are so similar, are there any objections to adding the comma-style bug to this issue? If that's fine I'll change the name and update the description.

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

Fine by me

@btmills btmills changed the title Crash in comma-spacing on array literal with empty element Crash in comma-spacing and comma-style on array literal with empty element Nov 30, 2014

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2014

@btmills Any progress on this? This is a major blocker for me using eslint in es6-shim.

@btmills

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2014

I found this testing the fix for #1418, but I'm not going to have time to start a fix for it until Tuesday. Anyone else can feel free to pick it up if interested.

@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 6, 2014

I'll take a look now and see if I can figure it out.

nzakas added a commit that referenced this issue Dec 6, 2014

@nzakas nzakas closed this in c58aad8 Dec 6, 2014

nzakas added a commit that referenced this issue Dec 6, 2014

Merge pull request #1555 from eslint/issue1492
Fix: comma-spacing to work with array literals (fixes #1492)

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

@eslint eslint bot added the archived due to age label Feb 7, 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.