Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

LSP: didChangeWatchedFiles should watch all files matching the glob #11882

Closed
wldmr opened this issue Oct 14, 2024 · 8 comments
Closed

LSP: didChangeWatchedFiles should watch all files matching the glob #11882

wldmr opened this issue Oct 14, 2024 · 8 comments
Labels
A-language-server Area: Language server client C-enhancement Category: Improvements

Comments

@wldmr
Copy link

wldmr commented Oct 14, 2024

Summary

It seems that Helix only notifies language servers about files that it has currently open.

From the Spec, the intention seems to be that the client should inform the server about all files matching the glob given during registration:

Servers are allowed to run their own file system watching mechanism and not rely on clients to provide file system events. However this is not recommended […]

That's how I interpret the spec, anyway. Otherwise I'm not sure I see a difference between this and the normal document sync mechanisms.

If you agree that this is something that Helix should do, then I could offer to implement it. I'm going to have to add file watching one way or another, and I'd much rather have it happen on the client side (for the reasons given in the spec).

Reproduction Steps

I tried this:

  1. start language server that registers for workspace/didChangeWatchedFiles notifications
  2. change & write file A within Helix
  3. change & write file A outside Helix
  4. change & write file B outside Helix

I expected this to happen:

  • didChangeWatchedFiles notifications for all three changes

Instead, this happened:

  • Notification only happens for step 2.

Helix log

I've attached the log entries sent from my experimental LSP. I don't expect you want to go to the trouble of installing it (it's in a very works-on-my-machine kind of state). If you really want to, I'll amend the report with instructions.

~/.cache/helix/helix.log

Init params sent by Helix:

[…] ink-tool err <- "init params: {\n"
[…] ink-tool err <- "  \"rootPath\": \"/home/wldmr/devel/ink-tool\",\n"
[…] ink-tool err <- "  \"rootUri\": \"file:///home/wldmr/devel/ink-tool\",\n"
[…] ink-tool err <- "  \"capabilities\": {\n"
[…] ink-tool err <- "    \"workspace\": {\n"
[…] ink-tool err <- "      […]
[…] ink-tool err <- "      \"didChangeWatchedFiles\": {\n"
[…] ink-tool err <- "        \"dynamicRegistration\": true,\n"
[…] ink-tool err <- "        \"relativePatternSupport\": false\n"
[…] ink-tool err <- "      },\n"
[…] ink-tool err <- "      […]
[…] ink-tool err <- "  \"clientInfo\": {\n"
[…] ink-tool err <- "    \"name\": \"helix\",\n"
[…] ink-tool err <- "    \"version\": \"24.7 (079f5442)\"\n"
[…] ink-tool err <- "  }\n"
[…] ink-tool err <- "}\n"

Init result sent by LSP:

[…] ink-tool err <- "init result: {\n"
[…] ink-tool err <- "  \"capabilities\": {\n"
[…] ink-tool err <- "    \"documentSymbolProvider\": true,\n"
[…] ink-tool err <- "    \"hoverProvider\": true,\n"
[…] ink-tool err <- "    \"positionEncoding\": \"utf-8\",\n"
[…] ink-tool err <- "    \"textDocumentSync\": {\n"
[…] ink-tool err <- "      \"change\": 2,\n"
[…] ink-tool err <- "      \"openClose\": true\n"
[…] ink-tool err <- "    }\n"
[…] ink-tool err <- "  },\n"
[…] ink-tool err <- "  \"serverInfo\": {\n"
[…] ink-tool err <- "    \"name\": \"ink-tool\",\n"
[…] ink-tool err <- "    \"version\": \"0.1.0\"\n"
[…] ink-tool err <- "  }\n"
[…] ink-tool err <- "}\n"

Registering for all *.ink files, recursively:

[…] ink-tool err <- "dynamic registration request: {\n"
[…] ink-tool err <- "  \"id\": 0,\n"
[…] ink-tool err <- "  \"method\": \"client/registerCapability\",\n"
[…] ink-tool err <- "  \"params\": {\n"
[…] ink-tool err <- "    \"registrations\": [\n"
[…] ink-tool err <- "      {\n"
[…] ink-tool err <- "        \"id\": \"ink-files-watcher\",\n"
[…] ink-tool err <- "        \"method\": \"workspace/didChangeWatchedFiles\",\n"
[…] ink-tool err <- "        \"registerOptions\": {\n"
[…] ink-tool err <- "          \"watchers\": [\n"
[…] ink-tool err <- "            {\n"
[…] ink-tool err <- "              \"globPattern\": \"**/*.ink\"\n"
[…] ink-tool err <- "            }\n"
[…] ink-tool err <- "          ]\n"
[…] ink-tool err <- "        }\n"
[…] ink-tool err <- "      }\n"
[…] ink-tool err <- "    ]\n"
[…] ink-tool err <- "  }\n"
[…] ink-tool err <- "}\n"
[…] ink-tool err <- "[…]
[…] ink-tool err <- "got response: Response { id: RequestId(I32(0)), result: None, error: None }\n"

After writing showcase.ink from Helix (corresponding to step 2. above):

[…] ink-tool err <- "unhandled notification Notification { method: \"workspace/didChangeWatchedFiles\", params: Object {\"changes\": Array [Object {\"type\": Number(2), \"uri\": String(\"file:///home/wldmr/devel/ink-tool/showcase.ink\")}]} }\n"

(unhandled means there's no code to do anything with it yet, but the point is: the language server is notified)

Writing showcase.ink or showcase2.ink from another editor (steps 3. and 4. above) don't generate any such notifications.

Platform

Fedora Linux

Terminal Emulator

Konsole (KDE)

Installation Method

Fedora repos (dnf install helix)

Helix Version

helix 24.7 (079f544)

@wldmr wldmr added the C-bug Category: This is a bug label Oct 14, 2024
@the-mikedavis
Copy link
Member

See #1125. The file watcher itself is the hard part of this and @pascalkuthe is working on it as time allows. There is already #2653 which uses the notify crate (which we'd like to avoid) if you'd like to use an existing patch in the meantime

@the-mikedavis the-mikedavis added A-language-server Area: Language server client C-enhancement Category: Improvements and removed C-bug Category: This is a bug labels Oct 14, 2024
@wldmr
Copy link
Author

wldmr commented Oct 14, 2024

Oops. I swear I looked before opening this, but didn't think to look for PRs. And dismissed #1125 as not quite addressing this.

So I'm fine with nixing this issue, if this is covered by #1125 and others. Didn't want to add to the clutter.

I'll look into the patch, thanks for the pointer. :)

@wldmr
Copy link
Author

wldmr commented Oct 15, 2024

After sleeping on this another night, I have some more thoughts: I do consider this a bug, because Helix advertises a feature but implements it incompletely.

Can I suggest locking the didChangeWatchedeFiles support behind a compile time feature (or maybe run time config), until it is fully baked? As it stands, servers may decide to rely on the client capabilities to decide whether to start their own file watcher, leading to surprising results.

Looks like this is the only issue that stumbled over it, so maybe it's not that big of a deal. Just throwing it out there.

@the-mikedavis
Copy link
Member

Is this a practical issue with a language server or theoretical? I don't know of any language servers that choose to start their own file watchers if the client doesn't advertise support. Unless the current implementation causes problems I don't see us disabling it or adding a way to configure out of it.

@wldmr
Copy link
Author

wldmr commented Oct 15, 2024

I don't know of any either, other than the one I'm currently writing (and I only thought of doing it this way after opening this issue). That's why I said "no big deal". If you're on this and it'll straighten itself out eventually, that's fine.

But I still think my point stands in principle: The whole point of the capability negotiation is that both sides can adapt their behavior based on what is advertised. If the server subscribes to file updates but doesn't get the ones it asked for, then the server will not function correctly, through no fault of its own.


OK, so I had a look at rust-analyzer, and it looks like it behaves that way as well:

https://github.com/rust-lang/rust-analyzer/blob/418c1365eccf20c9261b6948a6e637f789224af9/crates/rust-analyzer/src/config.rs#L1796-L1806

    pub fn files(&self) -> FilesConfig {
        FilesConfig {
            watcher: match self.files_watcher() {
                FilesWatcherDef::Client if self.did_change_watched_files_dynamic_registration() => {
                    FilesWatcher::Client
                }
                _ => FilesWatcher::Server,
            },
            exclude: self.files_excludeDirs().iter().map(|it| self.root_path.join(it)).collect(),
        }
    }

And then in reload.rs it registers for didChangeWatchedFiles if this is set to Client, and sets up its own vfs watcher if set to Server. (At least I think that's what's going on; I just skimmed the code).

@pascalkuthe
Copy link
Member

Rust analyzer does not have its own file watcher so it just does not do file watching in case the client doesn't supply it (because there is no file watcher of sufficient quality available in rust as writing one is genuienly very hard)

@wldmr
Copy link
Author

wldmr commented Oct 16, 2024

Rust analyzer does not have its own file watcher

It does:

  • Build helix with did_change_watched_files: None (let's call that one hx-no-watch).
  • Open helix-lsp/src/client.rs in both hx-no-watch and regular hx
  • Open helix-lsp/src/jsonrpc.rs in another editor and add V3 to the Version enum
  • hx-no-watch autocompletes the new variant right away while hx doesn't

So I don't think this problem is theoretical anymore.

@pascalkuthe
Copy link
Member

You can manually set this config option to enable Server side file watching but it's certainly not something recommended and not enabled by default. The watcher in RA is most likely buggy and just a placeholder:

https://github.com/rust-lang/rust-analyzer/blob/418c1365eccf20c9261b6948a6e637f789224af9/crates/vfs-notify/src/lib.rs#L3

What we have right now us a good intermediary solution, that I don't want to hide behind a feature gate. If you want to remove it then you can patch it out in your fork. The right solution is to build a proper file watcher into helix (for which we already have an issue)

@helix-editor helix-editor locked and limited conversation to collaborators Oct 16, 2024
@pascalkuthe pascalkuthe converted this issue into discussion #11903 Oct 16, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
A-language-server Area: Language server client C-enhancement Category: Improvements
Projects
None yet
Development

No branches or pull requests

3 participants