Language-server-protocol: Clarification of `number` type

[Aerijo]

The spec definition uses "number" in a lot of places. As it is TypeScript based, this would imply any integer or floating point number is permitted. This is a problem for languages that have separate types for integers and floats.

In practice I imagine most implementations are using integer values. What prompted this was the WorkDoneProgress set of notifications, which have a percentage property. The doc comment implies that integer values are expected, but it doesn't say this outright and it would not be unreasonable to send floating point values to represent a fraction of a percent.

It would help a lot if the kinds of number number can represent are explicitly stated. If this is already in the spec I couldn't find it before the first usage in the Content-Length header description.

[dbaeumer]

So far number is used as integer. The protocol is not using any floats.

[rwols]

But clangd reports the progress percentage with a float.

cc @sam-mccall

[sam-mccall]

Some wording around number vs integer would be appreciated, maybe "number means 53-bit integer unless otherwise specified"?

Sending percentage: 98.5 seems natural and compliant with the current spec. If WorkDoneProgressReport.percentage is supposed to be an integer, I think the spec should say so. (But I don't think this would be an improvement)

FWIW, we look at this case by case and in every other case we decided not to send/receive non-integers. E.g. we'll reject a request with VersionedTextDocumentIdentifier.version = 17.3, while accepting both 17 and 17.0. Maybe non-complaint by the letter of the spec, but no complaints yet.

[Aerijo]

I don't think specifying the exact integer bit count is good. Typically the range of a number would be determined by it's purpose, and many grow with the size of the file (e.g., Positions). So any width can only be a lower bound unless you want to tie the protocol to files below a certain size.

If a client/server can pick the representation width, then I would just let them pick whatever they think is sane, and respond to overflow issues as they see fit. At most I think the spec should just add some advice on recommended minimum widths. Implementations that can satisfy them can be happy, and ones that can't aren't automatically noncompliant just because they can't handle files above some arbitrary size.

Allowing floats for percentage seems reasonable, clients could just round it as they like. It wouldn't even contradict the spec as-is.

I definitely would appreciate if the signedness is specified, even if it's just to say everything is signed. File versions apparently have clients returning negative numbers. But one particular type that normally wouldn't be expected to contain negative numbers is Position, so there probably exist implementations that only take non-negative values. (negative values are useful for translating Positions by other Positions, but I don't know of any existing reason for this in the protocol).

[puremourning]

If a client/server can pick the representation width, then I would just let them pick whatever they think is sane, and respond to overflow issues as they see fit.

Completely disagree. A specification should be complete and data types are key parts of a specification. Otherwise it’s at best unpredictable, perhaps de facto, and at worst unworkable.

[Aerijo]

@puremourning I think the width discussion should be a separate issue. I opened this one to have 'number' differentiated between integers and floats. This is universally useful, as many source languages support both (even if they appear as a single kind). And, based on https://github.com/microsoft/language-server-protocol/issues/1037#issuecomment-662412400, integer-only languages are still compatible with the protocol as-is, and float-only languages can emulate integers. Languages without numeric types would already be unable to support LSP given number is required in many placed. As you support a complete specification, I think we can agree on this much being good and useful.

On the other hand, specifying integer widths is much more controversial and is not something many higher level languages have control over. I understand there exist standards (e.g., 2^53 comes from IEEE 754), but I don't presume to know that every potential implementation language follows this standard.

[puremourning]

I think we can agree on this much being good and useful.

no doubt! 👍

On the other hand, specifying integer widths is much more controversial

Yeah, sorry i didn't mean to imply that we should specify integers in bits, but that the _required_ range of integers should be specified. e..g. A conforming implementation _must_ support integers in the range -2^31 to 2^31-1 or something like that. What i object to is the idea that we let server and client vendors "pick whatever they think is sane". That's cthulhu.

[sam-mccall]

Sorry if the size thing is a distraction - solving only float vs int is still progress.

They are closely related though. Just like sending "7.5" as your document version is legal today but likely to lead to interop problems, so is "1180591620717411303424" (2^70). ±2^53 seems like the natural limit (per RFC7159, and the spec's general tendency towards JS/TS) and should be implementable everywhere. But any other (smaller) limit is obviously fine.

[KamasamaK]

This could be made more precise using a JSON schema (#67) as the related Debug Adapter Protocol does instead of being limited to the TypeScript basic types. This allows them to use "type": "integer" where appropriate, although the human-readable spec is still translated to TypeScript number. Regarding ranges, the DAP uses the sentence "The value should be less than or equal to 2147483647 (2^31 - 1)." to indicate this where applicable so I don't think it's unreasonable to expect LSP to be able to indicate this as well.

[dbaeumer]

I think adding the same sentence to LSP makes sense (e.g. defining number to be in the int32 range).

We could also do the following in the type definitions:

type int32 = number;
type uint32 = number;

and then use int32 and uint32 in the spec d.ts to make this clear. Opinions?

[sam-mccall]

Opinions?

Sounds great.

If you're on the fence, I'd lean towards signed int32 everywhere (even when semantically nonnegative) for simplicity and as a concession to Java's int type.

[Aerijo]

It seems fine, especially if there's precedence with DAP, and it's a lot more useful than the current situation. Would the change be visible in the specification document? I checked the DAP equivalent and the schema uses integer but the document that one would actually be reading only says number. If this was already the case with the LSP I would not have seen it.

I was also thinking over the benefits of fixing the value range. I guess implementations can take the fixed size in the spec to mean they don't need to validate the sizes, which may help with JSON libraries that silently round numbers that are too large (because all numbers must, by the spec, be in range in the first place). What should a sender do then if it needs to send a number outside of the range? Would detecting this need to be added to the reference implementation, or is the scenario just considered to be unsupported in the first place?

[dbaeumer]

I would opt to do the change in the spec and the d.ts file.

[dbaeumer]

OK. I did a pass over the spec. We have a one case were we allow decimals in the range of [0-1]. Do you think we need to say anything about the precision. I actually would leave it open.

[dbaeumer]

One other questsion: the precentage for work done reporting is currently defined as a value between [0-100]. I can either allow decimals or only integers. I think it is good enough to only allow integers. Any objection?

[dbaeumer]

Did all the edits. Please cast your votes for decimal and precentage

[dbaeumer]

Closing. Left percentage as a uinteger in the range [0, 100]

[KamasamaK]

@dbaeumer The ProgressToken type is still using number. Is this intentional?

[sam-mccall]

Sorry, missed this. We'd have a preference for work done being floating point, as stable clangd 10 + 11 releases don't round the percentages.

Happy to change the behavior going forward (next release is in march or so), but those releases will still be around for a couple of years, probably.

[dbaeumer]

@KamasamaK No, fixed it.