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

Jump to bottom

x/tools/gopls: gopls imports -w <file> does not update the imports statement #58891

Closed
nickwells opened this issue Mar 6, 2023 · 2 comments
Closed

x/tools/gopls: gopls imports -w <file> does not update the imports statement #58891

nickwells opened this issue Mar 6, 2023 · 2 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/backlog

Comments

@nickwells
Copy link

ATTENTION: Please answer these questions BEFORE submitting your issue. Thanks!

What did you do?

When I run "gopls" from within a Go program (using the exec package) against a file in a temporary directory in macOS the imports statement is not updated if I give the full pathname but is if I give a relative pathname.

Note:

  • this only happens on a Mac, not on Linux
  • It only happens if I run gopls from within a Go program, running it from the command line works
  • it only happens if I run it for a file within a newly created temp dir as generated by calling os.MkdirTemp("", ...), a temp dir in my home directory doesn't have this problem (but one in "/tmp" has the same problem)
  • only the imports subcommand has a problem, "gopls format -w " successfully updates the file
  • other import statement generators work - "goimports -w " successfully updates the file

The following program demonstrates the issue:

package main

import (
	"flag"
	"fmt"
	"os"
	"os/exec"
	"path/filepath"
	"strings"
)

const (
	filename      = "ggg.go"
	progname      = "ggg"
	tmpDirPattern = "ggg-*"

	fileTextStr = `package main
func main() {
fmt.Println("Hello")
}

`
)

var fileText = []byte(fileTextStr)

// execCmd ...
func execCmd(prog string, args ...string) {
	name := prog + " " + strings.Join(args, " ")
	fmt.Println(name)

	cmd := exec.Command(prog, args...)
	cmd.Stdout, cmd.Stderr = os.Stdout, os.Stderr
	err := cmd.Run()

	reportErr(name, err)
}

// reportErr reports the error if not nil and exits
func reportErr(name string, err error) {
	if err == nil {
		return
	}
	fmt.Println(name+": Error: ", err)
	os.Exit(1)
}

// cmp compares the contents of the file with the supplied original text. If
// they differ it will report the difference
func cmp(origText []byte, fileName string, showFileDiffs bool) {
	text, err := os.ReadFile(fileName)
	reportErr("reading file: "+fileName, err)
	differs := len(origText) != len(text)

	if !differs {
		for i, ch := range origText {
			if text[i] != ch {
				differs = true
				break
			}
		}
	}

	if !differs {
		fmt.Println("File is unchanged")
	} else {
		fmt.Println("File has changed")
		if showFileDiffs {
			fmt.Println("Before")
			fmt.Println(string(origText))
			fmt.Println("After")
			fmt.Println(string(text))
		}
	}
}

func main() {
	var desc,
		tmpDir string
	var goplsAct = "imports"

	var useRelName,
		useGoImports,
		showFileDiffs bool

	flag.BoolVar(&useRelName, "rel", false, "use the relative pathname")
	flag.BoolVar(&showFileDiffs, "show", false, "show the differences")
	flag.BoolVar(&useGoImports, "use-goimp", false, "use goimports not gopls")
	flag.StringVar(&tmpDir, "tmp", "", "use this as base temp dir")
	flag.StringVar(&goplsAct, "act", "imports", "the gopls action")
	flag.Parse()

	desc = `MkdirTemp("` + tmpDir + `", "` + tmpDirPattern + `")`
	fmt.Println(desc)
	dir, err := os.MkdirTemp(tmpDir, tmpDirPattern)
	reportErr(desc, err)

	desc = "Chdir " + dir
	fmt.Println(desc)
	err = os.Chdir(dir)
	reportErr(desc, err)

	execCmd("go", "mod", "init", progname)

	desc = "WriteFile " + filename
	fmt.Println(desc)
	err = os.WriteFile(filename, fileText, 0666)
	reportErr(desc, err)

	filePath := filepath.Join(dir, filename)
	if useRelName {
		filePath = filename
	}
	if useGoImports {
		execCmd("goimports", "-w", filePath)
	} else {
		execCmd("gopls", goplsAct, "-w", filePath)
	}
	cmp(fileText, filePath, showFileDiffs)

	execCmd("go", "mod", "tidy")

	execCmd("go", "build")

	execPath := filepath.Join(dir, progname)
	execCmd(execPath)
}

What did you expect to see?

I expected the Go program to have the imports statement filled in

What did you see instead?

The file was unchanged

Build info

the OS version as given by uname -a is

Darwin Nicholass-MBP.broadband 21.6.0 Darwin Kernel Version 21.6.0: Mon Dec 19 20:44:01 PST 2022; root:xnu-8020.240.18~2/RELEASE_X86_64 x86_64


golang.org/x/tools/gopls v0.11.0
    golang.org/x/tools/gopls@v0.11.0 h1:/nvKHdTtePQmrv9XN3gIUN9MOdUrKzO/dcqgbG6x8EY=
    github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
    github.com/google/go-cmp@v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/exp@v0.0.0-20221031165847-c99f073a8326 h1:QfTh0HpN6hlw6D3vu8DAwC8pBIwikq0AI1evdm+FksE=
    golang.org/x/exp/typeparams@v0.0.0-20221031165847-c99f073a8326 h1:fl8k2zg28yA23264d82M4dp+YlJ3ngDcpuB1bewkQi4=
    golang.org/x/mod@v0.7.0 h1:LapD9S96VoQRhi/GrNTqeBJFrUjs5UHCAtTlgwA5oZA=
    golang.org/x/sync@v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o=
    golang.org/x/sys@v0.2.0 h1:ljd4t30dBnAvMZaQCevtY0xLLD0A+bRZXbgLMLU1F/A=
    golang.org/x/text@v0.4.0 h1:BrVqGRd7+k1DiOgtnFvAkoQEWQvBc25ouMJM6429SFg=
    golang.org/x/tools@v0.3.1-0.20221213193459-ca17b2c27ca8 h1:7/HkGkN/2ktghBCSRRgp31wAww4syfsW52tj7yirjWk=
    golang.org/x/vuln@v0.0.0-20221109205719-3af8368ee4fe h1:qptQiQwEpETwDiz85LKtChqif9xhVkAm8Nhxs0xnTww=
    honnef.co/go/tools@v0.3.3 h1:oDx7VAwstgpYpb3wv0oxiZlxY+foCpRAwY7Vk6XpAgA=
    mvdan.cc/gofumpt@v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM=
    mvdan.cc/xurls/v2@v2.4.0 h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc=
go: go1.20.1
@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 Mar 6, 2023
@gopherbot gopherbot added this to the Unreleased milestone Mar 6, 2023
@hyangah hyangah modified the milestones: Unreleased, gopls/unplanned Mar 7, 2023
@adonovan
Copy link
Member

adonovan commented Apr 7, 2023 •
edited

Thanks for the exemplary bug report. I can reproduce the bug on a Mac at v0.11.0, but not at master, so the bug is fixed... except that the fix wasn't as deliberate as we would like, so there is more work to do.

Specifically, the CodeAction RPC used to return an "Organize Imports" action whose .Edit.DocumentChanges.TextDocumentEdit.TextDocument.URI subfield was the "realpath" of the original file name (i.e. symbolic links were expanded), so the client's simple search for the original file name failed to find it. The logic at master does not expand symbolic links, so the URI returned by the server is the same as the one requested by the user, and the client proceeds to update the file.

The reason this only shows up on the Mac, and only on temporary files, is that its /tmp dir is a symbolic link:

$ realpath /tmp
/private/tmp

This problem is related to an existing problem (whose issue I cannot now find) in which identifier renaming corrupts a file because it tries to apply identical edits to two files of different names that happen, thanks to symbolic links, to be aliases for the same file. The general solution is similar: ask the file system whether two files are equal; don't rely on lexical equivalence.

@findleyr findleyr modified the milestones: gopls/unplanned, gopls/later Jun 15, 2023
@adonovan
Copy link
Member

I was going to write a regression test for this but I can no longer reproduce this even with v0.11.0. I'd rather not spend any more time on it since the actual big is fixed.

This problem is related to an existing problem (whose issue I cannot now find) in which identifier renaming corrupts a file because it tries to apply identical edits to two files of different names that happen, thanks to symbolic links, to be aliases for the same file. The general solution is similar: ask the file system whether two files are equal; don't rely on lexical equivalence.

On further reflection, I think it is properly a client-side concern. The server tries as much as possible not to interpret file names, but merely to pass them down to open(1) or go list, or back to the client. At the risk of demanding that every LSP client implement the same solutions based on something like os.SameFile, I'm going to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/backlog
Development

No branches or pull requests
5 participants
@nickwells @hyangah @adonovan @gopherbot @findleyr