no-duplicated-branches: consider exceptions (#109)

Author: andrea-guarino-sonarsource

ruling/snapshots/no-duplicated-branches

@@ -1,17 +1,25 @@
 src/angular.js/src/ng/parse.js: 760,1327
+src/angular.js/src/ngAnimate/animateQueue.js: 194
 src/brackets/src/document/TextRange.js: 133
 src/brackets/src/extensions/default/JavaScriptQuickEdit/unittests.js: 151
 src/brackets/src/extensions/default/MDNDocs/InlineDocsViewer.js: 103
 src/brackets/src/language/CSSUtils.js: 1236
+src/brackets/src/utils/DragAndDrop.js: 54
+src/Chart.js/src/scales/scale.linear.js: 116,122
+src/Chart.js/src/scales/scale.logarithmic.js: 155,161
 src/freeCodeCamp/server/utils/user-stats.js: 30
 src/Ghost/core/server/models/post.js: 198
+src/jquery/external/sinon/sinon.js: 506
 src/react-native/Libraries/Renderer/ReactFabric-dev.js: 10298,10386
 src/react-native/Libraries/Renderer/ReactNativeRenderer-dev.js: 10668,10756
 src/react-native/Libraries/Renderer/ReactNativeRenderer-prod.js: 4202,4310
+src/react-native/local-cli/util/Config.js: 36
 src/react/packages/react-reconciler/src/ReactFiberCommitWork.js: 669,758
+src/reveal.js/js/reveal.js: 1133
 src/spectrum/src/reducers/dashboardFeed.js: 46
 src/spectrum/src/views/notifications/components/sortAndGroupNotificationMessages.js: 72
 src/three.js/editor/js/History.js: 61
+src/three.js/src/renderers/WebGLRenderer.js: 1636,1640,1646
 src/vue/packages/vue-server-renderer/basic.js: 4378
 src/vue/packages/vue-server-renderer/build.js: 4121
 src/vue/packages/vue-template-compiler/browser.js: 3493

src/rules/no-duplicated-branches.ts

@@ -23,7 +23,7 @@ import { Rule } from "eslint";
 import * as estree from "estree";
 import { getParent, isIfStatement, isBlockStatement } from "../utils/nodes";
 import { areEquivalent } from "../utils/equivalence";
-import { collectIfBranches, takeWithoutBreak } from "../utils/conditions";
+import { collectIfBranches, takeWithoutBreak, collectSwitchBranches } from "../utils/conditions";
 
 const MESSAGE = "This {{type}}'s code block is the same as the block for the {{type}} on line {{line}}.";
 
@@ -41,7 +41,12 @@ const rule: Rule.RuleModule = {
     function visitIfStatement(ifStmt: estree.IfStatement) {
       const parent = getParent(context);
       if (!isIfStatement(parent)) {
-        const { branches } = collectIfBranches(ifStmt);
+        const { branches, endsWithElse } = collectIfBranches(ifStmt);
+
+        if (allEquivalentWithoutDefault(branches, endsWithElse)) {
+          branches.slice(1).forEach((branch, i) => reportIssue(branch, branches[i], "branch"));
+          return;
+        }
 
         for (let i = 1; i < branches.length; i++) {
           if (hasRequiredSize([branches[i]])) {
@@ -55,23 +60,27 @@ const rule: Rule.RuleModule = {
       }
     }
 
-    function visitSwitchStatement({ cases }: estree.SwitchStatement) {
-      for (let i = 1; i < cases.length; i++) {
-        const firstClauseWithoutBreak = takeWithoutBreak(expandSingleBlockStatement(cases[i].consequent));
+    function visitSwitchStatement(switchStmt: estree.SwitchStatement) {
+      const { cases } = switchStmt;
+      const { endsWithDefault } = collectSwitchBranches(switchStmt);
+      const nonEmptyCases = cases
+        .map(c => takeWithoutBreak(expandSingleBlockStatement(c.consequent)))
+        .filter(c => c.length > 0);
+
+      if (allEquivalentWithoutDefault(nonEmptyCases, endsWithDefault)) {
+        cases.slice(1).forEach((caseStmt, i) => reportIssue(caseStmt, cases[i], "case"));
+        return;
+      }
+
+      for (let i = 1; i < nonEmptyCases.length; i++) {
+        const firstClauseWithoutBreak = nonEmptyCases[i];
 
         if (hasRequiredSize(firstClauseWithoutBreak)) {
           for (let j = 0; j < i; j++) {
-            const secondClauseWithoutBreak = takeWithoutBreak(expandSingleBlockStatement(cases[j].consequent));
+            const secondClauseWithoutBreak = nonEmptyCases[j];
 
             if (areEquivalent(firstClauseWithoutBreak, secondClauseWithoutBreak, context.getSourceCode())) {
-              context.report({
-                message: MESSAGE,
-                data: {
-                  type: "case",
-                  line: String(cases[j].loc!.start.line),
-                },
-                node: cases[i],
-              });
+              reportIssue(cases[i], cases[j], "case");
               break;
             }
           }
@@ -93,14 +102,7 @@ const rule: Rule.RuleModule = {
     function compareIfBranches(a: estree.Statement, b: estree.Statement) {
       const equivalent = areEquivalent(a, b, context.getSourceCode());
       if (equivalent && b.loc) {
-        context.report({
-          message: MESSAGE,
-          data: {
-            type: "branch",
-            line: String(b.loc.start.line),
-          },
-          node: a,
-        });
+        reportIssue(a, b, "branch");
       }
       return equivalent;
     }
@@ -114,6 +116,25 @@ const rule: Rule.RuleModule = {
       }
       return nodes;
     }
+
+    function allEquivalentWithoutDefault(branches: Array<estree.Node | estree.Node[]>, endsWithDefault: boolean) {
+      return (
+        !endsWithDefault &&
+        branches.length > 1 &&
+        branches.slice(1).every((branch, index) => areEquivalent(branch, branches[index], context.getSourceCode()))
+      );
+    }
+
+    function reportIssue(node: estree.Node, equivalentNode: estree.Node, type: string) {
+      context.report({
+        message: MESSAGE,
+        data: {
+          type,
+          line: String(equivalentNode.loc!.start.line),
+        },
+        node,
+      });
+    }
   },
 };
 

tests/rules/no-duplicated-branches.test.ts

@@ -82,6 +82,16 @@ ruleTester.run("no-duplicated-branches if", rule, {
         first();
       }`,
     },
+    {
+      code: `
+      if(a == 1) {
+        doSomething();  //no issue, usually this is done on purpose to increase the readability
+      } else if (a == 2) {
+        doSomethingElse();
+      } else {
+        doSomething();
+      }`,
+    },
   ],
   invalid: [
     {
@@ -128,6 +138,29 @@ ruleTester.run("no-duplicated-branches if", rule, {
       }`,
       errors: [{ line: 8 }],
     },
+    {
+      code: `
+      if(a == 1) {
+        doSomething();
+      } else if (a == 2) {
+        doSomething();
+      }`,
+      errors: [{ line: 4, message: "This branch's code block is the same as the block for the branch on line 2." }],
+    },
+    {
+      code: `
+      if(a == 1) {
+        doSomething();
+      } else if (a == 2) {
+        doSomething();
+      } else if (a == 3) {
+        doSomething();
+      }`,
+      errors: [
+        { line: 4, message: "This branch's code block is the same as the block for the branch on line 2." },
+        { line: 6, message: "This branch's code block is the same as the block for the branch on line 4." },
+      ],
+    },
   ],
 });
 
@@ -274,5 +307,17 @@ ruleTester.run("no-duplicated-branches switch", rule, {
       }`,
       errors: [{ line: 7 }, { line: 11 }, { line: 15 }],
     },
+    {
+      code: `
+      switch(a) {
+        case 1:
+          doSomething();
+          break;
+        case 2:
+          doSomething();
+          break;
+      }`,
+      errors: [{ line: 6, message: "This case's code block is the same as the block for the case on line 3." }],
+    },
   ],
 });