JS-1462 Fix FP on S6582 when optional chaining would introduce unsafe undefined#6637
Draft
sonar-nigel[bot] wants to merge 8 commits intomasterfrom
Conversation
added 4 commits
March 19, 2026 09:05
Tests cover the scenario where expressions like `arr && arr.length` are flagged as replaceable with optional chaining, but the replacement would change the type from `T | null` to `T | undefined`, breaking type safety in typed contexts (return types or variable declarations excluding undefined). The tests verify that: - The rule suppresses reports when checker.getContextualType() returns a type that excludes undefined (e.g., `number | null`, `string | null`) - The rule still reports in boolean contexts (if-conditions) where no contextual type is imposed and the replacement is type-safe - The rule still reports when the contextual type includes undefined - An upstream sentinel confirms the upstream prefer-optional-chain rule still raises on these patterns (validating the interceptor is needed) Relates to JS-1462
The rule incorrectly flagged expressions like `a && a.length` and `!a || !a.length` as replaceable with optional chaining even when the replacement would introduce a type-unsafe `undefined`. For example, `const x: number | null = a && a.length` — replacing with `a?.length` changes the type to `number | undefined`, which is not assignable to `number | null`. The fix adds a report interceptor that uses TypeScript's `checker.getContextualType()` to detect whether optional chaining would be type-safe at the usage site. Reports are suppressed only when a contextual type exists and excludes `undefined` (meaning the replacement would break assignability). In boolean/void contexts like `if` conditions, no contextual type is imposed, so reports pass through correctly. Implementation follows the approved proposal from the Jira ticket. Relates to JS-1462
Analyzed 2518 ruling entries for S6582 (prefer-optional-chain). The implementation correctly suppresses 119 entries where optional chaining would introduce type-unsafe `undefined` (e.g. `return file && file.snapshot` in a method returning `ISnapshot | null`). No implementation changes were needed as the contextual-type check aligns with the approved proposal and the "prefer not raising" principle. Added two test cases inspired by real-world ruling patterns: one for a class method with a named-interface return type (from the TypeScript compiler source), and one for an object-literal field with an explicit `string | null` type (from the TypeScript server source). Both demonstrate that replacing `a && a.b` with `a?.b` would change the result type from `T | null` to `T | undefined`, breaking type safety.
Contributor
added 4 commits
March 19, 2026 10:12
Remove redundant `return` statement (S3626) at the end of the `if (undefinedAssignable)` block in rule.ts. The return was the last statement before the callback function ended, making it a no-op jump. No functional change — the report suppression logic is identical.
Add unit tests for S6582 defensive guard fallback paths. Tests exercise the !node and !tsNode branches in the interceptor callback by using Proxy-based context wrappers to control getNodeByRangeIndex and esTreeNodeToTSNodeMap.get return values. This improves code coverage to meet the quality gate threshold.
s6582-multi-condition-logic-with--and--cannot-use-optional-chaining-sonnet
s6582-multi-condition-logic-with--and--cannot-use-optional-chaining-sonnet
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a false positive in rule S6582 (prefer-optional-chain) where expressions like
a && a.lengthor!a || !a.lengthwere incorrectly flagged as replaceable with optional chaining, even when doing so would change the result type fromT | nulltoT | undefined, breaking type safety.What changed
checker.getContextualType()to determine whether optional chaining would be type-safe at the usage site.undefined(e.g., a variable declared asnumber | nullor a method returningISnapshot | null).if-conditions) where no contextual type is imposed and the replacement is safe.Why
Replacing
const x: number | null = a && a.lengthwithconst x: number | null = a?.lengthchanges the inferred type tonumber | undefined, which is not assignable tonumber | null. The rule should not suggest a transformation that silently breaks the type contract.Testing
undefined(return types, variable declarations).undefined.Relates to JS-1462