Provide a simpler Geterr api in tsserver

[ananthakumaran]

Currently, tsserver exposes error checking functionality only via GeterrRequest. The semantics is that the client should send a list of fileNames to the server and the server will emit two types of events (semanticDiag, syntaxDiag) for each file if that file has those types of errors. This API creates too much complexity on the client-side unless the editor is modeled in a specific way.

1) It breaks one request => one response model that is used for all the other API calls 2) If any file has changed in the meantime, all the pending events are canceled. Effectively the client has to keep track of the last error event received and redo the Geterr request for the rest of the files. 3) There is no response in case of no error. So the client has to resort to some timeout mechanism.

For https://github.com/ananthakumaran/tide I have decided to patch the tsserver instead of handling all the above mentioned problems within tide codebase. Though it works well, it restricts tide to a specific typescript version. related issues https://github.com/ananthakumaran/tide/pull/22, https://github.com/ananthakumaran/tide/pull/21

[mhegazy]

check this PR out https://github.com/Microsoft/TypeScript/pull/8296

[ananthakumaran]

Well, that PR seems to go in the opposite direction. The problem is tide doesn't do the error display by itself. Some other framework(flycheck) does it and tide just provides it with the information. The framework decides when to do the error check(based on configuration like after newline or after save etc) and calls tide with the filename and expected to be called back with the information.

The Geterr api seems to be catered to a specific model. I am looking for a simpler api, which just gives back the semanticDiag and syntaxDiag errors based on the current state of the file.

[zhengbli]

The problem is that diagnostics checking is expensive and blocking, and asking for them could stop auto-completion etc. It is also likely that before the checking finished the user changed the files, making the rest of the checking not reliable. Is it possible for tide to just listen to the diagnostics events and report to your framework when it received something?

[ananthakumaran]

If I understand correctly, no events are emitted in case of no error. So let's say user makes some changes, saves the file.

1) if the file has errors, tsserver would emit error events and tide could respond back to the framework 2) if the file has no error, tsserver would do nothing. Now tide has to wait for a predefined time before it can respond back to the framework. It would very likely create a bad editing experience because the errors won't go right away.

btw, how does tsserver figure out the right time to do the error checking? Won't it block if the client makes an autocompletion request right after the setTimeout triggers the diagnostics checking?.

[zhengbli]

So in the PR, the tsserver recomputes diagnostics when:

1. One of the files' content was changed
2. A project reload was just finished

And if the file has no error, it would send out an event with zero diagnostics, so the host would know when to clear out the error messages.

The server doesn't process the entire project in one run. It processes one file at a time, then queue the next file via a setTimeout. Therefore if there are small things like get completion in between, they won't need to wait until the whole thing finished.

If possible, it might be better for tide to listen to the events in a child-process of some sort instead of waiting for a fixed amount of time after file changes, as that might have unpredictable performance.

[ananthakumaran]

How would tsserver prioritize which file should be checked for errors first? It's not uncommon to have a lot of files opened at a time as a user navigates through a project, but a user would mostly care about the one or two files that are visible and want to know the diagnostic information of those files first. As I see it, there is no prioritization happening in tsserver.

[zhengbli]

Normally we only check errors for files currently opened by the editor, which is not likely to be that many, and I do think they all have pretty high priority. If asked for a through project-wise check via the GetErrForProject command, we have some simple prioritization here, basically:

.ts files currently open > other files currently open > .ts files not open > .d.ts files not open

[ananthakumaran]

Normally we only check errors for files currently opened by the editor, which is not likely to be that many, and I do think they all have pretty high priority.

It might be true for some editors, but not in the case of emacs. Because emacs buffers are not resource heavy and the hidden ones don't clutter the UI in any way like the normal tabs do, emacs users tend not to kill buffers, but merely switch to new ones as they work. I have nearly 50 files opened now.

var x = "hello";
x.|

Users normally want to control when the error check happens. Doing it after every change ChangeRequest or ReloadRequest might not be ideal. Consider the example above, we definitely want to sync the file to get proper completions, but unlikely we want to do an error check at this point.

I have been using this patch for some time and don't experience any problem due to blocking (I guess it might be because it always requests diagnostics for a single file at a time).

May be another api can be added to get diagnostics for a single file, so the client can decide which one to use?

[zhengbli]

In the scenario as you described, it makes sense to let the host decide which files to check, as it seems that the "opened file" carries a different meaning compared to other editors. Given the different scenarios we have seen so far, it might be better to add more flexibility.

It boils down to two key factors:

• When to check error: This can be designed as one of the configure options that passed to the server upon start via the Configure command. We can add a configuration option something like ErrorCheckMode, which can be either Automatic (server decides when to check errors) or Manual (host decides).
• Responds error messages as events or normal responses: This should only be configurable when the aforementioned ErrorCheckMode was set to Manual. And this can be configured per request, meaning if the requireResponse property of a GetErr request was set to true, then the consequent error checking will be blocking, and will return as a normal response. Otherwise it is non-blocking, responding as events, and will be cancelled if any file content was changed.

@ananthakumaran @mhegazy @dbaeumer thoughts?

[ananthakumaran]

👍

btw, while we are at it, can you add a property akin to request_seq in event response. Right now there is no easy way to figure out which GetErr request triggered which event.

[dbaeumer]

👍

Actually the language server protocol actually only defines the automatic behavior.

[zhengbli]

Now two new APIs: SemanticDiagnosticsSync and SyntacticDiagnosticsSync are added.