JS-1307 Fix FP in S3516 for functions with intentional invariant non-literal returns (#6511)

Co-authored-by: Vibe Bot <vibe-bot@sonarsource.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Francois Mora <francois.mora@sonarsource.com>

Author: 4 people

its/ruling/src/test/expected/eigen/typescript-S3516.json

@@ -26,10 +26,17 @@ import { generateMeta } from '../helpers/generate-meta.js';
 import { getMainFunctionTokenLocation, report, toSecondaryLocation } from '../helpers/location.js';
 import * as meta from './generated-meta.js';
 
+interface BranchFrame {
+  hasSideEffect: boolean;
+  hasReturn: boolean;
+}
+
 interface FunctionContext {
   codePath: Rule.CodePath;
   containsReturnWithoutValue: boolean;
   returnStatements: estree.ReturnStatement[];
+  branchStack: BranchFrame[];
+  hasSideEffectOnlyBranch: boolean;
 }
 
 interface SingleWriteVariable {
@@ -59,6 +66,12 @@ export const rule: Rule.RuleModule = {
         returnStatement => returnStatement.argument as estree.Node,
       );
       if (areAllSameValue(returnedValues, context.sourceCode.getScope(node))) {
+        // Only suppress when the returned value is a non-literal (e.g., a variable used for chaining).
+        // Functions returning literals (false, null, 0, etc.) have no chaining rationale and are always flagged.
+        const firstValue = getLiteralValue(returnedValues[0], context.sourceCode.getScope(node));
+        if (firstValue === undefined && functionContext.hasSideEffectOnlyBranch) {
+          return;
+        }
         report(
           context,
           {
@@ -75,12 +88,45 @@ export const rule: Rule.RuleModule = {
       }
     }
 
+    function pushBranchFrame() {
+      functionContextStack.at(-1)?.branchStack.push({ hasSideEffect: false, hasReturn: false });
+    }
+
+    function popBranchFrame() {
+      const ctx = functionContextStack.at(-1);
+      if (ctx) {
+        const frame = ctx.branchStack.pop();
+        if (frame?.hasSideEffect && !frame.hasReturn) {
+          ctx.hasSideEffectOnlyBranch = true;
+        }
+      }
+    }
+
+    function recordSideEffect(node: estree.CallExpression | estree.AssignmentExpression) {
+      const ctx = functionContextStack.at(-1);
+      if (!ctx) {
+        return;
+      }
+      const frame = ctx.branchStack.at(-1);
+      if (frame) {
+        frame.hasSideEffect = true;
+      }
+      // Also handle bare statement branches where ExpressionStatement IS the branch body
+      // (e.g. `if (x) doSomething()` — no BlockStatement is pushed for such a branch)
+      const exprStmt = (node as TSESTree.Node).parent as TSESTree.Node | undefined;
+      if (exprStmt && isBranchBody(exprStmt)) {
+        ctx.hasSideEffectOnlyBranch = true;
+      }
+    }
+
     return {
       onCodePathStart(codePath: Rule.CodePath) {
         functionContextStack.push({
           codePath,
           containsReturnWithoutValue: false,
           returnStatements: [],
+          branchStack: [],
+          hasSideEffectOnlyBranch: false,
         });
         codePathSegments.push(currentCodePathSegments);
         currentCodePathSegments = [];
@@ -102,8 +148,34 @@ export const rule: Rule.RuleModule = {
           currentContext.containsReturnWithoutValue =
             currentContext.containsReturnWithoutValue || !returnStatement.argument;
           currentContext.returnStatements.push(returnStatement);
+          const frame = currentContext.branchStack.at(-1);
+          if (frame) {
+            frame.hasReturn = true;
+          }
         }
       },
+      BlockStatement(node: estree.Node) {
+        if (isBranchBody(node as TSESTree.Node)) {
+          pushBranchFrame();
+        }
+      },
+      'BlockStatement:exit'(node: estree.Node) {
+        if (isBranchBody(node as TSESTree.Node)) {
+          popBranchFrame();
+        }
+      },
+      SwitchCase() {
+        pushBranchFrame();
+      },
+      'SwitchCase:exit'() {
+        popBranchFrame();
+      },
+      'ExpressionStatement > CallExpression'(node: estree.CallExpression) {
+        recordSideEffect(node);
+      },
+      'ExpressionStatement > AssignmentExpression'(node: estree.AssignmentExpression) {
+        recordSideEffect(node);
+      },
       'FunctionDeclaration:exit': checkOnFunctionExit,
       'FunctionExpression:exit': checkOnFunctionExit,
       'ArrowFunctionExpression:exit': checkOnFunctionExit,
@@ -215,3 +287,28 @@ function evaluateUnaryLiteralExpression(operator: string, innerReturnedValue: Li
       return undefined;
   }
 }
+
+/**
+ * Returns true if the node is a direct branch body of a conditional or loop statement —
+ * i.e., the consequent/alternate of an IfStatement or the body of a loop.
+ * Used to push and pop branch frames on the stack when entering and exiting branches.
+ */
+function isBranchBody(node: TSESTree.Node): boolean {
+  const { parent } = node;
+  if (!parent) {
+    return false;
+  }
+  if (parent.type === 'IfStatement') {
+    return node === parent.consequent || node === parent.alternate;
+  }
+  if (
+    parent.type === 'WhileStatement' ||
+    parent.type === 'DoWhileStatement' ||
+    parent.type === 'ForStatement' ||
+    parent.type === 'ForInStatement' ||
+    parent.type === 'ForOfStatement'
+  ) {
+    return node === parent.body;
+  }
+  return false;
+}

packages/jsts/src/rules/S3516/rule.ts

@@ -205,7 +205,7 @@ describe('S3516', () => {
         }
         
         var arrowWithExpressionBody = p => p ? 1 : 1;
-        
+
         function sameSymbolicValueSameConstraint(p) { // FN - not using SE
           var num = foo() - bar();
           var num2 = num;
@@ -216,6 +216,94 @@ describe('S3516', () => {
           }
         }`,
         },
+        {
+          // chaining pattern: while-loop mutates obj in-place, invariant return enables chaining
+          code: `
+function deepFreezeExample<T>(obj: T): T {
+  if (!obj || typeof obj !== 'object') {
+    return obj;
+  }
+  const stack: any[] = [obj];
+  while (stack.length > 0) {
+    const current = stack.shift();
+    Object.freeze(current);
+    for (const key of Object.keys(current)) {
+      const val = current[key];
+      if (val && typeof val === 'object' && !Object.isFrozen(val)) {
+        stack.push(val);
+      }
+    }
+  }
+  return obj;
+}`,
+        },
+        {
+          // chaining pattern: if/else mutates linked-list pointers, invariant return enables chaining
+          code: `
+interface MoveRecord<V> {
+  value: V;
+  previousIndex: number;
+  _nextMoved: MoveRecord<V> | null;
+}
+function addToMovesExample<V>(
+  record: MoveRecord<V>,
+  movesTail: MoveRecord<V> | null,
+  toIndex: number,
+): MoveRecord<V> {
+  if (record.previousIndex === toIndex) {
+    return record;
+  }
+  if (movesTail === null) {
+    movesTail = record;
+  } else {
+    movesTail._nextMoved = record;
+  }
+  return record;
+}`,
+        },
+        {
+          // signaling pattern: conditional callback invocation, invariant return of non-literal
+          code: `
+interface SelectionState { empty: boolean; from: number; }
+interface PluginState {
+  decorations: string[];
+  selection: SelectionState;
+  getAnnotationsAt(pos: number): string[] | null;
+}
+function getDecorationsExample(
+  state: PluginState,
+  onUpdate: (annotations: string[]) => void,
+): string[] {
+  const decorations = state.decorations ?? [];
+  if (!state.selection.empty) {
+    return decorations;
+  }
+  const annotations = state.getAnnotationsAt(state.selection.from);
+  if (annotations) {
+    onUpdate(annotations);
+  }
+  return decorations;
+}`,
+        },
+        {
+          // middleware pattern: async function returns non-literal for chaining while a nested
+          // conditional performs side effects (e.g. signout, alert) without returning.
+          // Two returns of the same non-literal value but with an inner side-effect-only branch.
+          code: `
+async function middlewareExample(req, next) {
+  const res = await next(req)
+  if (res.errors && res.errors.length) {
+    if (alreadyHandled(req)) {
+      return res
+    }
+    const result = await fetch('/api/check')
+    if (result.status === 401) {
+      performSignOut()
+    }
+  }
+  return res
+}`,
+        },
       ],
       invalid: [
         {
@@ -479,6 +567,31 @@ describe('S3516', () => {
         var arrowEquivalent6 = (p) => { if (p) { return ~4; } return -5; };`,
           errors: 8,
         },
+        {
+          // literal return with side-effect conditional: literal guard prevents exemption
+          code: `
+function f(a, g) {
+  if (a < 0) { return false; }
+  if (a > 10) { g(a); }
+  return false;
+}`,
+          errors: 1,
+        },
+        {
+          // switch where the case with side effect ALSO has a return: not side-effect-only branch,
+          // so the exemption does not apply and the issue is correctly raised
+          code: `
+function dispatchInTheMiddleOfReducer(state = [], action) {
+  switch (action.type) {
+    case 'DISPATCH_IN_MIDDLE':
+      sideEffect()
+      return state
+    default:
+      return state
+  }
+}`,
+          errors: 1,
+        },
       ],
     });
   });

packages/jsts/src/rules/S3516/unit.test.ts

@@ -722,3 +722,4 @@ export function hasTypePredicateReturn(node: estree.Node): boolean {
   }
   return false;
 }
+