Skip to content
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

indexing with T extends keyof <Large Union> causes OOM if keyof has a number #24223

Closed
weswigham opened this issue May 17, 2018 · 1 comment · Fixed by #25859
Closed

indexing with T extends keyof <Large Union> causes OOM if keyof has a number #24223

weswigham opened this issue May 17, 2018 · 1 comment · Fixed by #25859
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@weswigham
Copy link
Member

Ref #23977 and discussion on #24137

Code

Isolated repro (needs dom lib for ElementTagNameMap and associated types):

// @strict: true
// Modified repro from #23977

declare global {
    interface ElementTagNameMap {
        [index: number]: HTMLElement
    }

    interface HTMLElement {
        [index: number]: HTMLElement;
    }
}

export function assertIsElement(node: Node | null): node is Element {
    let nodeType = node === null ? null : node.nodeType;
    return nodeType === 1;
}
  
export function assertNodeTagName<
    T extends keyof ElementTagNameMap,
    U extends ElementTagNameMap[T]>(node: Node | null, tagName: T): node is U {
    if (assertIsElement(node)) {
        const nodeTagName = node.tagName.toLowerCase();
         return nodeTagName === tagName;
    }
    return false;
}
  
export function assertNodeProperty<
    T extends keyof ElementTagNameMap,
    P extends keyof ElementTagNameMap[T],
    V extends HTMLElementTagNameMap[T][P]>(node: Node | null, tagName: T, prop: P, value: V) {
    if (assertNodeTagName(node, tagName)) {
        node[prop];
    }
}

Expected behavior:
No crash.

Actual behavior:
OOM crash after a very long delay.

Preliminary investigation shows the OOM occurs when we try to get the distributed form of keyof <Large Union> - namely the process is defined recursively, so when the union has ~170 members, we create 170 intermediate types (flattening one member at a time, while also wasting time recalculating type flags and other bits we will never need) before getting the final, distributed result. Now, picture doing this on every relation comparison operation for P (this distribution is uncached), and multiplying it for V in assertNodeProperty - the complexity is huge. Our fast path doesn't fix this, since the numeric index signatures cause us to bail on the fast path as it currently is.

@weswigham weswigham added the Bug A bug in TypeScript label May 17, 2018
mattmccutchen added a commit to mattmccutchen/TypeScript that referenced this issue Jul 17, 2018
Helps avoid exponential blowup for `keyof` large unions even when
`keyof` each type in the union is not a union of unit types (e.g.,
because there is an index signature or a type variable).

Remove the special handling of intersections of unions of unit types
because it's no longer needed.  This reverts the code changes of pull
request microsoft#24137 (commit 3fc3df3 with
respect to 3fc727b) but keeps the test.

Fixes microsoft#24223.
@mattmccutchen
Copy link
Contributor

I ran into this while attempting to write real-world code (I can provide the example if it's of interest). I have a fix up at #25728.

mattmccutchen added a commit to mattmccutchen/TypeScript that referenced this issue Jul 19, 2018
Helps avoid exponential blowup for `keyof` large unions even when
`keyof` each type in the union is not a union of unit types (e.g.,
because there is an index signature or a type variable).

Remove the special handling of intersections of unions of unit types
because it's no longer needed.  This reverts the code changes of pull
request microsoft#24137 (commit 3fc3df3 with
respect to 3fc727b) but keeps the test.

Fixes microsoft#24223.
mattmccutchen added a commit to mattmccutchen/TypeScript that referenced this issue Jul 20, 2018
Helps avoid exponential blowup for `keyof` large unions even when
`keyof` each type in the union is not a union of unit types (e.g.,
because there is an index signature or a type variable).

Fixes microsoft#24223.
@ahejlsberg ahejlsberg self-assigned this Jul 21, 2018
@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Jul 21, 2018
@mhegazy mhegazy added this to the TypeScript 3.1 milestone Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants