Rewrite the language server on top of a `ruff_server` fork #126

DavisVaughan · 2025-01-03T17:04:17Z

This PR is a huge move away from tower-lsp, instead implementing our language server on top of lsp-types and lsp-server directly. I believe this has some really nice advantages, and is something we have been discussing for some time now.

In particular, I've implemented this by effectively forking ruff_server, dropping everything we don't need, and layering in all of our own additions (viewFile, test clients, minimal text edits in format responses, etc).

Some huge benefits:

Drops all async code from the language server. We now use a scheduler that handles requests/notifications in one of 3 ways:
- Sync requests/notifs are handled on the main thread, and block
- Background requests/notifs are handled on a background thread with its own thread pool (typically 4 threads)
- Format requests have a special background thread pool since that is highly latency sensitive (note that sync requests that are in front of the format request still have to complete first, but this means it wont compete with other background requests)
- As a side note, it seems nice that the cli no longer needs tokio as a dependency, probably making the cli more portable and easier to build on more platforms?
Drops tower-lsp, which is currently not very well maintained and has given us some issues in the past (which we have successfully worked around, but we knew eventually we'd have to switch away)
Completely non-locking implementation. Background threads get a DocumentSnapshot (this could expand to include more things) containing an Arc<TextDocument> and Arc<Settings> that they are free to use without having to, say, lock a Mutex. If the main thread gets a Sync request/notif that needs to update a TextDocument or Settings while a background thread is working with it, the main thread will clone that TextDocument before modifying it. We should try extremely hard to avoid Mutexes in background threads going forward.
More control over request/notification handling, including some nice ergonomics improvements where Initialize and Initialized are handled before you enter the actual main loop, since those are "one time" requests/notifs. We also have full control over shutdown/exit.
A very solid code base to build from and reference going forward. The ruff code base itself seems inspired by rust-analyzer, so we are inheriting a combination of the two, and can reference both going forward as potential sources of inspiration. Both also use lsp-server, which is good news for us in terms of maintainability of that crate.

Some other improvements:

Better testing! We now have a global test client that is used across unit tests. It is stored behind a Mutex, which forces unit tests that require the client to run sequentially, while still allowing all other tests to run in parallel without issue. You use with_client(|client| { }) in a unit test to access it. You just have to be thoughtful to not do something in your unit test that puts the client in a state that affects other unit tests. If you need to destructively modify the client for a test, you create an integration test instead (see tests/initialization.rs which tests start up and shut down).
- Additionally, because we only have 1 client per process and the client tests are forced to run sequentially, using -- --nocapture actually makes sense now even without forcing 1 thread!

Because it is really nice to bind the `contents` to its `index` so you dont have to pass them around separately

Allowing us to remove our special casing code in logging/messaging

To be able to even more cheaply create a `DocumentQuery`

…tings Allowing us to show useful toast notifications when something goes wrong

Since our client tests are now synchronized through a mutex

DavisVaughan · 2025-01-03T17:35:29Z

LICENSE

+The `server` crate is a fork of `ruff_server`:
+
+- URL: https://github.com/astral-sh/ruff/tree/main/crates/ruff_server
+- Commit: aa429b413f8a7de9eeee14f8e54fdcb3b199b7b7
+- License: MIT
+
+MIT License
+
+Copyright (c) 2022 Charles Marsh
+


I tried the idea of putting some kind of header info in each file but it was not maintainable with the amount of changes I was also making to those files (deleting some, merging others, completely rewriting some, etc)

DavisVaughan · 2025-01-03T17:39:24Z

crates/server/src/message.rs

+
+use crate::server::ClientSender;
+
+static MESSENGER: OnceLock<ClientSender> = OnceLock::new();


In addition to logging.rs, this is the one other piece of global state we now have

This allows us to have free functions like show_err_msg!() that sends ShowMessage notifications to the client to pop up toast notifications for the user when something massive has gone wrong

DavisVaughan · 2025-01-03T17:43:51Z

crates/server/src/server/api/notifications/did_change.rs

+pub(crate) struct DidChange;
+
+impl super::NotificationHandler for DidChange {
+    type NotificationType = notif::DidChangeTextDocument;
+}
+
+impl super::SyncNotificationHandler for DidChange {
+    fn run(
+        session: &mut Session,
+        _notifier: Notifier,
+        _requester: &mut Requester,
+        types::DidChangeTextDocumentParams {
+            text_document:
+                types::VersionedTextDocumentIdentifier {
+                    uri,
+                    version: new_version,
+                },
+            content_changes,
+        }: types::DidChangeTextDocumentParams,
+    ) -> Result<()> {
+        let key = session.key_from_url(uri);
+
+        session
+            .update_text_document(&key, content_changes, new_version)
+            .with_failure_code(ErrorCode::InternalError)?;
+
+        Ok(())
+    }
+}


This is what "implementing" a sync handler looks like. We implement a super trait of NotificationHandler that gets the notification type, and then a more specific SyncNotificationHandler (or BackgroundNotificationHandler) trait that actually handles the request/notif.

In this case, since this is a sync request we have full mutable access to the server Session, so we just forward the information on to session.update_text_document()

DavisVaughan · 2025-01-03T17:45:53Z

crates/server/src/server/api/requests/view_file.rs

+#[derive(Debug, PartialEq, Clone, Deserialize, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub(crate) struct ViewFileParams {
+    /// From `lsp_types::TextDocumentPositionParams`
+    pub(crate) text_document: lsp_types::TextDocumentIdentifier,
+    pub(crate) position: lsp_types::Position,
+
+    /// Viewer type
+    pub(crate) kind: ViewFileKind,
+}
+
+#[derive(Debug)]
+pub(crate) enum ViewFile {}
+
+impl Request for ViewFile {
+    type Params = ViewFileParams;
+    type Result = String;
+    const METHOD: &'static str = "air/viewFile";
+}
+
+impl super::RequestHandler for ViewFile {
+    type RequestType = ViewFile;
+}
+
+impl super::BackgroundDocumentRequestHandler for ViewFile {


This is what implementing a custom request looks like. We have to implement lsp_types::request::Request for our custom request and supply the params/result types, but it's pretty easy!

DavisVaughan · 2025-01-03T17:51:51Z

crates/server/src/server/client.rs

+    /// Sends a request of kind `R` to the client, with associated parameters.
+    /// The task provided by `response_handler` will be dispatched as soon as the response
+    /// comes back from the client.
+    pub(crate) fn request<R>(


Typically the server receives requests/notifs from the client, but in a few places we will also send requests/notifs to the client, possibly in the middle of some other request.

That is handled too! We have access to a client.requester that we can send along a request through. When the response comes back, a handler that we provided is immediately run. The response is matched up to our handler through a RequestId that is created during the request.

We use this to register dynamic capabilities, like didChangeWatchedFiles server support

DavisVaughan · 2025-01-03T17:55:06Z

crates/server/src/session.rs

+/// The global state for the LSP
+pub(crate) struct Session {
+    /// Used to retrieve information about open documents and settings.
+    index: index::Index,
+    /// The global position encoding, negotiated during LSP initialization.
+    position_encoding: PositionEncoding,
+    /// Tracks what LSP features the client supports and doesn't support.
+    resolved_client_capabilities: ResolvedClientCapabilities,
+}
+
+/// An immutable snapshot of `Session` that references
+/// a specific document.
+pub(crate) struct DocumentSnapshot {
+    document_ref: index::DocumentQuery,
+    position_encoding: PositionEncoding,
+}


The Session is like our LspState and the DocumentSnapshot is like our WorldState

We can expand on these as needed going forward, but should be careful about what we put in DocumentSnapshot

DavisVaughan · 2025-01-03T17:56:37Z

crates/server/src/test/client.rs

+/// Global test client used by all unit tests
+///
+/// The language [Server] has per-process global state, such as the global `tracing`
+/// subscriber and a global `MESSENGER` to send `ShowMessage` notifications to the client.
+/// Because of this, we cannot just repeatedly call [test_client()] to start up a new
+/// client/server pair per unit test. Instead, unit tests use [with_client()] to access
+/// the global test client, which they can then manipulate. Synchronization is managed
+/// through a [Mutex], ensuring that multiple unit tests that need to mutate the client
+/// can't run simultaneously (while still allowing other unit tests to run in parallel).
+/// Unit tests should be careful not to put the client/server pair into a state that
+/// prevents other unit tests from running successfully.
+///
+/// If you need to modify a client/server pair in such a way that no other unit tests will
+/// be able to use it, create an integration test instead, which runs in its own process.
+static TEST_CLIENT: LazyLock<Mutex<TestClient>> = LazyLock::new(|| Mutex::new(test_client()));
+
+pub(crate) fn with_client<F>(f: F)
+where
+    F: FnOnce(&mut server_test::TestClient),
+{
+    let mut client = TEST_CLIENT.lock().unwrap();
+    f(&mut client)
+}


Our global test client

DavisVaughan added 30 commits December 24, 2024 10:08

Temp

2a8b891

Review ruff_source_file, merge in our line ending tools

cfc19e9

Drop ruff_text_size in favor of biome's

95487aa

Add in support for view ViewFile LSP method

7ae11db

Eat unused setTrace notification

b3a24eb

Add range formatting support, add test client support

cd1705b

Extract out TextSize proto bits

f94e493

Check for incoming messages during shutdown()

1f4605a

Assert same RequestId on shutdown

fd66159

Use ErrorVec approach in DidChangeWorkspace

11fbbb2

Add tracing

3c58e80

Move FormatResponse around

10fe146

Make define_document_url less magical

d3212bd

Nuke the old lsp

4bb77d3

Rename ruff_source_file to source_file

80fb276

Rename ruff_server to server

e937806

Remove ruff attribution in code blocks (will be added elsewhere)

0c5b612

Remove internal references to ruff

cde63e9

cargo machete clean up

455c767

Remove ruff specific TODOs

c9142a5

Don't need resolved capabilities in snapshots right now

1739f0c

Remove some dead_codes

3e40f5a

Spacing

dbc7ae6

Prefer anyhow::Result not crate::Result

5daa24a

Add a SourceFile for ergonomics

5084673

Because it is really nice to bind the `contents` to its `index` so you dont have to pass them around separately

Rename edit to document

f7c251f

Use a global test client accessible through with_client()

10ba9ce

Allowing us to remove our special casing code in logging/messaging

Separate settings discovery from SettingsResolver

8368ea2

Use Arc<Settings> in the LSP

ca41f92

To be able to even more cheaply create a `DocumentQuery`

Add a test for AIR_CRATE_NAMES

cf6715a

DavisVaughan added 7 commits January 3, 2025 10:12

Adjust LICENSE to reflect server fork

1391463

Internally handle errors when opening/closing/reloading workspace set…

32f8dcf

…tings Allowing us to show useful toast notifications when something goes wrong

Remove not needed clone()

bbd2a47

Remove unused clippy allow

951794c

Simplify by letting apply_changes() handle the empty case

2c58036

Add some documentation around the make_mut() call

a959982

We use >=1.83 MSRV, so can use PanicHookInfo directly

8c1ae65

DavisVaughan changed the base branch from main to feature/air-toml January 3, 2025 17:04

DavisVaughan added 2 commits January 3, 2025 12:15

Sort packages in AIR_CRATE_NAMES

b11f252

No longer need --test-threads 1

931f051

Since our client tests are now synchronized through a mutex

DavisVaughan commented Jan 3, 2025

View reviewed changes

Trim dependencies in the workspace toml

50af6f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the language server on top of a `ruff_server` fork #126

Rewrite the language server on top of a `ruff_server` fork #126

DavisVaughan commented Jan 3, 2025 •

edited

Loading

DavisVaughan Jan 3, 2025

DavisVaughan Jan 3, 2025

DavisVaughan Jan 3, 2025

DavisVaughan Jan 3, 2025

DavisVaughan Jan 3, 2025

DavisVaughan Jan 3, 2025

DavisVaughan Jan 3, 2025


		use crate::server::ClientSender;

		static MESSENGER: OnceLock<ClientSender> = OnceLock::new();

Rewrite the language server on top of a ruff_server fork #126

Are you sure you want to change the base?

Rewrite the language server on top of a ruff_server fork #126

Conversation

DavisVaughan commented Jan 3, 2025 • edited Loading

DavisVaughan Jan 3, 2025

Choose a reason for hiding this comment

DavisVaughan Jan 3, 2025

Choose a reason for hiding this comment

DavisVaughan Jan 3, 2025

Choose a reason for hiding this comment

DavisVaughan Jan 3, 2025

Choose a reason for hiding this comment

DavisVaughan Jan 3, 2025

Choose a reason for hiding this comment

DavisVaughan Jan 3, 2025

Choose a reason for hiding this comment

DavisVaughan Jan 3, 2025

Choose a reason for hiding this comment

Rewrite the language server on top of a `ruff_server` fork #126

Rewrite the language server on top of a `ruff_server` fork #126

DavisVaughan commented Jan 3, 2025 •

edited

Loading