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

sort-vars rule fails when memo is undefined #3474

Closed
gajus opened this Issue Aug 21, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@gajus
Contributor

gajus commented Aug 21, 2015

sort-vars rule crashes when memo is undefined.

node.declarations.reduce(function(memo, decl) {
    if (decl.id.type !== "ObjectPattern" && decl.id.type !== "ArrayPattern") {
        var lastVariableName,
            currenVariableName;

        lastVariableName = memo.id.name;
        currenVariableName = decl.id.name;

        if (ignoreCase) {
            lastVariableName = lastVariableName.toLowerCase();
            currenVariableName = currenVariableName.toLowerCase();
        }

        if (currenVariableName < lastVariableName) {
            context.report(decl, "Variables within the same declaration block should be sorted alphabetically");
            return memo;
        } else {
            return decl;
        }
    }
}, node.declarations[0]);

This affects tools such as eslint-watch and gulp-eslint, e.g.

adametry/gulp-eslint#86

@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot Aug 21, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

eslintbot commented Aug 21, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@eslintbot eslintbot added the triage label Aug 21, 2015

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Aug 21, 2015

Member

Can you provide a code example to reproduce this error?

Member

lo1tuma commented Aug 21, 2015

Can you provide a code example to reproduce this error?

@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Aug 21, 2015

Contributor

The code that is failing:

let selector,
    locationSelector = state => state.locations,
    screeningSelector = state => state.screenings,
    eventSelector;
let {
        foo,
        bar
    } = this.props;
Contributor

gajus commented Aug 21, 2015

The code that is failing:

let selector,
    locationSelector = state => state.locations,
    screeningSelector = state => state.screenings,
    eventSelector;
let {
        foo,
        bar
    } = this.props;
@BYK

This comment has been minimized.

Show comment
Hide comment
@BYK

BYK Aug 21, 2015

Member

@gajus - I can't reproduce this error on latest master. Which version of eslint are those packages using? Uninstalling and reinstalling them would probably solve the problem.

Member

BYK commented Aug 21, 2015

@gajus - I can't reproduce this error on latest master. Which version of eslint are those packages using? Uninstalling and reinstalling them would probably solve the problem.

@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Aug 21, 2015

Contributor

@BYK I have made sure that packages use the latest version of ESLint.

Contributor

gajus commented Aug 21, 2015

@BYK I have made sure that packages use the latest version of ESLint.

@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Aug 21, 2015

Contributor

Just to be clear, I could not reproduce the same issue using the ESLint CLI tool.

Contributor

gajus commented Aug 21, 2015

Just to be clear, I could not reproduce the same issue using the ESLint CLI tool.

@BYK

This comment has been minimized.

Show comment
Hide comment
@BYK

BYK Aug 21, 2015

Member

@gajus - Same here, can't repro in tests either. May be this is an issue with those packages?

Also, the only way memo can be undefined is when node.declarations is empty which is not possible.

Member

BYK commented Aug 21, 2015

@gajus - Same here, can't repro in tests either. May be this is an issue with those packages?

Also, the only way memo can be undefined is when node.declarations is empty which is not possible.

@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Aug 21, 2015

Contributor

I have opened the issue with gulp-eslint too, adametry/gulp-eslint#86. Lets wait for their follow up. It might be an issue in those two packages. It would be a bit odd, though if two completely separate packages implement the same error.

Contributor

gajus commented Aug 21, 2015

I have opened the issue with gulp-eslint too, adametry/gulp-eslint#86. Lets wait for their follow up. It might be an issue in those two packages. It would be a bit odd, though if two completely separate packages implement the same error.

@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Aug 24, 2015

Contributor

I have made a mistake. This issue is reproducible with vanilla ESLint using the following configuration:

{
    "ecmaFeatures": {
        "modules": true
    },
    "env": {
        "es6": true
    },
    "rules": {
        "sort-vars": 2
    }
}

https://github.com/gajus/eslint-issue-3474

Contributor

gajus commented Aug 24, 2015

I have made a mistake. This issue is reproducible with vanilla ESLint using the following configuration:

{
    "ecmaFeatures": {
        "modules": true
    },
    "env": {
        "es6": true
    },
    "rules": {
        "sort-vars": 2
    }
}

https://github.com/gajus/eslint-issue-3474

@BYK

This comment has been minimized.

Show comment
Hide comment
@BYK

BYK Aug 24, 2015

Member

I've also read the code incorrectly. I now can see how it can get an undefined memo.

Member

BYK commented Aug 24, 2015

I've also read the code incorrectly. I now can see how it can get an undefined memo.

@BYK BYK added bug rule accepted and removed triage labels Aug 24, 2015

@BYK BYK self-assigned this Aug 24, 2015

BYK added a commit that referenced this issue Aug 24, 2015

BYK added a commit that referenced this issue Aug 24, 2015

@nzakas nzakas closed this in #3500 Aug 24, 2015

@gvidon

This comment has been minimized.

Show comment
Hide comment
@gvidon

gvidon Jul 12, 2016

@BYK I can still reproduce it with eslint v3.0.1

Eslint config

{
  "parserOptions": {
    "ecmaVersion": 6,
    "sourceType": "module",

    "ecmaFeatures": {
      "experimentalObjectRestSpread": true,
      "modules": true,
      "jsx": true
    }
  },

  "rules": {
    "sort-vars": [1, {"ignoreCase": true}]
  }
}

Error message

Cannot read property 'toLowerCase' of undefined
TypeError: Cannot read property 'toLowerCase' of undefined
    at /usr/local/lib/node_modules/eslint/lib/rules/sort-vars.js:49:60
    at Array.reduce (native)
    at EventEmitter.VariableDeclaration (/usr/local/lib/node_modules/eslint/lib/rules/sort-vars.js:40:35)
    at emitOne (events.js:82:20)
    at EventEmitter.emit (events.js:169:7)
    at NodeEventGenerator.enterNode (/usr/local/lib/node_modules/eslint/lib/util/node-event-generator.js:40:22)
    at CodePathAnalyzer.enterNode (/usr/local/lib/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:607:23)
    at CommentEventGenerator.enterNode (/usr/local/lib/node_modules/eslint/lib/util/comment-event-generator.js:97:23)
    at Controller.traverser.traverse.enter (/usr/local/lib/node_modules/eslint/lib/eslint.js:905:36)
    at Controller.__execute (/usr/local/lib/node_modules/eslint/node_modules/estraverse/estraverse.js:397:31)

Code caused exception

class Calendar extends Component {
  render() {
    const
      {params: {date = defaultDate}, route: {scale = defaultScale}, pushURL, events} = this.props,
      ChildComponent = {Day, Week, Month}[_.capitalize(scale)];
  };
}

export default Calendar;

No error if I remove second var declaration:

class Calendar extends Component {
  render() {
    const
      {params: {date = defaultDate}, route: {scale = defaultScale}, pushURL, events} = this.props;
  };
}

export default Calendar;

gvidon commented Jul 12, 2016

@BYK I can still reproduce it with eslint v3.0.1

Eslint config

{
  "parserOptions": {
    "ecmaVersion": 6,
    "sourceType": "module",

    "ecmaFeatures": {
      "experimentalObjectRestSpread": true,
      "modules": true,
      "jsx": true
    }
  },

  "rules": {
    "sort-vars": [1, {"ignoreCase": true}]
  }
}

Error message

Cannot read property 'toLowerCase' of undefined
TypeError: Cannot read property 'toLowerCase' of undefined
    at /usr/local/lib/node_modules/eslint/lib/rules/sort-vars.js:49:60
    at Array.reduce (native)
    at EventEmitter.VariableDeclaration (/usr/local/lib/node_modules/eslint/lib/rules/sort-vars.js:40:35)
    at emitOne (events.js:82:20)
    at EventEmitter.emit (events.js:169:7)
    at NodeEventGenerator.enterNode (/usr/local/lib/node_modules/eslint/lib/util/node-event-generator.js:40:22)
    at CodePathAnalyzer.enterNode (/usr/local/lib/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:607:23)
    at CommentEventGenerator.enterNode (/usr/local/lib/node_modules/eslint/lib/util/comment-event-generator.js:97:23)
    at Controller.traverser.traverse.enter (/usr/local/lib/node_modules/eslint/lib/eslint.js:905:36)
    at Controller.__execute (/usr/local/lib/node_modules/eslint/node_modules/estraverse/estraverse.js:397:31)

Code caused exception

class Calendar extends Component {
  render() {
    const
      {params: {date = defaultDate}, route: {scale = defaultScale}, pushURL, events} = this.props,
      ChildComponent = {Day, Week, Month}[_.capitalize(scale)];
  };
}

export default Calendar;

No error if I remove second var declaration:

class Calendar extends Component {
  render() {
    const
      {params: {date = defaultDate}, route: {scale = defaultScale}, pushURL, events} = this.props;
  };
}

export default Calendar;
@gvidon

This comment has been minimized.

Show comment
Hide comment
@gvidon

gvidon Jul 12, 2016

Even this cause an exception:

handleDayClick(item) {
  const
    {selectDateHandler, uniqueId, handlePickDate} = this.props,
    date = getDate(item);

    selectDateHandler(uniqueId, date.format(settings.DATE_FORMAT));
    handlePickDate(date);
}

gvidon commented Jul 12, 2016

Even this cause an exception:

handleDayClick(item) {
  const
    {selectDateHandler, uniqueId, handlePickDate} = this.props,
    date = getDate(item);

    selectDateHandler(uniqueId, date.format(settings.DATE_FORMAT));
    handlePickDate(date);
}

@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.