diff --git a/docs/rules/no-unreachable-loop.md b/docs/rules/no-unreachable-loop.md new file mode 100644 index 00000000000..c58b97f382d --- /dev/null +++ b/docs/rules/no-unreachable-loop.md @@ -0,0 +1,190 @@ +# Disallow loops with a body that allows only one iteration (no-unreachable-loop) + +A loop that can never reach the second iteration is a possible error in the code. + +```js +for (let i = 0; i < arr.length; i++) { + if (arr[i].name === myName) { + doSomething(arr[i]); + // break was supposed to be here + } + break; +} +``` + +In rare cases where only one iteration (or at most one iteration) is intended behavior, the code should be refactored to use `if` conditionals instead of `while`, `do-while` and `for` loops. It's considered a best practice to avoid using loop constructs for such cases. + +## Rule Details + +This rule aims to detect and disallow loops that can have at most one iteration, by performing static code path analysis on loop bodies. + +In particular, this rule will disallow a loop with a body that exits the loop in all code paths. If all code paths in the loop's body will end with either a `break`, `return` or a `throw` statement, the second iteration of such loop is certainly unreachable, regardless of the loop's condition. + +This rule checks `while`, `do-while`, `for`, `for-in` and `for-of` loops. You can optionally disable checks for each of these constructs. + +Examples of **incorrect** code for this rule: + +```js +/*eslint no-unreachable-loop: "error"*/ + +while (foo) { + doSomething(foo); + foo = foo.parent; + break; +} + +function verifyList(head) { + let item = head; + do { + if (verify(item)) { + return true; + } else { + return false; + } + } while (item); +} + +function findSomething(arr) { + for (var i = 0; i < arr.length; i++) { + if (isSomething(arr[i])) { + return arr[i]; + } else { + throw new Error("Doesn't exist."); + } + } +} + +for (key in obj) { + if (key.startsWith("_")) { + break; + } + firstKey = key; + firstValue = obj[key]; + break; +} + +for (foo of bar) { + if (foo.id === id) { + doSomething(foo); + } + break; +} +``` + +Examples of **correct** code for this rule: + +```js +/*eslint no-unreachable-loop: "error"*/ + +while (foo) { + doSomething(foo); + foo = foo.parent; +} + +function verifyList(head) { + let item = head; + do { + if (verify(item)) { + item = item.next; + } else { + return false; + } + } while (item); + + return true; +} + +function findSomething(arr) { + for (var i = 0; i < arr.length; i++) { + if (isSomething(arr[i])) { + return arr[i]; + } + } + throw new Error("Doesn't exist."); +} + +for (key in obj) { + if (key.startsWith("_")) { + continue; + } + firstKey = key; + firstValue = obj[key]; + break; +} + +for (foo of bar) { + if (foo.id === id) { + doSomething(foo); + break; + } +} +``` + +Please note that this rule is not designed to check loop conditions, and will not warn in cases such as the following examples. + +Examples of additional **correct** code for this rule: + +```js +/*eslint no-unreachable-loop: "error"*/ + +do { + doSomething(); +} while (false) + +for (let i = 0; i < 1; i++) { + doSomething(i); +} + +for (const a of [1]) { + doSomething(a); +} +``` + +## Options + +This rule has an object option, with one option: + +* `"ignore"` - an optional array of loop types that will be ignored by this rule. + +## ignore + +You can specify up to 5 different elements in the `"ignore"` array: + +* `"WhileStatement"` - to ignore all `while` loops. +* `"DoWhileStatement"` - to ignore all `do-while` loops. +* `"ForStatement"` - to ignore all `for` loops (does not apply to `for-in` and `for-of` loops). +* `"ForInStatement"` - to ignore all `for-in` loops. +* `"ForOfStatement"` - to ignore all `for-of` loops. + +Examples of **correct** code for this rule with the `"ignore"` option: + +```js +/*eslint no-unreachable-loop: ["error", { "ignore": ["ForInStatement", "ForOfStatement"] }]*/ + +for (var key in obj) { + hasEnumerableProperties = true; + break; +} + +for (const a of b) break; +``` + +## Known Limitations + +Static code path analysis, in general, does not evaluate conditions. Due to this fact, this rule might miss reporting cases such as the following: + +```js +for (let i = 0; i < 10; i++) { + doSomething(i); + if (true) { + break; + } +} +``` + +## Related Rules + +* [no-unreachable](no-unreachable.md) +* [no-constant-condition](no-constant-condition.md) +* [no-unmodified-loop-condition](no-unmodified-loop-condition.md) +* [for-direction](for-direction.md) diff --git a/lib/rules/index.js b/lib/rules/index.js index 9e5571dd97d..6a211e598c9 100644 --- a/lib/rules/index.js +++ b/lib/rules/index.js @@ -212,6 +212,7 @@ module.exports = new LazyLoadingRuleMap(Object.entries({ "no-unmodified-loop-condition": () => require("./no-unmodified-loop-condition"), "no-unneeded-ternary": () => require("./no-unneeded-ternary"), "no-unreachable": () => require("./no-unreachable"), + "no-unreachable-loop": () => require("./no-unreachable-loop"), "no-unsafe-finally": () => require("./no-unsafe-finally"), "no-unsafe-negation": () => require("./no-unsafe-negation"), "no-unused-expressions": () => require("./no-unused-expressions"), diff --git a/lib/rules/no-unreachable-loop.js b/lib/rules/no-unreachable-loop.js new file mode 100644 index 00000000000..868a6ff98f8 --- /dev/null +++ b/lib/rules/no-unreachable-loop.js @@ -0,0 +1,150 @@ +/** + * @fileoverview Rule to disallow loops with a body that allows only one iteration + * @author Milos Djermanovic + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +const allLoopTypes = ["WhileStatement", "DoWhileStatement", "ForStatement", "ForInStatement", "ForOfStatement"]; + +/** + * Determines whether the given node is the first node in the code path to which a loop statement + * 'loops' for the next iteration. + * @param {ASTNode} node The node to check. + * @returns {boolean} `true` if the node is a looping target. + */ +function isLoopingTarget(node) { + const parent = node.parent; + + if (parent) { + switch (parent.type) { + case "WhileStatement": + return node === parent.test; + case "DoWhileStatement": + return node === parent.body; + case "ForStatement": + return node === (parent.update || parent.test || parent.body); + case "ForInStatement": + case "ForOfStatement": + return node === parent.left; + + // no default + } + } + + return false; +} + +/** + * Creates an array with elements from the first given array that are not included in the second given array. + * @param {Array} arrA The array to compare from. + * @param {Array} arrB The array to compare against. + * @returns {Array} a new array that represents `arrA \ arrB`. + */ +function getDifference(arrA, arrB) { + return arrA.filter(a => !arrB.includes(a)); +} + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = { + meta: { + type: "problem", + + docs: { + description: "disallow loops with a body that allows only one iteration", + category: "Possible Errors", + recommended: false, + url: "https://eslint.org/docs/rules/no-unreachable-loop" + }, + + schema: [{ + type: "object", + properties: { + ignore: { + type: "array", + items: { + enum: allLoopTypes + }, + uniqueItems: true + } + }, + additionalProperties: false + }], + + messages: { + invalid: "Invalid loop. Its body allows only one iteration." + } + }, + + create(context) { + const ignoredLoopTypes = context.options[0] && context.options[0].ignore || [], + loopTypesToCheck = getDifference(allLoopTypes, ignoredLoopTypes), + loopSelector = loopTypesToCheck.join(","), + loopsByTargetSegments = new Map(), + loopsToReport = new Set(); + + let currentCodePath = null; + + return { + onCodePathStart(codePath) { + currentCodePath = codePath; + }, + + onCodePathEnd() { + currentCodePath = currentCodePath.upper; + }, + + [loopSelector](node) { + + /** + * Ignore unreachable loop statements to avoid unnecessary complexity in the implementation, or false positives otherwise. + * For unreachable segments, the code path analysis does not raise events required for this implementation. + */ + if (currentCodePath.currentSegments.some(segment => segment.reachable)) { + loopsToReport.add(node); + } + }, + + onCodePathSegmentStart(segment, node) { + if (isLoopingTarget(node)) { + const loop = node.parent; + + loopsByTargetSegments.set(segment, loop); + } + }, + + onCodePathSegmentLoop(_, toSegment, node) { + const loop = loopsByTargetSegments.get(toSegment); + + /** + * The second iteration is reachable, meaning that the loop is valid by the logic of this rule, + * only if there is at least one loop event with the appropriate target (which has been already + * determined in the `loopsByTargetSegments` map), raised from either: + * + * - the end of the loop's body (in which case `node === loop`) + * - a `continue` statement + * + * This condition skips loop events raised from `ForInStatement > .right` and `ForOfStatement > .right` nodes. + */ + if (node === loop || node.type === "ContinueStatement") { + + // Removes loop if it exists in the set. Otherwise, `Set#delete` has no effect and doesn't throw. + loopsToReport.delete(loop); + } + }, + + "Program:exit"() { + loopsToReport.forEach( + node => context.report({ node, messageId: "invalid" }) + ); + } + }; + } +}; diff --git a/tests/lib/rules/no-unreachable-loop.js b/tests/lib/rules/no-unreachable-loop.js new file mode 100644 index 00000000000..10eaed40659 --- /dev/null +++ b/tests/lib/rules/no-unreachable-loop.js @@ -0,0 +1,431 @@ +/** + * @fileoverview Tests for the no-unreachable-loop rule + * @author Milos Djermanovic + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require("../../../lib/rules/no-unreachable-loop"); +const { RuleTester } = require("../../../lib/rule-tester"); + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2018 } }); + +const loopTemplates = { + WhileStatement: [ + "while (a) ", + "while (a && b) " + ], + DoWhileStatement: [ + "do while (a)", + "do while (a && b)" + ], + ForStatement: [ + "for (a; b; c) ", + "for (var i = 0; i < a.length; i++) ", + "for (; b; c) ", + "for (; b < foo; c++) ", + "for (a; ; c) ", + "for (a = 0; ; c++) ", + "for (a; b;) ", + "for (a = 0; b < foo; ) ", + "for (; ; c) ", + "for (; ; c++) ", + "for (; b;) ", + "for (; b < foo; ) ", + "for (a; ;) ", + "for (a = 0; ;) ", + "for (;;) " + ], + ForInStatement: [ + "for (a in b) ", + "for (a in f(b)) ", + "for (var a in b) ", + "for (let a in f(b)) " + ], + ForOfStatement: [ + "for (a of b) ", + "for (a of f(b)) ", + "for ({ a, b } of c) ", + "for (var a of f(b)) ", + "async function foo() { for await (const a of b) }" + ] +}; + +const validLoopBodies = [ + ";", + "{}", + "{ bar(); }", + "continue;", + "{ continue; }", + "{ if (foo) break; }", + "{ if (foo) { return; } bar(); }", + "{ if (foo) { bar(); } else { break; } }", + "{ if (foo) { continue; } return; }", + "{ switch (foo) { case 1: return; } }", + "{ switch (foo) { case 1: break; default: return; } }", + "{ switch (foo) { case 1: continue; default: return; } throw err; }", + "{ try { return bar(); } catch (e) {} }", + + // unreachable break + "{ continue; break; }", + + // functions in loops + "() => a;", + "{ () => a }", + "(() => a)();", + "{ (() => a)() }", + + // loops in loops + "while (a);", + "do ; while (a)", + "for (a; b; c);", + "for (; b;);", + "for (; ; c) if (foo) break;", + "for (;;) if (foo) break;", + "while (true) if (foo) break;", + "while (foo) if (bar) return;", + "for (a in b);", + "for (a of b);" +]; + +const invalidLoopBodies = [ + "break;", + "{ break; }", + "return;", + "{ return; }", + "throw err;", + "{ throw err; }", + "{ foo(); break; }", + "{ break; foo(); }", + "if (foo) break; else return;", + "{ if (foo) { return; } else { break; } bar(); }", + "{ if (foo) { return; } throw err; }", + "{ switch (foo) { default: throw err; } }", + "{ switch (foo) { case 1: throw err; default: return; } }", + "{ switch (foo) { case 1: something(); default: return; } }", + "{ try { return bar(); } catch (e) { break; } }", + + // unreachable continue + "{ break; continue; }", + + // functions in loops + "{ () => a; break; }", + "{ (() => a)(); break; }", + + // loops in loops + "{ while (a); break; }", + "{ do ; while (a) break; }", + "{ for (a; b; c); break; }", + "{ for (; b;); break; }", + "{ for (; ; c) if (foo) break; break; }", + "{ for(;;) if (foo) break; break; }", + "{ for (a in b); break; }", + "{ for (a of b); break; }", + + /** + * Additional cases where code path analysis detects unreachable code: after loops that don't have a test condition or have a + * constant truthy test condition, and at the same time don't have any exit statements in the body. These are special cases + * where this rule reports error not because the outer loop's body exits in all paths, but because it has an infinite loop + * inside, thus it (the outer loop) cannot have more than one iteration. + */ + "for (;;);", + "{ for (var i = 0; ; i< 10) { foo(); } }", + "while (true);" +]; + +/** + * Creates source code from the given loop template and loop body. + * @param {string} template Loop template. + * @param {string} body Loop body. + * @returns {string} Source code. + */ +function getSourceCode(template, body) { + const loop = template.replace("", body); + + return body.includes("return") && !template.includes("function") ? `function someFunc() { ${loop} }` : loop; +} + +/** + * Generates basic valid tests from `loopTemplates` and `validLoopBodies` + * @returns {IterableIterator} The list of source code strings. + */ +function *getBasicValidTests() { + for (const templates of Object.values(loopTemplates)) { + for (const template of templates) { + yield* validLoopBodies.map(body => getSourceCode(template, body)); + } + } +} + +/** + * Generates basic invalid tests from `loopTemplates` and `invalidLoopBodies` + * @returns {IterableIterator} The list of objects for the invalid[] array. + */ +function *getBasicInvalidTests() { + for (const [type, templates] of Object.entries(loopTemplates)) { + for (const template of templates) { + yield* invalidLoopBodies.map( + body => ({ + code: getSourceCode(template, body), + errors: [{ type, messageId: "invalid" }] + }) + ); + } + } +} + +ruleTester.run("no-unreachable-loop", rule, { + valid: [ + + ...getBasicValidTests(), + + // out of scope for the code path analysis and consequently out of scope for this rule + "while (false) { foo(); }", + "while (bar) { foo(); if (true) { break; } }", + "do foo(); while (false)", + "for (x = 1; x < 10; i++) { if (x > 0) { foo(); throw err; } }", + "for (x of []);", + "for (x of [1]);", + + // doesn't report unreachable loop statements, regardless of whether they would be valid or not in a reachable position + "function foo() { return; while (a); }", + "function foo() { return; while (a) break; }", + "while(true); while(true);", + "while(true); while(true) break;", + + // "ignore" + { + code: "while (a) break;", + options: [{ ignore: ["WhileStatement"] }] + }, + { + code: "do break; while (a)", + options: [{ ignore: ["DoWhileStatement"] }] + }, + { + code: "for (a; b; c) break;", + options: [{ ignore: ["ForStatement"] }] + }, + { + code: "for (a in b) break;", + options: [{ ignore: ["ForInStatement"] }] + }, + { + code: "for (a of b) break;", + options: [{ ignore: ["ForOfStatement"] }] + }, + { + code: "for (var key in obj) { hasEnumerableProperties = true; break; } for (const a of b) break;", + options: [{ ignore: ["ForInStatement", "ForOfStatement"] }] + } + ], + + invalid: [ + + ...getBasicInvalidTests(), + + // invalid loop nested in a valid loop (valid in valid, and valid in invalid are covered by basic tests) + { + code: "while (foo) { for (a of b) { if (baz) { break; } else { throw err; } } }", + errors: [ + { + messageId: "invalid", + type: "ForOfStatement" + } + ] + }, + { + code: "lbl: for (var i = 0; i < 10; i++) { while (foo) break lbl; } /* outer is valid because inner can have 0 iterations */", + errors: [ + { + messageId: "invalid", + type: "WhileStatement" + } + ] + }, + + // invalid loop nested in another invalid loop + { + code: "for (a in b) { while (foo) { if(baz) { break; } else { break; } } break; }", + errors: [ + { + messageId: "invalid", + type: "ForInStatement" + }, + { + messageId: "invalid", + type: "WhileStatement" + } + ] + }, + + // loop and nested loop both invalid because of the same exit statement + { + code: "function foo() { for (var i = 0; i < 10; i++) { do { return; } while(i) } }", + errors: [ + { + messageId: "invalid", + type: "ForStatement" + }, + { + messageId: "invalid", + type: "DoWhileStatement" + } + ] + }, + { + code: "lbl: while(foo) { do { break lbl; } while(baz) }", + errors: [ + { + messageId: "invalid", + type: "WhileStatement" + }, + { + messageId: "invalid", + type: "DoWhileStatement" + } + ] + }, + + // inner loop has continue, but to an outer loop + { + code: "lbl: for (a in b) { while(foo) { continue lbl; } }", + errors: [ + { + messageId: "invalid", + type: "WhileStatement" + } + ] + }, + + // edge cases - inner loop has only one exit path, but at the same time it exits the outer loop in the first iteration + { + code: "for (a of b) { for(;;) { if (foo) { throw err; } } }", + errors: [ + { + messageId: "invalid", + type: "ForOfStatement" + } + ] + }, + { + code: "function foo () { for (a in b) { while (true) { if (bar) { return; } } } }", + errors: [ + { + messageId: "invalid", + type: "ForInStatement" + } + ] + }, + + // edge cases where parts of the loops belong to the same code path segment, tests for false negatives + { + code: "do for (var i = 1; i < 10; i++) break; while(foo)", + errors: [ + { + messageId: "invalid", + type: "ForStatement" + } + ] + }, + { + code: "do { for (var i = 1; i < 10; i++) continue; break; } while(foo)", + errors: [ + { + messageId: "invalid", + type: "DoWhileStatement" + } + ] + }, + { + code: "for (;;) { for (var i = 1; i < 10; i ++) break; if (foo) break; continue; }", + errors: [ + { + messageId: "invalid", + type: "ForStatement", + column: 12 + } + ] + }, + + // "ignore" + { + code: "while (a) break; do break; while (b); for (;;) break; for (c in d) break; for (e of f) break;", + options: [{ ignore: [] }], + errors: [ + { + messageId: "invalid", + type: "WhileStatement" + }, + { + messageId: "invalid", + type: "DoWhileStatement" + }, + { + messageId: "invalid", + type: "ForStatement" + }, + { + messageId: "invalid", + type: "ForInStatement" + }, + { + messageId: "invalid", + type: "ForOfStatement" + } + ] + }, + { + code: "while (a) break;", + options: [{ ignore: ["DoWhileStatement"] }], + errors: [ + { + messageId: "invalid", + type: "WhileStatement" + } + ] + }, + { + code: "do break; while (a)", + options: [{ ignore: ["WhileStatement"] }], + errors: [ + { + messageId: "invalid", + type: "DoWhileStatement" + } + ] + }, + { + code: "for (a in b) break; for (c of d) break;", + options: [{ ignore: ["ForStatement"] }], + errors: [ + { + messageId: "invalid", + type: "ForInStatement" + }, + { + messageId: "invalid", + type: "ForOfStatement" + } + ] + }, + { + code: "for (a in b) break; for (;;) break; for (c of d) break;", + options: [{ ignore: ["ForInStatement", "ForOfStatement"] }], + errors: [ + { + messageId: "invalid", + type: "ForStatement" + } + ] + } + ] +}); diff --git a/tools/rule-types.json b/tools/rule-types.json index f33aa1b6678..683d0fef8d5 100644 --- a/tools/rule-types.json +++ b/tools/rule-types.json @@ -199,6 +199,7 @@ "no-unmodified-loop-condition": "problem", "no-unneeded-ternary": "suggestion", "no-unreachable": "problem", + "no-unreachable-loop": "problem", "no-unsafe-finally": "problem", "no-unsafe-negation": "problem", "no-unused-expressions": "suggestion",