lsp: Skip serializing of params if unit type by Spirrwell · Pull Request #46027 · zed-industries/zed

Spirrwell · 2026-01-04T15:21:57Z

Per the JSON-RPC specification, params "MUST be provided as a Structured value" or the member "MAY be omitted." See specification here: https://www.jsonrpc.org/specification#request_object

The code was passing in unit () for params which serde_json was serializing to null which is not valid.

This pulls request adds an is_unit function and annotates the params fields in the Request and Notification structs with the following:

#[serde(default, skip_serializing_if = "is_unit")]

so that the field will be omitted if the type is unit. This does also introduce a 'static bound to these structs.

Warning

While this seems to me like a simple change, I am not super familiar with Rust, please look at this PR with extra scrutiny. The last thing I want to do is break everyone's LSP integration.

Release Notes:

LSP integrations: send language server shutdown requests with correct parameters

cla-bot · 2026-01-04T15:22:00Z

We require contributors to sign our Contributor License Agreement, and we don't have @Spirrwell on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

Spirrwell · 2026-01-04T15:23:14Z

@cla-bot check

cla-bot · 2026-01-04T15:23:17Z

We require contributors to sign our Contributor License Agreement, and we don't have @Spirrwell on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

cla-bot · 2026-01-04T15:23:18Z

The cla-bot has been summoned, and re-checked this pull request!

Spirrwell · 2026-01-04T15:24:51Z

@cla-bot check

cla-bot · 2026-01-04T15:24:55Z

The cla-bot has been summoned, and re-checked this pull request!

SomeoneToIgnore · 2026-01-04T17:06:28Z

Thank you for the PR, I will come back to this next week most probably, one note after skimming: we do send both Shutdown and Exit messages with fn XXX_internal right?

zed/crates/lsp/src/lsp.rs

Line 950 in 5505e63

let shutdown_request = Self::request_internal::<request::Shutdown>(

zed/crates/lsp/src/lsp.rs

Line 988 in 5505e63

    
           Self::notify_internal::<notification::Exit>(&notification_serializers, ()).ok();

I'd expect we change this particular layer to accept Option of parameters, and all public methods like notify and request can be unchanged?

If I search for params: none in the current spec, there are only 2 client -> server requests (shutdown and exit), so we do not need any other case in the forseeable future, hence changing the public API seems wasteful.

Spirrwell · 2026-01-04T17:12:56Z

I can look closer at potentially leaving the public API unchanged. It also looks like I missed the testing framework completely, so there is certainly more work to be done than I thought. I will put this in Draft until I iron those things out. I will probably have something more complete by next weekend.

Spirrwell · 2026-01-05T02:35:25Z

Thank you for the PR, I will come back to this next week most probably, one note after skimming: we do send both Shutdown and Exit messages with fn XXX_internal right?

zed/crates/lsp/src/lsp.rs

Line 950 in 5505e63

let shutdown_request = Self::request_internal::<request::Shutdown>(

zed/crates/lsp/src/lsp.rs

Line 988 in 5505e63

Self::notify_internal::<notification::Exit>(&notification_serializers, ()).ok();

I'd expect we change this particular layer to accept Option of parameters, and all public methods like notify and request can be unchanged?

If I search for params: none in the current spec, there are only 2 client -> server requests (shutdown and exit), so we do not need any other case in the forseeable future, hence changing the public API seems wasteful.

So, there are two things here I'm not sure how to tackle if we wanted to not change the public API.

It seems like there is a fake LSP implementation that tests the public API. This can be seen here for example:

zed/crates/collab/src/tests/editor_tests.rs

Line 2111 in 88a0b31

.request::<lsp::request::InlayHintRefreshRequest>(())

Unless we're explicitly testing receiving a null params, would it not make sense for the public API to provide a correct way to omit params? (I had opted to pass None in my current changes)
There is also this ClearCache request for prettier, which I'm not familiar with, but also exists here:

zed/crates/prettier/src/prettier.rs

Line 532 in 88a0b31

.request::<ClearCache>(())

Rust has the ability to have optionals of reference type, yes? To minimize additional overhead from going from request -> request_internal -> request_internal_with_timer with copies of params at each, it might be conceivable to turn these into optional references. I haven't tried that yet, but if the concern is that modifying the public API in the current way is wasteful, perhaps that is an alternative?

I have updated my fork to make sure the tests build (I hope I got them all, the project diagnostics in Zed made that pretty easy if so!)

However, I have noticed some run to run variance in tests passing. But, that seems to hold true even if I run them on the main branch of Zed, not in my fork.

I'll wait for some more thoughts before I remove the Draft status.

SomeoneToIgnore · 2026-01-05T07:48:49Z

Both cases are our Prettier "protocol" and our tests, so I still do not understand why should we care and change half of the world for that.

Spirrwell · 2026-01-11T23:29:59Z

Well, I have a different solution that doesn't change half the world. I have updated the OP to reflect that.

I suppose there are some restrictions by adding the 'static bound. But, this does localize the changes to exactly one file, it does what I want it to do, and it keeps behavior consistent across the public and private API with minimal changes.

There does exist code with what appears to be a similar purpose here:

zed/crates/context_server/src/client.rs

Line 71 in d4f6ca4

fn is_null_value<T: Serialize>(value: &T) -> bool {

That I think would also work, but I believe that would incur the cost of serializing twice. Which since, there are only two things with no params, it seems silly to do.

I'm gonna mark this as ready, and hope this is okay!

SomeoneToIgnore

Finally got the time to focus on reviews, sorry for dragging.

This is definitely a great way to solve things, better than the one I've been talking about.
'static in this context means that we do not borrow T (it has no ref, &'a, types) and seems totally fine: we do use various lsp::InlayHintFooRequest-like objects across the codebase.

Given it compiles and it's not a change in gpui public library code, it's totally fine and we can adjust it later any other way we need.

Than you!

zed-industries-bot · 2026-01-22T13:06:27Z

	Warnings
⚠️	This PR is missing release notes. Please add a "Release Notes" section that describes the change: `Release Notes: - Added/Fixed/Improved ...` If your change is not user-facing, you can use "N/A" for the entry: `Release Notes: - N/A`

Generated by 🚫 dangerJS against e2c5a50

Closes zed-industries#45994 Per the JSON-RPC specification, `params` "MUST be provided as a Structured value" or the member "MAY be omitted." See specification here: https://www.jsonrpc.org/specification#request_object The code was passing in unit `()` for params which serde_json was serializing to `null` which is not valid. This pulls request adds an `is_unit` function and annotates the `params` fields in the `Request` and `Notification` structs with the following: ```rs #[serde(default, skip_serializing_if = "is_unit")] ``` so that the field will be omitted if the type is unit. This does also introduce a `'static` bound to these structs. > [!WARNING] > While this seems to me like a simple change, I am not super familiar with Rust, please look at this PR with extra scrutiny. The last thing I want to do is break everyone's LSP integration. Release Notes: - LSP integrations: send language server shutdown requests with correct parameters

cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 4, 2026

SomeoneToIgnore self-assigned this Jan 4, 2026

Spirrwell marked this pull request as draft January 4, 2026 17:13

Spirrwell force-pushed the main branch 2 times, most recently from e29413b to 9b0470a Compare January 5, 2026 02:02

lsp: Skip serializing of params if unit type

e2c5a50

Spirrwell force-pushed the main branch from 9b0470a to e2c5a50 Compare January 11, 2026 23:01

Spirrwell changed the title ~~lsp: Make request and notification params field optional~~ lsp: Skip serializing of params if unit type Jan 11, 2026

Spirrwell marked this pull request as ready for review January 11, 2026 23:30

SomeoneToIgnore approved these changes Jan 22, 2026

View reviewed changes

SomeoneToIgnore merged commit da53a2b into zed-industries:main Jan 22, 2026
23 of 26 checks passed

Uh oh!

Conversation

Spirrwell commented Jan 4, 2026 • edited by SomeoneToIgnore Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-bot bot commented Jan 4, 2026

Uh oh!

Spirrwell commented Jan 4, 2026

Uh oh!

cla-bot bot commented Jan 4, 2026

Uh oh!

cla-bot bot commented Jan 4, 2026

Uh oh!

Spirrwell commented Jan 4, 2026

Uh oh!

cla-bot bot commented Jan 4, 2026

Uh oh!

SomeoneToIgnore commented Jan 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Spirrwell commented Jan 4, 2026

Uh oh!

Spirrwell commented Jan 5, 2026

Uh oh!

SomeoneToIgnore commented Jan 5, 2026

Uh oh!

Spirrwell commented Jan 11, 2026

Uh oh!

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zed-industries-bot commented Jan 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Spirrwell commented Jan 4, 2026 •

edited by SomeoneToIgnore

Loading

SomeoneToIgnore commented Jan 4, 2026 •

edited

Loading