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: formatting result breaks the code #45648

Closed
zaakn opened this issue Apr 20, 2021 · 14 comments
Closed

x/tools/gopls: formatting result breaks the code #45648

zaakn opened this issue Apr 20, 2021 · 14 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.

Comments

@zaakn
Copy link

zaakn commented Apr 20, 2021

What version of Go are you using (go version)?

$ go version
go version go1.15.6 darwin/amd64

$ gopls version
golang.org/x/tools/gopls v0.6.10
    golang.org/x/tools/gopls@v0.6.10 h1:8Ebz8PymS2umcuCFhoz67unyJfWsipjTIrkBUF9kypg=

$ code -v
1.55.2
3c4e3df9e89829dce27b7b5c24508306b151f30d
x64

VSCode settings.json

All of the go/gopls related parameters use default values, but
"go.languageServerFlags": ["-rpc.trace"] for the tracing.

What did you do?

The formatting result on saving broke the code.

IMPORTANCE to reproduce:

  1. No empty line or more than ONE empty line at the end of the file.
  2. More than TWO places(code blocks) need to be indented. // (1) (2)
  3. The last element of the second block is a data type with the "}" character, e.g. map, struct.

The comments just for explanation, please remote them before testing.

package main

type Issue struct{
	Dummy int
}

var (
	foo = "bar" // (1)
	someIssues = []interface{}{
			Issue{}, // (2)
			Issue{}, // (2)
			Issue{}, // (2)
	}
) // no empty line at EOF

What did you expect to see?

package main

type Issue struct {
	Dummy int
}

var (
	foo        = "bar"
	someIssues = []interface{}{
		Issue{},
		Issue{},
		Issue{},
	}
)
// an empty line here

What did you see instead?

package main

type Issue struct{
{
{
	Dummy int
{
}
{

{
var (
{
	foo = "bar"
{
	someIssues = []interface{}{
{
			Issue{},
{
			Issue{},
{
			Issue{},
{
	}
	Dummy int
gopls (server) trace output
[Trace - 17:27:10.895 PM] Sending request 'textDocument/codeAction - (52)'.
Params: {"textDocument":{"uri":"file:///private/tmp/issue.go"},"range":{"start":{"line":0,"character":0},"end":{"line":13,"character":1}},"context":{"diagnostics":[],"only":["source.organizeImports"]}}

[Trace - 17:27:10.900 PM] Received response 'textDocument/codeAction - (52)' in 4ms.
Result: null

[Trace - 17:27:10.905 PM] Sending request 'textDocument/formatting - (53)'.
Params: {"textDocument":{"uri":"file:///private/tmp/issue.go"},"options":{"tabSize":4,"insertSpaces":false}}

[Trace - 17:27:10.915 PM] Received response 'textDocument/formatting - (53)' in 9ms.
Result: [{"range":{"start":{"line":3,"character":0},"end":{"line":3,"character":0}},"newText":""},{"range":{"start":{"line":3,"character":0},"end":{"line":3,"character":0}},"newText":"{\n{\n"},{"range":{"start":{"line":4,"character":0},"end":{"line":4,"character":0}},"newText":"{\n"},{"range":{"start":{"line":5,"character":0},"end":{"line":5,"character":0}},"newText":"{\n\n{"},{"range":{"start":{"line":7,"character":0},"end":{"line":7,"character":0}},"newText":"{\n"},{"range":{"start":{"line":8,"character":0},"end":{"line":8,"character":0}},"newText":"{\n"},{"range":{"start":{"line":9,"character":0},"end":{"line":9,"character":0}},"newText":"{\n"},{"range":{"start":{"line":10,"character":0},"end":{"line":10,"character":0}},"newText":"{\n"},{"range":{"start":{"line":11,"character":0},"end":{"line":11,"character":0}},"newText":"{\n"},{"range":{"start":{"line":12,"character":0},"end":{"line":12,"character":0}},"newText":"{\n"},{"range":{"start":{"line":13,"character":0},"end":{"line":14,"character":0}},"newText":""},{"range":{"start":{"line":14,"character":0},"end":{"line":14,"character":0}},"newText":"\tDummy int"}]

[Trace - 17:27:10.946 PM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"uri":"file:///private/tmp/issue.go","version":7},"contentChanges":[{"range":{"start":{"line":13,"character":1},"end":{"line":13,"character":1}},"rangeLength":0,"text":"\tDummy int"},{"range":{"start":{"line":13,"character":0},"end":{"line":13,"character":1}},"rangeLength":1,"text":""},{"range":{"start":{"line":12,"character":0},"end":{"line":12,"character":0}},"rangeLength":0,"text":"{\n"},{"range":{"start":{"line":11,"character":0},"end":{"line":11,"character":0}},"rangeLength":0,"text":"{\n"},{"range":{"start":{"line":10,"character":0},"end":{"line":10,"character":0}},"rangeLength":0,"text":"{\n"},{"range":{"start":{"line":9,"character":0},"end":{"line":9,"character":0}},"rangeLength":0,"text":"{\n"},{"range":{"start":{"line":8,"character":0},"end":{"line":8,"character":0}},"rangeLength":0,"text":"{\n"},{"range":{"start":{"line":7,"character":0},"end":{"line":7,"character":0}},"rangeLength":0,"text":"{\n"},{"range":{"start":{"line":5,"character":0},"end":{"line":5,"character":0}},"rangeLength":0,"text":"{\n\n{"},{"range":{"start":{"line":4,"character":0},"end":{"line":4,"character":0}},"rangeLength":0,"text":"{\n"},{"range":{"start":{"line":3,"character":0},"end":{"line":3,"character":0}},"rangeLength":0,"text":"{\n{\n"}]}

[Trace - 17:27:10.950 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/tmp/issue.go","version":7,"diagnostics":[{"range":{"start":{"line":3,"character":0},"end":{"line":3,"character":0}},"severity":1,"source":"syntax","message":"expected '}', found '{'"}]}

[Trace - 17:27:11.065 PM] Sending notification 'textDocument/didSave'.
Params: {"textDocument":{"uri":"file:///private/tmp/issue.go"}}

[Trace - 17:27:11.195 PM] Sending request 'textDocument/foldingRange - (54)'.
Params: {"textDocument":{"uri":"file:///private/tmp/issue.go"}}

[Trace - 17:27:11.198 PM] Sending request 'textDocument/codeLens - (55)'.
Params: {"textDocument":{"uri":"file:///private/tmp/issue.go"}}

[Trace - 17:27:11.208 PM] Received response 'textDocument/foldingRange - (54)' in 13ms.
Result: []

[Trace - 17:27:11.209 PM] Received response 'textDocument/codeLens - (55)' in 10ms.
Result: null

[Trace - 17:27:11.278 PM] Sending request 'textDocument/documentSymbol - (56)'.
Params: {"textDocument":{"uri":"file:///private/tmp/issue.go"}}

[Trace - 17:27:11.279 PM] Received response 'textDocument/documentSymbol - (56)' in 1ms.
Result: [{"name":"Issue","detail":"struct{...}","kind":23,"range":{"start":{"line":2,"character":5},"end":{"line":3,"character":1}},"selectionRange":{"start":{"line":2,"character":5},"end":{"line":2,"character":10}}}]

[Trace - 17:27:11.446 PM] Sending notification 'workspace/didChangeWatchedFiles'.
Params: {"changes":[{"uri":"file:///private/tmp/issue.go","type":2}]}

[Trace - 17:27:11.924 PM] Sending request 'textDocument/documentLink - (57)'.
Params: {"textDocument":{"uri":"file:///private/tmp/issue.go"}}

[Trace - 17:27:11.943 PM] Received response 'textDocument/documentLink - (57)' in 18ms.
Result: null

gopls_v0.6.10.darwin.tar.gz
gopls_v0.6.10.linux_x86_64.tar.gz

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Apr 20, 2021
@gopherbot gopherbot added this to the Unreleased milestone Apr 20, 2021
@findleyr
Copy link
Contributor

findleyr commented Apr 20, 2021

Thanks for the report. This looks very broken.

I am not able to reproduce. This is not surprising: if formatting were so easily broken, there would be many more reports. I suspect, instead, that there is either a conflicting formatting tool running in your editor, or perhaps some very strange edge case bug in the formatting logic, perhaps related to line endings.

If you are able to reproduce, we can try to narrow this down together. Questions for you:

  1. Are you able to reliably reproduce this bug?
  2. Do you have any other extensions installed that would auto-format?
  3. Are you on windows? (Edit: I notice now that the go version is darwin/amd64)
  4. To avoid any loss of fidelity in the repro at byte level, could you send me a base64-encoded copy of your repro? On linux, you can get this via base64 main.go.

@findleyr findleyr added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 20, 2021
@findleyr findleyr modified the milestones: Unreleased, Backlog Apr 20, 2021
@AlexRouSg
Copy link
Contributor

AlexRouSg commented Apr 20, 2021

@findleyr I am on windows and made the test file with windows line endings and I am not able to reproduce this on gopls v0.6.10

gopls format test.go output:

package main

type Issue struct {
        Dummy int
}

var (
        foo        = "bar"
        someIssues = []interface{}{
                Issue{},
                Issue{},
                Issue{},
        }
)

@findleyr
Copy link
Contributor

@AlexRouSg thanks for trying. I actually just noticed that GOOS/GOARCH is darwin/amd64, so this probably isn't related to line endings.

@zaakn
Copy link
Author

zaakn commented Apr 20, 2021

  1. Are you able to reliably reproduce this bug?

Yes, not only in VSCode but also gopls format command(thanks @AlexRouSg let me realize that it can run command directly).

  1. Do you have any other extensions installed that would auto-format?

I had disabled all other extensions and only kept the "Go" extension, and made VSCode settings.json empty.

  1. Are you on windows? (Edit: I notice now that the go version is darwin/amd64)

I did it on my local MacOS and the Linux remote development.

  1. To avoid any loss of fidelity in the repro at byte level, could you send me a base64-encoded copy of your repro? On linux, you can get this via base64 main.go.

if someone copy the original code to vim, please use vim -b issue.go and :set noeol to imitate that there is no new line('\n') at the end. And I missed another one place:

type Issue struct{ // no space between "struct" and "{" 
fai /tmp $ echo -n 'cGFja2FnZSBtYWluCgp0eXBlIElzc3VlIHN0cnVjdHsKCUR1bW15IGludAp9Cgp2YXIgKAoJZm9vID0gImJhciIKCXNvbWVJc3N1ZXMgPSBbXWludGVyZmFjZXt9ewoJCQlJc3N1ZXt9LAoJCQlJc3N1ZXt9LAoJCQlJc3N1ZXt9LAoJfQop' |base64 -d > issue.go
fai /tmp $ gopls format issue.go
2021/04/21 00:17:43 Error:2021/04/21 00:17:43 go/packages.Load: err: exit status 1: stderr: go: cannot find main module; see 'go help modules'

	snapshot=0
	directory=/tmp
	query=[./ builtin]
	packages=0
2021/04/21 00:17:43 Error:2021/04/21 00:17:43 initial workspace load failed: err: exit status 1: stderr: go: cannot find main module; see 'go help modules'
: packages.Load error
package main

type Issue struct{
{
{
	Dummy int
{
}
{

{
var (
{
	foo = "bar"
{
	someIssues = []interface{}{
{
			Issue{},
{
			Issue{},
{
			Issue{},
{
	}
	Dummy int

@bokunodev

This comment has been minimized.

@findleyr
Copy link
Contributor

Thanks so much for the careful repro steps! This is very helpful.

Surprisingly, I still cannot repro. We're brainstorming how this could be possible.

> echo -n 'cGFja2FnZSBtYWluCgp0eXBlIElzc3VlIHN0cnVjdHsKCUR1bW15IGludAp9Cgp2YXIgKAoJZm9vID0gImJhciIKCXNvbWVJc3N1ZXMgPSBbXWludGVyZmFjZXt9e
woJCQlJc3N1ZXt9LAoJCQlJc3N1ZXt9LAoJCQlJc3N1ZXt9LAoJfQop' | base64 -d > issue.go                                                                      
> gopls format issue.go                                                                                                                 
package main            
                                                                          
type Issue struct {     
        Dummy int
}             
                                     
var (                                                                     
        foo        = "bar"                                                                                                                           
        someIssues = []interface{}{
                Issue{},
                Issue{},                                                  
                Issue{},  
        }         
)                                                                                                                                                    
> gopls version                                                                                                                         
golang.org/x/tools/gopls v0.6.10                                                                                                                     
    golang.org/x/tools/gopls@(devel)                                      
> go version                                                 
go version go1.15.6 linux/amd64

@zaakn
Copy link
Author

zaakn commented Apr 20, 2021

i'm able to reproduce this on linux
interesting enough, its only happen if i format the code using gopls format main.go but not with gopls imports main.go
also not happen with the old goimpors

Yes, me too. Both on MacOS and Linux, gopls format meets the bad result, and gopls imports not.

@heschi
Copy link
Contributor

heschi commented Apr 20, 2021

Can someone who can reproduce this show us the output of locale?

@bokunodev
Copy link

bokunodev commented Apr 20, 2021

Can someone who can reproduce this show us the output of locale?

$ locale 
LANG=en_US.UTF-8
LC_CTYPE="en_US.UTF-8"
LC_NUMERIC="en_US.UTF-8"
LC_TIME="en_US.UTF-8"
LC_COLLATE="en_US.UTF-8"
LC_MONETARY="en_US.UTF-8"
LC_MESSAGES="en_US.UTF-8"
LC_PAPER="en_US.UTF-8"
LC_NAME="en_US.UTF-8"
LC_ADDRESS="en_US.UTF-8"
LC_TELEPHONE="en_US.UTF-8"
LC_MEASUREMENT="en_US.UTF-8"
LC_IDENTIFICATION="en_US.UTF-8"
LC_ALL=

@heschi
Copy link
Contributor

heschi commented Apr 20, 2021

OK. We're quite stumped, so, silly as it seems, can you attach your gopls binary to this issue?

@bokunodev
Copy link

bokunodev commented Apr 20, 2021

gopls.zip

@heschi
Copy link
Contributor

heschi commented Apr 20, 2021

Thanks, that was extremely helpful.

The problem is that the latest version of github.com/sergi/go-diff doesn't work well with gopls. I don't know where the bug is yet, but if you do GO111MODULE=on go get -u golang.org/x/tools/gopls@latest, the -u causes all dependencies to be upgraded and you get a broken gopls.

Reinstall without the -u and I believe the problem will go away.

@zaakn
Copy link
Author

zaakn commented Apr 20, 2021

Reinstall without the -u and I believe the problem will go away.

@heschi Thanks for your detailed analysis.
I can get the right formatting result after reinstalling gopls without "-u".

@hyangah
Copy link
Contributor

hyangah commented Apr 20, 2021

@zaakn can you tell us how you installed gopls with -u before? I want to understand how to improve VSCode Go's tool installation process.

@heschi heschi added NeedsFix The path to resolution is known, but the work has not been done. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Apr 21, 2021
@stamblerre stamblerre modified the milestones: Backlog, gopls/unplanned Apr 27, 2021
@suzmue suzmue closed this as completed Apr 30, 2021
@stamblerre stamblerre removed this from the gopls/unplanned milestone Apr 30, 2021
@golang golang locked and limited conversation to collaborators Apr 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

9 participants