Skip to content

Commit

Permalink
Update: Improve no-loop-func rule description and message (#11046)
Browse files Browse the repository at this point in the history 
The rule does not disallow declaring functions in loops, it disallows making unsafe references inside said functions.

The current description is misleading and might lead someone to dismiss the rule as irrelevant/unnecessary, when in fact it's protecting against a subtle but nasty source of bugs.
  • Loading branch information
@paol @platinumazure
paol authored and platinumazure committed Apr 13, 2019
1 parent 608a02c commit 5cfdc2d
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 33 deletions.
21 changes: 14 additions & 7 deletions lib/rules/no-loop-func.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,13 +157,17 @@ module.exports = {
type: "suggestion",

docs: {
description: "disallow `function` declarations and expressions inside loop statements",
description: "disallow function declarations that contain unsafe references inside loop statements",
category: "Best Practices",
recommended: false,
url: "https://eslint.org/docs/rules/no-loop-func"
},

schema: []
schema: [],

messages: {
unsafeRefs: "Function declared in a loop contains unsafe references to variable(s) {{ varNames }}."
}
},

create(context) {
Expand All @@ -185,11 +189,14 @@ module.exports = {
}

const references = context.getScope().through;

if (references.length > 0 &&
!references.every(isSafe.bind(null, loopNode))
) {
context.report({ node, message: "Don't make functions within a loop." });
const unsafeRefs = references.filter(r => !isSafe(loopNode, r)).map(r => r.identifier.name);

if (unsafeRefs.length > 0) {
context.report({
node,
messageId: "unsafeRefs",
data: { varNames: `'${unsafeRefs.join("', '")}'` }
});
}
}

Expand Down
55 changes: 29 additions & 26 deletions tests/lib/rules/no-loop-func.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ const rule = require("../../../lib/rules/no-loop-func"),
// Tests
//------------------------------------------------------------------------------

const ruleTester = new RuleTester(),
expectedErrorMessage = "Don't make functions within a loop.";
const ruleTester = new RuleTester();

ruleTester.run("no-loop-func", rule, {
valid: [
Expand Down Expand Up @@ -117,117 +116,121 @@ ruleTester.run("no-loop-func", rule, {
invalid: [
{
code: "for (var i=0; i<l; i++) { (function() { i; }) }",
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "FunctionExpression" }]
},
{
code: "for (var i=0; i<l; i++) { for (var j=0; j<m; j++) { (function() { i+j; }) } }",
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i', 'j'" }, type: "FunctionExpression" }]
},
{
code: "for (var i in {}) { (function() { i; }) }",
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "FunctionExpression" }]
},
{
code: "for (var i of {}) { (function() { i; }) }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "FunctionExpression" }]
},
{
code: "for (var i=0; i < l; i++) { (() => { i; }) }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "ArrowFunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }]
},
{
code: "for (var i=0; i < l; i++) { var a = function() { i; } }",
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "FunctionExpression" }]
},
{
code: "for (var i=0; i < l; i++) { function a() { i; }; a(); }",
errors: [{ message: expectedErrorMessage, type: "FunctionDeclaration" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "FunctionDeclaration" }]
},
{
code: "for (var i=0; (function() { i; })(), i<l; i++) { }",
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "FunctionExpression" }]
},
{
code: "for (var i=0; i<l; (function() { i; })(), i++) { }",
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "FunctionExpression" }]
},
{
code: "while(i) { (function() { i; }) }",
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "FunctionExpression" }]
},
{
code: "do { (function() { i; }) } while (i)",
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "FunctionExpression" }]
},

// Warns functions which are using modified variables.
{
code: "let a; for (let i=0; i<l; i++) { a = 1; (function() { a; });}",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
code: "let a; for (let i in {}) { (function() { a; }); a = 1; }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
code: "let a; for (let i of {}) { (function() { a; }); } a = 1; ",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
code: "let a; for (let i=0; i<l; i++) { (function() { (function() { a; }); }); a = 1; }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
code: "let a; for (let i in {}) { a = 1; function foo() { (function() { a; }); } }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionDeclaration" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionDeclaration" }]
},
{
code: "let a; for (let i of {}) { (() => { (function() { a; }); }); } a = 1;",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "ArrowFunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "ArrowFunctionExpression" }]
},
{
code: "for (var i = 0; i < 10; ++i) { for (let x in xs.filter(x => x != i)) { } }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "ArrowFunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'i'" }, type: "ArrowFunctionExpression" }]
},
{
code: "for (let x of xs) { let a; for (let y of ys) { a = 1; (function() { a; }); } }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
code: "for (var x of xs) { for (let y of ys) { (function() { x; }); } }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'x'" }, type: "FunctionExpression" }]
},
{
code: "for (var x of xs) { (function() { x; }); }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'x'" }, type: "FunctionExpression" }]
},
{
code: "var a; for (let x of xs) { a = 1; (function() { a; }); }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
code: "var a; for (let x of xs) { (function() { a; }); a = 1; }",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
code: "let a; function foo() { a = 10; } for (let x of xs) { (function() { a; }); } foo();",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
code: "let a; function foo() { a = 10; for (let x of xs) { (function() { a; }); } } foo();",
parserOptions: { ecmaVersion: 6 },
errors: [{ message: expectedErrorMessage, type: "FunctionExpression" }]
errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
}
]
});

0 comments on commit 5cfdc2d

Please sign in to comment.