diff --git a/ruling/snapshots/no-identical-conditions b/ruling/snapshots/no-identical-conditions index f25637b7..7cdbc155 100644 --- 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 index c2f409e3..6a24488b 100644 --- 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 = { meta: { messages: { - duplicatedBranch: duplicatedBranchMessage, + duplicatedCondition: duplicatedConditionMessage, duplicatedCase: duplicatedCaseMessage, sonarRuntime: '{{sonarRuntimeData}}', }, @@ -53,30 +52,38 @@ const rule: TSESLint.RuleModule = { 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 = { }, }; +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 index 0992b8cd..381ced5c 100644 --- 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',