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

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

Comments

@jakeleventhal
Copy link

@jakeleventhal jakeleventhal commented Sep 29, 2019

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

This comment has been minimized.

Copy link
Member

@g-plane g-plane commented Sep 29, 2019

Thanks for this issue.

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

@g-plane g-plane added evaluating rule and removed triage labels Sep 29, 2019
@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Sep 29, 2019

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

This comment has been minimized.

Copy link
Author

@jakeleventhal jakeleventhal commented Sep 29, 2019

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

This comment has been minimized.

Copy link
Author

@jakeleventhal jakeleventhal commented Sep 29, 2019

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

@douxc

This comment has been minimized.

Copy link

@douxc douxc commented Sep 29, 2019

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

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Sep 29, 2019

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

This comment has been minimized.

Copy link
Member

@g-plane g-plane commented Sep 29, 2019

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
@g-plane g-plane changed the title Bug when upgrading from 6.4.0 to 6.5.0 `no-useless-rename` doesn't work when upgrading from 6.4.0 to 6.5.0 Sep 29, 2019
@jakeleventhal

This comment has been minimized.

Copy link
Author

@jakeleventhal jakeleventhal commented Sep 29, 2019

Please note I wasn’t experiencing problems in 6.4.0

pi0 added a commit to nuxt/nuxt.js that referenced this issue Sep 29, 2019
@mysticatea

This comment has been minimized.

Copy link
Member

@mysticatea mysticatea commented Sep 29, 2019

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
@mysticatea mysticatea changed the title `no-useless-rename` doesn't work when upgrading from 6.4.0 to 6.5.0 [6.5.0] `no-useless-rename` with `babel-eslint` crash at rest properties Sep 29, 2019
@kopax

This comment has been minimized.

Copy link

@kopax kopax commented Sep 29, 2019

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?

@mysticatea mysticatea added accepted and removed evaluating labels Sep 29, 2019
@hawkeye64

This comment has been minimized.

Copy link

@hawkeye64 hawkeye64 commented Sep 29, 2019

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

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Sep 29, 2019

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

This comment has been minimized.

Copy link
Member

@mdjermanovic mdjermanovic commented Sep 29, 2019

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

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Sep 29, 2019

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

This comment has been minimized.

Copy link
Member

@g-plane g-plane commented Sep 29, 2019

Off topic:

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

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Sep 29, 2019

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

@dspacejs

This comment has been minimized.

Copy link

@dspacejs dspacejs commented Oct 9, 2019

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

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Oct 9, 2019

@dspacejs This has been fixed in v6.5.1.

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