Fix: curly with multi had false positive (fixes #3856)

eslint · Oct 2, 2015 · 9875101 · 9875101
1 parent 45863c1
commit 9875101
Show file tree

Hide file tree

Showing 4 changed files with 111 additions and 8 deletions.
diff --git a/lib/ast-utils.js b/lib/ast-utils.js
@@ -7,6 +7,12 @@
 
 "use strict";
 
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+var esutils = require("esutils");
+
 //------------------------------------------------------------------------------
 // Helpers
 //------------------------------------------------------------------------------
@@ -110,5 +116,18 @@ module.exports = {
                 comment.indexOf("eslint-") === 0
             )
         );
-    }
+    },
+
+    /**
+     * Gets the trailing statement of a given node.
+     *
+     *     if (code)
+     *         consequent;
+     *
+     * When taking this `IfStatement`, returns `consequent;` statement.
+     *
+     * @param {ASTNode} A node to get.
+     * @returns {ASTNode|null} The trailing statement's node.
+     */
+    getTrailingStatement: esutils.ast.trailingStatement
 };
diff --git a/lib/rules/curly.js b/lib/rules/curly.js
@@ -5,6 +5,12 @@
  */
 "use strict";
 
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+var astUtils = require("../ast-utils");
+
 //------------------------------------------------------------------------------
 // Rule Definition
 //------------------------------------------------------------------------------
@@ -14,6 +20,7 @@ module.exports = function(context) {
     var multiOnly = (context.options[0] === "multi");
     var multiLine = (context.options[0] === "multi-line");
     var multiOrNest = (context.options[0] === "multi-or-nest");
+    var always = !(multiOnly || multiLine || multiOrNest);
 
     //--------------------------------------------------------------------------
     // Helpers
@@ -44,6 +51,35 @@ module.exports = function(context) {
         return first.loc.start.line === last.loc.end.line;
     }
 
+    /**
+     * 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.
+     */
+    function requiresBraceOfConsequent(node) {
+        if (node.alternate && node.consequent.type === "BlockStatement") {
+            if (node.consequent.body.length >= 2) {
+                return true;
+            }
+
+            node = node.consequent.body[0];
+            while (node) {
+                if (node.type === "IfStatement" && !node.alternate) {
+                    return true;
+                }
+                node = astUtils.getTrailingStatement(node);
+            }
+        }
+
+        return false;
+    }
+
     /**
      * Reports "Expected { after ..." error
      * @param {ASTNode} node The node to report.
@@ -115,15 +151,13 @@ module.exports = function(context) {
     //--------------------------------------------------------------------------
 
     return {
-
         "IfStatement": function(node) {
-
-            checkBody(node, node.consequent, "if", "condition");
-
+            if (always || !requiresBraceOfConsequent(node)) {
+                checkBody(node, node.consequent, "if", "condition");
+            }
             if (node.alternate && node.alternate.type !== "IfStatement") {
                 checkBody(node, node.alternate, "else");
             }
-
         },
 
         "WhileStatement": function(node) {

diff --git a/package.json b/package.json
@@ -44,11 +44,12 @@
     "espree": "^2.2.4",
     "estraverse": "^4.1.0",
     "estraverse-fb": "^1.3.1",
+    "esutils": "^2.0.2",
+    "file-entry-cache": "^1.1.1",
     "glob": "^5.0.14",
     "globals": "^8.10.0",
     "handlebars": "^4.0.0",
     "inquirer": "^0.9.0",
-    "file-entry-cache": "^1.1.1",
     "is-my-json-valid": "^2.10.0",
     "is-resolvable": "^1.0.0",
     "js-yaml": "^3.2.5",

diff --git a/tests/lib/rules/curly.js b/tests/lib/rules/curly.js
@@ -32,6 +32,10 @@ ruleTester.run("curly", rule, {
             code: "if (foo) bar()",
             options: ["multi"]
         },
+        {
+            code: "if (a) { b; c; }",
+            options: ["multi"]
+        },
         {
             code: "if (foo) bar()",
             options: ["multi-line"]
@@ -91,8 +95,33 @@ ruleTester.run("curly", rule, {
         {
             code: "if (foo) { \n if(bar) \n doSomething(); \n } else \n doSomethingElse();",
             options: ["multi-or-nest"]
-        }
+        },
 
+        // https://github.com/eslint/eslint/issues/3856
+        {
+            code: "if (true) { if (false) console.log(1) } else console.log(2)",
+            options: ["multi"]
+        },
+        {
+            code: "if (a) { if (b) console.log(1); else if (c) console.log(2) } else console.log(3)",
+            options: ["multi"]
+        },
+        {
+            code: "if (true) { while(false) if (true); } else;",
+            options: ["multi"]
+        },
+        {
+            code: "if (true) { label: if (false); } else;",
+            options: ["multi"]
+        },
+        {
+            code: "if (true) { with(0) if (false); } else;",
+            options: ["multi"]
+        },
+        {
+            code: "if (true) { while(a) if(b) while(c) if (d); else; } else;",
+            options: ["multi"]
+        }
     ],
     invalid: [
         {
@@ -180,6 +209,26 @@ ruleTester.run("curly", rule, {
                 }
             ]
         },
+        {
+            code: "if (true) { if (false) console.log(1) }",
+            options: ["multi"],
+            errors: [
+                {
+                    message: "Unnecessary { after 'if' condition.",
+                    type: "IfStatement"
+                }
+            ]
+        },
+        {
+            code: "if (a) { if (b) console.log(1); else console.log(2) } else console.log(3)",
+            options: ["multi"],
+            errors: [
+                {
+                    message: "Unnecessary { after 'if' condition.",
+                    type: "IfStatement"
+                }
+            ]
+        },
         {
             code: "if (foo) \n baz()",
             options: ["multi-line"],