Skip to content
This repository has been archived by the owner on Oct 3, 2024. It is now read-only.

Commit

Permalink
Modify S1862 (no-identical-conditions): Detect duplicated condition…
Browse files Browse the repository at this point in the history 
… overlaps
  • Loading branch information
@yassin-kammoun-sonarsource
yassin-kammoun-sonarsource committed Mar 16, 2023
1 parent c347dae commit 4dfd992
Show file tree
Hide file tree
Showing 3 changed files with 227 additions and 34 deletions.
1 change: 1 addition & 0 deletions ruling/snapshots/no-identical-conditions
View file
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
src/react-native/Libraries/vendor/core/Map.js: 582
src/spectrum/api/queries/user/contextPermissions.js: 63
112 changes: 85 additions & 27 deletions src/rules/no-identical-conditions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}}',
},
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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;
148 changes: 141 additions & 7 deletions tests/rules/no-identical-conditions.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
{
Expand All @@ -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) {
Expand All @@ -47,7 +61,7 @@ ruleTester.run('no-identical-conditions', rule, {
`,
errors: [
{
messageId: 'duplicatedBranch',
messageId: 'duplicatedCondition',
data: {
line: 2,
},
Expand All @@ -64,7 +78,7 @@ ruleTester.run('no-identical-conditions', rule, {
else if (a) {}
// ^
`,
options: ['sonar-runtime'],
options: [SONAR_RUNTIME],
errors: [
{
messageId: 'sonarRuntime',
Expand All @@ -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',
}),
},
},
Expand All @@ -94,7 +108,7 @@ ruleTester.run('no-identical-conditions', rule, {
`,
errors: [
{
messageId: 'duplicatedBranch',
messageId: 'duplicatedCondition',
data: {
line: 3,
},
Expand All @@ -112,7 +126,7 @@ ruleTester.run('no-identical-conditions', rule, {
`,
errors: [
{
messageId: 'duplicatedBranch',
messageId: 'duplicatedCondition',
data: {
line: 2,
},
Expand All @@ -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) {
Expand All @@ -136,7 +270,7 @@ ruleTester.run('no-identical-conditions', rule, {
break;
}
`,
options: ['sonar-runtime'],
options: [SONAR_RUNTIME],
errors: [
{
messageId: 'sonarRuntime',
Expand Down

0 comments on commit 4dfd992

Please sign in to comment.