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: make eglot+gopls awesome #67529

Open
1 of 10 tasks
adonovan opened this issue May 20, 2024 · 7 comments
Open
1 of 10 tasks

x/tools/gopls: make eglot+gopls awesome #67529

adonovan opened this issue May 20, 2024 · 7 comments
Assignees
Labels
Friction Nuisances that make good candidates for our "friction" fix-it weeks gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented May 20, 2024

@stapelberg and I plan to work together to make Emacs+eglot smoother, faster, and better documented.

Some specific items (feel free to add or suggest more):

@adonovan adonovan added Friction Nuisances that make good candidates for our "friction" fix-it weeks gopls Issues related to the Go language server, gopls. labels May 20, 2024
@findleyr findleyr changed the title gopls: make eglot+gopls awesome x/tools/gopls: make eglot+gopls awesome May 20, 2024
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label May 20, 2024
@gopherbot gopherbot added this to the Unreleased milestone May 20, 2024
@hyangah hyangah modified the milestones: Unreleased, gopls/backlog May 23, 2024
@findleyr
Copy link
Contributor

From #62330: one thing to look at would be watched file support (didChangeWatchedFiles).

@danp
Copy link
Contributor

danp commented May 27, 2024

Re didChangeWatchedFiles, at least for macOS there's the possibly related emacs-lsp/lsp-mode#3296 (comment) for lsp-mode. Looks like eglot does use file-notify-add-watch like lsp-mode, which on macOS can't detect changes to existing files when the path given to file-notify-add-watch is a directory.

@jidicula
Copy link

jidicula commented May 27, 2024

I'm so glad to see this area getting some investment 😁. lsp-mode is nice, but the eglot experience works much better with TRAMP (especially in concert with https://github.com/patrickt/codespaces.el) and GitHub Codespaces.

@stapelberg
Copy link
Contributor

stapelberg commented Jun 14, 2024

Regarding the didChangeWatchedFiles issues, I have updated the first comment with a summary of what I understand is the problem.

Specifically, I can reproduce issue #62330 on macOS, but not on Linux.

On Linux, the (inotify-based) file content watching works and if I update go.mod’s contents (e.g. with echo "// test" >> go.mod), I do see an event being sent to gopls from Eglot in the *EGLOT…* buffer. Be careful when testing: touch go.mod is not sufficient, because Eglot is watching for file content changes, not file attribute changes.

On macOS, indeed I get no event at all. AFAIK, Emacs does its own file watching (falling back to polling, even) for opened files, which is why having a file open papers over the problem. I linked Emacs bug#51146, which I’ll summarize as: This is an inherent limitation of kqueue-based watching. Adding fsevents support (a macOS-specific file watching API) to Emacs would probably be the best way to fix this bug.


With that said, there is another problem regarding didChangeWatchedFiles, even on Linux, which is that Emacs’s filenotify package does not work recursively. This means that creating extra.go next to go.mod results in an event, but creating a new directory (mkdir internal) and creating a file within that directory (e.g. internal/extra.go) does not result in an event.

To better understand what’s happening, I instrumented Emacs like so:

(defun my-file-notify-add-watch-logger (file event &rest args)
  "log a message whenever file-notify-add-watch is called"
  (message "file-notify-add-watch called with file: %s, event: %s" file event))

(advice-add 'file-notify-add-watch :before #'my-file-notify-add-watch-logger)

When opening hello1.go, I see that Eglot expands the glob pattern on all known project files:

[eglot] Connected! Server `gopls' now managing `go-mode' buffers in project `hello1'.
[eglot] Server reports (type=4): Loading packages...
[eglot] Server reports (type=3): Finished loading packages.
file-notify-add-watch called with file: /home/stapelberg/repro-emacs/hello1/, event: (change)
file-notify-add-watch called with file: /home/stapelberg/repro-emacs/hello1/internal/foo/, event: (change)
file-notify-add-watch called with file: /home/stapelberg/repro-emacs/hello1/internal/, event: (change)

Creating a new directory does not result in a new expansion of the glob pattern, so the new directory is unwatched.


I have filed a bug report about this with Eglot, but given the rather dire state of client-side watching, are we sure that we don’t want to reconsider server-side watching? :)

@findleyr
Copy link
Contributor

I have joaotavora/eglot#1411, but given the rather dire state of client-side watching, are we sure that we don’t want to reconsider server-side watching? :)

Two years ago I would have responded with a philosophical argument that clients should be responsible for all state changes, etc etc, including the arguments here.

However, after years of struggling with client-side watching, I am at the point where the only barrier to switching to server-side watching is momentum; we have other high priorities, and file watching sort of works in most clients.

Perhaps I'm being overly pessimistic, though. In theory, it shouldn't be that hard to link a cross platform file watching library, and provide a gopls option to use server-side file watching. It may be worth an experiment.

@stapelberg
Copy link
Contributor

:)

I had a quick look and it seems like https://github.com/fsnotify/fsnotify is the most popular Go package for file watching. It does support recursive watching since last month (see fsnotify/fsnotify@a618f07), but not on macOS (it also does not support the fsevents API on macOS).

So I suppose by using the fsnotify package as of today, we would fix file watching on macOS in general, and recursive file watching on Linux, but not on macOS.

@findleyr
Copy link
Contributor

You inspired me to file #67995, dumping my thoughts on this issue.

If you are interested, feel free to give that a try. I think the three bullets listed there are the basic code pointers necessary, though I'm happy to provide more guidance. This is a relatively self-contained experiment, and doesn't require deep gopls knowledge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Friction Nuisances that make good candidates for our "friction" fix-it weeks 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
Development

No branches or pull requests

7 participants