JS-1505 Fix S1874 crash on TSX files with react-jsx in monorepos (#6695)

Co-authored-by: Victor Diez <victor.diez@sonarsource.com>

Author: francois-mora-sonarsource

packages/analysis/src/jsts/program/cache/sourceFileCache.ts

@@ -26,32 +26,32 @@ const sourceFileContentCache = new Map<string, string>();
 /**
  * Global cache for parsed TypeScript SourceFile ASTs
  *
- * This cache stores parsed ASTs keyed by (fileName, scriptTarget).
+ * This cache stores parsed ASTs keyed by (fileName, scriptTarget, jsx, importHelpers).
  * The contentHash is stored as a value to validate cache freshness.
- * Multiple programs can share the same parsed AST if they have the same target.
+ * Multiple programs can share the same parsed AST if they have the same options.
  *
- * IMPLEMENTATION NOTE: We cache SourceFiles per ScriptTarget to preserve the
- * languageVersion metadata stored in the SourceFile object. While our testing shows
- * that the AST structure is identical across targets and type checking uses the
- * program's target (not the SourceFile's languageVersion), we keep target-specific
- * caching for these reasons:
- *
- * 1. Performance gain of target-agnostic caching may be marginal since most analysis
- *    runs use the same target configuration
- * 2. Preserves metadata integrity - SourceFile.languageVersion matches the intended target
- * 3. Future-proofing - allows filtering rules based on configured target if needed
- * 4. TypeScript API contract - creating SourceFiles with their intended target is the
- *    documented usage pattern
- *
- * If profiling shows that different targets are commonly used and parsing becomes a
- * bottleneck, we could simplify to target-agnostic caching (always use ESNext) with
- * minimal impact on functionality.
+ * IMPLEMENTATION NOTE: We key on scriptTarget, jsx, and importHelpers because all
+ * three affect the structure of SourceFile.imports (the synthesized JSX runtime import
+ * prepended by TypeScript when jsx=react-jsx, and tslib helpers when importHelpers=true).
+ * Sharing a SourceFile between programs with different values for these options causes
+ * internal TypeScript assertion failures (e.g. in getSuggestionDiagnostics).
  */
 const parsedSourceFileCache = new Map<
   string, // normalized file path
-  Map<ts.ScriptTarget, { contentHash: string; sourceFile: ts.SourceFile }>
+  Map<string, { contentHash: string; sourceFile: ts.SourceFile }> // compound key → entry
 >();
 
+/**
+ * Build a compound cache key from the options that affect SourceFile.imports structure.
+ */
+function makeParsedSourceFileCacheKey(
+  scriptTarget: ts.ScriptTarget,
+  jsx: ts.JsxEmit | undefined,
+  importHelpers = false,
+): string {
+  return `${scriptTarget}:${jsx ?? -1}:${importHelpers ? 1 : 0}`;
+}
+
 /**
  * Current files context for lazy loading
  */
@@ -79,23 +79,28 @@ export function getCurrentFilesContext(): Record<string, { fileContent?: string
 }
 
 /**
- * Get a cached parsed SourceFile for a given file and target
+ * Get a cached parsed SourceFile for a given file and compiler options
  * @param fileName Normalized file path
  * @param scriptTarget TypeScript ScriptTarget (e.g., ES5, ES2020, ESNext)
  * @param contentHash Hash/version of the file content
+ * @param jsx The jsx compiler option (affects SourceFile.imports structure)
+ * @param importHelpers The importHelpers compiler option (affects SourceFile.imports structure)
  * @returns Cached SourceFile if available and content matches, undefined otherwise
  */
 export function getCachedSourceFile(
   fileName: string,
   scriptTarget: ts.ScriptTarget,
   contentHash: string,
+  jsx?: ts.JsxEmit,
+  importHelpers = false,
 ): ts.SourceFile | undefined {
-  const targetCache = parsedSourceFileCache.get(fileName);
-  if (!targetCache) {
+  const fileCache = parsedSourceFileCache.get(fileName);
+  if (!fileCache) {
     return undefined;
   }
 
-  const cached = targetCache.get(scriptTarget);
+  const key = makeParsedSourceFileCacheKey(scriptTarget, jsx, importHelpers);
+  const cached = fileCache.get(key);
   // If hash doesn't match, return undefined (cache miss). The caller will parse
   // the file and call setCachedSourceFile, which overwrites the stale entry.
   // This makes the cache self-healing - stale entries cause one miss, then get fixed.
@@ -112,20 +117,25 @@ export function getCachedSourceFile(
  * @param scriptTarget TypeScript ScriptTarget used to parse the file
  * @param contentHash Hash/version of the file content
  * @param sourceFile The parsed TypeScript SourceFile
+ * @param jsx The jsx compiler option (affects SourceFile.imports structure)
+ * @param importHelpers The importHelpers compiler option (affects SourceFile.imports structure)
  */
 export function setCachedSourceFile(
   fileName: string,
   scriptTarget: ts.ScriptTarget,
   contentHash: string,
   sourceFile: ts.SourceFile,
+  jsx?: ts.JsxEmit,
+  importHelpers = false,
 ): void {
-  let targetCache = parsedSourceFileCache.get(fileName);
-  if (!targetCache) {
-    targetCache = new Map();
-    parsedSourceFileCache.set(fileName, targetCache);
+  let fileCache = parsedSourceFileCache.get(fileName);
+  if (!fileCache) {
+    fileCache = new Map();
+    parsedSourceFileCache.set(fileName, fileCache);
   }
 
-  targetCache.set(scriptTarget, { contentHash, sourceFile });
+  const key = makeParsedSourceFileCacheKey(scriptTarget, jsx, importHelpers);
+  fileCache.set(key, { contentHash, sourceFile });
 }
 
 /**

packages/analysis/src/jsts/program/compilerHost.ts

@@ -50,7 +50,7 @@ export class IncrementalCompilerHost implements ts.CompilerHost {
   private readonly baseHost: ts.CompilerHost;
 
   constructor(
-    compilerOptions: ts.CompilerOptions,
+    private readonly compilerOptions: ts.CompilerOptions,
     private readonly baseDir: NormalizedAbsolutePath,
     private readonly skipNodeModuleLookupOutsideBaseDir = false,
   ) {
@@ -218,9 +218,11 @@ export class IncrementalCompilerHost implements ts.CompilerHost {
         ? languageVersionOrOptions
         : languageVersionOrOptions.languageVersion;
 
+    const { jsx, importHelpers } = this.compilerOptions;
+
     // Check global cache (unless forced to create new)
     if (!shouldCreateNewSourceFile) {
-      const cached = getCachedSourceFile(normalized, scriptTarget, contentHash);
+      const cached = getCachedSourceFile(normalized, scriptTarget, contentHash, jsx, importHelpers);
       if (cached) {
         // Set version for builder programs
         (cached as ts.SourceFile & { version?: string }).version = contentHash;
@@ -241,7 +243,7 @@ export class IncrementalCompilerHost implements ts.CompilerHost {
       );
 
       // Store in global cache for reuse across programs
-      setCachedSourceFile(normalized, scriptTarget, contentHash, sourceFile);
+      setCachedSourceFile(normalized, scriptTarget, contentHash, sourceFile, jsx, importHelpers);
     } else {
       // Fallback to base host for files not in our cache/context (e.g., TypeScript lib files).
       // readFile returns undefined for these, but baseHost can resolve them.

packages/analysis/tests/analyzeProject-sonarqube.test.ts

@@ -816,7 +816,7 @@ describe('SonarQube project analysis', () => {
     }
   });
 
-  it('should reproduce JSX cache collision with mixed jsx compiler options', async () => {
+  it('should analyze JSX cache collision fixture with mixed jsx compiler options', async () => {
     const baseDir = join(fixtures, 'jsx-cache-collision');
     const initialFile = join(baseDir, 'initial.ts');
     const triggerFile = join(baseDir, 'trigger.tsx');
@@ -842,11 +842,9 @@ describe('SonarQube project analysis', () => {
 
     const triggerResult = result.files[normalizeToAbsolutePath(triggerFile)];
     expect(triggerResult).toBeDefined();
-    expect('error' in triggerResult!).toBe(true);
-    if ('error' in triggerResult!) {
-      expect(triggerResult.error).toContain(
-        'Expected sourceFile.imports[0] to be the synthesized JSX runtime import',
-      );
+    expect('issues' in triggerResult!).toBe(true);
+    if ('issues' in triggerResult!) {
+      expect(triggerResult.issues.some(issue => issue.ruleId === 'S1874')).toBe(true);
     }
 
     expect(result.meta.telemetry?.compilerOptions.jsx).toEqual(

packages/analysis/tests/jsts/program/compilerHost.test.ts

@@ -17,6 +17,8 @@
 import { describe, it, beforeEach } from 'node:test';
 import { expect } from 'expect';
 import ts from 'typescript';
+import path from 'node:path';
+import { readFileSync } from 'node:fs';
 import { IncrementalCompilerHost } from '../../../src/jsts/program/compilerHost.js';
 import {
   setSourceFilesContext,
@@ -412,4 +414,86 @@ describe('IncrementalCompilerHost', () => {
       host.writeFile(normalizeToAbsolutePath('/project/dist/index.js'), 'content', false);
     });
   });
+
+  describe('cross-program SourceFile cache with different jsx options (JS-1505)', () => {
+    // Fixture: server.ts (jsx:preserve tsconfig) imports Component.tsx, which is also
+    // included in a jsx:react-jsx tsconfig. This reproduces the cross-batch crash scenario:
+    //
+    // Batch 1: server.ts is in the analysis context. Program A (jsx:preserve) is created.
+    //   TypeScript follows the import and calls getSourceFile(Component.tsx). Component.tsx
+    //   is NOT in the context → read from disk → SourceFile cached without synthesized import.
+    //
+    // Batch 2: Component.tsx is in the analysis context (same content as on disk).
+    //   Program B (jsx:react-jsx) is created. getSourceFile(Component.tsx) is called.
+    //   updateFile is a no-op (same content → same contentHash) → cache HIT → stale SourceFile.
+    //   getSuggestionDiagnostics on the stale SourceFile triggers TypeScript's internal assertion:
+    //   "Expected sourceFile.imports[0] to be the synthesized JSX runtime import"
+    //
+    // The fix: jsx is now part of the cache key, so Program B gets a cache miss and a fresh
+    // SourceFile with the correct synthesized import at file.imports[0].
+    const fixtureDir = normalizeToAbsolutePath(
+      path.join(import.meta.dirname, 'fixtures', 'tsx-cache-collision'),
+    );
+    const serverFile = path.join(fixtureDir, 'server.ts') as ReturnType<
+      typeof normalizeToAbsolutePath
+    >;
+    const componentFile = path.join(fixtureDir, 'Component.tsx') as ReturnType<
+      typeof normalizeToAbsolutePath
+    >;
+
+    it('should not serve a stale SourceFile to a jsx:react-jsx program when Component.tsx was first read from disk by a jsx:preserve program', () => {
+      // Read the actual file content so the "same content" invariant holds.
+      const serverContent = readFileSync(serverFile, 'utf-8');
+      const componentContent = readFileSync(componentFile, 'utf-8');
+
+      // --- Batch 1: only server.ts is in the analysis context ---
+      // Component.tsx is NOT in the context → getSourceFile reads it from disk.
+      setSourceFilesContext({ [serverFile]: { fileContent: serverContent } });
+
+      const noJsxConfig = ts.readConfigFile(
+        path.join(fixtureDir, 'tsconfig-no-jsx.json'),
+        ts.sys.readFile,
+      );
+      const noJsxOptions = ts.parseJsonConfigFileContent(
+        noJsxConfig.config,
+        ts.sys,
+        fixtureDir,
+      ).options;
+      const hostA = new IncrementalCompilerHost(noJsxOptions, fixtureDir);
+      // Creating Program A causes TypeScript to call getSourceFile(Component.tsx) transitively.
+      ts.createProgram({ rootNames: [serverFile], options: noJsxOptions, host: hostA });
+
+      // --- Batch 2: Component.tsx is now in the analysis context ---
+      // Same content as on disk → updateFile is a no-op → contentHash unchanged.
+      // Without the fix, Program B would receive Program A's stale SourceFile (wrong imports).
+      setSourceFilesContext({ [componentFile]: { fileContent: componentContent } });
+
+      const jsxConfig = ts.readConfigFile(
+        path.join(fixtureDir, 'tsconfig-jsx.json'),
+        ts.sys.readFile,
+      );
+      const jsxOptions = ts.parseJsonConfigFileContent(
+        jsxConfig.config,
+        ts.sys,
+        fixtureDir,
+      ).options;
+      const hostB = new IncrementalCompilerHost(jsxOptions, fixtureDir);
+      const programB = ts.createProgram({
+        rootNames: [componentFile],
+        options: jsxOptions,
+        host: hostB,
+      });
+      const checker = programB.getTypeChecker();
+      const sourceFile = programB.getSourceFile(componentFile);
+
+      // getSuggestionDiagnostics is the non-public API called by rule S1874.
+      // Without the fix it throws:
+      //   "Debug Failure. False expression: Expected sourceFile.imports[0] to be
+      //    the synthesized JSX runtime import"
+      expect(() => {
+        // @ts-ignore: TypeChecker#getSuggestionDiagnostics is not publicly exposed
+        checker.getSuggestionDiagnostics(sourceFile);
+      }).not.toThrow();
+    });
+  });
 });

packages/analysis/tests/jsts/program/fixtures/tsx-cache-collision/Component.tsx

@@ -0,0 +1,11 @@
+import { deprecated } from './deprecated';
+
+// JSX + user import: the user import is at file.imports[0].
+// When jsx=react-jsx, TypeScript synthesizes a react/jsx-runtime import that should be at
+// file.imports[0]. If the SourceFile is reused from a non-react-jsx program (stale cache),
+// file.imports[0] is this real import instead, triggering a TypeScript internal assertion
+// in getSuggestionDiagnostics: "Expected sourceFile.imports[0] to be the synthesized JSX
+// runtime import"
+export function Component() {
+  return <span>{deprecated()}</span>;
+}

packages/analysis/tests/jsts/program/fixtures/tsx-cache-collision/deprecated.ts

@@ -0,0 +1,4 @@
+/** @deprecated Use newFunction instead */
+export function deprecated(): string {
+  return 'deprecated';
+}

packages/analysis/tests/jsts/program/fixtures/tsx-cache-collision/server.ts

@@ -0,0 +1,4 @@
+// This file's import forces Program A (jsx:preserve) to process Component.tsx as a
+// transitive dependency, caching its SourceFile without the synthesized JSX runtime import.
+import type { Component } from './Component';
+void 0 as unknown as typeof Component;

packages/analysis/tests/jsts/program/fixtures/tsx-cache-collision/tsconfig-jsx.json

@@ -0,0 +1,6 @@
+{
+  "compilerOptions": {
+    "jsx": "react-jsx"
+  },
+  "include": ["Component.tsx"]
+}

packages/analysis/tests/jsts/program/fixtures/tsx-cache-collision/tsconfig-no-jsx.json

@@ -0,0 +1,6 @@
+{
+  "compilerOptions": {
+    "jsx": "preserve"
+  },
+  "include": ["server.ts"]
+}

packages/analysis/tests/jsts/program/sourceFileCache.test.ts

@@ -133,6 +133,57 @@ describe('sourceFileCache', () => {
         const result = getCachedSourceFile(testFileName, ts.ScriptTarget.ES2020, '1');
         expect(result).toBeUndefined();
       });
+
+      it('should return undefined when jsx option does not match', () => {
+        const sourceFile = createSourceFile(testFileName, testContent, ts.ScriptTarget.ESNext);
+
+        setCachedSourceFile(testFileName, ts.ScriptTarget.ESNext, '1', sourceFile, ts.JsxEmit.None);
+
+        const result = getCachedSourceFile(
+          testFileName,
+          ts.ScriptTarget.ESNext,
+          '1',
+          ts.JsxEmit.ReactJSX,
+        );
+        expect(result).toBeUndefined();
+      });
+
+      it('should return undefined when importHelpers option does not match', () => {
+        const sourceFile = createSourceFile(testFileName, testContent, ts.ScriptTarget.ESNext);
+
+        setCachedSourceFile(testFileName, ts.ScriptTarget.ESNext, '1', sourceFile, undefined, true);
+
+        const result = getCachedSourceFile(
+          testFileName,
+          ts.ScriptTarget.ESNext,
+          '1',
+          undefined,
+          false,
+        );
+        expect(result).toBeUndefined();
+      });
+
+      it('should return cached file when jsx and importHelpers match', () => {
+        const sourceFile = createSourceFile(testFileName, testContent, ts.ScriptTarget.ESNext);
+
+        setCachedSourceFile(
+          testFileName,
+          ts.ScriptTarget.ESNext,
+          '1',
+          sourceFile,
+          ts.JsxEmit.ReactJSX,
+          true,
+        );
+
+        const result = getCachedSourceFile(
+          testFileName,
+          ts.ScriptTarget.ESNext,
+          '1',
+          ts.JsxEmit.ReactJSX,
+          true,
+        );
+        expect(result).toBe(sourceFile);
+      });
     });
 
     describe('setCachedSourceFile', () => {
@@ -159,6 +210,37 @@ describe('sourceFileCache', () => {
         );
       });
 
+      it('should store separate entries for different jsx options', () => {
+        const sourceFileNoJsx = createSourceFile(testFileName, testContent, ts.ScriptTarget.ESNext);
+        const sourceFileReactJsx = createSourceFile(
+          testFileName,
+          testContent,
+          ts.ScriptTarget.ESNext,
+        );
+
+        setCachedSourceFile(
+          testFileName,
+          ts.ScriptTarget.ESNext,
+          '1',
+          sourceFileNoJsx,
+          ts.JsxEmit.None,
+        );
+        setCachedSourceFile(
+          testFileName,
+          ts.ScriptTarget.ESNext,
+          '1',
+          sourceFileReactJsx,
+          ts.JsxEmit.ReactJSX,
+        );
+
+        expect(
+          getCachedSourceFile(testFileName, ts.ScriptTarget.ESNext, '1', ts.JsxEmit.None),
+        ).toBe(sourceFileNoJsx);
+        expect(
+          getCachedSourceFile(testFileName, ts.ScriptTarget.ESNext, '1', ts.JsxEmit.ReactJSX),
+        ).toBe(sourceFileReactJsx);
+      });
+
       it('should overwrite cached source file when content hash changes', () => {
         const sourceFile1 = createSourceFile(testFileName, testContent, ts.ScriptTarget.ESNext);
         const sourceFile2 = createSourceFile(testFileName, 'const y = 2;', ts.ScriptTarget.ESNext);