no-useless-rename fails on spread operators in destructured statements

[EvHaus]

The new no-useless-rename rule will fail with:

TypeError: Cannot read property 'type' of undefined
    at EventEmitter.checkDestructured (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/rules/no-useless-rename.js:96:26)
    at emitOne (events.js:95:20)
    at EventEmitter.emit (events.js:182:7)
    at NodeEventGenerator.enterNode (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/util/node-event-generator.js:40:22)
    at CodePathAnalyzer.enterNode (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:607:23)
    at CommentEventGenerator.enterNode (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/util/comment-event-generator.js:97:23)
    at Controller.traverser.traverse.enter (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/eslint.js:886:36)
    at Controller.__execute (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/node_modules/estraverse/estraverse.js:397:31)
    at Controller.traverse (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/node_modules/estraverse/estraverse.js:501:28)
    at Controller.Traverser.controller.traverse (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/util/traverser.js:36:33)

When a spread operator is encountered, such as in this code:

const {...stuff} = myObject;

The issue is in the checkDestructured function which is expecting a properties[i].key to exist. But the ExperimentalSpread AST object does not have a key defined.

[platinumazure]

Could you please provide your configuration (at least for the rule), for ease of reproduction? Thanks!

Also, for bug reports, please do consider using the issue template that we have provided. It might seem like a pain, but it makes it much easier to triage and troubleshoot bug reports on our side. Thanks!

[EvHaus]

My apologies for not following the template. The rule configuration used in my case was the default:

"no-useless-rename": [2, {
        "ignoreDestructuring": false,
        "ignoreImport": false,
        "ignoreExport": false
}]

[platinumazure]

Thank you! I agree this seems like a bug- hopefully we can get on it pretty quickly.

Ping @mysticatea?

[michaelficarra]

eslint doesn't support object spread. That is part of a stage 2 proposal.

[EvHaus]

I should clarify that I am also using "parser": "babel-eslint" with babel-eslint version 6.0.4

[platinumazure]

This is where the issue template comes in handy :-)

[ilyavolodin]

@EvNaverniouk Can you try this with default parser? We do not maintain babel-eslint, and if the issue is coming from parser, there's nothing we can do about it.

P.S. @michaelficarra Object Rest/Spread is the only experimental feature that we support, it's controlled by experimentalObjectRestSpread switch in ecmaFeatures.

[michaelficarra]

@ilyavolodin That's strange. Why do we make such an exception?

[EvHaus]

This issue is reproducible without babel-eslint.

Command used to test: node_modules/.bin/eslint test.js

Contents of test.js:

var {...stuff} = myObject;

ESLint configuration:

{
    "parserOptions": {
        "ecmaFeatures": {
            "experimentalObjectRestSpread": true
        },
        "ecmaVersion": 6
    },
    "rules": {
        "no-useless-rename": [2, {
            "ignoreDestructuring": false,
            "ignoreImport": false,
            "ignoreExport": false
        }]
    }
}

Output:

Cannot read property 'type' of undefined
TypeError: Cannot read property 'type' of undefined
    at EventEmitter.checkDestructured (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/rules/no-useless-rename.js:94:38)
    at emitOne (events.js:90:13)
    at EventEmitter.emit (events.js:182:7)
    at NodeEventGenerator.enterNode (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/util/node-event-generator.js:40:22)
    at CodePathAnalyzer.enterNode (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:607:23)
    at CommentEventGenerator.enterNode (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/util/comment-event-generator.js:97:23)
    at Controller.traverser.traverse.enter (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/eslint.js:886:36)
    at Controller.__execute (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/node_modules/estraverse/estraverse.js:397:31)
    at Controller.traverse (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/node_modules/estraverse/estraverse.js:501:28)
    at Controller.Traverser.controller.traverse (/Users/evgueni.naverniouk/Git/ux/node_modules/eslint/lib/util/traverser.js:36:33)

[platinumazure]

@ilyavolodin @michaelficarra Now that we have a reproduction without babel-eslint, I think we need to accept this issue, unless I'm missing something. For what it's worth, I'm 👍 to avoiding rule crashes where possible.

[ilyavolodin]

@platinumazure Agree. Seems like a valid bug, rules shouldn't just crash.
@michaelficarra Mostly due to two reasons. This was number one requested new feature, and we were getting a lot of requests for it. And we were also getting a lot of false-positives from babel-eslint users, because they couldn't switch to Espree due to the lack of objectRestSpread, which is suggested by a lot of React examples/tutorials out there. We felt like in this case user experience mattered more then purity. It's marked as experimental and we don't have any rules targeting objectRestSpread. Just parser support.

[platinumazure]

Working on this.