`ruff server` - A new built-in LSP for Ruff, written in Rust #10158

snowsignal · 2024-02-29T01:29:22Z

Summary

This PR introduces the ruff_server crate and a new ruff server command. ruff_server is a re-implementation of ruff-lsp, written entirely in Rust. It brings significant performance improvements, much tighter integration with Ruff, a foundation for supporting entirely new language server features, and more!

This PR is an early version of ruff_lsp that we're calling the pre-release version. Anyone is more than welcome to use it and submit bug reports for any issues they encounter - we'll have some documentation on how to set it up with a few common editors, and we'll also provide a pre-release VSCode extension for those interested.

This pre-release version supports:

Diagnostics for .py files
Quick fixes
Full-file formatting
Range formatting
Multiple workspace folders
Automatic linter/formatter configuration - taken from any pyproject.toml files in the workspace. (this will be implemented in a follow-up pull request)

Many thanks to @MichaReiser for his proof-of-concept work, which was important groundwork for making this PR possible.

Architectural Decisions

I've made an executive choice to go with lsp-server as a base framework for the LSP, in favor of tower-lsp. There were several reasons for this:

I would like to avoid async in our implementation. LSPs are mostly computationally bound rather than I/O bound, and async adds a lot of complexity to the API, while also making harder to reason about execution order. This leads into the second reason, which is...
Any handlers that mutate state should be blocking and run in the event loop, and the state should be lock-free. This is the approach that rust-analyzer uses (also with the lsp-server/lsp-types crates as a framework), and it gives us assurances about data mutation and execution order. tower-lsp doesn't support this, which has caused some issues around data races and out-of-order handler execution.
In general, I think it makes sense to have tight control over scheduling and the specifics of our implementation, in exchange for a slightly higher up-front cost of writing it ourselves. We'll be able to fine-tune it to our needs and support future LSP features without depending on an upstream maintainer.

Test Plan

The pre-release of ruff_server will have snapshot tests for common document editing scenarios. An expanded test suite is on the roadmap for future version of ruff_server.

MichaReiser · 2024-03-01T17:13:06Z

@snowsignal How would you recommend to review this PR? Which parts of the PR are in its final state? Are there parts that you plan to iterate on in future PRs?

MichaReiser

This looks very exciting. I haven't managed to get through everything yet. I plan to continue my review on Monday.

I would appreciate a more in depth explanation of the architecture and why you decided to go with lsp-server over tower-lsp. Documenting this decision will be useful for the future.

Cargo.toml

crates/ruff/src/args.rs

crates/ruff_server/src/edit/document.rs

crates/ruff_server/src/lib.rs

crates/ruff_server/src/lint.rs

crates/ruff_server/src/server.rs

crates/ruff_server/src/server/schedule/thread/priority.rs

MichaReiser

As said before. This is excellent work. I haven't reviewed everything in detail, the PR is too large for that.

I think this work deserves a more in depth explanation of the architecture decisions. I like the simplicity of the handlers and that mutating tasks run on a dedicated thread. However, it comes at the cost that cancellation, the entire scheduling, dispatching, are problems that we now need to solve. That's why I'm interested on hearing your perspective on how to balance these tradeoffs and the complexity of implementing said features (I'm especially interested in cancellation).

I would find some additional comments useful that explain some concepts, especially around scheduling, how requests/notifications work etc. I can see that you have put a lot of thought into it, let's make sure that other contributors are aware of your deliberate design choices.

It might be nice if some of the scheduling work could be tested too. Although I fear that this isn't going to be easy.

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

MichaReiser · 2024-03-04T13:23:37Z

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

+        _notifier: Notifier,
+        params: types::DidChangeTextDocumentParams,
+    ) -> Result<()> {
+        super::define_document_url!(params: &types::DidChangeTextDocumentParams);


I don't fully understand why using a macro is necessary and it isn't obvious what's happening here (or where document_url is coming from). I assume it is because all params have a different shape. How about we define a trait DocumentUrl with a single document_url method (I don't like the trait name, maybe Document?) that we could then pass to the document_controller

Or we remove the macro because the implementation is trivial?

This macro was created as a way to avoid the repeated boilerplate of writing a function that returned &params.text_document.uri from an arbitrary parameter type in lsp-types (a parameter type is a deserialized message payload for requests and notifications). Since these types don't have a shared function to get the Url, even though they all follow the same internal layout, it needs to be a concrete function for each parameter type.

The reason why we need to define this function for each background handler is because we need a generic way to extract the document URL from the parameters of a request/notification, in order to take a session snapshot. Here, though, it's just defined for convenience.

That makes sense. IMO, the macro here seems overkill. The code it generates is minimal and very easy to write by hand. However, it adds significant complexity to readers because they have to open the macro definition or read its documentation to understand what's happening inside.

That's why I recommend removing the macro and instead implementing the trait function by hand (I'm sure code pilot can autocomplete it for you)

crates/ruff_server/src/server/api/notifications/did_open.rs

crates/ruff_server/src/server/api/requests/code_action.rs

crates/ruff_server/src/server/api/requests/diagnostic.rs

crates/ruff_server/src/session.rs

crates/ruff_server/src/server/schedule/thread/pool.rs

crates/ruff_server/src/server/schedule/task.rs

crates/ruff_server/src/server/api.rs

BurntSushi

Wow! There is so much here! Really nice work. :-)

Other than the comments I left, I have some holistic feedback too:

It feels like there is a lot of interesting documentation that could be written for a lot of your types that would help clarify some of their behavior. I found that the comments that did exist were more on the "inside baseball" side of the spectrum (e.g., the comments about not implementing a trait method or the comments about why a type isn't just a closure) rather than "here is the contract the caller needs to care about." I like the former. They belong. They help contextualize why code is the way it is. But I'd really like to see more of the latter.
I found it somewhat difficult to review this PR without a "big picture" of how the code is structure. These are tricky docs to write, I'd suggest the first step in doing so is to pick a target audience. For example, for me, I know very little about LSP, but perhaps the architecture docs should assume that knowledge. But maybe not. I'm not sure.

Overall amazing work. I can't believe how fast you got this built!

crates/ruff_server/src/edit.rs

crates/ruff_server/src/edit/document.rs

crates/ruff_server/src/edit/range.rs

crates/ruff_server/src/server/api.rs

crates/ruff_server/src/server/api/traits.rs

crates/ruff_server/src/server/schedule/thread/pool.rs

crates/ruff_server/src/server/schedule/thread/priority.rs

dhruvmanila · 2024-03-05T17:12:42Z

Cargo.toml

 libcst = { version = "1.1.0", default-features = false }
 log = { version = "0.4.17" }
+lsp-server = { version = "0.7.6" }
+lsp-types = { version = "0.95.0", features = ["proposed"] }


I would recommend to use the lsprotocol library (https://github.com/microsoft/lsprotocol/tree/main/packages/rust/lsprotocol) as it'll always be in sync with the latest protocol. The ruff-lsp package also uses the Python version of it.

I think we'll have to make this a follow-up issue - this would be a pretty fundamental re-write.

MichaReiser

The code looks great. Thanks for addressing all the feedback.

What's still missing, in my view, is a discussion of the architectural decisions as part of the PR summary. We've covered some of them in our sync meeting but I think it is beneficial to write those down to have a reference for the future. That's the reason why I'm requesting feedback. Once that's done, feel free to merge.

As @BurntSushi pointed out, I think it would be good to have some more conceptual documentation. Maybe the README would be a good start. It doesn't have to be extensive but a brief explanation of the core concept involved.

crates/ruff_server/src/edit/document.rs

MichaReiser · 2024-03-08T09:31:23Z

crates/ruff_server/src/edit/range.rs

+            encoding,
+        ),
+    }
+}


I agree on this @snowsignal do you plan to follow up on this in a separate PR?

crates/ruff_server/src/server/api/requests/format.rs

crates/ruff_server/src/session.rs

crates/ruff_server/src/server/schedule.rs

crates/ruff_server/src/server/api/traits.rs

crates/ruff_server/src/server.rs

github-actions · 2024-03-09T04:56:16Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+13 -0 violations, +0 -0 fixes in 1 projects; 42 projects unchanged)

pandas-dev/pandas (+13 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pandas/io/parsers/python_parser.py:1039:47: PLR6201 Use a `set` literal when testing for membership
+ pandas/io/parsers/python_parser.py:1106:9: PLR1702 Too many nested blocks (6 > 5)
+ pandas/io/parsers/python_parser.py:1106:9: PLR1702 Too many nested blocks (6 > 5)
+ pandas/io/parsers/python_parser.py:1208:9: PLR0917 Too many positional arguments (6/5)
+ pandas/io/parsers/python_parser.py:1343:9: PLR6301 Method `_remove_empty_lines` could be a function, class method, or static method
+ pandas/io/parsers/python_parser.py:1358:40: PLC1901 `v == ""` can be simplified to `not v` as an empty string is falsey
+ pandas/io/parsers/python_parser.py:391:9: PLR0914 Too many local variables (25/15)
+ pandas/io/parsers/python_parser.py:399:9: PLR1702 Too many nested blocks (6 > 5)
+ pandas/io/parsers/python_parser.py:399:9: PLR1702 Too many nested blocks (7 > 5)
+ pandas/io/parsers/python_parser.py:449:29: PLC1901 `c == ""` can be simplified to `not c` as an empty string is falsey
+ pandas/io/parsers/python_parser.py:701:9: PLR6301 Method `_is_line_empty` could be a function, class method, or static method
+ pandas/io/parsers/python_parser.py:815:37: PLR6201 Use a `set` literal when testing for membership
+ pandas/io/parsers/python_parser.py:863:9: PLR6301 Method `_remove_empty_lines` could be a function, class method, or static method

Changes by rule (6 rules affected)

code	total	+ violation
PLR1702	4	4
PLR6301	3	3
PLR6201	2	2
PLC1901	2	2
PLR0917	1	1
PLR0914	1	1

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser

…sh#10158)  ## Summary This PR introduces the `ruff_server` crate and a new `ruff server` command. `ruff_server` is a re-implementation of [`ruff-lsp`](https://github.com/astral-sh/ruff-lsp), written entirely in Rust. It brings significant performance improvements, much tighter integration with Ruff, a foundation for supporting entirely new language server features, and more! This PR is an early version of `ruff_lsp` that we're calling the **pre-release** version. Anyone is more than welcome to use it and submit bug reports for any issues they encounter - we'll have some documentation on how to set it up with a few common editors, and we'll also provide a pre-release VSCode extension for those interested. This pre-release version supports: - **Diagnostics for `.py` files** - **Quick fixes** - **Full-file formatting** - **Range formatting** - **Multiple workspace folders** - **Automatic linter/formatter configuration** - taken from any `pyproject.toml` files in the workspace. Many thanks to @MichaReiser for his [proof-of-concept work](astral-sh#7262), which was important groundwork for making this PR possible. ## Architectural Decisions I've made an executive choice to go with `lsp-server` as a base framework for the LSP, in favor of `tower-lsp`. There were several reasons for this: 1. I would like to avoid `async` in our implementation. LSPs are mostly computationally bound rather than I/O bound, and `async` adds a lot of complexity to the API, while also making harder to reason about execution order. This leads into the second reason, which is... 2. Any handlers that mutate state should be blocking and run in the event loop, and the state should be lock-free. This is the approach that `rust-analyzer` uses (also with the `lsp-server`/`lsp-types` crates as a framework), and it gives us assurances about data mutation and execution order. `tower-lsp` doesn't support this, which has caused some [issues](ebkalderon/tower-lsp#284) around data races and out-of-order handler execution. 3. In general, I think it makes sense to have tight control over scheduling and the specifics of our implementation, in exchange for a slightly higher up-front cost of writing it ourselves. We'll be able to fine-tune it to our needs and support future LSP features without depending on an upstream maintainer. ## Test Plan The pre-release of `ruff_server` will have snapshot tests for common document editing scenarios. An expanded test suite is on the roadmap for future version of `ruff_server`.

snowsignal force-pushed the jane/lsp/server-mvp branch 5 times, most recently from 0cb76b9 to d1c23eb Compare March 1, 2024 05:16

snowsignal marked this pull request as ready for review March 1, 2024 05:17

snowsignal requested a review from MichaReiser as a code owner March 1, 2024 05:17

snowsignal force-pushed the jane/lsp/server-mvp branch 2 times, most recently from a44b169 to 86419e2 Compare March 1, 2024 05:23

snowsignal requested a review from charliermarsh March 1, 2024 05:23

MichaReiser reviewed Mar 1, 2024

View reviewed changes

MichaReiser added the preview Related to preview mode features label Mar 1, 2024

MichaReiser reviewed Mar 4, 2024

View reviewed changes

BurntSushi reviewed Mar 4, 2024

View reviewed changes

dhruvmanila reviewed Mar 5, 2024

View reviewed changes

snowsignal added 3 commits March 7, 2024 10:00

Create the ruff_server crate and the ruff server command.

56c1b3c

Address PR suggestions

282c02a

Fix additional suggestions

113da96

snowsignal force-pushed the jane/lsp/server-mvp branch from 47cfd54 to 113da96 Compare March 7, 2024 18:02

Fix typos in error logs

72f7688

MichaReiser requested changes Mar 8, 2024

View reviewed changes

MichaReiser approved these changes Mar 8, 2024

View reviewed changes

snowsignal added 4 commits March 8, 2024 20:20

Address suggestions

702a046

Code Action no longer fails if diagnostic data is None

66e257a

Remove unnessecary index rebuild

6877bc0

Add server command to configuration.md

972759d

snowsignal merged commit 0c84fbb into main Mar 9, 2024
17 checks passed

snowsignal deleted the jane/lsp/server-mvp branch March 9, 2024 04:57

jlhamilton777 mentioned this pull request Mar 9, 2024

Package request: python-lsp-ruff void-linux/void-packages#46560

Closed

snowsignal mentioned this pull request Mar 12, 2024

Re-introduce configuration reloading to ruff server #10366

Closed

Y-Nak mentioned this pull request Mar 30, 2024

Language server concurrency and functionality upgrades ethereum/fe#979

Closed

37 tasks

TungstnBallon mentioned this pull request Mar 31, 2024

add LSP category to ruff mason-org/mason-registry#5183

Merged

2 tasks

zanieb mentioned this pull request Apr 3, 2024

Static type checking à la mypy #3893

Open

DavisVaughan mentioned this pull request Apr 24, 2024

Ark: LSP crash while looking for completions or creating new document context posit-dev/positron#2692

Closed

lionel- mentioned this pull request May 3, 2024

Ark: Synchronise handling of LSP messages posit-dev/positron#2999

Closed

rassie mentioned this pull request May 8, 2024

Support ruff server emacs-lsp/lsp-mode#4451

Closed

lionel- mentioned this pull request May 17, 2024

Run LSP handlers consecutively by default posit-dev/ark#361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ruff server` - A new built-in LSP for Ruff, written in Rust #10158

`ruff server` - A new built-in LSP for Ruff, written in Rust #10158

snowsignal commented Feb 29, 2024 •

edited

Loading

MichaReiser commented Mar 1, 2024

MichaReiser left a comment

MichaReiser left a comment

MichaReiser Mar 4, 2024

snowsignal Mar 4, 2024

MichaReiser Mar 8, 2024

BurntSushi left a comment

dhruvmanila Mar 5, 2024

snowsignal Mar 7, 2024

MichaReiser left a comment •

edited

Loading

MichaReiser Mar 8, 2024

github-actions bot commented Mar 9, 2024

ruff server - A new built-in LSP for Ruff, written in Rust #10158

ruff server - A new built-in LSP for Ruff, written in Rust #10158

Conversation

snowsignal commented Feb 29, 2024 • edited Loading

Summary

Architectural Decisions

Test Plan

MichaReiser commented Mar 1, 2024

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser Mar 4, 2024

Choose a reason for hiding this comment

snowsignal Mar 4, 2024

Choose a reason for hiding this comment

MichaReiser Mar 8, 2024

Choose a reason for hiding this comment

BurntSushi left a comment

Choose a reason for hiding this comment

dhruvmanila Mar 5, 2024

Choose a reason for hiding this comment

snowsignal Mar 7, 2024

Choose a reason for hiding this comment

MichaReiser left a comment • edited Loading

Choose a reason for hiding this comment

MichaReiser Mar 8, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 9, 2024

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

`ruff server` - A new built-in LSP for Ruff, written in Rust #10158

`ruff server` - A new built-in LSP for Ruff, written in Rust #10158

snowsignal commented Feb 29, 2024 •

edited

Loading

MichaReiser left a comment •

edited

Loading

`ruff-ecosystem` results