Skip to content

switch out lsp-types for gen-lsp-types#22115

Open
BenjaminBrienen wants to merge 1 commit intorust-lang:masterfrom
BenjaminBrienen:gen-lsp-types
Open

switch out lsp-types for gen-lsp-types#22115
BenjaminBrienen wants to merge 1 commit intorust-lang:masterfrom
BenjaminBrienen:gen-lsp-types

Conversation

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2026
Comment thread crates/rust-analyzer/src/bin/main.rs Outdated
Comment thread crates/rust-analyzer/src/handlers/request.rs Outdated
@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@BenjaminBrienen BenjaminBrienen force-pushed the gen-lsp-types branch 2 times, most recently from 552f6d5 to b10f69c Compare April 23, 2026 13:35
@rustbot

This comment has been minimized.

@ribru17
Copy link
Copy Markdown

ribru17 commented Apr 23, 2026

Heads up @BenjaminBrienen I just pushed v0.4.0, will probably affect this PR

@BenjaminBrienen
Copy link
Copy Markdown
Contributor Author

I already updated to it :)

@BenjaminBrienen BenjaminBrienen force-pushed the gen-lsp-types branch 2 times, most recently from fd865d7 to 90f1985 Compare April 24, 2026 00:47
@BenjaminBrienen
Copy link
Copy Markdown
Contributor Author

I'm assuming you would like a side of squash with this meal

@ribru17
Copy link
Copy Markdown

ribru17 commented Apr 24, 2026

Comment on lines -2660 to -2661
// FIXME: Also detect the proposed lsp version at caps.workspace.workspaceEdit.snippetEditSupport
// once lsp-types has it.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already have a draft PR based on top of this one that implements the split and uses the capability properly, so I just removed the comment for now. Is that fine?

Copy link
Copy Markdown
Contributor

@DropDemBits DropDemBits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going through all of the it.method == R::METHOD changes made me realize that there's no request-equivalent helper function of notification_is 😅

My main concern though is if the file:// scheme is still required though, as I think the tests ensure that we're emitting self-consistent values rather than if those values actually make sense for the LS client.

View changes since this review

Comment thread crates/rust-analyzer/src/lsp/utils.rs Outdated
Comment thread crates/rust-analyzer/src/lsp/utils.rs Outdated
Comment thread crates/rust-analyzer/src/lsp/semantic_tokens.rs Outdated
Comment thread crates/rust-analyzer/src/lsp/ext.rs
Comment thread crates/rust-analyzer/src/diagnostics/test_data/rustc_range_map_lsp_position.txt Outdated
Comment thread crates/rust-analyzer/src/handlers/dispatch.rs Outdated
Comment thread crates/rust-analyzer/src/handlers/dispatch.rs Outdated
Comment thread crates/rust-analyzer/src/handlers/dispatch.rs Outdated
Comment thread crates/rust-analyzer/src/handlers/dispatch.rs Outdated
Comment on lines -2660 to -2661
// FIXME: Also detect the proposed lsp version at caps.workspace.workspaceEdit.snippetEditSupport
// once lsp-types has it.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't known about the draft PR, sorry!

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 24, 2026

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@BenjaminBrienen
Copy link
Copy Markdown
Contributor Author

BenjaminBrienen commented Apr 24, 2026

It is failing because of something to do with the difference between resource operation and document change, I think. It is just one slow test. I couldn't figure out the reason.

@BenjaminBrienen BenjaminBrienen force-pushed the gen-lsp-types branch 2 times, most recently from 4e097a1 to c7b031d Compare April 24, 2026 20:56
Comment thread crates/rust-analyzer/src/lsp/capabilities.rs
Comment thread crates/rust-analyzer/src/lsp/capabilities.rs Outdated
{
let req = self.req.take_if(|it| it.method == R::METHOD)?;
let res = crate::from_json(R::METHOD, &req.params);
let req = self.req.take_if(|it| it.method.as_str() == R::METHOD.as_str())?;
Copy link
Copy Markdown
Member

@Veykril Veykril Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let req = self.req.take_if(|it| it.method.as_str() == R::METHOD.as_str())?;
let req = self.req.take_if(|it| it.method == R::METHOD)?;

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't compare std::string::String with gen_lsp_types::LspRequestMethod

Comment thread crates/rust-analyzer/src/lsp/capabilities.rs
Comment thread crates/rust-analyzer/src/lsp/from_proto.rs Outdated
Comment thread crates/rust-analyzer/src/lsp/utils.rs
Comment thread crates/rust-analyzer/src/lsp/utils.rs
Comment thread Cargo.lock
Comment thread crates/rust-analyzer/src/lsp/from_proto.rs
Comment thread crates/rust-analyzer/src/lsp/ext.rs Outdated
Comment thread crates/rust-analyzer/src/lsp/semantic_tokens.rs Outdated
Comment thread crates/rust-analyzer/src/main_loop.rs
@BenjaminBrienen BenjaminBrienen force-pushed the gen-lsp-types branch 2 times, most recently from 53e9186 to bcaaea6 Compare April 25, 2026 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants