Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metamodel: should all enumerations "support custom values"? #1088

Closed
michaelpj opened this issue Sep 25, 2022 · 11 comments
Closed

metamodel: should all enumerations "support custom values"? #1088

michaelpj opened this issue Sep 25, 2022 · 11 comments

Comments

@michaelpj
Copy link
Contributor

michaelpj commented Sep 25, 2022

The existence of the supportsCustomValues property naturally suggests that enumerations which do not have this property should not support custom values. But the specification says:

To support the evolution of enumerations the using side of an enumeration shouldn’t fail on an enumeration value it doesn’t know. It should simply ignore it as a value it can use and try to do its best to preserve the value on round trips. Lets look at the CompletionItemKind enumeration as an example again: if in a future version of the specification an additional completion item kind with the value n gets added and announced by a client a (older) server not knowing about the value should not fail but simply ignore the value as a usable item kind.

This suggests that all enumerations must always accept unknown values. That seems to me to be essentially the same as saying that they should all have supportsCustomValues (which further suggests that the property should just be removed as it applies to all enumerations). Or is there some other meaning to "supports custom values" beyond this?

@dbaeumer
Copy link
Member

LSP makes a difference in being tolerant to unknown values to be backwards compatible. But these unknow values must appear in a newer version of the specification. Custom values are values a client and server can agree on during a handshake and don't need to be in the spec.

I will close the issue since this is intended.

@michaelpj
Copy link
Contributor Author

But then what does supportsCustomValues actually tell a consumer of the metamodel? Specifically that there's a handshake for agreeing to custom values? Maybe it would be clearer to have a customValueCapabilility property that links to the capability that tells you the custom values?

My point is just that at the moment if you're generating types to support enumerations, supportsCustomValues is irrelevant: you have to support values not listed in the spec in all cases.

@dbaeumer
Copy link
Member

Maybe it would be clearer to have a customValueCapabilility property that links to the capability that tells you the custom values?

The specification doesn't have these custom values so we can't link to them.

you have to support values not listed in the spec in all cases.

If you can guarantee that you are always on the latest version of the specification then technically you don't. Since you should never get values that are not in the specification.

@michaelpj
Copy link
Contributor Author

Since you should never get values that are not in the specification.

Well, my concrete problem is apparently a langauge server producing a bunch of completion item kinds that are definitely not in the spec: haskell/lsp#460

The specification doesn't have these custom values so we can't link to them.

Sorry, I mis-typed! I meant to say: link to the capability that defines the custom types. That then provides you with some useful information you could not have otherwise.


Anyway: I think the point still stands. Any types generated from the metamodel must allow for enumeration values which are not in the specification version that corresponds to its metamodel, so must accept arbitrary unknown values.

And I still believe that suportsCustomValues is currently useless. What do you think a user of the metamodel can do with it?

@dbaeumer
Copy link
Member

dbaeumer commented Sep 27, 2022

I would use it to generate a correct model to be for example for the server side. Take a completion item as an example. If kind would support custom values the expected code to generate would be:

namespace CompletionItem {
    create(kind: CompletionItemKind | number) {
       ....
    }
}

Since kind doesn't support custom values I generate:

namespace CompletionItem {
    create(kind: CompletionItemKind) {
       ....
    }
}

which makes it clear that you can't use arbitrary completion item kinds.


This is already the case for the types I define in TS. If a enumeration supports custom values the type definitions usually are or types with `| number` or `| string`

@michaelpj
Copy link
Contributor Author

Doesn't that violate the spec claim that you always have to support additional enumeration values that you may not know about? That applies on the client side too - the client might be on a newer LSP spec and send you a value you don't know about yet. So your "no custom values" version is I think wrong? That's what I mean - in practice you have to treat all the enumerations as open regardless.

@dbaeumer
Copy link
Member

But the sending side doesn't have to support custom values. So when a server creates a completion item it doesn't need to provide API to create a completion item with an unknown completion item kind. So the signature of the create function can be kind: CompletionItemKind and not kind: CompletionItemKind | number

On the receiving side you need to handle unknown values. That is correct. But even there the strategy is as follows:

  • for values that don't travel back to the server you can simple fold them to a valid default and use it. What the spec says is that you should not throw on those
  • for values that travel back fold an unknown value to a default value and remember the unknow value so that you can send it back. See
    function asCompletionItemKind(value: ls.CompletionItemKind): [code.CompletionItemKind, ls.CompletionItemKind | undefined] {

Hope that clarifies it.

@michaelpj
Copy link
Contributor Author

Okay, I see that you can do what you suggested. But it adds a lot of complexity: you now have

  • A "proper" CompletionItemKind type
  • A "lax" CompletionItemKind type
  • Functions to convert between them, using some policy to pick a default value (which AFAICT isn't in the spec)
  • In some cases, also remember the original value so you can pass it back

IMO it's far simpler to just use (something equivalent to) CompletionItemKind | number everywhere. Only one type, no conversions, no arbitrary default policies, no additional logic for remembering the original value. You have a few places where you could pass an unknown kind where you shouldn't, but overall it's far simpler.

Personally, I think it would simplify the spec to just explicitly say that all enumerations are open, and that it's just that a few allow negotiation of specific additional entries. This nuance doesn't seem to add much except confusion to me 🤷

@dbaeumer
Copy link
Member

There is the perspective of a library provider and a library user. I see your points from a provider perspective but not from a user perspective.

Functions to convert between them, using some policy to pick a default value (which AFAICT isn't in the spec)

This is up to the client to decide since it depends on the value set available in the client.

IMO it's far simpler to just use (something equivalent to) CompletionItemKind | number everywhere

Then you basically loose all typing information for enumerations on the API level.

@michaelpj
Copy link
Contributor Author

Then you basically loose all typing information for enumerations on the API level.

Well, in Haskell we'd do it like this:

data CodeActionKind = Option1 | ... | CodeActionKindUnknown Integer

so you can't use the open case accidentally, you have to deliberately do it. No typing lost IMO.

@dbaeumer
Copy link
Member

But this will not work in most other languages I know and making all enumerations to support custom values will at the end force lib maintainers to generate such code. If Haskell supports a type save way I have no problem if you treat all enumerations as if they support custom values. But for the other languages I think explicitly marking them adds value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants