JS-1462 Fix S6582 over-suppression by suppressing only known false positives (#6830)

Author: francois-mora-sonarsource

its/ruling/src/test/expected/TypeScript/javascript-S6582.json

@@ -183,6 +183,7 @@
 43934,
 44204,
 44570,
+44761,
 45278,
 46088,
 46102,

its/ruling/src/test/expected/desktop/typescript-S6582.json

@@ -256,7 +256,8 @@
 314
 ],
 "desktop:app/src/ui/toolbar/branch-dropdown.tsx": [
-329
+329,
+330
 ],
 "desktop:script/changelog/parser.ts": [
 21,

its/ruling/src/test/expected/knockout/javascript-S6582.json

@@ -22,6 +22,9 @@
 "knockout:src/binding/defaultBindings/value.js": [
 38
 ],
+"knockout:src/binding/editDetection/arrayToDomNodeChildren.js": [
+89
+],
 "knockout:src/binding/expressionRewriting.js": [
 119,
 184

its/ruling/src/test/expected/paper.js/javascript-S6582.json

@@ -60,7 +60,8 @@
 71,
 110,
 111,
-140
+140,
+145
 ],
 "paper.js:src/item/Item.js": [
 921,
@@ -157,6 +158,7 @@
 200,
 310,
 418,
+442,
 919,
 946,
 959,

packages/analysis/src/jsts/rules/S6582/rule.ts

@@ -28,25 +28,6 @@ import * as meta from './generated-meta.js';
  */
 const preferOptionalChainRule = tsEslintRules['prefer-optional-chain'];
 
-/**
- * Matches negated access guards that stay boolean after optional chaining.
- *
- * Pseudo code:
- * if (!value || !value.property) {
- *   // `value?.property` remains wrapped by `!`
- * }
- */
-function isNegatedOptionalChainGuard(node: Rule.Node) {
-  return (
-    node.type === 'LogicalExpression' &&
-    node.operator === '||' &&
-    node.left.type === 'UnaryExpression' &&
-    node.left.operator === '!' &&
-    node.right.type === 'UnaryExpression' &&
-    node.right.operator === '!'
-  );
-}
-
 /**
  * Resolves the AST node associated with a report descriptor.
  *
@@ -108,29 +89,6 @@ function allowsUndefined(type: ts.Type): boolean {
   );
 }
 
-/**
- * Returns the type imposed by the surrounding context on the reported expression.
- *
- * Pseudo code:
- * let result: string | null;
- * result = value && value.property;
- *
- * Here, `node` is `value && value.property`, and its contextual type is `string | null`
- * because that is the type expected by the assignment target `result`.
- */
-function getContextualTypeOfNode(
-  services: Rule.RuleContext['sourceCode']['parserServices'],
-  checker: ts.TypeChecker,
-  node: Rule.Node,
-): ts.Type | null {
-  const tsNode = services.esTreeNodeToTSNodeMap.get(node);
-  if (!tsNode) {
-    return null;
-  }
-
-  return checker.getContextualType(tsNode as ts.Expression) ?? null;
-}
-
 /**
  * Sanitized rule 'prefer-optional-chain' from TypeScript ESLint.
  *
@@ -171,40 +129,163 @@ export const rule: Rule.RuleModule = {
     }
 
     const checker = services.program.getTypeChecker();
-    return interceptReport(preferOptionalChainRule, (ctx, descriptor) => {
-      const node = findReportNode(ctx, descriptor);
-      if (!node) {
-        return;
+
+    /**
+     * Returns the type imposed by the surrounding context on the reported expression.
+     *
+     * Pseudo code:
+     * let result: string | null;
+     * result = value && value.property;
+     *
+     * Here, `node` is `value && value.property`, and its contextual type is `string | null`
+     * because that is the type expected by the assignment target `result`.
+     */
+    function getContextualTypeOfNode(node: Rule.Node): ts.Type | null {
+      const tsNode = services.esTreeNodeToTSNodeMap.get(node);
+      if (!tsNode) {
+        return null;
       }
 
-      // Negation patterns (!a || !a.prop): the ! operator always returns boolean,
-      // so optional chaining (!a?.prop) is always type-safe regardless of context.
-      // Both sides must be negated — !a || (comparison) is not a negation pattern
-      // and should fall through to the contextual type check.
-      if (isNegatedOptionalChainGuard(node)) {
-        ctx.report(descriptor);
-        return;
+      return checker.getContextualType(tsNode as ts.Expression) ?? null;
+    }
+
+    function getTypeOfNode(node: Rule.Node): ts.Type | null {
+      const tsNode = services.esTreeNodeToTSNodeMap.get(node);
+      if (!tsNode) {
+        return null;
       }
 
-      // Comparison patterns (e.g. !a || a.prop !== b): the comparison operator always
-      // returns boolean, so the optional-chain rewrite (a?.prop !== b) is also boolean —
-      // no undefined leaks into the surrounding type regardless of context.
-      if (
+      return checker.getTypeAtLocation(tsNode);
+    }
+
+    function getContextualTypeSubject(node: Rule.Node) {
+      let current = node;
+      while (
+        current.parent?.type === 'LogicalExpression' &&
+        (current.parent.left === current || current.parent.right === current)
+      ) {
+        current = current.parent;
+      }
+      return current;
+    }
+
+    function hasTypeUnsafeContextualType(node: Rule.Node) {
+      const contextualType = getContextualTypeOfNode(getContextualTypeSubject(node));
+      return contextualType != null && !allowsUndefined(contextualType);
+    }
+
+    function matchesFunctionReturnFalsePositive(node: Rule.Node) {
+      return (
+        node.type === 'LogicalExpression' &&
+        node.operator === '&&' &&
+        node.parent?.type === 'ReturnStatement' &&
+        node.right.type !== 'BinaryExpression' &&
+        hasTypeUnsafeContextualType(node)
+      );
+    }
+
+    function matchesTypedVariableInitializerFalsePositive(node: Rule.Node) {
+      return (
+        node.type === 'LogicalExpression' &&
+        node.operator === '&&' &&
+        node.parent?.type === 'VariableDeclarator' &&
+        node.parent.init === node &&
+        node.right.type !== 'BinaryExpression' &&
+        hasTypeUnsafeContextualType(node)
+      );
+    }
+
+    function matchesObjectLiteralPropertyFalsePositive(node: Rule.Node) {
+      return (
         node.type === 'LogicalExpression' &&
-        node.right.type === 'BinaryExpression' &&
-        ['==', '!=', '===', '!==', '<', '>', '<=', '>=', 'instanceof', 'in'].includes(
-          node.right.operator,
-        )
+        node.operator === '&&' &&
+        node.parent?.type === 'Property' &&
+        node.parent.value === node &&
+        node.right.type !== 'BinaryExpression' &&
+        hasTypeUnsafeContextualType(node)
+      );
+    }
+
+    function matchesCallArgumentFalsePositive(node: Rule.Node) {
+      const subject = getContextualTypeSubject(node);
+      const parent = subject.parent;
+      if (
+        node.type !== 'LogicalExpression' ||
+        node.operator !== '&&' ||
+        node.right.type === 'BinaryExpression' ||
+        parent?.type !== 'CallExpression' ||
+        !parent.arguments.includes(subject as never)
+      ) {
+        return false;
+      }
+
+      const contextualType = getContextualTypeOfNode(subject);
+      return contextualType != null && !allowsUndefined(contextualType);
+    }
+
+    function matchesAssignmentFalsePositive(node: Rule.Node) {
+      if (
+        node.type !== 'LogicalExpression' ||
+        node.operator !== '&&' ||
+        node.right.type === 'BinaryExpression' ||
+        node.parent?.type !== 'AssignmentExpression' ||
+        node.parent.right !== node ||
+        node.parent.operator !== '='
       ) {
-        ctx.report(descriptor);
+        return false;
+      }
+
+      const parent = node.parent;
+      if (parent.left.type !== 'Identifier' && parent.left.type !== 'MemberExpression') {
+        // Keep the matcher narrowly scoped to ordinary typed assignment targets.
+        // Broader target coverage can be added once we have concrete FP examples.
+        return false;
+      }
+
+      const targetType = getTypeOfNode(parent.left as Rule.Node);
+      return targetType != null && !allowsUndefined(targetType);
+    }
+
+    /**
+     * Returns true when the upstream report is a known false positive that should be suppressed.
+     *
+     * We suppress only when the optional-chain rewrite would leak a type-unsafe `undefined`
+     * into the surrounding context. All other cases are reported by default.
+     */
+    function isKnownFalsePositive(node: Rule.Node): boolean {
+      if (matchesFunctionReturnFalsePositive(node)) {
+        return true;
+      }
+
+      if (matchesTypedVariableInitializerFalsePositive(node)) {
+        return true;
+      }
+
+      if (matchesObjectLiteralPropertyFalsePositive(node)) {
+        return true;
+      }
+
+      if (matchesCallArgumentFalsePositive(node)) {
+        return true;
+      }
+
+      if (matchesAssignmentFalsePositive(node)) {
+        return true;
+      }
+
+      return false;
+    }
+
+    return interceptReport(preferOptionalChainRule, (ctx, descriptor) => {
+      const node = findReportNode(ctx, descriptor);
+      if (!node) {
+        return;
+      }
+
+      if (isKnownFalsePositive(node)) {
         return;
       }
 
-      // Left-operand-of-|| or left-operand-of-?? pattern:
-      // When (a && a.prop) is the left operand of || or ??, the enclosing operator
-      // absorbs the undefined introduced by optional chaining, making the rewrite
-      // type-safe. E.g. (repo && repo.name) || fallback → repo?.name || fallback
-      // preserves the overall type because || and ?? provide a fallback for undefined.
       const parent = node.parent;
       if (
         parent?.type === 'LogicalExpression' &&
@@ -225,18 +306,7 @@ export const rule: Rule.RuleModule = {
         return;
       }
 
-      const contextualType = getContextualTypeOfNode(services, checker, node);
-      if (!contextualType) {
-        // No contextual type (e.g. if/while boolean context) — replacement is safe
-        ctx.report(descriptor);
-        return;
-      }
-
-      if (allowsUndefined(contextualType)) {
-        // undefined is assignable to the contextual type — replacement is type-safe
-        ctx.report(descriptor);
-      }
-      // undefined is NOT assignable to the contextual type — suppress the report
+      ctx.report(descriptor);
     }).create(context);
   },
 };

packages/analysis/src/jsts/rules/S6582/unit.test.ts

@@ -267,6 +267,16 @@ describe('S6582', () => {
           code: `interface Commit { parentSHAs: string[] } function hasMultipleCommits(commit: Commit | undefined): boolean { return commit !== undefined && commit.parentSHAs.some(x => x.length > 0) }`,
           filename: path.join(import.meta.dirname, 'fixtures/index.ts'),
         },
+        {
+          // FP: assignment after declaration to an identifier typed as 'string | null' excludes undefined
+          code: `interface Item { name: string } function f(item: Item | null) { let value: string | null = null; value = item && item.name; }`,
+          filename: path.join(import.meta.dirname, 'fixtures/index.ts'),
+        },
+        {
+          // FP: assignment after declaration to a typed property excludes undefined
+          code: `interface Item { name: string } function f(item: Item | null) { const holder: { value: string | null } = { value: null }; holder.value = item && item.name; }`,
+          filename: path.join(import.meta.dirname, 'fixtures/index.ts'),
+        },
       ],
       invalid: [
         {