Update LSP Protocol Types #73911

mhutch · 2024-06-09T18:06:28Z

Updates all LSP Protocol types to match the current 3.17 spec. Improves documentation, fixes some minor issues, and adds lots of missing types, properties and methods.

I submoduled Roslyn in the MSBuild Editor repo so I would have definitions to use with CLaSP. After running into some missing things and having to replace some of the definitions locally, I figured the best solution for long-term maintenance would be to update all the definitions to fully cover the spec and contribute them back.

It would be great if these definitions could be included in the CLaSP source package but let's have that discussion separately.

Because of the large amount of changes, I have broken this up into smaller atomic commits structured so that they can be reviewed independently.

I have made a few minor formatting changes and moved a couple files to subdirectories where it made sense together with other changes I was making. These are some bigger changes I'd like to make, such as moving completion types to a Completion directory, switching existing code to file scoped namespaces, and changing properties to required and init, but I'll do those in later PRs if they're acceptable.

Marking as draft as I have not yet checked whether I need to update other Roslyn code to track the handful of API changes I made.

dibarbet

These are some bigger changes I'd like to make, such as moving completion types to a Completion directory, switching existing code to file scoped namespaces, and changing properties to required and init, but I'll do those in later PRs if they're acceptable.

I don't remember - is switching to required a binary breaking change? I think switching to init only would be, so we'd have to be careful about that one.

Otherwise, looks good, some minor comments here and there, but overall great! Thanks for the changes!!

dibarbet · 2024-06-10T21:59:46Z

src/LanguageServer/Protocol/Protocol/IPartialResultParams.cs

-            set;
-        }
+        [JsonPropertyName(Methods.PartialResultTokenName)]
+        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]


Is there a reason to put these on the interface definitions? I might be wrong but I don't think the attributes are inherited by the implementations

The main reason I did this was to make it easy to copy/paste when implementing the interface 😄

dibarbet · 2024-06-10T22:04:39Z

...LanguageServer/Protocol/Protocol/Internal/Diagnostics/VSInternalDocumentDiagnosticsParams.cs

        [JsonPropertyName(Methods.WorkDoneTokenName)]
        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
-        public IProgress<VSInternalDiagnosticReport[]>? WorkDoneToken { get; set; }
+        public IProgress<WorkDoneProgress> WorkDoneToken { get; set; }


Interesting - I don't know if we even use the WorkDoneProgress type here. I think it only uses partial result version. Not opposed to fixing this though

I'm pretty sure the previous WorkDoneProgress impl was plain wrong, so I'd be surprised if it was being used successfully

dibarbet · 2024-06-10T22:07:15Z

src/LanguageServer/Protocol/Protocol/WorkDoneProgress.cs

+[JsonDerivedType(typeof(WorkDoneProgressBegin), "begin")]
+[JsonDerivedType(typeof(WorkDoneProgressReport), "report")]
+[JsonDerivedType(typeof(WorkDoneProgressEnd), "end")]
+[JsonPolymorphic(TypeDiscriminatorPropertyName = "kind")]


didn't know you could do this. interesting!

Neither did I before this, so I hope it works 😄

dibarbet · 2024-06-10T22:13:36Z

src/LanguageServer/Protocol/Protocol/WorkDoneProgressBegin.cs

+    /// Title of the progress operation. Used to briefly inform about the kind of operation being performed.
+    /// Examples: "Indexing" or "Linking dependencies".
+    /// </summary>
+    [JsonPropertyName("title")]


I suspect we're not super consistent here, but ideally we'd mark new non-nullable properties with JsonRequired as well

Yup I added a bunch of missing JsonRequired to existing members too but I may have missed a few

dibarbet · 2024-06-10T22:18:09Z

src/LanguageServer/Protocol/Protocol/GeneralClientCapabilities.cs

+    /// </summary>
+    [JsonPropertyName("markdown")]
+    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+    public MarkdownClientCapabilities Markdown { get; init; }


Should be nullable I think

dibarbet · 2024-06-10T23:57:02Z

src/LanguageServer/Protocol/Protocol/FileOperations/DidChangeWorkspaceFoldersParams.cs

+    /// The actual workspace folder change event.
+    /// </summary>
+    [JsonPropertyName("event")]
+    public WorkspaceFoldersChangeEvent Event { get; init; }


jsonrequired

dibarbet · 2024-06-10T23:57:16Z

src/LanguageServer/Protocol/Protocol/FileOperations/FileOperationFilter.cs

+    /// A Uri like `file` or `untitled`.
+    /// </summary>
+    [JsonPropertyName("scheme")]
+    public string? Scheme { get; init; }


dibarbet · 2024-06-10T23:57:53Z

src/LanguageServer/Protocol/Protocol/FileOperations/FileOperationFilter.cs

+    /// The actual file operation pattern.
+    /// </summary>
+    [JsonPropertyName("pattern")]
+    public FileOperationPattern Pattern { get; init; }


jsonrequired

dibarbet · 2024-06-10T23:58:56Z

src/LanguageServer/Protocol/Protocol/FileOperations/FileOperationPattern.cs

+    /// </list>
+    /// </summary>
+    [JsonPropertyName("glob")]
+    public string Glob { get; init; }


jsonrequired

dibarbet · 2024-06-10T23:59:09Z

src/LanguageServer/Protocol/Protocol/FileOperations/FileOperationRegistrationOptions.cs

+    /// The actual filters.
+    /// </summary>
+    [JsonPropertyName("filters")]
+    public FileOperationFilter[] Filters { get; init; }


jsonrequired

This makes it easier to update them

azure-pipelines · 2024-08-15T22:49:39Z

Azure Pipelines successfully started running 3 pipeline(s).

dibarbet

I think I'm good with this (as long as CI passes). One small comment. I didn't go through every file with a fine toothed comb though.

dibarbet · 2024-08-15T22:55:14Z

src/LanguageServer/Protocol/Handler/References/FindAllReferencesHandler.cs

@@ -22,7 +22,7 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler
 {
    [ExportCSharpVisualBasicStatelessLspService(typeof(FindAllReferencesHandler)), Shared]
    [Method(LSP.Methods.TextDocumentReferencesName)]
-    internal sealed class FindAllReferencesHandler : ILspServiceDocumentRequestHandler<LSP.ReferenceParams, LSP.SumType<LSP.VSInternalReferenceItem, LSP.Location>[]?>
+    internal sealed class FindAllReferencesHandler : ILspServiceDocumentRequestHandler<VSInternalReferenceParams, LSP.SumType<VSInternalReferenceItem, LSP.Location>[]?>


I think I'd rather leave the handler definitions as the base type where possible

System.Text.Json doesn't respect DefaultValueAttribute

mhutch · 2024-08-20T03:34:09Z

@dibarbet i finally got it green!

Fixes integration test failures in Find All References. Roslyns LSP types got much more spec compliant in dotnet/roslyn#73911 and we were never sending the `Context` property in our request, so deserialization failed on their end.

dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 9, 2024

dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 9, 2024

dibarbet reviewed Jun 11, 2024

View reviewed changes

mhutch force-pushed the lsp-protocol-types branch from a090e25 to 85029e6 Compare June 17, 2024 02:03

mhutch added 25 commits June 16, 2024 23:07

LSP Protocol: Implement WorkDoneProgress types

15025cf

LSP Protocol: Use spec ordering in capabilities types

b3c3459

This makes it easier to update them

LSP Protocol: Add window/general client capabilities

d1fdfed

LSP Protocol: Update lifecycle messages

4a62b71

LSP Protocol: Update document sync messages

6bf6885

LSP Protocol: Add notebook types/methods

eccb029

LSP Protocol: Update navigation methods/types

ceafc11

LSP Protocol: Update documentLink & documentHighlight

6dceb2e

LSP Protocol: Update Hover & CodeLens

71dfa3c

LSP Protocol: Update FoldingRange, add SelectionRange

1e09482

LSP Protocol: Update DocumentSymbol classes

11cf351

LSP Protocol: Update semantic token types

a320492

LSP Protocol: Update Inlay Hint types

5c451b5

LSP Protocol: Add Inline Value types/methods

71d1108

LSP Protocol: Add Moniker types

f13f458

LSP Protocol: Update completion types

a466e02

LSP Protocol: Update Diagnostic types/methods

ccd8d17

LSP Protocol: Update Signature Help types

e611615

LSP Protocol: Update CodeAction types

d489f14

LSP Protocol: Update DocumentColor types/methods

6393717

LSP Protocol: Update formatting types/methods

7b7b3f8

LSP Protocol: Update rename types/methods

8a46f2c

LSP Protocol: Update TextDocumentEdit/WorkspaceEdit

c5a1e56

LSP Protocol: Update Linked Editing Range types

edd5648

LSP Protocol: Update workspace symbol types

b9b589b

mhutch force-pushed the lsp-protocol-types branch 3 times, most recently from 70ede42 to 8503877 Compare August 15, 2024 16:30

mhutch added 2 commits August 15, 2024 12:40

LSP Protocol: fix FormattingOptions.OtherOptions serialization

f165938

LSP Protocol: Fix some serialization errors

3adb76d

mhutch force-pushed the lsp-protocol-types branch from 8503877 to 3adb76d Compare August 15, 2024 16:40

dibarbet approved these changes Aug 15, 2024

View reviewed changes

build-analysis bot mentioned this pull request Aug 16, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

mhutch added 3 commits August 15, 2024 22:45

Merge remote-tracking branch 'origin' into lsp-protocol-types

9829e8a

LSP Protocol: fix more CI failures

1b59c82

LSP Protocol: Fix another couple serialization errors

1f80181

System.Text.Json doesn't respect DefaultValueAttribute

mhutch force-pushed the lsp-protocol-types branch from e83696f to 1f80181 Compare August 18, 2024 00:28

Merge remote-tracking branch 'origin' into lsp-protocol-types

2e3dc5f

mhutch force-pushed the lsp-protocol-types branch 2 times, most recently from c40f668 to 8b2ab62 Compare August 19, 2024 21:17

LSP Protocol: Support Uri in SumType

e89c402

mhutch force-pushed the lsp-protocol-types branch from 8b2ab62 to e89c402 Compare August 20, 2024 00:56

LSP Protocol: Fix another CI failure

3320e83

dibarbet approved these changes Aug 20, 2024

View reviewed changes

dibarbet merged commit 2543698 into dotnet:main Aug 20, 2024
25 checks passed

dotnet-policy-service bot added this to the Next milestone Aug 20, 2024

dibarbet modified the milestones: Next, 17.12 P2 Aug 26, 2024

This was referenced Aug 27, 2024

[Automated] PRs inserted in VS build main-35226.156 #74912

Closed

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35227.190 #74926

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35227.116 #74928

Closed

davidwengier mentioned this pull request Aug 28, 2024

Fix find all references calls to Roslyn dotnet/razor#10807

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update LSP Protocol Types #73911

Update LSP Protocol Types #73911

mhutch commented Jun 9, 2024

dibarbet left a comment

dibarbet Jun 10, 2024

mhutch Jun 11, 2024

dibarbet Jun 10, 2024

mhutch Jun 11, 2024

dibarbet Jun 10, 2024

mhutch Jun 11, 2024

dibarbet Jun 10, 2024

mhutch Jun 11, 2024

dibarbet Jun 10, 2024

dibarbet Jun 10, 2024

dibarbet Jun 10, 2024

dibarbet Jun 10, 2024

dibarbet Jun 10, 2024

dibarbet Jun 10, 2024

azure-pipelines bot commented Aug 15, 2024

dibarbet left a comment

dibarbet Aug 15, 2024

mhutch commented Aug 20, 2024

Update LSP Protocol Types #73911

Update LSP Protocol Types #73911

Conversation

mhutch commented Jun 9, 2024

dibarbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-pipelines bot commented Aug 15, 2024

dibarbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhutch commented Aug 20, 2024