"Type instantiation is excessively deep and possibly infinite." for big keyof types

[sgrishchenko]

TypeScript Version: 3.7.2

Search Terms:

Expected behavior:
There isn't any error message in TypeScript v3.5.1

Actual behavior:
"Type instantiation is excessively deep and possibly infinite." message is emitted

Compiler Options

{
  "compilerOptions": {
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "useDefineForClassFields": false,
    "alwaysStrict": true,
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    "downlevelIteration": false,
    "noEmitHelpers": false,
    "noLib": false,
    "noStrictGenericChecks": false,
    "noUnusedLocals": false,
    "noUnusedParameters": false,
    "esModuleInterop": true,
    "preserveConstEnums": false,
    "removeComments": false,
    "skipLibCheck": false,
    "checkJs": false,
    "allowJs": false,
    "declaration": true,
    "experimentalDecorators": false,
    "emitDecoratorMetadata": false,
    "target": "ES2017",
    "module": "ESNext"
  }
}

Playground Link: Provided

[RyanCavanaugh]

@weswigham this doesn't appear to me to be something that should trigger this error?

[taxilian]

This particular error crops up a lot in a lot of cases; while I agree that there needs to be a limit for sanity sake, it'd be really nice to be able to set that limit in tsconfig or at the very least increase it by 50-100%.

[weswigham]

This seems to be fixed in the latest nightlies. Based on a bisect I ran, @amcasey it looks like 865c120 fixed this. Do you want this playground (or something like it) added as a test case, since #36951 didn't have any tests?

[weswigham]

Specifically, that change made it so we no longer try to instantiate unions of literals (neat). However the broader problem is thus: for each mapped type member, we instantiate the member type with the template mapper (to perform substitutions). In doing to, we perform instantiation on every member of the union. In this case, that meant for 2236 properties, we did 2236 instantiations per property, which is juuuust under the 5,000,000 global instantiations limit (so with lib stuff, easily over it). See, thing is, I really don't think noop instantiations like this should count towards that work limit. I may save/restore that counter such that it only increases when an instantiation operation actually produces a change (rather than just traversing a type). @ahejlsberg that make sense to you?

[weswigham]

I can't easily modify the test case to trigger under current master - the change I mentioned above plus the change to defer mapped type member types make it super hard to trigger the (property count)^2 instantiations on a single statement like we were doing before, so this does seem well and thoroughly fixed; so yeah @amcasey you think a test case (beyond anything implied by those changes) is warranted?

[amcasey]

@weswigham When I made the change, I was thinking of it as a perf enhancement that wouldn't change the semantics, so I didn't add a semantic test. I guess there's an extremity where a perf fix becomes a correctness fix, but I have mixed feelings about how valuable a regression test for such a change would be. Personally, I would probably just add a comment to the kick-out lining explaining that it can enable scenarios with very large string unions, so that it's not inadvertently removed. I might even go so far as to link this bug in the comment, but I don't think I'd make every test run slower to confirm that we don't regress this.

[weswigham]

"Make every test run slower" here being like 2-3 seconds, tops. It's not some monster 30s test or anything.

[amcasey]

I think the probability and impact of a regression are both small, so I'm not inclined to pay very much to avoid it. If you view the trade-off differently, then go ahead and add a test.

[weswigham]

Alright, I suppose I'll just close the issue, then.