langserver: TextEdits computed from diff for gofmt

[keegancsmith]

Before this commit we would send an edit which deleted everything followed up by
inserting the formatted code. This lead to a bad experience in vscode. This
change implemented a more minimal number of TextEdits by using difflib. The
behaviour now matches the behaviour of vscode-go.

WIP Known Issues

• Trailing whitespace is not completely removed
• test harness needs updating
• more testing in vscode
• order of textedits may be incorrect

Fixes #76

[keegancsmith]

I've tested this functionality a bunch. However, I've disabled the formatting test and I need to file an issue on vscode for the deletion of trailing whitespace. The reason I disabled the test is I need to make some changes to the test harness to make it possible to test, but I have done plenty of manual testing in vscode.

[emidoots]

Neat! No complaints from me. Sorry for the late review!

[keegancsmith]

@slimsag no worries, I've been wanting to adjust the implementation anyways to avoid the vscode bug.

[keegancsmith]

@slimsag This PR is still very close to completion. The issue I was experiencing with it was around trailing whitespace and removing it. I don't think I'll be able to finish it off soon though, if you are interested in looking at it?

[ramya-rao-a]

@keegancsmith @slimsag Any updates on this?

One update from VS Code is that, now even if you send the original edit that deletes everything and adds the new output, VS Code will take care of calculating the minimal edits required.

[keegancsmith]

Great, yeah this PR has been on the backburner a long time (more than a year)! I'll see how it works in vscode today, previously I was running into a bug around text edits and trailing whitespace in a file.

One potential issue of enabling this functionality is that it just uses gofmt. However, I assume some users want goreturns or gimports instead?

[keegancsmith]

@ramya-rao-a the current behaviour of the go-langserver is to send an edit which deleted everything followed up by inserting the formatted code. So it seems that is good for you? This PR is about sending minimal edits ourselves.

[keegancsmith]

Just tried this again, and getting some errors. I added three empty lines then formatted and got these trace logs. Likely bugs in our implementation

--> notif: textDocument/didChange: {"textDocument":{"uri":"file:///Users/keegan/go/src/github.com/sourcegraph/go-langserver/langserver/cache.go","version":2},"contentChanges":[]}
--> notif: textDocument/didChange: {"textDocument":{"uri":"file:///Users/keegan/go/src/github.com/sourcegraph/go-langserver/langserver/cache.go","version":3},"contentChanges":[{"range":{"start":{"line":158,"character":0},"end":{"line":158,"character":0}},"rangeLength":0,"text":"\n"}]}
jsonrpc2 handler: notification "textDocument/didChange" handling error: received textDocument/didChange for invalid position {'\u009e' '\x00'} on "file:///Users/keegan/go/src/github.com/sourcegraph/go-langserver/langserver/cache.go": file only has 158 lines
--> notif: textDocument/didChange: {"textDocument":{"uri":"file:///Users/keegan/go/src/github.com/sourcegraph/go-langserver/langserver/cache.go","version":4},"contentChanges":[{"range":{"start":{"line":159,"character":0},"end":{"line":159,"character":0}},"rangeLength":0,"text":"\n"}]}
jsonrpc2 handler: notification "textDocument/didChange" handling error: received textDocument/didChange for invalid position {'\u009f' '\x00'} on "file:///Users/keegan/go/src/github.com/sourcegraph/go-langserver/langserver/cache.go": file only has 158 lines
--> request #1: textDocument/formatting: {"textDocument":{"uri":"file:///Users/keegan/go/src/github.com/sourcegraph/go-langserver/langserver/cache.go"},"options":{"tabSize":4,"insertSpaces":false}}
<-- result #1: textDocument/formatting: null

[ramya-rao-a]

I assume some users want goreturns or gimports instead?

Yes, we should come up with a good model of sending configurations over to the LS

The choice of formatting tool is one
The choice of passing flags to the formatting tool is another.
The choice to whether or not include function snippet when auto-completing (from the other PR for auto-completion) is another
Once we have linting/vetting support, they would need their own set of flags
And so on

[lloiser]

Is there anything left here that prevents this from being merged? Looking at the linked issues above it might improve things :)

[lloiser]

I can have a look at the merge conflicts later today if you want :)

[lloiser]

@keegancsmith here is a patch.txt to fix this branch :)

[dianedanlai]

I look forward to the changes!! May I know if we will have the change in master by this week?

[keegancsmith]

Apologies I meant to get to this this week. I have scheduled some time on Tuesday to tackle this.

[dianedanlai]

@keegancsmith Thank you very much!!