JS-2407 Implement rule S9383: Promises should not be left unhandled - #7896
martin-strecker-sonarsource wants to merge 10 commits into
Conversation
Wraps typescript-eslint's no-floating-promises, following the S6544 wrapping pattern. Suggestions (add await/void) are stripped for now since quickfix support isn't implemented yet.
New expected ruling output for S9383 across the JS/TS ruling corpus, generated with npm run ruling / ruling-sync. Spot-checked the two highest-volume projects (desktop: 360 issues, eigen: 257 issues) against source - both are genuine fire-and-forget async calls (unawaited async methods in constructors, unawaited calls in useEffect/JSX handlers), not a false-positive pattern.
Pins rspec.sha to SonarSource/rspec#8128 (still unmerged) so CI's prepare_rspec_rule_data job fetches the real RSPEC metadata for S9383 instead of failing with ENOENT on resources/rule-data/javascript/S9383.json, per docs/BUILD.md's documented process for a SonarJS PR that depends on an in-flight RSPEC change. Remove this file once SonarSource/rspec#8128 merges to the RSPEC default branch.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Stop stripping the "add await"/"add void" suggestions from no-floating-promises. Following the established pattern for decorated wraps of suggestion-producing typescript-eslint rules (e.g. S6572, S6582): pass the upstream suggest payload through unmodified and test the exact fix output via the [[qfN]]/fix@/edit@ comment-based DSL, instead of defensively discarding it. RSPEC now declares quickfix=partial (not all report shapes carry a suggestion - the promise-array case never does), updated on both the in-review RSPEC PR and the local mirror.
| export const rule: Rule.RuleModule = { | ||
| meta: generateMeta(meta, { ...noFloatingPromisesRule.meta }), | ||
| create(context: Rule.RuleContext) { | ||
| return noFloatingPromisesRule.create(context); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
💡 Quality: PR description still claims suggestions are stripped and quickfix unknown
The PR summary states "Suggestions (Add await operator. / Add void operator to ignore.) are stripped for now — quickfix support isn't implemented yet (RSPEC quickfix: unknown)", but this commit removes the interceptor that stripped them and flips the RSPEC mirror to partial, so suggestions are now passed through and asserted in cb.fixture.ts. Update the description (and the linked RSPEC PR's quickfix value) so reviewers and the RSPEC sync are not driven by the stale claim.
Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
description updated
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…overed The previous rspec.sha pin was a commit based on plain master, but CI's unpinned default (and the rspec-maven-plugin's rule-data generation) resolves against dogfood-automerge, which has diverged from master. Pinning to master-only meant deploy-rule-data couldn't find resources/rule-data/javascript/S7503.json (unrelated rule) and prepare_rspec_rule_data failed with ENOENT. Fix: merge the RSPEC S9383 branch onto the current dogfood-automerge tip (SonarSource/rspec@2d0508b, via a throwaway Martin/dogfood-pin-S9383 branch so PR #8128's own history isn't rewritten) and pin rspec.sha to that commit instead. Verified locally with `npm run rspec:refresh` - resolves cleanly now, no ENOENT. Also mirrors the RSPEC-side quickfix=covered fix (see previous commit) in the local resource file, confirmed by the live refresh pulling the same value from the pinned commit.
✅ Code review updated (blocking issues remain unresolved).
|
S9383 ruling validation — 791 findings
Confirmed false positives — 2 distinct root causes, both pre-existing upstream limitations (not introduced by our wrapping)Root cause A —
Root cause B — array-mutation method misread as a floating promise array (2 instances)
Borderline cases (10) — technically true positives per the rule's literal contract, but low real-world signal
Non-issue worth a one-line mention
Bottom line: zero false positives traceable to our wrapper — both FP categories are pre-existing upstream 🤖 Generated with Claude Code |
|
Re: #7896 (comment) Checked both points against current state rather than the diff snapshot the bot analyzed: Point 2 (description contradicts implementation) — already stale. The PR description was updated two commits ago to reflect that suggestions are passed through (not stripped) and RSPEC declares Point 1 (no automated guard against The actual gap: I checked this repo's branch-protection rulesets via the API ( That's a repo-level branch-protection/ruleset setting, not something a code change in this PR's diff can fix — happy to raise it separately with whoever administers branch protection for this repo, since it'd protect every future |
Upstream's isValidRejectionHandler() treats any as "not a function" since it structurally has zero call signatures, even though it's callable at runtime. typescript-eslint/typescript-eslint#12848 was closed working-as-intended, so fix it locally: interceptReport() re-checks the handler's type and drops the report when it's exactly any, covering the 4 real-world FPs from ruling.
Code Review 👍 Approved with suggestions 4 resolved / 5 findingsImplements S9383 (unhandled promises) as a decorated wrap of typescript-eslint's 💡 Quality: PR description still claims suggestions are stripped and quickfix unknown📄 packages/analysis/src/jsts/rules/S9383/rule.ts:26-31 📄 packages/analysis/src/jsts/rules/S9383/cb.fixture.ts:5-9 📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json:25 The PR summary states "Suggestions ( ✅ 4 resolved✅ Bug: Local S9383 RSPEC mirror is wiped by rspec:refresh, breaking CI
✅ Bug: Root rspec.sha pin overrides the dogfood-automerge RSPEC branch
✅ Bug: quickfix 'partial' + hasSuggestions fails validate-quickfix build step
✅ Quality: Committed temporary pin has no automated guard against reaching master
🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Update (2026-09-10): typescript-eslint/typescript-eslint#12848 (filed above for root cause A) was closed by the maintainer ( |
Drop optional-chaining and void-unwrapping - none of the 4 real-world FPs from the ruling review took those shapes, and the decorator shouldn't guess at forms we haven't actually seen.
Removes the 4 false positives fixed in the decorator: ace:114,139, p5.js:93, eigen:82 - the exact any-typed .catch() handlers from the ruling review.
guillemsarda
left a comment
guillemsarda
left a comment
There was a problem hiding this comment.
LGTM. Worth adding the two tests.
| } | ||
|
|
||
| // `any` has no call signatures though it's callable at runtime; upstream closed this | ||
| // as working-as-intended (typescript-eslint/typescript-eslint#12848), so fix it here. |
There was a problem hiding this comment.
Thanks for the Issue!
| // edit@qf6 {{ await (async () =>}} | ||
| await fetchData(); | ||
| })(); | ||
| } |
There was a problem hiding this comment.
Worth adding two regression tests for the Optional chaining and Computed member access cases.
Optional chaining:
declare function fetchData(): Promise<void>;
function handle(next: any) {
fetchData()?.catch(next);
}
declare const maybePromise: Promise<void> | undefined;
function handle(next: any) {
maybePromise?.then(undefined, next);
}Computed member access:
declare function fetchData(): Promise<void>;
function handle(next: any) {
fetchData()['catch'](next);
}
function handle(next: any) {
fetchData()[`then`](undefined, next);
}| @@ -0,0 +1,99 @@ | |||
| /* | |||
There was a problem hiding this comment.
(Optional) This rule puts the decoration logic in rule.ts, but the established convention across the codebase (~95 decorated rules) is to split it into a dedicated decorator.ts, with index.ts doing export const rule = decorate(getBaseRule(...)). Only a few rules (S6544, S6572, S6582) currently deviate from that and inline everything in rule.ts.
Could we rename rule.ts → decorator.ts here (and adjust index.ts accordingly) to follow the majority pattern, rather than growing the outlier group to 4? Makes it easier for reviewers/future maintainers to find the FP-suppression/quick-fix logic in a consistent place across rules.
Follows the codebase's majority pattern for decorated typescript-eslint rules (index.ts calling decorate() from decorator.ts) instead of inlining everything in rule.ts, per review feedback.
…cess Locks in that rejection handlers reached only through optional chaining (?.catch/?.then) or computed member access (['catch']/[`then`]) fall outside findRejectionHandler's direct-call match and are still reported, even when any-typed, per review feedback.
CI failed: The Maven build failed during rule data deployment due to a missing rule definition file (S9381.json) encountered by the rspec refresh script.Overview1 unique build failure pattern was detected across 3 failed jobs. The failure is directly related to the PR changes involving RSPEC rule integration. FailuresMissing Rule Data File (confidence: high)
SummaryCode Review 👍 Approved with suggestions 4 resolved / 5 findingsImplements S9383 (unhandled promises) as a decorated wrap of typescript-eslint's 💡 Quality: PR description still claims suggestions are stripped and quickfix unknown📄 packages/analysis/src/jsts/rules/S9383/rule.ts:26-31 📄 packages/analysis/src/jsts/rules/S9383/cb.fixture.ts:5-9 📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json:25 The PR summary states "Suggestions ( ✅ 4 resolved✅ Bug: Local S9383 RSPEC mirror is wiped by rspec:refresh, breaking CI
✅ Bug: Root rspec.sha pin overrides the dogfood-automerge RSPEC branch
✅ Bug: quickfix 'partial' + hasSuggestions fails validate-quickfix build step
✅ Quality: Committed temporary pin has no automated guard against reaching master
🤖 Prompt for agentsReview coverageFunctional validation No results Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request: Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
Part of JS-2407
Summary
no-floating-promisesthe same way S6544 wrapsno-misused-promises.ignoreVoid: true,checkThenables: false,ignoreIIFE: false,allowForKnownSafeCalls/allowForKnownSafePromisesempty).quickfix: coveredto match this codebase'svalidate-quickfix.ts, which only acceptscoveredas the pass state forhasSuggestions=true(notpartial, even though that's also a documented RSPEC value).isValidRejectionHandler()misclassifies anany-typed rejection handler as "not a function", sinceanystructurally has zero call signatures despite being callable at runtime. Filed typescript-eslint/typescript-eslint#12848 with a proposed upstream fix — closed by the maintainer as working-as-intended (deliberate FP/FN tradeoff on their side, not a bug).S9383/rule.tsnow intercepts the twofloatingUselessRejectionHandler(Void)messageIds and suppresses the report locally when the handler's resolved type is exactlyany, covering all 4 real-world occurrences from the ruling review (see follow-up comment below for details).S9383.json) is included so the rule builds and rulings pass before that PR merges; it should be superseded automatically once the RSPEC sync job picks up the merged rule.rspec.shais temporarily pinned, perdocs/BUILD.md#baseline-ci-mismatches/docs/DEV.md, so CI'sprepare_rspec_rule_datajob fetches the real S9383 metadata instead of failing (it isn't on the RSPEC default branch yet). The pin points at a merge commit combining the currentdogfood-automergetip (the actual default RSPEC branch this tooling resolves against, notmaster) with the S9383 RSPEC branch, pushed to a throwawayMartin/dogfood-pin-S9383branch on the rspec repo so PR #8128's own history stays clean. An earlier master-based pin caused CI to fail withENOENTon an unrelated rule's data file (S7503.json) because it was missing whateverdogfood-automergehas thatmasterdoesn't yet. Must be removed before merging to master, and re-pinned again ifdogfood-automergemoves further before this merges.Ruling
desktop(360 issues/76 files): unawaitedasyncconditionalVersion(...)calls inside DB constructors (Dexie migration setup) — real fire-and-forget async work.eigen(257 issues/186 files): spread ~1/file, e.g. unawaited async calls insideuseEffectand unawaitednavigate(...)in ternaries — genuine floating promises, not a detection bug.npm run rulingis green (59/59) afterruling-sync; re-verified green after the suggestion pass-through change (locations/messages unaffected).Test plan
npx tsx --test packages/analysis/src/jsts/rules/S9383/cb.test.tspasses (12 comment-based scenarios: floating statement, awaited, returned,.catch(),.then()with/without rejection handler,void-ignored, floating promise array vs.Promise.all(), floating async IIFE,any-typed rejection handler (not reported), non-function rejection handler (still reported)), including exact quick-fix output assertions ([[qfN]]/fix@/edit@) for the scenarios that carry suggestions.npm run bbfbuilds clean.mvn install -DskipTestsbuilds clean (validatedvalidate-quickfixand RSPEC refresh against the repinned commit).npm run rulinggreen after baseline sync.rspec.shapin before merging to master.🤖 Generated with Claude Code