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

Clean up JSON-RPC types and deserialization code #379

Merged
merged 11 commits into from
Mar 5, 2023

Conversation

ebkalderon
Copy link
Owner

@ebkalderon ebkalderon commented Mar 5, 2023

Added

  • Implement std::str::FromStr for Request and Response.
  • Implement From<jsonrpc::ErrorCode> for i64.

Changed

  • Avoid heap allocation in Version deserialization.
  • Make Version a private struct instead of pub(crate).
  • Un-derive Copy for Version because it's unnecessary.
  • Group jsonrpc::Result<T>and not_initialized_error() with all the error stuff, re-export it to parent module for public access
  • Replace custom Serialize/Deserialize implementations for ErrorCode with #[serde(into/from)].
  • Move Request/RequestBuilder to src/jsonrpc/request.rs submodule.
  • Move Response to src/jsonrpc/response.rs submodule.
  • Better document panic behavior for lsp-types conversions as Request::from_{notification,request} doc comments.

This pull request contains a grab-bag of miscellaneous improvements to the jsonrpc module that should improve deserialization performance slightly, but mostly make the code easier to read, grok, and maintain.

It was also supposed to include some major improvements to deserializing request params and ids, and handling invalid JSON-RPC messages in the Message type (see commits in the full improve-jsonrpc-deserialization branch), but I ran into an inconsistency in the official LSP spec and chose to not pursue this for now. I've asked for clarification in microsoft/language-server-protocol#1686, filed earlier today.

The blanket implementation of `Deserialize<'de>` for `Cow<'de, str>`
simply creates a `Cow::Owned`, which allocates on the heap. To avoid
this, we must use a simple newtype with the `#[serde(borrow)]` attribute
instead.
This lets us drop the manual `Serialize` and `Deserialize`
implementations for this type in favor of the more concise
`serde-derive` macros.
This change should improve the developer experience and should make unit
testing easier, going forward.
@ebkalderon ebkalderon self-assigned this Mar 5, 2023
Besides being more flexible than `Into`, this also addresses the Clippy
lint triggered in CI.
@ebkalderon ebkalderon force-pushed the min-improve-jsonrpc-deserialization branch from d088269 to ced93b7 Compare March 5, 2023 05:59
@ebkalderon ebkalderon merged commit 3234520 into master Mar 5, 2023
@ebkalderon ebkalderon deleted the min-improve-jsonrpc-deserialization branch March 5, 2023 06:08
@ebkalderon
Copy link
Owner Author

Regarding microsoft/language-server-protocol#1686: It turns out that the params field of the telemetry/event notification not being of type object | array is a bug in the LSP spec. I will open a PR against lsp-types fixing this by changing the Params associated type for this notification to something like OneOf<serde_json::Map<String, Value>, Vec<Value>>.

This also means it should be safe to add the strict Params type from improve-jsonrpc-deserialization (4a0e471) after all! 🎉

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.

1 participant