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

fix(prisma-fmt): use UTF-16 offset in the response for the schema that contains multi-byte characters #4815

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

key-moon
Copy link

@key-moon key-moon commented Apr 6, 2024

The current Prisma's LSP cannot handle schemas containing multibyte characters correctly.

image
image

This is because the offsets returned by prisma-fmt are calculated based on the number of bytes in UTF-8 encoding. In the LSP protocol, text offsets should be represented by the length in UTF-16 unless otherwise specified.

This pull request includes changes to the offset_to_position and position_to_offset functions, as well as the implementation of the offset_to_lsp_offset function and its usage within lint::run.

This fixes the above issue.
image

@key-moon key-moon requested a review from a team as a code owner April 6, 2024 12:07
@key-moon key-moon requested review from jkomyno and removed request for a team April 6, 2024 12:07
@CLAassistant
Copy link

CLAassistant commented Apr 6, 2024

CLA assistant check
All committers have signed the CLA.

@key-moon key-moon changed the title fix(prisma-fmt): use UTF-16 offset in the response for the schema that contains multi-byte charactors fix(prisma-fmt): use UTF-16 offset in the response for the schema that contains multi-byte characters Apr 6, 2024
Copy link

codspeed-hq bot commented Apr 8, 2024

CodSpeed Performance Report

Merging #4815 will not alter performance

Comparing key-moon:fix-multibyte (4b96152) with main (6144ce0)

Summary

✅ 11 untouched benchmarks

@key-moon
Copy link
Author

key-moon commented Apr 8, 2024

There are warnings about dead_code for many functions in offset.rs. This is due to the fact that most functions in offset.rs are only used in lib.rs. I have allowed dead_code in the whole file for a now, but if you have a better solution, I would appreciate suggestions.

@aqrln
Copy link
Member

aqrln commented Apr 8, 2024

@key-moon this happens because the module is included twice in both the library crate and the binary crate. I see that some existing modules in this package follow this pattern too but it's not really the best way to do it. Aside from leading to problems like the one you encountered, it will also lead to compiling the same code twice and potentially bloating the binary (for us maybe there's no binary size impact because -Os collapses identical functions, but it will affect the compilation speed regardless).

The way it's normally done in Rust is you only include such modules in the library crate (i.e. in lib.rs) and export what you need to use in the binary crate. In other words, if you turn mod offsets; in lib.rs into pub mod offsets; and remove mod offsets; in main.rs, you can use the prisma_fmt::offsets module anywhere in the binary crate (note that we refer to prisma_fmt as an external crate here even though both crates are in the same package with the same cargo manifest and name) and you won't have any dead or duplicate code.

@key-moon
Copy link
Author

key-moon commented Apr 8, 2024

Understood. I am new to Rust, so your help with the basics was very helpful.

While applying the suggested fix, I ran into a problem caused by the fact that lint uses offsets. Since lint is included by main and lib, two different ways of referencing the function, crate::offsets::offset_to_lsp_offset; and prisma_fmt::offsets::offset_to_lsp_offset; conflict.

This could be solved by not including lint in main and publishing the lint module as well. However, as this would be a relatively large change, I'm hesitate to make such a change.

Edit: I just pushed the fix that was described above. I'm not sure whether this fix was the right way to do it.

@key-moon
Copy link
Author

key-moon commented Apr 10, 2024

The error caused by the conflict has been resolved. It took some time to resolve, but now this PR is back to a problem-free state.
One thing I discovered in the process of fixing the error. The MiniError in lint::run does not seem to return information about which file the warning was emitted from. I believe this is an unintended behavior.

@key-moon
Copy link
Author

Is there anything you are having trouble with for the merge? If there is, I will work towards resolving it.

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.

None yet

3 participants