Modify S1862 (no-identical-conditions): Detect duplicated condition overlaps

yassin-kammoun-sonarsource · yassin-kammoun-sonarsource · commit 4dfd992eecca · 2023-03-16T15:38:53.000+01:00
diff --git a/ruling/snapshots/no-identical-conditions b/ruling/snapshots/no-identical-conditions
@@ -1 +1,2 @@
+src/react-native/Libraries/vendor/core/Map.js: 582
 src/spectrum/api/queries/user/contextPermissions.js: 63
diff --git a/src/rules/no-identical-conditions.ts b/src/rules/no-identical-conditions.ts
@@ -20,18 +20,17 @@
 // https://sonarsource.github.io/rspec/#/rspec/S1862
 
 import type { TSESTree, TSESLint } from '@typescript-eslint/experimental-utils';
-import { isIfStatement } from '../utils/nodes';
 import { areEquivalent } from '../utils/equivalence';
 import { report, issueLocation } from '../utils/locations';
 import docsUrl from '../utils/docs-url';
 
-const duplicatedBranchMessage = 'This branch duplicates the one on line {{line}}';
+const duplicatedConditionMessage = 'This condition is covered by the one on line {{line}}';
 const duplicatedCaseMessage = 'This case duplicates the one on line {{line}}';
 
 const rule: TSESLint.RuleModule<string, string[]> = {
   meta: {
     messages: {
-      duplicatedBranch: duplicatedBranchMessage,
+      duplicatedCondition: duplicatedConditionMessage,
       duplicatedCase: duplicatedCaseMessage,
       sonarRuntime: '{{sonarRuntimeData}}',
     },
@@ -53,30 +52,38 @@ const rule: TSESLint.RuleModule<string, string[]> = {
     const sourceCode = context.getSourceCode();
     return {
       IfStatement(node: TSESTree.Node) {
-        const ifStmt = node as TSESTree.IfStatement;
-        const condition = ifStmt.test;
-        let statement = ifStmt.alternate;
-        while (statement) {
-          if (isIfStatement(statement)) {
-            if (areEquivalent(condition, statement.test, sourceCode)) {
-              const line = ifStmt.loc && ifStmt.loc.start.line;
-              if (line && condition.loc) {
-                report(
-                  context,
-                  {
-                    messageId: 'duplicatedBranch',
-                    data: {
-                      line,
-                    },
-                    node: statement.test,
-                  },
-                  [issueLocation(condition.loc, condition.loc, 'Original')],
-                  duplicatedBranchMessage,
-                );
-              }
-            }
-            statement = statement.alternate;
-          } else {
+        const { test } = node as TSESTree.IfStatement;
+        const conditionsToCheck =
+          test.type === 'LogicalExpression' && test.operator === '&&'
+            ? [test, ...splitByAnd(test)]
+            : [test];
+
+        let current = node;
+        let operandsToCheck = conditionsToCheck.map(c => splitByOr(c).map(splitByAnd));
+        while (current.parent?.type === 'IfStatement' && current.parent.alternate === current) {
+          current = current.parent;
+
+          const currentOrOperands = splitByOr(current.test).map(splitByAnd);
+          operandsToCheck = operandsToCheck.map(orOperands =>
+            orOperands.filter(
+              orOperand =>
+                !currentOrOperands.some(currentOrOperand =>
+                  isSubset(currentOrOperand, orOperand, sourceCode),
+                ),
+            ),
+          );
+
+          if (operandsToCheck.some(orOperands => orOperands.length === 0)) {
+            report(
+              context,
+              {
+                messageId: 'duplicatedCondition',
+                data: { line: current.test.loc.start.line },
+                node: test,
+              },
+              [issueLocation(current.test.loc, current.test.loc, 'Covering')],
+              duplicatedConditionMessage,
+            );
             break;
           }
         }
@@ -113,4 +120,55 @@ const rule: TSESLint.RuleModule<string, string[]> = {
   },
 };
 
+const splitByOr = splitByLogicalOperator.bind(null, '||');
+const splitByAnd = splitByLogicalOperator.bind(null, '&&');
+
+function splitByLogicalOperator(
+  operator: '??' | '&&' | '||',
+  node: TSESTree.Node,
+): TSESTree.Node[] {
+  if (node.type === 'LogicalExpression' && node.operator === operator) {
+    return [
+      ...splitByLogicalOperator(operator, node.left),
+      ...splitByLogicalOperator(operator, node.right),
+    ];
+  }
+  return [node];
+}
+
+function isSubset(
+  first: TSESTree.Node[],
+  second: TSESTree.Node[],
+  sourceCode: TSESLint.SourceCode,
+): boolean {
+  return first.every(fst => second.some(snd => isSubsetOf(fst, snd, sourceCode)));
+
+  function isSubsetOf(
+    first: TSESTree.Node,
+    second: TSESTree.Node,
+    sourceCode: TSESLint.SourceCode,
+  ): boolean {
+    if (first.type !== second.type) {
+      return false;
+    }
+
+    if (first.type === 'LogicalExpression') {
+      const second1 = second as TSESTree.LogicalExpression;
+      if (
+        (first.operator === '||' || first.operator === '&&') &&
+        first.operator === second1.operator
+      ) {
+        return (
+          (isSubsetOf(first.left, second1.left, sourceCode) &&
+            isSubsetOf(first.right, second1.right, sourceCode)) ||
+          (isSubsetOf(first.left, second1.right, sourceCode) &&
+            isSubsetOf(first.right, second1.left, sourceCode))
+        );
+      }
+    }
+
+    return areEquivalent(first, second, sourceCode);
+  }
+}
+
 export = rule;
diff --git a/tests/rules/no-identical-conditions.test.ts b/tests/rules/no-identical-conditions.test.ts
@@ -20,6 +20,8 @@
 import { ruleTester } from '../rule-tester';
 import rule = require('../../src/rules/no-identical-conditions');
 
+const SONAR_RUNTIME = 'sonar-runtime';
+
 ruleTester.run('no-identical-conditions', rule, {
   valid: [
     {
@@ -28,6 +30,18 @@ ruleTester.run('no-identical-conditions', rule, {
     {
       code: 'if (a) {} else {}',
     },
+    {
+      code: 'if (a && b) {} else if (a) {}',
+    },
+    {
+      code: 'if (a && b) {} else if (c && d) {}',
+    },
+    {
+      code: 'if (a || b) {} else if (c || d) {}',
+    },
+    {
+      code: 'if (a ?? b) {} else if (c) {}',
+    },
     {
       code: `
       switch (a) {
@@ -47,7 +61,7 @@ ruleTester.run('no-identical-conditions', rule, {
       `,
       errors: [
         {
-          messageId: 'duplicatedBranch',
+          messageId: 'duplicatedCondition',
           data: {
             line: 2,
           },
@@ -64,7 +78,7 @@ ruleTester.run('no-identical-conditions', rule, {
         else if (a) {}
         //       ^
       `,
-      options: ['sonar-runtime'],
+      options: [SONAR_RUNTIME],
       errors: [
         {
           messageId: 'sonarRuntime',
@@ -77,10 +91,10 @@ ruleTester.run('no-identical-conditions', rule, {
                   column: 12,
                   endLine: 2,
                   endColumn: 13,
-                  message: 'Original',
+                  message: 'Covering',
                 },
               ],
-              message: 'This branch duplicates the one on line 2',
+              message: 'This condition is covered by the one on line 2',
             }),
           },
         },
@@ -94,7 +108,7 @@ ruleTester.run('no-identical-conditions', rule, {
       `,
       errors: [
         {
-          messageId: 'duplicatedBranch',
+          messageId: 'duplicatedCondition',
           data: {
             line: 3,
           },
@@ -112,7 +126,7 @@ ruleTester.run('no-identical-conditions', rule, {
       `,
       errors: [
         {
-          messageId: 'duplicatedBranch',
+          messageId: 'duplicatedCondition',
           data: {
             line: 2,
           },
@@ -122,6 +136,126 @@ ruleTester.run('no-identical-conditions', rule, {
         },
       ],
     },
+    {
+      code: `
+        if (a || b) {}
+        // >^^^^^^
+        else if (a) {}
+        //       ^`,
+      options: [SONAR_RUNTIME],
+      errors: [
+        {
+          messageId: 'sonarRuntime',
+          line: 4,
+          column: 18,
+          endLine: 4,
+          endColumn: 19,
+          data: {
+            line: 2,
+            sonarRuntimeData: JSON.stringify({
+              secondaryLocations: [
+                {
+                  line: 2,
+                  column: 12,
+                  endLine: 2,
+                  endColumn: 18,
+                  message: 'Covering',
+                },
+              ],
+              message: 'This condition is covered by the one on line 2',
+            }),
+          },
+        },
+      ],
+    },
+    {
+      code: `if (a || b) {} else if (b) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if ((a === b && fn(c)) || d) {} else if (a === b && fn(c)) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a) {} else if (a && b) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a && b) ; else if (b && a) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a) {} else if (b) {} else if (c && a || b) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a) {} else if (b) {} else if (c && (a || b)) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a) {} else if (b && c) {} else if (d && (a || e && c && b)) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a || b && c) {} else if (b && c && d) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a || b) {} else if (b && c) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a) {} else if (b) {} else if ((a || b) && c) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if ((a && (b || c)) || d) {} else if ((c || b) && e && a) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a && b || b && c) {} else if (a && b && c) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a) {} else if (b && c) {} else if (d && (c && e && b || a)) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a || (b && (c || d))) {} else if ((d || c) && b) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a || b) {} else if ((b || a) && c) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a || b) {} else if (c) {} else if (d) {} else if (b && (a || c)) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a || b || c) {} else if (a || (b && d) || (c && e)) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a || (b || c)) {} else if (a || (b && c)) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a || b) {} else if (c) {} else if (d) {} else if ((a || c) && (b || d)) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a) {} else if (b) {} else if (c && (a || d && b)) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a) {} else if (a || a) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
+    {
+      code: `if (a) {} else if (a && a) {}`,
+      errors: [{ messageId: 'duplicatedCondition' }],
+    },
     {
       code: `
         switch (a) {
@@ -136,7 +270,7 @@ ruleTester.run('no-identical-conditions', rule, {
             break;
         }
       `,
-      options: ['sonar-runtime'],
+      options: [SONAR_RUNTIME],
       errors: [
         {
           messageId: 'sonarRuntime',

Original file line number	Diff line number	Diff line change
`@@ -1 +1,2 @@`
	`1`	`+src/react-native/Libraries/vendor/core/Map.js: 582`
`1`	`2`	`src/spectrum/api/queries/user/contextPermissions.js: 63`