Fix: no-else-return handles multiple else-if blocks (fixes #3015)

docs/rules/no-else-return.md

@@ -14,7 +14,7 @@ function foo() {
 
 ## Rule Details
 
-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 an `if` containing a `return`.
+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.
 
 The following patterns are considered warnings:
 
@@ -27,6 +27,16 @@ function foo() {
     }
 }
 
+function foo() {
+    if (x) {
+        return y;
+    } else if (z) {
+        return w;
+    } else {
+        return t;
+    }
+}
+
 function foo() {
     if (x) {
         return y;
@@ -62,6 +72,16 @@ function foo() {
     return z;
 }
 
+function foo() {
+    if (x) {
+        return y;
+    } else if (z) {
+        var t = "foo";
+    } else {
+        return w;
+    }
+}
+
 function foo() {
     if (x) {
         if (z) {

lib/rules/no-else-return.js

@@ -80,15 +80,30 @@ module.exports = function(context) {
     /**
      * Check the consequent/body node to make sure it is not
      * a ReturnStatement or an IfStatement that returns on both
-     * code paths.  If it is, display the context report.
+     * code paths.
      *
      * @param {Node} node The consequent or body node
      * @param {Node} alternate The alternate node
-     * @returns {void}
+     * @returns {boolean} `true` if it is a Return/If node that always returns.
+     */
+    function checkForReturnOrIf(node) {
+        return checkForReturn(node) || checkForIf(node);
+    }
+
+
+    /**
+     * Check whether a node returns in every codepath.
+     * @param {Node} node The node to be checked
+     * @returns {boolean} `true` if it returns on every codepath.
      */
-    function checkForReturnOrIf(node, alternate) {
-        if (checkForReturn(node) || checkForIf(node)) {
-            displayReport(alternate);
+    function alwaysReturns(node) {
+        // If we have a BlockStatement, check each consequent body node.
+        if (node.type === "BlockStatement") {
+            return node.body.some(checkForReturnOrIf);
+        // If not a block statement, make sure the consequent isn't a ReturnStatement
+        // or an IfStatement with returns on both paths
+        } else {
+            return checkForReturnOrIf(node);
         }
     }
 
@@ -99,22 +114,26 @@ module.exports = function(context) {
     return {
 
         "IfStatement": function(node) {
-            // Don't bother finding a ReturnStatement, if there's no `else`
-            // or if the alternate is also an if (indicating an else if).
-            if (hasElse(node)) {
-                var consequent = node.consequent,
-                    alternate = node.alternate;
-                // If we have a BlockStatement, check each consequent body node.
-                if (consequent.type === "BlockStatement") {
-                    var body = consequent.body;
-                    body.forEach(function(bodyNode) {
-                        checkForReturnOrIf(bodyNode, alternate);
-                    });
-                // If not a block statement, make sure the consequent isn't a ReturnStatement
-                // or an IfStatement with returns on both paths
-                } else {
-                    checkForReturnOrIf(consequent, alternate);
+            var parent = context.getAncestors().pop(),
+                consequents,
+                alternate;
+
+            // Only "top-level" if statements are checked, meaning the first `if`
+            // in a `if-else-if-...` chain.
+            if (parent.type === "IfStatement" && parent.alternate === node) {
+                return;
+            }
+
+            for (consequents = []; node.type === "IfStatement"; node = node.alternate) {
+                if (!node.alternate) {
+                    return;
                 }
+                consequents.push(node.consequent);
+                alternate = node.alternate;
+            }
+
+            if (consequents.every(alwaysReturns)) {
+                displayReport(alternate);
             }
         }
 

tests/lib/rules/no-else-return.js

@@ -24,6 +24,8 @@ eslintTester.addRuleTest("lib/rules/no-else-return", {
         "function foo() { if (true) { for (;;) { return x; } } else { return y; } }",
         "function foo() { var x = true; if (x) { return x; } else if (x === false) { return false; } }",
         "function foo() { if (true) notAReturn(); else return y; }",
+        "function foo() {if (x) { notAReturn(); } else if (y) { return true; } else { notAReturn(); } }",
+        "function foo() {if (x) { return true; } else if (y) { notAReturn() } else { notAReturn(); } }",
         "if (0) { if (0) {} else {} } else {}"
     ],
     invalid: [
@@ -40,6 +42,10 @@ eslintTester.addRuleTest("lib/rules/no-else-return", {
         { code: "function foo() { if (true) { if (false) { if (true) return x; else return y; } else { w = x; } } else { return z; } }", errors: [
             { message: "Unexpected 'else' after 'return'.", type: "ReturnStatement"},
             { message: "Unexpected 'else' after 'return'.", type: "BlockStatement"}
-        ] }
+        ] },
+        {
+            code: "function foo() {if (x) { return true; } else if (y) { return true; } else { notAReturn(); } }",
+            errors: [{message: "Unexpected 'else' after 'return'.", type: "BlockStatement"}]
+        }
     ]
 });