Added constraints to ParameterInformation.label break possible use cases

[oblitum]

Beyond adding support for inclusive start and exclusive end offsets to spec, commit 3854f1c added additional constraints to ParameterInformation.label when in string form, which can preclude neat functionality for users:

Note: A label of type string must be a substring of its containing signature label.

For example, before, compliant client and servers could take advantage of not having such limitation to provide terser prototype form on SignatureInformation.label, while providing full parameter types on each ParameterInformation.label. Types can be rather long, so having full form just for active parameter that clients can pick from parameters, while having short form of the rest (from SignatureInformation.label), can provide ideal UX and better language support (see Go example bellow).

Beyond that, the current addition require clients to inform the capability of reading labels as offsets (otherwise it could potentially break clients), while the following doesn't need it.

Proposal

To solve this, while still have the added constraints available, I think the best protocol would not change ParameterInformation.label from string to string | [number, number] (making a label work as label or as highlighting), but instead add a specific field to convey a highlight over SignatureInformation.label:

label: string,                 // unconstrained label
highlight?: [number, number],  // "highlight" over SignatureInformation.label

This keep intentions for fields (ParameterInformation) and sub-fields (label) clear, while nor the previous or the current state is clear at all. And also gives room for the potential use-cases referred in this thread, which could in fact be in use already.

In this proposal label just means to be a textual label, as it could have always been interpreted before. It is not to mean label whose job is highlighting, but when in string form (which is the usual form of labels) is unable to do that job correctly (1. there could be multiple matches, int matches in 3 places in the int foo(int, int) signature; 2. it's misnamed "label").

highlight is a proper highlight (correctly named), and not an option to string, so that it can do its job correctly as a properly indexed [number, number] sub-string view, and never fail at that (no multiple matches).

This is nice because beyond adding an always correct highlighting, the text for label is free from being identical to the highlight, to keep doing its job as a simple label on its own. It can't also break clients. There's no need to announce capability.

Better language support

For example, with this proposal we could have for Go a foo(a, b: int) -> int signature label, with non matching a: int and b: int parameter labels (or simply int for each), with highlight on a or b in the signature. It's impossible to do that with the current state.

[oblitum]

[dbaeumer]

@oblitum I see your point however what would the client have done with that information. Can you give me an example about what a client rendered in the case you described. A client is not able to reconstruct a signature from the parameter info since it usually doesn't know what parameter separators to use.

[oblitum]

@dbaeumer I've given an example in the image above. With both offsets over short signatures and detailed parameters, clients can do that.

 U<X<A, B>, Y<C, D>>               // Current parameter in full form
   foo(t, ▁                        // User typing
foo(T<…>, *U<…>*, V<…>) → W<…>     // Cleaned up signature

Alternatively:

   foo(t, ▁
foo(T<…>, *U<X<A, B>, Y<C, D>>*, V<…>) → W<…>

These are just a few use cases, I imagine detailed parameters being used in other ways.

[oblitum]

This gif shows how signatures can be rather noisy and how the above gives room to fix this:

[dbaeumer]

I am still unsure I understand this since I am not sure what you propose a client should render. Do you propose that clients show both the short and the long form?

The current protocol already allows showing detailed information for the active parameter since clients are supposed to call signature help after each key stroke when in this mode. So the label of the SignatureInformation can change with every key stroke. So in your example when on the second parameter the label can be:

foo(T<…>, U<X<A, B>, Y<C, D>>, V<…>) → W<…>

The information in the ParameterInformation is only used to help the client highlight certain things in the SignatureInformation label. In the above case the U<X<A, B>, Y<C, D>> part in the signature label.

[oblitum]

I am still unsure I understand this since I am not sure what you propose a client should render. Do you propose that clients show both the short and the long form?

Yes and No. On the examples above the client will have opportunity to chose to show both, and this can be at user's option, if it wishes to see detailed parameter info (this due to detailed per parameter info), client is able to provide a setting for that, and show two menus or one with composed result or some other idea, otherwise user could simple be content with the short prototype with (short) current parameter highlighted (this due to the offset information).

The current protocol already allows showing detailed information for the active parameter since clients are supposed to call signature help after each key stroke when in this mode. So the label of the SignatureInformation can change with every key stroke. So in your example when on the second parameter the label can be:

foo(T<…>, U<X<A, B>, Y<C, D>>, V<…>) → W<…>

Yes, but then this form will have to be hardcoded server side and it is unable to provide the former suggestion:

 U<X<A, B>, Y<C, D>>               // Current parameter in full form
   foo(t, ▁                        // User typing
foo(T<…>, *U<…>*, V<…>) → W<…>     // Cleaned up signature with short current parameter highlighted

The information in the ParameterInformation is only used to help the client highlight certain things in the SignatureInformation label. In the above case the U<X<A, B>, Y<C, D>> part in the signature label.

The proposed idea is to remove the new constraint that ParameterInformation.label can't provide more detailed type than what comes in the signature.

Formerly it was possible to do this, because it was just an unconstrained string without any implied relation to highlighting over the signature. It was just ParameterInformation, now it's becoming just ParameterHighlighting.

[dbaeumer]

Point taken. However I would like to see clients using this information (or at least having a UI for this) before we start adding this to the protocol. The original intention of the label of the ParameterInformation was not that it could be rendered start alone in the UI.

[oblitum]

@dbaeumer you mean non disclosed intention? Serious question because 3854f1c is the first time I've seen that declared. Having types shown alone has never been an strange idea for me:

• https://www.youtube.com/watch?v=I9NCiAVc0-0
• https://s3.amazonaws.com/f.cl.ly/items/1e2F0A123h331c1G0L0R/SadBart.gif

[dbaeumer]

I am not saying it is a strange idea and I am not against such a feature. All I am saying is that the field was used different by clients so far and we tried to specify this better.

This is why I marked it as a feature request after understanding your idea.

[oblitum]

I am not saying it is a strange idea and I am not against such a feature. All I am saying is that the field was used different by clients so far and we tried to specify this better.

My intention with the question was not to mean you viewed it as strange, just that it's simply not a strange usage, and that given that I don't see how a specific usage have been implied before, unknown client implementations could be already doing it. Hence I asked as "serious question" whether specific application has been disclosed elsewhere before, or whether it was just an internal Microsoft view that has never been disclosed until now.

[oblitum]

This is why I marked it as a feature request after understanding your idea.

In this view, it's not solely feature request, since it's removing something that has never been disallowed. So much so that the original string ParameterInfo.label application seemed to fit better for displaying parameter type than highlighting. At int foo(int, int, int), all ParameterInfo.label are just int, so that it would imply it was to be viewed as substring of signature for highlighting (which parameter?), instead of just parameter information, seems a stretch.

[dbaeumer]

The LSP spec is not perfect and has holes. If we detect an under specified part we usually tighten the spec even if it breaks some unintended use cases. However we are open to add these use cases back if they do make sense in the intention of the spec.

Usually a signature help info include the argument names. In your example foo(x: int, y: int). The parameter info label was then x: int and y:int which works in a lot of cases but not in all. This is why we added the [number, number] option and specified what the original intend to the label was.

I changed the sentence to this to make this more clear:

Note: a label of type string should be a substring of its containing signature label. Its intended use case is to highlight the parameter label part in the SignatureInformation.label.

[oblitum]

OK. I don't mean to offend, but this change breaks the previous possible user interpretations of this field badly, in my opinion it has ended up misnamed now, ParameterInformation conveyed more generality than the actual niche application, which could simply be named ParameterHighlighting.

[dbaeumer]

I am open for renaming it when we add additional use cases for this.

As said earlier if the spec is underspecified we usually try to tighten it not to add additional unintended use cases. The current change is to clarify this. Do you have a better wording to do so?

[oblitum]

Better wording for what ParameterInformation is able to convey now is ParameterHighlighting, because it's the sole job it can do, even when it comes as string (with documentation? meaning a documentation about what got highlighted). The intention of the label as a string would be completely clear, it's just a highlighting over the SignatureInformation.label and nothing else. Such change is more dramatic, and I doubt it would be done, but correctly fits the actual intention that's being applied. Another approach would be to rename ParameterInformation.label as ParameterInformation.highlight, for the same reason. All this more dramatic than what this proposal tries to do, which is instead to have room for the previous possible applications while still serving the niche intention without any drawback.

[oblitum]

Actually, you made realize of the label name itself. Better than my initial proposal would be to leave label as originally unconstrained string, and add highlight as optional offset range OR constrained string. This would make stuff completely clarified and cover all cases.

[oblitum]

FWIW, I'm modifying the proposal for that, just to register it correctly.

[dbaeumer]

This is a breaking change which I usually try to avoid. My current goal is to clarify the current intended behavior around signature help requests and to support cases were the parameter label is not unique without breaking any existing clients or servers (this is why I changed must to should :-)).

If we want to support other use cases we need to think about whether we can fit it into the current request or whether we need to think about a new request clients and servers can opt into.

[dbaeumer]

Ignore my last comment. I misread your latest proposal which is not breaking.

However my primary goal is still to clarifying the current intended behavior.

[oblitum]

However my primary goal is still to clarifying the current intended behavior.

That's the issue, the (now rewritten) proposal is clearer at that than what has been done. "label" generally conveys a label, but now, in this specific place it's meant to mean a range over another piece of text. Change while it's early! :)

[oblitum]

FWIW, I can see it from the frame of mind of hierarchical label (signature) and (sub)labels (parameter), as it seems is what is desired to attain (albeit limiting). It was just not my frame of mind at first, and probably isn't for many other developers too, given the generic names employed, label, not sublabel, highlight, etc.

[dbaeumer]

I totally agree naming this label was a bad choice and highlight would have been the better name. But if not absolutely necessary we try to clarify things and not change intention or to break.

I am absolutely open to extend this to allow unconstrained labels that can be presented in the UI as well.

[oblitum]

I could suggest fullLabel in addition to the label that acts as a range, but it sounds very convoluted, so despite attaining the functionality here discussed at length, such addition doesn't fit a protocol in early design, I'd rather break the few clients using the label as [number, number] that was added 7 days back than causing it to look worse.

[dbaeumer]

Even if we split this up the label would be specified to be a sub label to highlight a portion in the signature label. To make it clearer that its intended use case is being such a highlight I thought it is better to have one property instead of two.

[oblitum]

I think it's revolving back to original problems. Anyway, thanks for hearing my thoughts, ¯\_(ツ)_/¯.

[oblitum]

Added Go language example.

[MaskRay]

@oblitum's proposal looks good to me. highlight can specify the range of the simplified label while label specifies the elaborated one. There can be a user preference item to prefer one over the other and the two views can be toggled with some shortcut.

label: string,                 // unconstrained label
highlight?: [number, number],  // "highlight" over SignatureInformation.label

I'd rather break the few clients using the label as [number, number] that was added 7 days back than causing it to look worse.

As a server and client developer who has also watched many other implementations, I'd say yes. Breaking sooner than later just causes less pain. Really few people have noticed the Dec 13 change given that there is still a typo describing the newly added textDocument/declaration.

Add support for textDocument/definition request.

label: string | [number, number];

Another thing worth clarifying is do the offsets specify Unicode code points, or UTF-16 code units? This may hardly matter in practice, though. (I hope it cares APL, C++, Haskell and some others)

backlog

As a non-native English speaker this intimidates me :( I hope this issue can be addressed soon. The vey few client-side developers who have observed this feature are still sitting on the fence.

[dbaeumer]

@MaskRay thanks for pointing out the wrong mentioning in the change log. I fixed that.

Just to clarify my thinking here:

• I am not against having this feature. Not at all. This is why it is still open :-)
• my first priority is to clarify the existing indented behavior before adding new once.
• if reasonable / possible I try to not break clients nor servers.

If we add an un-constraint label and no client does something with it then we don't win anything. So my proposal would be to have a client capability unconstraint on the parameterInformation capability. If set servers know that they can set an unconstraint label and the the client does something meaningful with it. If not set the client will not present it.

Having these capability flags is how we moved the protocol forward so far and I would like to keep it that way.

I will clarify the offset thing. It is the same definition as with all positions and ranges. Thanks for pointing this out.

[oblitum]

If we add an un-constraint label and no client does something with it then we don't win anything

I don't understand this since I state in the proposal:

In this proposal label just means to be a textual label, as it could have always been interpreted before.

3854f1c is the first place I see the constraint over the label show up in protocol. In my view unknown client is being broken with that change already. Which I'm begging to be reverted for something that's actually non-breaking.

[oblitum]

I'll declare what for me this looks like. The protocol was general on that (as stated repeatedly, the word "label" and the type string is not in favor of the field to be constrained/viewed as highlighting), Microsoft started to have that interpretation in their tools, then thought to add it without community feedback. The lack of RFC process for a protocol makes me very sad about language server protocol.

[oblitum]

As I've demonstrated previously, I'm here more for fixing than adding a feature I wish. If it's to insert unconstraint into the already sorry state, I'm not in favor, my opinion is to just leave as it is, other use-cases can go look for another protocol.

[dbaeumer]

@oblitum I can see why you think that way but this is not what is happening. The LSP protocol started as a VS Code only protocol and then the community saw the usefulness of it and ask us to move it to a more public space. Due to this some parts are under specified or with the knowledge of today would have been better specified differently.

This being said the first goal if things are under specified is to make clear what the original intention was. Reason is that we want to make sure new clients implement this behavior and servers can rely on a certain behavior. In this specific use case it is about being able to highlight the active parameter in the signature. This got clarified by the comment to make sure clients and server understand the original intend. Since a string is a bad idea and is not sufficient we added the [number, number] however it is control by a flag. A server is not allowed to use [number, number] unless a client allows it. Based on your comments (to not break clients that use the label differently) I made the containment a should and not a must (2 days ago commit: 335dd85) but made it even clearer what the intention is.

I used this approach for all specification clarification so far and as said I try to balance the different aspects as good as I can. Believe me if I could start this in a green field I would do it differently.

[oblitum]

This being said the first goal if things are under specified is to make clear what the original intention was.

This is statement alone give me chills. It translates to me as: unknown changes can be added anytime (we never know when a new under specified thing with unknown original intentions will come up), outsiders can't do nothing against that.

[oblitum]

Another thing worth clarifying is do the offsets specify Unicode code points,
or UTF-16 code units? This may hardly matter in practice, though. (I hope it
cares APL, C++, Haskell and some others)

Beyond the Unicode and Emoji those programming languages allow, couldn't LSP be minimally usable for natural languages too? Does it have to make life difficult for clients/servers that wish to provide not only programming language completion/highlighting but also simple textual completion/highlighting unified in a single protocol without have to battle with LSP mix of UTF-16 offsets and UTF-8? Going further, what about syntax highlighting, what about code comments in Japanese (javadoc, markdown, ...)?

@MaskRay thanks for your input.

related #376

This is a bit off-topic theme in this issue, so, just a complement.