Store and check deferred nodes by containing file#27378
Conversation
| const id = "" + getNodeId(node); | ||
| deferredNodes.set(id, node); | ||
| links.deferredNodes.set(id, node); | ||
| } |
There was a problem hiding this comment.
Should we assert in the else case that deferred node already exists?
There was a problem hiding this comment.
No. We are perfectly fine adding the node multiple times (and in fact we will in many cases), we're still only visiting it once for errors in the end. That's why we swapped to a map in the first place, instead of an array.
There was a problem hiding this comment.
I meant when file was typechecked? The assert would be to verify the deferredNode exists in the map.
There was a problem hiding this comment.
Seeings as previously if deferredNodes didn't exist, we'd quietly do nothing, I don't think an assert is quite correct. And experimentally, it looks like such an assert is only triggered during a full typecheck walk after a normal check (like in tests) under scenarios with erroneous syntax, such as a top-level return statement.
Fixes #26680
During checking, we collect deferred nodes which we do not want to perform analysis on until later. These nodes may not actually be in the file being checked (the only time we care about deferred nodes), since a type in one file can pull on an expression in another. In an extreme case, a single deferred node in one file can pull on literally every other type in the program in a deferred fashion - causing us to check all of those nodes when checking the original file. Add in a bit of mutual recursion, and you have a situation where we end up rechecking the same deferred nodes at the end of every file (or even just a subset, which is still quite bad), because every file "depends" on them. To avoid these redundant checks, I now store all deferrals onto a list unique to the source file the node is contained within, provided that file hasn't been checked yet. (If it has, the node should have already been checked in a deferred way and there's nothing to do.)
As this is a perf issue, I don't really have a regression test for this (testing perf issues that aren't an OOM are not something we're equipped for), but here are some charts:
Before:
After:
This is to say, this reduced the error update for the active file in the editor for this repro down from almost two seconds (sluggish, effectively a whole program check just to get updated errors for one file) to around a quarter second (where message serialization time takes a bit of overhead).