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

Document requirements of FullTextDocument::update() #24

Open
bolinfest opened this issue Aug 3, 2023 · 4 comments
Open

Document requirements of FullTextDocument::update() #24

bolinfest opened this issue Aug 3, 2023 · 4 comments

Comments

@bolinfest
Copy link
Contributor

I think there is a subtle edge case that should be documented for FullTextDocument::update() (which currently has no docstring).

Specifically, I believe that each TextDocumentContentChangeEvent passed to update() assumes that its associated ranges are specified using utf-16 as the position encoding (which, as of VS Code 1.81.0, appears to be only position encoding that it supports, though as of version 3.17 of the LSP spec, UTF-8 and UTF-32 are also options an LSP client can offer):

https://docs.rs/lsp-types/latest/lsp_types/struct.PositionEncodingKind.html#associatedconstant.UTF16

Because the position encoding is specified up front when the LSP connection is negotiated so that such metadata doesn't have to be sent with every LSP message that references a Range or Position, the position encoding cannot be determined for an isolated TextDocumentContentChangeEvent event.

To that end, I think the docs for update() should include a disclaimer that it assumes the utf-16 position encoding.

Ultimately, it might be nice to support other position encodings, though until VS Code bothers to do so, this does not seem pressing.

@bolinfest
Copy link
Contributor Author

Stepping back, it seems that this requirement applies to any public API of FullTextDocument that takes a Range or Position, as well?

@bolinfest
Copy link
Contributor Author

Also, while on the topic of fixing up docstrings, the one for position_at() seems to start with an incomplete sentence:

/// Converts a zero-based byte offset in the UTF8-encoded content to a position
///
/// the offset is in bytes, the position is in UTF16 code units. rounds down if
/// the offset is not on a code unit boundary, or is beyond the end of the
/// content.

@bolinfest
Copy link
Contributor Author

Based on microsoft/vscode-languageserver-node#1224, it doesn't sound like the UTF-8 option is coming to VS Code anytime soon...

@GiveMe-A-Name
Copy link
Owner

I agree with you. The root docstring should remark the utf-16 position encoding only support by default.

Actually, in design that user don't need to care about the update of FullTextDocument . For user, the FullTextDocument is a readonly status.The language client is responsible for updating it.

I think we need support the options utf-8 and utf-32 in addition to write the doc. It is a base lib that should need supports many Language Clients(IDE) not only vscode.

I have long time that not care about lsp, lsp-textdocument. In the future, I would like to spend my time to do 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

No branches or pull requests

2 participants