Partial update

[njuCZ]

fix #34

add support for file partial update.

• personally I want to make func (fc *fileChange) Range() hcl.Range to return a pointer, which is convenient to check whether to apply full content replace or partial update, but it seems need to change the interface and other parts, please confirm whether I should do in this way.

[hashicorp-cla]

All committers have signed the CLA.

[radeksimko]

Thank you for the PR @njuCZ !

It looks pretty good on the first sight, but I will read it more thoroughly 🔜

One other test case that may be worth adding is text with some unicode characters, to ensure that byte-position is calculated correctly in such case too. The logic may be error-prone in this context especially because LSP uses UTF-16 and HCL is always encoded as UTF-8. I'm not saying it is a problem here, but the encoding differences is something worth extra attention.

I also want to try this out with a few clients/IDEs which support it, just as a sanity check.

personally I want to make func (fc *fileChange) Range() hcl.Range to return a pointer, which is convenient to check whether to apply full content replace or partial update, but it seems need to change the interface and other parts, please confirm whether I should do in this way.

I can see why that would be convenient in this context, but also I think that there is some value in not using pointers as they can lead to crashes, when nil is passed somewhere unexpectedly where there isn't explicit nil check. Since the interface doesn't have consumers outside of the filesystem package, then it's probably fine though.

I also tried to keep it aligned with interfaces within HCL itself, which practically doesn't use *hcl.Pos anywhere. Not that consistency here is a strong argument, but I thought I'd mention it for completeness.

[radeksimko]

In addition to the inline comments:

• There is a few tests failing in langserver/handlers.
  • Perhaps we should look into centralizing the server capabilities struct somewhere and just marshal it into the test data to make these tests less fragile as I imagine we will be adding many new capabilities often and it feels wrong to keep fixing these tests because of that. - but I'm happy for that to be addressed in a separate PR.
• As discussed via Slack I'm happy for you to merge fileChange and testChange in internal/filesystem tests
• I would prefer us to avoid exporting fileChange out of the filesystem package
  • Maintaining interface is usually easier (from compatibility perspective) than maintaining a struct and it also makes handling of internal state more difficult, since we'd either need to add some more methods and/or constructor, export all fields etc.
  • Interfaces can help avoiding import cycles as they could be more easily extracted into their own package with very few dependencies, as opposed to any real struct/implementation.
  • A little copying is better than a little dependency

[radeksimko]

One more note - there is a bug potentially affecting updates near EOF which I fixed in a397452 - feel free to cherry-pick that to your branch until #104 is merged.

[njuCZ]

@radeksimko I have refined the codes according to your suggestions. Please have a look again

[radeksimko]

There are few things we discussed outside of GitHub, which I'm going to describe here just for posterity.

---

(1) The fork of go-lsp prevents us from unmarshaling version from the client request, as I also described here.

I have created #117 and #118 to discuss the problem in more generic way, but I reckon that will take some time & effort to tackle, so I'd suggest you to just create a new struct in internal/lsp - basically a copy of the go-lsp's structs - we can switch back to whatever library we decide to use later, when we have one that doesn't suffer from the flaw described.

---

(2) When partially updating a document, the ordering of requests/responses matters and we need a way of guaranteeing that.

I believe the best way to address this would be implementing a priority queue when applying any partial changes to a document which would be taking changes in the form of filesystem.FileChange and executing them in the right order in applyChange. Basically applyChange could be reading changes from the queue and retrying (with some back-off) if the version difference is >1.

Non-partial changes should not fall into any queue though - they should IMO be processed the exact same way they are processed now and keep benefiting from the simple parallelism.

There is a few edge cases related to this approach worth discussing - see below.

Missing versions

This may occur e.g. when the server crashes mid-flight and starts again, while the client still preserves versions of the document.

e.g. server would receive versions 1, 2, 4, 5, 6

Presumably we'd be queueing 4, 5 and 6 while waiting for 3 to arrive, but we can have a timeout at which we'd just error out and display and maybe prompt the user to reopen the file as we have no way of updating it consistently via a complete sequence of updates.

I'm not sure what's a sensible timeout, but I would be conservative and keep it pretty low as I assume that document updates should generally be quick.

Duplicate versions

This could happen due to misconfiguration where e.g. client runs in 2 or more instances while talking to the same server in the same session, or simply client sending the same request twice for any reason. This would more often be a bug on the client side, but I've seen that happening a few times and we have to reflect it somehow.

e.g. server would receive versions 1, 2, 3, 3, 4

We could just ignore the second 3 in that sequence, assuming that the previous 3 was already applied and we know that the next expected version is 4 - the mutex already guarantees that no two versions would be applied at the same time. We should however still surface this somehow to the user, so they can report it as bug to the language client.

---

I appreciate with the above this would probably grow into much bigger chunk of work you may originally expected and I'd be happy to take it over if you don't feel confident about implementing the proposal - just let me know. Also I'm open to alternative ideas!

Either way thank you for all your work so far, which helped uncover these bugs 🐛 🙏

[radeksimko]

#120 resolves the (2) problem in slightly different and far less complex way - pretty much the way you proposed originally via Slack 😄 - by setting concurrency to 1. I think the strong factor was going through the pros and cons - which I didn't do originally (sorry about that). Either way #120 hopefully explains the context of that decision.

The only major problem left to solve here seems to be (1) and I think we could take the shortcut I proposed - just add a new struct to internal/lsp - then we can later replace it with proper longer-term solution per #117 and #118

[njuCZ]

@radeksimko thanks for your suggestion. I have refined the PR, please have a look again

[radeksimko]

This is looking pretty good and it looks like it already helped me discover a bug in coc.nvim which skipped a version completely, see log. I will report it later upstream.

I left you mostly minor comments inline, the only major things is the error handling and version incrementing, otherwise it works well!

[njuCZ]

@radeksimko thanks for your review. Hope the version missing issue is rare for most editors. Besides I wonder it's the version 9 message lost or just the editor send the wrong version (not strictly incremental)

[radeksimko]

I wonder it's the version 9 message lost or just the editor send the wrong version (not strictly incremental)

🤔 I would argue that such behaviour would be fine for non-partial updates, but since partial updates send deltas which need to be applied in the right order then there should be a way of guaranteeing that order and consistency.

[njuCZ]

agreed, maybe we could have a whitelist to decide partial update or full update or make a config for user to choose?

[radeksimko]

What I meant by way of guaranteeing order and consistency is that I assume this is built into the protocol and we're essentially just adding guardrails to enforce this assumption. If it turns out that assumption is wrong (e.g. because we receive a bug report) then we can react accordingly and apply different assumptions, but I think this gives us a starting point.

maybe we could have a whitelist to decide partial update or full update or make a config for user to choose?

I think the decision whether to use partial updates should be between the client and the server and how they communicate their capabilities. Such settings would be also quite difficult to communicate to the user. Most users are IMO looking at settings generally from the perspective of "getting things done" and from that perspective they may not be aware of how LSP works nor that there is even such thing as "updating document". How document is updated in memory is an implementation detail for them and it I think it should stay that way.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.