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

Fix #124: Treat :character offsets as UTF-16 code units #125

Closed
wants to merge 1 commit into from
Closed

Fix #124: Treat :character offsets as UTF-16 code units #125

wants to merge 1 commit into from

Conversation

mkcms
Copy link
Collaborator

@mkcms mkcms commented Oct 3, 2018

https://microsoft.github.io/language-server-protocol/specification#text-documents

The spec says that :character offsets are based on a UTF-16
representation. Previously we were assuming that :character specifies
the number of characters.

  • eglot.el (eglot--count-utf-16-code-units): New function.
    (eglot--pos-to-lsp-position):
    (eglot--lsp-position-to-point): Use it.

@MaskRay
Copy link
Contributor

MaskRay commented Oct 3, 2018

Rather than use UTF-16 unconditionally, you may consider hiding this under an option (I would even suggest using code points for performance consideration). Some other language clients don't UTF-8 (LanguageClient-neovim, lsp-mode) either and this isn't an issue in practice because code points in Supplementary Planes are rarely used. I dislike UTF-16 so much that I didn't even consider doing the conversion in cquery and now in ccls.

There is a bug tracking the conversion to code points and you may raise that topic again microsoft/language-server-protocol#376

@joaotavora
Copy link
Owner

Rather than use UTF-16 unconditionally, you may consider hiding this under an option

What potential problems could arise from using it unconditionally?

I would even suggest using code points for performance consideration

For the UTF-16-ignorant like me, what are code points?

@MaskRay
Copy link
Contributor

MaskRay commented Oct 7, 2018

A code point usually represents a single character (the closest concept is probably "grapheme cluster"). 𝛱 has 1 code point but requires 2 UTF-16 code units to encode, thus causing the incorrect computation of character

I find the time spent on forward-line is positively related to the distance. Converting between position and LSP Location is already slow, doing transcoding will make it even slower (I discovered this when optimizing semantic highlight). I understand that clangd conforms to LSP in this respect and you want make clients to do so, but I believe switching to code points is possible and this should not punish other language servers/clients using code points.

@mkcms
Copy link
Collaborator Author

mkcms commented Oct 7, 2018

@MaskRay

Supplementary Planes are rarely used

LSP can be used for things other than just programming. I didn't really benchmark this change, but maybe you're right and it will make eglot slower, although probably not by a large factor.

@MaskRay
Copy link
Contributor

MaskRay commented Oct 7, 2018

@mkcms although probably not by a large factor.

Yes if line/character -> position conversion is only done a few times. But in some scenarios (semantic highlight, codeLens), computation through (forward-line) exhibits noticeable overhead. I had strong feelings when I was optimizing the semantic highlight feature. There are many clang .cpp files with 5000+ lines of code. liblmdb/mdb.c has more than 10000+.

Many language servers may deliberately choose not to conform to LSP for the UTF-16 transcoding. It seems palantir/python-language-server diverges from LSP, too.

Please provide the UTF-16 transcoding under a defvar-local (or whatever) toggle.

The spec says that :character offsets are based on a UTF-16
representation.  Previously we were assuming that :character specifies
the number of characters.

* eglot.el (eglot-full-position-conversion): New defvar.
(eglot--count-characters): New function.
(eglot--pos-to-lsp-position):
(eglot--lsp-position-to-point): Use it.
@mkcms
Copy link
Collaborator Author

mkcms commented Nov 7, 2018

@MaskRay

Please provide the UTF-16 transcoding under a defvar-local (or whatever) toggle.

I pushed a commit which adds a defvar which controls whether UTF-16 encoding is used.

@mkcms mkcms requested a review from joaotavora November 7, 2018 20:39
Copy link
Owner

@joaotavora joaotavora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say @mkcms, I don't like this at all. It's:

I feel some effort should be made to use "normal" emacs abstractions. I'll have a better look at the problem, until then, let's shelve this.

@@ -721,23 +721,52 @@ CONNECT-ARGS are passed as additional arguments to
(let ((warning-minimum-level :error))
(display-warning 'eglot (apply #'format format args) :warning)))

(defvar eglot-full-position-conversion t
"Whether positions are calculated in full compliance with the standard.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If non-nil, calculate positions in full compliance with standard"

(defvar eglot-full-position-conversion t
"Whether positions are calculated in full compliance with the standard.
Setting this to nil may improve performance, but it can also
introduce bugs when characters wider than two UTF-16 code units
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably meant: "wider than one".

introduce bugs when characters wider than two UTF-16 code units
are used.")

(defun eglot--count-characters (beg end)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd name this "eglot--lsp-chars-between"

If `eglot-full-position-conversion' is non-nil, then convert the
region to UTF-16 and count the number of code units. Otherwise
return the distance between BEG and END."
(cond
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need so many newlines, it makes reading very strenuous, for me at least.

(line-beginning-position) (line-end-position))))
(pos 0))
(while (< pos lsp-pos)
(cl-incf pos (eglot--count-characters (point) (1+ (point))))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole section is extremely hacky, counting character lengths one by one, it's like you're reimplementing Emacs's coding systems.

@joaotavora
Copy link
Owner

@mkcms, have a look at 1999245 and tell me what you think. There's a very simple test, which we could overhaul with many difficult situations.

It'd be interesting to compare the performance of your approach and mine's (which I think uses more of Emacs's built-in machinery to do what you do by hand, and is therefore more readable).

Interestingly, Emacs's bufferpos-to-filepos can return exact or approximate char positions, which supposedly affects performance. I'm using exact but we could use something else.

@joaotavora
Copy link
Owner

Actually look here (3bbc937), I bungled that other commit quite badly with some stuff that wasn't supposed to go in.

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 8, 2018

@joaotavora Thanks for the review.

  • bad in performance, more than probably;

Yes, of course it will slow down the position conversion functions. What I meant before was overall performance that affects the user. My reasoning was that converting Emacs to LSP positions only takes a fraction of the overall time (e.g. when applying workspace edits, I/O and buffer modifications take the most time). Obviously, I haven't really benchmarked, so it's possible that my change would slow down Eglot by a large factor in other use cases. I only tested this change by doing regular coding work and I haven't noticed any slowdown.

I agree that 99% of the time, this is just redundant computation. Actually this issue only happened to me exactly once, and it wasn't even serious.


Actually look here (3bbc937),

I also looked at the bufferpos/filepos functions before. I didn't use them because they always calculate the position from beginning of buffer. This seems like significant overhead, although I have no idea how they're implemented. Maybe they're heavily optimized.


Maybe instead of unconditionally converting the positions, perhaps we could add a eglot-position-conversion-function (or just agree to keep eglot--lsp-position-to-point and eglot--pos-to-lsp-position always backwards-compatible) so that users who experience this problem can override those functions, and add a note in README.

@joaotavora
Copy link
Owner

joaotavora commented Nov 8, 2018

My reasoning was that converting Emacs to LSP positions only takes a fraction of the overall time

Depends. Consider a very big file with a very large list of diagnostics. It might take a much larger fraction there.

I agree that 99% of the time, this is just redundant computation. Actually this issue only happened to me exactly once, and it wasn't even serious.

Actually, it doesn't just happen with crazy characters like the one you had, my name João is also a candidade because of the ã. But I don't write my name often in code, only in the header at the most.
I would say 90%.

I also looked at the bufferpos/filepos functions before. I didn't use them because they always calculate the position from beginning of buffer.

This is true, but the buffer we're talking about doesn't need to be the buffer visiting the file, but a temporary buffer whose only contents are the line you're trying to discover the column in (EDIT: and this is what my version does).

This seems like significant overhead, although I have no idea how they're implemented. Maybe they're heavily optimized.

Most likely. They also offer this exactness/performance trade-off

Maybe instead of unconditionally converting the positions, perhaps we could add a eglot-position-conversion-function

This is a good idea.

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 11, 2018

This is a good idea.

Alright, I will do this.


I also did some benchmarks, comparing execution times of eglot--lsp-position-to-point on master, this branch and scratch/fix-124-idea.
The test case is converting a couple of thousand lsp positions to emacs points in a 1000 line file with some wide characters.
Here are the results:

$ ./run-benchmark.sh 
master 1.4869308209999998s
fix/issue-124 1.803970126s
scratch/fix-124-idea 7.018219207s

I've also managed to optimize my solution to get these times:

$ ./run-benchmark.sh 
master 1.389679567s
fix/issue-124 1.4014020470000002s
scratch/fix-124-idea 6.454922788999999s

The benchmark suite: benchmark.zip

@joaotavora
Copy link
Owner

@mkcms, I'll have a look at your benchmark but in the meantime, to avoid confusion of mental translations I've pushed two branches scratch/fix-124-joaotavora and scratch/fix-125-mkcms to hold the two approaches. I'll push tweaks to the first while you push to the second, and in the end we can decide which is the best.

@joaotavora
Copy link
Owner

@mkcms your benchmark idea is very good, but I don't understand insert-file-contents-literally. Shouldn't we be opening the test file normally, with its utf-8 encoding?

Also, positions.el as returned from an hypothetical UTF-16-counting LSP server doesn't point to the positions of the multibyte characters in the generated text.txt file. Please verify on your side.

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 11, 2018

I don't understand insert-file-contents-literally. Shouldn't we be opening the test file normally, with its utf-8 encoding?

Right, I missed that.

Attached is a new benchmark suite which will also test the number of times each solution was incorrect.
Here is my output for now:

$ ./run-benchmark.sh 
master 1.687929406s mismatches: 16960
scratch/fix-124-mkcms 2.620455026s mismatches: 0
scratch/fix-124-joaotavora 15.336856984s mismatches: 0

With optimization:

$ ./run-benchmark.sh 
master 1.655803003s mismatches: 17320
scratch/fix-124-mkcms 1.7465138349999998s mismatches: 0
scratch/fix-124-joaotavora 13.732144245s mismatches: 0

The optimization is this:
Instead of iterating from the beginning of line, first forward-char the same number of chars as :character, then iterate backwards until the number of code units between point-at-bol and point equals :character.


benchmark.zip

@joaotavora
Copy link
Owner

@mkcms, so I basically did the same as you and rewrote the benchmark to verify correctness.

Indeed the Emacs function bufferpos-to-filepos isn't suitable at all for the job. I though they were some kind of highly optimized C function but they're just plain Emacs Lisp that has to cater to multiple coding systems and therefore really really slow. So I came up with this solution, which is basically yours but a somes lines shorter

(defun eglot--lsp-char-to-column (lsp-charpos)
  "Helper for `eglot--lsp-position-to-point' with LSP-CHARPOS."
  (save-excursion
    (goto-char (line-beginning-position))
    (cl-loop
     with i = 0 while (< i (* 2 lsp-charpos))
     do (cl-incf i (- (length (encode-coding-region (point) (1+ (point))
                                                    'utf-16 t))
                      2))
     sum 1 do (forward-char))))

I pushed it to scratch/fix-124-joaotavora. It's a 50% slowdown, but I think we can take it.

master 1.023066238s total time, 8415 misses
scratch/fix-124-joaotavora 1.4330311040000001s total time, 0 misses
scratch/fix-124-mkcms 1.451512525s total time, 0 misses

I'm using your unoptimized version, I think. What is the optimization you had? Can it be applied to my version?

@joaotavora
Copy link
Owner

And some followup questions: won't this break those servers that are not following the standard and returning just the actual column number when confronted with complex utf-something characters? Are those the majority or the minority?

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 11, 2018

What is the optimization you had? Can it be applied to my version?

Yes, it can be applied from what I can see.
Consider this line:

yyyyyyyyyyyyyyyyyyyyyyyyyyyyyy𐐀x𐐀zyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy

To get to :character 35 (33rd column), currently we're iterating from beginning of line one by one until we get there (33 iterations).
Instead of doing that, we can simply forward 35 characters at once (this will put us on 35th column, 37th code unit), then simply iterate backward until the number of code units between point-at-bol and point is equal to 35. This will take only two iterations.
We can always do this because skipping :character chars forward will always put as exactly at desired column or somewhere after it.

won't this break those servers that are not following the standard and returning just the actual column number when confronted with complex utf-something characters? Are those the majority or the minority?

Good question. I will test some servers and report the stats here.

@joaotavora
Copy link
Owner

joaotavora commented Nov 12, 2018

Yes, it can be applied from what I can see.

Nice. I thought of that just now, too. It's a little faster indeed

(defun eglot--lsp-char-to-column (lsp-charpos)
  "Helper for `eglot--lsp-position-to-point' with LSP-CHARPOS."
  (save-excursion
    (let ((lbp (line-beginning-position)))
      (move-to-column lsp-charpos)
      (cl-loop
       while (< lsp-charpos
                (/ (- (length (encode-coding-region lbp (point)
                                                    'utf-16 t))
                      2)
                   2))
       do (backward-char)
       finally (cl-return (current-column))))))

But it might take more than two iterations if there are more multibyte chars before the target column.

@joaotavora
Copy link
Owner

Continuing this fun project, I've managed to optimized it a tad further by using a binary search:

(defun eglot--lsp-char-to-column (lsp-charpos)
  "Helper for `eglot--lsp-position-to-point' with LSP-CHARPOS."
  (save-excursion
    (cl-loop
     initially (move-to-column lsp-charpos)
     with lbp = (line-beginning-position)
     for diff = (- lsp-charpos
                   (/ (- (length (encode-coding-region lbp (point)
                                                       'utf-16 t))
                         2)
                      2))
     until (zerop diff)
     for offset = (max 1 (abs (/ diff 2)))
     do (if (> diff 0) (forward-char offset) (backward-char offset))
     finally
     (cl-return (current-column)))))

More seriously though, this breaks at least pyls, so I'm thinking of introducing an eglot--move-to-column function but leave it set to move-to-column.

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 13, 2018

@joaotavora Don't we need to also have a variable for eglot--point-to-lsp-position?

@MaskRay
Copy link
Contributor

MaskRay commented Nov 13, 2018

Nice:) This is interpolation search.

forward-char works on negative argment. Can

for offset = (max 1 (abs (/ diff 2)))
     do (if (> diff 0) (forward-char offset) (backward-char offset))

be simplified to something like (forward-char (if (> diff 0) (/ (1+ diff) 2) (/ (1- diff) 2)))

@joaotavora
Copy link
Owner

be simplified to something like (forward-char (if (> diff 0) (/ (1+ diff) 2) (/ (1- diff) 2)))

lol, not sure if that would be simplifying it. I was looking for a sign function though, and couldn't find it.

@joaotavora Don't we need to also have a variable for eglot--point-to-lsp-position?

I don't think so. Do we? Don't we just need an alternate way to move to the LSP column?

@joaotavora
Copy link
Owner

joaotavora commented Nov 13, 2018

(forward-char (if (> diff 0) (/ (1+ diff) 2) (/ (1- diff) 2)))

Actually, it isn't a bad idea, especially if I change this to (forward-char (/ (if (> diff 0) (1+ diff) (1- diff)) 2)

joaotavora added a commit that referenced this pull request Nov 13, 2018
* eglot.el (eglot-move-to-lsp-abiding-column): Simplify slightly.
@mkcms
Copy link
Collaborator Author

mkcms commented Nov 13, 2018

@joaotavora Don't we need to also have a variable for eglot--point-to-lsp-position?

I don't think so. Do we? Don't we just need an alternate way to move to the LSP column?

Yes, we do.
When I have a buffer with the same contents as in #124 and I manually fix the error by changing circle_are to circle_area, clangd gives me this diagnostic on the circle_area symbol:

Use of undeclared identifier 'circle_arae'; did you mean 'circle_area'?

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 13, 2018

Also instead of having two variables we can have one that defaults to #'identity which will

  1. Return new column position for current line when called with an integer argument (the LSP :character)
  2. Return :character for current column when called with a nil argument.

@joaotavora
Copy link
Owner

When I have a buffer with the same contents as in #124 and I manually fix the error by changing circle_are to circle_area, clangd gives me this diagnostic on the circle_area symbol:

Curious, but I don't understand what is happening, in what situation did eglot move to the wrong position?

@joaotavora
Copy link
Owner

@joaotavora Don't we need to also have a variable for eglot--point-to-lsp-position?

Never mind. Of course we do: we need to translate back to the format that clangd is expecting.

So I'd make a second variable indeed that is commonly set to current-column but could be set to eglot-lsp-abiding-column, which we have to write.

Then we can take care of the problem of setting both variables at once, if it is indeed a problem.

@joaotavora joaotavora reopened this Nov 13, 2018
@joaotavora
Copy link
Owner

@mkcms have a look at my latest commit. Curiously, the problem in #124 hasn't gone away, at least in clangd-6.0. It's like my clang is reporting for utf-8, so the function that corrects in my case is

(defun eglot-move-to-lsp-abiding-column (column)
  "Move to COLUMN abiding by the LSP spec."
  (cl-loop
   initially (move-to-column column)
   with lbp = (line-beginning-position)
   for diff = (- column
                 (/ (- (length (encode-coding-region lbp (point) 'utf-8 t))
                       0)
                    1))
   until (zerop diff)
   do (forward-char (/ (if (> diff 0) (1+ diff) (1- diff)) 2))))

What clangd version are you using?

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 14, 2018

@joaotavora Thanks, that seems to fix the issue.
I'm building newest clangd from source.

clangd version 8.0.0 (https://git.llvm.org/git/clang 551269913315bd099dbf45e2d37caf7bcfc57e74) (https://git.llvm.org/git/llvm 57aa4fc38b0878c6328ab565a128bb79805e29bd)

EDIT: If clangd-6.0 is older than April 27 2018, then it won't use UTF-16 code units:
llvm-mirror/clang-tools-extra@aa3548e

@joaotavora
Copy link
Owner

@mkcms, thanks for the tests.

To solve the problem of automatically setting the variables for clangd or other servers, I think the next step would be to introduce a new variable eglot-server-bindings, an association between server and variable bindings. The bindings would be set buffer locally in buffers managed by a server, and reset when the server stops managing it. But one has to think first about how to match a server to a group of bindings. As far as I see there is no standard way for a server to report its identity/version, so we would probably have to regexp-match the server executable (which would fail for TCP connections).

Also, in retrospect, I think my original review of your PR was too harsh :-) I wasn't reading it correctly and had kind of a knee-jerk reaction.

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 14, 2018

I think the next step would be to introduce a new variable eglot-server-bindings, an association between server and variable bindings.

That's a cool idea, I opened #152 for it.

Also, in retrospect, I think my original review of your PR was too harsh

No problem, I appreciate how much you pay attention to code quality. I'm not a professional lisper myself, so hopefully your reviews will make me a better emacs hacker.

non-Jedi added a commit to non-Jedi/lsp-range-unit-survey that referenced this pull request Oct 18, 2019
Eglot treats the offset as characters by default (codepoints).
joaotavora/eglot#125

Eglot and lsp-mode no longer listed as "work in progress" on https://langserver.org
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
Also close joaotavora/eglot#125.

Idea and much of design contributed by Michał Krzywkowski
<k.michal@zoho.com>

This introduces the variable eglot-move-to-column-function.

According to the standard, LSP column/character offsets are based
on a count of UTF-16 code units, not actual visual columns.  So
when LSP says position 3 of a line containing just \"aXbc\",
where X is a multi-byte character, it actually means `b', not
`c'.  This is what the function
`eglot-move-to-lsp-abiding-column' does.

However, many servers don't follow the spec this closely, and
thus this variable should be set to `move-to-column' in buffers
managed by those servers.

* eglot.el (eglot-move-to-column-function): New variable.
(eglot-move-to-lsp-abiding-column): New function.
(eglot--lsp-position-to-point): Use eglot-move-to-column-function.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
* eglot.el (eglot-move-to-lsp-abiding-column): Simplify slightly.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
* eglot.el (eglot-current-column-function): New variable.
(eglot-lsp-abiding-column): New helper.
(eglot--pos-to-lsp-position): Use eglot-current-column-function.
(eglot-move-to-column-function): Tweak docstring.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
Also close joaotavora/eglot#125.

Idea and much of design contributed by Michał Krzywkowski
<k.michal@zoho.com>

This introduces the variable eglot-move-to-column-function.

According to the standard, LSP column/character offsets are based
on a count of UTF-16 code units, not actual visual columns.  So
when LSP says position 3 of a line containing just \"aXbc\",
where X is a multi-byte character, it actually means `b', not
`c'.  This is what the function
`eglot-move-to-lsp-abiding-column' does.

However, many servers don't follow the spec this closely, and
thus this variable should be set to `move-to-column' in buffers
managed by those servers.

* eglot.el (eglot-move-to-column-function): New variable.
(eglot-move-to-lsp-abiding-column): New function.
(eglot--lsp-position-to-point): Use eglot-move-to-column-function.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
* eglot.el (eglot-move-to-lsp-abiding-column): Simplify slightly.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
* eglot.el (eglot-current-column-function): New variable.
(eglot-lsp-abiding-column): New helper.
(eglot--pos-to-lsp-position): Use eglot-current-column-function.
(eglot-move-to-column-function): Tweak docstring.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
Also close #125.

Idea and much of design contributed by Michał Krzywkowski
<k.michal@zoho.com>

This introduces the variable eglot-move-to-column-function.

According to the standard, LSP column/character offsets are based
on a count of UTF-16 code units, not actual visual columns.  So
when LSP says position 3 of a line containing just \"aXbc\",
where X is a multi-byte character, it actually means `b', not
`c'.  This is what the function
`eglot-move-to-lsp-abiding-column' does.

However, many servers don't follow the spec this closely, and
thus this variable should be set to `move-to-column' in buffers
managed by those servers.

* eglot.el (eglot-move-to-column-function): New variable.
(eglot-move-to-lsp-abiding-column): New function.
(eglot--lsp-position-to-point): Use eglot-move-to-column-function.

#124: joaotavora/eglot#124
#125: joaotavora/eglot#125
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
* eglot.el (eglot-move-to-lsp-abiding-column): Simplify slightly.

#125: joaotavora/eglot#125
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
* eglot.el (eglot-current-column-function): New variable.
(eglot-lsp-abiding-column): New helper.
(eglot--pos-to-lsp-position): Use eglot-current-column-function.
(eglot-move-to-column-function): Tweak docstring.

#125: joaotavora/eglot#125
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
Also close joaotavora/eglot#125.

Idea and much of design contributed by Michał Krzywkowski
<k.michal@zoho.com>

This introduces the variable eglot-move-to-column-function.

According to the standard, LSP column/character offsets are based
on a count of UTF-16 code units, not actual visual columns.  So
when LSP says position 3 of a line containing just \"aXbc\",
where X is a multi-byte character, it actually means `b', not
`c'.  This is what the function
`eglot-move-to-lsp-abiding-column' does.

However, many servers don't follow the spec this closely, and
thus this variable should be set to `move-to-column' in buffers
managed by those servers.

* eglot.el (eglot-move-to-column-function): New variable.
(eglot-move-to-lsp-abiding-column): New function.
(eglot--lsp-position-to-point): Use eglot-move-to-column-function.

GitHub-reference: fix joaotavora/eglot#124
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
* eglot.el (eglot-move-to-lsp-abiding-column): Simplify slightly.

GitHub-reference: joaotavora/eglot#125
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
* eglot.el (eglot-current-column-function): New variable.
(eglot-lsp-abiding-column): New helper.
(eglot--pos-to-lsp-position): Use eglot-current-column-function.
(eglot-move-to-column-function): Tweak docstring.

GitHub-reference: fix joaotavora/eglot#125
@astoff
Copy link
Contributor

astoff commented Feb 23, 2023

@MaskRay Since you participated in this discussion and in the definition of positionEncodings capability of LSP, I would like to point out this thread pertaining to the latter in emacs-bugs: https://yhetil.org/emacs-bugs/87a614g628.fsf@gmail.com/

Also, do you know about any servers that already implement positionEncodings?

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

Successfully merging this pull request may close these issues.

4 participants