fix(lsp): handle mbc documents properly

[kitsonk]

Resolves #9018

This PR refactors the way in memory documents are handled and changed, specifically properly handling the way vscode and tsc handle text positions in documents which now properly handles unicode characters which have > 2 bytes for encoding. It also consolidates the way that TypeScript diagnostics are collected. There are other minor improvements and changes as well.

[ry]

Fixed a couple lint errors to save you some time tomorrow.

Patch looks good.

Please check that the tests provided by @uki00a in #8953 are included (for example cli/tests/lsp/did_open_notification_mbc.json looks useful and I didn't see that here)

[lucacasonato]

Just tried this out with VS Code Canary 0.0.6 (debug build) in https://github.com/lucacasonato/deno_s3, but after a few seconds of everything working fine (maybe before the extension loads? extension & lsp are loaded at this point, and is responding to requests) the whole LSP becomes unresponsive and the deno lsp process pins my CPU. This can be 1. seen by the laptop fan going into overdrive, and 2. the hover not responding anymore.

After about 3 minutes everything calms down again, and the LSP becomes responsive again. Something is going on here, because deno_s3 is not a very large repo.

[lucacasonato]

It seems you can get it to reliably happen by hovering on a imported item in an import statement.

[kitsonk]

@lucacasonato it feels like it is some sort of deadlock issue with the hover stuff... I am experiencing a similar problem. I am looking at it now.

[kitsonk]

@ry I had unit tests for the issues I encountered that went above the other PR, but I have added an integration test that is even more in depth that what was in the PR. It opens the document, edits it at a point where we had problems before and then hovers in another location and validates the hover reflects the edit properly.

@lucacasonato I believe I have fixed the dead-lock issue. We were fetching all line index async, even when ones for open documents should be retrieved sync, which meant we ran into a dead-lock issue between handling the request and tsc, which both need access to line indexes.

[lucacasonato]

Awesome. That hover deadlock seems to be fixed. I have found a new issue though (happens when hovering in deno.ns lib:

[Error - 1:25:06 AM] Request textDocument/hover failed.
  Message: An unexpected specifier (asset:https:///lib.deno.ns.d.ts) was provided.
  Code: -32602 

I also encountered another deadlock that occurred after I tried out some commands, and then did Goto Implementation on the DeleteObjectResponse interface in /src/types.ts of https://github.com/lucacasonato/deno_s3. I can not reproduce it anymore now, but this feels like a race condition somewhere in the Goto Implementation.

[kitsonk]

@lucacasonato I know what the hover problem is... I forgot we can have assets open.

I will take a look at Goto Implementation too...

[kitsonk]

I've fixed the problem with the assets, but I have tried a couple other projects and on a debug build I can pretty easily get it to hang. I have a theory that something before was preventing us being "thread bound" with tsc, but now I think we might be trying to send multiple requests into tsc at the same time, causing hangs. Either way, I need to add some instrumentation to see where we are really hanging.

[ry]

It would be very nice if we could reproduce this hang in the tests before we fix it.

@kitsonk Maybe we can pair program this later today

[kitsonk]

It would be very nice if we could reproduce this hang in the tests before we fix it.

I can try... We need to narrow down where it is exactly happening so it can be reliably reproduced... Right now it requires clicking around quickly in an editor. Sometimes it just hangs for 10 seconds or so, sometimes I lose patience.

[kitsonk]

Thanks to pairing with @ry I think I have discovered the issue and fixed it. It is rather complex:

• We implemented text snapshots in our integration with TypeScript to speed up when documents change.
• To implement snapshots, we had to implement the ability to produce a change range between two versions of a document. This allows tsc to only request the parts of a document that have changed from one version to another.
• We use a different versioning scheme between when a version is open in the editor and when a version is cached on disk.
• When you click through on a document that was in cache and you open it in the editor, the version changes, but the content doesn't.
• TypeScript doesn't know that the versions aren't different, so it asks Rust for any changes.
• Originally we used dissimilar to calculate the changes, but it does not handle certain mbc characters properly, so this PR switched to difference.
• Difference is really really slow compared to dissimilar. So much that when it was comparing the saved version to the open version on any reasonably sized file, it would cause a deadlock on the sources, which would block everything.
• Putting in a short circuit if the strings are equal fixes the issue when opening documents, but the performance of difference is so poor, that if you start editing a large-ish document, it is quite easy to cause the deadlock again.

So I think we are going to have to revert to dissimilar and know that it will not handle certain documents properly (though it is really an edge case that will cause the problem) and figure out how to resolve the dissimilar issue.

I think I will be able to recreate a test failure now by editing a large document and then requesting hovers on it.

Other points we discussed:

• the sources reads files from the disk cache synchronously... we might want to convert it to async so it can be less likely to block.
• we need some level of instrumentation built into the lsp so we aren't randomly adding eprintln!() to try to figure out where things are going wrong.

[ry]

LGTM - I defer to @lucacasonato for final review.

[lucacasonato]

Tested this locally, and everything works without hangs now. Great job! I think this abstraction is a lot simpler, and more useful.

LGTM