From 41e4edf1d6470bee37443aa91ec79d4c3d7e6082 Mon Sep 17 00:00:00 2001 From: Francois Mora Date: Thu, 30 Jul 2026 17:35:23 +0200 Subject: [PATCH 1/2] JS-2192 Fix S4782 external indexed access --- .../node_modules/fake-lib/index.d.ts | 7 ++- .../src/jsts/rules/S4782/unit.test.ts | 43 +++++++++++++++++++ .../src/jsts/rules/helpers/type-origin.ts | 38 ++++++++++++---- .../node_modules/fake-lib/index.d.ts | 7 ++- .../jsts/rules/helpers/type-origin.test.ts | 22 ++++++++++ 5 files changed, 107 insertions(+), 10 deletions(-) diff --git a/packages/analysis/src/jsts/rules/S4782/fixtures/strict-null-checks/node_modules/fake-lib/index.d.ts b/packages/analysis/src/jsts/rules/S4782/fixtures/strict-null-checks/node_modules/fake-lib/index.d.ts index d6d1061553b..c708d598750 100644 --- a/packages/analysis/src/jsts/rules/S4782/fixtures/strict-null-checks/node_modules/fake-lib/index.d.ts +++ b/packages/analysis/src/jsts/rules/S4782/fixtures/strict-null-checks/node_modules/fake-lib/index.d.ts @@ -3,6 +3,11 @@ export type FakeUndefinedUnion = string | number | undefined; export type FakeNonUndefinedUnion = string | number; +export interface FakeExternalProperties { + optional?: string; + required: string; +} + // Mirrors Vue's `MaybeRef = T | Ref`: an external generic that // distributes its type argument into the resolved top-level union. export type FakeMaybeRef = T | { ref: T }; @@ -11,4 +16,4 @@ export type FakeMaybeRef = T | { ref: T }; // pin down that the walker does NOT flag inline `| undefined` buried inside // a non-distributive wrapper (e.g. `Map`), where the top-level `undefined` // comes from this wrapper rather than from the buried inline keyword. -export type FakeNullableWrapper = T | undefined; \ No newline at end of file +export type FakeNullableWrapper = T | undefined; diff --git a/packages/analysis/src/jsts/rules/S4782/unit.test.ts b/packages/analysis/src/jsts/rules/S4782/unit.test.ts index cbada64e115..bec96bbb2f9 100644 --- a/packages/analysis/src/jsts/rules/S4782/unit.test.ts +++ b/packages/analysis/src/jsts/rules/S4782/unit.test.ts @@ -460,6 +460,17 @@ 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-1789 Peach-comment reproducer: React.ReactNode resolves to a // union containing undefined, but the user cannot edit React's @@ -527,6 +538,38 @@ 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']; + }; + `, + }, + ], + }, + ], + }, { code: ` import type { FakeUndefinedUnion } from 'fake-lib'; diff --git a/packages/analysis/src/jsts/rules/helpers/type-origin.ts b/packages/analysis/src/jsts/rules/helpers/type-origin.ts index a23d4df354c..47cbb15c1f3 100644 --- a/packages/analysis/src/jsts/rules/helpers/type-origin.ts +++ b/packages/analysis/src/jsts/rules/helpers/type-origin.ts @@ -35,10 +35,13 @@ 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 symbol and inspects its declarations. + * Both forms are external only when ALL declarations live in files that + * satisfy `isSourceFileFromExternalLibrary` or `isSourceFileDefaultLibrary`. + * Any local declaration (declaration-merging escape hatch) makes the member + * internal. * - Any other composite constructor (TSIntersectionType, TSArrayType, * TSTypeLiteral, TSConditionalType, ...) -> internal at the top level. We * do not recurse; callers can if they need to. @@ -64,12 +67,16 @@ export function classifyTypesByOrigin( } function isExternalMember(member: TSESTree.TypeNode, services: RequiredParserServices): boolean { - if (member.type !== 'TSTypeReference') { + const checker = services.program.getTypeChecker(); + const tsNode = services.esTreeNodeToTSNodeMap.get(member); + let symbol: ts.Symbol | undefined; + if (member.type === 'TSTypeReference' && ts.isTypeReferenceNode(tsNode)) { + symbol = checker.getSymbolAtLocation(tsNode.typeName); + } else if (member.type === 'TSIndexedAccessType' && ts.isIndexedAccessTypeNode(tsNode)) { + symbol = getIndexedPropertySymbol(tsNode, checker); + } else { 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) { @@ -87,3 +94,18 @@ function isExternalMember(member: TSESTree.TypeNode, services: RequiredParserSer ); }); } + +function getIndexedPropertySymbol( + node: ts.IndexedAccessTypeNode, + checker: ts.TypeChecker, +): ts.Symbol | 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); + return checker.getPropertyOfType(objectType, literal.text); +} diff --git a/packages/analysis/tests/jsts/rules/helpers/fixtures/external-library/node_modules/fake-lib/index.d.ts b/packages/analysis/tests/jsts/rules/helpers/fixtures/external-library/node_modules/fake-lib/index.d.ts index c47e1329b31..12bcd5dd8d4 100644 --- a/packages/analysis/tests/jsts/rules/helpers/fixtures/external-library/node_modules/fake-lib/index.d.ts +++ b/packages/analysis/tests/jsts/rules/helpers/fixtures/external-library/node_modules/fake-lib/index.d.ts @@ -1,6 +1,11 @@ export type FakeExternalType = string | undefined; export type FakeUnionWithoutUndefined = string | number; +export interface FakeExternalProperties { + optional?: string; + required: string; +} + export namespace FakeNamespace { export type Nested = string | undefined; -} \ No newline at end of file +} diff --git a/packages/analysis/tests/jsts/rules/helpers/type-origin.test.ts b/packages/analysis/tests/jsts/rules/helpers/type-origin.test.ts index de044cd6b5b..d569aab0609 100644 --- a/packages/analysis/tests/jsts/rules/helpers/type-origin.test.ts +++ b/packages/analysis/tests/jsts/rules/helpers/type-origin.test.ts @@ -159,6 +159,28 @@ 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 a qualified name from node_modules as external', () => { const parsed = parse(` import type * as FakeLib from 'fake-lib'; From f56bd0cab58f9bb340534dc6b9deb03c7113f0a0 Mon Sep 17 00:00:00 2001 From: Francois Mora Date: Mon, 3 Aug 2026 09:13:17 +0200 Subject: [PATCH 2/2] JS-2192 Classify index-signature access by origin in S4782 Follow-up on the indexed-access fix: an access that resolves through an index signature rather than a named property (`ExternalRecord['anyKey']`) was still classified as project-local, so S4782 kept reporting it and its suggestion still made the property required. Resolve those accesses through the index signature's own declaration, and fall back to the declaration of the indexed type for signatures the checker synthesizes (mapped types such as `Record`, which have no declaration node). Cover both shapes, plus the non-literal-index path that stays reportable, at the helper and rule levels, and restate the shared all-declarations-external rule and the known limitations in the docblock. Co-Authored-By: Claude Opus 5 (1M context) --- .../node_modules/fake-lib/index.d.ts | 4 + .../src/jsts/rules/S4782/unit.test.ts | 73 ++++++++++++++ .../src/jsts/rules/helpers/type-origin.ts | 95 +++++++++++++++---- .../node_modules/fake-lib/index.d.ts | 8 ++ .../jsts/rules/helpers/type-origin.test.ts | 53 +++++++++++ 5 files changed, 212 insertions(+), 21 deletions(-) diff --git a/packages/analysis/src/jsts/rules/S4782/fixtures/strict-null-checks/node_modules/fake-lib/index.d.ts b/packages/analysis/src/jsts/rules/S4782/fixtures/strict-null-checks/node_modules/fake-lib/index.d.ts index c708d598750..e49eef0bc37 100644 --- a/packages/analysis/src/jsts/rules/S4782/fixtures/strict-null-checks/node_modules/fake-lib/index.d.ts +++ b/packages/analysis/src/jsts/rules/S4782/fixtures/strict-null-checks/node_modules/fake-lib/index.d.ts @@ -8,6 +8,10 @@ export interface FakeExternalProperties { required: string; } +// Indexing this one resolves through a synthesized index signature rather +// than a named property. +export type FakeExternalRecord = Record; + // Mirrors Vue's `MaybeRef = T | Ref`: an external generic that // distributes its type argument into the resolved top-level union. export type FakeMaybeRef = T | { ref: T }; diff --git a/packages/analysis/src/jsts/rules/S4782/unit.test.ts b/packages/analysis/src/jsts/rules/S4782/unit.test.ts index bec96bbb2f9..9f754121c11 100644 --- a/packages/analysis/src/jsts/rules/S4782/unit.test.ts +++ b/packages/analysis/src/jsts/rules/S4782/unit.test.ts @@ -471,6 +471,17 @@ describe('S4782', () => { `, 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 @@ -570,6 +581,68 @@ describe('S4782', () => { }, ], }, + { + // 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'; diff --git a/packages/analysis/src/jsts/rules/helpers/type-origin.ts b/packages/analysis/src/jsts/rules/helpers/type-origin.ts index 47cbb15c1f3..454ae660aea 100644 --- a/packages/analysis/src/jsts/rules/helpers/type-origin.ts +++ b/packages/analysis/src/jsts/rules/helpers/type-origin.ts @@ -37,18 +37,31 @@ export type TypeOrigin = { * - TSTypeReference -> resolves the type name to a symbol and inspects its * declarations. * - TSIndexedAccessType with a literal property name -> resolves the accessed - * property symbol and inspects its declarations. - * Both forms are external only when ALL declarations live in files that - * satisfy `isSourceFileFromExternalLibrary` or `isSourceFileDefaultLibrary`. - * Any local declaration (declaration-merging escape hatch) makes the member - * internal. + * 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`). * - 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, @@ -69,24 +82,19 @@ export function classifyTypesByOrigin( function isExternalMember(member: TSESTree.TypeNode, services: RequiredParserServices): boolean { const checker = services.program.getTypeChecker(); const tsNode = services.esTreeNodeToTSNodeMap.get(member); - let symbol: ts.Symbol | undefined; + let declarations: readonly ts.Declaration[] | undefined; if (member.type === 'TSTypeReference' && ts.isTypeReferenceNode(tsNode)) { - symbol = checker.getSymbolAtLocation(tsNode.typeName); + declarations = declarationsOfSymbol(checker.getSymbolAtLocation(tsNode.typeName), checker); } else if (member.type === 'TSIndexedAccessType' && ts.isIndexedAccessTypeNode(tsNode)) { - symbol = getIndexedPropertySymbol(tsNode, checker); + declarations = indexedAccessDeclarations(tsNode, checker); } else { return false; } - // 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); - } - 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) || @@ -95,10 +103,22 @@ function isExternalMember(member: TSESTree.TypeNode, services: RequiredParserSer }); } -function getIndexedPropertySymbol( +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, -): ts.Symbol | undefined { +): readonly ts.Declaration[] | undefined { if (!ts.isLiteralTypeNode(node.indexType)) { return undefined; } @@ -107,5 +127,38 @@ function getIndexedPropertySymbol( return undefined; } const objectType = checker.getTypeAtLocation(node.objectType); - return checker.getPropertyOfType(objectType, literal.text); + 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` 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; } diff --git a/packages/analysis/tests/jsts/rules/helpers/fixtures/external-library/node_modules/fake-lib/index.d.ts b/packages/analysis/tests/jsts/rules/helpers/fixtures/external-library/node_modules/fake-lib/index.d.ts index 12bcd5dd8d4..b80fff42f77 100644 --- a/packages/analysis/tests/jsts/rules/helpers/fixtures/external-library/node_modules/fake-lib/index.d.ts +++ b/packages/analysis/tests/jsts/rules/helpers/fixtures/external-library/node_modules/fake-lib/index.d.ts @@ -6,6 +6,14 @@ export interface FakeExternalProperties { required: string; } +// Two shapes of index signature: an explicit one, which has an +// `IndexSignatureDeclaration` node, and a mapped type, whose index info is +// synthesized by the checker and has no declaration. +export interface FakeExternalIndexed { + [key: string]: string | undefined; +} +export type FakeExternalRecord = Record; + export namespace FakeNamespace { export type Nested = string | undefined; } diff --git a/packages/analysis/tests/jsts/rules/helpers/type-origin.test.ts b/packages/analysis/tests/jsts/rules/helpers/type-origin.test.ts index d569aab0609..f4971c0185e 100644 --- a/packages/analysis/tests/jsts/rules/helpers/type-origin.test.ts +++ b/packages/analysis/tests/jsts/rules/helpers/type-origin.test.ts @@ -181,6 +181,59 @@ describe('classifyTypesByOrigin', () => { 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; + 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';