-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Add type instantiation count limiter #32079
Conversation
@typescript-bot run dt |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 49e3f17. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 49e3f17. You can monitor the build here. It should now contribute to this PR's status checks. |
Clean DT test run, RWC looks clean too (not sure what the failures are, but don't appear related to this). |
@weswigham @RyanCavanaugh I'm thinking this might be a better way to fix #31823. It errors on the code in #31823 after about one second but otherwise doesn't affect any of our other tests. It certainly seems to me that any expression or statement that causes >5,000,000 type instantiations (be they cached or not) is highly suspect. |
@@ -11408,13 +11409,14 @@ namespace ts { | |||
if (!type || !mapper || mapper === identityMapper) { | |||
return type; | |||
} | |||
if (instantiationDepth === 50) { | |||
if (instantiationDepth === 50 || instantiationCount >= 5000000) { |
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.
Do we still need instantiationDepth
if we're also looking at a total count?
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.
We definitely do. The depth limiter is there to catch infinitely recursive types, we'd blow the stack way before the count limiter gets to 5M.
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.
Fair enough. Although we could always modify instantiation to use a trampoline instead of direct recursion so we don't have to worry about stack depth, and then the total count of instantiations would really be the absolute measure of complexity, without need for a specific depth-measuring mechanism. (And would enable people to nest those if/else style conditionals more than 50 levels deep, like we've occasionally seen reports of - those may be deep, but they're often trivial)
Sooooo.... What's the probability of me running into this limit? Does tsc currently tell you how many instantiations there are per type? |
you can get the absolute count of types with
@typescript-bot pack this so you can try |
Heya @weswigham, I've started to run the tarball bundle task on this PR at 49e3f17. You can monitor the build here. It should now contribute to this PR's status checks. |
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running |
@AnyhowStep You can use edit: I got ninja’d by like 20 minutes... that’ll teach me to refresh the page before posting... |
The odds you'd hit this limit without either an adverserially-crafted repro or a program that would have generated an unbounded number of types are extremely low. A compilation generating 5M types wouldn't be one you would want to wait for to finish. |
Note that the 5M limit is an absolute limit on requested type instantiations during the checking of a single expression or statement node, not a limit on unique instantiated types. For the counter to reach 5M a substantial portion of the types would have to be types that were previously instantiated and cached. The particular repro in #31823 does indeed request the same type instantiations over and over and then combines them into a relatively smaller number of new types. The core idea with the counter is to monitor work performed vs. resources consumed. |
Also note that |
What connotation does "Woohoo!" have? lol |
By the way, this is a composite project made up of 34 sub-projects. It takes 9 minutes to compile from scratch. However, most of these sub-projects take about 20-30s to compile. The screenshot above is for one of those sub-projects that usually takes 20-30s to compile. I modified the package @weswigham packed for me to print out the highest instantiation count found per project. It looks like I do run into the 5 million limit! [Edit] I see a bunch of these projects having I'm actually rather surprised that particular sub-project runs into the 5 million limit. It usually takes 20-30s to build. I suspected that a different sub-project would run into problems but it didn't. It takes 70s to build at the moment, instantiates 100k at most. [Edit] I went "Woohoo!" because I was excited that I ran into the limit. Then the excitement wore off and I got a little sad. Then I realized that running into the limit is a bad thing because it means my projects will break =( [Edit] A compiler option to disable the limit would be nice =( [Edit] In case anyone is interested in part of the output from building, I just found |
The limit implemented here is for 5M instantiation requests per expression/statement, hitting that regardless of project size seems... very surprising! |
I removed the
Notice that it checks in I think I could quite easily make a repo that reproduces this, if necessary. This is another sub-project. The compile-safe MySQL query builder sub-project.
It takes way longer to check, |
So, I just created a new monster query in the MySQL sub-project and built it,
It adds 7 seconds to the check time (11+7 = 18). It stays under the The generated sql string is 6k+ lines and 206,408 characters. The query selects 50+ expressions, with sub-expressions nested pretty deeply and it is all type-checked. So, I'm guessing the query is one of those "wide" types but also pretty "deep". |
@AnyhowStep I think the 5M instantiation count limit is quite reasonable. In fact, I feel it is pretty much an order of magnitude above reasonable. Any single expression or statement node that generates that much work for the type checker is highly suspect and beyond what we can promise to support with reasonable performance as the compiler evolves. |
@weswigham @RyanCavanaugh I think we should merge this. |
Checking one expression with 7.7M instantiations and hundreds of other smaller expressions in 2s isn't a long time, though. Even the 14M one checks in 7s, which isn't too bad. Couldn't the limit be higher, if dead set on a limit? I'd like to be able to trade away some performance for extra type safety. The expressions with 7M and 14M instantiations I have are a necessary part of the project that isn't at all like #31823 |
This is too large. At least you should show your actual code, to consider its validness. |
It's for work. I can probably make a repo for the 7M expression, that part isn't too revealing. The 14M expression, I'd rather only mail it to the TS team, if they want to look at it. The 6k+ line SQL query is valid. It fulfills an actual business requirement for work. Unless you mean validity in another sense. The TS code that generates the large SQL query is just a simple |
I just tried out a build containing this change. I think Right now, I have a problem where VS code is inferring the return type of my 14M function just fine. No red squiggly lines or anything. However
To create a repro is going to take me a while because there's a lot of code involved but I'll open an issue when I get to it. Just thought I'd post this comment and see if anyone here has any clue why this might happen [Edit] I think it might have to do with |
This PR adds a type instantiation count limiter. When more than 5,000,000 type instantiations occur during the checking of a single expression or statement, we report an error similiar to the maximum instantiation depth limiter.
Fixes #31823.