Skip to content

resolve relative paths from workspace/didChangeConfiguration #2372

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

Conversation

dotcarmen
Copy link
Contributor

@dotcarmen dotcarmen commented Jun 24, 2025

ok admittedly i made the last changes to #2331 without thorough testing.

when I tried to use the feature from #2331 just now, i found out that it wasn't working with the following .zed/settings.json i have for my ziglang/zig checkout:

// see the documentation: https://zed.dev/docs/configuring-zed#settings-files
{
  "lsp": {
    "zls": {
      "settings": {
        "builtin_path": "./src/Builtin.zig",
        "zig_lib_path": "./lib",
        "zig_exe_path": "./build/stage3/bin/zig",
        "build_runner_path": "./lib/compiler/build_runner.zig",
      }
    }
  }
}

(ignore the incorrect builtin_path pls)

after further debugging, i found 2 issues:

  1. zed is sending these settings as part of workspace/didChangeConfiguration notifications. Right now zls only correctly resolves relative paths that are a response to workspace/configuration server-side requests (in other words, client messages with i_haz_configuration id)
  2. because resolve relative paths in dynamic configurations #2331 only used std.fs.path.join to resolve the relative paths, ZLS seemed to have some double-accounting of the paths. at some point, I had a log message indicating that both ..././src/Builtin.zig and .../src/Builtin.zig were being watched by ZLS. which is redundant and presumably a (minor) waste of memory

this PR accounts for both of these issues:

  • logic for resolving relative paths have been moved back to updateConfiguration method on Server
    • this only happens when options.resolve_relative_paths argument is true
    • this is directly opposed to this review comment which was addressed in resolve relative paths in dynamic configurations #2331.
      • the review comment mentions that only handleConfiguration should be receiving messages with relative configuration paths. As noted above, this is not true - it should also handle relative paths in workspace/didChangeConfiguration notifications, which is handled by didChangeConfigurationHandler
      • I think it's better to handle this behavior in updateConfiguration since handleConfiguration and didChangeConfigurationHandler have different handling of the message
      • later on, when zls receives the i_haz_configuration response from the client, it seems Zed is not re-sending the updated relative paths. while the workspace/configuration model seems to be the more modern feature, it seems Zed still expects workspace/didChangeConfiguration to handle the initial values from .zed/settings.json
  • instead of using std.fs.path.join to resolve relative paths, use std.fs.path.resolve.

@dotcarmen
Copy link
Contributor Author

@Techatrix let me know if there's another way this should be handled. sorry for not testing again before merging, last few weeks were busy and i was focused on other projects 😬

@Techatrix
Copy link
Member

According to Zed's client capabilities, they claim to support workspace/configuration so this workaround shouldn't be necessary in theory. But they also send configuration value with workspace/didChangeConfiguration and ZLS does not handle this well. I will need to investigate this further to figure out what to do here. I will keep you updated once I have something more concrete to say.

@Techatrix
Copy link
Member

With the changes in #2416, ZLS will now properly use workspace/configuration with Zed and various other editors instead of relying on the outdated workspace/didChangeConfiguration request to receive config options. This means that the changes in this PR should no longer be needed to get this working. Feel free to let me know if #2416 works well for you.

@Techatrix Techatrix closed this Jul 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants