Document synchronization between client and server goes awry

[rchl]

Editing document produces random diagnostics errors related to missing commas and such.

Server Version

0.8.0

Terraform Version

Terraform v0.13.5

Client Version

Sublime Text (LSP plugin) v1.0.18

Terraform Configuration Files

---

Log Output

https://gist.github.com/rchl/3fc2bbaa175a9ecbafd7d3b111d87217

Expected Behavior

No spurious errors should be reported.

Actual Behavior

Random errors are reported on editing files.

Steps to Reproduce

1. Create document:

module "app" {
  service_listeners = [
    {
      hosts    = [var.service_host]
      listener = module.foo.arn
    }
  ]
}

2. Place the cursor after opening bracket: [|
3. Press enter to add new-line
4. Press tab to add more indent

This is the relevant part where with didChange notification:
a) add new-line

"contentChanges":[
    {
        "text":"\n",
        "range":{
            "start":{"character":18,"line":3},
            "end":{"character":18,"line":3},
        },
        "rangeLength":0,
    },
    {
        "text":"      ",
        "rangeLength":0,
        "range":{
            "start":{"character":0,"line":4},
            "end":{"character":0,"line":4},
        }
    }
]

b) add indent:

"contentChanges":[
    {
        "text":"  ",
        "range":{
            "start":{"character":6,"line":4},
            "end":{"character":6,"line":4},
        }
        "rangeLength":0,
    }
]

I think maybe the server doesn't correctly apply the first notification that has multiple edits.

[rchl]

I can't reproduce it in VSCode but it creates just one edit when adding newline so it might not be triggering the bug:

"contentChanges":[
    {
        "text":"\n          "}
        "range":{
            "start":{"line":3,"character":15},
            "end":{"line":3,"character":15}
        },
        "rangeLength":0,
    ]
}

[rchl]

The issue is that the server calculates DocumentChanges on the initial version of the document. It needs to calculate those changes based on the document version modified by the previous change.

So here:

terraform-ls/langserver/handlers/did_change.go

Lines 43 to 50 in 588f9d5

	changes, err := ilsp.DocumentChanges(params.ContentChanges, f)
	if err != nil {
	return err
	}
	err = fs.ChangeDocument(fh, changes)
	if err != nil {
	return err
	}

instead of calculating the changes before calling fs.ChangeDocument, it needs to do that as it modifies the document since changes reported by LSP are based on previous change.

[radeksimko]

Hi @rchl
Thank you for the report, but I wasn't able to reproduce this with 0.8.0 nor the latest version of the language server.

FWIW the ilsp.DocumentChanges you linked to above doesn't actually calculate anything. In general the whole internal/lsp package is merely responsible for converting data between the "LSP" and "HCL" format.

terraform-ls/internal/lsp/file_change.go

Lines 32 to 42 in 588f9d5

	func DocumentChanges(events []lsp.TextDocumentContentChangeEvent, f File) (filesystem.DocumentChanges, error) {
	changes := make(filesystem.DocumentChanges, len(events))
	for i, event := range events {
	ch, err := ContentChange(event, f)
	if err != nil {
	return nil, err
	}
	changes[i] = ch
	}
	return changes, nil
	}

After all the fs.ChangeDocument receives the whole slice of changes and these are then applied in the same order, one by one:

terraform-ls/internal/filesystem/filesystem.go

Lines 85 to 90 in 588f9d5

	for _, ch := range changes {
	err := fs.applyDocumentChange(&buf, ch)
	if err != nil {
	return err
	}
	}

I don't understand how this could contribute to a bug you're describing, but I do believe that there may be a bug.

---

I'm going to try to reproduce this by replaying the attached log in an integration test.

[radeksimko]

I should note though that I only tried reproducing it with Sublime Text LSP 0.14.1, which seems to be the latest available version for my version of Sublime Text (3.2.2). I will also try to reproduce in later version - or ideally - the same version. 😄

[rchl]

The "LSP" to "HCL" conversion of the changes needs to be done on top of the previous document state.

So when we have the document in state A and receive two changes that change the document from A->B and B->C, the first change needs to be "calculated" (or resolved) on state A and the second change needs to be resolved on state B. Currently, both are resolved on state A.

There is ST4 available in beta testing which I'm using on a daily basis and the LSP version available for it much further in development - LSP for ST3 is in maintenance mode. I believe LSP for ST3 doesn't support incremental changes so it likely avoids this issue.

[radeksimko]

The "LSP" to "HCL" conversion of the changes needs to be done on top of the previous document state.

Maybe I didn't explain it very well (apologies), but what I meant to say above is that the whole internal/lsp package doesn't care about any document state at all. It doesn't compute anything, it just takes the same sequence of changes and puts them into different structs.

If there is any bug with calculation of changes, then that would be in the filesystem as that's where we're applying changes.

I believe LSP for ST3 doesn't support incremental changes so it likely avoids this issue.

I see, that's a very good point - Thank you! I will try to obtain ST4. 👍

[rchl]

This is the invite to the Discord server where you'll find it in #announcements: https://github.com/sublimelsp/LSP#chat

You can also hop into #lsp where LSP developers (including me) hang out. :)

BTW. ST4 is still in kinda closed beta so it requires an ST3 license.

[rchl]

If there is any bug with calculation of changes, then that would be in the filesystem as that's where we're applying changes.

You are likely right here. I've assumed that the type conversion transforms line/character into a byte offset but it doesn't do that.

[radeksimko]

Just reproduced this in Sublime Text 4.

[radeksimko]

I've assumed that the type conversion transforms line/character into a byte offset but it doesn't do that.

You were absolutely right here, it actually does and it does look like the root cause of this bug, I must have overlooked this.

I'm working on a fix which will probably move the conversion outside of lsp into filesystem as that's the place where we can reliably compute the correct byte offset.

[radeksimko]

Fix is queued for review in #304 which also has two tests attached with the data from this issue.

[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.