JS-1663 Fix S2094 false positive for classes with property initialization by sonar-nigel[bot] · Pull Request #6991 · SonarSource/SonarJS

sonar-nigel · 2026-05-07T16:45:24Z

Relates to JS-1663

S2094 no longer reports onlyConstructor for data-container and value-object classes whose constructors initialize instance state with direct this.* assignments. The decorator now walks constructor control flow recursively while staying bounded to constructor-level initialization.

Suppresses reports for direct this.* assignments, including assignments nested in loops, branches, and try/catch.
Keeps callback and nested-class assignments noncompliant by stopping traversal at function and class boundaries.
Leaves all non-onlyConstructor reports from the upstream rule unchanged.

Tests cover the scenario where classes with constructors that initialize instance properties via this.* assignments are incorrectly flagged as useless. The tests verify that the decorator suppresses onlyConstructor reports when this.* assignments exist at any nesting level in the constructor body (including inside for, for...in, while, and if constructs), while still reporting when assignments are only inside nested function or arrow function bodies. Relates to JS-1663

S2094 incorrectly flagged classes as useless when the constructor initializes instance properties via this.* assignments. These are legitimate data-container and value-object patterns. Add a decorator that intercepts 'onlyConstructor' reports and suppresses them when the constructor body contains at least one this.* assignment. The check walks the constructor body recursively through control-flow constructs (loops, if/else, try/catch) but stops at nested function/arrow-function boundaries, so only direct constructor-level state initialization is recognized. Follows the approved implementation guidelines from JS-1663. Relates to JS-1663

The ruling analysis confirmed that the S2094 decorator implementation is correct with no mismatches between shouldRaise and actualRaise across all 22 entries analyzed. The 18 true-positive entries (empty classes with no this.* constructor assignments) correctly continue to raise, including empty exception classes, JSDoc placeholder classes, and mock classes in real-world projects. The 4 suppressed entries are correctly identified as data-container classes: three _User and WithPrivateField classes in watchtower.js and expressionist.js (with direct this.* assignments), and one class in custom-jsts/S3854.js. The recursive constructor body walk with function-boundary stopping works as intended. No implementation changes or test additions were needed: the decorator's walkForThisAssignment function correctly handles all patterns found in the ruling, and the existing unit tests already cover every edge case discovered (direct assignments, loop bodies, conditionals, and function boundaries).

Ticket: JS-1663

Remove two unnecessary type assertions (typescript:S4325) in decorator.ts: - Line 58: `return parent as ClassDeclaration | ClassExpression` → `return parent`. After the `parent.type === 'ClassDeclaration' || 'ClassExpression'` guard, TypeScript already narrows the type; the explicit cast was redundant. - Lines 73-75: Replaced the boolean predicate in `find()` with a type predicate `(member): member is MethodDefinition =>`, so TypeScript infers `MethodDefinition | undefined` directly and the `as MethodDefinition | undefined` cast is no longer needed.

sonar-nigel · 2026-05-07T16:45:26Z

Rule Profile

Field	Value
Rule	`S2094` - Classes should not be empty
Severity / type	Minor CODE_SMELL
Sonar Way	Yes
Scope	Main

class DeferredInit {
  constructor() {
    setTimeout(() => {
      this.value = 1;
    }, 0);
  }
}

False Positive Pattern

The flagged classes are not empty in practice: their constructors initialize instance properties, so the class is carrying state as a data container or value object. S2094 previously treated these classes as having "only a constructor" even when the constructor established the instance state directly.

goog.labs.mock.timeout.TimeoutMode = class TimeoutMode {
  constructor(duration) {
    this.duration = duration;
  }
};

class PropertyDictionary {
  constructor(source) {
    for (const key in source) {
      this[key] = source[key];
    }
  }
}

False Negative Risk

The suppression is intentionally bounded. It only applies to onlyConstructor reports and only when the constructor body contains a direct this.* assignment reachable without crossing a function or nested class boundary. This means callback-based assignments and inner-class field initializers still report, even if they syntactically contain this.*.

class Outer {
  constructor() {
    class Inner {
      field = (this.value = 1);
    }
  }
}

Fix Summary

The S2094 decorator keeps the upstream no-extraneous-class rule enabled, but filters onlyConstructor reports for classes whose constructor performs direct instance-state initialization.

Replaces the previous shallow constructor-body check with a recursive walk using existing AST helpers.
Traverses constructor control flow such as loops, branches, and try/catch.
Stops at function and class boundaries so callbacks and nested classes do not suppress reports for the outer class.

github-actions · 2026-05-07T16:54:45Z

Ruling Report

Code no longer flagged (4 issues)

S2094

custom-jsts/S3854.js:350

   348 | 
   349 | 
>  350 | class Foo {
   351 |     constructor(name, options = {}) {
   352 |         this.name = name;

expressionist.js/test/parser.spec.js:864

   862 | }
   863 | 
>  864 | class WithPrivateField {
   865 |   constructor(){
   866 |     this.publicField = 4;

watchtower.js/test/dirtychecking.spec.js:576

   574 | });
   575 | 
>  576 | class _User {
   577 |   constructor(first, last, age) {
   578 |     this.first = first;

watchtower.js/test/observer.spec.js:132

   130 | });
   131 | 
>  132 | class _User {
   133 |   constructor(name) {
   134 |     this.name = name;

New issues flagged (24 issues)

S105

hs-template/apps/admin/src/components/legacy-component.tsx:9

     7 | 
     8 | interface LegacyComponentProps {
>    9 | 	userId: string;
    10 | 	label: string;
    11 | }

S106

hs-template/apps/admin/src/components/legacy-component.tsx:50

    48 | 		nextState: LegacyComponentState,
    49 | 	) {
>   50 | 		console.log("about to update", nextProps.userId, nextState.count);
    51 | 	}
    52 |

S1441

hs-template/apps/admin/src/components/legacy-component.tsx:2

     1 | // Importing PropTypes from "react" is considered deprecated starting at React version 15.5.
>    2 | import React, { PropTypes } from "react";
     3 | // `render`, `hydrate` and `unmountComponentAtNode` from "react-dom" are considered deprecated starting at React version 18.0.
     4 | import ReactDOM, { render, hydrate, unmountComponentAtNode } from "react-dom";

hs-template/apps/admin/src/components/legacy-component.tsx:4

     2 | import React, { PropTypes } from "react";
     3 | // `render`, `hydrate` and `unmountComponentAtNode` from "react-dom" are considered deprecated starting at React version 18.0.
>    4 | import ReactDOM, { render, hydrate, unmountComponentAtNode } from "react-dom";
     5 | // `renderToNodeStream` from "react-dom/server" is considered deprecated starting at React version 18.0.
     6 | import ReactDOMServer, { renderToNodeStream } from "react-dom/server";

hs-template/apps/admin/src/components/legacy-component.tsx:6

     4 | import ReactDOM, { render, hydrate, unmountComponentAtNode } from "react-dom";
     5 | // `renderToNodeStream` from "react-dom/server" is considered deprecated starting at React version 18.0.
>    6 | import ReactDOMServer, { renderToNodeStream } from "react-dom/server";
     7 | 
     8 | interface LegacyComponentProps {

hs-template/apps/admin/src/components/legacy-component.tsx:50

    48 | 		nextState: LegacyComponentState,
    49 | 	) {
>   50 | 		console.log("about to update", nextProps.userId, nextState.count);
    51 | 	}
    52 |

S1444

hs-template/apps/admin/src/components/legacy-component.tsx:23

    21 | > {
    22 | 	// Using `PropTypes` from the "react" package is considered deprecated starting at React version 15.5.
>   23 | 	static propTypes = {
    24 | 		userId: PropTypes.string,
    25 | 		label: PropTypes.string,

S1537

hs-template/apps/admin/src/components/legacy-component.tsx:25

    23 | 	static propTypes = {
    24 | 		userId: PropTypes.string,
>   25 | 		label: PropTypes.string,
    26 | 	};
    27 |

hs-template/apps/admin/src/components/legacy-component.tsx:48

    46 | 	componentWillUpdate(
    47 | 		nextProps: LegacyComponentProps,
>   48 | 		nextState: LegacyComponentState,
    49 | 	) {
    50 | 		console.log("about to update", nextProps.userId, nextState.count);

S6791

hs-template/apps/admin/src/components/legacy-component.tsx:34

    32 | 
    33 | 	// `componentWillMount` is considered deprecated starting at React version 16.9.
>   34 | 	componentWillMount() {
    35 | 		this.setState({ data: `loading-${this.props.userId}` });
    36 | 	}

...and 14 more

📋 View full report

Code no longer flagged (4)

S2094

New issues flagged (24)

S105

hs-template/apps/admin/src/components/legacy-component.tsx:9

S106

hs-template/apps/admin/src/components/legacy-component.tsx:50

S1441

S1444

hs-template/apps/admin/src/components/legacy-component.tsx:23

S1537

S6791

S6957

sonar-review-alpha · 2026-05-07T17:05:14Z

Summary

This PR fixes false positives in the S2094 rule (no extraneous classes) by adding a targeted decorator that suppresses onlyConstructor warnings when a class constructor directly initializes instance state with this.* assignments.

The fix recognizes that classes serving as data containers with constructor-level initialization are legitimate, and suppresses warnings for patterns like:

Direct assignments: this.name = value
Assignments in control flow: this.field = x inside loops, conditionals, and try/catch blocks

The decorator stops at function boundaries (nested functions, arrow functions, deferred calls, nested classes), ensuring callbacks and deferred initialization still raise appropriate warnings.

Real false positives in expressionist.js and watchtower.js test suites are resolved.

What reviewers should know

Where to start:

Review the core logic in decorator.ts — the walkForThisAssignment function is the heart of the fix. It recursively traverses the constructor body looking for this.* property assignments while respecting function and class boundaries.
Check unit.test.ts for the comprehensive test coverage — the valid/invalid cases show exactly what the decorator suppresses and what it doesn't.

Key design decisions:

Uses a decorator pattern (targeted interception) rather than enabling allowConstructorOnly globally, keeping the fix narrow and maintainable.
Stops at ALL function boundaries (including arrow functions in callbacks), not just named functions — this prevents false negatives where deferred initialization (e.g., in setTimeout) would incorrectly suppress the warning.
The sentinel test in unit.test.ts verifies the upstream ESLint rule still reports these patterns — if that test starts passing without code changes, the decorator can be safely removed.

Watch for:

The test expectations in its/ruling/ — these reflect real codebases where false positives were occurring. The removals confirm the fix is working.
The README.md change moves S2094 from "compatible but not shipped" to "custom rules" — metadata reflects it's now a decorated rule, not a raw external rule.

Generate Walkthrough
Generate Diagram

🗣️ Give feedback

Remove unnecessary type assertions (S4325) and rename a variable that shadows the built-in `constructor` property (S2424): - Line 58: removed redundant `as estree.ClassDeclaration | estree.ClassExpression` cast in `getClassNode`; TypeScript accepts the narrowed intersection type directly. - Line 73: renamed local variable `constructor` to `ctor` to avoid shadowing the built-in `constructor` property (S2424). - Line 82: removed `as estree.FunctionExpression` cast on `ctor.value`; the estree typings already type `MethodDefinition.value` as `FunctionExpression`. - Line 104: removed `as estree.AssignmentExpression` cast; TypeScript narrows `node` to `AssignmentExpression` after the early-return type guard. - Line 107: removed `as estree.MemberExpression` cast; TypeScript narrows `left` to `MemberExpression` via the `&&` discriminant check on `left.type`.

Arrow functions preserve the enclosing `this` binding, so `this.x = y` inside a constructor arrow callback initializes the class instance just like a direct assignment or a loop body. Stopping recursion at ArrowFunctionExpression caused forEach-style data containers to still be flagged while for...of equivalents were suppressed. Change the stop condition to only block FunctionExpression and FunctionDeclaration. Add a comment explaining the distinction, move the arrow-function test case to valid, and add it to the sentinel. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…rc/jsts/rules/S2094/unit.test.ts Comment: The PR description and decorator comment both mention `try/catch` as a supported control-flow construct, but there is no test case for it. Given that the implementation relies on `childrenOf` + `visitorKeys` without any explicit `TryStatement`/`CatchClause` handling, a test verifying suppression works through `try/catch/finally` would lock this in. ```js { // Compliant: this.* inside try/catch code: `class SafeContainer { constructor(source) { try { this.value = source.value; } catch (e) { this.value = null; } } }`, options, }, ``` - [ ] Mark as noise

sonar-nigel · 2026-05-07T17:36:16Z

Both issues were addressed in this run:

Arrow function asymmetry — fixed in commit 4fc9a6e59: walkForThisAssignment now stops only at FunctionExpression/FunctionDeclaration and traverses into arrow functions, since they preserve the constructor's this binding. The decision is explained in the comment on line 86–89 of decorator.ts. The sentinel and a compliant test case for the forEach-arrow pattern were also added.
Missing try/catch test — added in commit 84f70aee4: unit.test.ts now includes a compliant test case for this.* inside a try/catch/finally block (line 143–154).

francois-mora-sonarsource · 2026-05-08T17:45:14Z

ArrowFunctionExpression should not be part of the supported suppression scope here.

Right now walkForThisAssignment traverses into arrow callbacks, which suppresses cases like:

class DeferredInit {
  constructor() {
    setTimeout(() => {
      this.value = 1;
    }, 0);
  }
}

That class does not initialize instance state during construction, so it should still be reported as onlyConstructor.

Please make ArrowFunctionExpression a stop boundary again, keep the support limited to direct constructor control-flow (if, loops, try/catch, etc.), remove the arrow-callback valid test, and add an invalid regression test for the deferred-callback case.

Comment: `ArrowFunctionExpression` should not be part of the supported suppression scope here. Right now `walkForThisAssignment` traverses into arrow callbacks, which suppresses cases like: ```js class DeferredInit { constructor() { setTimeout(() => { this.value = 1; }, 0); } } ``` That class does not initialize instance state during construction, so it should still be reported as `onlyConstructor`. Please make `ArrowFunctionExpression` a stop boundary again, keep the support limited to direct constructor control-flow (`if`, loops, `try/catch`, etc.), remove the arrow-callback valid test, and add an invalid regression test for the deferred-callback case.

…rc/jsts/rules/S2094/unit.test.ts Comment: The label "deferred arrow callback" implies that deferral is what makes this noncompliant, but `isFunctionNode` stops at **all** `ArrowFunctionExpression` nodes — deferred or not. A non-deferred arrow like `items.forEach(item => { this[item.key] = item.value; })` is also noncompliant under the current code, yet there is no invalid test case for it. This gap matters because a future refactor that reverts arrow functions to being traversed (as in the previous commit) would silently pass all current tests — the only thing locking in the old behaviour (the `DataStore` sentinel entry) was removed in this commit. Please either add an `invalid` case for a synchronous arrow callback (e.g., `forEach(item => { this[item] = ... })`), or rename the comment to clarify that both deferred and immediate arrow callbacks are stopped. - [ ] Mark as noise

…rc/jsts/rules/S2094/decorator.ts Comment: The comment "assignments in callbacks do not count" is ambiguous — it could be read as applying only to passed-in callbacks, not to all arrow functions. Since `isFunctionNode` stops at `ArrowFunctionExpression` too (see `FUNCTION_NODES` in `helpers/ast.ts`), the comment should be explicit about this. Otherwise the next reader may wonder why `isFunctionNode` was preferred over the narrower `FunctionExpression | FunctionDeclaration` check that was here before. ```suggestion // Stop at all function boundaries (FunctionDeclaration, FunctionExpression, and // ArrowFunctionExpression): `this.*` assignments nested inside any callback — even // arrow callbacks whose `this` is bound to the constructor — are not considered direct // constructor-level state initialization. if (isFunctionNode(node)) { ``` - [ ] Mark as noise

francois-mora-sonarsource · 2026-05-08T19:46:33Z

The current suppression walk is still a bit too broad because it only stops at function boundaries.

For example, this outer class is currently treated as compliant even though the only this.* assignment belongs to an inner class field initializer, not to the outer constructor's instance state:

class Outer {
  constructor() {
    class Inner {
      field = (this.value = 1);
    }
  }
}

walkForThisAssignment() descends into the nested ClassDeclaration / ClassBody / PropertyDefinition, sees this.value = 1, and suppresses the outer onlyConstructor report. That is outside the PR contract of "constructor-level instance initialization".

Can we stop the traversal at nested class boundaries as well, and add an invalid regression test for this shape?

Comment: The current suppression walk is still a bit too broad because it only stops at function boundaries. For example, this outer class is currently treated as compliant even though the only `this.*` assignment belongs to an inner class field initializer, not to the outer constructor's instance state: ```js class Outer { constructor() { class Inner { field = (this.value = 1); } } } ``` `walkForThisAssignment()` descends into the nested `ClassDeclaration` / `ClassBody` / `PropertyDefinition`, sees `this.value = 1`, and suppresses the outer `onlyConstructor` report. That is outside the PR contract of "constructor-level instance initialization". Can we stop the traversal at nested class boundaries as well, and add an `invalid` regression test for this shape?

sonar-review-alpha

LGTM! ✅

🗣️ Give feedback

sonarqube-next · 2026-05-08T20:21:16Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

Vibe Bot added 5 commits May 7, 2026 16:06

Sync expected ruling files after rule-specific refinement

d404fff

Ticket: JS-1663

sonar-nigel Bot marked this pull request as ready for review May 7, 2026 17:04

sonar-nigel Bot requested a review from francois-mora-sonarsource May 7, 2026 17:04

Conversation

sonar-nigel Bot commented May 7, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonar-nigel Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rule Profile

False Positive Pattern

False Negative Risk

Fix Summary

Uh oh!

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

Uh oh!

Ruling Report

Code no longer flagged (4 issues)

S2094

New issues flagged (24 issues)

S105

S106

S1441

S1444

S1537

S6791

Code no longer flagged (4)

New issues flagged (24)

Uh oh!

sonar-review-alpha Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What reviewers should know

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as outdated.

Uh oh!

sonar-nigel Bot commented May 7, 2026

Uh oh!

This comment was marked as outdated.

Uh oh!

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

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as outdated.

Uh oh!

francois-mora-sonarsource commented May 8, 2026

Uh oh!

sonar-review-alpha Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqube-next Bot commented May 8, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

sonar-nigel Bot commented May 7, 2026 •

edited by atlassian Bot

Loading

sonar-nigel Bot commented May 7, 2026 •

edited

Loading

github-actions Bot commented May 7, 2026 •

edited

Loading

sonar-review-alpha Bot commented May 7, 2026 •

edited

Loading

francois-mora-sonarsource commented May 8, 2026 •

edited

Loading