Hover / Suggest inconsistency: Plaintext Linebreak

[octref]

Split from #78718:

Plaintext Linebreak

const markupContent: MarkupContent =  {
	kind: 'plaintext',
	value: 'foo\nbar'
};

[octref]

The problem comes from our API:

vscode.d.ts:

type MarkedString = MarkdownString | string | { language: string; value: string }

class CompletionItem {
  documentation?: string | MarkdownString
}
class Hover {
  contents: MarkedString[]
}

Observations:

• 'foo' as CompletionItem#documentation would be rendered as plaintext
• ['foo'] as Hover#contents would be rendered as markdown
• Completion can rendered either Markdown or Plaintext. Hover can't.

This somewhat makes sense, since string is explicitly an type allowed on CompletionItem#documentation, and the string of 'foo' is interpreted as a subtype of MarkedString in Hover#contents.

However in LSP we have:

type MarkupKind = 'plaintext' | 'markdown'
interface MarkupContent {
  kind: MarkupKind
  value: string
}
interface Hover {
  contents: MarkupContent | MarkedString | MarkedString[]
}
interface CompletionItem {
  documentation?: string | MarkupContent
}

Observations:

• Both 'foo' and { kind: 'plaintext': value: 'foo' } as CompletionItem#documentation would be rendered as plaintext
• Both 'foo' and { kind: 'plaintext': value: 'foo' } as Hover#contents would be rendered as markdown.
• foo as as Hover#contents is subtype of MarkedString, so it's fine if it's rendered as Markdown
• { kind: 'plaintext': value: 'foo' } as Hover#contents rendered as Markdown doesn't make sense and is inconsistent with CompletionItem#documentation.

We can fix this by allowing string in Hover#contents, like:

class Hover {
  contents: (MarkedString | string)[]
}

for VS Code API, and make LSP node library adopt it.

@jrieken and @dbaeumer, what's your thoughts?

[dbaeumer]

I do see the problem that we don't support string type directly in Hover however it is supported through MarkdownString.appendText which to my understanding adds plain text to the MarkdownString. I would not add string to Hover.contents. I would rather deprecate string from CompletionItem#documentation

For the describe problem with the line break \n I would fix this in the converter in the LSP client here: https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/protocolConverter.ts#L307

[octref]

I'm wondering if this change should happen in the LS client library or vscode side, since MarkdownString.appendText('\n') should append \n\n if properly escaped. And MarkedString.appendText does advertise "escaping":

https://github.com/microsoft/vscode/blob/master/src/vs/base/common/htmlContent.ts#L25

	export class MarkdownString {
		/**
		 * Appends and escapes the given string to this markdown string.
		 * @param value Plain text.
		 */
		appendText(value: string): MarkdownString;
	}

[dbaeumer]

@octref that would even be better. Lets see what @jrieken thinks.

[jrieken]

👍 for fixing this in appendText

[dbaeumer]

@octref nice!

[dbaeumer]

Doesn't work for me.

I think the change also needs to be done here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/api/common/extHostTypes.ts#L1230

[octref]

Good catch. My bad.