x/tools/gopls: unable to organize imports after go mod tidy / go get

[jroimartin]

gopls version

Build info
----------
golang.org/x/tools/gopls v0.13.2
    golang.org/x/tools/gopls@v0.13.2 h1:Pyvx6MKvatbX3zzZmdGiFRfQZl0ohPlt2sFxO/5j6Ro=
    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-20220722155223-a9213eeb770e h1:+WEEuIdZHnUeJJmEUjyYC2gfUMj69yZXw17EnHg/otA=
    golang.org/x/exp/typeparams@v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y=
    golang.org/x/mod@v0.12.0 h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc=
    golang.org/x/sync@v0.3.0 h1:ftCYgMx6zT/asHUrPw8BLLscYtGznsLAnjq5RH9P66E=
    golang.org/x/sys@v0.10.0 h1:SqMFp9UcQJZa+pmYuAKjd9xq1f0j5rLcDIk0mj4qAsA=
    golang.org/x/text@v0.11.0 h1:LAntKIrcmeSKERyiOh0XMV39LXS8IE9UL2yP7+f5ij4=
    golang.org/x/tools@v0.11.2-0.20230810185051-cc6b5804b8cf h1:Oush7UwPamr2/iNeNFBuNFj89YyHn0YY69EKDdvANnk=
    golang.org/x/vuln@v0.0.0-20230110180137-6ad3e3d07815 h1:A9kONVi4+AnuOr1dopsibH6hLi1Huy54cbeJxnq4vmU=
    honnef.co/go/tools@v0.4.2 h1:6qXr+R5w+ktL5UkwEbPp+fEvfyoMPche6GkOpGHZcLc=
    mvdan.cc/gofumpt@v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM=
    mvdan.cc/xurls/v2@v2.4.0 h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc=
go: go1.21.0

go env

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/user/.cache/go-build'
GOENV='/home/user/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/user/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/user/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/user/goroot'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/user/goroot/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build695062736=/tmp/go-build -gno-record-gcc-switches'

What did you do?

1. go mod init github.com/jroimartin/goplsissue
2. Create a file main.go with the following contents:

package main

func main() {
	_ = proxy.Proxy{}
}

3. Save the file. The following package is imported correctly:

import "github.com/jroimartin/proxy"

And, as expected, the following error is reported by flymake:

compiler [BrokenImport]: could not import github.com/jroimartin/proxy (no required module provides package "github.com/jroimartin/proxy")

4. go mod tidy
5. As expected, the flymake error goes away
6. Delete the import github.com/jroimartin/proxy
7. Save the file
8. The package is not imported again

What did you expect to see?

After saving the file, the package github.com/jroimartin/proxy should be imported again.

What did you see instead?

No package is imported.

Editor and settings

Editor: GNU Emacs 28.2
LSP client: eglot 1.15

Relevant init.el snippet:

(add-hook 'go-mode-hook
	  #'(lambda ()
	      (eglot-ensure)
	      (add-hook 'before-save-hook
			#'(lambda ()
			    (call-interactively #'eglot-code-action-organize-imports))
			nil
			t)
	      (add-hook 'before-save-hook #'eglot-format-buffer nil t)))

;; Look for the nearest parent go.mod file as the project root
(defun jrm-project-find-go-module (dir)
  (when-let ((root (locate-dominating-file dir "go.mod")))
    (cons 'go-module root)))

(cl-defmethod project-root ((project (head go-module)))
  (cdr project))

(add-hook 'project-find-functions #'jrm-project-find-go-module)

Logs

I'm not sure if it is related, but the following error caught my attention:

	  (:type 3 :message "2023/08/28 20:09:39 background refresh finished after 16.488675ms: empty GOPATH\n"))

[jroimartin]

Extra info

If I do M-x eglot and tell it to restart gopls, the "organize imports" functionality starts working again.

[findleyr]

Thanks for the issue, and the repro. We will take a look.

[pjweinb]

This doesn't seem to reproduce any more, at least in vscode. Is it still an issue, or could it be closed?

[findleyr]

@pjweinb I believe this is related to emacs support of didChangeWatchedFile. I am curious if it still reproduces, with the recent changes to goimports.

@adonovan would you be able to do a quick test in eglot?

[adonovan]

I just tried this with GNU Emacs 29.3 and eglot-1.17. Here's what I saw:

go mod init github.com/jroimartin/goplsissue
Create a file main.go with the following contents:...
Save the file. The following package is imported correctly:
import "github.com/jroimartin/proxy"

At this point, the import that was added in my case was a different one: "github.com/docker/go-connections/proxy".
I assume that's because the imports algorithm uses my GOMODCACHE state as an input, and no two users' caches are alike. So I ran go get github.com/jroimartin/proxy and then deleted the newly added require line in the go.mod, and tried again, and this time the jroimartin/proxy package was imported, as expected.

And, as expected, the following error is reported by flymake:
compiler [BrokenImport]: could not import github.com/jroimartin/proxy (no required module provides package "github.com/jroimartin/proxy")

The flymake error I saw here actually related to the previous docker package, even though the source code no longer mentions it. (There's a metadata inconsistency somewhere.) Only after deleting the import and saving, so that organize-imports ran again, did the import and the error message become consistent about the jroimartin package.

go mod tidy

(To be clear, I ran this on the shell, not through gopls from the go.mod file.)

As expected, the flymake error goes away

No, I still see the error. Another inconsistency.

Delete the import github.com/jroimartin/proxy
Save the file
The package is not imported again

No, the package was imported again, and this time without error.

Unfortunately I think there are at least two bugs here.

[findleyr]

@adonovan are you getting didChangeWatchedFiles notifications? If not, I think that explains the inconsistency. Gopls is not notified of the go.mod change, and therefore does not reload.

Deleting and reimporting causes a reload, which is why the error goes away the second time.

[adonovan]

Yep, that's it: Emacs doesn't report a didChange notification for the edits to the unopened go.mod file from go mod tidy; but if I open the go.mod file and then modify it from the command line, eglot sends the notification and gopls updates the diagnostics correctly.

[findleyr]

In that case, this is an unfortunate emacs issue. Perhaps something to add to #67529.

[findleyr]

Closing this specific issue as unactionable, though some team members who use eglot are hoping to work on improving the status quo here in #67529.

[pjweinb]

go.mod is fairly special. what are the downsides to gopls checking it too often?

…

On Fri, May 24, 2024 at 1:07 PM findleyr ***@***.***> wrote: In that case, this is an unfortunate emacs issue. Perhaps something to add to #67529 <#67529>. — Reply to this email directly, view it on GitHub <#62330 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJIAI6T35FTMNCLPTZE67LZD5XV7AVCNFSM6AAAAAA4BZ5YVWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZQGAYTANRYGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[findleyr]

@pjweinb it is not recommended for servers to do their own file watching, for the reasons listed here:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeWatchedFiles

We could poll the go.mod file, but that would only benefit the relatively small fraction of users on LSP clients that don't support file watching. It also would introduce a source of additional complexity in testing. Also, there are other on-disk changes we care about, such as changes in dependencies, and it seems inconsistent to watch go.mod files and not others.

On the other hand, file watching is a real source of inconsistent behavior across clients. Certain clients are optimized for watching e.g. "**/*.go", and others are not... so much so that this is the only place in all of gopls where we actually switch behaviors based on the client's user agent.

So, I am not in favor of adding ad-hoc watching of go.mod files. However, I would consider a larger project to give up entirely on client-side file watching, in favor of reverting to server side watching. Speaking with the Dart folks, they have server-side watching and haven't had major problems with it.

Unfortunately, we're not currently in a position to make these types of large changes for marginal gain.

[jrockway]

I know pretty much nothing about the eglot internals, but why can't we make eglot/go-mode watch go.mod? This bug is an Emacs quirk. We can just fix it in Emacs, right?

[adonovan]

I know pretty much nothing about the eglot internals, but why can't we make eglot/go-mode watch go.mod? This bug is an Emacs quirk. We can just fix it in Emacs, right?

Exactly. The server gopls tells the client eglot which files to watch, and I think that set includes go.mod, but for some reason it does not seem to be taking effect.