contentChanges in onDidChangeTextDocument may be manipulated by other extensions

[DanTup]

I don't know if this is really a bug, but it exposed a bug in the Dart extension and I thought I should highlight it.

VS Code usually sorts the edits passed to onDidChangeTextDocument in reverse-order, so that if they are applied sequentially (without dealing with the fact that offsets are all based on the original document) they still work. Based on @alexdima's comment at #11487 (comment) I don't think this is guaranteed, but it is the current behaviour (even if I send edits in offset-ascending order, they come through in offset-descending order).

Dart was accidentally taking advantage of this - and edits are applied by the language server sequentially. Some users reported weird behaviour, which turned out to be a different extension they had installed called reverse() on contentChanges, which resulted in the edits being reversed when they got to Dart, and thus we sent nonsense edits to the language server (the result was weird phantom errors).

The "bad" extension code is here:

https://github.com/austenc/vscode-blade-spacer/blob/ae88ae1e82719c7b9fc088b93903285ac61b283a/src/extension.ts#L35

It's not really bad - in that, Dart should not have assumed the order of edits would always be reverse-offset (it's not specified to be that way), however it's an easy trap to accidentally fall into (especially as the docs do not call this out at all). It also seems strange for another extension to be able to upset things in this way.

I think the docs should at least call out in the onDidChangeTextDocument description (and the contentChanges field) that edits should not be assumed to be in an order that can be applied sequentially, or they should always be provided this way (and not allowed to be manipulated by other extensions).

(Note: I'm fixing Dart to sort them when sending to the language server - this was definitely my bug - but it'd be good to reduce the chances of others having it in future - especially when it's not so easy to track down when caused by another extension doing something like this).

[DanTup]

Oops, I hit submit too quickly before reading the dupe suggestions. This was raised a few days ago and fixed a few hours ago in:

#88310

That said - I think the docs should still be clearer here. If the intention is to guarantee the order, I think it should be documented so we know if we can rely on it.

cc @jrieken

@dbaeumer I think the LSP spec should be clearer about this too. I suspect I have a similar bug in our server there if this extension would also cause the LSP client to send the changes in the wrong order.

[vscodebot]

Thanks for creating this issue! We figured it's covering the same as another one we already have. Thus, we closed this one as a duplicate. You can search for existing issues here. See also our issue reporting guidelines.

Happy Coding!

[DanTup]

@jrieken can you clarify the guarantees here? The issue was fixed as if VS Code is going to guarantee the order, but the docs do not mention this (one way or the other). It would be better if the spec was explicit to avoid this coming up again (and it not being clear if it's a bug).