tools: avoid let in for loops by jalafel · Pull Request #9049 · nodejs/node · GitHub

Checklist

tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

Tools.

Description of change

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions.

Thanks for the PR! This generally looks good to me, aside from one small change.

The implementation for ForStatement nodes looks good. I think checking ForInStatement and ForOfStatement nodes is a good idea, but I don't think this rule is correctly checking those nodes at the moment, since they don't have an init property. Instead, I think you would want to check the left property.

Ah that's right. Thank you for pointing that out! I feel silly about that one. I will update the file.

So it looks like you will need to add an entry into the .eslintrc if you want to get the rule to work

You can check http://eslint.org/docs/developer-guide/working-with-rules#runtime-rules for more details

edit: you may want to alternatively put it in lib/.eslintrc if we only want to test the lib

once the rule is added you will notice TONS of failures in our test / benchmark directories. You will likely want to fix all of those in a separate commit. Once you have done that you will want to rebase the rule to after the changes, so that the repo will always be in a working state.

$ git rebase -i HEAD~5 # start and interactive rebase for the last 5 commits

this will give you a list of commits, you can then move the commits around in that list, save, and close. This will edit the history

You will likely want to compile and test locally to make sure it works.

$ ./configure
$ make -j8
$ make test

Let me know if you have any questions 😄

I don't think that we need to run both checks on every iteration. It may be better to have a bit of repeated code instead of doing an extra evaluation on every for loop.

That makes a lot of sense! I will update the test and add it to the .eslintrc. I was wondering if it would be more appropriate to report an error or simply a warning on the rule?

I'd go with error.

@thealphanerd okay! thank you

Is there any context on why to avoid let in loops?

Use checkForLet (point-free style). (node) => checkForLet(node) is a unnecessary wrapper.

@princejwesley I think this was based on the way a number of other lint rules have been designed

@Trott can you chime in?

I think the suggestion from @princejwesley is a good one. The other tests that use a wrapper are also passing context because it is out of scope the way they are written, but not this one.

Thank you @princejwesley ! I've learned (more formally) about tacit programming from this review. :)

@cosmosgenius See #8637. At the moment, var has better performance than let.

Nit: To be consistent with naming conventions in ESLint itself, maybe change the name of the rule from restrict-let-in-for-loops to no-let-in-for-loops or no-let-in-for-declarations or something like that.

Nit: Maybe specify that it's just the declarations (or whatever the right terminology is)? I think this makes it sound like we're forbidding it in the body as well, but we're not.

from the mdn docs

I think it would make sense to refer to it as the prohibit the use of let in for loop initialization

@Trott Definitely agree with you. I changed the wording to have Prohibit the use ofletas the loop variable in the initialization of for, and the left-hand iterator in forIn and forOf loops. Which may be too verbose? I just missed @thealphanerd comment before changing this. Let me know what you think!

I'm OK with comments being verbose. 👍

Aside/question: I know there are benchmarks that show for (let i=0; i<j; i++) is slower than using var. And @ofrobots warns against that usage, I believe. But is it also the case for let in for loop using in and/or of as well?

Only change I'd want to see is reporting the right column, although honestly, if that is onerous or controversial, this can land as is.

Nit: In my opinion, this ends up reporting the wrong column. It indicates the issue is in the column where for starts, but it should really report the column where let occurs. Is it possible to fix that? Maybe instead of returning a boolean, testForLoop() and friends can return the offending node and that can be passed to context.report() rather than the for node?

Thanks @Trott, with the review given from @thealphanerd, I broke up this function and returned the offensive node as you suggested to report the correct column!

Oh, and +1 to adding it to lib/.eslintrc only.

thanks for the review @Trott

@jessicaquynh if you move the rule to the lib/ folder you should no longer see any of the errors. Please let me know if you need any support on this at all.

LGTM.

CI: https://ci.nodejs.org/job/node-test-pull-request/4511/

nit: the subsystem should be tools, not eslint, in the commit message.

Thanks for the patience through the review process @jessicaquynh 🎉

LGTM

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#8873

LGTM

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#8873 PR-URL: nodejs#9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in b16a97e

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: #9045 Ref: #8873 PR-URL: #9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#9553 Ref: nodejs#8873 PR-URL: nodejs#9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: #9045 Ref: #9553 Ref: #8873 PR-URL: #9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 12, 2016

jalafel mentioned this pull request Oct 12, 2016

Create Lint Rule to avoid let in for loops #9045

Closed

not-an-aardvark suggested changes Oct 12, 2016

View reviewed changes

MylesBorins suggested changes Oct 12, 2016

View reviewed changes

princejwesley reviewed Oct 12, 2016

View reviewed changes

Trott reviewed Oct 12, 2016

View reviewed changes

Trott requested changes Oct 12, 2016

View reviewed changes

jalafel force-pushed the issue#9045 branch from cc249b0 to 442a742 Compare October 13, 2016 20:44

Trott approved these changes Oct 13, 2016

View reviewed changes

MylesBorins approved these changes Oct 13, 2016

View reviewed changes

not-an-aardvark approved these changes Oct 13, 2016

View reviewed changes

princejwesley approved these changes Oct 13, 2016

View reviewed changes

tools: avoid let in for loops

8b704d6

This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#8873

jalafel force-pushed the issue#9045 branch from 442a742 to 8b704d6 Compare October 14, 2016 02:19

jalafel changed the title eslint: avoid let in for loops tools: avoid let in for loops Oct 14, 2016

jasnell approved these changes Oct 14, 2016

View reviewed changes

Trott closed this Oct 14, 2016

MylesBorins added land-on-v6.x labels Nov 11, 2016

MylesBorins mentioned this pull request Nov 11, 2016

Backport removing let in for loop declaration #9553

Closed

3 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 18, 2016

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

not-an-aardvark mentioned this pull request Jan 13, 2017

test,util: remove lint workarounds #10785

Closed

2 tasks

aliscco mentioned this pull request Dec 1, 2023

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#903

Open

leonardoadame mentioned this pull request Dec 3, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#508

Open

tools: avoid let in for loops by jalafel · Pull Request #9049 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: avoid let in for loops - #9049

tools: avoid let in for loops#9049jalafel wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jalafel:issue#9045jalafel/node:issue#9045Copy head branch name to clipboard

Conversation

jalafel commented Oct 12, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

not-an-aardvark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosmosgenius commented Oct 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark commented Oct 12, 2016

Uh oh!

Trott commented Oct 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 12, 2016

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 12, 2016

Uh oh!

MylesBorins commented Oct 13, 2016

Uh oh!

Trott commented Oct 13, 2016

Uh oh!

Trott commented Oct 13, 2016

Uh oh!

MylesBorins commented Oct 13, 2016

Uh oh!

princejwesley left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 14, 2016

Uh oh!

tools: avoid let in for loops#9049
jalafel wants to merge 1 commit into
nodejs:masterfrom
jalafel:issue#9045

MylesBorins commented Oct 12, 2016 •
edited
Loading

Trott Oct 12, 2016 •
edited
Loading