Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_lsp): utf encoding #4308

Merged
merged 1 commit into from
Mar 19, 2023
Merged

Conversation

denbezrukov
Copy link
Contributor

@denbezrukov denbezrukov commented Mar 17, 2023

Summary

Fix #4222
Fix #4182 (comment)

The server and the client communicate with each other using messages which contain information about the Position.

interface Position {
   /**
    * Line position in a document (zero-based).
    */
   line: uinteger;

   /**
    * Character offset on a line in a document (zero-based). The meaning of this
    * offset is determined by the negotiated `PositionEncodingKind`.
    *
    * If the character value is greater than the line length it defaults back
    * to the line length.
    */
   character: uinteger;
}

The character offset can be encoded with PositionEncodingKind:

  • UTF8
  • UTF16
  • UTF32

Our server file representation is a rust string encoded in UTF8.
If we get an offset encoded in UTF16/UTF32, we need to convert it to UTF8 to handle the content of our file and convert it back before sending it to the client.

tower-lsp = { version = "0.19.0" } provides position_encodings - ebkalderon/tower-lsp#375

The main logic how to transform different UTF was copied from rust-analyzer

Test Plan

cargo test

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 93448b5
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64174065582aec0008f481ea

@github-actions github-actions bot added A-LSP Area: language server protocol A-Project Area: project configuration and loading labels Mar 17, 2023
.documents
.get(&params.path)
.ok_or(WorkspaceError::not_found())?;
Ok(document.content.clone())
Copy link
Contributor Author

@denbezrukov denbezrukov Mar 17, 2023

Choose a reason for hiding this comment

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

I don't know what we can do here to replace clone.
Because we clone a content of a file here.
We had the same issue here when we clone a document every time:

pub(crate) fn document(&self, url: &lsp_types::Url) -> Result<Document, WorkspaceError> {
self.documents
.read()
.unwrap()
.get(url)
.cloned()
.ok_or_else(WorkspaceError::not_found)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

That's OK to clone the content. It's not an issue; it's the architecture constraint, which is what we want. Workspace requests need to travel - among other things - across a server, which means we need owned data.

@@ -281,10 +281,7 @@ pub struct ReportNotSerializable {
pub struct NotFound;

#[derive(Debug, Serialize, Deserialize, Diagnostic)]
#[diagnostic(
Copy link
Contributor Author

@denbezrukov denbezrukov Mar 18, 2023

Choose a reason for hiding this comment

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

There was a typo in the message.
#4182 (comment) could have an incorrect error message.

@denbezrukov denbezrukov marked this pull request as ready for review March 18, 2023 15:02
crates/rome_lsp/src/converters/from_proto.rs Outdated Show resolved Hide resolved
crates/rome_lsp/src/converters/line_index.rs Show resolved Hide resolved

tracing::trace!("new document: {:?}", doc.text());
let old_text = session.workspace.get_file_content(GetFileContentParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you tell me why we need to get the text from the workspace now? We didn't do it before. I'm genuinely interested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had a content field in a LineIndex struct.

#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) struct LineIndex {
text: String,
/// Offset the the beginning of each line, zero-based
newlines: Vec<TextSize>,
}

It looks like it was ineffective. Because we had two copies of a file content - in the workspace and in the line index.
Moreover, the index is part of the document, which is cloned in each handler. So it could be a performance improvement too

.documents
.get(&params.path)
.ok_or(WorkspaceError::not_found())?;
Ok(document.content.clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

That's OK to clone the content. It's not an issue; it's the architecture constraint, which is what we want. Workspace requests need to travel - among other things - across a server, which means we need owned data.

crates/rome_wasm/src/lib.rs Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-LSP Area: language server protocol A-Project Area: project configuration and loading
Projects
None yet
3 participants