Fix more bugs in eglot--apply-text-edits

[mkcms]

This fixes bugs which I found when using the eclipse.jdt.ls server. If there are multiple insertions in the same place, the inserted text must be in the same order as the TextEdit[] array itself. https://microsoft.github.io/language-server-protocol/specification#textedit-1

To reproduce it, simply run the eclipse.jdt.ls server without this fix and try some code actions.

[joaotavora]

OK I finally understand the problem. By the way a simpler way to reproduce (tried installing Eclipse but failed miserably) is to dogfood eglot--apply-text-edits directly in scratch fro example.

Erase your *scratch* and put this there:

;; aaabbb

(eglot--apply-text-edits
 [(:range
   (:start
    (:line 0 :character 6)
    :end
    (:line 0 :character 9))
   :newText "bar")
  (:range
   (:start
    (:line 0 :character 3)
    :end
    (:line 0 :character 6))
   :newText "foo")
  ])

Now, I think the current implementation is confusing. There are three alternatives:

1. If we are going to make both markers "push-type" markers then we might as well do it ineglot--lsp-position-to-point by just adding a t to the copy marker call.

2. The most correct way would be to make the beg marker "push" and the end marker "stay". This is because of adjacent replacements where the first replacement touches the markers of an unrelated edit. (I've dealt with this problem repeatedly in yasnippet in the past) In the case of 0-length edits we should just return the same "push" marker twice. This would be done in eglot--range-region.
   Now this might be overkill because the problem avoided by this problem does not happen with replace-buffer-contents. It does with an implementation based on delete-region and insert though. However, because this behaviour is not specified in replace-buffer-contents I wouldn't take the risk. The example I pasted above shows what could go wrong if replace-buffer-contents starts behaving like delete-region+insert and we don't have this implementation.

3. If we don't care about 2, then by far the easiest way is to revert the order of the edits. This is because the specification tells us that the order is only relevant in the "same position call", so except for that case, it doesn't matter in which order we apply the edits, and we might as well apply them in reverse and fix this bug.

[joaotavora]

BTW I'm on the fence between 2 and 3, because 3 is the easiest. But FTR here's an implementation of 2.

diff --git a/eglot.el b/eglot.el
index d29bef5..7e34113 100644
--- a/eglot.el
+++ b/eglot.el
@@ -608,16 +608,15 @@ CONNECT-ARGS are passed as additional arguments to
          :character (- (goto-char (or pos (point)))
                        (line-beginning-position)))))
 
-(defun eglot--lsp-position-to-point (pos-plist &optional marker)
-  "Convert LSP position POS-PLIST to Emacs point.
-If optional MARKER, return a marker instead"
+(defun eglot--lsp-position-to-point (pos-plist)
+  "Convert LSP position POS-PLIST to Emacs point."
   (save-excursion (goto-char (point-min))
                   (forward-line (min most-positive-fixnum
                                      (plist-get pos-plist :line)))
                   (forward-char (min (plist-get pos-plist :character)
                                      (- (line-end-position)
                                         (line-beginning-position))))
-                  (if marker (copy-marker (point-marker)) (point))))
+                  (point)))
 
 (defun eglot--path-to-uri (path)
   "URIfy PATH."
@@ -673,9 +672,14 @@ under cursor."
 (defun eglot--range-region (range &optional markers)
   "Return region (BEG . END) that represents LSP RANGE.
 If optional MARKERS, make markers."
-  (let* ((st (plist-get range :start))
-         (beg (eglot--lsp-position-to-point st markers))
-         (end (eglot--lsp-position-to-point (plist-get range :end) markers)))
+  (let* ((beg (set-marker (make-marker)
+                          (eglot--lsp-position-to-point
+                           (plist-get range :start))))
+         (end (set-marker (make-marker)
+                          (eglot--lsp-position-to-point
+                           (plist-get range :end)))))
+    (set-marker-insertion-type beg t)
+    (if (= beg end) (setq end beg))
     (cons beg end)))
 
 
@@ -1477,7 +1481,9 @@ If SKIP-SIGNATURE, don't try to send textDocument/signatureHelp."
                           ;; https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32237
                           ;; https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32278
                           (let ((inhibit-modification-hooks t)
-                                (length (- end beg)))
+                                (length (- end beg))
+                                (beg (marker-position beg))
+                                (end (marker-position end)))
                             (run-hook-with-args 'before-change-functions
                                                 beg end)
                             (replace-buffer-contents temp)

[mkcms]

If we don't care about 2, then by far the easiest way is to revert the order of the edits.

I chose this option, is it OK?

[joaotavora]

Yeah, I was going to choose that too. Give it bit of testing if you can though.

[mkcms]

Give it bit of testing if you can though.

Alright, I tested it a bit and so far I haven't seen any problems.

[joaotavora]

Good, then go ahead and do the change.

[mkcms]

Good, then go ahead and do the change.

I already did. I see you already committed, but one more thing that is missing is calling change functions with marker positions instead of markers. I remember them signaling errors if the arguments are not integers, but I can't really reproduce it.

[joaotavora]

I see you already committed

Sorry, I missed your commit somehow.

I remember them signaling errors if the arguments are not integers, but I can't really reproduce it

They shouldn't signal errors just because the are passed markers. But indeed, if you make the markers push, then you must pass positions otherwise the arguments you pass to the before and after change functions aren't the ones that are supposed to.