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

Fix: Disable no-var autofixer in some incorrect cases in loops #7811

Merged
merged 1 commit into from Dec 27, 2016

Conversation

Projects
None yet
7 participants
@alangpierce
Contributor

alangpierce commented Dec 23, 2016

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Tell us about your environment

  • ESLint Version: 3.12.2
  • Node Version: 6.9.1
  • npm Version: 3.10.8

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

default

Please show your full configuration:

module.exports = {
  env: {
    "es6": true
  },
  rules: {
    "no-var": "error"
  }
};

What did you do? Please include the actual source code causing the issue.

I ran eslint --fix on a file with this code:

for (const val of [1, 2, 3]) {
  var seen;
  console.log(seen);
  seen = true;
}

What did you expect to happen?

I expected the var to remain, so that the output is:

undefined
true
true

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

No errors were reported, and the var was changed to let, changing the behavior to have this output:

undefined
undefined
undefined

What changes did you make? (Give an overview)

The autofixer for the no-var rule was converting var to let within loops, but in some cases that can introduce incorrect behavior because let variables in loops only live for the lifetime of their loop iteration, while var variables within loops use the same variable across all iterations.

We can still convert to let in typical cases as long as we check for cases that might cause a behavior difference:

  • If the variable is referenced from a closure, then that closure might be called after the current loop iteration ends. For var declarations, the closure refers to the shared variable across all iterations, and for let declarations, the closure refers to the variable just from that one iteration, so changing var to let can change the behavior.
  • If the variable is used before its first assignment in the loop body, then for var declarations it will retain its value from the previous iteration, but for let declarations it will start as undefined.

This change skips the autofixer for any variables referenced by any closure, and for any variables that are not initialized right when they are declared. Some additional static analysis can make both of these cases smarter, but this should handle most common cases.

I also updated the docs to fix a typo and use the same phrasing as the prefer-const rule, since not all cases can be fixed.

Also, since isInLoop was already used in two places and I needed a third, I moved it into ast-utils, which required reworking things there a little.

For a little more context, I recently implemented a similar fix for the same issue in the esnext project:
esnext/esnext#113
decaffeinate/decaffeinate#624

(The reason I have code that looks like that in the first place is that it's auto-generated from CoffeeScript using the decaffeinate project, so var declarations get added in a way that's correct, but not always clean.)

Is there anything you'd like reviewers to focus on?

I think the main thing is to sanity check that this change indeed fixes the correctness issues I mentioned, and that the autofixer's behavior is still reasonable after this change. Also, since this is my first eslint PR, there may be style issues or other things I've overlooked.

I also was a little unsure how to move isInLoop into ast-utils. I ended up moving some other functions to be top-level helpers so I could call them, but let me know if I should have added it to the module.exports object.

Also, the isLoopAssignee function checks for ForInStatement and ForOfStatement, and ideally it would also check for for await statements once those get standardized (currently they're stage 3). If there's a way to make sure that gets added later or a way to make the code more future-proof, would be nice.

@jsf-clabot

This comment has been minimized.

Show comment
Hide comment
@jsf-clabot

jsf-clabot Dec 23, 2016

CLA assistant check
All committers have signed the CLA.

jsf-clabot commented Dec 23, 2016

CLA assistant check
All committers have signed the CLA.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Dec 23, 2016

@alangpierce, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @kaicataldo and @vitorbal to be potential reviewers.

mention-bot commented Dec 23, 2016

@alangpierce, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @kaicataldo and @vitorbal to be potential reviewers.

@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Dec 23, 2016

LGTM

@not-an-aardvark

Thanks! This generally looks very good -- I just have one minor suggestion to add some tests for isInLoop.

* @param {ASTNode} node - The node to check.
* @returns {boolean} `true` if the node is in a loop.
*/
function isInLoop(node) {

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Dec 24, 2016

Member

Since this has been moved to ast-utils.js, I think it might be good to add separate tests for it (in tests/lib/ast-utils.js).

@not-an-aardvark

not-an-aardvark Dec 24, 2016

Member

Since this has been moved to ast-utils.js, I think it might be good to add separate tests for it (in tests/lib/ast-utils.js).

This comment has been minimized.

@alangpierce

alangpierce Dec 24, 2016

Contributor

Ok, I just pushed an update that added some tests for this function.

@alangpierce

alangpierce Dec 24, 2016

Contributor

Ok, I just pushed an update that added some tests for this function.

Fix: Disable no-var autofixer in some incorrect cases in loops
The autofixer for the no-var rule was converting `var` to `let` within loops,
but in some cases that can introduce incorrect behavior because `let` variables
in loops only live for the lifetime of their loop iteration, while `var`
variables within loops use the same variable across all iterations.

We can still convert to `let` in typical cases as long as we check for cases
that might cause a behavior difference:
* If the variable is referenced from a closure, then that closure might be
  called after the current loop iteration ends. For `var` declarations, the
  closure refers to the shared variable across all iterations, and for `let`
  declarations, the closure refers to the variable just from that one iteration.
* If the variable is used before its first assignment in the loop body, then for
  `var` declarations it will retain its value from the previous iteration, but
  for `let` declarations it will start as undefined.

This change skips the autofixer for any variables referenced by any closure, and
for any variables that are not initialized right when they are declared. Some
additional static analysis can make both of these cases smarter, but this should
handle most common cases.
@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Dec 24, 2016

LGTM

@not-an-aardvark

LGTM. Thanks!

@platinumazure

LGTM, thanks!

@not-an-aardvark not-an-aardvark merged commit fd4cd3b into eslint:master Dec 27, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

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

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.