bug#61726: [PATCH] Eglot: Support positionEncoding capability

[João Távora]

On Fri, Feb 24, 2023 at 9:15 AM Augusto Stoffel <arstoffel@gmail.com> wrote:
> >                   ^^^^^^^^^^^
> > This last part shouldn't be necessary: we should move by characters,
> > not by columns.  Why is it necessary?
>
> Maybe João can clarify, but I'm pretty sure this is there to support the
> UTF-16 way of counting offsets, so this ideally should move to
> eglot-move-to-lsp-abiding-column.

I have to be brutally honest here: I don't like this patch.  I
appreciate the effort, I really do, and thank for guiding us its
the motions, but there are two main things I really don't like
about it, and 1 that I'm on the fence about.

The first thing I don't like is likely the reason that Eli is
confused here.  The late binding of column-counting strategies
is confusing.  I wrote these functions so that each one has
a separate well-defined, readable-inasmuch-as-possible,
vc-region-history-traceable, performant column-counting
strategy. The "lsp-abiding" naming might be off, I admit, but
only since LSP started supporting more than one strategy.

The second thing I don't like is also due to the late-binding idea.
This is a hotspot in Eglot, some of these functions are called
many many times, for each LSP server interaction depending
on how many document positions are exchanged (and they can
be a lot).  I do remember benchmarking strategies at the time
and seeing a perceptible difference.  Plus, this late-binding is
really useless as a server will guaranteedly _not_ change its
column-counting standard during the LSP session.

The third thing that I'm not crazy with but I don't mind is
the necessity to support the "utf-8" strategy.  If "utf-16"
is mandatory, and we already support "utf-32" anyway, why should
we be adding this additional complexity.  But, if it can be
hidden behind a new pair of functions and Eli accepts it,
I'm OK with it.

Finally, here's a patch that doesn't use late-binding, doesn't
introduce new strategies and supports "utf-32" and "utf-16"
today.  As you can see, the patch is nearly trivial.

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index eea8be6d1aa..ae8afa69651 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -807,6 +807,7 @@ eglot-client-capabilities
              :rangeFormatting    `(:dynamicRegistration :json-false)
              :rename             `(:dynamicRegistration :json-false)
              :inlayHint          `(:dynamicRegistration :json-false)
+             :general            `(:positionEncodings ["utf-32" "utf-16"])
              :publishDiagnostics (list :relatedInformation :json-false
                                        ;; TODO: We can support
:codeDescription after
                                        ;; adding an appropriate UI to
@@ -1789,6 +1790,9 @@ eglot--managed-mode
       (add-hook 'eldoc-documentation-functions #'eglot-signature-eldoc-function
                 nil t)
       (eldoc-mode 1))
+    (when (eq (eglot--server-capable :positionEncoding) "utf-16")
+      (eglot--setq-saving eglot-move-to-column-function #'eglot-move-to-column)
+      (eglot--setq-saving eglot-current-column-function
#'eglot-current-column))
     (cl-pushnew (current-buffer) (eglot--managed-buffers
(eglot-current-server))))
    (t
     (remove-hook 'after-change-functions 'eglot--after-change t)

As I said, enhancing this patch with a new pair of "current/move-to"
functions that add in utf-8 support is acceptable.

João