From c8c10966133cbbe88e6f23e5fd7bad3e125c198f Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <me@rgrinberg.com>
Date: Thu, 19 Jan 2023 12:45:48 -0600
Subject: [PATCH] refactor(lsp): batch updates with a zipper (#1004)

---
 CHANGES.md                      |   8 +-
 lsp/src/import.ml               |   2 +
 lsp/src/lsp.ml                  |   2 +
 lsp/src/position.ml             |   5 +
 lsp/src/string_zipper.ml        | 257 ++++++++++++++++++++++++++++++++
 lsp/src/string_zipper.mli       |  31 ++++
 lsp/src/substring.ml            | 121 ++++++++++++++-
 lsp/src/substring.mli           |  39 +++++
 lsp/src/text_document.ml        |  42 +++---
 lsp/test/dune                   |   1 +
 lsp/test/string_zipper_tests.ml | 142 ++++++++++++++++++
 lsp/test/substring_tests.ml     | 120 +++++++++++++++
 lsp/test/text_document_tests.ml |  79 ++++++++--
 13 files changed, 810 insertions(+), 39 deletions(-)
 create mode 100644 lsp/src/position.ml
 create mode 100644 lsp/src/string_zipper.ml
 create mode 100644 lsp/src/string_zipper.mli
 create mode 100644 lsp/test/string_zipper_tests.ml
 create mode 100644 lsp/test/substring_tests.ml

diff --git a/CHANGES.md b/CHANGES.md
index 766fefa6f..15fab8fcb 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -1,3 +1,9 @@
+# Unreleased
+
+## Fixes
+
+- Fix a document syncing issue when utf-16 is the position encoding (#1004)
+
 # 1.15.1
 
 ## Fixes
@@ -7,8 +13,6 @@
   [#941](https://github.com/ocaml/ocaml-lsp/issues/941),
   [#1003](https://github.com/ocaml/ocaml-lsp/issues/1003))
 
-# 1.15.0
-
 ## Features
 
 - Enable [semantic highlighting](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_semanticTokens)
diff --git a/lsp/src/import.ml b/lsp/src/import.ml
index 769f877ce..f35d0c121 100644
--- a/lsp/src/import.ml
+++ b/lsp/src/import.ml
@@ -1,6 +1,8 @@
 module List = Stdlib.ListLabels
 module Option = Stdlib.Option
 module Array = Stdlib.ArrayLabels
+module Bytes = Stdlib.BytesLabels
+module Map = Stdlib.MoreLabels.Map
 
 module Result = struct
   include Stdlib.Result
diff --git a/lsp/src/lsp.ml b/lsp/src/lsp.ml
index 7b6619cfc..00d9cc799 100644
--- a/lsp/src/lsp.ml
+++ b/lsp/src/lsp.ml
@@ -15,4 +15,6 @@ module Diff = Diff
 
 module Private = struct
   module Array_view = Array_view
+  module Substring = Substring
+  module String_zipper = String_zipper
 end
diff --git a/lsp/src/position.ml b/lsp/src/position.ml
new file mode 100644
index 000000000..737e54265
--- /dev/null
+++ b/lsp/src/position.ml
@@ -0,0 +1,5 @@
+include Types.Position
+
+let zero = create ~line:0 ~character:0
+
+let is_zero (t : t) = t.line = zero.line && t.character = zero.character
diff --git a/lsp/src/string_zipper.ml b/lsp/src/string_zipper.ml
new file mode 100644
index 000000000..033262b79
--- /dev/null
+++ b/lsp/src/string_zipper.ml
@@ -0,0 +1,257 @@
+open! Import
+module Range = Types.Range
+
+module T = struct
+  type t =
+    { left : Substring.t list
+    ; rel_pos : int  (** the cursor's position *)
+    ; current : Substring.t
+          (** [current] needed to prevent fragmentation of the substring. E.g.
+              so that moving inside the substring doesn't create unnecessary
+              splits *)
+    ; line : int
+          (** the number of '\n' characters traversed past the current position *)
+    ; right : Substring.t list
+    }
+end
+
+include T
+
+let of_string s =
+  { left = []
+  ; rel_pos = 0
+  ; current = Substring.of_string s
+  ; right = []
+  ; line = 0
+  }
+
+let length =
+  let f acc sub = acc + Substring.length sub in
+  fun { current; left; right; rel_pos = _; line = _ } ->
+    let init = Substring.length current in
+    let init = List.fold_left ~init ~f left in
+    List.fold_left ~init ~f right
+
+let to_string t =
+  let dst = Bytes.make (length t) '\000' in
+  let dst_pos = ref 0 in
+  let f sub =
+    Substring.blit sub ~dst ~dst_pos:!dst_pos;
+    dst_pos := !dst_pos + Substring.length sub
+  in
+  List.iter (List.rev t.left) ~f;
+  f t.current;
+  List.iter t.right ~f;
+  Bytes.unsafe_to_string dst
+
+let empty = of_string ""
+
+let to_string_debug t =
+  let left, right = Substring.split_at t.current t.rel_pos in
+  List.rev_append t.left (left :: Substring.of_string "|" :: right :: t.right)
+  |> List.map ~f:Substring.to_string
+  |> String.concat ~sep:""
+
+let cons sub list = if Substring.length sub = 0 then list else sub :: list
+
+let is_end t =
+  let res = Substring.length t.current = t.rel_pos in
+  (if res then
+   match t.right with
+   | [] -> ()
+   | _ :: _ ->
+     invalid_arg
+       (sprintf "invalid state: current = %S" (Substring.to_string t.current)));
+  res
+
+let is_begin t =
+  match t.left with
+  | [] -> t.rel_pos = 0
+  | _ :: _ -> false
+
+let insert t (x : string) =
+  if String.length x = 0 then t
+  else
+    let current = Substring.of_string x in
+    let rel_pos = 0 in
+    if t.rel_pos = 0 then
+      { t with current; rel_pos; right = cons t.current t.right }
+    else if t.rel_pos = Substring.length t.current then
+      { t with current; rel_pos; left = cons t.current t.left }
+    else
+      let l, r = Substring.split_at t.current t.rel_pos in
+      { t with current; rel_pos; left = l :: t.left; right = r :: t.right }
+
+let advance_char t =
+  if is_end t then t
+  else
+    let line =
+      match Substring.get_exn t.current t.rel_pos with
+      | '\n' -> t.line + 1
+      | _ -> t.line
+    in
+    let rel_pos = t.rel_pos + 1 in
+    if rel_pos < Substring.length t.current then { t with rel_pos; line }
+    else
+      match t.right with
+      | [] -> { t with rel_pos; line }
+      | current :: right ->
+        { left = t.current :: t.left; current; line; right; rel_pos = 0 }
+
+let rec find_next_nl t =
+  if is_end t then t
+  else
+    match Substring.index_from t.current ~pos:t.rel_pos '\n' with
+    | Some rel_pos -> { t with rel_pos }
+    | None -> (
+      match t.right with
+      | [] -> { t with rel_pos = Substring.length t.current }
+      | current :: right ->
+        { t with left = t.current :: t.left; current; right; rel_pos = 0 }
+        |> find_next_nl)
+
+let rec goto_line_forward t n =
+  if n = 0 then t
+  else if is_end t then t
+  else
+    let t = find_next_nl t in
+    let t = advance_char t in
+    goto_line_forward t (n - 1)
+
+(* put the cursor left of the previous newline *)
+let rec prev_newline t =
+  if is_begin t then t
+  else
+    match Substring.rindex_from t.current ~pos:t.rel_pos '\n' with
+    | Some rel_pos -> { t with rel_pos; line = t.line - 1 }
+    | None -> (
+      match t.left with
+      | [] -> { t with rel_pos = 0 }
+      | current :: left ->
+        prev_newline
+          { t with
+            current
+          ; left
+          ; rel_pos = Substring.length current
+          ; right = t.current :: t.right
+          })
+
+let beginning_of_line t =
+  let t = prev_newline t in
+  if is_begin t then t else advance_char t
+
+let rec goto_line_backward t = function
+  | 0 -> beginning_of_line t
+  | n -> goto_line_backward (prev_newline t) (n - 1)
+
+let goto_line t n =
+  if t.line = n then beginning_of_line t
+  else if t.line > n then goto_line_backward t (t.line - n)
+  else goto_line_forward t (n - t.line)
+
+let newline = Uchar.of_char '\n'
+
+let nln = `ASCII newline
+
+module Advance (Char : sig
+  val units_of_char : Uchar.t -> int
+end) : sig
+  val advance : t -> code_units:int -> t
+end = struct
+  let feed_current_chunk dec t = Substring.Uutf.src t.current ~pos:t.rel_pos dec
+
+  let finish_chunk (t : t) consumed =
+    let rel_pos = t.rel_pos + consumed in
+    if rel_pos < Substring.length t.current then { t with rel_pos }
+    else (
+      assert (rel_pos = Substring.length t.current);
+      match t.right with
+      | [] -> { t with rel_pos }
+      | current :: right ->
+        { t with current; left = t.current :: t.left; right; rel_pos = 0 })
+
+  let rec loop dec (t : t) byte_count_ex_this_chunk (remaining : int) : t =
+    if remaining = 0 then
+      finish_chunk t (Uutf.decoder_byte_count dec - byte_count_ex_this_chunk)
+    else
+      match Uutf.decode dec with
+      | `Malformed _ -> assert false
+      | `End | `Await -> next_chunk dec t remaining
+      | `Uchar u ->
+        if Uchar.equal u newline then
+          finish_chunk
+            t
+            (Uutf.decoder_byte_count dec - byte_count_ex_this_chunk - 1)
+        else
+          let remaining = remaining - Char.units_of_char u in
+          loop dec t byte_count_ex_this_chunk remaining
+
+  and next_chunk dec (t : t) remaining =
+    match t.right with
+    | [] -> { t with rel_pos = Substring.length t.current }
+    | current :: right ->
+      let t =
+        { t with left = t.current :: t.left; current; right; rel_pos = 0 }
+      in
+      feed_current_chunk dec t;
+      loop dec t (Uutf.decoder_byte_count dec) remaining
+
+  let advance t ~code_units =
+    if code_units = 0 then t
+    else
+      let dec = Uutf.decoder ~nln ~encoding:`UTF_8 `Manual in
+      feed_current_chunk dec t;
+      loop dec t 0 code_units
+end
+
+let advance_utf16 =
+  let module Char = struct
+    let units_of_char u = Uchar.utf_16_byte_length u / 2
+  end in
+  let module F = Advance (Char) in
+  F.advance
+
+let advance_utf8 =
+  let module Char = struct
+    let units_of_char = Uchar.utf_8_byte_length
+  end in
+  let module F = Advance (Char) in
+  F.advance
+
+let drop_until from until =
+  if is_end from then from
+  else
+    let right = cons (Substring.drop until.current until.rel_pos) until.right in
+    let left = cons (Substring.take from.current from.rel_pos) from.left in
+    match right with
+    | current :: right -> { from with left; right; current; rel_pos = 0 }
+    | [] -> (
+      match left with
+      | [] -> empty
+      | current :: left ->
+        { from with left; right; current; rel_pos = Substring.length current })
+
+let apply_change t (range : Range.t) encoding ~replacement =
+  let advance =
+    match encoding with
+    | `UTF8 -> advance_utf8
+    | `UTF16 -> advance_utf16
+  in
+  let t = goto_line t range.start.line in
+  let t = advance t ~code_units:range.start.character in
+  let t' =
+    let delta_line = range.end_.line - range.start.line in
+    let delta_character =
+      if delta_line = 0 then range.end_.character - range.start.character
+      else range.end_.character
+    in
+    let t = if delta_line = 0 then t else goto_line t range.end_.line in
+    advance t ~code_units:delta_character
+  in
+  insert (drop_until t t') replacement
+
+module Private = struct
+  include T
+
+  let reflect x = x
+end
diff --git a/lsp/src/string_zipper.mli b/lsp/src/string_zipper.mli
new file mode 100644
index 000000000..7a9c20414
--- /dev/null
+++ b/lsp/src/string_zipper.mli
@@ -0,0 +1,31 @@
+type t
+
+val of_string : string -> t
+
+val to_string : t -> string
+
+val to_string_debug : t -> string
+
+(* [insert t s] right of the current position *)
+val insert : t -> string -> t
+
+val goto_line : t -> int -> t
+
+val drop_until : t -> t -> t
+
+val apply_change :
+  t -> Types.Range.t -> [ `UTF16 | `UTF8 ] -> replacement:string -> t
+
+module Private : sig
+  type zipper := t
+
+  type nonrec t =
+    { left : Substring.t list
+    ; rel_pos : int
+    ; current : Substring.t
+    ; line : int
+    ; right : Substring.t list
+    }
+
+  val reflect : zipper -> t
+end
diff --git a/lsp/src/substring.ml b/lsp/src/substring.ml
index 4f9de3503..fa7bf0a75 100644
--- a/lsp/src/substring.ml
+++ b/lsp/src/substring.ml
@@ -1,5 +1,4 @@
-module Array = ArrayLabels
-module Bytes = BytesLabels
+open Import
 
 type t =
   { pos : int
@@ -7,13 +6,19 @@ type t =
   ; base : string
   }
 
+let empty = { pos = 0; len = 0; base = "" }
+
 let of_slice base ~pos ~len =
   assert (pos >= 0 && pos + len <= String.length base);
   assert (len >= 0);
   { base; pos; len }
 
+let of_string base = { base; len = String.length base; pos = 0 }
+
 exception Result of int
 
+let length t = t.len
+
 let compare t { pos; len; base } =
   try
     for i = 0 to min t.len len - 1 do
@@ -35,8 +40,120 @@ let concat arr =
   done;
   Bytes.unsafe_to_string dst
 
+let drop t len =
+  if len = t.len then empty
+  else if len = 0 then t
+  else (
+    assert (len > 0);
+    let len = min len t.len in
+    let pos = t.pos + len in
+    let len = t.len - len in
+    { t with pos; len })
+
+let take t len =
+  if len = t.len then t
+  else if len = 0 then empty
+  else (
+    assert (len > 0);
+    let len = min t.len len in
+    { t with len })
+
+let to_string { base; len; pos } = String.sub base ~pos ~len
+
+let add_buffer { base; len; pos } buf = Buffer.add_substring buf base pos len
+
+let split_at t n = (take t n, drop t n)
+
+let rsplit_at t n =
+  let n = t.len - n in
+  split_at t n
+
+let index_from =
+  let rec loop s pos len c =
+    if pos >= len then None
+    else if s.[pos] = c then Some pos
+    else loop s (pos + 1) len c
+  in
+  fun t ~pos c ->
+    match loop t.base (t.pos + pos) (t.pos + t.len) c with
+    | None -> None
+    | Some pos -> Some (pos - t.pos)
+
+let rindex_from =
+  let rec loop s pos outside c =
+    if pos <= outside then None
+    else if s.[pos] = c then Some pos
+    else loop s (pos - 1) outside c
+  in
+  fun t ~pos c -> loop t.base (t.pos + pos - 1) (t.pos - 1) c
+
+let get_exn t i =
+  if i < t.len then t.base.[t.pos + i]
+  else invalid_arg "Substring.get: out of bounds"
+
+let rindex =
+  let rec loop s pos outside c =
+    if pos <= outside then None
+    else if s.[pos] = c then Some pos
+    else loop s (pos - 1) outside c
+  in
+  fun t c -> loop t.base (t.len + t.pos - 1) (t.pos - 1) c
+
+let blit t ~dst ~dst_pos =
+  Bytes.blit_string ~src:t.base ~src_pos:t.pos ~len:t.len ~dst ~dst_pos
+
+type move =
+  { newlines : int
+  ; consumed : int
+  }
+
+let move_right =
+  let rec loop base ~newlines ~pos ~outside =
+    if pos = outside then (newlines, pos)
+    else if base.[pos] = '\n' then
+      loop base ~newlines:(newlines + 1) ~pos:(pos + 1) ~outside
+    else loop base ~newlines ~pos:(pos + 1) ~outside
+  in
+  fun t ~pos ~len ->
+    if pos = t.len then { newlines = 0; consumed = 0 }
+    else (
+      assert (len >= 0);
+      assert (pos >= 0 && pos <= t.len);
+      let real_pos = t.pos + pos in
+      let outside = real_pos + min (t.len - pos) len in
+      let newlines, final_pos =
+        loop t.base ~newlines:0 ~pos:real_pos ~outside
+      in
+      { newlines; consumed = final_pos - real_pos })
+
+let move_left =
+  let rec loop base ~newlines ~pos ~outside =
+    if pos = outside then (newlines, pos)
+    else if base.[pos] = '\n' then
+      loop base ~newlines:(newlines + 1) ~pos:(pos - 1) ~outside
+    else loop base ~newlines ~pos:(pos - 1) ~outside
+  in
+  fun t ~pos ~len ->
+    if pos = 0 then { newlines = 0; consumed = 0 }
+    else (
+      assert (pos >= 0 && pos <= t.len);
+      let real_pos = t.pos + pos - 1 in
+      let outside = max (t.pos - 1) (real_pos - len) in
+      let newlines, final_pos =
+        loop t.base ~newlines:0 ~pos:real_pos ~outside
+      in
+      { newlines; consumed = real_pos - final_pos })
+
 module Map = MoreLabels.Map.Make (struct
   type nonrec t = t
 
   let compare = compare
 end)
+
+module Uutf = struct
+  let src t ~pos decoder =
+    let len = t.len - pos in
+    if len > 0 then
+      let pos = t.pos + pos in
+      Uutf.Manual.src decoder (Bytes.unsafe_of_string t.base) pos len
+end
diff --git a/lsp/src/substring.mli b/lsp/src/substring.mli
index 5eda06f5e..9efbff4ef 100644
--- a/lsp/src/substring.mli
+++ b/lsp/src/substring.mli
@@ -2,8 +2,47 @@ type t
 
 val of_slice : string -> pos:int -> len:int -> t
 
+val of_string : string -> t
+
 val compare : t -> t -> int
 
 val concat : t Array_view.t -> string
 
+val take : t -> int -> t
+
+val drop : t -> int -> t
+
+val to_string : t -> string
+
+val length : t -> int
+
+val add_buffer : t -> Buffer.t -> unit
+
+val split_at : t -> int -> t * t
+
+val rsplit_at : t -> int -> t * t
+
+val index_from : t -> pos:int -> char -> int option
+
+val rindex : t -> char -> int option
+
+val rindex_from : t -> pos:int -> char -> int option
+
+val get_exn : t -> int -> char
+
+type move =
+  { newlines : int
+  ; consumed : int
+  }
+
+val move_left : t -> pos:int -> len:int -> move
+
+val move_right : t -> pos:int -> len:int -> move
+
+val blit : t -> dst:bytes -> dst_pos:int -> unit
+
+module Uutf : sig
+  val src : t -> pos:int -> Uutf.decoder -> unit
+end
+
 module Map : MoreLabels.Map.S with type key = t
diff --git a/lsp/src/text_document.ml b/lsp/src/text_document.ml
index 2ad4c2de7..77c9c111d 100644
--- a/lsp/src/text_document.ml
+++ b/lsp/src/text_document.ml
@@ -1,12 +1,20 @@
-open Types
-module String = StringLabels
-module List = ListLabels
-module Map = MoreLabels.Map
+open Import
 
-exception Invalid_utf8
+include struct
+  open Types
+  module DidOpenTextDocumentParams = DidOpenTextDocumentParams
+  module Range = Range
+  module TextDocumentItem = TextDocumentItem
+  module TextDocumentContentChangeEvent = TextDocumentContentChangeEvent
+  module TextEdit = TextEdit
+end
 
 exception Outside
 
+exception Invalid_utf8
+
+let newline = Uchar.of_char '\n'
+
 let find_nth_nl =
   let rec find_nth_nl str nth pos len =
     if nth = 0 then pos
@@ -20,8 +28,6 @@ let find_nth_nl =
     | n -> n
     | exception Outside -> len
 
-let newline = Uchar.of_char '\n'
-
 let find_utf8_pos =
   let rec find_pos newline char dec =
     if char = 0 then Uutf.decoder_byte_count dec
@@ -133,31 +139,19 @@ let version (t : t) = t.document.version
 
 let languageId (t : t) = t.document.languageId
 
-let apply_change encoding text (change : TextDocumentContentChangeEvent.t) =
+let apply_change encoding sz (change : TextDocumentContentChangeEvent.t) =
   match change.range with
-  | None -> change.text
+  | None -> String_zipper.of_string change.text
   | Some range ->
-    let start_offset, end_offset =
-      let utf8 = text in
-      match encoding with
-      | `UTF16 -> find_offset_16 ~utf8 range
-      | `UTF8 -> find_offset_8 ~utf8 range
-    in
-    [| Substring.of_slice text ~pos:0 ~len:start_offset
-     ; Substring.of_slice change.text ~pos:0 ~len:(String.length change.text)
-     ; Substring.of_slice
-         text
-         ~pos:end_offset
-         ~len:(String.length text - end_offset)
-    |]
-    |> Array_view.make ~pos:0 |> Substring.concat
+    String_zipper.apply_change sz range encoding ~replacement:change.text
 
 let apply_content_changes ?version t changes =
   let text =
     List.fold_left
       ~f:(apply_change t.position_encoding)
-      ~init:t.document.text
+      ~init:(String_zipper.of_string t.document.text)
       changes
+    |> String_zipper.to_string
   in
   let document = { t.document with text } in
   let document =
diff --git a/lsp/test/dune b/lsp/test/dune
index e34b07fd8..f8262452e 100644
--- a/lsp/test/dune
+++ b/lsp/test/dune
@@ -7,6 +7,7 @@
   stdune
   lsp
   yojson
+  dyn
   ;; This is because of the (implicit_transitive_deps false)
   ;; in dune-project
   base
diff --git a/lsp/test/string_zipper_tests.ml b/lsp/test/string_zipper_tests.ml
new file mode 100644
index 000000000..b308bac8b
--- /dev/null
+++ b/lsp/test/string_zipper_tests.ml
@@ -0,0 +1,142 @@
+open Stdune
+module String_zipper = Lsp.Private.String_zipper
+module Substring = Lsp.Private.Substring
+
+let to_dyn { String_zipper.Private.left; rel_pos; current; right; line } =
+  let open Dyn in
+  let sub x = string (Substring.to_string x) in
+  let subs = list sub in
+  record
+    [ ("left", subs left)
+    ; ("rel_pos", int rel_pos)
+    ; ("current", sub current)
+    ; ("right", subs right)
+    ; ("line", int line)
+    ]
+
+type op =
+  [ `Goto_line of int
+  | `Insert of string
+  ]
+
+let test ?(which = `All) mode start operations =
+  let results =
+    List.fold_left
+      operations
+      ~init:[ (`Hide, start) ]
+      ~f:(fun acc (op : [ op | `Hide of op ]) ->
+        let final =
+          let _, last = List.hd acc in
+          let commit_op op =
+            match op with
+            | `Insert s -> String_zipper.insert last s
+            | `Goto_line g -> String_zipper.goto_line last g
+          in
+          match op with
+          | `Hide op -> (`Hide, commit_op op)
+          | #op as x -> (`Show, commit_op x)
+        in
+        final :: acc)
+    |> List.rev |> List.tl
+  in
+  let results =
+    match which with
+    | `All -> results
+    | `Last -> [ List.rev results |> List.hd ]
+  in
+  List.filter_map results ~f:(fun (display, res) ->
+      match display with
+      | `Hide -> None
+      | `Show -> Some res)
+  |> List.iter ~f:(fun res ->
+         let res =
+           match mode with
+           | `Dyn ->
+             String_zipper.Private.reflect res |> to_dyn |> Dyn.to_string
+           | `String ->
+             let line = (String_zipper.Private.reflect res).line in
+             Printf.sprintf
+               "line %d: %S"
+               line
+               (String_zipper.to_string_debug res)
+         in
+         Printf.printf "%s\n" res)
+
+let%expect_test "goto line" =
+  let foo = String_zipper.of_string "foo\nX\nY" in
+  test `String foo [ `Goto_line 0 ];
+  [%expect {| line 0: "|foo\nX\nY" |}];
+  test
+    `String
+    foo
+    [ `Goto_line 0
+    ; `Goto_line 1
+    ; `Goto_line 2
+    ; `Goto_line 3
+    ; `Goto_line 2
+    ; `Goto_line 1
+    ; `Goto_line 0
+    ; `Goto_line 0
+    ];
+  [%expect
+    {|
+    line 0: "|foo\nX\nY"
+    line 1: "foo\n|X\nY"
+    line 2: "foo\nX\n|Y"
+    line 2: "foo\nX\nY|"
+    line 2: "foo\nX\n|Y"
+    line 1: "foo\n|X\nY"
+    line 0: "|foo\nX\nY"
+    line 0: "|foo\nX\nY" |}];
+  test `String (String_zipper.of_string "") [ `Goto_line 100; `Goto_line 0 ];
+  [%expect {|
+    line 0: "|"
+    line 0: "|" |}];
+  test `String foo [ `Insert "baz"; `Goto_line 1; `Insert "1" ];
+  [%expect
+    {|
+    line 0: "|bazfoo\nX\nY"
+    line 1: "bazfoo\n|X\nY"
+    line 1: "bazfoo\n|1X\nY" |}]
+
+let%expect_test "insertions" =
+  let foo = String_zipper.of_string "foo" in
+  test `String foo [ `Insert "" ];
+  [%expect {|
+    line 0: "|foo" |}];
+  test `String foo [ `Insert "a" ];
+  [%expect {|
+    line 0: "|afoo" |}];
+  test `String foo [ `Insert "a"; `Insert "b" ];
+  [%expect {|
+    line 0: "|afoo"
+    line 0: "|bafoo" |}]
+
+let%expect_test "mixed insert goto" =
+  let foo = String_zipper.of_string "foo" in
+  test `String foo [ `Insert "XXX"; `Insert "YYY"; `Insert "zzz" ];
+  [%expect
+    {|
+    line 0: "|XXXfoo"
+    line 0: "|YYYXXXfoo"
+    line 0: "|zzzYYYXXXfoo" |}]
+
+let%expect_test "drop_until" =
+  let t = String_zipper.of_string "foo\nbar\nxxx" in
+  let t = String_zipper.goto_line t 1 in
+  let t' = String_zipper.goto_line t 2 in
+  let t = String_zipper.drop_until t t' in
+  printfn "%S" (String_zipper.to_string_debug t);
+  [%expect {|
+    "foo\n|xxx" |}];
+  let t = String_zipper.of_string "foo\nbar\n" in
+  let t = String_zipper.goto_line t 2 in
+  let t = String_zipper.drop_until t t in
+  printfn "%S" (String_zipper.to_string_debug t);
+  [%expect {|
+    "foo\nbar\n|" |}];
+  let t = String_zipper.of_string "123\n" in
+  let t = String_zipper.goto_line t 1 in
+  let t = String_zipper.drop_until t t in
+  printfn "%S" (String_zipper.to_string_debug t);
+  [%expect {| "123\n|" |}]
diff --git a/lsp/test/substring_tests.ml b/lsp/test/substring_tests.ml
new file mode 100644
index 000000000..ae1992fa2
--- /dev/null
+++ b/lsp/test/substring_tests.ml
@@ -0,0 +1,120 @@
+module Substring = Lsp.Private.Substring
+module List = ListLabels
+
+let printf = Printf.printf
+
+let make_sub pre sub post =
+  let res = String.concat "" [ pre; sub; post ] |> Substring.of_string in
+  let res = Substring.drop res (String.length pre) in
+  let res = Substring.take res (String.length sub) in
+  assert (sub = Substring.to_string res);
+  res
+
+let common_variations sub =
+  List.map
+    [ ("foo", ""); ("", "baz"); ("a", "b"); ("\n", ""); ("", "\n") ]
+    ~f:(fun (pre, post) ->
+      let name = Printf.sprintf "(%S, %S, %S)" pre sub post in
+      (name, make_sub pre sub post))
+
+let%expect_test "split_at" =
+  let test sub i =
+    let l, r = Substring.split_at sub i in
+    printf "l = %S r = %S\n" (Substring.to_string l) (Substring.to_string r)
+  in
+  let s = Substring.of_string "foo|bar" in
+  test s 0;
+  [%expect {| l = "" r = "foo|bar" |}];
+  test s 7;
+  [%expect {| l = "foo|bar" r = "" |}];
+  test s 3;
+  [%expect {| l = "foo" r = "|bar" |}];
+  test s 1;
+  [%expect {| l = "f" r = "oo|bar" |}]
+
+let%expect_test "index_from" =
+  let test sub pos char =
+    match Substring.index_from sub ~pos char with
+    | None -> print_endline "Not found"
+    | Some pos ->
+      printf "drop %d = %S\n" pos (Substring.drop sub pos |> Substring.to_string)
+  in
+  let s = Substring.of_string "foo|bar" in
+  test s 0 '|';
+  [%expect {| drop 3 = "|bar" |}];
+  test s 3 '|';
+  [%expect {| drop 3 = "|bar" |}];
+  test s 4 '|';
+  [%expect {| Not found |}]
+
+let%expect_test "rsplit_at" =
+  let test sub i =
+    let l, r = Substring.rsplit_at sub i in
+    printf "%S %S\n" (Substring.to_string l) (Substring.to_string r)
+  in
+  let s = Substring.of_string "foo|bar" in
+  test s 0;
+  [%expect {|
+    "foo|bar" "" |}];
+  test s 4;
+  [%expect {|
+    "foo" "|bar" |}];
+  test s 7;
+  [%expect {|
+    "" "foo|bar" |}]
+
+let test f sub ~pos ~len =
+  let res = f (Substring.of_string sub) ~pos ~len in
+  let print { Substring.newlines; consumed } =
+    Printf.printf "newlines = %d consumed = %d\n" newlines consumed
+  in
+  print_endline "[definitive]";
+  print res;
+  let variations = common_variations sub in
+  List.iter variations ~f:(fun (name, sub) ->
+      let res' = f sub ~pos ~len in
+      if res <> res' then (
+        printf "[FAIL] %s:\n" name;
+        print res'))
+
+let%expect_test "move_left" =
+  let test = test Substring.move_left in
+  test "foobar" ~pos:3 ~len:2;
+  [%expect {|
+    [definitive]
+    newlines = 0 consumed = 2 |}];
+  test "foobar" ~pos:3 ~len:0;
+  [%expect {|
+    [definitive]
+    newlines = 0 consumed = 0 |}];
+  test "fo\no\nbar" ~pos:4 ~len:3;
+  [%expect {|
+    [definitive]
+    newlines = 1 consumed = 3 |}];
+  test "fo\no\nbar" ~pos:4 ~len:2;
+  [%expect {|
+    [definitive]
+    newlines = 1 consumed = 2 |}];
+  test "fo" ~pos:1 ~len:2;
+  [%expect {|
+    [definitive]
+    newlines = 0 consumed = 1 |}]
+
+let%expect_test "move_right" =
+  let test = test Substring.move_right in
+  test "foobar" ~pos:3 ~len:2;
+  [%expect {|
+    [definitive]
+    newlines = 0 consumed = 2 |}];
+  test "foobar" ~pos:3 ~len:0;
+  [%expect {|
+    [definitive]
+    newlines = 0 consumed = 0 |}];
+  test "\n\nf" ~pos:2 ~len:3;
+  [%expect {|
+    [definitive]
+    newlines = 0 consumed = 1 |}];
+  test "fo\no\nbar" ~pos:4 ~len:2;
+  [%expect {|
+    [definitive]
+    newlines = 1 consumed = 2 |}]
diff --git a/lsp/test/text_document_tests.ml b/lsp/test/text_document_tests.ml
index 9d3162f07..f8f0ad7d4 100644
--- a/lsp/test/text_document_tests.ml
+++ b/lsp/test/text_document_tests.ml
@@ -1,6 +1,7 @@
 open Lsp
 open Lsp.Types
 module List = ListLabels
+module String = StringLabels
 
 let tuple_range start end_ =
   { Range.start =
@@ -30,8 +31,8 @@ let test_general text changes =
     in
     Text_document.text td
   in
-  let utf16 = test `UTF16 in
   let utf8 = test `UTF8 in
+  let utf16 = test `UTF16 in
   let printf = Printf.printf in
   if String.equal utf16 utf8 then printf "result: %s\n" (String.escaped utf8)
   else (
@@ -62,9 +63,51 @@ let%expect_test "no range" =
   [%expect {|
     result: XXXX |}]
 
+let%expect_test "char by char" =
+  test_multiple
+    ""
+    [ (tuple_range (0, 0) (0, 0), "f")
+    ; (tuple_range (0, 1) (0, 1), "o")
+    ; (tuple_range (0, 2) (0, 2), "o")
+    ];
+  [%expect {|
+    result: foo |}]
+
+let%expect_test "char by char - 2" =
+  test_multiple
+    "char by char - 2\n"
+    [ (tuple_range (1, 10) (1, 10), "b")
+    ; (tuple_range (1, 10) (1, 10), "a")
+    ; (tuple_range (1, 10) (1, 10), "r")
+    ; (tuple_range (1, 1) (1, 2), "")
+    ];
+  [%expect {|
+    result: char by char - 2\nbr |}]
+
+let%expect_test "char by char - 3" =
+  test_multiple
+    "first line skip\nchar by char - 2\n"
+    [ (tuple_range (1, 4) (1, 5), "")
+    ; (tuple_range (1, 3) (1, 4), "")
+    ; (tuple_range (1, 3) (1, 3), "x")
+    ];
+  [%expect {|
+    result: first line skip\nchaxby char - 2\n |}]
+
+let%expect_test "insert last" =
+  test "x" (tuple_range (0, 1) (0, 1)) ~change:"y";
+  [%expect {|
+    result: xy |}];
+  test "x\ny" (tuple_range (1, 1) (1, 1)) ~change:"z";
+  [%expect {|
+    result: x\nyz |}];
+  test "x\ny" (tuple_range (1, 10) (1, 10)) ~change:"z";
+  [%expect {|
+    result: x\nyz |}]
+
 let%expect_test "replace second line" =
   let range = tuple_range (1, 0) (2, 0) in
-  test "foo\n\bar\nbaz\n" range ~change:"XXXX\n";
+  test "foo\nbar\nbaz\n" range ~change:"XXXX\n";
   [%expect {|
     result: foo\nXXXX\nbaz\n |}]
 
@@ -75,10 +118,14 @@ let%expect_test "edit in second line" =
     result: foo\nb-XXX-r\nbaz\n |}]
 
 let%expect_test "insert at the end" =
-  let range = tuple_range (3, 1) (4, 0) in
-  test "foo\n\bar\nbaz\n" range ~change:"XXX";
+  let range = tuple_range (3, 0) (3, 0) in
+  test "foo\nbar\nbaz\n" range ~change:"XXX";
   [%expect {|
-   result: foo\n\bar\nbaz\nXXX |}]
+    result: foo\nbar\nbaz\nXXX |}];
+  let range = tuple_range (3, 0) (4, 0) in
+  test "foo\nbar\nbaz\n" range ~change:"XXX";
+  [%expect {|
+    result: foo\nbar\nbaz\nXXX |}]
 
 let%expect_test "insert at the beginning" =
   let range = tuple_range (0, 0) (0, 0) in
@@ -131,7 +178,8 @@ let%expect_test "remove text" =
 
 let%expect_test "remove newline - 1" =
   test "\n" (tuple_range (0, 0) (0, 1)) ~change:"";
-  [%expect {| result: \n |}]
+  [%expect {|
+    result: \n |}]
 
 let%expect_test "remove newlines - 2" =
   test_multiple "\nXXX\n" [ (tuple_range (0, 0) (0, 1), "") ];
@@ -142,8 +190,17 @@ let%expect_test "remove newlines - 3" =
   test_multiple
     "\nXXX\n\n"
     [ (tuple_range (0, 0) (0, 1), ""); (tuple_range (0, 1) (0, 2), "") ];
-  [%expect
-    {|
-    [FAILURE] utf16 and utf8 disagree
-    utf16: XX\n\n
-    utf8:  \nXXX\n\n |}]
+  [%expect {|
+    result: \nXXX\n\n |}]
+
+let%expect_test "update when inserting a line at the end of the doc" =
+  test "let x = 1;\n\nlet y = 2;" (tuple_range (2, 10) (2, 10)) ~change:"\n-ZZZ";
+  [%expect {|
+    result: let x = 1;\n\nlet y = 2;\n-ZZZ |}]
+
+let%expect_test "update when inserting a line at the end of the doc" =
+  test_multiple
+    "1\n2\n3\n"
+    [ (tuple_range (1, 9) (1, 9), "l"); (tuple_range (1, 9) (1, 10), "") ];
+  [%expect {|
+    result: 1\n2l\n3\n |}]