JS-1947 Group S2933 issues by class by francois-mora-sonarsource · Pull Request #7401 · SonarSource/SonarJS

francois-mora-sonarsource · 2026-06-30T13:44:15Z

Summary

Recreates the closed work from #7366 on top of the latest origin/master, using the new JS Jira key instead of the old AT-23 branch.

This groups S2933 (typescript-eslint/prefer-readonly) reports by class, preserving each readonly-eligible member as a secondary location and keeping the aggregate quick fix for adding readonly.

The branch also carries the nyc ESM loader source-map fallback from the original PR, adapted to the newer SourceMaps cache/content-hash implementation now present on master.

Notes

The old generated rule JSON changes already match current origin/master. The profile files from the old PR were not replayed because current master contains newer profile entries and regenerating rule data produced no profile diff.

Validation

npm install
npm run generate-rule-data:maven
npm run generate-meta
npx tsx --tsconfig packages/tsconfig.test.json --test packages/analysis/src/jsts/rules/S2933/unit.test.ts packages/analysis/tests/jsts/linter/index.test.ts packages/analysis/tests/tools/nyc-esm-loader.test.ts
npx tsc -b packages
npm run bbf

hashicorp-vault-sonar-prod · 2026-06-30T13:44:31Z

JS-1947

github-actions · 2026-06-30T13:51:57Z

Ruling Report

Code no longer flagged (3251 issues)

S2486

Ghost/core/client/app/controllers/feature.js:16

    14 |             try {
    15 |                 value = JSON.parse(this.get('setting.labs') || {});
>   16 |             } catch (err) {
    17 |                 value = {};
    18 |             }

Ghost/core/client/app/controllers/settings/navigation.js:59

    57 |         try {
    58 |             navItems = JSON.parse(this.get('model.navigation') || [{}]);
>   59 |         } catch (e) {
    60 |             navItems = [{}];
    61 |         }

Ghost/core/client/app/services/config.js:19

    17 |         try {
    18 |             return JSON.parse(val);
>   19 |         } catch (e) {
    20 |             return val;
    21 |         }

Ghost/core/client/app/utils/ajax.js:30

    28 |                 message =  request.responseJSON.error || 'Unknown Error';
    29 |             }
>   30 |         } catch (e) {
    31 |             msgDetail = request.status ? `${request.status} - ${request.statusText}` : 'Server was not available';
    32 |             message = `The server returned an error (${msgDetail}).`;

Ghost/core/server/api/settings.js:48

    46 |         try {
    47 |             labsValue = JSON.parse(settingsCache.labs.value);
>   48 |         } catch (e) {
    49 |             errors.logError.apply(this, errorMessages);
    50 |         }

Ghost/core/server/config/index.js:95

    93 |         try {
    94 |             pg = require('pg');
>   95 |         } catch (e) {
    96 |             pg = require('pg.js');
    97 |         }

Ghost/core/server/ghost-server.js:46

    44 |             try {
    45 |                 fs.unlinkSync(socketConfig.path);
>   46 |             } catch (e) {
    47 |                 // We can ignore this.
    48 |             }

Ghost/core/test/utils/index.js:325

   323 |             try {
   324 |                 data = JSON.parse(fileContents);
>  325 |             } catch (e) {
   326 |                 return new Error('Failed to parse the file');
   327 |             }

TypeScript/lib/tsc.js:2198

  2196 |                             stat = _fs.statSync(name);
  2197 |                         }
> 2198 |                         catch (e) {
  2199 |                             continue;
  2200 |                         }

TypeScript/lib/tsc.js:2310

  2308 |                         return _fs.statSync(path).mtime;
  2309 |                     }
> 2310 |                     catch (e) {
  2311 |                         return undefined;
  2312 |                     }

...and 3241 more

New issues flagged (845 issues)

S2933

Joust/ts/MetaData.ts:4

     2 | import {MetaDataType} from "./enums";
     3 | 
>    4 | export default class MetaData {
     5 | 	constructor(private _type: MetaDataType, private _data: number, private _entities?: Immutable.Set<number>) {
     6 | 		if (!this._entities) {

Joust/ts/components/LoadingScreen.tsx:8

     6 | }
     7 | 
>    8 | export default class LoadingScreen extends React.Component<LoadingScreenProps, void> {
     9 | 	private messages = [
    10 | 		// Cardgames

Joust/ts/components/Scrubber.tsx:39

    37 | }
    38 | 
>   39 | export default class Scrubber extends React.Component<ScrubberProps, ScrubberState> {
    40 | 
    41 | 	private static SPEEDS = [0.75, 1, 1.5, 2, 3, 4, 8];

Joust/ts/components/SetupWidget.tsx:29

    27 | }
    28 | 
>   29 | export default class SetupWidget extends React.Component<SetupWidgetProps, SetupWidgetState> {
    30 | 	private forceWebsocket: boolean;
    31 |

Joust/ts/components/Timeline.tsx:20

    18 | }
    19 | 
>   20 | export default class Timeline extends React.Component<TimelineProps, TimelineState> implements StreamScrubberInhibitor {
    21 | 	private ref: HTMLDivElement;
    22 | 	private mouseMove: (e) => void;

Joust/ts/components/game/EntityInPlay.tsx:7

     5 | import MetaData from "../../MetaData";
     6 | 
>    7 | abstract class EntityInPlay<P extends EntityInPlayProps> extends React.Component<P, EntityInPlayState> {
     8 | 
     9 | 	private baseClassName: string = '';

Joust/ts/components/game/visuals/CardArt.tsx:22

    20 | }
    21 | 
>   22 | class CardArt extends React.Component<CardArtProps, CardArtState> {
    23 | 
    24 | 	constructor(props: CardArtProps, context: any) {

Joust/ts/protocol/HSReplayDecoder.ts:33

    31 | }
    32 | 
>   33 | export default class HSReplayDecoder extends Stream.Transform implements CardOracle, MulliganOracle {
    34 | 
    35 | 	private sax: SAXStream;

Joust/ts/protocol/KettleEncoder.ts:8

     6 | import GameStateTracker from "../state/GameStateTracker";
     7 | 
>    8 | export default class KettleEncoder extends Stream.Readable implements InteractiveBackend {
     9 | 
    10 | 	private tracker: GameStateTracker;

Joust/ts/state/GameStateDescriptor.ts:5

     3 | import MetaData from "../MetaData";
     4 | 
>    5 | export default class GameStateDescriptor {
     6 | 
     7 | 	constructor(private _entityId: number, private _target: number, private _action: BlockType, private _metaData?: Immutable.Set<MetaData>) {

...and 835 more

📋 View full report

Code no longer flagged (3251)

S2486

S2933

✅ All blocking issues resolved.

_{Configure merge blocking}

gitar-bot · 2026-07-01T15:32:54Z

Code Review ✅ Approved 2 resolved / 2 findings

Groups S2933 reports by class while preserving member locations and aggregate quick fixes, incorporating an updated nyc ESM loader. Dependency cleanup resolved issues with extraneous Vue rule imports and decorator stability.

✅ 2 resolved

✅ Bug: Removed eslint-plugin-vue dependency still imported by Vue rules

📄 package.json:82
This commit removes the eslint-plugin-vue devDependency from package.json, but the package is still a hard dependency of the codebase. packages/analysis/src/jsts/rules/external/vue.ts:17 does import pluginVue from 'eslint-plugin-vue';, and that module backs the recently added rule S8951 (vue/no-mutating-props) via packages/analysis/src/jsts/rules/S8951/index.ts. It is also patched at build time in esbuild-common.mjs. With the dependency no longer declared, a clean npm install will not install eslint-plugin-vue, and the build/runtime import will fail (module not found), breaking the Vue rules and the analysis bundle.

This appears to be an unintended casualty of the "Revert unrelated changes" commit reverting a Renovate bump: rather than reverting the version bump, the entry was deleted entirely. Restore the eslint-plugin-vue entry in package.json (at the previously committed version) instead of removing it.

✅ Quality: Decorator silently degrades if upstream selector key changes

📄 packages/analysis/src/jsts/rules/S2933/decorator.ts:57-58 📄 packages/analysis/src/jsts/rules/S2933/decorator.ts:62-74 📄 tools/nyc-esm-loader.js:75-79
In decorate(), grouping depends on the upstream typescript-eslint/prefer-readonly rule registering its reporting listener under the exact selector key 'ClassDeclaration, ClassExpression:exit' (decorator.ts:58). If a future version of @typescript-eslint/eslint-plugin renames or splits this selector (e.g. into separate ClassDeclaration:exit / ClassExpression:exit handlers), classExitListener becomes undefined, callListener no-ops, and reportGroupedIssue is never invoked. The upstream reports would then flow straight through context.report (ungrouped), silently reverting to one-issue-per-member without any error — exactly the behavior this PR aims to change, and it would only be caught by the ruling tests.

This is inconsistent with the defensive assertSourceMapsApi() guard added in the same PR to nyc-esm-loader.js, which throws when the nyc SourceMaps API changes. Consider adding an analogous assertion so the breakage is loud rather than silent.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

francois-mora-sonarsource · 2026-07-03T10:22:28Z

Follow-up note on inline ESLint suppressions: this grouping decorator intercepts raw upstream context.report() calls and re-reports one grouped issue on the class. ESLint applies inline disable comments only after rule execution, so a member-level disable such as eslint-disable-next-line @typescript-eslint/prefer-readonly would not match the grouped class-level report.

I do not think this should block JS-1947: inline suppressions for this rule are expected to be uncommon, and the PR improves the default reporting experience substantially. Handling this properly also deserves a generic solution for decorators that relocate or aggregate reports, instead of special-casing S2933 or duplicating ESLint directive semantics.

Tracked as JS-2001: add a reusable metadata-driven guard so decorators can opt out of custom grouping/relocation when relevant inline ESLint disables are present, forwarding original reports and letting ESLint apply its native suppression logic.

erwan-leforestier-sonarsource · 2026-07-06T07:13:42Z

+    if (
+      classNode.range[0] <= index &&
+      index <= classNode.range[1] &&
+      (!owner || classNode.range[0] >= owner.range[0])
+    ) {


Worth adding a comment about what this condition is checking because it's not clear to me

erwan-leforestier-sonarsource · 2026-07-06T07:21:19Z

+    },
+    secondaryLocations,
+  );
+}


I saw that you tried to extract this reportGroupedIssue in a reusable helper in this commit: http://github.com/SonarSource/SonarJS/pull/7401/changes/bf16d3e62c8ba32c120dcbc04416bf004fa21efc
But it was then reverted. Why not having this in a separate helper ?

(cherry picked from commit b19fe6a)

🤖 Generated with GitHub Actions (cherry picked from commit cd96404)

Refactor the S2933 (prefer-readonly) grouping decorator and address review findings: - Reuse the shared interceptReport helper instead of hand-rolling context interception. - Route the fallback path through report() so forwarded issues stay encoded and decodable under hasSecondaries (was throwing in decode). - Delegate location/message resolution to location.ts helpers. - Extract the "group an upstream rule's reports into one issue with secondary locations" mechanism into helpers/decorators/grouper.ts; S2933 becomes a thin consumer. - Fail loudly when the decorated rule registers listeners but none under the configured selector (upstream selector rename), while treating a fully empty listener (sanitized/inactive rule) as a no-op. - Sort and drop overlapping combined fixes so a grouped quick fix can never crash file analysis, and pin the merged edit in the integration test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…constructor-grouping

sonarqube-next · 2026-07-08T15:14:39Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
90.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

gitar-bot · 2026-07-08T15:18:31Z

Code Review ✅ Approved 8 resolved / 8 findings

Groups S2933 reports by class and integrates an NYC ESM loader, resolving identified dependency issues and messaging inconsistencies. No open findings remain.

✅ 8 resolved

✅ Bug: Removed eslint-plugin-vue dependency still imported by Vue rules

📄 package.json:82
This commit removes the eslint-plugin-vue devDependency from package.json, but the package is still a hard dependency of the codebase. packages/analysis/src/jsts/rules/external/vue.ts:17 does import pluginVue from 'eslint-plugin-vue';, and that module backs the recently added rule S8951 (vue/no-mutating-props) via packages/analysis/src/jsts/rules/S8951/index.ts. It is also patched at build time in esbuild-common.mjs. With the dependency no longer declared, a clean npm install will not install eslint-plugin-vue, and the build/runtime import will fail (module not found), breaking the Vue rules and the analysis bundle.

This appears to be an unintended casualty of the "Revert unrelated changes" commit reverting a Renovate bump: rather than reverting the version bump, the entry was deleted entirely. Restore the eslint-plugin-vue entry in package.json (at the previously committed version) instead of removing it.

✅ Quality: Decorator silently degrades if upstream selector key changes

📄 packages/analysis/src/jsts/rules/S2933/decorator.ts:57-58 📄 packages/analysis/src/jsts/rules/S2933/decorator.ts:62-74 📄 tools/nyc-esm-loader.js:75-79
In decorate(), grouping depends on the upstream typescript-eslint/prefer-readonly rule registering its reporting listener under the exact selector key 'ClassDeclaration, ClassExpression:exit' (decorator.ts:58). If a future version of @typescript-eslint/eslint-plugin renames or splits this selector (e.g. into separate ClassDeclaration:exit / ClassExpression:exit handlers), classExitListener becomes undefined, callListener no-ops, and reportGroupedIssue is never invoked. The upstream reports would then flow straight through context.report (ungrouped), silently reverting to one-issue-per-member without any error — exactly the behavior this PR aims to change, and it would only be caught by the ruling tests.

This is inconsistent with the defensive assertSourceMapsApi() guard added in the same PR to nyc-esm-loader.js, which throws when the nyc SourceMaps API changes. Consider adding an analogous assertion so the breakage is loud rather than silent.

✅ Edge Case: Overlapping upstream fixes are silently dropped from combined fix

📄 packages/analysis/src/jsts/rules/S2933/decorator.ts:281-295
removeOverlappingFixes (decorator.ts:317-332) sorts all per-member fixes by start index and drops any fix whose range[0] falls before the previous fix's end. For prefer-readonly this is safe in practice because each member's autofix is an independent, non-overlapping readonly insertion. However, the behavior is silent: if upstream ever emits two edits that genuinely overlap (e.g. a broader replacement plus a nested insertion), one of them is discarded with no diagnostic, producing a partial quick fix that could leave source in an unexpected state while still reporting the aggregate issue as fixable. Consider at minimum a comment documenting the assumption, or logging/skipping the combined fix entirely when a real conflict is detected so the grouped issue does not advertise an incomplete fix.

✅ Quality: convert-source-map imported as an undeclared (phantom) dependency

📄 tools/nyc-esm-loader.js:22
tools/nyc-esm-loader.js now does import convertSourceMap from 'convert-source-map', but convert-source-map is not declared as a direct dependency in any package.json (grep of package.json files returns nothing; it only appears in package-lock.json as a transitive dependency of nyc/@babel/core). This works today only because npm hoists the transitive copy to the root node_modules. If the hoisted version is deduplicated differently, removed, or the layout changes (e.g. a stricter installer), the import will fail at loader startup and break coverage-instrumented ESM test runs. Since this is a test tooling loader the impact is limited to CI/coverage, hence minor, but the dependency should be declared explicitly.

✅ Quality: convert-source-map uses caret range vs. repo's exact pins

📄 package.json:74
All devDependencies in this package.json use exact pinned versions (e.g. @istanbuljs/load-nyc-config": "1.1.0", test-exclude": "6.0.0", @istanbuljs/schema": "0.1.3"), which matches the repository convention of fully reproducible installs. The newly added convert-source-map entry uses a caret range ^2.0.0 instead of an exact version, allowing minor/patch upgrades that could drift from the lockfile-verified version and diverge from the surrounding style. Pin it to an exact version for consistency (e.g. 2.0.0, matching the resolved version in package-lock.json).

...and 3 more resolved from earlier reviews

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

francois-mora-sonarsource requested a review from a team June 30, 2026 13:44

github-actions Bot mentioned this pull request Jun 30, 2026

Update ruling results for PR #7401 #7403

Closed

This comment has been minimized.

Sign in to view

francois-mora-sonarsource changed the title ~~[codex] JS-1947 Group S2933 issues by class~~ JS-1947 Group S2933 issues by class Jun 30, 2026

gitar-bot Bot reviewed Jun 30, 2026

View reviewed changes

Comment thread packages/analysis/src/jsts/rules/S2933/decorator.ts Outdated

francois-mora-sonarsource marked this pull request as ready for review June 30, 2026 18:53

gitar-bot Bot reviewed Jun 30, 2026

View reviewed changes

Comment thread package.json

This comment was marked as resolved.

Sign in to view

francois-mora-sonarsource force-pushed the codex/JS-1947-s2933-constructor-grouping branch from e119f04 to 8b8f4c8 Compare June 30, 2026 19:14

francois-mora-sonarsource marked this pull request as draft July 1, 2026 07:38

francois-mora-sonarsource removed the request for review from a team July 1, 2026 07:39

gitar-bot Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread packages/analysis/src/jsts/rules/S2933/decorator.ts Outdated

francois-mora-sonarsource force-pushed the codex/JS-1947-s2933-constructor-grouping branch from 6571fc5 to 6a077ac Compare July 2, 2026 12:48

gitar-bot Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread tools/nyc-esm-loader.js Outdated

gitar-bot Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread package.json Outdated

francois-mora-sonarsource marked this pull request as ready for review July 3, 2026 08:55

francois-mora-sonarsource requested a review from a team July 3, 2026 08:55

erwan-leforestier-sonarsource approved these changes Jul 6, 2026

View reviewed changes

gitar-bot Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread packages/analysis/src/jsts/rules/S2933/reporting.ts Outdated

francois-mora-sonarsource force-pushed the codex/JS-1947-s2933-constructor-grouping branch from 19e668c to 9eabe4d Compare July 7, 2026 07:15

erwan-leforestier-sonarsource and others added 5 commits July 7, 2026 11:58

Fix JS coverage handoff for Sonar analysis

c3b712a

(cherry picked from commit b19fe6a)

Fail fast on missing JS coverage reports

b580d14

JS-1947 Group S2933 issues by class

ddce902

Update ruling results

84f05dd

🤖 Generated with GitHub Actions (cherry picked from commit cd96404)

Revert unrelated changes

c8adc37

francois-mora-sonarsource and others added 15 commits July 7, 2026 15:07

Revert unrelated changes

9bb6248

JS-1947 Remove S2933 listener selector dependency

a574587

JS-1947 Remove S2933 listener selector dependency

c76997a

JS-1947 Fix S2933 decorator nesting warning

7929f14

docs: improve VS Code SonarJS patch workflow

652056c

Fix phantom convert-source-map dependency

da60fb4

Fix NYC loader dependency declarations

8b544da

Pin convert-source-map version

0b57a27

Revert unrelated NYC and docs changes

8056004

Restore NYC loader from master

1d9e0d8

Add helper for nested S2933 class ownership

9deb77e

Extract S2933 report grouping helper

d6d8294

Polish S2933 grouped readonly wording

b60cd20

Document S2933 grouped readonly wording

bf66c89

francois-mora-sonarsource force-pushed the codex/JS-1947-s2933-constructor-grouping branch from 9eabe4d to bf66c89 Compare July 7, 2026 13:07

Merge remote-tracking branch 'origin/master' into codex/JS-1947-s2933-…

ddad605

…constructor-grouping

gitar-bot Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread packages/analysis/src/jsts/rules/S2933/reporting.ts Outdated

gitar-bot Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread packages/analysis/src/jsts/rules/S2933/reporting.ts

francois-mora-sonarsource added 2 commits July 8, 2026 16:48

JS-1947 Anchor anonymous S2933 classes on keyword

efcf4f8

JS-1947 Cast anonymous class token lookup for CI

20d284c

francois-mora-sonarsource merged commit 4afe1b9 into master Jul 8, 2026
43 checks passed

francois-mora-sonarsource deleted the codex/JS-1947-s2933-constructor-grouping branch July 8, 2026 15:17

Uh oh!

Conversation

francois-mora-sonarsource commented Jun 30, 2026

Summary

Notes

Validation

Uh oh!

hashicorp-vault-sonar-prod Bot commented Jun 30, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ruling Report

Code no longer flagged (3251 issues)

S2486

New issues flagged (845 issues)

S2933

Code no longer flagged (3251)

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

gitar-bot Bot commented Jul 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

francois-mora-sonarsource commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erwan-leforestier-sonarsource Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

erwan-leforestier-sonarsource Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqube-next Bot commented Jul 8, 2026

Quality Gate passed

Uh oh!

Uh oh!

gitar-bot Bot commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hashicorp-vault-sonar-prod Bot commented Jun 30, 2026 •

edited by atlassian Bot

Loading

github-actions Bot commented Jun 30, 2026 •

edited

Loading

francois-mora-sonarsource commented Jul 3, 2026 •

edited

Loading