getTypeChecker returns the same checker manufactured for diagnostics …

[weswigham]

…if it has already been made

Fixes#27997

[sheetalkamat]

no.. no.. no.. There is potential that the diagnostic producing checker was initialized and later was thrown away because there was some kind of cancellation. That makes it even harder to guarantee you have correct type checker or same type checker.

[weswigham]

runWithCancellationToken resets both anyway (for exactly this reason AFAIK).

[weswigham]

dropDiagnosticsProducingTypeChecker is the only thing that could otherwise prematurely drop the checker; but despite it's presence in our internal API, it's never actually called anywhere (vestigial API?)

[weswigham]

dropDiagnosticsProducingTypeChecker is now gone (it was unused) and runWithCancellationToken specifically resets both to avoid a situation like this, so I think this is fine now.

[zzmp]

Friendly ping! Is there anything holding this back @weswigham@sheetalkamat?

[evmar]

Ping, we are curious too.

[weswigham]

@DanielRosenwasser@ahejlsberg do we want to merge this?

[DanielRosenwasser]

I don't know yet, but @sheetalkamat had some concerns and I'd like her to weigh in on this.

[ahejlsberg]

It turns out there is actually a slight semantic difference between the two checkers. We have the following line in resolveCall:

returnproduceDiagnostics||!args ? resolveErrorCall(node) : getCandidateForOverloadFailure(node,candidates,args,!!candidatesOutArray);

This means that the diagnostics producing checker always resolves failed calls to unknownSignature whereas the non-diagnostics producing checker attempts to find a best signature (presumably because that gives a better experience in the IDE). I have actually gotten bitten by this a few times because types shown in error messages end up disagreeing with types shown in quick infos.

I think we need to reconcile this before we start sharing checkers.

[weswigham]

This means that the diagnostics producing checker always resolves failed calls to unknownSignature whereas the non-diagnostics producing checker attempts to find a best signature (presumably because that gives a better experience in the IDE). I have actually gotten bitten by this a few times because types shown in error messages end up disagreeing with types shown in quick infos.
I think we need to reconcile this before we start sharing checkers.

@Andy-MS opened a PR to change this, I believe - #28564

[sheetalkamat]

@weswigham@ahejlsberg Currently in type checker there are lot for expression checks where we check if it is diagnostic producing checker and then and then only we do extra checks to ensure correct error reporting. One of the reason for two typechecker as far as I remember was that we didn't want to do extra checks from ide unless asked and yet we didn't want to loose errors (eg expression types are cached, so the checkSpecificExpression kind happens only once and we would not report errors if those types were materialized as part of ide. Note that we also want to use cached expression to ensure correct recursiveness behaviour so just doing check even if expression is cached wont help). So we need something to handle that part as well before this can go in?

[zzmp]

@sheetalkamat I don't think that will be affected.

we didn't want to do extra checks from ide unless asked and yet we didn't want to lose errors

This PR should not do extra checks from IDE, as if it uses the diagnostics producing type checker, it should have already been instantiated, checked, and cached the expressions. This also means it won't lose errors.

This won't affect the IDE because the IDE won't have a diagnostics producing type checker instantiated. Even if it did, it shouldn't affect performance because the diagnostics producing type checker (if it is returned instead of the non-diagnostics-producing checker) will have already cached expressions.

[sheetalkamat]

@zzmp That's not true. From ide we could use diagnostics producing type checker to report errors. So that assumption that it would already be instantiated and checked is incorrect. (It depends on when the errors are requested)

[zzmp]

If you're using the diagnostics producing type checker from an IDE after you're using the non-diagnostics-producing one, this PR would still have the non-diagnostics one instantiated and returned. It's only after the diagnostics-producing one has already been instantiated that it is returned for either case.

Can you reiterate why this is a bad thing? I don't think I fully understand your first comments.

[sheetalkamat]

@zzmp diagnosticProducingTypeChecker instantiated does not mean all files are type checked. So getting symbol from any file is not safe and run into problem I mentioned.

[weswigham]

So getting symbol from any file is not safe and run into problem I mentioned.

The LS already asks for diagnostics for files in an arbitrary order - how is asking for type information in an arbitrary order any different?

[sheetalkamat]

The LS already asks for diagnostics for files in an arbitrary order - how is asking for type information in an arbitrary order any different?

Yes but it uses dedicated typechecker for it. So it doesn't need to worry about symbols and types materialized by other means. Eg. consider a.ts has reported errors, user hovers over b.ts somewhere expression type is calculated and cached. Now asking errors on b.ts will have incorrect behavior about reporting errors (because the types were materialized in b,ts without type checking)

[weswigham]

Now asking errors on b.ts will have incorrect behavior about reporting errors (because the types were materialized in b,ts without type checking)

They wouldn't have been materialized without type checking, they would have been materialized within a checker that had type checking on (since the first action was asking for diagnostics). Worst case is that elaborations occur in different places - which is already true for checking files in different orders (elaborations will only appear in the first file checked with that error).

[zzmp]

Is there anything holding this back? It seems like concerns were addressed.

AFAICT, this was waiting on #28564, which seems to have been abandoned. @weswigham Is this waiting on a similar PR? Are there unaddressed concerns? Is there anything I can do to help this along?

I was hopeful about this 4 months ago as it significantly reduced checker time in most of my workflows. I'd like to see it make it in and not be abandoned.

[molayodecker]

@ahejlsberg@sheetalkamat@DanielRosenwasser Is there something we can do to help merge this Pull PR. Hope the great job done here will not be abandoned.

[zzmp]

@weswigham any update on this PR? Any way I can help to revive it? Any reason why it might be denied?

[zzmp]

@RyanCavanaugh?

[zzmp]

@weswigham@DanielRosenwasser@ahejlsberg is there anything blocking this? Is there anything that I, as an outside contributor, could do to push this forward?
I've been tracking this for 8 months now and would love to see some movement on it.

[sandersn]

@weswigham this is unblocked for 3.9 now. I'm not sure whether @sheetalkamat's concerns were ever addressed though. Can you

1. merge from master
2. see whether those concerns still apply?

[weswigham]

merge from master
see whether those concerns still apply?

1. Done.
2. It shouldn't, as I said in the comments, the API to drop only one checker is now gone (it was unused), and cancellation has always conservatively reset both checkers anyway.

[ark120202]

We've been using diagnostics-producing type checker for a while now (with an internal API), and I want to note that it might cause minor behavior changes in some rare cases. Basically, if API consumer calls some type checker API on a node that wasn't checked, it might add some new diagnostics (which is not the case with a regular type checker). Should it be considered as a bug? I've opened #34913 and #35973 (closed now), but I think I've seen more similar cases

[sandersn]

@sheetalkamat@amcasey can you review this now?

[amcasey]

I found @sheetalkamat's offline explanation for why the type checkers are not interchangeable very persuasive and I don't think this should be merged before the discrepancies are addressed.

[weswigham]

And they are?

[sheetalkamat]

Shouldn't we look at issues like #34913 and #35973 pointed out by @ark120202 before merging this.

Looking into this in detail, it seems like current change wont affect any of our current tsserver/language service or tsc --watch scenarios at all (except when doing incremental/watch compilation on program with no modules)..

1. Language service always calls for getTypeChecker after creating program, which means its always going to have work saving one.
2. Builder when creating program, always calls getTypeChecker to resolve the imports etc

So that means we don't have enough testing to find issue like already reported one..
If it is ok to use diagnostics producing typechecker instead of the one that doesnt, why is it not ok to use it all the time instead of creating two depending on order of calling?

[weswigham]

Shouldn't we look at issues like #34913 and #35973 pointed out by @ark120202 before merging this.

Sure, absolutely.

If it is ok to use diagnostics producing typechecker instead of the one that doesnt, why is it not ok to use it all the time instead of creating two depending on order of calling?

The idea is that if you never request diagnostics, we can save on work done in the checker by skipping ever producing all those diagnostics and related spans and such. So if you, say, cancel the editor's getErr query (tossing both checkers), then request completions/definitions a bunch of times, we can use the non-diagnostic-producing checker to shave some time off those queries (by not generating any expensive error messages); then on edit we toss both checkers, the next getErr request comes in and we need to generate errors for it. This time it completes before cancellation - we now have a checker primed with cached types we can use for even quicker completion/definition responses. From what I can see, it's about both doing the minimum work possible, and leveraging the work we've already done for prior requests, where possible.

The one thing we're missing, which would be nice, would be the ability to "upgrade" the non-diagnostic-producing checker to a diagnostic-producing one after the fact (so you could leverage the completions you requested for a faster following getErr request); but that'd require some lazy error generation structures that we just don't have in place.

[sheetalkamat]

So if you, say, cancel the editor's getErr query (tossing both checkers), then request completions/definitions a bunch of times, we can use the non-diagnostic-producing checker to shave some time off those queries (by not generating any expensive error messages); then on edit we toss both checkers, the next getErr request comes in and we need to generate errors for it.

But that's the thing, in most cases when operation is cancelled, next request is not going to be getErr but completions or quickinfo or something similar.. Which means we would again create typechecker that does not produce diagnostics..

then on edit we toss both checkers, the next getErr request comes in and we need to generate errors for it.

But even if getErr is first request, whenever language service creates program, it calls getTypeChecker as I pointed out in #28584 (comment) So it will use non diagnostic producing typechecker, even in that case..

[amcasey]

I think it would be very interesting to measure the perf hit of using the more expensive checker everywhere. If the optimized checker turns out to be a small win, it sounds like we can save ourselves a lot of complexity.

[sheetalkamat]

Agree with @amcasey , Sometimes using diagnostics producing typechecker seems like a bad choice considering we don't do that always because its not always performant.. LS doesn't get semantic diagnostics for all files (I think only VS does that and it is also very lazy which means it doesnt take priority) so that means if you use diagnostics producing type checker (which would be picked up only in small cases), you are likely to make extra work rather than reuse already computed work. So if its ok in some cases why is it not ok in other cases. It would just make things more complex...

Can we atleast get numbers on is diagnostics producing type checker being used at all or what percentage of times in the program during editing sessions..

[sandersn]

I'm going to close this PR to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here. It sounds like the next step is to gather more performance data.