[6.5.0] no-useless-rename with babel-eslint crash at rest properties

[jakeleventhal]

Tell us about your environment

• ESLint Version: 6.5.0
• Node Version: 12.10.0
• npm Version: 6.10.3

What parser (default, Babel-ESLint, etc.) are you using?
babel eslint

Please show your full configuration:

Configuration

{
  "extends": [
    "airbnb",
    "plugin:react/all"
  ],
  "env": {
    "browser": true,
    "node": true,
    "es6": true
  },
  "parser": "babel-eslint",
  "plugins": [
    "react",
    "simple-import-sort"
  ],
  "rules": {
    "arrow-parens": [
      "error",
      "always"
    ],
    "camelcase": "error",
    "comma-dangle": [
      "error",
      "never"
    ],
    "curly": [
      "error",
      "all"
    ],
    "func-names": "error",
    "indent": [
      "error",
      2
    ],
    "max-len": [
      "error",
      {
        "code": 120,
        "ignoreUrls": true,
        "tabWidth": 2
      }
    ],
    "no-param-reassign": "off",
    "no-underscore-dangle": "off",
    "no-plusplus": "off",
    "no-console": "error",
    "no-unused-vars": [
      "error",
      {
        "argsIgnorePattern": "dispatch"
      }
    ],
    "simple-import-sort/sort": "error",
    "sort-keys": [
      "error",
      "asc",
      {
        "natural": true,
        "caseSensitive": false
      }
    ],
    "sort-vars": "error",
    "react/jsx-indent": [
      "error",
      2
    ],
    "react/jsx-indent-props": [
      "error",
      2
    ],
    "react/jsx-max-depth": "off",
    "react/jsx-no-literals": "off",
    "react/jsx-one-expression-per-line": "off",
    "react/destructuring-assignment": "off",
    "react/jsx-key": "off",
    "react/no-did-mount-set-state": "off",
    "react/no-set-state": "off",
    "react/prefer-stateless-function": "off",
    "react/jsx-props-no-spreading": "off",
    "jsx-a11y/click-events-have-key-events": "off",
    "jsx-a11y/no-static-element-interactions": "off",
    // TODO: All rules below here will eventually be accounted for
    "react/jsx-handler-names": "off",
    "react/prop-types": "off",
    "react/jsx-fragments": "off"
  }
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

import { Button as MuiButton, withStyles } from '@material-ui/core';
import { darken } from '@material-ui/core/styles';
import * as React from 'react';
import { Link } from 'react-router-dom';

class Button extends React.Component {
  // TODO: Implement
  shouldComponentUpdate() {
    return true;
  }

  render() {
    const {
      classes, href, icon, ...otherProps
    } = this.props;

    const buttonContent = (
      <div className={classes.buttonChildrenContainer}>
        {icon && (
          <div className={classes.buttonIcon}>
            <img
              alt="button-icon"
              className={classes.buttonIconImg}
              src={icon}
            />
          </div>
        )}
        <div className={classes.buttonText}>{this.props.children}</div>
      </div>
    );

    return href ? (
      <Link to={`/${href}`}>
        <MuiButton
          className={classes.root}
          color="primary"
          fullWidth
          variant="contained"
          {...otherProps}
        >
          {buttonContent}
        </MuiButton>
      </Link>
    ) : (
      <MuiButton
        className={classes.root}
        color="primary"
        fullWidth
        variant="contained"
        {...this.props}
      >
        {buttonContent}
      </MuiButton>
    );
  }
}

const styles = (theme) => ({
  buttonChildrenContainer: {
    display: 'flex',
    height: 48,
    width: '100%'
  },
  buttonIcon: {
    backgroundColor: 'white',
    borderRadius: '3px 0 0 3px',
    padding: '10px 4px',
    width: 48
  },
  buttonIconImg: {
    maxHeight: '100%',
    maxWidth: '100%'
  },
  buttonText: {
    flex: 1,
    fontSize: 16,
    paddingTop: 10,
    textAlign: 'center'
  },
  root: {
    '&:hover': {
      backgroundColor: darken(theme.palette.secondary.main, 0.1)
    },
    backgroundColor: theme.palette.secondary.main,
    boxShadow: 'none',
    padding: 2,
    textTransform: 'none'
  }
});

export default withStyles(styles, { withTheme: true })(Button);

./node_modules/.bin/eslint --ext .js --ext .jsx .

What did you expect to happen?
Everything to lint fine as it does in 6.4.0

What actually happened? Please include the actual, raw output from ESLint.

▶ npm run lint --prefix packages/client

> client@1.0.0 lint /Users/jakeleventhal/Projects/ecominate/packages/client
> ../../node_modules/.bin/eslint --ext .js --ext .jsx .

TypeError: Cannot read property 'type' of undefined
Occurred while linting /Users/jakeleventhal/Projects/ecominate/packages/client/src/view/SignInUp/Components/Button.jsx:13
    at checkDestructured (/Users/jakeleventhal/Projects/ecominate/node_modules/eslint/lib/rules/no-useless-rename.js:104:43)
    at /Users/jakeleventhal/Projects/ecominate/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/jakeleventhal/Projects/ecominate/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/Users/jakeleventhal/Projects/ecominate/node_modules/eslint/lib/linter/node-event-generator.js:253:26)
    at NodeEventGenerator.applySelectors (/Users/jakeleventhal/Projects/ecominate/node_modules/eslint/lib/linter/node-event-generator.js:282:22)
    at NodeEventGenerator.enterNode (/Users/jakeleventhal/Projects/ecominate/node_modules/eslint/lib/linter/node-event-generator.js:296:14)
    at CodePathAnalyzer.enterNode (/Users/jakeleventhal/Projects/ecominate/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:646:23)
    at /Users/jakeleventhal/Projects/ecominate/node_modules/eslint/lib/linter/linter.js:935:32
    at Array.forEach (<anonymous>)
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! client@1.0.0 lint: `../../node_modules/.bin/eslint --ext .js --ext .jsx .`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the client@1.0.0 lint script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/jakeleventhal/.npm/_logs/2019-09-29T06_40_12_714Z-debug.log

Are you willing to submit a pull request to fix this bug?
No

[g-plane]

Thanks for this issue.

Can you minimize your code example to help us ensure which part caused the bug? Thanks.

[kaicataldo]

It looks like this line is throwing an error on this node:

    const {
      classes, href, icon, ...otherProps
    } = this.props;

Do you mind checking to see if the same error occurs with the default parser? It might be some difference between Espree and babel-eslint.

[jakeleventhal]

The problem is the spread operator here i think

The following code causes the error

import * as React from 'react';

class CustomButton extends React.Component {
  render() {
    const { ...otherProps } = this.props;

    return (
      <div
        {...otherProps}
      />
    );
  }
}

export default CustomButton;

The following code does not cause an error:

import * as React from 'react';

class CustomButton extends React.Component {
  render() {
    return (
      <div
        {...this.props.otherProps}
      />
    );
  }
}

export default CustomButton;

[jakeleventhal]

I'd rather not swap my parser around but this is the most simplified example I can produce

[douxc]

It looks like this line is throwing an error on this node:

    const {
      classes, href, icon, ...otherProps
    } = this.props;

Do you mind checking to see if the same error occurs with the default parser? It might be some difference between Espree and babel-eslint.

same error, when change ...otherProps everything ok

[platinumazure]

I think the issue is with this section of the no-useless-rename code, if babel-eslint is still using ExperimentalRestProperty. The code is fine for espree.

With babel-eslint (at least version 9 in ASTExplorer), the code uses ExperimentalRestProperty, so it does not meet the condition in line 100 and thus bail out via continue on line 101. Instead, it executes line 104, which looks for properties on node.key when node.key does not exist.

Not sure if babel-eslint@10 or babel-eslint@11 would run into the same problem.

Similarly, I don't think espree should be broken-- @douxc If you think that line crashes while using the default parser, please provide more information about your configuration so we have a better idea of how to reproduce that. Thanks!

[g-plane]

I've tested with different parsers. Here is result:

Parser	Did it work?
espree	Yes
babel-eslint@9	No
babel-eslint@10	No
babel-eslint@11	No

[jakeleventhal]

Please note I wasn’t experiencing problems in 6.4.0

[mysticatea]

The root cause is that babel-eslint generates invalid AST for some syntax. I don't oppose to fix ESLint itself for this, but I hope babel-eslint to make valid AST.

Syntax	ESTree spec	babel-eslint
Rest properties	RestElement	ExperimentalRestProperty
Spread properties	SpreadElement	ExperimentalSpreadProperty
import()	ImportExpression	CallExpression with Import

[kopax]

I had the same issue and adding the eslint rule "no-useless-rename": 0 fixed the issue for me.

Is a fix scheduled for this?

[hawkeye64]

I have the same thing. With this code:

                    const key = (property.key.type === "Identifier" && property.key.name) || (property.key.type === "Literal" && property.key.value);
                    const renamedKey = property.value.type === "AssignmentPattern" ? property.value.left.name : property.value.name;

                    if (key === renamedKey) {
                        reportError(property, property.key, property.value, "Destructuring assignment");
                    }

and temporarily changed it in node-modules to this:

                if (property.key) {
                    const key = (property.key.type === "Identifier" && property.key.name) || (property.key.type === "Literal" && property.key.value);
                    const renamedKey = property.value.type === "AssignmentPattern" ? property.value.left.name : property.value.name;

                    if (key === renamedKey) {
                        reportError(property, property.key, property.value, "Destructuring assignment");
                    }
                }

Error is:

$ yarn lint
yarn run v1.17.3
$ eslint --ext .js,.vue src
TypeError: Cannot read property 'type' of undefined
Occurred while linting /data2/projects/quasar/app-extensions/qcalendar/demo/src/examples/DayViewSwipe.vue:34

Line of code complaining about is:

    handleSwipe ({ evt, ...info }) {

[platinumazure]

Hi @kopax, if we can identify an easy fix, there's a possibility it could get into a patch release early this coming week. Otherwise, the next release will be in about 2 weeks (close to October 11th). Please see our README for more information.

A PR would be welcome. I think adding a check for !node.key (maybe instead of the node.type === "RestProperty" check) would probably solve the issue. We would also want a unit test case that uses a mock babel-eslint parser and the generated AST, in order to avoid a regression here. 😄

Just as a general reminder for everyone: This project, like most open-source projects, is 100% volunteer-run and the maintainers can only do so much by themselves. I appreciate the great discussion and the quick report of the issue, and I also appreciate everyone's patience as we figure out how best to fix this.

[mdjermanovic]

An option is also node.type !== "Property" instead of node.type === "RestProperty"

[kaicataldo]

This is a bug in babel-eslint. I’m not opposed to adding a temporary fix to the rule, but it should be fixed by babel-eslint. I’ll take a look later today and see if we can get it patched there.

[g-plane]

Off topic:

@kaicataldo Maybe it can be done at babel/babel-eslint#785 .

[kaicataldo]

@g-plane Yes, that should fix it! Whipping up a PR to fix this on our end in the meantime.

[demeralde]

Instead of disabling the entire no-useless-rename rule as a workaround, you can just disable the check for destructuring:

{
  "no-useless-rename": [1, { "ignoreDestructuring": true }]
}

[kaicataldo]

@dspacejs This has been fixed in v6.5.1.