Merge diagnosticsProducing and nonDiagnosticsProducing checkers into a single checker supporting lazy diagnostics #36747
Conversation
…a single checker supporting lazy diagnostics
weswigham
requested review from
sandersn,
ahejlsberg,
RyanCavanaugh,
sheetalkamat and
amcasey
|
I don't expect any command-line type changes, so the following should all duplicate @typescript-bot perf test this |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 3d7348f. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 3d7348f. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 3d7348f. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 3d7348f. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..36747
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at ea9dbe1. 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 There is also a playground for this build. |
typescript-bot
added
For Uncommitted Bug
| @@ -15,7 +15,7 @@ | |||
| "containerName": "", | |||
| "fileName": "/a.js", | |||
| "kind": "local class", | |||
| "name": "(local class) C", | |||
| "name": "(local class) C\nmodule C", | |||
There was a problem hiding this comment.
This new result is more correct (it accurately reflects the merge done on the symbol), however the "new" result isn't actually stable - symbol merges are ad-hoc in the checker, and the merge that produces this result is only created when the symbol for the source file is pulled on (as by diagnostic production), but not when the local class is pulled on (you can probably trigger the same inconsistency today by requesting all refs for the file followed by all refs for the class)! This is probably an amazingly subtle bug somewhere in JS checking that's been revealed by this diff, and fixing it is a separate issue. (The fix should involve making the merge no matter which entrypoint requests the symbol.)
There was a problem hiding this comment.
Opened #48197 to track the issue.
| const prototypeAssignment = symbol.exports?.get("prototype" as __String)?.declarations?.[0]?.parent; | ||
| if (prototypeAssignment && isBinaryExpression(prototypeAssignment) && isObjectLiteralExpression(prototypeAssignment.right) && some(prototypeAssignment.right.properties, isConstructorAssignment)) { | ||
| // fn.prototype = { constructor: fn } | ||
| // Already deleted in `createClassElement` in first pass |
There was a problem hiding this comment.
Likewise, convertToES6Class had an issue where a js prototype property merge sometimes happens and sometimes does not (and it breaks when the merge does occur in main). Since I didn't like crashing fourslash tests, in lieu of fixing the underlying issue (making the merge always occur regardless of what symbol is requested as an entrypoint), I've hardened the implementation to work when a symbol appears as both a .prototype member and has been merged into the actual symbol.members set (and, coincedentally, moved static members to before instance members in the generated class, which I much prefer). Just like with the only-sometimes-merged js baseline symbol, I think fixing that (existing) bug should be a separate issue.
|
@typescript-bot pack this |
|
Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
The fuzzer ran yesterday (March 22) with this PR's code instead of the usual TypeScript package, and I don't see any new issues/crashes reported. |
|
Oh boy! @RyanCavanaugh does that mean I can sync this (again) and merge it? Reviews appreciated~ |
|
Oh, also, @gabritto, does the fuzzer record memory used for each project it fuzzes in its logs somewhere? It'd be cool to know the before/after memory usage stats. |
|
cc @alexeagle since you were interested in the original issue, many moons ago 👀 |
|
Does that mean we can someday have custom diagnostics configured in |
|
Uhh, it means you should no longer have to worry about the two-checker overhead when doing both diagnostic reporting and other LS operations. |
A more complete alternative to #28584 based on the discussions there.
Rather than optimistically using an existing checker, we choose to only ever produce a single checker. That checker is responsible for both collecting diagnostics, and responding to LS queries. It does this (hopefully efficiently) by deferring calculating diagnostics for any nodes it visited during LS operations until they are later requested. This allows results from a "full typecheck" (or
getErrrequest) to then inform a following completions or quickinfo request, without needing to recalculate anything that's already been calculated. Plus, we no longer need to hold onto 2 checkers in services, which should commensurately lower services memory usage.To be as minimally breaky as possible, in situations where we know we're about to fetch diagnostics, we produce the diagnostics as they're registered, in order to preserve existing union ordering (as union order is based upon order of discovery, which may change if diagnostic production is fully unconditionally deferred until after checking is complete). This means command-line compiler runs before and after this PR should be identical (as nothing has changed when it executes). What is deferred is any diagnostics we produce while doing LS operations in the checker before we request diagnostics. These lazy diagnostic requests are backlogged until a diagnostic (or suggestion) request is made, at which point they are executed and produced. This means that errors made via the IDE might have different union ordering in their types than errors made on the command line (if you have prior LS results executed that caused a partial load of other types in a different order). But that's likely a fine tradeoff to make; especially since the order already differed depending on how files were included/checked in the IDE, anyway.
Do note: Only diagnostics which depended upon the old
produceDiagnosticschecker flag become lazy in this PR - further work should probably be applied to make more diagnostics lazy, as there are undoubtedly heaps of other diagnostic code that was unconditionally done, which could probably be made lazy to improve LS performance slightly.Tl;DR: With this change, there can be only one checker. Use
addLazyDiagnosticto register diagnostic-specific work in the checker.Fixes #27997