Skip to content

Commit 670bef8

Browse files
sonar-nigel[bot]Vibe Bot
andauthored
JS-1584 Fix S905 false positive for comma operators sequencing side effects (#6855)
Co-authored-by: Vibe Bot <vibe-bot@sonarsource.com>
1 parent 2e39e52 commit 670bef8

5 files changed

Lines changed: 90 additions & 14 deletions

File tree

its/ruling/src/test/expected/Ghost/javascript-S905.json

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
11
{
2-
"Ghost:core/client/tests/integration/components/gh-profile-image-test.js": [
3-
46
4-
],
52
"Ghost:core/test/functional/routes/api/posts_spec.js": [
63
818
74
],

its/ruling/src/test/expected/ace/javascript-S905.json

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,6 @@
77
6606,
88
6611
99
],
10-
"ace:lib/ace/mode/javascript/jshint.js": [
11-
2387
12-
],
1310
"ace:lib/ace/worker/worker_client_v2.js": [
1411
55
1512
],

packages/analysis/src/jsts/rules/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ If you have any questions, encounter any bugs, or have feature requests, please
158158
| [confidential-information-logging](https://sonarsource.github.io/rspec/#/rspec/S5757/javascript) | Allowing confidential information to be logged is security-sensitive || | | | |
159159
| [constructor-for-side-effects](https://sonarsource.github.io/rspec/#/rspec/S1848/javascript) | Objects should not be created to be dropped immediately without being used || | | | |
160160
| [content-length](https://sonarsource.github.io/rspec/#/rspec/S5693/javascript) | HTTP request content length should be limited || | | | |
161-
| [content-security-policy](https://sonarsource.github.io/rspec/#/rspec/S5728/javascript) | Disabling content security policy fetch directives is security-sensitive || | | | |
161+
| [content-security-policy](https://sonarsource.github.io/rspec/#/rspec/S5728/javascript) | Content security policy fetch directives should not be disabled || | | | |
162162
| [cookie-no-httponly](https://sonarsource.github.io/rspec/#/rspec/S3330/javascript) | Creating cookies without the "HttpOnly" flag is security-sensitive || | | | |
163163
| [cookies](https://sonarsource.github.io/rspec/#/rspec/S2255/javascript) | Writing cookies is security-sensitive | | | | ||
164164
| [cors](https://sonarsource.github.io/rspec/#/rspec/S5122/javascript) | Cross-Origin Resource Sharing (CORS) policy should be restricted to trusted origins || | | | |
@@ -169,7 +169,7 @@ If you have any questions, encounter any bugs, or have feature requests, please
169169
| [destructuring-assignment-syntax](https://sonarsource.github.io/rspec/#/rspec/S3514/javascript) | Destructuring syntax should be used for assignments | | | | | |
170170
| [different-types-comparison](https://sonarsource.github.io/rspec/#/rspec/S3403/javascript) | Strict equality operators should not be used with dissimilar types || | 💡 | 💭 | |
171171
| [disabled-auto-escaping](https://sonarsource.github.io/rspec/#/rspec/S5247/javascript) | Disabling auto-escaping in template engines is security-sensitive || | | 💭 | |
172-
| [disabled-resource-integrity](https://sonarsource.github.io/rspec/#/rspec/S5725/javascript) | Using remote artifacts without integrity checks is security-sensitive || | | 💭 | |
172+
| [disabled-resource-integrity](https://sonarsource.github.io/rspec/#/rspec/S5725/javascript) | Remote artifacts should not be used without integrity checks || | | 💭 | |
173173
| [disabled-timeout](https://sonarsource.github.io/rspec/#/rspec/S6080/javascript) | Disabling Mocha timeouts should be explicit || | | | |
174174
| [dns-prefetching](https://sonarsource.github.io/rspec/#/rspec/S5743/javascript) | Allowing browsers to perform DNS prefetching is security-sensitive | | | | ||
175175
| [dompurify-unsafe-config](https://sonarsource.github.io/rspec/#/rspec/S8479/javascript) | DOMPurify configuration should not be bypassable || | | | |

packages/analysis/src/jsts/rules/S905/decorator.ts

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import type estree from 'estree';
1919
import type { TSESTree } from '@typescript-eslint/utils';
2020
import { generateMeta } from '../helpers/generate-meta.js';
2121
import { interceptReport } from '../helpers/decorators/interceptor.js';
22-
import { last } from '../helpers/collection.js';
2322
import * as meta from './generated-meta.js';
2423

2524
export function decorate(rule: Rule.RuleModule): Rule.RuleModule {
@@ -34,7 +33,7 @@ export function decorate(rule: Rule.RuleModule): Rule.RuleModule {
3433
containsChaiExpect(expr) ||
3534
containsValidChaiShould(expr) ||
3635
isAuraLightningComponent(expr) ||
37-
isSequenceWithSideEffects(expr),
36+
hasOnlySideEffects(expr),
3837
),
3938
);
4039
}
@@ -91,10 +90,22 @@ function isIife(node: estree.Node): boolean {
9190
);
9291
}
9392

94-
function isSequenceWithSideEffects(node: estree.Node): boolean {
95-
return (
96-
node.type === 'SequenceExpression' && last(node.expressions).type === 'AssignmentExpression'
97-
);
93+
function hasOnlySideEffects(node: estree.Node): boolean {
94+
switch (node.type) {
95+
case 'AssignmentExpression':
96+
case 'CallExpression':
97+
case 'NewExpression':
98+
case 'UpdateExpression':
99+
return true;
100+
case 'SequenceExpression':
101+
return node.expressions.every(hasOnlySideEffects);
102+
case 'ConditionalExpression':
103+
return hasOnlySideEffects(node.consequent) && hasOnlySideEffects(node.alternate);
104+
case 'LogicalExpression':
105+
return hasOnlySideEffects(node.right);
106+
default:
107+
return false;
108+
}
98109
}
99110

100111
function isAuraLightningComponent(node: estree.Node): boolean {

packages/analysis/src/jsts/rules/S905/unit.test.ts

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,29 @@
1515
* along with this program; if not, see https://sonarsource.com/license/ssal/
1616
*/
1717
import { rule } from './index.js';
18+
import { rules as tsEslintRules } from '../external/typescript-eslint/index.js';
1819
import { NoTypeCheckingRuleTester } from '../../../../tests/jsts/tools/testers/rule-tester.js';
1920
import { describe, it } from 'node:test';
2021

2122
const ruleTester = new NoTypeCheckingRuleTester();
2223

24+
// Sentinel: verify that the upstream ESLint rule still raises on the patterns our decorator fixes.
25+
// If this test starts failing (i.e., the upstream rule no longer reports these patterns),
26+
// it signals that the decorator can be safely removed.
27+
describe('S905 upstream sentinel', () => {
28+
it('upstream no-unused-expressions raises on comma operator sequences with side effects that decorator suppresses', () => {
29+
const upstreamRule = tsEslintRules['no-unused-expressions'];
30+
ruleTester.run('no-unused-expressions', upstreamRule, {
31+
valid: [],
32+
invalid: [
33+
{ code: `a(), b();`, errors: 1 }, // sequence of calls — suppressed by decorator, raised by upstream
34+
{ code: `++i, ++j;`, errors: 1 }, // sequence of updates — suppressed by decorator, raised by upstream
35+
{ code: `x = 1, f();`, errors: 1 }, // assignment then call — suppressed by decorator, raised by upstream
36+
],
37+
});
38+
});
39+
});
40+
2341
describe('S905', () => {
2442
it('S905', () => {
2543
ruleTester.run('Disallow unused expressions', rule, {
@@ -87,6 +105,54 @@ describe('S905', () => {
87105
});
88106
`,
89107
},
108+
// comma operator sequencing multiple function calls — all operands are calls with side effects
109+
{
110+
code: `
111+
for (let i = 0; i < items.length; i++) {
112+
output.push(items[i]), log.push(i);
113+
}
114+
`,
115+
},
116+
// comma operator sequencing increments — all operands are updates with side effects
117+
{
118+
code: `
119+
while (index < items.length) {
120+
++index, ++total;
121+
}
122+
`,
123+
},
124+
// assignment then call — both operands have side effects
125+
{
126+
code: `
127+
while (lexer.pos < lexer.source.length) {
128+
escaped = true, advance();
129+
}
130+
`,
131+
},
132+
// call then multiple increments — all operands have side effects
133+
{
134+
code: `
135+
while (i < j) {
136+
swap(array, i, j), ++i, --j;
137+
}
138+
`,
139+
},
140+
// ternary where alternate branch is a comma-operator sequence of assignments
141+
{
142+
code: `condition ? (state.x = 0) : (state.y = 1, state.z = 2);`,
143+
},
144+
// logical expression where right side is a comma-operator sequence of calls
145+
{
146+
code: `condition && (f(), g());`,
147+
},
148+
// nested ternary (UMD-style) where all branches are assignments or calls
149+
{
150+
code: `cond1 ? module.exports = factory() : cond2 ? define(factory) : (g = this, g.lib = factory());`,
151+
},
152+
// sequence of test-framework calls separated by commas
153+
{
154+
code: `it('first test', function() { assert(true); }), it('second test', function() { assert(true); });`,
155+
},
90156
],
91157
invalid: [
92158
{
@@ -152,6 +218,11 @@ describe('S905', () => {
152218
},
153219
],
154220
},
221+
// sequence where one element has no side effect — still flagged
222+
{
223+
code: `a + b, c();`,
224+
errors: 1,
225+
},
155226
],
156227
});
157228
});

0 commit comments

Comments
 (0)