Skip to content

Commit 1d04da8

Browse files
ss-vibe-bot[bot]Vibe Botclaude
authored
JS-1111 JS-1111 Fix FP on S1143 for guard returns in finally blocks (#6332)
Co-authored-by: Vibe Bot <vibe-bot@sonarsource.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3cf63a1 commit 1d04da8

File tree

5 files changed

+414
-4
lines changed

5 files changed

+414
-4
lines changed

packages/jsts/src/rules/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,6 @@ SonarJS uses some rules are not shipped in this ESLint plugin to avoid duplicati
406406
| [S1090](https://sonarsource.github.io/rspec/#/rspec/S1090/javascript) | [jsx-a11y/iframe-has-title](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/iframe-has-title.md) |
407407
| [S1117](https://sonarsource.github.io/rspec/#/rspec/S1117/javascript) | [typescript-eslint/no-shadow](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-shadow.mdx) |
408408
| [S1131](https://sonarsource.github.io/rspec/#/rspec/S1131/javascript) | [eslint/no-trailing-spaces](https://eslint.org/docs/latest/rules/no-trailing-spaces) |
409-
| [S1143](https://sonarsource.github.io/rspec/#/rspec/S1143/javascript) | [eslint/no-unsafe-finally](https://eslint.org/docs/latest/rules/no-unsafe-finally) |
410409
| [S1199](https://sonarsource.github.io/rspec/#/rspec/S1199/javascript) | [eslint/no-lone-blocks](https://eslint.org/docs/latest/rules/no-lone-blocks) |
411410
| [S1314](https://sonarsource.github.io/rspec/#/rspec/S1314/javascript) | [eslint/no-octal](https://eslint.org/docs/latest/rules/no-octal) |
412411
| [S1321](https://sonarsource.github.io/rspec/#/rspec/S1321/javascript) | [eslint/no-with](https://eslint.org/docs/latest/rules/no-with) |
@@ -602,6 +601,7 @@ The following rules are used in SonarJS but not available in this ESLint plugin.
602601
| [S1082](https://sonarsource.github.io/rspec/#/rspec/S1082/javascript) | [jsx-a11y/mouse-events-have-key-events](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/mouse-events-have-key-events.md)<br>[jsx-a11y/click-events-have-key-events](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/click-events-have-key-events.md) |
603602
| [S1105](https://sonarsource.github.io/rspec/#/rspec/S1105/javascript) | [eslint/brace-style](https://eslint.org/docs/latest/rules/brace-style) |
604603
| [S1116](https://sonarsource.github.io/rspec/#/rspec/S1116/javascript) | [@stylistic/eslint-plugin/no-extra-semi](https://github.com/eslint-stylistic/eslint-stylistic/blob/main/packages/eslint-plugin/rules/no-extra-semi/README.md) |
604+
| [S1143](https://sonarsource.github.io/rspec/#/rspec/S1143/javascript) | [eslint/no-unsafe-finally](https://eslint.org/docs/latest/rules/no-unsafe-finally) |
605605
| [S1186](https://sonarsource.github.io/rspec/#/rspec/S1186/javascript) | [typescript-eslint/no-empty-function](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-empty-function.mdx) |
606606
| [S1438](https://sonarsource.github.io/rspec/#/rspec/S1438/javascript) | [@stylistic/eslint-plugin/semi](https://github.com/eslint-stylistic/eslint-stylistic/blob/main/packages/eslint-plugin/rules/semi/README.md) |
607607
| [S1534](https://sonarsource.github.io/rspec/#/rspec/S1534/javascript) | [eslint/no-dupe-keys](https://eslint.org/docs/latest/rules/no-dupe-keys)<br>[typescript-eslint/no-dupe-class-members](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-dupe-class-members.mdx)<br>[react/jsx-no-duplicate-props](https://github.com/jsx-eslint/eslint-plugin-react/blob/HEAD/docs/rules/jsx-no-duplicate-props.md) |
Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
/*
2+
* SonarQube JavaScript Plugin
3+
* Copyright (C) 2011-2025 SonarSource Sàrl
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
// https://sonarsource.github.io/rspec/#/rspec/S1143/javascript
18+
19+
import type { Rule } from 'eslint';
20+
import type { TSESTree } from '@typescript-eslint/utils';
21+
import { generateMeta, interceptReport } from '../helpers/index.js';
22+
import { findFirstMatchingAncestor } from '../helpers/ancestor.js';
23+
import * as meta from './generated-meta.js';
24+
25+
/**
26+
* Decorates the no-unsafe-finally rule to suppress false positives for guard return patterns.
27+
*
28+
* Suppresses reports when a return statement in a finally block is:
29+
* 1. The only statement in an IfStatement's consequent (single-statement guard)
30+
* 2. The IfStatement's test is a simple Identifier (boolean flag check like `cancelled`)
31+
* 3. There are statements after the IfStatement in the finally block
32+
* 4. The return has no argument (void return, not overriding a value)
33+
*
34+
* This pattern is commonly used in React async effect cleanup to prevent
35+
* state updates on unmounted components.
36+
*/
37+
export function decorate(rule: Rule.RuleModule): Rule.RuleModule {
38+
return interceptReport(
39+
{
40+
...rule,
41+
meta: generateMeta(meta, rule.meta),
42+
},
43+
(context, reportDescriptor) => {
44+
if (!('node' in reportDescriptor)) {
45+
context.report(reportDescriptor);
46+
return;
47+
}
48+
49+
const node = reportDescriptor.node as TSESTree.Node;
50+
51+
// Only handle ReturnStatement - other jump statements (break, continue, throw) are always flagged
52+
if (node.type !== 'ReturnStatement') {
53+
context.report(reportDescriptor);
54+
return;
55+
}
56+
57+
// Return with argument is always flagged (could override return value)
58+
if (node.argument !== null) {
59+
context.report(reportDescriptor);
60+
return;
61+
}
62+
63+
if (isGuardReturnInFinally(node)) {
64+
// Suppress the report - this is a valid guard pattern
65+
return;
66+
}
67+
68+
context.report(reportDescriptor);
69+
},
70+
);
71+
}
72+
73+
/**
74+
* Checks if the return statement is a guard pattern in a finally block.
75+
*
76+
* Algorithm:
77+
* 1. Find the enclosing IfStatement
78+
* 2. Verify the return is the only statement in the consequent
79+
* 3. Verify the condition is a simple Identifier
80+
* 4. Find the finally block containing the IfStatement
81+
* 5. Verify there are statements after the IfStatement in the finally block
82+
*/
83+
function isGuardReturnInFinally(returnNode: TSESTree.ReturnStatement): boolean {
84+
// Step 1: Find the enclosing IfStatement
85+
const ifStatement = findEnclosingIfStatement(returnNode);
86+
if (!ifStatement) {
87+
return false;
88+
}
89+
90+
// Step 2: Verify the return is the only statement in the consequent
91+
if (!isOnlyStatementInConsequent(returnNode, ifStatement)) {
92+
return false;
93+
}
94+
95+
// Step 3: Verify the condition is a simple Identifier
96+
if (ifStatement.test.type !== 'Identifier') {
97+
return false;
98+
}
99+
100+
// Step 4: Find the finally block containing the IfStatement
101+
const finallyBlock = findFinallyBlock(ifStatement);
102+
if (!finallyBlock) {
103+
return false;
104+
}
105+
106+
// Step 5: Verify there are statements after the IfStatement
107+
return hasStatementsAfter(ifStatement, finallyBlock);
108+
}
109+
110+
/**
111+
* Finds the immediate enclosing IfStatement for the return node.
112+
* The return must be directly inside the if's consequent.
113+
*/
114+
function findEnclosingIfStatement(
115+
returnNode: TSESTree.ReturnStatement,
116+
): TSESTree.IfStatement | null {
117+
let parent = returnNode.parent;
118+
119+
// Handle both `if (x) return;` and `if (x) { return; }`
120+
if (parent?.type === 'BlockStatement') {
121+
parent = parent.parent;
122+
}
123+
124+
if (parent?.type === 'IfStatement') {
125+
return parent;
126+
}
127+
128+
return null;
129+
}
130+
131+
/**
132+
* Checks if the return is the only statement in the if's consequent.
133+
*/
134+
function isOnlyStatementInConsequent(
135+
returnNode: TSESTree.ReturnStatement,
136+
ifStatement: TSESTree.IfStatement,
137+
): boolean {
138+
const consequent = ifStatement.consequent;
139+
140+
// Direct return: if (x) return;
141+
if (consequent === returnNode) {
142+
return true;
143+
}
144+
145+
// Block with single return: if (x) { return; }
146+
if (
147+
consequent.type === 'BlockStatement' &&
148+
consequent.body.length === 1 &&
149+
consequent.body[0] === returnNode
150+
) {
151+
return true;
152+
}
153+
154+
return false;
155+
}
156+
157+
/**
158+
* Finds the finally block that contains the given if statement.
159+
*/
160+
function findFinallyBlock(ifStatement: TSESTree.IfStatement): TSESTree.BlockStatement | null {
161+
const tryStatement = findFirstMatchingAncestor(
162+
ifStatement,
163+
node => node.type === 'TryStatement',
164+
) as TSESTree.TryStatement | undefined;
165+
166+
if (!tryStatement?.finalizer) {
167+
return null;
168+
}
169+
170+
// Verify the if statement is actually inside the finally block
171+
const finalizer = tryStatement.finalizer;
172+
if (isDescendantOf(ifStatement, finalizer)) {
173+
return finalizer;
174+
}
175+
176+
return null;
177+
}
178+
179+
/**
180+
* Checks if child is a descendant of parent.
181+
*/
182+
function isDescendantOf(child: TSESTree.Node, parent: TSESTree.BlockStatement): boolean {
183+
let current: TSESTree.Node | undefined = child;
184+
while (current) {
185+
if (current === parent) {
186+
return true;
187+
}
188+
current = current.parent;
189+
}
190+
return false;
191+
}
192+
193+
/**
194+
* Checks if there are statements after the if statement in the finally block.
195+
*/
196+
function hasStatementsAfter(
197+
ifStatement: TSESTree.IfStatement,
198+
finallyBlock: TSESTree.BlockStatement,
199+
): boolean {
200+
const statements = finallyBlock.body;
201+
const ifIndex = statements.indexOf(ifStatement);
202+
203+
// The if must be a direct child of the finally block
204+
if (ifIndex === -1) {
205+
return false;
206+
}
207+
208+
// There must be statements after the if
209+
return ifIndex < statements.length - 1;
210+
}

packages/jsts/src/rules/S1143/index.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,4 +15,6 @@
1515
* along with this program; if not, see https://sonarsource.com/license/ssal/
1616
*/
1717
import { getESLintCoreRule } from '../external/core.js';
18-
export const rule = getESLintCoreRule('no-unsafe-finally');
18+
import { decorate } from './decorator.js';
19+
20+
export const rule = decorate(getESLintCoreRule('no-unsafe-finally'));

packages/jsts/src/rules/S1143/meta.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,6 @@
1414
* You should have received a copy of the Sonar Source-Available License
1515
* along with this program; if not, see https://sonarsource.com/license/ssal/
1616
*/
17-
export const implementation = 'external';
17+
export const implementation = 'decorated';
1818
export const eslintId = 'no-unsafe-finally';
19-
export const externalPlugin = 'eslint';
19+
export const externalRules = [{ externalPlugin: 'eslint', externalRule: 'no-unsafe-finally' }];

0 commit comments

Comments
 (0)