Clarify undefined/null vs empty array/empty object/false

[Marwes]

There are a fair amount of optional fields on booleans and arrays where I think that the intention of leaving the field as undefined or null has the same meaning as false or the empty array. If this is the case we could simplify the deserialized structure in the Rust bindings a bit by removing the wrapping Option and telling the deserializer to just deserialize to false/[]/{}.

https://github.com/gluon-lang/languageserver-types/blob/315294fcec89b87278d7f50348071bcf9e22f452/src/lib.rs#L920

Could we have the protocol define that, unless otherwise stated, that a undefined field has the same meaning as false/[]/{}? Or if they should be handled differently then that should be stated.

Examples

The parameters field on SignatureInformation

Most fields on the capabilities objects https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#workspaceclientcapabilities-define-capabilities-the-editor--tool-provides-on-the-workspace

[Marwes]

On a related note, there seems to be some conflicting understandings of whether a optional field can be set to null gluon-lang/lsp-types#41. I thought I had read it to be equivalent in #87 but re-reading that now I seem to have misread it.

Is null fields allowed for ? fields? #87 seems to indicate no, but VS Code's language server appear to handle null the same as the field being missing.

[dbaeumer]

The protocol uses the following rules: if a property can be assigned 'null' it must be explicitly listed as a type (e.g. | null). Optional properties are marked with a question mark and NOT with | undefined to make clear that the property is not part of the JSON structure.

I am reluctant to change this since I am pretty sure that causes swirls for someone else. The fact that the implementation tolerates this is insufficient error checking in the implementation. Any objection to close the issue?

[rictic]

It sounds like the core question is:

Could we have the protocol define that, unless otherwise stated, an undefined/null field has the same meaning as false/[]/{}? Or if they should be handled differently then that should be stated.

[dbaeumer]

I am not a fan of this. Consider the following example:

interface CapOne {
   propertyP?: {
       value: number;
   }
}

This would mean that { propertyP: {} } is legal although it is very likely an error caused by not adding the value property.

[Marwes]

The fact that the implementation tolerates this is insufficient error checking in the implementation.

With regards to null vs the field being undefined. As far as I can tell from VS code's client it treats these the same. The rust language server as well as gluon's (the one I work on) use https://github.com/gluon-lang/languageserver-types which would serialize optional fields as null if they aren't set and both VS Code and https://github.com/autozimu/LanguageClient-neovim has handled nulls.

To clarify, I am fine if nulls are supposed to be rejected for ? fields, but I think it should be documented in that case. Any language client/server can still accept null as equivalent to undefined field if they want (to preserve backwards compatibility if they have done so by mistake). They just shouldn't send any nulls in these cases.

This would mean that { propertyP: {} } is legal although it is very likely an error caused by not adding the value property.

Fair enough. Maybe this rule only applies if the inner object only has optional fields as well? Unless that feels to specific. field?: bool and field:? [type] do not have that issue however.

[dbaeumer]

To clarify, I am fine if nulls are supposed to be rejected for ? fields, but I think it should be documented in that case.

The idea here is that if a type annotation doesn't include | null null is not a valid value. In TypeScript speak the protocol is defined using strict mode. Will this work for you?

[Marwes]

The idea here is that if a type annotation doesn't include | null null is not a valid value. In TypeScript speak the protocol is defined using strict mode. Will this work for you?

Yeah, that is fine. Only reason I have been passing nulls is that it is the default when serializing optionals in Rust, so since I weren't familiar with how this was specified in Typescript and nulls happened to work I never thought about it any further.

[dbaeumer]

Then I opt to close this issue. Any objections ?

[Marwes]

@dbaeumer There is also the case of the original question. Should undefined fields be treated equivalent to false/[]/{} (empty object only if it only has optional fields?)? I'd prefer to define these to do just that since that seems to be the intent in every case I have seen and it will simplify the Rust API for the language server protocol.

[dbaeumer]

Due to #355 (comment) is would not do this for {}. For arrays I wouldn't either. Even JS doesn't treat [] as a falsified value. So what is left is for boolean and there I am open to add this.

[Marwes]

Due to #355 (comment) is would not do this for {}. For arrays I wouldn't either. Even JS doesn't treat [] as a falsified value. So what is left is for boolean and there I am open to add this.

Ok. I don't want to drag this out much more but if the lack of a field and an empty array or object are not supposed to be treated equivalently we need to instead document how the client or server should handle the difference. Which, as far as I understand, means each of these optional fields will be defined to say that they ought to be treated the same (if my understating is wrong I guess that just means that documenting it is more important).

(Some examples)
actions https://microsoft.github.io/language-server-protocol/specification#window_showMessageRequest
arguments https://microsoft.github.io/language-server-protocol/specification#workspace_executeCommand
triggerCharacters https://microsoft.github.io/language-server-protocol/specification#textDocument_signatureHelp

Which seems a bit tedious. If there ever is a 4.0 it may be useful to remove this problem by requiring such fields to exist.

[rcjsuen]

On the flip side of the topic, is it fair to say that any non-optional fields that get sent as null or undefined (and of course are not defined with | null or | undefined) must result in an error being sent back?

Take the result of a textDocument/signatureHelp request for example. null is a valid return type but what if I just return { signatures: undefined } or { signatures: null }? Should the client throw an error back to the server in response? What about a { signatures: [] } since the specification says One or more signatures.?

[Marwes]

... must result in an error being sent back?

I don't think it needs to be specified that they must error on this though it would be nice if implementations did error (or warn perhaps) on things like that to prevent things being accidentally standardized.

[dbaeumer]

I will close the issue since I think we shouldn't change anything now. I added some clarification to the 3.17 version of the spec.