Skip to content

Conversation

@david-driscoll
Copy link
Member

This adds the current proposals for the next version of the language server protocol.

I'm introducing these with [Obsolete] attribute to indicate that they are preview and they may change in the future. When 3.16 comes out we can simply remove the attributes and move the namespaces.

I got this working with a nice disco look in regards to semantic tokens 😄
image

@bjorkstromm
Copy link
Member

I'm introducing these with [Obsolete] attribute to indicate that they are preview and they may change in the future. When 3.16 comes out we can simply remove the attributes and move the namespaces.

If there only was a [Preview] attribute with a analyzer that warns that these are preview 😀 Obsole feels wrong in this case.

…at the supported set can be defined by the client
… like enums but can be extended with other values based on clients
@david-driscoll
Copy link
Member Author

@mholo65 maybe we should make one! Until then for now it'll at least point out that these bits may change or many not work without extra work.

@codecov
Copy link

codecov bot commented May 2, 2020

Codecov Report

Merging #230 into master will increase coverage by 0.65%.
The diff coverage is 64.20%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #230 +/- ## ========================================== + Coverage 43.94% 44.60% +0.65%  ========================================== Files 361 360 -1 Lines 8060 8280 +220 Branches 987 1023 +36 ========================================== + Hits 3542 3693 +151  - Misses 4226 4286 +60  - Partials 292 301 +9 
Impacted Files Coverage Δ
...c/Client/Clients/TextDocumentClient.Completions.cs 100.00% <ø> (+22.22%) ⬆️
...rc/Client/Clients/TextDocumentClient.Definition.cs 100.00% <ø> (+22.22%) ⬆️
...t/Clients/TextDocumentClient.DocumentHighlights.cs 100.00% <ø> (+22.22%) ⬆️
...Client/Clients/TextDocumentClient.FoldingRanges.cs 100.00% <ø> (+14.28%) ⬆️
...Client/Clients/TextDocumentClient.SignatureHelp.cs 100.00% <ø> (+22.22%) ⬆️
src/Client/Clients/TextDocumentClient.Sync.cs 0.00% <0.00%> (ø)
src/Client/Clients/TextDocumentClient.cs 76.00% <ø> (ø)
src/JsonRpc/JsonRpcServer.cs 0.00% <0.00%> (ø)
src/JsonRpc/JsonRpcServerExtensions.cs 54.54% <0.00%> (-12.13%) ⬇️
src/JsonRpc/JsonRpcServerOptions.cs 0.00% <ø> (ø)
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91f7859...aaf6347. Read the comment docs.


/// <summary>
/// The client supports tags on `SymbolInformation`.Tags are supported on
/// `DocumentSymbol` if `hierarchicalDocumentSymbolSupport` is set tot true.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// `DocumentSymbol` if `hierarchicalDocumentSymbolSupport` is set tot true.
/// `DocumentSymbol` if `hierarchicalDocumentSymbolSupport` is set to true.
return builder.Commit().GetSemanticTokensEdits();
}

// TODO: Is this correct?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that was from before I had added the helpers for tokenization. At this point I think it's good based on my testing, and if it isn't I made it virtual for a reason 😄

/// <summary>
/// Helper methods for working with LSP document URIs.
/// </summary>
public static class DocumentUri
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting class. Would it handle VS Code's conflicting Uri type (aka the fact that C: is encoded to C%3A)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should add that here with some unit tests to make sure.

This was brought up from the client I think.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In those tests, can you also include tests of Chinese characters or Cyrillic or something? Those don't always play nicely with the Uri type...

/// Character offset on a line in a document (zero-based).
/// </summary>
public long Character { get; set; }
public int Character { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason for changing all the longs to ints?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it easier to deal with the math when generating semantic tokens, otherwise a lot of potential conversions can happen. I can try and roll it back if it is needed for something. I dunno how many files have 2GB characters on a single line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PowerShell uses ints so I'll be happy to remove a bunch of (int)s all over the place :)

@TylerLeonhardt
Copy link
Collaborator

I assume that this should "just work" with language servers that only support 3.15 handlers and also clients that only support 3.15-styled messages.

Co-authored-by: Tyler James Leonhardt <tylerl0706@gmail.com>
@david-driscoll
Copy link
Member Author

Yeah, the goal here is existing servers will work, and if implement the new features and run into an editor that also implements the new features, things will "just work".

{
_outputHandler.LogTrace(e, "Could not write to output handler, perhaps serialization failed?");
_outputIsFinished.TrySetException(e);
// _thread.Abort(e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented out code?

<PackageDescription>Primitives for working with JsonRpc. This library is used as the base for communication with language servers</PackageDescription>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="System.Collections.Immutable" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious what this is good for over IReadOnly* types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I grabbed it for ImmutableArray mainly because it "does the right thing" when it comes to creating new and transformed arrays.

I used this so I don't have to think too hard about copying arrays around. This is because SemanticTokensDocument keeps a copy of the previous tokens, and the newly computed tokens, so that it can handle computing the diff between the two.

I was porting the builder from https://github.com/microsoft/vscode-languageserver-node/blob/master/server/src/semanticTokens.proposed.ts#L45 😄

@david-driscoll david-driscoll merged commit 79b1056 into master May 3, 2020
@david-driscoll david-driscoll deleted the feature/proposed branch May 3, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants