The Wayback Machine - https://web.archive.org/web/20220424223116/https://github.com/microsoft/TypeScript/pull/42448
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

Limit tuple size resulting from spread #42448

Merged
merged 4 commits into from Feb 2, 2021

Conversation

Copy link
Member

@andrewbranch andrewbranch commented Jan 21, 2021

Adds a (currently very conservative, based on design meeting feedback) limit on how large of a tuple type we will create through spread normalization.

On each recursion, checking T['length'] kicks off resolveObjectTypeMembers, which instantiates base types. Recall that an N-tuple is modeled something like

interface Tuple<T0, T1, ..., TN> extends Array<T0 | T1 | ... | TN> {
  0: T0;
  1: T1;
  // ...
}

So in order to instantiate the base type, we have to pass along an array type mapper mapping T0...TN to the “type arguments,” or the element types of this particular tuple. So even though, in this case, every element type is any, the source side of the mapper is an N+1-length array filled with N unique type parameter identities (plus the this type parameter). When we instantiate each union constituent T0 | T1 | ... | TN, we have to search through that N-length array of sources, even though in this case we’ll always find any in the target side.

So you can imagine by the time we get to the 15th recursion or so, this is starting to take a while, and we’re creating quite a lot of type parameter identities. Pretty much all the time was being spent in this spiral of trying to instantiate the Array base type.

Fixes #41771

Copy link
Member

@weswigham weswigham left a comment

Yeah, this looks pretty reasonable. It's another limit someone may eventually come to us and say "but I need it higher for so and so reasons", but we can deal with that as it happens.

Copy link
Member

@ahejlsberg ahejlsberg left a comment

I like having a limiter here, but I'm thinking 10,000 is a more reasonable threshold. Also, I think it is better to have only a single diagnostic, "Expression produces a tuple type...", instead of two. We only have a single diagnostic for the other limiters and it makes it easer to search for solutions on stack overflow, etc.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jan 22, 2021

Looks good - 4.2-bound?

@andrewbranch
Copy link
Member Author

@andrewbranch andrewbranch commented Jan 22, 2021

The first message I wrote was “Spread produces a tuple type...” but I wasn’t sure how I felt about it since the error could appear far from the actual spread via instantiation. I guess I can delete the “Type” variant, it just felt wrong to call a type an expression.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jan 22, 2021

I think seeing "spread" on a type alias would actually be confusing. I would prefer the 2 variants.

@DanielRosenwasser DanielRosenwasser added the Breaking Change label Jan 25, 2021
@andrewbranch
Copy link
Member Author

@andrewbranch andrewbranch commented Jan 29, 2021

  • I agree that 10k would be fine as a limit. Will update.
  • @DanielRosenwasser if you’re concerned about this going into 4.2 as a breaking change at this point, I think it can wait until 4.3. But I think as breaking changes go, it’s not super likely to be observed, especially after I update the limit to 10k.
  • Any consensus on the error message(s)?

@andrewbranch andrewbranch merged commit 71de94a into microsoft:master Feb 2, 2021
9 checks passed
@andrewbranch andrewbranch deleted the bug/41771 branch Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change For Milestone Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants