Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: new code disappears or is modified after formatting #34955

Closed
stamblerre opened this issue Oct 17, 2019 · 31 comments
Closed

x/tools/gopls: new code disappears or is modified after formatting #34955

stamblerre opened this issue Oct 17, 2019 · 31 comments
Assignees
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Oct 17, 2019

A number of VSCode issues have been raised regarding problems with formatting (see microsoft/vscode-go#2824, microsoft/vscode-go#2787). There are a few possibilities about what the source of this issue might be, but it is likely that either, (1) formatting is being run on old code, or (2) something is going wrong with the diff algorithm.

If you have this issue, please share the following information so that we can diagnose it:

  1. Your VSCode settings (Ctrl + Shift + P -> Preferences: Open Settings (JSON))
  2. Your gopls version (gopls version)
  3. The output of gopls -rpc.trace -v format path/to/file.go
  4. Your gopls logs from when this issue occurs (see https://github.com/golang/tools/blob/master/gopls/doc/troubleshooting.md#capturing-logs for information on how to capture logs).

Furthermore, we have recently substituted the gopls diff algorithm for a better alternative. It's possible that this was the culprit. If you are able to install gopls at master:

$ GO111MODULE=on go get golang.org/x/tools/gopls@master golang.org/x/tools@master

then you can try out this new diff algorithm. Try it by adding the following setting:

"gopls": {
    "go-diff": true
}

If any users test out go-diff, please do report if this changes anything.

Finally, it will be helpful to determine if the issue is formatting or imports. Please try disabling the following settings one at a time and please report which alters the behavior.

"editor.formatOnSave": true,
"editor.codeActionsOnSave": {
    "source.organizeImports": true,
}
@jpvillaseca
Copy link

@jpvillaseca jpvillaseca commented Oct 22, 2019

The issue ocurred with the gopls@(devel), and the normal v0.1.6 version. Also with the v0.1.7 version suggested by vscode-go.

It's worth noting that the type of "code rewrite" is different with the go-diff: true and without it.
Without it, it discards the current changes and repeats the last couple of lines of the code.
With it, it discards the first columns of the current file. For example:
Before saving:

	// insert in the database
	client.generateID()
	client.hub.register <- client

And after saving:

/ insert in the database
	client.generateID()
lient.hub.register <- client

What I have to do to recover from this is to undo the 2 edits made by the formatter, and then execute "reload window".

gopls version:

golang.org/x/tools/gopls v0.1.6
    golang.org/x/tools/gopls@(devel)`

Vscode settings.json:

{
	"workbench.startupEditor": "newUntitledFile",
	"extensions.ignoreRecommendations": true,
	"tslint.jsEnable": true,
	"editor.formatOnPaste": true,
	"editor.formatOnSave": true,
	"editor.codeActionsOnSave": {
		"source.fixAll.tslint": true
	},
	"typescript.updateImportsOnFileMove.enabled": "always",
	"editor.detectIndentation": false,
	"editor.insertSpaces": false,
	"zenMode.hideTabs": false,
	"editor.renderWhitespace": "boundary",
	"workbench.iconTheme": "vscode-icons",
	"bracketPairColorizer.showHorizontalScopeLine": false,
	"bracketPairColorizer.showBracketsInRuler": true,
	"vsicons.dontShowNewVersionMessage": true,
	"explorer.confirmDragAndDrop": false,
	"files.watcherExclude": {
		"dist/**": true
	},
	"files.exclude": {
		"**/dist/**": true
	},
	"search.exclude": {
		"**/dist": true,
		"**/docs": true
	},
	"workbench.colorTheme": "Monokai",
	"terminal.integrated.shell.windows": "C:\\Windows\\System32\\cmd.exe",
	"git.path": "C:/Users/noflo/AppData/Local/Atlassian/SourceTree/git_local/bin/git.exe",
	"editor.mouseWheelScrollSensitivity": 3,
	"go.useLanguageServer": true,
	"gopls": {
		"go-diff": true
	},
}

Output of gopls window:

[Info  - 9:21:14 AM] 2019/10/22 09:21:14 c:\Users\noflo\go\src\jarvis-server\app\session does not have prefix C:\Users\noflo\go\src
[Info  - 9:21:14 AM] 2019/10/22 09:21:14 go/packages.Load
	packages = 1
[Info  - 9:21:14 AM] 2019/10/22 09:21:14 go/packages.Load
	package = builtin
	files = [c:\go\src\builtin\builtin.go]
[Error - 9:21:15 AM] Request textDocument/definition failed.
  Message: no identifier found
  Code: 0 
[Error - 9:22:42 AM] Request textDocument/definition failed.
  Message: no identifier found
  Code: 0 
[Error - 9:23:43 AM] Request textDocument/definition failed.
  Message: no identifier found
  Code: 0 
[Error - 9:25:08 AM] Request textDocument/definition failed.
  Message: no identifier found
  Code: 0 

The output of gopls trace is on this gist:
https://gist.github.com/jpvillaseca/8a63cb8ca3f5e5343a5b28c91b4f423d

EDIT:
I tried setting this, but the formatting error appeared after a while.

"editor.codeActionsOnSave": {
    "source.organizeImports": true,
}
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 22, 2019

@jpvillaseca: Thank you for the detailed report! What is the output of gopls -rpc.trace -v format path/to/file.go?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 22, 2019

Oh I'm sorry I see you've attached it. I will look into this more thoroughly ASAP.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 22, 2019

To get more detailed gopls logs, you can add:

 "go.languageServerFlags": [
    "serve",
    "-rpc.trace",
],

to your VSCode settings. Based on the trace you attached, it seems like the file you were trying to format already had syntax errors and wasn't formatted. Can you try formatting a file that doesn't have any syntax errors?

@jpvillaseca
Copy link

@jpvillaseca jpvillaseca commented Oct 22, 2019

Thank you, @stamblerre .
Oh, the problem was that the file shouldn't have had syntax errors since they were introduced during saving, and I couldn't repair the file within vscode. To get those traces from gopls in that situation, when the issue resurfaces, I'll have to edit the file in another editor, fix the issues there and save the file, and run gopls trace. Because I can't do that from vscode. Stay tuned 😅

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 22, 2019

Ah I see - makes sense. Thank you for doing this - it will be very helpful to get this information!

@jpvillaseca
Copy link

@jpvillaseca jpvillaseca commented Oct 22, 2019

Ok. The verbose trace from within vscode is:
https://gist.github.com/jpvillaseca/8cdc67772db6a40d8f1dfb8f190c49bf

And the output of the gopls trace command on the file without issues:
https://gist.github.com/jpvillaseca/b62aee8c5094d0f150631473e5154a1a

(This is without go-diff: true)

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 22, 2019

Do you mind sharing the same but with go-diff: true? At this point I think we will definitely be migrating to the go-diff algorithm in the next release.

@jpvillaseca
Copy link

@jpvillaseca jpvillaseca commented Oct 22, 2019

Of course. Here are the traces with go-diff: true
It was easy to trigger the issue: I set the go-diff: true option, restarted vscode, saved an open file from the workspace (the same from the previous post), to trigger a format document and it crippled the following line of code:
Before saving, this is line 18:

var db *gorm.DB // gorm handles the connection pool using the recommended approach for each database engine

After saving. It ate the comments and some words. I had to do 4 undos to get back to the working version, but it is already crippled on-disk.

var db *gorm.DB  pool using the recommended approach for each database engine

Here is the trace from vscode:
https://gist.github.com/jpvillaseca/9187d9c3f9aebe92e87c9da837b3ca19

And this is the trace from gopls: (I reverted the file to the original version using notepad)
https://gist.github.com/jpvillaseca/45a285c72b15b5de5efb99e567c3cb62

I hope this helps =)

EDIT: Another example with the same file, but now it started replacing the first columns of some lines:
https://gist.github.com/jpvillaseca/b1ad7e13e3a73545244a0eb875ee0638
Such as:

var db *gorm.DB 

// Config stores database connection parameters
var Config DatabaseConfig

Was crippled to:

r db *gorm.DB 

// Config stores databaseonnection parameters
r Config DatabaseConfig
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 24, 2019

Thank you so much for this report. Definitely very unexpected behavior. @ianthehat: Any ideas here?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 24, 2019

@jpvillaseca: Were you able to determine if this happens as a result formatting or organizing imports? That is, can you reproduce it by right-clicking and selecting Format Document? Or does it happen when you remove an import and then do Ctrl + Shift + P -> Source Action -> Organize Imports? Or does it happen in both cases? Also, does it happen consistently or only on occasion?

@jpvillaseca
Copy link

@jpvillaseca jpvillaseca commented Oct 25, 2019

It seems organize imports and format document are both culprit.

I added an intentionally failing import, run the Organize Imports command and it triggered the issue: Vscode gopls trace: https://gist.github.com/jpvillaseca/e943fb6d9e865184d3722735d6c4e30d

And also the same with Format document. The characters erased from the file are from the last lines. Both triggered the same types of artifacts.
Vscode gopls trace: https://gist.github.com/jpvillaseca/7bd712337a2060ec116f1c5fb38a25c1

The original last 17 lines from the file:

// CheckForStalledJobs looks for any dangling jobs that didn't report progress on time
func CheckForStalledJobs() {
	client := redispool.GetClient()
	if client == nil {
		return errors.New("Unable to get redis client")
	}

	// for each job in the in-progress list look for their entry in the inprogress list
	inprogressJobs := jobs.LRange("inprogress-jobs", 0, -1)


	// if not there, mark it as retry and move it to the queue or cancel it if it exceeds the retry threshold
}

// func DequeueJob() job *jc.Job {
// 	// Look for a
// }

And after running either command: (it even ate the "b" from DequeueJob in the commented out section, or the "e" in "entry" in another comment, and some spacesand curly braces in between

// CheckForStalledJobs looksfor any dangling jobs that didn't report progress on time
func CheckForStalledJobs() {
	client := redispoo.GetClient()
	if client == nil {
		eturn errors.New("Unable to get redis client")
}

	// for each job in the in-progress list look for their ntry in the inprogress list


	/ if not there, mark it as retry and move it to the queue or cancel it if it exceeds the retry threshold


// func DequeueJo() job *jc.Job {
// 	/ Look for a
// }
@jpvillaseca
Copy link

@jpvillaseca jpvillaseca commented Dec 17, 2019

Hello @stamblerre I know this issue is still marked open. But wanted to test the issue in the latest version of gopls 0.2.2. It still persists, the behavior remains the same.
Additionally, I started increasing the editor.codeActionsOnSaveTimeout, but it doesn't prevent the issue

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Dec 17, 2019

Can you try reproducing on the current master? v0.2.2 was just a small patch release with only 2 small fixes.

@dhcgn
Copy link

@dhcgn dhcgn commented Dec 27, 2019

I have the same problem: https://streamable.com/69fgs

I would be happy to help with this issue.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Dec 27, 2019

@dhcgn: Can you see if this reproduces for you at master? You can download gopls at master by running GO111MODULE=on go get golang.org/x/tools/gopls@master golang.org/x/tools@master.

@dhcgn
Copy link

@dhcgn dhcgn commented Dec 27, 2019

@stamblerre the behavior changed, now I can save the file without it will be corrupted but without auto-format when saving a file. Auto-Succession was broken too.

How can I get the gopls from master and keep the integration in vscode working?

After I update the go tools the old behavior is back again:

go.toolsGopath setting is not set. Using GOPATH C:\Users\danie\go
Installing 18 tools at C:\Users\danie\go\bin in module mode.
  gocode
  gopkgs
  go-outline
  go-symbols
  guru
  gorename
  gotests
  gomodifytags
  impl
  fillstruct
  goplay
  godoctor
  dlv
  gocode-gomod
  godef
  goimports
  golint
  gopls

image

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Dec 27, 2019

Can you share your VS Code settings.json file? Updating to master should not break formatting. If you can also share the gopls logs, that would be helpful too (see how to capture logs here).

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Jan 31, 2020

A number of users have reported that this has been resolved in gopls/v0.3.0. If someone encounters this again, please open a new issue.

@stamblerre stamblerre closed this Jan 31, 2020
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 7, 2020

Re-opening this issue, as it's been reported several times with gopls/v0.3.1. For all those who have this issue, please provide logs from an instance when this happened in your editor. Information on how to capture logs can be found here.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 11, 2020

Change https://golang.org/cl/218859 mentions this issue: internal/lsp: propagate file invalidations to all views

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 11, 2020

I believe I've figured out the issue and have a fix for it. If affected users could try applying the patch before it is merged to confirm that it fixes the issue, that would be very helpful. Here are the steps to do so:

$ git clone https://go.googlesource.com/tools
$ cd tools
$ git fetch "https://go.googlesource.com/tools" refs/changes/59/218859/3 && git cherry-pick FETCH_HEAD
$ cd gopls
$ go install
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 11, 2020

The change is now merged, so gopls can instead be downloaded by running GO111MODULE=on go get golang.org/x/tools/gopls@master golang.org/x/tools@master.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 11, 2020

Change https://golang.org/cl/219125 mentions this issue: internal/lsp: propagate file invalidations to all views

gopherbot pushed a commit to golang/tools that referenced this issue Feb 12, 2020
We were previously only invalidating files if they were contained within
a subdirectory of a view, but we should really be invalidating all files
that are known to a view.

Fixes golang/go#34955

Change-Id: I2eb1476e6b5f74a64dbb6d47459f4009648c720d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218859
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit e2a38c8)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219125
@galenwarren
Copy link

@galenwarren galenwarren commented Feb 20, 2020

I encountered the issue today with version v0.3.2-pre1. On save, it changed the imports from:

import (
	"context"
	"github.com/pkg/errors"
	"github.com/coachclientconnect/system/core/golang/services"
)

... to:

import (
	"context"

	"github.com/coachclientconnect/system/core/golang/services"
	"github.com/pkg/errors"
	"github.com/pkg/errors"
)

The logs are here.

gopls version output:
golang.org/x/tools/gopls v0.3.2
golang.org/x/tools/gopls@v0.3.2-pre1 h1:xuQYMIznE2b+x+0Efw5qdcfOcHqftRLKOv12kvM3vGI=

gopls -rpc.trace -v format coachingprogram_entity_server_extras.go output is here. Not sure if this is relevant, but one thing I noticed about this is that the output doesn't show the doubled up "github.com/pkg/errors" entry even though it was doubled in the file when gopls -rpc.trace -v format coachingprogram_entity_server_extras.go was run.

Thanks for your help on this!

@stamblerre stamblerre reopened this Feb 20, 2020
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 20, 2020

Thank you for following up!

As expected with this bug, we see a codeAction response with a version of 0:

[Trace - 09:46:25.588 AM] Received response 'textDocument/codeAction - (7)' in 1178ms.
Result: [{"title":"Organize Imports","kind":"source.organizeImports","edit":{"documentChanges":[{"textDocument":{"version":0,"uri":"file:///home/galen/projects/git/@coachclientconnect/system-dev/modules/backend/program/coachingprogram_entity_server_extras.go"},"edits":[{"range":{"start":{"line":7,"character":0},"end":{"line":8,"character":0}},"newText":""}]}]}}]

Most notably, the file has a @ in it, which VS Code escapes when the didOpen is sent to the server:

[Trace - 09:46:19.625 AM] Sending notification 'textDocument/didOpen'.
Params: {"textDocument":{"uri":"file:///home/galen/projects/git/%40coachclientconnect/system-dev/modules/backend/program/coachingprogram_entity_server_extras.go","languageId":"go","version":1,"text":"<redacted>"}}

@heschik has recently made some changes to our handling of URIs because of this exact issue. @galenwarren: Can you download gopls at master and see if the issue still reproduces? To download at master, run the following command: GO111MODULE=on go get golang.org/x/tools/gopls@master golang.org/x/tools@master.

@galenwarren
Copy link

@galenwarren galenwarren commented Feb 20, 2020

I've installed the master version -- will see how it goes and report back. Thanks again.

@jpvillaseca
Copy link

@jpvillaseca jpvillaseca commented Feb 20, 2020

@stamblerre what I found that worked for me was to enable Go Modules. Without them, the issue appeared on any settings and version configuration, but with go modules, gopls works wonders.
These are my relevant settings

"go.useLanguageServer": true,
    "go.languageServerExperimentalFeatures": {
        "format": false, 
        "signatureHelp": true,
        "workspaceSymbols": true,
    },
    "gopls": {
        "usePlaceholders": true,
    },
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 25, 2020

@jpvillaseca: Setting format to false may also mask the issue. I would expect that this issue should be fixed at master, however (unless there is a corner case we haven't yet encountered, which is very possible).

@galenwarren
Copy link

@galenwarren galenwarren commented Feb 27, 2020

Just wanted to let you that I've been using the master version for a few days now and all seems to be good! Any thoughts on when that version might be published? Thanks.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 27, 2020

Glad to hear it! I'll close this issue then. (Please re-open if something new comes up.)
A new release will be coming shortly (next week, hopefully). An update will be published here when it's available.

@stamblerre stamblerre closed this Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.