Rework text sync

[davidanthoff]

Fixes #564. Or at least I hope it does ;)

I essentially started from the code at https://github.com/microsoft/vscode-languageserver-node/blob/master/textDocument/src/main.ts and ported that over to Julia.

The main difference to the previous implementation is that offset is now always a valid Julia string index. The range in the edit chunk is the text that gets replaced (expressed as a Julia range that returns that text if used to index into the String). Generally the code is simplified, fewer if else stories in the whole didChange story, hopefully that makes this more robust.

I've only changed the text sync stuff over to the new offset interpretation, everything else still uses the old version. Not ideal, but I thought maybe we should first try and see whether this actually works or not...

This also replaces #582 by incorporating most of it.

I think before we invest more time into the various diagnostic PRs we created, we should maybe just try to ship this PR here in our insider build and see whether the problem just goes away. If not, we can still go back to trying to figure out what is going on with the old code.

[codecov]

Codecov Report

Merging #584 into master will increase coverage by 0.22%.
The diff coverage is 86.36%.

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
+ Coverage    28.4%   28.63%   +0.22%     
==========================================
  Files          23       23              
  Lines        1651     1673      +22     
==========================================
+ Hits          469      479      +10     
- Misses       1182     1194      +12

Impacted Files	Coverage Δ
src/requests/actions.jl	0% <0%> (ø)	⬆️
src/requests/textdocument.jl	53.18% <100%> (-0.84%)	⬇️
src/document.jl	80% <93.33%> (-7.36%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 972802d...41f7f61. Read the comment docs.

[ZacLN]

Looks fine, a couple of questions

• line_offsets2 == line_offsets + 1 ?
• I'm not clear on the reason for assignment of line_offsets before use (i.e. Make sure get_line_offsets is used #582), is this parallel prep?

[davidanthoff]

line_offsets2 == line_offsets + 1 ?

I'm not entirely sure. I just copied the code from the node implementation over and changed it to return 1 based indices. I think for the most part it is probably identical, but there are some things with min max that are in this new code that might not have been in the old version? In general, I never understood what the old line_offsets actually were, was the idea that these are just indices into the String, but 0 based?

I'm not clear on the reason for assignment of line_offsets before use (i.e. #582), is this parallel prep?

I don't think I understand the question :) No, this is not for parallel use. Do you have a specific code location in mind?

[ZacLN]

I think all the min/max additions were quick mindless attempts to prevent out bounds errors...

For the later query: https://github.com/julia-vscode/LanguageServer.jl/pull/584/files#diff-fa40c0364137c41562b5b5ea251562ccR195 is an example - we assign a local version of line_offsets - no issue, I'm just curious why

[davidanthoff]

I think all the min/max additions were quick mindless attempts to prevent out bounds errors...

True, I actually remove one of them because it was very clear that it couldn't bind :) Less clear to me whether the next_line_offset thing would ever hold, but maybe safer for now if that is also used in the reference implementation.

For the later query: https://github.com/julia-vscode/LanguageServer.jl/pull/584/files#diff-fa40c0364137c41562b5b5ea251562ccR195 is an example - we assign a local version of line_offsets - no issue, I'm just curious why

I just thought it would be safer to never access the field _line_offsets directly anywhere other than in the get_line_offsets function (and set_text!) so that we can be sure that the recompute logic kicks in when needed.

Ready to merge if you approve the PR?