From d3e3e39823a325f1efb7a8d08df59e2383e7bab0 Mon Sep 17 00:00:00 2001 From: nathsou Date: Tue, 4 Aug 2026 14:51:57 +0200 Subject: [PATCH 1/3] JS-2207: Implement S9135 --- .../custom-jsts/javascript-S9135.json | 6 + its/sources/custom/jsts/S9135.js | 34 +++ .../analysis/src/jsts/rules/S9135/index.ts | 17 ++ .../analysis/src/jsts/rules/S9135/meta.ts | 19 ++ .../analysis/src/jsts/rules/S9135/rule.ts | 226 ++++++++++++++++ .../src/jsts/rules/S9135/unit.test.ts | 251 ++++++++++++++++++ rspec.sha | 1 + .../javascript/rules/javascript/S9135.json | 31 +++ 8 files changed, 585 insertions(+) create mode 100644 its/ruling/src/test/expected/custom-jsts/javascript-S9135.json create mode 100644 its/sources/custom/jsts/S9135.js create mode 100644 packages/analysis/src/jsts/rules/S9135/index.ts create mode 100644 packages/analysis/src/jsts/rules/S9135/meta.ts create mode 100644 packages/analysis/src/jsts/rules/S9135/rule.ts create mode 100644 packages/analysis/src/jsts/rules/S9135/unit.test.ts create mode 100644 rspec.sha create mode 100644 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9135.json diff --git a/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json b/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json new file mode 100644 index 00000000000..bc0db89e1c1 --- /dev/null +++ b/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json @@ -0,0 +1,6 @@ +{ +"custom-jsts:S9135.js": [ +25, +28 +] +} diff --git a/its/sources/custom/jsts/S9135.js b/its/sources/custom/jsts/S9135.js new file mode 100644 index 00000000000..a89070fa9f1 --- /dev/null +++ b/its/sources/custom/jsts/S9135.js @@ -0,0 +1,34 @@ +// Copyright 2026 The Closure Library Authors. All Rights Reserved. +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute this program and/or modify it under the terms of + * the Sonar Source-Available License as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE, either express or implied. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +import lodash from 'lodash'; +import underscore from 'underscore'; + +const user = { address: { city: 'Bern' } }; +const address = user.address; + +const lodashCopy = lodash.clone(user); +lodashCopy.address.city = 'Geneva'; + +const underscoreCopy = underscore.clone(user); +underscoreCopy.address.city = 'Geneva'; + +const deepCopy = structuredClone(user); +deepCopy.address.city = 'Geneva'; + +const topLevelCopy = lodash.clone(user); +topLevelCopy.address = address; diff --git a/packages/analysis/src/jsts/rules/S9135/index.ts b/packages/analysis/src/jsts/rules/S9135/index.ts new file mode 100644 index 00000000000..6abe8e6e2e6 --- /dev/null +++ b/packages/analysis/src/jsts/rules/S9135/index.ts @@ -0,0 +1,17 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +export { rule } from './rule.js'; diff --git a/packages/analysis/src/jsts/rules/S9135/meta.ts b/packages/analysis/src/jsts/rules/S9135/meta.ts new file mode 100644 index 00000000000..10ff2e0edee --- /dev/null +++ b/packages/analysis/src/jsts/rules/S9135/meta.ts @@ -0,0 +1,19 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +export const implementation = 'original'; +export const eslintId = 'avoid-mutating-nested-properties-of-shallow-clones'; +export const hasSecondaries = true; diff --git a/packages/analysis/src/jsts/rules/S9135/rule.ts b/packages/analysis/src/jsts/rules/S9135/rule.ts new file mode 100644 index 00000000000..ca64fc61d7e --- /dev/null +++ b/packages/analysis/src/jsts/rules/S9135/rule.ts @@ -0,0 +1,226 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +// https://sonarsource.github.io/rspec/#/rspec/S9135/javascript + +import type { Rule } from 'eslint'; +import type estree from 'estree'; +import { getUniqueWriteReference, getVariableFromName } from '../helpers/ast.js'; +import { getParent } from '../helpers/ancestor.js'; +import { report, toSecondaryLocation } from '../helpers/location.js'; +import { getFullyQualifiedName } from '../helpers/module.js'; +import { generateMeta } from '../helpers/generate-meta.js'; +import * as meta from './generated-meta.js'; + +const LODASH_MESSAGE = + 'Mutating a nested property of this _.clone() changes the original value; use structuredClone() or _.cloneDeep() when isolation is required.'; +const UNDERSCORE_MESSAGE = + 'Mutating a nested property of this _.clone() changes the original value; use structuredClone() when isolation is required.'; +const SECONDARY_MESSAGE = 'Shallow clone created here.'; +const NOSONAR_COMMENT = ' // NOSONAR: shared nested state is intentional'; + +const CLONE_LIBRARIES = new Map([ + ['lodash.clone', 'lodash'], + ['lodash-es.clone', 'lodash'], + ['underscore.clone', 'underscore'], +]); + +const LOOP_TYPES = new Set([ + 'DoWhileStatement', + 'ForInStatement', + 'ForOfStatement', + 'ForStatement', + 'WhileStatement', +]); + +type MutationNode = estree.AssignmentExpression | estree.UpdateExpression | estree.UnaryExpression; + +type StaticMemberChain = { + root: estree.Identifier; + depth: number; +}; + +export const rule: Rule.RuleModule = { + meta: generateMeta(meta, { hasSuggestions: true }), + create(context: Rule.RuleContext) { + return { + AssignmentExpression: (node: estree.Node): void => { + const assignment = node as estree.AssignmentExpression; + checkMutation(context, assignment.left, assignment); + }, + UpdateExpression: (node: estree.Node): void => { + const update = node as estree.UpdateExpression; + checkMutation(context, update.argument, update); + }, + UnaryExpression: (node: estree.Node): void => { + const unary = node as estree.UnaryExpression; + if (unary.operator === 'delete') { + checkMutation(context, unary.argument, unary); + } + }, + }; + }, +}; + +function checkMutation( + context: Rule.RuleContext, + mutatedNode: estree.Node, + mutation: MutationNode, +): void { + const statement = getMutationStatement(context, mutation); + if (statement === undefined || isInsideLoop(mutation)) { + return; + } + + const memberChain = getStaticMemberChain(mutatedNode); + if (memberChain === undefined) { + return; + } + + const cloneCall = getCloneCall(context, memberChain.root); + if ( + cloneCall === undefined || + isInsideLoop(cloneCall) || + getScopeBoundary(cloneCall) !== getScopeBoundary(mutation) + ) { + return; + } + + const library = getCloneLibrary(context, cloneCall); + if (library === undefined) { + return; + } + + const argument = cloneCall.arguments[0]; + if (argument === undefined || argument.type === 'SpreadElement') { + return; + } + + report( + context, + { + node: statement, + message: library === 'underscore' ? UNDERSCORE_MESSAGE : LODASH_MESSAGE, + suggest: [ + { + desc: 'Replace the shallow clone with structuredClone()', + fix: replaceWithStructuredClone(cloneCall, argument, context), + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + fix: addNosonarComment(statement), + }, + ], + }, + [toSecondaryLocation(cloneCall, SECONDARY_MESSAGE)], + ); +} + +function getMutationStatement( + context: Rule.RuleContext, + node: MutationNode, +): estree.ExpressionStatement | undefined { + const parent = getParent(context, node); + return parent?.type === 'ExpressionStatement' ? parent : undefined; +} + +function getStaticMemberChain(node: estree.Node): StaticMemberChain | undefined { + let current: estree.Node = node; + let depth = 0; + + while (current.type === 'MemberExpression') { + if (current.computed || current.property.type !== 'Identifier') { + return undefined; + } + depth += 1; + current = current.object; + } + + return current.type === 'Identifier' && depth >= 2 ? { root: current, depth } : undefined; +} + +function getCloneCall( + context: Rule.RuleContext, + root: estree.Identifier, +): estree.CallExpression | undefined { + const variable = getVariableFromName(context, root.name, root); + const writeExpression = getUniqueWriteReference(variable); + if (writeExpression?.type !== 'CallExpression' || writeExpression.arguments.length !== 1) { + return undefined; + } + return writeExpression; +} + +function getCloneLibrary( + context: Rule.RuleContext, + cloneCall: estree.CallExpression, +): 'lodash' | 'underscore' | undefined { + return CLONE_LIBRARIES.get(getFullyQualifiedName(context, cloneCall.callee) ?? ''); +} + +function isInsideLoop(node: estree.Node): boolean { + let current = getNodeParent(node); + while (current != null) { + if (LOOP_TYPES.has(current.type)) { + return true; + } + if (isFunctionLike(current)) { + return false; + } + current = getNodeParent(current); + } + return false; +} + +function getScopeBoundary(node: estree.Node): estree.Node { + let current = node; + while (getNodeParent(current) != null) { + const parent = getNodeParent(current); + if (parent.type === 'Program' || isFunctionLike(parent)) { + return parent; + } + current = parent; + } + return current; +} + +function isFunctionLike( + node: estree.Node, +): node is estree.ArrowFunctionExpression | estree.FunctionDeclaration | estree.FunctionExpression { + return ( + node.type === 'ArrowFunctionExpression' || + node.type === 'FunctionDeclaration' || + node.type === 'FunctionExpression' + ); +} + +function replaceWithStructuredClone( + cloneCall: estree.CallExpression, + argument: estree.Node, + context: Rule.RuleContext, +): Rule.ReportFixer { + const argumentText = context.sourceCode.getText(argument); + return (fixer: Rule.RuleFixer): Rule.Fix => + fixer.replaceText(cloneCall, `structuredClone(${argumentText})`); +} + +function addNosonarComment(statement: estree.ExpressionStatement): Rule.ReportFixer { + return (fixer: Rule.RuleFixer): Rule.Fix => fixer.insertTextAfter(statement, NOSONAR_COMMENT); +} + +function getNodeParent(node: estree.Node): estree.Node { + return (node as estree.Node & { parent: estree.Node }).parent; +} diff --git a/packages/analysis/src/jsts/rules/S9135/unit.test.ts b/packages/analysis/src/jsts/rules/S9135/unit.test.ts new file mode 100644 index 00000000000..8fd08af61c8 --- /dev/null +++ b/packages/analysis/src/jsts/rules/S9135/unit.test.ts @@ -0,0 +1,251 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +import { describe, it } from 'node:test'; +import { DefaultParserRuleTester } from '../../../../tests/jsts/tools/testers/rule-tester.js'; +import { rule } from './rule.js'; + +const LODASH_MESSAGE = + 'Mutating a nested property of this _.clone() changes the original value; use structuredClone() or _.cloneDeep() when isolation is required.'; +const UNDERSCORE_MESSAGE = + 'Mutating a nested property of this _.clone() changes the original value; use structuredClone() when isolation is required.'; + +describe('S9135', () => { + it('reports nested mutations of shallow clones and offers both suggestions', () => { + const ruleTester = new DefaultParserRuleTester(); + + ruleTester.run('avoid-mutating-nested-properties-of-shallow-clones', rule, { + valid: [ + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address.city; +`, + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +const alias = copy; +alias.address.city = 'Geneva'; +`, + }, + { + code: ` +import _ from 'lodash'; +let copy = _.clone(user); +copy = other; +copy.address.city = 'Geneva'; +`, + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +function update() { + copy.address.city = 'Geneva'; +} +`, + }, + { + code: ` +import _ from 'lodash'; +for (const user of users) { + const copy = _.clone(user); + copy.address.city = 'Geneva'; +} +`, + }, + { + code: ` +const copy = clone(user); +copy.address.city = 'Geneva'; +`, + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy['address'].city = 'Geneva'; +`, + }, + ], + invalid: [ + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address.city = 'Geneva'; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +copy.address.city = 'Geneva'; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import clone from 'lodash/clone'; +const copy = clone(user); +copy.settings.retryCount++; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import clone from 'lodash/clone'; +const copy = structuredClone(user); +copy.settings.retryCount++; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import clone from 'lodash/clone'; +const copy = clone(user); +copy.settings.retryCount++; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import { clone } from 'lodash-es'; +const copy = clone(user); +delete copy.address.city; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import { clone } from 'lodash-es'; +const copy = structuredClone(user); +delete copy.address.city; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import { clone } from 'lodash-es'; +const copy = clone(user); +delete copy.address.city; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'underscore'; +const copy = _.clone(user); +copy.address.city = 'Geneva'; +`, + errors: [ + { + message: UNDERSCORE_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'underscore'; +const copy = structuredClone(user); +copy.address.city = 'Geneva'; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'underscore'; +const copy = _.clone(user); +copy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + ], + }); + }); + + it('reports the clone call as a secondary location in Sonar runtime mode', () => { + const ruleTester = new DefaultParserRuleTester(); + + ruleTester.run('avoid-mutating-nested-properties-of-shallow-clones', rule, { + valid: [], + invalid: [ + { + code: `import _ from 'lodash';\nconst copy = _.clone(user);\ncopy.address.city = 'Geneva';`, + settings: { sonarRuntime: true }, + errors: [ + { + message: JSON.stringify({ + message: LODASH_MESSAGE, + secondaryLocations: [ + { + message: 'Shallow clone created here.', + column: 13, + line: 2, + endColumn: 26, + endLine: 2, + }, + ], + }), + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: `import _ from 'lodash';\nconst copy = structuredClone(user);\ncopy.address.city = 'Geneva';`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: `import _ from 'lodash';\nconst copy = _.clone(user);\ncopy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional`, + }, + ], + }, + ], + }, + ], + }); + }); +}); diff --git a/rspec.sha b/rspec.sha new file mode 100644 index 00000000000..0b3ad273745 --- /dev/null +++ b/rspec.sha @@ -0,0 +1 @@ +c309512b913f2542cf37a70d3bc8950d006310ec diff --git a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9135.json b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9135.json new file mode 100644 index 00000000000..61598f34b17 --- /dev/null +++ b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9135.json @@ -0,0 +1,31 @@ +{ + "title": "Avoid mutating nested properties of shallow clones", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant/Issue", + "constantCost": "5min" + }, + "tags": [ + "lodash", + "underscore.js" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9135", + "sqKey": "S9135", + "scope": "Main", + "quickfix": "covered", + "code": { + "impacts": { + "RELIABILITY": "MEDIUM" + }, + "attribute": "LOGICAL" + }, + "compatibleLanguages": [ + "js", + "ts" + ], + "defaultQualityProfiles": [ + "Sonar way" + ] +} From 830012c726049a194edc1ab09e79b9a3d5b57953 Mon Sep 17 00:00:00 2001 From: nathsou Date: Tue, 4 Aug 2026 15:15:35 +0200 Subject: [PATCH 2/3] JS-2207: Address S9135 review feedback --- .../analysis/src/jsts/rules/S9135/rule.ts | 90 ++++++----- .../src/jsts/rules/S9135/unit.test.ts | 141 ++++++++++++++++-- rspec.sha | 2 +- 3 files changed, 183 insertions(+), 50 deletions(-) diff --git a/packages/analysis/src/jsts/rules/S9135/rule.ts b/packages/analysis/src/jsts/rules/S9135/rule.ts index ca64fc61d7e..a9fb8b8ad99 100644 --- a/packages/analysis/src/jsts/rules/S9135/rule.ts +++ b/packages/analysis/src/jsts/rules/S9135/rule.ts @@ -18,7 +18,11 @@ import type { Rule } from 'eslint'; import type estree from 'estree'; -import { getUniqueWriteReference, getVariableFromName } from '../helpers/ast.js'; +import { + getUniqueWriteReference, + getVariableFromName, + unwrapTypeScriptExpression, +} from '../helpers/ast.js'; import { getParent } from '../helpers/ancestor.js'; import { report, toSecondaryLocation } from '../helpers/location.js'; import { getFullyQualifiedName } from '../helpers/module.js'; @@ -38,14 +42,6 @@ const CLONE_LIBRARIES = new Map([ ['underscore.clone', 'underscore'], ]); -const LOOP_TYPES = new Set([ - 'DoWhileStatement', - 'ForInStatement', - 'ForOfStatement', - 'ForStatement', - 'WhileStatement', -]); - type MutationNode = estree.AssignmentExpression | estree.UpdateExpression | estree.UnaryExpression; type StaticMemberChain = { @@ -81,7 +77,7 @@ function checkMutation( mutation: MutationNode, ): void { const statement = getMutationStatement(context, mutation); - if (statement === undefined || isInsideLoop(mutation)) { + if (statement === undefined) { return; } @@ -91,11 +87,7 @@ function checkMutation( } const cloneCall = getCloneCall(context, memberChain.root); - if ( - cloneCall === undefined || - isInsideLoop(cloneCall) || - getScopeBoundary(cloneCall) !== getScopeBoundary(mutation) - ) { + if (cloneCall === undefined || getScopeBoundary(cloneCall) !== getScopeBoundary(mutation)) { return; } @@ -109,20 +101,25 @@ function checkMutation( return; } + const nosonarFix = getNosonarFix(context, statement, mutation); report( context, { - node: statement, + node: mutation, message: library === 'underscore' ? UNDERSCORE_MESSAGE : LODASH_MESSAGE, suggest: [ { desc: 'Replace the shallow clone with structuredClone()', fix: replaceWithStructuredClone(cloneCall, argument, context), }, - { - desc: 'Add // NOSONAR: shared nested state is intentional', - fix: addNosonarComment(statement), - }, + ...(nosonarFix === undefined + ? [] + : [ + { + desc: 'Add // NOSONAR: shared nested state is intentional', + fix: nosonarFix, + }, + ]), ], }, [toSecondaryLocation(cloneCall, SECONDARY_MESSAGE)], @@ -132,13 +129,19 @@ function checkMutation( function getMutationStatement( context: Rule.RuleContext, node: MutationNode, -): estree.ExpressionStatement | undefined { - const parent = getParent(context, node); - return parent?.type === 'ExpressionStatement' ? parent : undefined; +): estree.Node | undefined { + let parent = getParent(context, node); + while (parent !== undefined) { + if (parent.type.endsWith('Statement') || parent.type === 'VariableDeclaration') { + return parent; + } + parent = getParent(context, parent); + } + return undefined; } function getStaticMemberChain(node: estree.Node): StaticMemberChain | undefined { - let current: estree.Node = node; + let current: estree.Node = unwrapTypeScriptExpression(node); let depth = 0; while (current.type === 'MemberExpression') { @@ -146,7 +149,7 @@ function getStaticMemberChain(node: estree.Node): StaticMemberChain | undefined return undefined; } depth += 1; - current = current.object; + current = unwrapTypeScriptExpression(current.object); } return current.type === 'Identifier' && depth >= 2 ? { root: current, depth } : undefined; @@ -171,20 +174,6 @@ function getCloneLibrary( return CLONE_LIBRARIES.get(getFullyQualifiedName(context, cloneCall.callee) ?? ''); } -function isInsideLoop(node: estree.Node): boolean { - let current = getNodeParent(node); - while (current != null) { - if (LOOP_TYPES.has(current.type)) { - return true; - } - if (isFunctionLike(current)) { - return false; - } - current = getNodeParent(current); - } - return false; -} - function getScopeBoundary(node: estree.Node): estree.Node { let current = node; while (getNodeParent(current) != null) { @@ -217,10 +206,31 @@ function replaceWithStructuredClone( fixer.replaceText(cloneCall, `structuredClone(${argumentText})`); } -function addNosonarComment(statement: estree.ExpressionStatement): Rule.ReportFixer { +function getNosonarFix( + context: Rule.RuleContext, + statement: estree.Node, + mutation: MutationNode, +): Rule.ReportFixer | undefined { + if ( + statement.loc?.start.line !== mutation.loc?.start.line || + statement.loc?.end.line !== mutation.loc?.start.line || + !hasNoTokensAfter(context, statement) + ) { + return undefined; + } return (fixer: Rule.RuleFixer): Rule.Fix => fixer.insertTextAfter(statement, NOSONAR_COMMENT); } +function hasNoTokensAfter(context: Rule.RuleContext, node: estree.Node): boolean { + const range = node.range; + if (range === undefined) { + return false; + } + const source = context.sourceCode.getText(); + const lineEnd = source.indexOf('\n', range[1]); + return source.slice(range[1], lineEnd === -1 ? source.length : lineEnd).trim() === ''; +} + function getNodeParent(node: estree.Node): estree.Node { return (node as estree.Node & { parent: estree.Node }).parent; } diff --git a/packages/analysis/src/jsts/rules/S9135/unit.test.ts b/packages/analysis/src/jsts/rules/S9135/unit.test.ts index 8fd08af61c8..d49a37525c8 100644 --- a/packages/analysis/src/jsts/rules/S9135/unit.test.ts +++ b/packages/analysis/src/jsts/rules/S9135/unit.test.ts @@ -16,6 +16,8 @@ */ import { describe, it } from 'node:test'; import { DefaultParserRuleTester } from '../../../../tests/jsts/tools/testers/rule-tester.js'; +import { RuleTester as ESLintRuleTester } from 'eslint'; +import tsParser from '@typescript-eslint/parser'; import { rule } from './rule.js'; const LODASH_MESSAGE = @@ -59,15 +61,6 @@ const copy = _.clone(user); function update() { copy.address.city = 'Geneva'; } -`, - }, - { - code: ` -import _ from 'lodash'; -for (const user of users) { - const copy = _.clone(user); - copy.address.city = 'Geneva'; -} `, }, { @@ -199,6 +192,136 @@ copy.address.city = 'Geneva'; import _ from 'underscore'; const copy = _.clone(user); copy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +for (const user of users) { + const copy = _.clone(user); + copy.address.city = 'Geneva'; +} +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +for (const user of users) { + const copy = structuredClone(user); + copy.address.city = 'Geneva'; +} +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +for (const user of users) { + const copy = _.clone(user); + copy.address.city = 'Geneva'; // NOSONAR: shared nested state is intentional +} +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +if (enabled) copy.address.city = 'Geneva'; else reset(); +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +if (enabled) copy.address.city = 'Geneva'; else reset(); +`, + }, + ], + }, + ], + }, + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +const result = (copy.address.city = 'Geneva'); +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +const result = (copy.address.city = 'Geneva'); +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +const copy = _.clone(user); +const result = (copy.address.city = 'Geneva'); // NOSONAR: shared nested state is intentional +`, + }, + ], + }, + ], + }, + ], + }); + }); + + it('reports nested mutations through TypeScript non-null assertions', () => { + const ruleTester = new ESLintRuleTester({ + languageOptions: { parser: tsParser }, + }); + + ruleTester.run('avoid-mutating-nested-properties-of-shallow-clones', rule, { + valid: [], + invalid: [ + { + code: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address!.city = 'Geneva'; +`, + errors: [ + { + message: LODASH_MESSAGE, + suggestions: [ + { + desc: 'Replace the shallow clone with structuredClone()', + output: ` +import _ from 'lodash'; +const copy = structuredClone(user); +copy.address!.city = 'Geneva'; +`, + }, + { + desc: 'Add // NOSONAR: shared nested state is intentional', + output: ` +import _ from 'lodash'; +const copy = _.clone(user); +copy.address!.city = 'Geneva'; // NOSONAR: shared nested state is intentional `, }, ], diff --git a/rspec.sha b/rspec.sha index 0b3ad273745..4824cbfa0ac 100644 --- a/rspec.sha +++ b/rspec.sha @@ -1 +1 @@ -c309512b913f2542cf37a70d3bc8950d006310ec +14dbb7cbb63262250263c3153f1ba04b6fed8109 From 87f8d4893f5916c03caa9db5af1642d956c37280 Mon Sep 17 00:00:00 2001 From: nathsou Date: Tue, 4 Aug 2026 16:30:11 +0200 Subject: [PATCH 3/3] JS-2207: Fix S9135 review feedback --- .../src/test/expected/custom-jsts/javascript-S1451.json | 3 +++ .../src/test/expected/custom-jsts/javascript-S9135.json | 4 ++-- its/sources/custom/jsts/S9135.js | 1 - packages/analysis/src/jsts/rules/S9135/rule.ts | 4 ++-- packages/analysis/src/jsts/rules/S9135/unit.test.ts | 4 ++-- 5 files changed, 9 insertions(+), 7 deletions(-) diff --git a/its/ruling/src/test/expected/custom-jsts/javascript-S1451.json b/its/ruling/src/test/expected/custom-jsts/javascript-S1451.json index acd8b00378c..8dd03a37ad7 100644 --- a/its/ruling/src/test/expected/custom-jsts/javascript-S1451.json +++ b/its/ruling/src/test/expected/custom-jsts/javascript-S1451.json @@ -212,6 +212,9 @@ "custom-jsts:S7790.js": [ 0 ], +"custom-jsts:S9135.js": [ +0 +], "custom-jsts:boundOrAssignedEvalOrArguments.js": [ 0 ] diff --git a/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json b/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json index bc0db89e1c1..bc80b600bbb 100644 --- a/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json +++ b/its/ruling/src/test/expected/custom-jsts/javascript-S9135.json @@ -1,6 +1,6 @@ { "custom-jsts:S9135.js": [ -25, -28 +24, +27 ] } diff --git a/its/sources/custom/jsts/S9135.js b/its/sources/custom/jsts/S9135.js index a89070fa9f1..6b7c600146a 100644 --- a/its/sources/custom/jsts/S9135.js +++ b/its/sources/custom/jsts/S9135.js @@ -1,4 +1,3 @@ -// Copyright 2026 The Closure Library Authors. All Rights Reserved. /* * SonarQube JavaScript Plugin * Copyright (C) SonarSource Sàrl diff --git a/packages/analysis/src/jsts/rules/S9135/rule.ts b/packages/analysis/src/jsts/rules/S9135/rule.ts index a9fb8b8ad99..72f634d2492 100644 --- a/packages/analysis/src/jsts/rules/S9135/rule.ts +++ b/packages/analysis/src/jsts/rules/S9135/rule.ts @@ -30,9 +30,9 @@ import { generateMeta } from '../helpers/generate-meta.js'; import * as meta from './generated-meta.js'; const LODASH_MESSAGE = - 'Mutating a nested property of this _.clone() changes the original value; use structuredClone() or _.cloneDeep() when isolation is required.'; + 'Mutating a nested property of this shallow clone changes the original value; use structuredClone() or _.cloneDeep() when isolation is required.'; const UNDERSCORE_MESSAGE = - 'Mutating a nested property of this _.clone() changes the original value; use structuredClone() when isolation is required.'; + 'Mutating a nested property of this shallow clone changes the original value; use structuredClone() when isolation is required.'; const SECONDARY_MESSAGE = 'Shallow clone created here.'; const NOSONAR_COMMENT = ' // NOSONAR: shared nested state is intentional'; diff --git a/packages/analysis/src/jsts/rules/S9135/unit.test.ts b/packages/analysis/src/jsts/rules/S9135/unit.test.ts index d49a37525c8..9f5e983758c 100644 --- a/packages/analysis/src/jsts/rules/S9135/unit.test.ts +++ b/packages/analysis/src/jsts/rules/S9135/unit.test.ts @@ -21,9 +21,9 @@ import tsParser from '@typescript-eslint/parser'; import { rule } from './rule.js'; const LODASH_MESSAGE = - 'Mutating a nested property of this _.clone() changes the original value; use structuredClone() or _.cloneDeep() when isolation is required.'; + 'Mutating a nested property of this shallow clone changes the original value; use structuredClone() or _.cloneDeep() when isolation is required.'; const UNDERSCORE_MESSAGE = - 'Mutating a nested property of this _.clone() changes the original value; use structuredClone() when isolation is required.'; + 'Mutating a nested property of this shallow clone changes the original value; use structuredClone() when isolation is required.'; describe('S9135', () => { it('reports nested mutations of shallow clones and offers both suggestions', () => {