diff --git a/docs/rules/no-else-return.md b/docs/rules/no-else-return.md index 3127e0f511f..eb6f8fab349 100644 --- a/docs/rules/no-else-return.md +++ b/docs/rules/no-else-return.md @@ -16,6 +16,23 @@ function foo() { This rule is aimed at highlighting an unnecessary block of code following an `if` containing a return statement. As such, it will warn when it encounters an `else` following a chain of `if`s, all of them containing a `return` statement. +## Options + +This rule has an object option: + +```json +{ + "no-else-return": ["error", { "allowElseIf": true }], + // or + "no-else-return": ["error", { "allowElseIf": false }] +} +``` + +* `allowElseIf: true` (default) allows `else if` blocks after a return +* `allowElseIf: false` disallows `else if` blocks after a return + +### `allowElseIf: true` + Examples of **incorrect** code for this rule: ```js @@ -49,6 +66,16 @@ function foo() { return t; } +function foo() { + if (error) { + return 'It failed'; + } else { + if (loading) { + return "It's still loading"; + } + } +} + // Two warnings for nested occurrences function foo() { if (x) { @@ -95,4 +122,44 @@ function foo() { return z; } } + +function foo() { + if (error) { + return 'It failed'; + } else if (loading) { + return "It's still loading"; + } +} +``` + +### `allowElseIf: false` + +Examples of **incorrect** code for this rule: + +```js +/*eslint no-else-return: ["error", {allowElseIf: false}]*/ + +function foo() { + if (error) { + return 'It failed'; + } else if (loading) { + return "It's still loading"; + } +} +``` + +Examples of **correct** code for this rule: + +```js +/*eslint no-else-return: ["error", {allowElseIf: false}]*/ + +function foo() { + if (error) { + return 'It failed'; + } + + if (loading) { + return "It's still loading"; + } +} ``` diff --git a/lib/ast-utils.js b/lib/ast-utils.js index 47cc71990f9..fc0471e9a4a 100644 --- a/lib/ast-utils.js +++ b/lib/ast-utils.js @@ -1041,7 +1041,8 @@ module.exports = { } else if (parent.type === "Property" || parent.type === "MethodDefinition") { if (parent.kind === "constructor") { return "constructor"; - } else if (parent.kind === "get") { + } + if (parent.kind === "get") { tokens.push("getter"); } else if (parent.kind === "set") { tokens.push("setter"); diff --git a/lib/cli.js b/lib/cli.js index 962a4be0eff..f0444809705 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -152,7 +152,8 @@ const cli = { if (files.length) { log.error("The --print-config option must be used with exactly one file name."); return 1; - } else if (text) { + } + if (text) { log.error("The --print-config option is not available for piped-in code."); return 1; } diff --git a/lib/formatters/html.js b/lib/formatters/html.js index e61fdea6a93..d450f9dee24 100644 --- a/lib/formatters/html.js +++ b/lib/formatters/html.js @@ -51,7 +51,8 @@ function renderSummary(totalErrors, totalWarnings) { function renderColor(totalErrors, totalWarnings) { if (totalErrors !== 0) { return 2; - } else if (totalWarnings !== 0) { + } + if (totalWarnings !== 0) { return 1; } return 0; diff --git a/lib/rules/callback-return.js b/lib/rules/callback-return.js index 08600c01e58..0fa7f278a18 100644 --- a/lib/rules/callback-return.js +++ b/lib/rules/callback-return.js @@ -60,7 +60,8 @@ module.exports = { if (node.type === "MemberExpression") { if (node.object.type === "Identifier") { return true; - } else if (node.object.type === "MemberExpression") { + } + if (node.object.type === "MemberExpression") { return containsOnlyIdentifiers(node.object); } } diff --git a/lib/rules/capitalized-comments.js b/lib/rules/capitalized-comments.js index bb5e5825a36..1a276080673 100644 --- a/lib/rules/capitalized-comments.js +++ b/lib/rules/capitalized-comments.js @@ -247,7 +247,8 @@ module.exports = { if (capitalize === "always" && isLowercase) { return false; - } else if (capitalize === "never" && isUppercase) { + } + if (capitalize === "never" && isUppercase) { return false; } diff --git a/lib/rules/new-cap.js b/lib/rules/new-cap.js index 2f02c0924bb..f01e8f90ac3 100644 --- a/lib/rules/new-cap.js +++ b/lib/rules/new-cap.js @@ -178,7 +178,8 @@ module.exports = { // char has no uppercase variant, so it's non-alphabetic return "non-alpha"; - } else if (firstChar === firstCharLower) { + } + if (firstChar === firstCharLower) { return "lower"; } return "upper"; diff --git a/lib/rules/newline-before-return.js b/lib/rules/newline-before-return.js index 1b9b0c79392..ef6c4a9264e 100644 --- a/lib/rules/newline-before-return.js +++ b/lib/rules/newline-before-return.js @@ -59,9 +59,11 @@ module.exports = { if (parentType === "IfStatement") { return isPrecededByTokens(node, ["else", ")"]); - } else if (parentType === "DoWhileStatement") { + } + if (parentType === "DoWhileStatement") { return isPrecededByTokens(node, ["do"]); - } else if (parentType === "SwitchCase") { + } + if (parentType === "SwitchCase") { return isPrecededByTokens(node, [":"]); } return isPrecededByTokens(node, [")"]); diff --git a/lib/rules/no-alert.js b/lib/rules/no-alert.js index ecb52cedd6e..bc1087253bf 100644 --- a/lib/rules/no-alert.js +++ b/lib/rules/no-alert.js @@ -71,7 +71,8 @@ function isShadowed(scope, node) { function isGlobalThisReferenceOrGlobalWindow(scope, node) { if (scope.type === "global" && node.type === "ThisExpression") { return true; - } else if (node.name === "window") { + } + if (node.name === "window") { return !isShadowed(scope, node); } diff --git a/lib/rules/no-control-regex.js b/lib/rules/no-control-regex.js index 1ebf9800009..14981f4ab13 100644 --- a/lib/rules/no-control-regex.js +++ b/lib/rules/no-control-regex.js @@ -31,7 +31,8 @@ module.exports = { function getRegExp(node) { if (node.value instanceof RegExp) { return node.value; - } else if (typeof node.value === "string") { + } + if (typeof node.value === "string") { const parent = context.getAncestors().pop(); diff --git a/lib/rules/no-else-return.js b/lib/rules/no-else-return.js index 36bfc26b3d4..6eb6717495b 100644 --- a/lib/rules/no-else-return.js +++ b/lib/rules/no-else-return.js @@ -24,8 +24,15 @@ module.exports = { recommended: false }, - schema: [], - + schema: [{ + type: "object", + properties: { + allowElseIf: { + type: "boolean" + } + }, + additionalProperties: false + }], fixable: "code" }, @@ -134,13 +141,13 @@ module.exports = { /** * Check to see if the node is valid for evaluation, - * meaning it has an else and not an else-if + * meaning it has an else. * * @param {Node} node The node being evaluated * @returns {boolean} True if the node is valid */ function hasElse(node) { - return node.alternate && node.consequent && node.alternate.type !== "IfStatement"; + return node.alternate && node.consequent; } /** @@ -189,14 +196,15 @@ module.exports = { return checkForReturnOrIf(node); } + /** - * Check the if statement + * Check the if statement, but don't catch else-if blocks. * @returns {void} * @param {Node} node The node for the if statement to check * @private */ - function IfStatement(node) { - const parent = context.getAncestors().pop(); + function checkIfWithoutElse(node) { + const parent = node.parent; let consequents, alternate; @@ -221,13 +229,40 @@ module.exports = { } } + /** + * Check the if statement + * @returns {void} + * @param {Node} node The node for the if statement to check + * @private + */ + function checkIfWithElse(node) { + const parent = node.parent; + + + /* + * Fixing this would require splitting one statement into two, so no error should + * be reported if this node is in a position where only one statement is allowed. + */ + if (!astUtils.STATEMENT_LIST_PARENTS.has(parent.type)) { + return; + } + + const alternate = node.alternate; + + if (alternate && alwaysReturns(node.consequent)) { + displayReport(alternate); + } + } + + const allowElseIf = !(context.options[0] && context.options[0].allowElseIf === false); + //-------------------------------------------------------------------------- // Public API //-------------------------------------------------------------------------- return { - "IfStatement:exit": IfStatement + "IfStatement:exit": allowElseIf ? checkIfWithoutElse : checkIfWithElse }; diff --git a/lib/rules/no-extra-parens.js b/lib/rules/no-extra-parens.js index 9c8fe70f030..020d2aeb10f 100644 --- a/lib/rules/no-extra-parens.js +++ b/lib/rules/no-extra-parens.js @@ -195,10 +195,12 @@ module.exports = { function containsAssignment(node) { if (node.type === "AssignmentExpression") { return true; - } else if (node.type === "ConditionalExpression" && + } + if (node.type === "ConditionalExpression" && (node.consequent.type === "AssignmentExpression" || node.alternate.type === "AssignmentExpression")) { return true; - } else if ((node.left && node.left.type === "AssignmentExpression") || + } + if ((node.left && node.left.type === "AssignmentExpression") || (node.right && node.right.type === "AssignmentExpression")) { return true; } @@ -219,7 +221,8 @@ module.exports = { if (node.type === "ReturnStatement") { return node.argument && containsAssignment(node.argument); - } else if (node.type === "ArrowFunctionExpression" && node.body.type !== "BlockStatement") { + } + if (node.type === "ArrowFunctionExpression" && node.body.type !== "BlockStatement") { return containsAssignment(node.body); } return containsAssignment(node); diff --git a/lib/rules/no-mixed-requires.js b/lib/rules/no-mixed-requires.js index 55ad1e73e07..171052a52a3 100644 --- a/lib/rules/no-mixed-requires.js +++ b/lib/rules/no-mixed-requires.js @@ -104,14 +104,16 @@ module.exports = { // "var x = require('util');" return DECL_REQUIRE; - } else if (allowCall && + } + if (allowCall && initExpression.type === "CallExpression" && initExpression.callee.type === "CallExpression" ) { // "var x = require('diagnose')('sub-module');" return getDeclarationType(initExpression.callee); - } else if (initExpression.type === "MemberExpression") { + } + if (initExpression.type === "MemberExpression") { // "var x = require('glob').Glob;" return getDeclarationType(initExpression.object); @@ -131,7 +133,8 @@ module.exports = { // "var x = require('glob').Glob;" return inferModuleType(initExpression.object); - } else if (initExpression.arguments.length === 0) { + } + if (initExpression.arguments.length === 0) { // "var x = require();" return REQ_COMPUTED; @@ -149,7 +152,8 @@ module.exports = { // "var fs = require('fs');" return REQ_CORE; - } else if (/^\.{0,2}\//.test(arg.value)) { + } + if (/^\.{0,2}\//.test(arg.value)) { // "var utils = require('./utils');" return REQ_FILE; diff --git a/lib/rules/sort-imports.js b/lib/rules/sort-imports.js index 74db02ad3df..9f82a025a88 100644 --- a/lib/rules/sort-imports.js +++ b/lib/rules/sort-imports.js @@ -67,9 +67,11 @@ module.exports = { function usedMemberSyntax(node) { if (node.specifiers.length === 0) { return "none"; - } else if (node.specifiers[0].type === "ImportNamespaceSpecifier") { + } + if (node.specifiers[0].type === "ImportNamespaceSpecifier") { return "all"; - } else if (node.specifiers.length === 1) { + } + if (node.specifiers.length === 1) { return "single"; } return "multiple"; diff --git a/lib/util/source-code.js b/lib/util/source-code.js index af01ff3fafa..828eb85cd03 100644 --- a/lib/util/source-code.js +++ b/lib/util/source-code.js @@ -332,7 +332,8 @@ class SourceCode extends TokenStore { } return parent && parent.type !== "FunctionDeclaration" && parent.type !== "Program" ? findJSDocComment(parentLeadingComments, parent.loc.start.line) : null; - } else if (leadingComments.length) { + } + if (leadingComments.length) { return findJSDocComment(leadingComments, node.loc.start.line); } diff --git a/packages/eslint-config-eslint/default.yml b/packages/eslint-config-eslint/default.yml index 2af0a614f2a..95ec500b851 100644 --- a/packages/eslint-config-eslint/default.yml +++ b/packages/eslint-config-eslint/default.yml @@ -51,7 +51,7 @@ rules: no-confusing-arrow: "error" no-console: "error" no-delete-var: "error" - no-else-return: "error" + no-else-return: ["error", { allowElseIf: false }] no-eval: "error" no-extend-native: "error" no-extra-bind: "error" diff --git a/tests/lib/linter.js b/tests/lib/linter.js index b9d1c5d3ace..3f3c7533388 100644 --- a/tests/lib/linter.js +++ b/tests/lib/linter.js @@ -20,7 +20,8 @@ function compatRequire(name, windowName) { if (typeof window === "object") { return window[windowName || name]; - } else if (typeof require === "function") { + } + if (typeof require === "function") { return require(name); } throw new Error(`Cannot find object '${name}'.`); diff --git a/tests/lib/rules/no-else-return.js b/tests/lib/rules/no-else-return.js index d081de88fbb..f88407151f2 100644 --- a/tests/lib/rules/no-else-return.js +++ b/tests/lib/rules/no-else-return.js @@ -42,7 +42,19 @@ ruleTester.run("no-else-return", rule, { if (bar) return; else baz; } - ` + `, + { + code: "function foo19() { if (true) { return x; } else if (false) { return y; } }", + options: [{ allowElseIf: true }] + }, + { + code: "function foo20() {if (x) { return true; } else if (y) { notAReturn() } else { notAReturn(); } }", + options: [{ allowElseIf: true }] + }, + { + code: "function foo21() { var x = true; if (x) { return x; } else if (x === false) { return false; } }", + options: [{ allowElseIf: true }] + } ], invalid: [ { @@ -96,6 +108,18 @@ ruleTester.run("no-else-return", rule, { output: "function foo9() {if (x) { return true; } else if (y) { return true; } notAReturn(); }", errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }] }, + { + code: "function foo9a() {if (x) { return true; } else if (y) { return true; } else { notAReturn(); } }", + output: "function foo9a() {if (x) { return true; } if (y) { return true; } else { notAReturn(); } }", + options: [{ allowElseIf: false }], + errors: [{ message: "Unnecessary 'else' after 'return'.", type: "IfStatement" }] + }, + { + code: "function foo9b() {if (x) { return true; } if (y) { return true; } else { notAReturn(); } }", + output: "function foo9b() {if (x) { return true; } if (y) { return true; } notAReturn(); }", + options: [{ allowElseIf: false }], + errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }] + }, { code: "function foo10() { if (foo) return bar; else (foo).bar(); }", output: "function foo10() { if (foo) return bar; (foo).bar(); }", @@ -140,6 +164,24 @@ ruleTester.run("no-else-return", rule, { code: "function foo18() { if (foo) return function() {} \nelse [1, 2, 3].map(bar) }", output: null, errors: [{ message: "Unnecessary 'else' after 'return'.", type: "ExpressionStatement" }] + }, + { + code: "function foo19() { if (true) { return x; } else if (false) { return y; } }", + output: "function foo19() { if (true) { return x; } if (false) { return y; } }", + options: [{ allowElseIf: false }], + errors: [{ message: "Unnecessary 'else' after 'return'.", type: "IfStatement" }] + }, + { + code: "function foo20() {if (x) { return true; } else if (y) { notAReturn() } else { notAReturn(); } }", + output: "function foo20() {if (x) { return true; } if (y) { notAReturn() } else { notAReturn(); } }", + options: [{ allowElseIf: false }], + errors: [{ message: "Unnecessary 'else' after 'return'.", type: "IfStatement" }] + }, + { + code: "function foo21() { var x = true; if (x) { return x; } else if (x === false) { return false; } }", + output: "function foo21() { var x = true; if (x) { return x; } if (x === false) { return false; } }", + options: [{ allowElseIf: false }], + errors: [{ message: "Unnecessary 'else' after 'return'.", type: "IfStatement" }] } ] });