JS-1731 Fix S6551 typeof guard false positives by francois-mora-sonarsource · Pull Request #7404 · SonarSource/SonarJS

francois-mora-sonarsource · 2026-06-30T14:08:52Z

Summary

Fixes JS-1731 by adding a narrow S6551 decorator suppression for identifiers used in implicit string contexts when an enclosing if consequent is guarded by typeof checks proving the same bound variable is primitive.

The suppression handles primitive equality checks, reversed comparisons, typeof value !== "object", conjunctions, and disjunctions. It resolves identifiers through ESLint scope so same-name shadowing does not suppress incorrectly, and it rejects suppression after reassignment before the report site.

Why

@typescript-eslint/no-base-to-string can still report after guards such as typeof value !== "object" because TypeScript may expose a broad narrowed type like {} / {} | undefined, even though runtime object stringification cannot occur in the guarded branch.

Tests

npx tsx --tsconfig packages/tsconfig.test.json --test --test-reporter=spec --test-reporter-destination=stdout packages/analysis/src/jsts/rules/S6551/cb.test.ts
npm run bridge:compile
git diff --check
npx prettier --check packages/analysis/src/jsts/rules/S6551/decorator.ts packages/analysis/src/jsts/rules/S6551/cb.fixture.ts

Summary by Gitar

CI/CD pipeline updates:
- Modified build.yml to export and validate JavaScript coverage reports via actions/upload-artifact and actions/download-artifact.
- Removed redundant coverage caching logic in favor of direct artifact propagation between CI jobs.
Refactored S6551 rule logic:
- Extracted false positive suppression logic into a dedicated false-positives/ module to improve maintainability and support complex conditional checks.

_{This will update automatically on new commits.}

hashicorp-vault-sonar-prod · 2026-06-30T14:09:06Z

JS-1731

github-actions · 2026-06-30T14:17:12Z

Ruling Report

Code no longer flagged (3253 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 3243 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 (3253)

S2486

S2933

gitar-bot · 2026-07-03T12:58:57Z

Code Review 👍 Approved with suggestions 1 resolved / 2 findings

Suppresses false positives in S6551 by accounting for typeof guard scopes, resolving the previous issue with reassignments. Ensure consistency regarding closure and hoisted writes, as the recent removal of isFunctionNode leads to unintended recursion into nested functions.

💡 Edge Case: Closure/hoisted writes handled inconsistently after isFunctionNode removal

📄 packages/analysis/src/jsts/rules/S6551/decorator.ts:396-410

Removing the isFunctionNode early-return from hasWriteBefore (line 397) makes it recurse into nested function bodies. This has two asymmetric consequences:

A write to the guarded variable inside a nested function/closure that is declared before the usage but never actually executes before it (e.g. const cb = () => { value = obj; }; \${value}`;`) is now treated as an invalidating write, so a legitimate suppression is skipped and the false positive this PR targets is left unsuppressed (over-conservative).
Conversely, a write inside a hoisted function declaration placed textually after the usage but called before it is still missed, because hasWriteBefore bails out on node.range[0] >= end before recursing into that function body — so the guard is incorrectly suppressed (false negative).

Both are narrow edge cases and the direction of #1 is safe (errs toward reporting), but the behavior around nested-function writes is now range-based rather than execution-based, which is easy to trip over. Consider only counting writes that are lexically in the same function scope as the usage (ignoring nested function bodies entirely) so the semantics are consistent, or add fixtures covering these two shapes to pin the intended behavior.

✅ 1 resolved

✅ Edge Case: hasWriteBefore misses reassignments in nested functions/after use

📄 packages/analysis/src/jsts/rules/S6551/decorator.ts:391-405
hasWriteBefore only detects reassignments that appear lexically before the report offset (node.range[0]) and it stops descending at function boundaries (isFunctionNode(node) returns false). This means two reassignment patterns are missed, causing the guard to be treated as still valid and the S6551 report to be suppressed incorrectly:

Reassignment via a synchronously-invoked callback, e.g.
if (typeof value !== 'object') {
  [1].forEach(() => { value = {}; });
  return `${value}`; // value may now be an object, but suppressed
}
The write value = {} lives inside a nested arrow function, which the recursion prunes, so it is never counted.

Reassignment that occurs lexically after the report but re-executes on a subsequent loop iteration, e.g.
if (typeof value !== 'object') {
  for (...) {
    console.log(`${value}`); // reported first
    value = getObject();      // reassigns after, loops back
  }
}
Because the write starts after end, it is not detected and later iterations stringify an object.

Both are edge cases (the fixtures cover the common straight-line reassignment case correctly). If tighter soundness is desired, consider treating the presence of any nested-function reassignment of the tracked variable, or any reassignment anywhere in the guarded consequent (not just before the report), as invalidating the guard. Otherwise this is an acceptable, conservative-leaning approximation worth documenting.

🤖 Prompt for agents

Code Review: Suppresses false positives in S6551 by accounting for typeof guard scopes, resolving the previous issue with reassignments. Ensure consistency regarding closure and hoisted writes, as the recent removal of isFunctionNode leads to unintended recursion into nested functions.

1. 💡 Edge Case: Closure/hoisted writes handled inconsistently after isFunctionNode removal
   Files: packages/analysis/src/jsts/rules/S6551/decorator.ts:396-410

   Removing the `isFunctionNode` early-return from `hasWriteBefore` (line 397) makes it recurse into nested function bodies. This has two asymmetric consequences:
   
   1. A write to the guarded variable inside a nested function/closure that is declared before the usage but never actually executes before it (e.g. `const cb = () => { value = obj; }; \`${value}\`;`) is now treated as an invalidating write, so a legitimate suppression is skipped and the false positive this PR targets is left unsuppressed (over-conservative).
   
   2. Conversely, a write inside a hoisted function declaration placed textually *after* the usage but called before it is still missed, because `hasWriteBefore` bails out on `node.range[0] >= end` before recursing into that function body — so the guard is incorrectly suppressed (false negative).
   
   Both are narrow edge cases and the direction of #1 is safe (errs toward reporting), but the behavior around nested-function writes is now range-based rather than execution-based, which is easy to trip over. Consider only counting writes that are lexically in the same function scope as the usage (ignoring nested function bodies entirely) so the semantics are consistent, or add fixtures covering these two shapes to pin the intended behavior.

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}

(cherry picked from commit b19fe6a)

Treat destructuring assignment targets (`[value] = arr`, `({ value } = obj)`) as invalidating writes so a proven-primitive value reassigned to an object inside a typeof-guarded branch keeps reporting instead of being suppressed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

francois-mora-sonarsource · 2026-07-08T12:52:46Z

Review feedback

Reviewed the diff — logic checks out overall (scope resolution, shadowing, reassignment/destructuring/loop invalidation, and the conjunction/disjunction semantics in conditionMakesBranchSafeToStringify all behave correctly against the fixture cases). Two minor, non-blocking points:

Likely dead code — isImplicitlyStringified in guarded-by-typeof-check.ts climbs through TSAsExpression/TSTypeAssertion/TSNonNullExpression parents to "transparently walk through TypeScript-only wrappers." But the caller (isGuardedByTypeofCheckFalsePositive) already requires node.type === 'Identifier' before calling in. Per the upstream no-base-to-string rule, when a template-literal/+ operand is wrapped (e.g. `${value as any}` or `${value!}`), the reported node is the wrapper itself, not the inner identifier — so by the time this loop runs, node.parent being a TS wrapper shouldn't occur from a real report. Worth a quick RuleTester check with a wrapped-identifier fixture case; if confirmed dead, the branch (and its comment) could be dropped for clarity.
Completeness gap (safe direction) — the same function only recognizes TemplateLiteral and BinaryExpression (+) as implicit-stringification contexts, not AssignmentExpression with +=. So a guarded case like if (typeof value !== 'object') { rendered += value; } (no template literal) stays unsuppressed even though it's just as safe as the tested rendered += `${value}` case. This only means a missed suppression, never an incorrect one, so it's safe but incomplete relative to the PR's goal — could be a good follow-up if worth the extra branch.

Both are minor/non-blocking — no correctness bugs found, and test coverage (fixture Compliant/Noncompliant cases + upstream sentinel tests) is solid.

sonarqube-next · 2026-07-08T16:04:48Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

asya-vorobeva

LGTM 🥇

gitar-bot · 2026-07-09T19:43:23Z

Code Review ✅ Approved 3 resolved / 3 findings

Refines S6551 false-positive suppression by adding support for guarded type checks and destructuring reassignments, resolving previous issues with hoisted writes and inconsistent closure handling.

✅ 3 resolved

✅ Edge Case: hasWriteBefore misses reassignments in nested functions/after use

📄 packages/analysis/src/jsts/rules/S6551/decorator.ts:391-405
hasWriteBefore only detects reassignments that appear lexically before the report offset (node.range[0]) and it stops descending at function boundaries (isFunctionNode(node) returns false). This means two reassignment patterns are missed, causing the guard to be treated as still valid and the S6551 report to be suppressed incorrectly:

Reassignment via a synchronously-invoked callback, e.g.
if (typeof value !== 'object') {
  [1].forEach(() => { value = {}; });
  return `${value}`; // value may now be an object, but suppressed
}
The write value = {} lives inside a nested arrow function, which the recursion prunes, so it is never counted.

Reassignment that occurs lexically after the report but re-executes on a subsequent loop iteration, e.g.
if (typeof value !== 'object') {
  for (...) {
    console.log(`${value}`); // reported first
    value = getObject();      // reassigns after, loops back
  }
}
Because the write starts after end, it is not detected and later iterations stringify an object.

Both are edge cases (the fixtures cover the common straight-line reassignment case correctly). If tighter soundness is desired, consider treating the presence of any nested-function reassignment of the tracked variable, or any reassignment anywhere in the guarded consequent (not just before the report), as invalidating the guard. Otherwise this is an acceptable, conservative-leaning approximation worth documenting.

✅ Edge Case: Closure/hoisted writes handled inconsistently after isFunctionNode removal

📄 packages/analysis/src/jsts/rules/S6551/decorator.ts:396-410
Removing the isFunctionNode early-return from hasWriteBefore (line 397) makes it recurse into nested function bodies. This has two asymmetric consequences:

A write to the guarded variable inside a nested function/closure that is declared before the usage but never actually executes before it (e.g. const cb = () => { value = obj; }; \${value}`;`) is now treated as an invalidating write, so a legitimate suppression is skipped and the false positive this PR targets is left unsuppressed (over-conservative).

Conversely, a write inside a hoisted function declaration placed textually after the usage but called before it is still missed, because hasWriteBefore bails out on node.range[0] >= end before recursing into that function body — so the guard is incorrectly suppressed (false negative).

Both are narrow edge cases and the direction of #1 is safe (errs toward reporting), but the behavior around nested-function writes is now range-based rather than execution-based, which is easy to trip over. Consider only counting writes that are lexically in the same function scope as the usage (ignoring nested function bodies entirely) so the semantics are consistent, or add fixtures covering these two shapes to pin the intended behavior.

✅ Edge Case: typeof guard ignores reassignments inside the if condition

📄 packages/analysis/src/jsts/rules/S6551/false-positives/guarded-by-typeof-check.ts:76-88 📄 packages/analysis/src/jsts/rules/S6551/false-positives/guarded-by-typeof-check.ts:139-143 📄 packages/analysis/src/jsts/rules/S6551/false-positives/guarded-by-typeof-check.ts:213-223
isGuardedByTypeofCheckFalsePositive proves a branch is safe via conditionMakesBranchSafeToStringify (for a && consequent it only needs .some conjunct to be a valid typeof guard), then rejects suppression only when hasInvalidatingWrite finds a write in branch.node (the consequent/alternate body) or a loop ancestor. Writes that happen inside the if test itself are never inspected. So a condition like if (typeof value === 'string' && (value = getObject())) { return \${value}`; }is incorrectly suppressed even thoughvalue` is an object at the report site — a false negative for S6551.

This is an edge case (assignment inside a condition is uncommon), so severity is minor. To close the gap, also check for invalidating writes in the guard condition after the proving typeof operand, e.g. scan parent.test for writes to variable up to usage.range[0].

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}

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

Update ruling results for PR #7404 #7405

Closed

This comment has been minimized.

Sign in to view

gitar-bot Bot reviewed Jul 3, 2026

View reviewed changes

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

gitar-bot Bot reviewed Jul 3, 2026

View reviewed changes

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

francois-mora-sonarsource force-pushed the fix/S6551-JS-1731-fp branch 2 times, most recently from 862b2cd to 12d1db1 Compare July 3, 2026 15:42

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

erwan-leforestier-sonarsource added 2 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

gitar-bot Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread packages/analysis/src/jsts/rules/S6551/false-positives/guarded-by-typeof-check.ts

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

JS-1731 Fix S6551 typeof guard false positives

27f4819

JS-1731 Update vitest ruling expectations

6b14576

JS-1731 Tighten S6551 guarded write invalidation

229bc31

JS-1731 Fix S6551 loop helper typing

acb33b8

JS-1731 Keep S6551 write scan local

4180592

Add S6551 typeof sentinel coverage

ce621e2

Split rule implementation in false positive submodules

35008af

Extend to symetric if/else cases

b68ef21

Separate rule logic from static checks

f68f164

Fix S6551 false-positive helper typing

8764efd

Handle S6551 writes inside typeof guards

1e55751

francois-mora-sonarsource force-pushed the fix/S6551-JS-1731-fp branch from e08ae90 to c10802b Compare July 7, 2026 13:51

Merge remote-tracking branch 'origin/master' into fix/S6551-JS-1731-fp

f0ad9d0

JS-1731 Support guarded plus assignments

59860a1

asya-vorobeva self-requested a review July 9, 2026 14:09

asya-vorobeva approved these changes Jul 9, 2026

View reviewed changes

francois-mora-sonarsource merged commit e76d467 into master Jul 9, 2026
45 checks passed

francois-mora-sonarsource deleted the fix/S6551-JS-1731-fp branch July 9, 2026 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JS-1731 Fix S6551 typeof guard false positives#7404

JS-1731 Fix S6551 typeof guard false positives#7404
francois-mora-sonarsource merged 16 commits into
masterfrom
fix/S6551-JS-1731-fp

francois-mora-sonarsource commented Jun 30, 2026 •

edited by gitar-bot Bot

Loading

Uh oh!

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

edited by atlassian Bot

Loading

Uh oh!

github-actions Bot commented Jun 30, 2026 •

edited

Loading

Code no longer flagged (3253)

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

gitar-bot Bot commented Jul 3, 2026

Uh oh!

Uh oh!

francois-mora-sonarsource commented Jul 8, 2026

Uh oh!

sonarqube-next Bot commented Jul 8, 2026

Uh oh!

asya-vorobeva left a comment

Uh oh!

Uh oh!

gitar-bot Bot commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

francois-mora-sonarsource commented Jun 30, 2026 • edited by gitar-bot Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Tests

Summary by Gitar

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 (3253 issues)

S2486

New issues flagged (845 issues)

S2933

Code no longer flagged (3253)

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

gitar-bot Bot commented Jul 3, 2026

Uh oh!

Uh oh!

francois-mora-sonarsource commented Jul 8, 2026

Review feedback

Uh oh!

sonarqube-next Bot commented Jul 8, 2026

Quality Gate passed

Uh oh!

asya-vorobeva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gitar-bot Bot commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

francois-mora-sonarsource commented Jun 30, 2026 •

edited by gitar-bot Bot

Loading

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

edited by atlassian Bot

Loading

github-actions Bot commented Jun 30, 2026 •

edited

Loading