bug#74807: 30.0.90; Eglot: Non-Markdown strings rendered as Markdown

[Troy Brown <brownts@troybrown.dev>]

On Mon, Jan 6, 2025 at 6:56 AM João Távora <joaotavora@gmail.com> wrote:
>
> Hello Troy,  I've finally had time to look at your bug report.

Thanks, I appreciate it.

>
> > I've run across a situation where Eglot receives a documentation
> > string as part of a "completionItem/resolve".  The documentation is
> > being provided as a regular "string", not MarkupContent, yet it is
> > being rendered as markup.  Since the string contains characters which
> > are being interpreted as markdown (e.g., ":"), it causes the
> > documentation to be rendered incorrectly.
>
> After perusing the spec, I'm not sure the simple strings cannot be
> rendered as Markdown by the client.  The spec doesn't seem to
> disambiguate this.  All it says about the documentation field of a
> Completion structure, besides its polymorphic type, is that it is a
>
>     /**
>      * A human-readable string that represents a doc-comment.
>      */
>      documentation?: string | MarkupContent;
>
>
> Anyway I think if the server wants to ensure something is _not_ rendered
> as Markdown it should use the more advanced MarkupContent structure and
> explicitly specify 'plaintext' in its MarkupKind field.
>
> I understand this sounds counter-intuitive, but I have to be very
> careful to do these kinds of changes.  As you know, Eglot works with a
> large body of servers, and I wouldn't be at all surprised that some of
> those servers (or, more importantly, regulat users of those servers) do
> actually expect plain strings to be rendered as Markdown when such a
> renderer is available.
>

I'm fairly confident that plain text is heavily implied by the type
being "string".  If the spec wanted to indicate that the string could
be markup, it would have been specified as "MarkedString" instead of
"string", like this:

   documentation?: MarkedString | MarkupContent;

The fact that it is specified as "string" strongly suggests that this
was intentional in order to indicate that it does not contain markup.
I understand, and appreciate your careful consideration of these kinds
of changes, however even the VSCode implementation does not treat this
as markdown, as can be seen at the following link where when the
CompletionItem.documentation is a "string", the markdown renderer is
not applied to the documentation:

   https://github.com/microsoft/vscode/blob/aaa576acca01852119f6a6b0260cf5aa74a30c58/src/vs/editor/contrib/suggest/browser/suggestWidgetDetails.ts#L168-L185

Furthermore, I would think that if, as you suggest, there were servers
which expected plain strings to be rendered as Markdown here, the
VSCode implementation would not avoid rendering plain strings as
markdown.

> Lastly, and adding to my reluctance to address this in code, I don't
> understand what ':' is tripping the renderer here.  Last I checked, ':'
> doesn't have any special meaning in Markdown, especially in the middle
> of the sequence.  Emacs's `markdown-view-mode` from the markdown.el
> package does give the left and right parts of:
>
>    Indentation kind: spaces | tabs
>
> a different face (I don't know why, the online renderers I've tried do
> not do that).  But I wouldn't say it is being rendered "incorrectly".
>

While you might argue that the example I've shown is a fringe example,
I'm sure you could envision other documentation content which uses
characters which are interpreted as markup, and shouldn't be.  I stand
by my wording of "rendered incorrectly" when based on the content of
the documentation, you might see it displayed differently, possibly
more severely based on the documentation content.

> Anyway, I lean strongly towards not touching this.

I hope you reconsider, given the additional information I've provided.


Thanks,

Troy.