feature: allow utf8 encoding #919

rgrinberg · 2022-11-16T03:46:38Z

Signed-off-by: Rudi Grinberg me@rgrinberg.com

ps-id: ac0de96a-2c5f-4c90-b1af-40acdc1a8344

ulugbekna · 2022-11-16T12:32:26Z

Will review by Friday

ulugbekna

Looks good to me except for nits

CHANGES.md

ulugbekna · 2022-11-17T11:31:36Z

lsp/src/text_document.ml

    (change : TextDocumentContentChangeEvent.t) =
  (* Changes can only be applied using utf16 offsets *)
  let version =
    match version with
-    | None -> t.version + 1
+    | None -> t.document.version + 1


Slightly offtopic but since we're on this: the version bump happens for each change event within a single textDocument/didChange notification (vs. once per notification). Is this what we want? I don't recall the version being useful to either client or server, but still

good point. i got rid of this weirdness

ulugbekna · 2022-11-17T11:38:50Z

lsp/src/text_document.ml

      String.concat
        ~sep:""
-        [ String.sub t.text ~pos:0 ~len:start_offset
+        [ String.sub t.document.text ~pos:0 ~len:start_offset
        ; change.text
        ; String.sub
-            t.text
+            t.document.text
            ~pos:end_offset
-            ~len:(String.length t.text - end_offset)
+            ~len:(String.length t.document.text - end_offset)
        ]


also offtopic a bit: Given how many times and how often a document is edited, should we could use a data structure more specialized for updates (tree/rope?) or at least do the string concat manually and avoid two copies that happen because of String.sub calls?

Batching updates sounds useful but a rope wouldn't help much since we'd need to convert to a string for merlin anyway.

ulugbekna · 2022-11-17T11:55:34Z

lsp/src/text_document.mli

@@ -2,7 +2,7 @@ open Types

 type t

-val make : DidOpenTextDocumentParams.t -> t
+val make : encoding:[ `UTF8 | `UTF16 ] -> DidOpenTextDocumentParams.t -> t


Would it make sense to specify that it's "position encoding" rather than "encoding"?

Since position encoding is specified for all documents (ie docs don't vary by their pos encoding), should we perhaps not keep encoding for each document? Would it be better if the lsp library client passes the pos encoding to apply_content_change? I just think it's a bit of misrepresentation of the domain that every document can have different pos encoding, but I agree there isn't obvious better solution.

I suppose so. It really only applies to updates anyway

This would be better if the encoding could be changed between updates but this isn't the case. So neither reflect the domain very well.

What would be a better long term fix is to general Document_store and promote it into the lsp library.

ulugbekna · 2022-11-17T13:17:09Z

lsp/src/text_document.ml

-let find_offset ~utf8 ~utf16_range:range =
+exception Outside
+
+let rec find_nth_nl str nth pos len =


should this be defined within find_nth_nl definition below as an auxiliary fn it is?

ulugbekna · 2022-11-17T14:03:16Z

Also, do I understand correctly that the text document encoding is still only utf-8 and is passed in the rpc message header, while this new encoding negotiation is only for the encoding that's used for denoting positions in the text document?

rgrinberg · 2022-11-17T16:05:23Z

Also, do I understand correctly that the text document encoding is still only utf-8 and is passed in the rpc message header, while this new encoding negotiation is only for the encoding that's used for denoting positions in the text document?

Yeah. The summary is:

the encoding for protocol packets is always utf8
we always store everything internally as utf8
only character offsets are interpreted as utf8 or utf16 depending on the client.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com> ps-id: ac0de96a-2c5f-4c90-b1af-40acdc1a8344

CHANGES: ## Fixes - Fix document syncing for ranges that span an entire line (ocaml/ocaml-lsp#927) - Respect the client's completion item resolve and preSelect capabilities (ocaml/ocaml-lsp#925, ocaml/ocaml-lsp#936) - Disable polling for dune's watch mode on Windows and OCaml 4.14.0 (ocaml/ocaml-lsp#935) ## Features - Semantic highlighting support is enabled by default (ocaml/ocaml-lsp#933) - Re-enable `ocamlformat-rpc` for formatting code snippets (but not files) (ocaml/ocaml-lsp#920, ocaml/ocaml-lsp#939) One needs to have either `ocamlformat` version > 0.21.0 or, otherwise, `ocamlformat-rpc` package installed. - Add custom ocamllsp/hoverExtended request (ocaml/ocaml-lsp#561) - Support utf-8 position encoding clients (ocaml/ocaml-lsp#919) - Upgrade to merlin 4.7 and use merlin's `verbosity=smart` by default, which allows unwrapping module alias types (ocaml/ocaml-lsp#942) ## Fixes - Fix semantic highlighting of "long identifiers," e.g., `Foo.Bar.x` (ocaml/ocaml-lsp#932)

CHANGES: ## Features - Add support for OCaml 5.0 - Semantic highlighting support is enabled by default (ocaml/ocaml-lsp#933) - Re-enable `ocamlformat-rpc` for formatting code snippets (but not files) (ocaml/ocaml-lsp#920, ocaml/ocaml-lsp#939) One needs to have either `ocamlformat` version > 0.21.0 or, otherwise, `ocamlformat-rpc` package installed. - Add custom ocamllsp/hoverExtended request (ocaml/ocaml-lsp#561) - Support utf-8 position encoding clients (ocaml/ocaml-lsp#919) - Upgrade to merlin 4.7 and use merlin's `verbosity=smart` by default, which allows unwrapping module alias types (ocaml/ocaml-lsp#942) ## Fixes - Fix document syncing for ranges that span an entire line (ocaml/ocaml-lsp#927) - Respect the client's completion item resolve and preSelect capabilities (ocaml/ocaml-lsp#925, ocaml/ocaml-lsp#936) - Disable polling for dune's watch mode on Windows and OCaml 4.14.0 (ocaml/ocaml-lsp#935) - Fix semantic highlighting of "long identifiers," e.g., `Foo.Bar.x` (ocaml/ocaml-lsp#932)

CHANGES: ## Features - Enable [semantic highlighting](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_semanticTokens) support by default (ocaml/ocaml-lsp#933) - Support connecting over pipes and socket. Pipes on Windows aren't yet supported (ocaml/ocaml-lsp#946) [More](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#implementationConsiderations) about communication channels in LSP specification. - Re-enable `ocamlformat-rpc` for formatting code snippets (but not files and not on Windows) (ocaml/ocaml-lsp#920, ocaml/ocaml-lsp#939) One needs to have installed either `ocamlformat` package version > 0.21.0 or, otherwise, `ocamlformat-rpc` package. Note that previously `ocamlformat-rpc` came in a standalone OPAM package, but since `ocamlformat` version > 0.21.0, it comes within `ocamlformat` package. - Add custom [`ocamllsp/hoverExtended`](https://github.com/ocaml/ocaml-lsp/blob/e165f6a3962c356adc7364b9ca71788e93489dd0/ocaml-lsp-server/docs/ocamllsp/hoverExtended-spec.md#L1) request (ocaml/ocaml-lsp#561) - Support utf-8 position encoding clients (ocaml/ocaml-lsp#919) [More](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#position) about position encoding in LSP specification. - Show unwrapped module alias types on hovering over module names. This is due to upgrading to merlin 4.7 and using merlin's `verbosity=smart` by default (ocaml/ocaml-lsp#942) ## Fixes - Respect the client's completion item resolve and preSelect capabilities (ocaml/ocaml-lsp#925, ocaml/ocaml-lsp#936) - Disable polling for dune's watch mode on Windows and OCaml 4.14.0 (ocaml/ocaml-lsp#935) - Fix semantic highlighting of "long identifiers," e.g., `Foo.Bar.x` (ocaml/ocaml-lsp#932) - Fix syncing of document contents: - For ranges that span an entire line (ocaml/ocaml-lsp#927) - Previously, whole line edits would incorrectly eat the newline characters (ocaml/ocaml-lsp#971)

CHANGES: ## Features - Add support for OCaml 5.0 - Enable [semantic highlighting](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_semanticTokens) support by default (ocaml/ocaml-lsp#933) - Support connecting over pipes and socket. Pipes on Windows aren't yet supported (ocaml/ocaml-lsp#946) [More](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#implementationConsiderations) about communication channels in LSP specification. - Re-enable `ocamlformat-rpc` for formatting code snippets (but not files and not on Windows) (ocaml/ocaml-lsp#920, ocaml/ocaml-lsp#939) One needs to have installed either `ocamlformat` package version > 0.21.0 or, otherwise, `ocamlformat-rpc` package. Note that previously `ocamlformat-rpc` came in a standalone OPAM package, but since `ocamlformat` version > 0.21.0, it comes within `ocamlformat` package. - Add custom [`ocamllsp/hoverExtended`](https://github.com/ocaml/ocaml-lsp/blob/e165f6a3962c356adc7364b9ca71788e93489dd0/ocaml-lsp-server/docs/ocamllsp/hoverExtended-spec.md#L1) request (ocaml/ocaml-lsp#561) - Support utf-8 position encoding clients (ocaml/ocaml-lsp#919) [More](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#position) about position encoding in LSP specification. - Show unwrapped module alias types on hovering over module names. This is due to upgrading to merlin 4.7 and using merlin's `verbosity=smart` by default (ocaml/ocaml-lsp#942) ## Fixes - Respect the client's completion item resolve and preSelect capabilities (ocaml/ocaml-lsp#925, ocaml/ocaml-lsp#936) - Disable polling for dune's watch mode on Windows and OCaml 4.14.0 (ocaml/ocaml-lsp#935) - Fix semantic highlighting of "long identifiers," e.g., `Foo.Bar.x` (ocaml/ocaml-lsp#932) - Fix syncing of document contents: - For ranges that span an entire line (ocaml/ocaml-lsp#927) - Previously, whole line edits would incorrectly eat the newline characters (ocaml/ocaml-lsp#971)

rgrinberg requested a review from ulugbekna November 16, 2022 03:46

rgrinberg added this to the 1.15.0 milestone Nov 16, 2022

rgrinberg force-pushed the ps/rr/feature__allow_utf8_encoding branch 2 times, most recently from f48f53d to bfd0f6e Compare November 16, 2022 03:48

ulugbekna reviewed Nov 17, 2022

View reviewed changes

rgrinberg force-pushed the ps/rr/feature__allow_utf8_encoding branch from bfd0f6e to 4000f49 Compare November 17, 2022 21:28

feature: allow utf8 encoding

4ffee3c

Signed-off-by: Rudi Grinberg <me@rgrinberg.com> ps-id: ac0de96a-2c5f-4c90-b1af-40acdc1a8344

rgrinberg force-pushed the ps/rr/feature__allow_utf8_encoding branch from 4000f49 to 4ffee3c Compare November 17, 2022 21:45

rgrinberg merged commit d1bd355 into master Nov 17, 2022

ulugbekna mentioned this pull request Dec 16, 2022

[new release] ocaml-lsp-server, lsp and jsonrpc (1.14.3) ocaml/opam-repository#22691

Closed

ulugbekna mentioned this pull request Dec 16, 2022

[new release] ocaml-lsp-server, lsp and jsonrpc (1.15.0~5.0preview1) ocaml/opam-repository#22701

Merged

ulugbekna mentioned this pull request Jan 7, 2023

[new release] ocaml-lsp-server, lsp and jsonrpc (1.15.0-4.14) ocaml/opam-repository#22878

Merged

ulugbekna mentioned this pull request Jan 7, 2023

[new release] ocaml-lsp-server, lsp and jsonrpc (1.15.0-5.0) ocaml/opam-repository#22879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: allow utf8 encoding #919

feature: allow utf8 encoding #919

rgrinberg commented Nov 16, 2022

ulugbekna commented Nov 16, 2022

ulugbekna left a comment

ulugbekna Nov 17, 2022

rgrinberg Nov 17, 2022

ulugbekna Nov 17, 2022

rgrinberg Nov 17, 2022

ulugbekna Nov 17, 2022

rgrinberg Nov 17, 2022

ulugbekna Nov 17, 2022

ulugbekna commented Nov 17, 2022

rgrinberg commented Nov 17, 2022

feature: allow utf8 encoding #919

feature: allow utf8 encoding #919

Conversation

rgrinberg commented Nov 16, 2022

ulugbekna commented Nov 16, 2022

ulugbekna left a comment

Choose a reason for hiding this comment

ulugbekna Nov 17, 2022

Choose a reason for hiding this comment

rgrinberg Nov 17, 2022

Choose a reason for hiding this comment

ulugbekna Nov 17, 2022

Choose a reason for hiding this comment

rgrinberg Nov 17, 2022

Choose a reason for hiding this comment

ulugbekna Nov 17, 2022

Choose a reason for hiding this comment

rgrinberg Nov 17, 2022

Choose a reason for hiding this comment

ulugbekna Nov 17, 2022

Choose a reason for hiding this comment

ulugbekna commented Nov 17, 2022

rgrinberg commented Nov 17, 2022