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

tools: avoid let in for loops #9049

Closed
wants to merge 1 commit into from
Closed

Conversation

jalafel
Copy link
Contributor

@jalafel jalafel commented Oct 12, 2016

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.

Fixes: #9045
Ref: #8873

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

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return {
'ForStatement': (node) => testForLoop(node),
'ForInStatement': (node) => testForLoop(node),
'ForOfStatement': (node) => testForLoop(node)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 12, 2016

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

tumblr_ob945yoevc1qg2p2fo1_500

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 😄

* Report function to test if the for-loop is declared using `let`.
*/
function checkForLet(node) {
if (testForLoop(node) || testForInOfLoop(node)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thealphanerd okay! thank you

@cosmosgenius
Copy link
Contributor

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


return {
'ForStatement': (node) => checkForLet(node),
'ForInStatement': (node) => checkForLet(node),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Trott can you chime in?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@not-an-aardvark
Copy link
Contributor

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

@Trott
Copy link
Member

Trott commented Oct 12, 2016

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.

@@ -0,0 +1,41 @@
/**
* @fileoverview Prohibit the use of `let` in `for` loops.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with comments being verbose. 👍

@Trott
Copy link
Member

Trott commented Oct 12, 2016

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?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
function checkForLet(node) {
if (testForLoop(node) || testForInOfLoop(node)) {
context.report(node, msg);
Copy link
Member

@Trott Trott Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@Trott
Copy link
Member

Trott commented Oct 12, 2016

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

@MylesBorins
Copy link
Contributor

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.

@Trott
Copy link
Member

Trott commented Oct 13, 2016

LGTM.

@Trott
Copy link
Member

Trott commented Oct 13, 2016

@MylesBorins
Copy link
Contributor

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

Thanks for the patience through the review process @jessicaquynh 🎉

Copy link
Contributor

@princejwesley princejwesley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

Fixes: nodejs#9045
Ref: nodejs#8873
@jalafel jalafel changed the title eslint: avoid let in for loops tools: avoid let in for loops Oct 14, 2016
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 14, 2016
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>
@Trott
Copy link
Member

Trott commented Oct 14, 2016

Landed in b16a97e

@Trott Trott closed this Oct 14, 2016
jasnell pushed a commit that referenced this pull request Oct 17, 2016
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>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Nov 11, 2016
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>
MylesBorins pushed a commit that referenced this pull request Nov 14, 2016
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>
This was referenced Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Lint Rule to avoid let in for loops
8 participants