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

nickwells commented 1 year ago

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

adonovan commented 1 year ago

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.

adonovan commented 1 year ago

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.

golang / go