helix icon indicating copy to clipboard operation
helix copied to clipboard

File watcher implementation & LSP DidChangeWatchedFiles notification

Open kyrime opened this issue 2 years ago • 6 comments

Closes #2479 and is a prerequisite for #1125. Also probably supersedes #588.

I don't think this is quite ready to merge but it's definitely due for some reviewing. Functionality works fine for closing the intended issue but I've got concerns when it comes to other issues that depend on file watching.

notify is the main dependency added. I've reused globset as a dependency to parse the LSP spec's glob patterns, it's already included through ignore.

There are a couple different choices with regard to what directories to set notify watching:

  1. Recursively watch the current working directory.
  2. Set watches only on the specific files that are of interest.
  3. Same as (1) but mix in non-recursive watching to allow for ignoring certain directories.

The issue with (1) is that there's no way to outright ignore directories: it seems likely that the amount of file changes rust-analyzer generates in the target directory on every write through cargo check could cause stuttering on lower-specced machines. The issue with (2) is that it can't detect new files as it's not watching for them, and the LSP may be interested in events from files that aren't open in the editor. So I've gone with (3) here: non-recursively watching the root directory, ignoring those matching some default list of build/cache directories ala Rust's target directory, and recursively watching the rest. This approach is taken from emacs-lsp, which has a seemingly pretty comprehensive list of ignored directories. The downside is that this adds some complexity with keeping track of directory creations/deletions/renames in the root directory, but it's not bad.

File watching works as follows: Watcher sets up notify watches on open LSP workspaces. Interested parties register a callback to be called when the watcher receives events matching a path filter in a specific workspace.

Some considerations:

  • DidChangeWatchedFiles can only be registered dynamically, so supporting it requires handling RegisterCapability and UnregisterCapability. I haven't yet added support for unregistering the DidChangeWatchedFiles capability as I'm not sure where to store the required state: it seems to belong with the LSP client, but mutating the client seems to require interior mutability right now which gives me pause.
  • Helix supports having multiple LSPs in use which all may have their own root workspace folders, so I've attempted to handle the case of multiple workspaces in the file watcher. I'm not really sure if that was the right move.
  • Something weird here is that you might expect the watcher to default to watching everything from the current working directory down, but right now it just works with LSP workspaces. The current model also might cause issues with something like #1125 where any file could be open in the editor, even outside any workspace or the working directory, so seems like that would require mixing in some functionality to facilitate choice (2) above. Any thoughts would be welcome.
  • I haven't wired up LSP clients to add/remove their workspaces to/from the watcher's list of workspaces when they're created or dropped yet, mostly because I'm not really sure if I'm taking the right approach with how workspaces are handled right now.
  • The only build directory I have ignored right now is target. Happy to add others from emacs-lsp's list in if maintainers are fine with this approach. Something nagging me is that since these directories are per-language it would also make sense to me if these were configured per-language, rather than one big list.
  • I've used the pre-release version of notify which has been around for a while and seems pretty stable. It doesn't support debouncing yet but it seems like it's not too far off either. I haven't added debouncing to this PR yet either, it's possible it's best to just leave it as is and wait until notify's debounce support hits if the lack of debouncing isn't causing any issues.

That's all for now off the top of my head, but there's probably other stuff lurking around in the PR.

kyrime avatar Jun 03 '22 00:06 kyrime

@kyrime Sorry for the delay on this! I've been meaning to review this for a while.

The issue with (1) is that there's no way to outright ignore directories: it seems likely that the amount of file changes rust-analyzer generates in the target directory on every write through cargo check could cause stuttering on lower-specced machines.

Hmm isn't this how watchman would handle this internally? A recursive watch, but a way to drop unwanted notifications very early to avoid sending it through the notification system.

lsp-mode seems to also do recursive watching: https://github.com/emacs-lsp/lsp-mode/blob/8091f5d13efbde5bfabc774970dd689487053333/lsp-mode.el#L1871-L1910

I'd start with a simple recursive watch and implement something more complicated only if it becomes necessary.

it seems to belong with the LSP client, but mutating the client seems to require interior mutability right now which gives me pause.

We could use an ArcSwap on the server capabilities, similar to how we do the editor config. That way it stays immutable but we can swap it out with another updated capability struct.

Something weird here is that you might expect the watcher to default to watching everything from the current working directory down, but right now it just works with LSP workspaces.

We should watch from the root directory down, then if a filepath is also inside the LSP workspace, we can also trigger a LSP notification.

Happy to add others from emacs-lsp's list in if maintainers are fine with this approach. Something nagging me is that since these directories are per-language it would also make sense to me if these were configured per-language, rather than one big list.

I think we could use a global ignore list. The problem with per-language setting is that in a monorepo we might have a mix of different languages. Just because I'm editing a rust file doesn't mean there isn't a node_modules present in the same tree.

archseer avatar Nov 01 '22 12:11 archseer

The issue with (1) is that there's no way to outright ignore directories Happy to add others from emacs-lsp's list in if maintainers are fine with this approach. Something nagging me is that since these directories are per-language it would also make sense to me if these were configured per-language, rather than one big list. I think we could use a global ignore list. The problem with per-language setting is that in a monorepo we might have a mix of different languages. Just because I'm editing a rust file doesn't mean there isn't a node_modules present in the same tree.

yeah this is annoying. I wish there was an editor-independent (in LSP or outside) way of marking paths as "not containing sources". Would be nice to piggy-back on gitignore but that's not exactly the same thing.

I'd start with a simple recursive watch and implement something more complicated only if it becomes necessary.

I agree. This is also what kak-lsp does because I couldn't reproduce performance problems. I still feel iffy about watching everything but maybe the OS-imposed limits take care of that

krobelus avatar Dec 27 '22 13:12 krobelus

Any updates recently?

violin0622 avatar Jan 25 '24 08:01 violin0622

This feature is the only one preventing me from switching over to helix completely. I love helix, but being out of sync with the state of the file on the disk is a little jarring sometimes. What needs to be done for this to work?

yesudeep avatar Mar 09 '24 20:03 yesudeep

On a quick glance this seems reasonable apart from the needlessly-complex recursive watch logic. It's probably appropriate to use notify-debouncer-full.

You should know that kakoune-lsp recently disabled the file watcher by default because it caused very high latency for pyright-langserver startup in a repo with many files. (Didn't root cause it yet. Very odd because the file watcher runs in its own thread.) I'm happy to test your implementation for this issue.

krobelus avatar Mar 10 '24 04:03 krobelus

part of the work done here has already been merged with #7665

glehmann avatar May 07 '24 05:05 glehmann