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

Make specification more machine-readable #67

Closed
hediet opened this issue Sep 11, 2016 · 101 comments
Closed

Make specification more machine-readable #67

hediet opened this issue Sep 11, 2016 · 101 comments
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities
Milestone

Comments

@hediet
Copy link
Member

hediet commented Sep 11, 2016

It would be very handy if parts of the specification could be read by machines.
This would help developing SDKs for typed languages a lot, as many types can be auto generated.

Besides, there are some inconsistencies in non-normative parts of the specification that could be easily fixed (e.g. interfaces vs exported interfaces, params vs param).

Something like this would be awesome in addition to the existing specification:
(comments removed to keep it short)

interface RequestMessage<TMethod extends string, TParams> {
    id: number | string;
    method: TMethod;
    params: TParams
}

interface ResponseMessage<TResult, TError> {
    id: number | string;
    result?: TResult;
    error?: ResponseError<TError>;
}

interface Request<TRequest, TResponse> {}
interface Notification<TRequest> {}

// ...

type InitializeRequest = Request<
    RequestMessage<"initialize", InitializeParams>,
    ResponseMessage<InitializeResult, InitializeError>
>;

type ShutdownRequest = Request<
    RequestMessage<"shutdown", undefined>,
    ResponseMessage<undefined, undefined>
>;

type ShowMessageNotification = Notification<
    RequestMessage<"window/showMessage", ShowMessageParams>
>;

What do you think? I can offer to add these additional typescript definitions for all of the about 30 notifications / requests.

@gorkem
Copy link
Contributor

gorkem commented Sep 12, 2016

#25 may be related

@hediet
Copy link
Member Author

hediet commented Sep 12, 2016

The link mentioned in the first comment in #25 is not valid anymore and #40 only covers the types, not the actual protocol (methods, request/notification, expected response types and so on).

@hediet
Copy link
Member Author

hediet commented Sep 13, 2016

I basically was looking for protocol.ts.

@dbaeumer dbaeumer added this to the 3.0 milestone Sep 17, 2016
@dbaeumer dbaeumer added the feature-request Request for new features or functionality label Sep 17, 2016
@dbaeumer
Copy link
Member

Agree with @gorkem that a machine readable specification should be in JSON Schema since this is more universal. There are quite some tools out there to generate interface files from JSON for different languages.

@dbaeumer dbaeumer added the help wanted Issues identified as good community contribution opportunities label Sep 17, 2016
@felixfbecker
Copy link

JSON schema would be awesome, and also allow to be more specific than TS interfaces. For example, you can apply RegExp constraints to strings.

@vicencb
Copy link

vicencb commented Nov 5, 2017

An editor will need to have fast response times from a server to be interactive.
It looks to me that a text based protocol will hurt this interactivity. It is well known that text processing is much slower than well structured binary data.
So, why not use protocol buffers:
https://github.com/google/upb

With upb the backend can be chosen:

  1. use the fast protocol buffers in release mode
  2. use the human readable JSON format in debug mode

The protocol is defined with an schema and is fully machine-readable.

@dbaeumer
Copy link
Member

@vicencb the protocol buffer is about the transport. Since the message header allows to specify a content type we can send binary data if this is necessary due to performance.

This issue is about having the specification in a machine readable from.

@DanTup
Copy link
Contributor

DanTup commented Jan 14, 2019

Not sure if you want to collect examples of what's difficult to parse here in this issue, but I just came across another one and thought it worth noting.. The response to textDocument/prepareRename is documented as Range | { range: Range, placeholder: string } | null.

However this text only appears in the results: note against the request, there's no typescript codeblock with the definition. This means if you want to codegen a class for the range+placeholder, you need to regex it out, but the same line contains a bunch of english:

  • result: Range | { range: Range, placeholder: string } | null describing the range of the string to rename and optionally a placeholder text of the string content to be renamed. If null is returned then it is deemed that a 'textDocument/rename' request is not valid at the given position.

I really want to avoid too many special cases to make it easier to keep my classes up-to-date, but now I have a regex like \* result:[^\.]*({.*}) to try and extract this reliably. The rest of the spec outgrew my regexes (they got really complicated) and I ended up writing a small TypeScript parser in order to extract everything from here). I don't know how the other servers are doing it, but it feels like a bit of a burden :-(

@DanTup
Copy link
Contributor

DanTup commented Feb 26, 2019

Another inconsistency found while parsing:

Some notifications list their params as params: void and others use params: none. As far as I can tell, these mean the same, which is the params field can be omitted.

@dbaeumer
Copy link
Member

@DanTup when I started to convert this to JSON schema I used a TS compiler to parse the TS file from https://github.com/Microsoft/vscode-languageserver-node/blob/dbaeumer/464/protocol/src/protocol.ts#L1 which should give the best result.

@DanTup
Copy link
Contributor

DanTup commented Feb 26, 2019

@dbaeumer Thanks! That might've been a better thing to parse when I started, though I now have a fairly-complete parser for the spec and it's probably not worth changing. I was just noting inconsistencies I noticed in the hope the official spec might become friendlier for parsing.

Out of interest, you say started to convert to JSON schema - is that still in progress? Will it become an official schema? I don't want to mess with the parser too much now it (mostly) works, but switching from a TS-parser-written-in-Dart to something structured would definitely be nice.

@dbaeumer
Copy link
Member

@DanTup it is on hold right now due to other priorities. As always community help is appreciated here.

@metaleap
Copy link

metaleap commented Jun 20, 2019

Just another vote for JSON-Schema or some sort of protocol.json as core first-class part of the LSP project's spec efforts. I had previously for example utilized https://github.com/Microsoft/vscode-debugadapter-node/blob/master/debugProtocol.json to generate code scaffolds with great success, and was "almost sure something like LSP must have something equivalent --- by now" until just minutes ago. Looks like not-yet.. but should substantially ease the burden on contributors to the ecosystem (client and server implementations written in countless languages) especially when it comes to them keeping up with new versions (swiftly or at-all), supporting multiple versions, remaining motivated to not abandon their client or server implementation, or if that happens easier and swifter springing up of alternative implementations etc.

Alternatively the question could be whether the TypeScript project offers-and-supports (or whether it should) some sort of baseline / stable / maintained .d.ts-to-JSON converter that keeps up with TS' own movements. Then anyone could use that for both LSP protocols and whatever else floats around in .d.ts land.

@dbaeumer
Copy link
Member

I agree with this and when I last looked into this I was experimenting with converting the d.ts to JSON schema automatically. This would be a great opportunity for the community :-)

@timeraider4u
Copy link

Out of curiosity: Are there any good tools available for this task? Have you encountered any pitfalls with the automatic conversion so far?

A little bit off-topic: For the other way around I like using json-schema-to-typescript.

@felixfbecker
Copy link

Alternatively the question could be whether the TypeScript project offers-and-supports (or whether it should) some sort of baseline / stable / maintained .d.ts-to-JSON converter that keeps up with TS' own movements. Then anyone could use that for both LSP protocols and whatever else floats around in .d.ts land.

I've used https://github.com/YousefED/typescript-json-schema with great success for this. We could use that to generate the initial schema, manually optimize it, commit it and then delete the original TypeScript and start generating it from the JSON schema with https://github.com/bcherny/json-schema-to-typescript.

@bollwyvl
Copy link

I've some success to report with:

  • some light regex-based parsing of the markdown spec (now i have two problems)
  • generating some typescript to capture request/response pairs
  • running ts-json-schema-generator

Here's the whole gist (or binder, if you're into that sort of thing), and the schema as JSON (or YAML).

It hasn't been exhaustively tested, but by inspection appears to be fairly thorough. I'd love to see something like this made more official, but will probably keep tinkering with it.

👏 Thanks to @domoritz, for helping me get voids added to ts-json-schema-generator!

@KamasamaK
Copy link
Contributor

Would love to see JSON schema be the first-class spec as it is with DAP. The fact that the TS and JSON data types don't align perfectly came up in #833 (comment).

@bollwyvl
Copy link

Sounds good! Given there is some precedent with DAP, is the schema...

  • explicitly versioned?
  • used to generate the human-readable documentation?
  • used to generate the typings for the reference implementation?
  • published in any way other than digging through gh-pages?
  • used in an installable conformance suite?

As a downstream consumer of the spec, all of these properties would be desirable.

A canonical representation in YAML, TOML, or some other JSON-data-model-but-not-braces-and-escaped-newlines format with embedded markdown (or commonmark) could fill the bill, be linted/formatted, and remain pretty PR-able.

A schema can even include extra, validator-opaque metadata, like versionIntroduced, versionDeprecated, etc. currently buried in narrative, which can be used for documentation generation... and can in turn be validated by a meta-schema.

@KamasamaK
Copy link
Contributor

KamasamaK commented Oct 25, 2019

@bollwyvl DAP does generate the human-readable documentation specification.md using spec-generator.

@bollwyvl
Copy link

bollwyvl commented Oct 25, 2019 via email

@dbaeumer dbaeumer removed this from the 4.0 milestone Oct 30, 2019
@DanTup
Copy link
Contributor

DanTup commented May 10, 2022

serverNotInitialized is in ErrorCodes

Oh, I see. What's the difference between ErrorCodes and LSPErrorCodes? It looks like most of ErrorCodes are not LSP specific, but it also contains things like ServerNotInitialized which seems like an LSP code. Having two types makes my definition of the response class a little awkward because it could have either an ErrorCodes or LSPErrorCodes (I can find a way to handle this, but I wanted to check if it's necessary first).

There's also a bit of a mismatch in casing (for example ErrorCodes.serverErrorStart and ErrorCodes.ServerNotInitialized have different casing of s in server) - should they be the same? (if so, changing them before publishing will be much easier than when external code may be using the names).

@dbaeumer
Copy link
Member

ErrorCodes are basically JSONRPC error codes. At the beginning I put LSP error in the ErrorCodes even in a wrong number range. This got fixed with LSPErrorCodes but for backwards compatibility I kept ErrorCodes.

  • ErrorCodes.serverErrorStart: this is a range marker. Hence lower case. Not a real error code.
  • ErrorCodes.ServerNotInitialized: is a real error code. Hence upper case.

@dbaeumer
Copy link
Member

If you have problems in Dart with this I suggest that you create one enum instead of two. There is no need that your generated libraries follows this artificial split

@DanTup
Copy link
Contributor

DanTup commented May 10, 2022

ErrorCodes.serverErrorStart: this is a range marker

Ah, of course!

This got fixed with LSPErrorCodes but for backwards compatibility I kept ErrorCodes.

Ah, gotcha. In that case I'll just make ErrorCodes a base class for LSPErrorCodes and that way I can use ErrorCodes for ResponseError.code and should be able to assign either.

I haven't found any other issues yet, but I'm not quite done. I'll let you know if I do (or if I complete the migration and have no other issues). Thanks!

@dbaeumer
Copy link
Member

OK. I will publish a release today / tomorrow. Updating the meta model is something we can always do. None of the things you reported did actually need a change the specification itself. It was more generation bugs.

@dbaeumer
Copy link
Member

Add to 3.17. The meta model is also reachable via the specification site

@DanTup
Copy link
Contributor

DanTup commented May 10, 2022

@dbaeumer awesome, thanks! I'm hoping to finish the migration in the next day or so (I think I'm down to the last few issues, but I'm first migrating via the TS version because it'll be much easier to review switching from TS -> metaModel if it's not also the 3.16 -> 3.16 upgrade).

Just FYI in case you hadn't noticed:

@dbaeumer
Copy link
Member

Actually https://microsoft.github.io//language-server-protocol/specifications/specification-current is forwarded correctly. Let me see if I can do that same for specifications/specification-current/``

Will remove the upcoming

@dbaeumer
Copy link
Member

Done.

@KamasamaK
Copy link
Contributor

Why is there a double slash (//) between microsoft.github.io and language-server-protocol on redirect?

@dbaeumer
Copy link
Member

Don't know. Let me see if I can fix this.

@dbaeumer
Copy link
Member

I tried to fix this, but I was not able to do so. Interestingly it doesn't happen when executing jekyll locally. Does anyone has an idea?

@DanTup
Copy link
Contributor

DanTup commented May 11, 2022

@dbaeumer I think WorkDoneProgressBegin/WorkDoneProgressReport and friends may be missing from the model. I can't find "percentage" anywhere in this json but it's in the web version here.

Why is there a double slash (//) between microsoft.github.io and language-server-protocol on redirect?

I tried to fix this, but I was not able to do so. Interestingly it doesn't happen when executing jekyll locally. Does anyone has an idea?

Not sure, but could it be related to these two things being mashed together:

baseurl: /language-server-protocol # the subpath of your site, e.g. /blog/
url: https://microsoft.github.io/ # the base hostname & protocol for your site

In this page it says "Leave off trailing forward slashes when setting url". I don't know if you have anything relying on it that could break if you remove it though 😬

@dbaeumer
Copy link
Member

@DanTup thanks for the Jekyll tip. Applied and works locally.

@DanTup
Copy link
Contributor

DanTup commented May 12, 2022

@dbaeumer a few other minor things (let me know if here is not still the best place to report them):

  • Some documentation has been truncated:
    "documentation": "Capabilities specific to the `textDocument/documentColor`"
  • Some documentation has forced linebreaks for wrapping that are hard to tell from "good" linebreaks which results in bad wrapping (or us having to guess which newlines to strip/keep) if we need to wrap the docs to fit max widths (taking into account indentation). Seems like it would be better not to have these in the spec and let people do their own wrapping?
    Screenshot 2022-05-12 at 11 27 36
  • Missing WorkDoneProgressBegin etc. (noted above)

@dbaeumer
Copy link
Member

@DanTup
Copy link
Contributor

DanTup commented May 12, 2022

@dbaeumer oh, I see - where is that populated from? In the Markdown version of the spec (and the web version on GH Pages) it looks like this:

/**
	 * Capabilities specific to the `textDocument/documentColor` and the
	 * `textDocument/colorPresentation` request.
	 *
	 * @since 3.6.0
	 */
	colorProvider?: DocumentColorClientCapabilities;

@DanTup
Copy link
Contributor

DanTup commented May 12, 2022

  • There are quite a few typos in the new model - I can fix these up if you can tell me where is the source of truth (they appear in many places and I'm not sure which one is the source)
    Screenshot 2022-05-12 at 13 55 27
  • Enum values have lost their docs
    Screenshot 2022-05-12 at 14 05 53
  • FullDocumentDiagnosticReport.kind is a string literal Full (uppercase F) in the JSON, but export const Full = 'full'; in the web spec

@dbaeumer
Copy link
Member

@dbaeumer
Copy link
Member

The correct value is full. It is uppercase Full due to kind: typeof DocumentDiagnosticReportKind.Full which is not handled correctly

@DanTup
Copy link
Contributor

DanTup commented May 16, 2022

The truth of source are the TypeScript files defined via this TS project

I'm not sure I understand. In that repo, the docs for colorProvider?: DocumentColorClientCapabilities; are all truncated:

	/**
	 * Capabilities specific to the `textDocument/documentColor`
	 */
	colorProvider?: DocumentColorClientCapabilities;

But in the web version of the spec at https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/ they are complete:

/**
	 * Capabilities specific to the `textDocument/documentColor` and the
	 * `textDocument/colorPresentation` request.
	 *
	 * @since 3.6.0
	 */
	colorProvider?: DocumentColorClientCapabilities;

Is one of these generated from the other, or are they maintained separately?

Edit: The place I can find the correct text is at https://github.com/microsoft/language-server-protocol/blame/35065fc549a5a3e2183769f37aba98457c8cd490/_specifications/lsp/3.17/general/initialize.md#L190 but it's not clear to me where to fix it so the metamodel has all of that text.

@dbaeumer
Copy link
Member

The TS file in the vscode-languagserver-node repository and the specification published to the Web are currently maintained by hand :-(

So fixes for the meta model have to go into https://github.com/microsoft/vscode-languageserver-node

I do have plan to auto generate the specification from the meta model, however I am not there yet.

@DanTup
Copy link
Contributor

DanTup commented May 16, 2022

So before I start making changes - I can make fixes to the docs directly in these TS files like here:

https://github.com/microsoft/vscode-languageserver-node/blob/fd9bcbdb699d2573ee9c53e580c39be2a315ba11/protocol/src/common/protocol.colorProvider.ts#L65

And they won't be overwritten (and will be used for meta model, and eventually the website)?

(I just want to be sure I don't spend time fixing up something that will be overwritten from another source 😄)

@DanTup
Copy link
Contributor

DanTup commented May 16, 2022

@dbaeumer some other things I in my diff:

  • TraceValue has "compact" in meta model but not in published spec
  • NotebookDocumentSyncClientCapabilities has executionSummarySupport in meta model but not in published spec
  • PrepareRenameParams might be missing WorkDoneProgressParams as a base in published spec? (it's in meta model)
  • In meta model, InitializeParams extends WorkspaceFoldersInitializeParams which does not mark workspaceFolders as optional (only nullable), but in the TS spec, it's both optional (?) and nullable: workspaceFolders?: WorkspaceFolder[] | null;
  • Documentation is missing for quite a few things (it's in source TS, but missing in meta model):
    • CodeAction.disabled.reason ("Human readable description of why the code action is currently disabled.")
    • completionItem.commitCharactersSupport ("Client supports commit characters on a completion item.")
    • CompletionTriggerKind ("How a completion was triggered")
    • Many (all?) enums (eg.: CodeActionKind.Empty, CodeActionTriggerKind.Automatic)

I opened microsoft/vscode-languageserver-node#945 to address some of the typos and truncated docs (I don't believe it addresses any of the above though).

@dbaeumer
Copy link
Member

I created microsoft/vscode-languageserver-node#946 since the work actually has to happen in that repository.

@brettcannon
Copy link
Member

We just published https://pypi.org/project/lsprotocol/ . It's a Python package containing the various types and messages from LSP based on the programmatic definition. That also means we have code now to read the programmatic definition which could be used by others to generate other code representations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities
Projects
None yet
Development

No branches or pull requests