-
Notifications
You must be signed in to change notification settings - Fork 235
fix: omit utility type with recursive interfaces causing stack overflow #2426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Changes from all commits
a7f83e8
5da19a8
d64176a
89ded07
4c1b725
ad79c0e
6b7a477
b3221ea
4075d29
162c0a3
c646d44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import type { ReferenceType } from "../Type/ReferenceType.js"; | |
| import { isNodeHidden } from "../Utils/isHidden.js"; | ||
| import { isPublic, isStatic } from "../Utils/modifiers.js"; | ||
| import { getKey } from "../Utils/nodeKey.js"; | ||
| import { isTypeScriptLibFile } from "../Utils/isTypeScriptLibFile.js"; | ||
|
|
||
| export class InterfaceAndClassNodeParser implements SubNodeParser { | ||
| public constructor( | ||
|
|
@@ -96,7 +97,27 @@ export class InterfaceAndClassNodeParser implements SubNodeParser { | |
| return node.heritageClauses.reduce( | ||
| (result: BaseType[], baseType) => [ | ||
| ...result, | ||
| ...baseType.types.map((expression) => this.childNodeParser.createType(expression, context)), | ||
| ...baseType.types | ||
| .map((expression) => { | ||
| // Skip processing of TypeScript lib utility types in heritage clauses | ||
| // to avoid infinite recursion with recursive types | ||
| const typeSymbol = this.typeChecker.getSymbolAtLocation(expression.expression); | ||
| if (typeSymbol && typeSymbol.flags & ts.SymbolFlags.Alias) { | ||
| const aliasedSymbol = this.typeChecker.getAliasedSymbol(typeSymbol); | ||
| // Check if any declaration is from a TypeScript lib file | ||
| // Lib utility types (Omit, Pick, etc.) should be skipped to prevent | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's test that omit and pick if not recursive work well even after this change.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Existing tests for non-recursive Omit and Pick already exist and pass:
These verify that the fix doesn't break non-recursive usage of these utility types. |
||
| // following into their internal mapped type implementations | ||
| const isLibType = aliasedSymbol.declarations?.some((decl) => | ||
| isTypeScriptLibFile(decl.getSourceFile()), | ||
| ); | ||
| if (isLibType) { | ||
| // This is a lib utility type - skip it | ||
| return null; | ||
| } | ||
| } | ||
| return this.childNodeParser.createType(expression, context); | ||
| }) | ||
| .filter((type): type is BaseType => type !== null), | ||
| ], | ||
| [], | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import type ts from "typescript"; | ||
|
|
||
| /** | ||
| * Regular expression pattern for detecting TypeScript lib files. | ||
| * Matches file paths like: /path/to/typescript/lib/lib.es5.d.ts | ||
| */ | ||
| export const TYPESCRIPT_LIB_FILE_PATTERN = /[/\\]typescript[/\\]lib[/\\]lib\.[^/\\]+\.d\.ts$/i; | ||
|
|
||
| /** | ||
| * Checks if a source file is part of the TypeScript standard library. | ||
| * This is used to identify utility types (like Omit, Pick, Exclude, etc.) | ||
| * that should be treated specially to avoid infinite recursion issues. | ||
| * | ||
| * @param sourceFile The source file to check | ||
| * @returns true if the file is a TypeScript lib file, false otherwise | ||
| */ | ||
| export function isTypeScriptLibFile(sourceFile: ts.SourceFile | undefined): boolean { | ||
| if (!sourceFile) { | ||
| return false; | ||
| } | ||
|
|
||
| return TYPESCRIPT_LIB_FILE_PATTERN.test(sourceFile.fileName); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import { assertValidSchema } from "../../utils"; | ||
| import { test } from "node:test"; | ||
|
|
||
| test( | ||
| "valid-data - type-omit-recursive", | ||
| assertValidSchema("type-omit-recursive", ["IFormProps", "SimpleComplexItem", "PickedItem"]), | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| // Test case for Omit with recursive interfaces | ||
| // This ensures that when an interface extends Omit<RecursiveType, 'property'>, | ||
| // the schema generator correctly handles the utility type without infinite recursion | ||
|
|
||
| // Simple recursive interface case | ||
| interface IItems { | ||
| items?: IItems; | ||
| value: string; | ||
| } | ||
|
|
||
| // IFormProps extends Omit<IItems, 'items'> should only have the 'value' property | ||
| // The 'items' property is correctly omitted | ||
| export interface IFormProps extends Omit<IItems, 'items'> { | ||
| } | ||
|
|
||
| // Complex case with multiple properties | ||
| interface ComplexItem { | ||
| id: string; | ||
| nested?: ComplexItem; | ||
| data: number; | ||
| optional?: string; | ||
| } | ||
|
|
||
| // Omit multiple properties | ||
| export interface SimpleComplexItem extends Omit<ComplexItem, 'nested' | 'optional'> { | ||
| } | ||
|
|
||
| // Pick variant (also a utility type) | ||
| export interface PickedItem extends Pick<IItems, 'value'> { | ||
| } | ||
|
|
||
| // Note: Cases with conditional types and infer (like ArrayElement<T>) are known to still | ||
| // cause issues and are tracked separately as they require different handling |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "definitions": { | ||
| "IFormProps": { | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "value": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "value" | ||
| ], | ||
| "type": "object" | ||
| }, | ||
| "PickedItem": { | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "value": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "value" | ||
| ], | ||
| "type": "object" | ||
| }, | ||
| "SimpleComplexItem": { | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "data": { | ||
| "type": "number" | ||
| }, | ||
| "id": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "data", | ||
| "id" | ||
| ], | ||
| "type": "object" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file has rather involved changes. Are they really required since the error seems to be a regression (and the previous code worked).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core change is minimal - it adds a filter to skip TypeScript lib utility types in heritage clauses to prevent infinite recursion. The involved formatting is due to:
.map()to.map().filter()chainThe alternative would be improving circular reference detection to handle the recursive type + Omit case, which would be a much larger change. This approach prevents the parser from following into the internal mapped type implementations of lib utility types (Omit, Pick, etc.) which was causing the recursion.