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

Send structured InitializationOptions to the server #121

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DavisVaughan
Copy link
Collaborator

@DavisVaughan DavisVaughan commented Dec 20, 2024

Closes #100

Update: I am no longer convinced this is the right approach for most user or workspace level settings. These aren't strictly required at initialization time, unlike the initial logLevel and dependencyLogLevels, which are used in Initialize for logging setup (which we want ASAP). Instead, we should "pull" user and workspace level settings using a Configuration request once after Initialized for user and workspace settings, and again after workspace open events and changed file events (the only thing to worry about is that a Configuration request is typically async, so in theory we could get other requests while waiting on the response, but right now in practice all messages are sequential anyways). If we ever wanted logLevel to eventually be dynamic, it should come through initializationOptions AND also be "pull"ed alongside the other settings. See microsoft/language-server-protocol#567 which confirmed my beliefs about all this.

The game plan would be:

  • Trim back this PR to just send a flat set of options in initializationOptions. It would just be logLevel and dependencyLogLevels for now
  • Do a follow up PR after Add support for reading air.toml in the CLI and LSP #122 is merged to add support for "refreshable" user and workspace level options (which we don't have any of yet, but probably will...), supporting them in:
    • Initialized notification handler, where we'd request them for the first time (possibly also adding a lock to prevent any other requests from running until we've requested them and set them)
    • DidChangeWorkspaceFolders, requesting them for opened workspaces, purging them for closed workspaces
    • DidChangeConfiguration, where we use DidChangeConfiguration as a simple notification that "something" has changed in configuration, so we have to repull every scope we are interested in (this is now how you are supposed to use DidChangeConfiguration apparently)
    • Consider also "pulling" logLevel and dependencyLogLevels in addition to sending them in initializationOptions to make them dynamic, if we can figure out how to update those in the logger without adding global state.

The point of this PR is to send structured data through the InitializeParams "free field" of initializationOptions. This is usable by LSPs as a way to send initialization options specific to that LSP through on startup.

Currently, we send all 3 of ClientGlobalSettings, ClientUserSettings and ClientWorkspaceSettings through here, but only ClientGlobalSettings is populated (with log level settings). The other 2 are placeholders for now, but I wanted to get the infrastructure right.

The reason I am uncertain about user and workspace settings going through here is that:

  • Going through initializationOptions provides us no way to "refresh" the settings if they change
  • Going through initializationOptions provides us no way to access settings for newly opened workspaces that we get notified about through DidChangeWorkspaceFolders. Ruff currently has this problem as well (and they have a TODO about it on their side)

I am strongly considering instead having the air server send a Configuration LSP request to the Client immediately after we get an Initialized LSP notification. We will have the initial set of workspace names from the Initialize request, so we will be able to correctly "scope" our requests for "air." configuration settings (one request at user level, and then one request per open workspace).

The big benefit of that approach is that we can request and refresh Configuration at any time, including:

  • After Initialized, with the initial set of workspaces
  • After DidChangeWorkspaceFolders, with the newly opened workspace
  • After either a DidChangeConfiguration notification or DidChangeWatchedFiles notification where we watch settings.json files. In both cases we'd probably just trigger a full Configuration request for the relevant changed scope.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration

I do not believe there are any other benefits of using initializationOptions over Configuration for this. Even in Zed I believe you will just do:

# this
"lsp": {
  "air": {
    "logLevel": "error"
  }
}

# instead of this
"lsp": {
  "air": {
    "initialization_options": {
      "logLevel": "error"
    }
  }
}

In particular, this lets us respect `air.logLevel` and `air.dependencyLogLevels` on server startup

User and workspace level settings are not used yet but are included for completeness
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.

Hook up user specified options of air.logLevel and air.dependencyLogLevels in VS Code extension
1 participant