JS-2192 Fix S4782 false positive for external indexed access

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

@@ -460,6 +460,28 @@ describe('S4782', () => {
             `,
             filename: path.join(import.meta.dirname, 'fixtures', 'strict-null-checks', 'index.ts'),
           },
+          {
+            // JS-2192: an imported optional property resolves to a union with
+            // undefined, but its declaration cannot be changed locally.
+            code: `
+            import type { FakeExternalProperties } from 'fake-lib';
+            interface Example {
+              attribute?: FakeExternalProperties['optional'];
+            };
+            `,
+            filename: path.join(import.meta.dirname, 'fixtures', 'strict-null-checks', 'index.ts'),
+          },
+          {
+            // JS-2192: the access resolves through an index signature declared
+            // externally, so the `undefined` is just as unremovable.
+            code: `
+            import type { FakeExternalRecord } from 'fake-lib';
+            interface Example {
+              attribute?: FakeExternalRecord['anyKey'];
+            };
+            `,
+            filename: path.join(import.meta.dirname, 'fixtures', 'strict-null-checks', 'index.ts'),
+          },
           {
             // JS-1789 Peach-comment reproducer: React.ReactNode resolves to a
             // union containing undefined, but the user cannot edit React's
@@ -527,6 +549,100 @@ describe('S4782', () => {
           },
         ],
         invalid: [
+          {
+            // The same indexed-access form remains reportable when its
+            // optional property is declared in the project.
+            code: `
+            interface LocalProperties {
+              optional?: string;
+            }
+            interface Example {
+              attribute?: LocalProperties['optional'];
+            };
+            `,
+            filename: path.join(import.meta.dirname, 'fixtures', 'strict-null-checks', 'index.ts'),
+            errors: [
+              {
+                message:
+                  "Consider removing 'undefined' type or '?' specifier, one of them is redundant.",
+                suggestions: [
+                  {
+                    desc: 'Remove "?" operator',
+                    output: `
+            interface LocalProperties {
+              optional?: string;
+            }
+            interface Example {
+              attribute: LocalProperties['optional'];
+            };
+            `,
+                  },
+                ],
+              },
+            ],
+          },
+          {
+            // An index signature declared in the project stays reportable: the
+            // user can drop `| undefined` from their own signature.
+            code: `
+            interface LocalIndexed {
+              [key: string]: string | undefined;
+            }
+            interface Example {
+              attribute?: LocalIndexed['anyKey'];
+            };
+            `,
+            filename: path.join(import.meta.dirname, 'fixtures', 'strict-null-checks', 'index.ts'),
+            errors: [
+              {
+                message:
+                  "Consider removing 'undefined' type or '?' specifier, one of them is redundant.",
+                suggestions: [
+                  {
+                    desc: 'Remove "?" operator',
+                    output: `
+            interface LocalIndexed {
+              [key: string]: string | undefined;
+            }
+            interface Example {
+              attribute: LocalIndexed['anyKey'];
+            };
+            `,
+                  },
+                ],
+              },
+            ],
+          },
+          {
+            // A non-literal index cannot be resolved to a declaration, so the
+            // access stays reportable even against an external type.
+            code: `
+            import type { FakeExternalProperties } from 'fake-lib';
+            type Key = 'optional';
+            interface Example {
+              attribute?: FakeExternalProperties[Key];
+            };
+            `,
+            filename: path.join(import.meta.dirname, 'fixtures', 'strict-null-checks', 'index.ts'),
+            errors: [
+              {
+                message:
+                  "Consider removing 'undefined' type or '?' specifier, one of them is redundant.",
+                suggestions: [
+                  {
+                    desc: 'Remove "?" operator',
+                    output: `
+            import type { FakeExternalProperties } from 'fake-lib';
+            type Key = 'optional';
+            interface Example {
+              attribute: FakeExternalProperties[Key];
+            };
+            `,
+                  },
+                ],
+              },
+            ],
+          },
           {
             code: `
             import type { FakeUndefinedUnion } from 'fake-lib';

packages/analysis/src/jsts/rules/helpers/type-origin.ts

@@ -35,17 +35,33 @@ export type TypeOrigin = {
  * Classification rules per top-level member:
  * - Keyword / literal types -> internal (the user wrote them directly).
  * - TSTypeReference -> resolves the type name to a symbol and inspects its
- *   declarations. A reference is external only when ALL declarations live in
- *   files that satisfy `isSourceFileFromExternalLibrary` or
- *   `isSourceFileDefaultLibrary`. Any local declaration (declaration-merging
- *   escape hatch) makes the reference internal.
+ *   declarations.
+ * - TSIndexedAccessType with a literal property name -> resolves the accessed
+ *   property and inspects its declarations. When no named property matches,
+ *   the access goes through an index signature and the signature's own
+ *   declaration is used instead (falling back to the declaration of the
+ *   indexed type itself for signatures the checker synthesizes, such as
+ *   `Record<K, V>`).
  * - Any other composite constructor (TSIntersectionType, TSArrayType,
  *   TSTypeLiteral, TSConditionalType, ...) -> internal at the top level. We
  *   do not recurse; callers can if they need to.
  *
- * Known limitation: alias chains are not followed. If the user re-aliases an
- * external type locally (e.g. `type Inner = ReactNode`), the local alias is
- * internal because the user has a place to edit.
+ * A member is external only when ALL of the declarations found for it live in
+ * files that satisfy `isSourceFileFromExternalLibrary` or
+ * `isSourceFileDefaultLibrary`. Any local declaration (declaration-merging
+ * escape hatch) makes the member internal, and so does a member whose
+ * declarations cannot be resolved at all — being reportable is the safe
+ * default, since callers use this to decide whether a fix is applicable.
+ *
+ * Known limitations:
+ * - Alias chains are not followed. If the user re-aliases an external type
+ *   locally (e.g. `type Inner = ReactNode`), the local alias is internal
+ *   because the user has a place to edit.
+ * - An indexed access with a non-literal index (`Ext[Key]`) is internal: the
+ *   property cannot be pinned down syntactically.
+ * - A property inherited from an external base is external even though the
+ *   local type could redeclare it, because the declaration the user would
+ *   have to change is the external one.
  */
 export function classifyTypesByOrigin(
   typeNode: TSESTree.TypeNode,
@@ -64,26 +80,85 @@ export function classifyTypesByOrigin(
 }
 
 function isExternalMember(member: TSESTree.TypeNode, services: RequiredParserServices): boolean {
-  if (member.type !== 'TSTypeReference') {
-    return false;
-  }
   const checker = services.program.getTypeChecker();
-  const tsNode = services.esTreeNodeToTSNodeMap.get(member) as ts.TypeReferenceNode;
-  let symbol = checker.getSymbolAtLocation(tsNode.typeName);
-  // Imported names resolve to a local alias symbol pointing at the import
-  // statement; without following the alias, external imports would look local.
-  if (symbol && symbol.flags & ts.SymbolFlags.Alias) {
-    symbol = checker.getAliasedSymbol(symbol);
+  const tsNode = services.esTreeNodeToTSNodeMap.get(member);
+  let declarations: readonly ts.Declaration[] | undefined;
+  if (member.type === 'TSTypeReference' && ts.isTypeReferenceNode(tsNode)) {
+    declarations = declarationsOfSymbol(checker.getSymbolAtLocation(tsNode.typeName), checker);
+  } else if (member.type === 'TSIndexedAccessType' && ts.isIndexedAccessTypeNode(tsNode)) {
+    declarations = indexedAccessDeclarations(tsNode, checker);
+  } else {
+    return false;
   }
-  if (!symbol?.declarations?.length) {
+  if (!declarations?.length) {
     return false;
   }
   const program = services.program;
-  return symbol.declarations.every(decl => {
+  return declarations.every(decl => {
     const sourceFile = decl.getSourceFile();
     return (
       program.isSourceFileFromExternalLibrary(sourceFile) ||
       program.isSourceFileDefaultLibrary(sourceFile)
     );
   });
 }
+
+function declarationsOfSymbol(
+  symbol: ts.Symbol | undefined,
+  checker: ts.TypeChecker,
+): readonly ts.Declaration[] | undefined {
+  // Imported names resolve to a local alias symbol pointing at the import
+  // statement; without following the alias, external imports would look local.
+  if (symbol && symbol.flags & ts.SymbolFlags.Alias) {
+    symbol = checker.getAliasedSymbol(symbol);
+  }
+  return symbol?.declarations;
+}
+
+function indexedAccessDeclarations(
+  node: ts.IndexedAccessTypeNode,
+  checker: ts.TypeChecker,
+): readonly ts.Declaration[] | undefined {
+  if (!ts.isLiteralTypeNode(node.indexType)) {
+    return undefined;
+  }
+  const { literal } = node.indexType;
+  if (!ts.isStringLiteral(literal) && !ts.isNumericLiteral(literal)) {
+    return undefined;
+  }
+  const objectType = checker.getTypeAtLocation(node.objectType);
+  const property = checker.getPropertyOfType(objectType, literal.text);
+  if (property) {
+    return declarationsOfSymbol(property, checker);
+  }
+  return indexSignatureDeclarations(objectType, literal, checker);
+}
+
+/**
+ * Declarations backing an access that resolves through an index signature
+ * rather than a named property, e.g. `ExternalRecord['anyKey']`. A numeric
+ * literal can also hit a string signature, hence the two lookups.
+ *
+ * Mapped types (`Record<K, V>` and friends) have an index signature the
+ * checker synthesizes, with no declaration node to attribute. There we fall
+ * back to whatever declares the indexed type itself, which is the file the
+ * user would have to edit anyway.
+ */
+function indexSignatureDeclarations(
+  objectType: ts.Type,
+  literal: ts.StringLiteral | ts.NumericLiteral,
+  checker: ts.TypeChecker,
+): readonly ts.Declaration[] | undefined {
+  const kinds = ts.isNumericLiteral(literal)
+    ? [ts.IndexKind.Number, ts.IndexKind.String]
+    : [ts.IndexKind.String];
+  for (const kind of kinds) {
+    const indexInfo = checker.getIndexInfoOfType(objectType, kind);
+    if (indexInfo) {
+      return indexInfo.declaration
+        ? [indexInfo.declaration]
+        : (objectType.aliasSymbol ?? objectType.getSymbol())?.declarations;
+    }
+  }
+  return undefined;
+}

packages/analysis/tests/jsts/rules/helpers/type-origin.test.ts

@@ -159,6 +159,81 @@ describe('classifyTypesByOrigin', () => {
     expect(result.internal).toHaveLength(0);
   });
 
+  it('classifies indexed access to an external property as external', () => {
+    const parsed = parse(`
+      import type { FakeExternalProperties } from 'fake-lib';
+      type Subject = FakeExternalProperties['optional'];
+    `);
+    const result = classifyTypesByOrigin(findAliasType(parsed, 'Subject'), parsed.services);
+    expect(result.external).toHaveLength(1);
+    expect(result.internal).toHaveLength(0);
+  });
+
+  it('classifies indexed access to a local property as internal', () => {
+    const parsed = parse(`
+      interface LocalProperties {
+        optional?: string;
+      }
+      type Subject = LocalProperties['optional'];
+    `);
+    const result = classifyTypesByOrigin(findAliasType(parsed, 'Subject'), parsed.services);
+    expect(result.external).toHaveLength(0);
+    expect(result.internal).toHaveLength(1);
+  });
+
+  it('classifies indexed access through an external index signature as external', () => {
+    const parsed = parse(`
+      import type { FakeExternalIndexed } from 'fake-lib';
+      type Subject = FakeExternalIndexed['anyKey'];
+    `);
+    const result = classifyTypesByOrigin(findAliasType(parsed, 'Subject'), parsed.services);
+    expect(result.external).toHaveLength(1);
+    expect(result.internal).toHaveLength(0);
+  });
+
+  it('classifies indexed access through an external mapped type as external', () => {
+    const parsed = parse(`
+      import type { FakeExternalRecord } from 'fake-lib';
+      type Subject = FakeExternalRecord['anyKey'];
+    `);
+    const result = classifyTypesByOrigin(findAliasType(parsed, 'Subject'), parsed.services);
+    expect(result.external).toHaveLength(1);
+    expect(result.internal).toHaveLength(0);
+  });
+
+  it('classifies indexed access through a local index signature as internal', () => {
+    const parsed = parse(`
+      interface LocalIndexed {
+        [key: string]: string | undefined;
+      }
+      type Subject = LocalIndexed['anyKey'];
+    `);
+    const result = classifyTypesByOrigin(findAliasType(parsed, 'Subject'), parsed.services);
+    expect(result.external).toHaveLength(0);
+    expect(result.internal).toHaveLength(1);
+  });
+
+  it('classifies indexed access through a local mapped type as internal', () => {
+    const parsed = parse(`
+      type LocalRecord = Record<string, string | undefined>;
+      type Subject = LocalRecord['anyKey'];
+    `);
+    const result = classifyTypesByOrigin(findAliasType(parsed, 'Subject'), parsed.services);
+    expect(result.external).toHaveLength(0);
+    expect(result.internal).toHaveLength(1);
+  });
+
+  it('classifies indexed access with a non-literal index as internal', () => {
+    const parsed = parse(`
+      import type { FakeExternalProperties } from 'fake-lib';
+      type Key = 'optional';
+      type Subject = FakeExternalProperties[Key];
+    `);
+    const result = classifyTypesByOrigin(findAliasType(parsed, 'Subject'), parsed.services);
+    expect(result.external).toHaveLength(0);
+    expect(result.internal).toHaveLength(1);
+  });
+
   it('classifies a qualified name from node_modules as external', () => {
     const parsed = parse(`
       import type * as FakeLib from 'fake-lib';