Clangd should respect isPreferredSupport in CodeActionClientCapabilities

[canatella]

Since llvm/llvm-project@8392685, clangd is always sending isPreferred property in code action suggestions even if the matching capability iso not advertised by the client in CodeActionClientCapabilities

[kadircet]

Thanks for flagging this, sent out https://reviews.llvm.org/D90452 to address the issue.

[canatella]

Thank YOU for acting so quickly on it :)

[sam-mccall]

Generally we only check capabilities when doing things that are not backwards compatible.
And we assume adding extra fields to objects is backwards-compatible (as this is how the protocol is extended, sometimes without controlling capabilities).

For example:

• We send PublishDiagnosticsParams.version without checking PublishDiagnosticsClientCapabilities.versionSupport
• We set CompletionItem.deprecated without checking CompletionClientCapabilities.deprecatedSupport
• We set score as an extension on CompletionItem and SymbolInformation without guarding this behind a capability

The reason is: sending messages to exactly the schema the client negotiates, rather than a compatible one, adds more complexity and increases the support surface. And in practice we don't know that it brings any benefit.

Can you explain what problem this caused? Does eglot reject messages that contain unknown properties? Any idea why this doesn't manifest with e.g. CompletionItem etc?

(Strictly speaking I think this is legal per LSP as the types are specified as interfaces, and objects defining additional properties are legal values for them in the TypeScript type system. But I'm more interested in practical interop problems than exactly what the spec says...)

[joaotavora]

Can you explain what problem this caused? Does eglot reject messages that contain unknown properties?

When used in "strict" mode it does that. But I doubt that OP is using it in that mode. Likely eglot just ignored the property and did nothing useful with it.

as this is how the protocol is extended, sometimes without controlling capabilities

Sure about this? Is the protocol extended without adding the controlling capabilities? I don't recall examples, but I have been out of the loop for a while.

[canatella]

Can you explain what problem this caused? Does eglot reject messages that contain unknown properties?

When used in "strict" mode it does that. But I doubt that OP is using it in that mode. Likely eglot just ignored the property and did nothing useful with it.

It did cause an eglot-error in eglot--dcase handling of the action list.

[joaotavora]

It did cause an eglot-error in eglot--dcase handling of the action list.

This is odd. What value do you have in eglot-strict-mode? It should be nil for end users, meaning "lax". Maybe you can give us a backtrace (perhaps in the Eglot issue?).

[joaotavora]

But I'm more interested in practical interop problems than exactly what the spec says..

@sam-mccall , I have to say I disagree with this: If we do as you say, the spec -- and thus the whole point of LSP -- slowly starts becoming useless.

[canatella]

This is odd. What value do you have in eglot-strict-mode? It should be nil for end users, meaning "lax". Maybe you can give us a backtrace (perhaps in the Eglot issue?).

I can reproduce it easily with eglot-strict-mode set to nil and the lastest clangd snapshot. I added the backtrace to joaotavora/eglot#558

[joaotavora]

I've had a look @canatella , and it doesn't seem to have anything to do with isPreferred, but I might be wrong. Anyway, to this discussion, what's relevant is that there is an eglot-strict-mode by default set to nil, settable to t when debugging/double-checking servers.

[canatella]

I've had a look @canatella , and it doesn't seem to have anything to do with isPreferred, but I might be wrong.

Well all I know is that this PR fixes the problem :)

Anyway, to this discussion, what's relevant is that there is an eglot-strict-mode by default set to nil, settable to t when debugging/double-checking servers.

Good to know, I'll use it for next time it will certainly ease the debug process 😅