feat: support workspace/didChangeWatchedFiles for imported files

[WillLillis]

This adds support for workspace/didChangeConfiguration. I have a few questions/ points of interest:

• I ran into some minor issues with with the FileSystemWatcher's GlobPattern.

  • Trying out just "**/*.{zig,zon}" (as well as "**/*.zig") caused the server to receive no notifications. I'm not sure if that was an issue with my implementation and/or Neovim's client implementation.
  • I opted to go with the basic Pattern glob because the RelativePattern glob is behind the capability's registration params, so there may be some LSP clients that don't implement this feature.
  • Because registering for "**" includes all files in the project directory (including those in .zig-cache), I tried filtering out extraneous notifications by only applying updates to uris already in the document store. I believe this should cover all imported files, but I'm not 100% confident.

• Reading the entire file seems pretty expensive for every update, but it looks like the notification doesn't include any edit information. Maybe there's a smarter way around this?

• I wasn't sure if it was more better to use the document store's refreshDocument method in the .Changed case. The logic between .Created and .Changed would still be almost identical, except that refreshDocument takes a null terminated slice, and requires allocation via the document store.

• openDocument is marked as not thread safe. If this is to be used in the .Changed case, should some additional checks/ locking be added at the call site?

• I wasn't sure how to add tests for this.

Closes #2160

[Techatrix]

About the file system watch pattern being used: Not sure why neovim wouldn't be able to handle **/*.{zig,zon}. Perhaps it would make sense to look into neovim's implementation to check if is a bug. We should also look at other editors to check whether it works and if they have the same limitation as neovim. I will try to look into that after the rest of my comment has been addressed.

About the usage of refreshDocument and openDocument: It is important to note that the LSP protocol has a document synchronization mechanism that is independent of the actual file system. The openDocument, closeDocument and refreshDocument functions in ZLS are meant to implement this. Even if the document on the file system has been modified as notified by workspace/didChangeConfiguration, this event must be ignored if the document is synchronized over the protocol. I see now that the documentation in DocumentStore.zig is insufficiently explaining these requirements so I will look into improve them.

To resolve this issue, I would suggest to add the following (untested) function to the DocumentStore:

/// **Thread safe** takes an exclusive lock when called on different documents
/// **Not thread safe** when called on the same document
pub fn refreshDocumentFromFileSystem(self: *DocumentStore, uri: Uri) !void {
    {
        const handle = self.getHandle(uri) orelse return;
        if (handle.getStatus().open) return;
    }

    {
        self.lock.lock();
        defer self.lock.unlock();

        const kv = self.handles.fetchSwapRemove(uri) orelse return;
        log.debug("Closing document {s}", .{kv.key});
        kv.value.deinit();
        self.allocator.destroy(kv.value);
    }
}

This function will essentially remove the document from the document store unless it synchronized over the protocol. If the file system watching reports that the document has been modified or deleted, this function can be called. The existing logic in ZLS will read the new state from disk if it is needed again. This is why there is need to do anything when a file has been created.

Also after parsing the uri from the client, the file extension should be checked.

• I wasn't sure how to add tests for this.

The document store lacks testing for now so there is no easy way to add tests for this. Should be fine for now.

[WillLillis]

About the usage of refreshDocument and openDocument: It is important to note that the LSP protocol has a document synchronization mechanism that is independent of the actual file system. The openDocument, closeDocument and refreshDocument functions in ZLS are meant to implement this. Even if the document on the file system has been modified as notified by workspace/didChangeConfiguration, this event must be ignored if the document is synchronized over the protocol. I see now that the documentation in DocumentStore.zig is insufficiently explaining these requirements so I will look into improve them.
To resolve this issue, I would suggest to add the following (untested) function to the DocumentStore:

/// **Thread safe** takes an exclusive lock when called on different documents
/// **Not thread safe** when called on the same document
pub fn refreshDocumentFromFileSystem(self: *DocumentStore, uri: Uri) !void {
    {
        const handle = self.getHandle(uri) orelse return;
        if (handle.getStatus().open) return;
    }

    {
        self.lock.lock();
        defer self.lock.unlock();

        const kv = self.handles.fetchSwapRemove(uri) orelse return;
        log.debug("Closing document {s}", .{kv.key});
        kv.value.deinit();
        self.allocator.destroy(kv.value);
    }
}

This function will essentially remove the document from the document store unless it synchronized over the protocol. If the file system watching reports that the document has been modified or deleted, this function can be called. The existing logic in ZLS will read the new state from disk if it is needed again. This is why there is need to do anything when a file has been created.

Ah ok. It sounds obvious now, I just didn't realize those functions were so intended to go hand in hand with textDocument/didOpen etc. Thanks for the explanation! I tried out refreshDocumentFromFileSystem locally and it's working great.

Also after parsing the uri from the client, the file extension should be checked.

Gotcha. I'm assuming just checking for .zig or .zon should be ok?

About the file system watch pattern being used: Not sure why neovim wouldn't be able to handle **/*.{zig,zon}. Perhaps it would make sense to look into neovim's implementation to check if is a bug. We should also look at other editors to check whether it works and if they have the same limitation as neovim. I will try to look into that after the rest of my comment has been addressed.

Sounds good. I'll push up the other changes for now, and spend some time debugging Neovim's implementation.

[WillLillis]

Update: I haven't been able to pin down what's wrong in NeovimI bisected and believe I have the issue figured out (neovim/neovim#29865 (comment)). The **/*.{zig,zon} pattern does work in Zed.

Since this appears to be a neovim issue, maybe a workaround like this makes sense for now?

diff --git a/src/Server.zig b/src/Server.zig
index 5b9d687..43aeceb 100644
--- a/src/Server.zig
+++ b/src/Server.zig
@@ -629,7 +629,12 @@ fn initializedHandler(server: *Server, arena: std.mem.Allocator, notification: t
     if (server.client_capabilities.supports_workspace_did_change_watched_files) {
         // `{ "watchers": [ { "globPattern": "**" } ] }`
         var watcher = std.json.ObjectMap.init(arena);
-        try watcher.put("globPattern", .{ .string = "**" });
+        if (std.mem.eql(u8, "Neovim", server.client_capabilities.client_name orelse "")) {
+            try watcher.put("globPattern", .{ .string = "**" });
+        } else {
+            try watcher.put("globPattern", .{ .string = "**/*.{zig,zon}" });
+        }
         var watchers_arr = try std.ArrayList(std.json.Value).initCapacity(arena, 1);
         watchers_arr.appendAssumeCapacity(.{ .object = watcher });
         var fs_watcher_obj: std.json.ObjectMap = std.json.ObjectMap.init(arena);

[Techatrix]

Since this appears to be a neovim issue, maybe a workaround like this makes sense for now?

I think that's reasonable. But instead of checking checking the client name right where the glob pattern is created, I would suggest some changes to be more in line with how ZLS handles other editor specific workarounds:

• add a new field to ClientCapabilities that represents this workaround. The name of the field is up to you, just make sure to add a link to the neovim issue so it can be tracked more easily. As a reference, the max_detail_len field is a workaround for Sublime Text.
• add a check for neovim that enables this new field here
• adjust the diff to check for the new field instead of the client name

[Techatrix]

I checked a few editors and here is what I found:

• VS Code - wasn't working in some cases which should be fixed by 8468899
• Sublime Text - client capabilities say that it is supported by no notifications are send
• JetBrains IDEs - ok
• Helix - unsupported
• Zed - ok
• NeoVim - Isn't enabled by default on Linux but was working when manually enabled
• Emacs - too lazy to setup emacs again
• Kate - unsupported

I also tested the **/*.{zig,zon} pattern in neovim which was surprisingly working for me. Perhaps the issue you are experiencing is tied to the file watch backend that is used by neovim. Could you check what :checkhealth vim.lsp outputs for you about file watching?

Here is what I get after manually configuring neovim to enable workspace.didChangeWatchedFiles.dynamicRegistration:

vim.lsp: File Watcher ~
- File watch backend: libuv-watchdirs
- WARNING libuv-watchdirs has known performance issues. Consider installing inotify-tools.

After installing inotify-tools, I got the following output while the **/*.{zig,zon} pattern was still working:

vim.lsp: File Watcher ~
- File watch backend: Custom (@/nix/store/sm3ycnp9hcqvq1bpnq45164wvbdk4f4z-neovim-unwrapped-0.11.0/share/nvim/runtime/lua/vim/_watch.lua)

[WillLillis]

I checked a few editors and here is what I found:

* VS Code - wasn't working in some cases which should be fixed by [8468899](https://github.com/zigtools/zls/commit/8468899b803a4ebc2f0e13e887e5b1aa13abf08e)

* Sublime Text - client capabilities say that it is supported by no notifications are send

* JetBrains IDEs - ok

* Helix - unsupported

* Zed - ok

* NeoVim - Isn't enabled by default on Linux but was working when manually enabled

* Emacs - too lazy to setup emacs again

* Kate - unsupported

Awesome, thanks so much for checking all of these!

I also tested the **/*.{zig,zon} pattern in neovim which was surprisingly working for me. Perhaps the issue you are experiencing is tied to the file watch backend that is used by neovim. Could you check what :checkhealth vim.lsp outputs for you about file watching?

Here is what I get after manually configuring neovim to enable workspace.didChangeWatchedFiles.dynamicRegistration:

vim.lsp: File Watcher ~
- File watch backend: libuv-watchdirs
- WARNING libuv-watchdirs has known performance issues. Consider installing inotify-tools.

After installing inotify-tools, I got the following output while the **/*.{zig,zon} pattern was still working:

vim.lsp: File Watcher ~
- File watch backend: Custom (@/nix/store/sm3ycnp9hcqvq1bpnq45164wvbdk4f4z-neovim-unwrapped-0.11.0/share/nvim/runtime/lua/vim/_watch.lua)

I got the same warning about libuv-watchdirs after running checkhealth. After installing inotify-tools, the **/*.{zig,zon} pattern worked for me! I uninstalled inotify-tools after just to make sure and the pattern still doesn't work for with the libuv-watchdirs backend. What commit is your neovim install built from? I bisected the regresion to neovim/neovim@b87505e.

I also noticed the ** pattern causes some spurious errors to be printed about watching files in .zig-cache when the inotify-tools backend is in use. Given this, Do you think it makes sense to just use the **/*.{zig,zon} pattern?

[Techatrix]

What commit is your neovim install built from?

I used the 0.11.0 release.

I uninstalled inotify-tools after just to make sure and the pattern still doesn't work for with the libuv-watchdirs backend.

This is the thing that is confusing me. Both backends are working for me. If am I understanding this correctly that you are saying that the issues occurs since neovim/neovim@b87505e? So how can I not reproduce this issue with 0.11?

Also, did you need to manually configure neovim to enable the workspace.didChangeWatchedFiles.dynamicRegistration client capability like this:

init.lua

vim.lsp.config['zls'] = {
    cmd = { '/home/techatrix/repos/zls/zig-out/bin/zls' },
    filetypes = { 'zig' },
    root_markers = { 'build.zig' },
    settings = {
        zls = {
            zig_exe_path = '/home/techatrix/.config/Code/User/globalStorage/ziglang.vscode-zig/zig/linux-x86_64-0.15.0-dev.151+6e8493daa/zig'
        }
    },
    capabilities = {
        workspace = {
            didChangeWatchedFiles = {
                dynamicRegistration = true,
            }
        }
    },
}
vim.lsp.enable('zls')

---

I also tested windows where neovim was behaving as expected. There is just a minor issue with ZLS around missing uri escape sequence normalization which I will resolve later.

[WillLillis]

I uninstalled inotify-tools after just to make sure and the pattern still doesn't work for with the libuv-watchdirs backend.

This is the thing that is confusing me. Both backends are working for me. If am I understanding this correctly that you are saying that the issues occurs since neovim/neovim@b87505e? So how can I not reproduce this issue with 0.11?

I'm honestly not sure, but I'll spend some more time looking into it.

Also, did you need to manually configure neovim to enable the workspace.didChangeWatchedFiles.dynamicRegistration client capability like this:

init.lua

vim.lsp.config['zls'] = {

    cmd = { '/home/techatrix/repos/zls/zig-out/bin/zls' },

    filetypes = { 'zig' },

    root_markers = { 'build.zig' },

    settings = {

        zls = {

            zig_exe_path = '/home/techatrix/.config/Code/User/globalStorage/ziglang.vscode-zig/zig/linux-x86_64-0.15.0-dev.151+6e8493daa/zig'

        }

    },

    capabilities = {

        workspace = {

            didChangeWatchedFiles = {

                dynamicRegistration = true,

            }

        }

    },

}

vim.lsp.enable('zls')

---

Yep!

I also tested windows where neovim was behaving as expected. There is just a minor issue with ZLS around missing uri escape sequence normalization which I will resolve later.

👍

[Techatrix]

I believe I found the cause of the inconsistency between us. When I do the same steps as #2160 but move mod.zig into its own subdirectory like src/mod.zig, I get the same issue. Based on neovim/neovim#29865 (comment) I assume that this is how you are testing it as well, right?

So if the issue only occurs on linux where file watching is disabled by default and the libuv-watchdirs backend is warned against anyway, I think we can just forget about the workaround and always use the **/*.{zig,zon} pattern.

[WillLillis]

I believe I found the cause of the inconsistency between us. When I do the same steps as #2160 but move mod.zig into its own subdirectory like src/mod.zig, I get the same issue. Based on neovim/neovim#29865 (comment) I assume that this is how you are testing it as well, right?

Yeah, sorry I should have mentioned that. I was testing this in a template project created by zig init.

So if the issue only occurs on linux where file watching is disabled by default and the libuv-watchdirs backend is warned against anyway, I think we can just forget about the workaround and always use the **/*.{zig,zon} pattern.

👍

[WillLillis]

Thank you so much for all the help! @Techatrix