Implement diagnostics

[tomv564]

For the users who use the server through their editor, project-wide diagnostics is probably pretty high on the wish list.

I hacked around a bit in diagnostics-hack, but this functionality probably needs careful design.

Do you have any further requirements (eg. should be optional?) and ideas (eg. how to convert didChange events into eventual notifications)?
Is there a minimal scope for an initial implementation?

[felixfbecker]

We definitely need this. It should definitely not poll with a setTimeout, but publish them when they become available. I am not familiar enough with the TS API to know what is provided here.

[tomv564]

VS Code also only offers diagnostics for files currently open in the editor and suggests users to run another background task (tsc -w) to get project-wide errors on save.

There is discussion and work being done to have a running tsserver instance respect the compileOnSave tsconfig.json setting at microsoft/vscode#7015

I believe the simplest implementation would behave like VS Code (only open files). Do you feel the language service should also publish project diagnostics at startup and save?

[felixfbecker]

@tomv564 that is not true, you can report diagnostics at any time. For example, the PHP language server reports them as soon as they are detected during indexing.

[tomv564]

Agreed, I was exploring why the existing TS support in VS Code doesn't send diagnostics for all files all the time. I'll try to get the same behaviour as the PHP language server if the TS API allows for it.

[tomv564]

Great, diagnostics are flowing once the first file is opened. Two considerations:

• Problems with ill-configured typings in dependencies create a really distracting amount of diagnostics.
  • Should we apply some kind of strategy to prevent huge payloads of diagnostics from being re-published (when the file in question never changes)?
• The TS service exposes diagnostics for some common tsconfig errors (eg. No files matched include pattern). We could expose these as well!

Let me know if you would like to close this issue, I can create separate issues for the above.

[felixfbecker]

Not republishing not-changed diagnostics sounds good. I'm wondering whether there is an efficient way in the compiler API to do this vs deep array comparison.

It also looks like diagnostics had a perf impact, a lot more tests are marked as slow now:
before: https://travis-ci.org/sourcegraph/javascript-typescript-langserver/builds/227149983
after: https://travis-ci.org/sourcegraph/javascript-typescript-langserver/builds/227189384

[felixfbecker]

Tracing for diagnostics would be good.

[felixfbecker]

Whole test time went from 1min to 3min, I'm pretty sure this has a significant perf impact. Definitely need tracing and optimisations in this

[tomv564]

Is there an easy to set up tool for consuming + looking at traces while developing?

[felixfbecker]

Not that I'm aware of, it would be useful to have a tracer that just prints to the terminal or serves a HTTP server locally. Wouldn't be hard to build.
You could use ctrace, which logs JSON to STDOUT: https://www.npmjs.com/package/ctrace-js

[felixfbecker]

@tomv564 do you think you can fix the performance regressions in the next days? Otherwise I will have to revert the commit (which I would like to avoid)

[tomv564]

You should see a PR with tracing support shortly, it would be super helpful if those traces can reveal where time is spent in the tests?

I have a few ideas:

• Test if requesting a subset of (instead of all pre-emit) diagnostics helps performance
• Explore if retrieving diagnostics per file helps performance
• Test if --skipLibCheck can be added for now (https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#new---skiplibcheck)

Hope we can avoid reverting, but I understand if you do!

[felixfbecker]

It should be possible with some tinkering locally, otherwise it should be possible to find out with simple logging too. I think the current pattern of retrieving and sending diagnostics for the whole program on every single re-compile is definitely not the best way to go, if it is possible to do per-file that would of course be better. And if the native way of only showing diagnostics for the open file is more performant we should go for that.

Why skipLibCheck?

[tomv564]

skipLibCheck would likely save the compiler work checking installed declarations.

Did a quick check on ensureReferencedFiles. This test was already slow and I found the following opportunities:

• Test triggers 4 syncPrograms, 2 of which do actual work (387ms and 189ms)
  • Eliminating extra compilations in these bootstrapping flows would greatly speed up these tests
  • Initial responsiveness would also improve for the end-user.
• Diagnostics are retrieved even when no compilation is done at a cost of 200-400ms each time
  • This is responsible for the performance regression
  • The getProgram() and synchronizeHostData() APIs do not give much indication if a compile was necessary / performed

Sending diagnostics is cheap compared to TS calls, so I suggest we leave optimisation there for later.
I will start by not requesting certain types of diagnostics and post the findings.

Ensuring module structure
init configuration
syncing program
syncProgram: 387.850ms
diagnostics []
getPreEmitDiagnostics: 419.914ms
updateFileDiagnostics: 0.729ms
Ensuring basic files
Adding basic file /node_modules/@types/node/index.d.ts
syncing because basic file added
syncing program
syncProgram: 2.937ms
diagnostics []
getPreEmitDiagnostics: 273.750ms
updateFileDiagnostics: 0.145ms
resolveReferences /src/dummy.ts
init configuration
syncing program
syncProgram: 189.921ms
diagnostics []
getPreEmitDiagnostics: 204.618ms
updateFileDiagnostics: 0.052ms
Ensuring basic files
Adding basic file /node_modules/@types/node/index.d.ts
syncing because basic file added
syncing program
syncProgram: 1.165ms
diagnostics []
getPreEmitDiagnostics: 290.593ms
updateFileDiagnostics: 0.084ms
resolveReferences /node_modules/somelib/index.js
resolveReferences /node_modules/@types/node/index.d.ts
resolveReferences /node_modules/somelib/pathref.d.ts

[felixfbecker]

Great investigation. I wonder why we need to "sync" the program at all (I didn't write that code) - to me it feels like the getProgram() API was designed so that getProgram() should be called only when the Program object is actually needed, which is why it compiles lazy and is fast when no recompilation is needed. Maybe we could just remove syncProgram()?

[tomv564]

Great idea to try to run without syncProgram. I think it may cause re-evaluation of how a few things are done.

getSemanticDiagnostics consumes 100% of the time getPreEmitDiagnostics takes.
As the test itself does not provide any content to validate, I believe the compiler is going through the built-in declarations instead.

Setting skipLibCheck on the CompilerOptions for the LanguageServiceHost results in diagnostics consuming < 1ms per syncProgram.
Setting the similar but deprecated skipDefaultLibCheck option instead has the same result, with the benefit that user-created and imported declarations still get checked.

Because of this overhead, I think the intended usage is to request diagnostics per file as much as possible. skipDefaultLibCheck would be a good stopgap until this logic is in place.

Relevant getSemanticDiagnostics logic: https://github.com/Microsoft/TypeScript/blob/master/src/compiler/program.ts#L918-L931)

[felixfbecker]

Sounds good, let's do it per-file then.