bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Troy Brown]

With the LSP language server I'm using, it's possible the language
server will return a textDocument/hover response MarkedString
containing "\r" (i.e., carriage return) for the line terminator when
the content contains multiple lines.  An example of a typical response
is as follows:

[jsonrpc] e[19:19:44.606] <-- textDocument/hover[38]
{"jsonrpc":"2.0","id":38,"result":{"contents":[{"language":"gpr","value":"type
Library_Kinds is (\"relocatable\", \"static\",
\"static-pic\");\rLibrary_Kind : Library_Kinds :=
\"static\";"},"gtkada_shared.gpr:37:04"]}}

The font locking is performed correctly (as the highlighting looks
fine), but when the content is placed in the *eldoc* buffer, content
surrounding the carriage return is not placed on separate lines.
Instead it remains on the same line and "^M" is displayed in the
buffer.

This is what I expect:

type Library_Kinds is ("relocatable", "static", "static-pic");
Library_Kind : Library_Kinds := "static";
gtkada_shared.gpr:37:04

This is what I'm observing (single line with embedded ^M):

type Library_Kinds is ("relocatable", "static",
"static-pic");^MLibrary_Kind : Library_Kinds := "static";
gtkada_shared.gpr:37:04

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Eli Zaretskii]

> Cc: João Távora <joaotavora@gmail.com>
> From: Troy Brown <brownts@troybrown.dev>
> Date: Mon, 12 Aug 2024 21:16:40 -0400
> 
> With the LSP language server I'm using, it's possible the language
> server will return a textDocument/hover response MarkedString
> containing "\r" (i.e., carriage return) for the line terminator when
> the content contains multiple lines.  An example of a typical response
> is as follows:
> 
> [jsonrpc] e[19:19:44.606] <-- textDocument/hover[38]
> {"jsonrpc":"2.0","id":38,"result":{"contents":[{"language":"gpr","value":"type
> Library_Kinds is (\"relocatable\", \"static\",
> \"static-pic\");\rLibrary_Kind : Library_Kinds :=
> \"static\";"},"gtkada_shared.gpr:37:04"]}}

Does this behavior comply to the LSP standards?

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Felician Nemeth]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: João Távora <joaotavora@gmail.com>
>> From: Troy Brown <brownts@troybrown.dev>
>> Date: Mon, 12 Aug 2024 21:16:40 -0400
>> 
>> With the LSP language server I'm using, it's possible the language
>> server will return a textDocument/hover response MarkedString
>> containing "\r" (i.e., carriage return) for the line terminator when
>> the content contains multiple lines.  An example of a typical response
>> is as follows:
>> 
>> [jsonrpc] e[19:19:44.606] <-- textDocument/hover[38]
>> {"jsonrpc":"2.0","id":38,"result":{"contents":[{"language":"gpr","value":"type
>> Library_Kinds is (\"relocatable\", \"static\",
>> \"static-pic\");\rLibrary_Kind : Library_Kinds :=
>> \"static\";"},"gtkada_shared.gpr:37:04"]}}
>
> Does this behavior comply to the LSP standards?

The way I understand it, Eglot calls gfm-view-mode to format this text.
So even if the observed behavior is not in line with the LSP
specification, it is the fault of gfm-view-mode and not directly
Eglot's.

gfm-view-mode is part of the markdown-mode package, which is NonGNU
ELPA.

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[João Távora]

On Tue, Aug 13, 2024 at 7:32 PM Felician Nemeth
<felician.nemeth@gmail.com> wrote:

> The way I understand it, Eglot calls gfm-view-mode to format this text.
> So even if the observed behavior is not in line with the LSP
> specification, it is the fault of gfm-view-mode and not directly
> Eglot's.
>
> gfm-view-mode is part of the markdown-mode package, which is NonGNU
> ELPA.

Very correct.  Worth noting, though, that markdown-mode is an _optional_
dependency of Eglot. It doesn't need it (but it will use it if it finds it).

João

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Troy Brown]

On Tue, Aug 13, 2024 at 2:45 PM João Távora <joaotavora@gmail.com> wrote:
>
> On Tue, Aug 13, 2024 at 7:32 PM Felician Nemeth
> <felician.nemeth@gmail.com> wrote:
>
> > The way I understand it, Eglot calls gfm-view-mode to format this text.
> > So even if the observed behavior is not in line with the LSP
> > specification, it is the fault of gfm-view-mode and not directly
> > Eglot's.
> >
> > gfm-view-mode is part of the markdown-mode package, which is NonGNU
> > ELPA.
>
> Very correct.  Worth noting, though, that markdown-mode is an _optional_
> dependency of Eglot. It doesn't need it (but it will use it if it finds it).
>

I don't think it's gfm-view-mode's fault.  I can get the same
problematic behavior with MarkupContent of "plaintext" (which uses
text-mode), as follows:

(eglot--format-markup '(:value "foo\rbar" :kind "plaintext"))

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Eli Zaretskii]

> From: Troy Brown <brownts@troybrown.dev>
> Date: Tue, 13 Aug 2024 17:35:57 -0400
> Cc: Felician Nemeth <felician.nemeth@gmail.com>, Eli Zaretskii <eliz@gnu.org>, 72597@debbugs.gnu.org
> 
> On Tue, Aug 13, 2024 at 2:45 PM João Távora <joaotavora@gmail.com> wrote:
> >
> > On Tue, Aug 13, 2024 at 7:32 PM Felician Nemeth
> > <felician.nemeth@gmail.com> wrote:
> >
> > > The way I understand it, Eglot calls gfm-view-mode to format this text.
> > > So even if the observed behavior is not in line with the LSP
> > > specification, it is the fault of gfm-view-mode and not directly
> > > Eglot's.
> > >
> > > gfm-view-mode is part of the markdown-mode package, which is NonGNU
> > > ELPA.
> >
> > Very correct.  Worth noting, though, that markdown-mode is an _optional_
> > dependency of Eglot. It doesn't need it (but it will use it if it finds it).
> >
> 
> I don't think it's gfm-view-mode's fault.  I can get the same
> problematic behavior with MarkupContent of "plaintext" (which uses
> text-mode), as follows:
> 
> (eglot--format-markup '(:value "foo\rbar" :kind "plaintext"))

Can you please answer my question whether this is standard-complying
behavior according to LSP protocol?  IOW, what is the source of the
assumption that CR characters will be interpreted as newlines in these
cases?

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Felician Nemeth]

Eli Zaretskii <eliz@gnu.org> writes:

> Can you please answer my question whether this is standard-complying
> behavior according to LSP protocol?  IOW, what is the source of the
> assumption that CR characters will be interpreted as newlines in these
> cases?

It seems this is standard-complying, because the specification has this:
"To ensure that both client and server split the string into the same
line representation the protocol specifies the following end-of-line
sequences: ‘\n’, ‘\r\n’ and ‘\r’."

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Eli Zaretskii]

> From: Felician Nemeth <felician.nemeth@gmail.com>
> Cc: Troy Brown <brownts@troybrown.dev>,  joaotavora@gmail.com,
>   72597@debbugs.gnu.org
> Date: Wed, 14 Aug 2024 07:54:24 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can you please answer my question whether this is standard-complying
> > behavior according to LSP protocol?  IOW, what is the source of the
> > assumption that CR characters will be interpreted as newlines in these
> > cases?
> 
> It seems this is standard-complying, because the specification has this:
> "To ensure that both client and server split the string into the same
> line representation the protocol specifies the following end-of-line
> sequences: ‘\n’, ‘\r\n’ and ‘\r’."

This just caters to the 3 known EOL formats: the Unix, the
DOS/Windows, and the Mac one.  If this is the basis for the described
behavior, then Emacs should bind coding-system-for-read when reading
this stuff, or maybe manually decode the strings after detecting EOL
format.  But that would only work if the EOL format is used
consistently in the entire response of the server; if they just use
the above as a means to sneak in multiple-line responses, and
otherwise use some other EOL format, then we need to handle a lone \r
specially in Eglot's code, something I'm not sure we like to do.

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Troy Brown]

On Wed, Aug 14, 2024 at 2:37 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Felician Nemeth <felician.nemeth@gmail.com>
> > Cc: Troy Brown <brownts@troybrown.dev>,  joaotavora@gmail.com,
> >   72597@debbugs.gnu.org
> > Date: Wed, 14 Aug 2024 07:54:24 +0200
> >
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> > > Can you please answer my question whether this is standard-complying
> > > behavior according to LSP protocol?  IOW, what is the source of the
> > > assumption that CR characters will be interpreted as newlines in these
> > > cases?
> >
> > It seems this is standard-complying, because the specification has this:
> > "To ensure that both client and server split the string into the same
> > line representation the protocol specifies the following end-of-line
> > sequences: ‘\n’, ‘\r\n’ and ‘\r’."
>
> This just caters to the 3 known EOL formats: the Unix, the
> DOS/Windows, and the Mac one.  If this is the basis for the described
> behavior, then Emacs should bind coding-system-for-read when reading
> this stuff, or maybe manually decode the strings after detecting EOL
> format.  But that would only work if the EOL format is used
> consistently in the entire response of the server; if they just use
> the above as a means to sneak in multiple-line responses, and
> otherwise use some other EOL format, then we need to handle a lone \r
> specially in Eglot's code, something I'm not sure we like to do.

Other than what Felician already quoted from the LSP specification,
there doesn't seem to be much else stated about EOL.  In this
particular case, I've seen it on both Linux and Windows, therefore the
use of the CR doesn't seem to be consistent with the platform or the
document format.  It's unclear to me why this particular EOL format
was chosen for the hover information, but I can't find anything in the
specification that says it is incorrect.

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Eli Zaretskii]

Ping! How can we make progress with this issue?

> From: Troy Brown <brownts@troybrown.dev>
> Date: Wed, 14 Aug 2024 07:41:36 -0400
> Cc: Felician Nemeth <felician.nemeth@gmail.com>, joaotavora@gmail.com, 72597@debbugs.gnu.org
> 
> On Wed, Aug 14, 2024 at 2:37 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > From: Felician Nemeth <felician.nemeth@gmail.com>
> > > Cc: Troy Brown <brownts@troybrown.dev>,  joaotavora@gmail.com,
> > >   72597@debbugs.gnu.org
> > > Date: Wed, 14 Aug 2024 07:54:24 +0200
> > >
> > > Eli Zaretskii <eliz@gnu.org> writes:
> > >
> > > > Can you please answer my question whether this is standard-complying
> > > > behavior according to LSP protocol?  IOW, what is the source of the
> > > > assumption that CR characters will be interpreted as newlines in these
> > > > cases?
> > >
> > > It seems this is standard-complying, because the specification has this:
> > > "To ensure that both client and server split the string into the same
> > > line representation the protocol specifies the following end-of-line
> > > sequences: ‘\n’, ‘\r\n’ and ‘\r’."
> >
> > This just caters to the 3 known EOL formats: the Unix, the
> > DOS/Windows, and the Mac one.  If this is the basis for the described
> > behavior, then Emacs should bind coding-system-for-read when reading
> > this stuff, or maybe manually decode the strings after detecting EOL
> > format.  But that would only work if the EOL format is used
> > consistently in the entire response of the server; if they just use
> > the above as a means to sneak in multiple-line responses, and
> > otherwise use some other EOL format, then we need to handle a lone \r
> > specially in Eglot's code, something I'm not sure we like to do.
> 
> Other than what Felician already quoted from the LSP specification,
> there doesn't seem to be much else stated about EOL.  In this
> particular case, I've seen it on both Linux and Windows, therefore the
> use of the CR doesn't seem to be consistent with the platform or the
> document format.  It's unclear to me why this particular EOL format
> was chosen for the hover information, but I can't find anything in the
> specification that says it is incorrect.
> 

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Troy Brown]

On Sat, Aug 31, 2024 at 3:55 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> Ping! How can we make progress with this issue?
>

Just FYI, for the time being I've been using this "return filter"
advice on eglot--format-markup which splits all lines irregardless of
the EOL marker and rejoins them with a newline.  It's a bit
heavy-handed, but it gets the job done.

(defun init.el/fix-eol/eglot--format-markup (value)
  (let ((strings (list value)))
    (dolist (eol '("\r\n" "\n" "\r"))
      (setq strings
            (flatten-list (mapcar (lambda (value)
                                    (split-string value eol))
                                  strings))))
    (string-join strings "\n")))

(advice-add 'eglot--format-markup :filter-return
#'init.el/fix-eol/eglot--format-markup)

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Eli Zaretskii]

> From: Troy Brown <brownts@troybrown.dev>
> Date: Sun, 1 Sep 2024 16:49:04 -0400
> Cc: joaotavora@gmail.com, felician.nemeth@gmail.com, 72597@debbugs.gnu.org
> 
> On Sat, Aug 31, 2024 at 3:55 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > Ping! How can we make progress with this issue?
> >
> 
> Just FYI, for the time being I've been using this "return filter"
> advice on eglot--format-markup which splits all lines irregardless of
> the EOL marker and rejoins them with a newline.  It's a bit
> heavy-handed, but it gets the job done.
> 
> (defun init.el/fix-eol/eglot--format-markup (value)
>   (let ((strings (list value)))
>     (dolist (eol '("\r\n" "\n" "\r"))
>       (setq strings
>             (flatten-list (mapcar (lambda (value)
>                                     (split-string value eol))
>                                   strings))))
>     (string-join strings "\n")))
> 
> (advice-add 'eglot--format-markup :filter-return
> #'init.el/fix-eol/eglot--format-markup)

Thanks, but isn't the above the same as setting the car of
process-coding-system for the Eglot subprocesses to 'dos'?

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[João Távora]

On Mon, Sep 2, 2024 at 12:24 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > (advice-add 'eglot--format-markup :filter-return
> > #'init.el/fix-eol/eglot--format-markup)
>
> Thanks, but isn't the above the same as setting the car of
> process-coding-system for the Eglot subprocesses to 'dos'?

If that works consistently across all OSs and servers, it's
a much better idea.  I don't know how this works in terms of
jsonrpc.el though. I hope it doesn't mess up the byte count
of JSON objects transmitted back and forth, and only affects
how we interpret strings inside those JSON objects. AFAIR
the buffer where we do the JSON deserialization is in utf-8,
but it uses position-bytes for everything.

Anyway, if the solution is to be performed at this lower
level (which I think it should), then Daniel Petterson
jsonrpc.el maintainer should be added.

João

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Daniel Pettersson]

João Távora <joaotavora@gmail.com> writes:

> On Mon, Sep 2, 2024 at 12:24 PM Eli Zaretskii <eliz@gnu.org> wrote:
>> Thanks, but isn't the above the same as setting the car of
>> process-coding-system for the Eglot subprocesses to 'dos'?

I would think that a better place for encoding the displayed string at
the place where we are displaying the string rather then before parsing
the json.

> Anyway, if the solution is to be performed at this lower
> level (which I think it should), then Daniel Petterson
> jsonrpc.el maintainer should be added.

I might be missing something, but jsonrpc should not in my mind
convert/format/encode any json data.  Any assumptions on line endings
can only be made on header and content separators, not on the json
payload itself.

/Daniel Pettersson

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[João Távora]

On Thu, Sep 5, 2024 at 10:32 PM Daniel Pettersson
<daniel@dpettersson.net> wrote:

> I might be missing something, but jsonrpc should not in my mind
> convert/format/encode any json data.  Any assumptions on line endings
> can only be made on header and content separators, not on the json
> payload itself.

That's also a reasonable position, I guess. The line ending
convention is somewhat of a grey zone. jsonrpc.el produces
Elisp strings from JSON strings.  It could apply Emasc's
line-ending conventions.  Or maybe not, maybe you're right
and Eglot the jsonrpc.el client should do it.  Or really this
quirky server should just use \n like everyone else.

João

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Eli Zaretskii]

> From: Daniel Pettersson <daniel@dpettersson.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Troy Brown <brownts@troybrown.dev>,
>   felician.nemeth@gmail.com,  72597@debbugs.gnu.org
> Date: Thu, 05 Sep 2024 23:32:08 +0200
> 
> > Anyway, if the solution is to be performed at this lower
> > level (which I think it should), then Daniel Petterson
> > jsonrpc.el maintainer should be added.
> 
> I might be missing something, but jsonrpc should not in my mind
> convert/format/encode any json data.  Any assumptions on line endings
> can only be made on header and content separators, not on the json
> payload itself.

That depends on the reason why the CR character appeared there in the
first place.  Are they required for the strings in questions, or are
they simply an artifact of how the server marshaled JSON data on the
wire?  In the latter case, the CR characters are the result of
Windows-style text writing, which adds a CR to each newline, and
therefore removing that CR should indeed happen where Emacs decodes
the EOLs of the incoming stuff.

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Daniel Pettersson]

Eli Zaretskii <eliz@gnu.org> writes:

> That depends on the reason why the CR character appeared there in the
> first place.  Are they required for the strings in questions, or are
> they simply an artifact of how the server marshaled JSON data on the
> wire?

I am not convinced, in my mind if the marshaling operation converts \n
to \r\n (or any other character combination) within a string literal,
then the server's JSON conversion is broken.

> In the latter case, the CR characters are the result of
> Windows-style text writing, which adds a CR to each newline, and
> therefore removing that CR should indeed happen where Emacs decodes
> the EOLs of the incoming stuff.

I fail to see how that is not due to an broken json marshaling and not
intentionally chosen line endings by the server.

Of course I might be overlooking something.

/Daniel Pettersson

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[João Távora]

On Fri, Sep 6, 2024 at 6:44 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Daniel Pettersson <daniel@dpettersson.net>
> > Cc: Eli Zaretskii <eliz@gnu.org>,  Troy Brown <brownts@troybrown.dev>,
> >   felician.nemeth@gmail.com,  72597@debbugs.gnu.org
> > Date: Thu, 05 Sep 2024 23:32:08 +0200
> >
> > > Anyway, if the solution is to be performed at this lower
> > > level (which I think it should), then Daniel Petterson
> > > jsonrpc.el maintainer should be added.
> >
> > I might be missing something, but jsonrpc should not in my mind
> > convert/format/encode any json data.  Any assumptions on line endings
> > can only be made on header and content separators, not on the json
> > payload itself.
>
> That depends on the reason why the CR character appeared there in the
> first place.  Are they required for the strings in questions, or are
> they simply an artifact of how the server marshaled JSON data on the
> wire?

Indeed this is the question.  And it is not necessarily easy to asnwer.
For example most LSP servers may be used to edit CRLF terminated
files, and when doing so they will possibly exchange file snippets with
intentional CR chars in them (however odd that may be for most
people, it can happen).  But that is theoretically completely independent
of the decision to terminate documentation snippets with CRLF or just
LF or just CR.  Both these kinds of snippets are transmitted as JSON
strings over the wire.  jsonrpc.el makes them Elisp strings and (afair)
doesn't do any newline conversion.

Even if Eglot is most well positioned to answer this question in
the LSP case, i don't think it can: there's no interface for doing so.
If it could, then it could theoretically ask jsonrpc.el to do that
conversion. But it can't.

So perhaps something like Troys patch for the "doc view" layer
is best if inefficient/hacky.  Even fixing it in gfm-view-mode isn't a bad
idea. But also, I don't think this is a very urgent matter: the server
that does this is clearly in the minority.  I would just ask this server's
devs if they wouldn't mind not sending these odd doc snippets.

João

bug#72597: 30.0.60; Eglot: MarkedString with embedded Carriage Return

[Troy Brown]

On Fri, Sep 6, 2024 at 3:31 PM João Távora <joaotavora@gmail.com> wrote:
>
> I would just ask this server's
> devs if they wouldn't mind not sending these odd doc snippets.
>

I've logged an inquiry with the LS developers.  Hopefully we'll at
least gain some insight with a rationale.

https://github.com/AdaCore/ada_language_server/issues/1204