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..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 @@ -3,6 +3,15 @@ export type FakeUndefinedUnion = string | number | undefined; export type FakeNonUndefinedUnion = string | number; +export interface FakeExternalProperties { + optional?: string; + 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 }; @@ -11,4 +20,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..9f754121c11 100644 --- a/packages/analysis/src/jsts/rules/S4782/unit.test.ts +++ b/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'; diff --git a/packages/analysis/src/jsts/rules/helpers/type-origin.ts b/packages/analysis/src/jsts/rules/helpers/type-origin.ts index a23d4df354c..454ae660aea 100644 --- a/packages/analysis/src/jsts/rules/helpers/type-origin.ts +++ b/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`). * - 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,22 +80,21 @@ 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) || @@ -87,3 +102,63 @@ function isExternalMember(member: TSESTree.TypeNode, services: RequiredParserSer ); }); } + +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` 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 c47e1329b31..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 @@ -1,6 +1,19 @@ export type FakeExternalType = string | undefined; export type FakeUnionWithoutUndefined = string | number; +export interface FakeExternalProperties { + optional?: string; + 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; -} \ 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..f4971c0185e 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,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; + 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';