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

Commit

Permalink
Modify rule S1862 (no-identical-conditions): Consider identical cas…
Browse files Browse the repository at this point in the history
…es of switch statements
  • Loading branch information
yassin-kammoun-sonarsource committed Mar 13, 2023
1 parent eaa92b6 commit 0a10153
Show file tree
Hide file tree
Showing 4 changed files with 123 additions and 8 deletions.
39 changes: 37 additions & 2 deletions docs/rules/no-identical-conditions.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# no-identical-conditions

A chain of `if`/`else if` statements is evaluated from top to bottom. At most, only
one branch will be executed: the first one with a condition that evaluates to `true`.
A chain of `if-else-if` and `switch-case` statements is evaluated from top to bottom. At most, only
one branch will be executed: the first one with a condition that evaluates to `true` or that matches the discriminant.

Therefore, duplicating a condition automatically leads to dead code. Usually, this is due to a
copy/paste error. At best, it's simply dead code and at worst, it's a bug that is likely to induce
Expand All @@ -17,4 +17,39 @@ if (param == 1) {
} else if (param == 1) { // Noncompliant
moveWindowToTheBackground();
}

switch (param) {
case 1:
openWindow();
break;
case 2:
closeWindow();
break;
case 1: // Noncompliant
moveWindowToTheBackground();
break;
}
```

## Compliant Solution

```javascript
if (param == 1)
openWindow();
else if (param == 2)
closeWindow();
else if (param == 3)
moveWindowToTheBackground();

switch (param) {
case 1:
openWindow();
break;
case 2:
closeWindow();
break;
case 3:
moveWindowToTheBackground();
break;
}
```
2 changes: 1 addition & 1 deletion ruling/snapshots/no-identical-conditions
Original file line number Diff line number Diff line change
@@ -1 +1 @@

src/spectrum/api/queries/user/contextPermissions.js: 63
42 changes: 37 additions & 5 deletions src/rules/no-identical-conditions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,20 @@ import { areEquivalent } from '../utils/equivalence';
import { report, issueLocation } from '../utils/locations';
import docsUrl from '../utils/docs-url';

const message = 'This branch duplicates the one on line {{line}}';
const duplicatedBranchMessage = 'This branch duplicates the one on line {{line}}';
const duplicatedCaseMessage = 'This case duplicates the one on line {{line}}';

const rule: TSESLint.RuleModule<string, string[]> = {
meta: {
messages: {
duplicatedBranch: message,
duplicatedBranch: duplicatedBranchMessage,
duplicatedCase: duplicatedCaseMessage,
sonarRuntime: '{{sonarRuntimeData}}',
},
type: 'problem',
docs: {
description: 'Related "if/else if" statements should not have the same condition',
description:
'Related "if-else-if" and "switch-case" statements should not have the same condition',
recommended: 'error',
url: docsUrl(__filename),
},
Expand All @@ -47,14 +50,15 @@ const rule: TSESLint.RuleModule<string, string[]> = {
],
},
create(context) {
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, context.getSourceCode())) {
if (areEquivalent(condition, statement.test, sourceCode)) {
const line = ifStmt.loc && ifStmt.loc.start.line;
if (line && condition.loc) {
report(
Expand All @@ -67,7 +71,7 @@ const rule: TSESLint.RuleModule<string, string[]> = {
node: statement.test,
},
[issueLocation(condition.loc, condition.loc, 'Original')],
message,
duplicatedBranchMessage,
);
}
}
Expand All @@ -77,6 +81,34 @@ const rule: TSESLint.RuleModule<string, string[]> = {
}
}
},
SwitchStatement(node: TSESTree.Node) {
const switchStmt = node as TSESTree.SwitchStatement;
const previousTests: TSESTree.Expression[] = [];
for (const switchCase of switchStmt.cases) {
if (switchCase.test) {
const { test } = switchCase;
const duplicateTest = previousTests.find(previousTest =>
areEquivalent(test, previousTest, sourceCode),
);
if (duplicateTest) {
report(
context,
{
messageId: 'duplicatedCase',
data: {
line: duplicateTest.loc.start.line,
},
node: test,
},
[issueLocation(duplicateTest.loc, duplicateTest.loc, 'Original')],
duplicatedCaseMessage,
);
} else {
previousTests.push(test);
}
}
}
},
};
},
};
Expand Down
48 changes: 48 additions & 0 deletions tests/rules/no-identical-conditions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ ruleTester.run('no-identical-conditions', rule, {
{
code: 'if (a) {} else {}',
},
{
code: `
switch (a) {
case 1: break;
case 2: break;
case 3: break;
default: break;
}
`,
},
],
invalid: [
{
Expand Down Expand Up @@ -112,5 +122,43 @@ ruleTester.run('no-identical-conditions', rule, {
},
],
},
{
code: `
switch (a) {
case 1:
f();
break;
case 2:
g();
break;
case 1:
h();
break;
}
`,
options: ['sonar-runtime'],
errors: [
{
messageId: 'sonarRuntime',
data: {
line: 9,
column: 15,
endColumn: 16,
sonarRuntimeData: JSON.stringify({
secondaryLocations: [
{
line: 3,
column: 15,
endLine: 3,
endColumn: 16,
message: 'Original',
},
],
message: 'This case duplicates the one on line 3',
}),
},
},
],
},
],
});

0 comments on commit 0a10153

Please sign in to comment.