diff --git a/lib/rules/curly.js b/lib/rules/curly.js index ee2fe4dceb8..29f00c0ad0b 100644 --- a/lib/rules/curly.js +++ b/lib/rules/curly.js @@ -141,33 +141,14 @@ module.exports = { } /** - * Checks a given IfStatement node requires braces of the consequent chunk. - * This returns `true` when below: - * - * 1. The given node has the `alternate` node. - * 2. There is a `IfStatement` which doesn't have `alternate` node in the - * trailing statement chain of the `consequent` node. - * @param {ASTNode} node A IfStatement node to check. - * @returns {boolean} `true` if the node requires braces of the consequent chunk. + * Determines whether the given node has an `else` keyword token as the first token after. + * @param {ASTNode} node The node to check. + * @returns {boolean} `true` if the node is followed by an `else` keyword token. */ - function requiresBraceOfConsequent(node) { - if (node.alternate && node.consequent.type === "BlockStatement") { - if (node.consequent.body.length >= 2) { - return true; - } - - for ( - let currentNode = node.consequent.body[0]; - currentNode; - currentNode = astUtils.getTrailingStatement(currentNode) - ) { - if (currentNode.type === "IfStatement" && !currentNode.alternate) { - return true; - } - } - } + function isFollowedByElseKeyword(node) { + const nextToken = sourceCode.getTokenAfter(node); - return false; + return Boolean(nextToken) && isElseKeywordToken(nextToken); } /** @@ -224,6 +205,110 @@ module.exports = { return false; } + /** + * Determines whether the code represented by the given node contains an `if` statement + * that would become associated with an `else` keyword directly appended to that code. + * + * Examples where it returns `true`: + * + * if (a) + * foo(); + * + * if (a) { + * foo(); + * } + * + * if (a) + * foo(); + * else if (b) + * bar(); + * + * while (a) + * if (b) + * if(c) + * foo(); + * else + * bar(); + * + * Examples where it returns `false`: + * + * if (a) + * foo(); + * else + * bar(); + * + * while (a) { + * if (b) + * if(c) + * foo(); + * else + * bar(); + * } + * + * while (a) + * if (b) { + * if(c) + * foo(); + * } + * else + * bar(); + * @param {ASTNode} node Node representing the code to check. + * @returns {boolean} `true` if an `if` statement within the code would become associated with an `else` appended to that code. + */ + function hasUnsafeIf(node) { + switch (node.type) { + case "IfStatement": + if (!node.alternate) { + return true; + } + return hasUnsafeIf(node.alternate); + case "ForStatement": + case "ForInStatement": + case "ForOfStatement": + case "LabeledStatement": + case "WithStatement": + case "WhileStatement": + return hasUnsafeIf(node.body); + default: + return false; + } + } + + /** + * Determines whether the existing curly braces around the single statement are necessary to preserve the semantics of the code. + * The braces, which make the given block body, are necessary in either of the following situations: + * + * 1. The statement is a lexical declaration. + * 2. Without the braces, an `if` within the statement would become associated with an `else` after the closing brace: + * + * if (a) { + * if (b) + * foo(); + * } + * else + * bar(); + * + * if (a) + * while (b) + * while (c) { + * while (d) + * if (e) + * while(f) + * foo(); + * } + * else + * bar(); + * @param {ASTNode} node `BlockStatement` body with exactly one statement directly inside. The statement can have its own nested statements. + * @returns {boolean} `true` if the braces are necessary - removing them (replacing the given `BlockStatement` body with its single statement content) + * would change the semantics of the code or produce a syntax error. + */ + function areBracesNecessary(node) { + const statement = node.body[0]; + + return isLexicalDeclaration(statement) || + hasUnsafeIf(statement) && isFollowedByElseKeyword(node); + } + /** * Prepares to check the body of a node to see if it's a block statement. * @param {ASTNode} node The node to report if there's a problem. @@ -242,30 +327,29 @@ module.exports = { const hasBlock = (body.type === "BlockStatement"); let expected = null; - if (node.type === "IfStatement" && node.consequent === body && requiresBraceOfConsequent(node)) { + if (hasBlock && (body.body.length !== 1 || areBracesNecessary(body))) { expected = true; } else if (multiOnly) { - if (hasBlock && body.body.length === 1 && !isLexicalDeclaration(body.body[0])) { - expected = false; - } + expected = false; } else if (multiLine) { if (!isCollapsedOneLiner(body)) { expected = true; } + + // otherwise, the body is allowed to have braces or not to have braces + } else if (multiOrNest) { - if (hasBlock && body.body.length === 1 && isOneLiner(body.body[0])) { - const leadingComments = sourceCode.getCommentsBefore(body.body[0]); - const isLexDef = isLexicalDeclaration(body.body[0]); - - if (isLexDef) { - expected = true; - } else { - expected = leadingComments.length > 0; - } - } else if (!isOneLiner(body)) { - expected = true; + if (hasBlock) { + const statement = body.body[0]; + const leadingCommentsInBlock = sourceCode.getCommentsBefore(statement); + + expected = !isOneLiner(statement) || leadingCommentsInBlock.length > 0; + } else { + expected = !isOneLiner(body); } } else { + + // default "all" expected = true; } diff --git a/tests/lib/rules/curly.js b/tests/lib/rules/curly.js index 457e55f49fb..1d5ee8c6e4f 100644 --- a/tests/lib/rules/curly.js +++ b/tests/lib/rules/curly.js @@ -336,6 +336,89 @@ ruleTester.run("curly", rule, { // https://github.com/feross/standard/issues/664 code: "if (true) foo()\n;[1, 2, 3].bar()", options: ["multi-line"] + }, + + // https://github.com/eslint/eslint/issues/12928 (also in invalid[]) + { + code: "if (x) for (var i in x) { if (i > 0) console.log(i); } else console.log('whoops');", + options: ["multi"] + }, + { + code: "if (a) { if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) { if (b) foo(); } else bar();", + options: ["multi-or-nest"] + }, + { + code: "if (a) { if (b) foo(); } else { bar(); }", + options: ["multi", "consistent"] + }, + { + code: "if (a) { if (b) foo(); } else { bar(); }", + options: ["multi-or-nest", "consistent"] + }, + { + code: "if (a) { if (b) { foo(); bar(); } } else baz();", + options: ["multi"] + }, + { + code: "if (a) foo(); else if (b) { if (c) bar(); } else baz();", + options: ["multi"] + }, + { + code: "if (a) { if (b) foo(); else if (c) bar(); } else baz();", + options: ["multi"] + }, + { + code: "if (a) if (b) foo(); else { if (c) bar(); } else baz();", + options: ["multi"] + }, + { + code: "if (a) { lbl:if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) { lbl1:lbl2:if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) { for (;;) if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) { for (key in obj) if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) { for (elem of arr) if (b) foo(); } else bar();", + options: ["multi"], + parserOptions: { ecmaVersion: 2015 } + }, + { + code: "if (a) { with (obj) if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) { while (cond) if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) { while (cond) for (;;) for (key in obj) if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) while (cond) { for (;;) for (key in obj) if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) while (cond) for (;;) { for (key in obj) if (b) foo(); } else bar();", + options: ["multi"] + }, + { + code: "if (a) while (cond) for (;;) for (key in obj) { if (b) foo(); } else bar();", + options: ["multi"] } ], invalid: [ @@ -1091,6 +1174,114 @@ ruleTester.run("curly", rule, { output: "do \ndoSomething()\n;\n while (foo)", options: ["multi-or-nest"], errors: [{ messageId: "unexpectedCurlyAfter", data: { name: "do" }, type: "DoWhileStatement" }] + }, + + // https://github.com/eslint/eslint/issues/12928 (also in valid[]) + { + code: "if (a) { if (b) foo(); }", + output: "if (a) if (b) foo(); ", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }] + }, + { + code: "if (a) { if (b) foo(); else bar(); }", + output: "if (a) if (b) foo(); else bar(); ", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }] + }, + { + code: "if (a) { if (b) foo(); else bar(); } baz();", + output: "if (a) if (b) foo(); else bar(); baz();", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }] + }, + { + code: "if (a) { while (cond) if (b) foo(); }", + output: "if (a) while (cond) if (b) foo(); ", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }] + }, + { + code: "if (a) while (cond) { if (b) foo(); }", + output: "if (a) while (cond) if (b) foo(); ", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "while" }, type: "WhileStatement" }] + }, + { + code: "if (a) while (cond) { if (b) foo(); else bar(); }", + output: "if (a) while (cond) if (b) foo(); else bar(); ", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "while" }, type: "WhileStatement" }] + }, + { + code: "if (a) { while (cond) { if (b) foo(); } bar(); baz() } else quux();", + output: "if (a) { while (cond) if (b) foo(); bar(); baz() } else quux();", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "while" }, type: "WhileStatement" }] + }, + { + code: "if (a) { if (b) foo(); } bar();", + output: "if (a) if (b) foo(); bar();", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }] + }, + { + code: "if(a) { if (b) foo(); } if (c) bar(); else baz();", + output: "if(a) if (b) foo(); if (c) bar(); else baz();", + options: ["multi-or-nest"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }] + }, + { + code: "if (a) { do if (b) foo(); while (cond); } else bar();", + output: "if (a) do if (b) foo(); while (cond); else bar();", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }] + }, + { + code: "if (a) do { if (b) foo(); } while (cond); else bar();", + output: "if (a) do if (b) foo(); while (cond); else bar();", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfter", data: { name: "do" }, type: "DoWhileStatement" }] + }, + { + code: "if (a) { if (b) foo(); else bar(); } else baz();", + output: "if (a) if (b) foo(); else bar(); else baz();", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }] + }, + { + code: "if (a) while (cond) { bar(); } else baz();", + output: "if (a) while (cond) bar(); else baz();", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "while" }, type: "WhileStatement" }] + }, + { + code: "if (a) { for (;;); } else bar();", + output: "if (a) for (;;); else bar();", + options: ["multi"], + errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }] + }, + { + code: "if (a) { while (cond) if (b) foo() } else bar();", + output: "if (a) { while (cond) if (b) foo() } else {bar();}", + options: ["multi", "consistent"], + errors: [{ messageId: "missingCurlyAfter", data: { name: "else" }, type: "IfStatement" }] + }, + { + + /** + * Reports 2 errors, but one pair of braces is necessary if the other pair gets removed. + * Auto-fix will remove only outer braces in the first iteration. + * After that, the inner braces will become valid and won't be removed in the second iteration. + * If user manually removes inner braces first, the outer braces will become valid. + */ + code: "if (a) { while (cond) { if (b) foo(); } } else bar();", + output: "if (a) while (cond) { if (b) foo(); } else bar();", + options: ["multi"], + errors: [ + { messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }, + { messageId: "unexpectedCurlyAfterCondition", data: { name: "while" }, type: "WhileStatement" } + ] } ] });