microsoft / language-server-protocol

Defines a common protocol for language servers.
https://microsoft.github.io/language-server-protocol/
Creative Commons Attribution 4.0 International
11.17k stars 788 forks source link

MetaModel.json use during LSP implementation #1847

Closed aoime closed 8 months ago

aoime commented 10 months ago

I'm looking for advice on how to use the provided metaModel.json to implement the LSP specification specifically as it relates to extended enumeration values. The specification states that enumerations are not exhaustive and unlisted / unknown values should be allowed to pass through. The MetaModel includes a "supportsCustomValues" field on some enumeration types that conceivably could be used to allow the enumeration type it is associated with to allow for any value. If the metaModel is used as a source truth in a code-generation process to determine which enum values are allowed or not, enumerations that do not include the "supportsCustomValues" field could be seen to generate code that disallows unlisted values. That would seem to conflict with the specification that states that all enumeration type should allow any value "To support the evolution of enumerations...". How would you advise the metaModel and the "supportsCustomValues" field be used to inform a code-generation process?

The specific issue I'm attempting to address is that Visual Studio 2022 - v17.8.1 (not VS Code) sends values in the CustomItemKind and DiagnosticTag fields during the initialize handshake that are not specified in the metaModel but would seem to be allowed by the text of the specification. The Zig language server (ZLS) uses a code generation process with the metaModel.json as input in this fashion where enums without the "supportsCustomValues" field do not allow for unknown enum values which causes the handshake to fail and ZLS to be unusable with VS 2022. It seems that the ZLS process is either implementing the code generation process incorrectly (enforcing no unknown enum values on fields without the "supportsCustomValues" tag) or metaModel.json is ambiguous in its definition. Should there be another tag in the metaModel for enums or should code generated from the metaModel allow for all values on all enum types?

Fields sent by Visual Studio in initialize request. Note the 0 and all values >= 118115 in CompletionItemKind and the -1, -2 values of the DiagnosticTag field are not listed in the spec but only allowed under the "Enumerations" specific text in the spec document. "completionItemKind":{"valueSet":[0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,118115,118116,118117,118118,118119,118120,118121,118122,118123,118124,118125,118126]} "publishDiagnostics":{"tagSupport":{"valueSet":[1,2,-1,-2]}}

Essentially, should all enumerations be generated to allow custom values and be defined as signed integers? What is the value in metaModel.json defining numeric enumeration types as 'uinteger' instead of 'integer' and only some types instead of all types as supportsCustomValues?

dbaeumer commented 10 months ago

There is a difference between custom values and evolution of enums. Custom values might be interpreted by the client or server depending on their implementation. An example are FoldingRangeKinds. They support custom values and a server could sent function and a client could do something special with it. Custom values don't show up in the specification.

Evolution of enums is different from that. The enum set is defined in the specification. However a 3.18 client could talk to a 3.17 server hence sending enum values that are unknown to the server. The server should handle the unknown values gracefully (e.g. not throwing an exception) and if such a value roundtrip should even send the not known value back.

That is why the code that is generated for TypeScript doesn't use an enum type for enum definitions. It uses number with const definitions (see https://github.com/microsoft/vscode-languageserver-node/blob/02806427ce7251ec8fa2ff068febd9a9e59dbd2f/types/src/main.ts#L1933)

This allows that an enum with an unknown value will be assigned to a number variable and will be preserved, but while coding only the const in the namespace declaration are used. I know that this is sometimes hard to understand but otherwise a 3.18 client might only be able to talk to a 3.18 server which makes backwards compatibility very hard.

aoime commented 10 months ago

Thank you for the insight. To try to be clear about it, the text of the spec is the spec and the metaModel.json is a potentially helpful tool but doesn't negate the text of the spec. i.e. as long as a client is sending well formed requests even if they include unknown values for enums or negative values where the metaModel.json shows the type as an unsigned integer, a proper LSP implementation that adheres to the spec should accept those values without shutting down or refusing an initialize request. Is that fair?

dbaeumer commented 10 months ago

Actually the negative values are a little tricky. Let me pull in someone from VS to understand why they send them to public servers. @MariaSolOs do you know whom I could ping about the VS LSP client implementation?

dbaeumer commented 10 months ago

For positive values the statement is actually correct.

MariaSolOs commented 10 months ago

Actually the negative values are a little tricky. Let me pull in someone from VS to understand why they send them to public servers. @MariaSolOs do you know whom I could ping about the VS LSP client implementation?

@dbaeumer I think @matteo-prosperi could help us here.

matteo-prosperi commented 10 months ago

From the VS point of view, I can take DiagnosticTag as an example, since it is one of the VS-specific LSP extensions that are documented: see here. The special DiagnosticTag values that are supported by Visual Studio are indeed negative. I believe the reason is to avoid conflicts with new values that could be added to the LSP specification in the future. Also, the existence of these values predates the metamodel. So, at the time, there was nothing in the LSP specification forbidding negative values. @dbaeumer, my suggestion would be:

aoime commented 10 months ago
* update the metamodel to use `integer` instead of `uinteger` in order to avoid confusion and to help with the correct creation of any autogenerated parsers.

Is your suggestion to update the backing type on DiagnosticTag only, all numeric enums in the spec, or some subset of enums that VS can provide a list of for backward compatibility that predate the metaModel? As there was not any limitation in older specs on negative values that I could find, all numeric enums as 'integer' seems appropriate but does that cause some value to be lost in having some types as 'uinteger'?

matteo-prosperi commented 10 months ago

My suggestion is to use integer instead of uinteger for all numeric enumerations in the metamodel.

aoime commented 9 months ago

@dbaeumer any followup on the suggestion to use integer for all numeric enumerations?

dbaeumer commented 9 months ago

I think this is more complicated. From a compatibility perspective we can't simply change this since it is widening a type which is a breaking change. We even recommend in the spec that enumerations should start with 1 and historically we never had any negative enumeration values. We do have some that start with 0 and we kept them to not break any clients / servers.

Need to think about what a good solution is.

aoime commented 8 months ago

@dbaeumer I'm just wondering if there has been any progress on what to do here. Are there other steps / investigations in progress?

Can you further explain or state examples on the compatibility issues / breaking change concern?

The VS2022 extension is released on VS marketplace now but is based on a fork of ZLS that allows the DiagnosticTag (signed) and CompletionItemKind (spec extended) values through. I'd like to be able to drop the fork and revert back to only supporting the master ZLS branch when this is resolved. Thank you.

dbaeumer commented 8 months ago

@aoime yes, Visual Studio will change their code so that it doesn't send any negative values for enumerations anymore. This will keep the spec as is. But it might take a little bit until the change is available in VS.

aoime commented 1 month ago

@dbaeumer @matteo-prosperi Hi, just wondering if there is any info available on the timeline for the VS change to remove the negative values. I'm using 17.10.5 now and still seeing the same -1, -2 values in the initialize method.

"publishDiagnostics": { "tagSupport": { "valueSet": [ 1, 2, 2147483646, 2147483645, -1, -2 ] } }

Also, is it possible to intercept the initialize method in an extension? I've been able to intercept all language server calls after the initialize but not the initialize itself.

dbaeumer commented 1 month ago

@matteo-prosperi can you comment on that?

matteo-prosperi commented 1 month ago

@aoime, this is being worked on. My best guess is that you will see negative values not being sent anymore in the initialize message some time before the end of this year.