Switch Roslyn protocol types to System.Text.Json serialization #73207

dibarbet · 2024-04-24T01:05:41Z

Switches the Roslyn server to use System.Text.Json serialization for all LSP messages.

I suggest reviewing one commit at a time

6046471 is almost entirely mechanical find/replace of DataContract/DataMember/other newtonsoft attributes with the STJ version inside the Protocol/Protocol folder. I also copied the new converters https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/530064 (adjusted for style warnings).
b292a51 is the meat of the change - switching streamjsonrpc to use STJ and implementing STJ support in the roslyn language server. It also covers modifying custom Roslyn protocol types to use STJ attributes.
1a4c3f4 - I apologize for this commit, its very unfortunate. Basically we have not opted the client side into STJ serialization, its still using newtonsoft (blocked on Razor transitioning to new APIs). This means that when we make document symbol LSP requests for document outline, we need to use newtonsoft compatible types, otherwise the client can't serialize it. Rather than 'poisoning' the rest of the roslyn protocol with newtonsoft attributes, I created duplicate types that we can delete as soon we as swtich to STJ on the client side. I also updated the tests to actually serialize using newtonsoft on the client side (server side still uses STJ).
cade388 - small random cleanup

Tested C#/Razor/F#/TS in VS, document symbols in VS, and C#/Razor in VSCode.

TODO - PR validation in progress.
Initial results are showing an almost 25% reduction in allocations in the lightbulb speedometer tests. There are some methods JIT regressions, currently testing new optprof data to see if that helps.

Resolves #72867

have not switched to STJ on the client side

dibarbet · 2024-05-03T00:16:42Z

...es/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/AbstractLanguageServer.cs

-
-            // Retrieve the language of the request so we know how to deserialize it.
-            var language = _target.GetLanguageForRequest(_method, request);
+        public abstract MethodInfo GetEntryPoint(bool hasParameter);


I created two subclasses of AbstractLanguageServer, one for Newtonsoft and one for STJ (we still support webtools/razor using clasp using newtonsoft).

This can all be simplified once we move Razor/XAML over to our protocol types and no longer need to manually handle serialization

CyrusNajmabadi · 2024-05-03T00:17:16Z

Any early numbers about it this improves anything?

dibarbet · 2024-05-03T00:17:20Z

.../LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/NewtonsoftLanguageServer.cs

+        /// StreamJsonRpc entry point for all handler methods.
+        /// The optional parameters allow StreamJsonRpc to call into the same method for any kind of request / notification (with any number of params or response).
+        /// </summary>
+        private async Task<JToken?> ExecuteRequestAsync(JToken? request = null, CancellationToken cancellationToken = default)


this is the same as before, just moved into its own subclass

dibarbet · 2024-05-03T00:17:43Z

...guageServer/Microsoft.CommonLanguageServerProtocol.Framework/SystemTextJsonLanguageServer.cs

+
+namespace Microsoft.CommonLanguageServerProtocol.Framework;
+
+internal abstract class SystemTextJsonLanguageServer<TRequestContext>(JsonRpc jsonRpc, JsonSerializerOptions options, ILspLogger logger) : AbstractLanguageServer<TRequestContext>(jsonRpc, logger)


this is all new for STJ support.

dibarbet · 2024-05-03T00:21:51Z

...guageServer/Microsoft.CommonLanguageServerProtocol.Framework/SystemTextJsonLanguageServer.cs

+        /// <summary>
+        /// StreamJsonRpc entry point for handlers with parameters (and any response) type.
+        /// </summary>
+        private async Task<JsonElement?> ExecuteRequestAsync(JsonElement? request, CancellationToken cancellationToken = default)


STJ uses JsonElement, which is a struct, whereas newtonsoft uses JToken (a class). This caused all sorts of nullability differences between the two impls, so 100x easier to separate them (even if there is a small amount of duplication) here.

And as mentioned, all of this is simplified once we switch Razor/XAML to our types and remove the custom serialization logic here.

dibarbet · 2024-05-03T00:23:22Z

src/Features/LanguageServer/Protocol/Protocol/Converters/JsonConverterCollectionUtilities.cs

+
+namespace Roslyn.LanguageServer.Protocol
+{
+    using Newtonsoft.Json;


unnecessary, deleted in followup commit.

dibarbet · 2024-05-03T00:24:22Z

src/Features/LanguageServer/Protocol/Protocol/Internal/Converters/DropProgressConverter.cs

+    /// <summary>
+    /// Converter used to deserialize objects dropping any <see cref="IProgress{T}"/> property.
+    /// </summary>
+    internal class DropProgressConverter : JsonConverter


unnecessary, delted in followup

dibarbet · 2024-05-03T00:25:50Z

eng/Directory.Packages.props

@@ -99,7 +99,7 @@
    <PackageVersion Include="Microsoft.VisualStudio.Language" Version="$(MicrosoftVisualStudioCoreVersion)" />
    <PackageVersion Include="Microsoft.VisualStudio.Language.NavigateTo.Interfaces" Version="$(MicrosoftVisualStudioCoreVersion)" />
    <PackageVersion Include="Microsoft.VisualStudio.Language.StandardClassification" Version="$(MicrosoftVisualStudioCoreVersion)" />
-    <PackageVersion Include="Microsoft.VisualStudio.LanguageServer.Client" Version="17.10.11-preview" />
+    <PackageVersion Include="Microsoft.VisualStudio.LanguageServer.Client" Version="17.10.72-preview" />


switching to newer version that has APIs using concrete types instead of JToken. Will make the transition to STJ on the client side easier later.

dibarbet · 2024-05-03T00:26:39Z

...es/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.InitializationOptions.cs

            internal bool CallInitialized { get; init; } = true;
            internal object? ClientTarget { get; init; } = null;
            internal string? Locale { get; init; } = null;
            internal IEnumerable<DiagnosticAnalyzer>? AdditionalAnalyzers { get; init; } = null;
+            internal IJsonRpcMessageFormatter? ClientMessageFormatter { get; init; } = null;


Allows us to use newtonsoft on the client side in tests for serialization

dibarbet · 2024-05-03T00:27:39Z

src/VisualStudio/Core/Def/DocumentOutline/DocumentOutlineViewModel_Utilities.cs

@@ -23,43 +25,37 @@ namespace Microsoft.VisualStudio.LanguageServices.DocumentOutline;
 using LspDocumentSymbol = DocumentSymbol;
 using Range = Roslyn.LanguageServer.Protocol.Range;

-internal delegate Task<ManualInvocationResponse?> LanguageServiceBrokerCallback(
-    ITextBuffer textBuffer, Func<JToken, bool> capabilitiesFilter, string languageServerName, string method, Func<ITextSnapshot, JToken> parameterFactory, CancellationToken cancellationToken);
+internal delegate Task<TResponse?> LanguageServiceBrokerCallback<TRequest, TResponse>(Request<TRequest, TResponse> request, CancellationToken cancellationToken);


switches to the new lsp client API

dibarbet · 2024-05-03T00:28:32Z

src/VisualStudio/Core/Def/DocumentOutline/DocumentOutlineViewModel_Utilities.cs

        string textViewFilePath,
        CancellationToken cancellationToken)
    {
        ITextSnapshot? requestSnapshot = null;
-        JToken ParameterFunction(ITextSnapshot snapshot)
+
+        var request = new DocumentRequest<DocumentSymbolNewtonsoft.NewtonsoftRoslynDocumentSymbolParams, DocumentSymbolNewtonsoft.NewtonsoftRoslynDocumentSymbol[]>()


Even though we switched to the new LSP client API that uses concrete types instead of JTOken - the streamjsonrpc instance on the client side is still newtonsoft. Therefore we have to use type definitions compatible with newtonsoft serialization

dibarbet · 2024-05-03T00:30:23Z

src/VisualStudio/Core/Def/DocumentOutline/DocumentSymbolNewtonsoft.cs

+/// https://github.com/dotnet/roslyn/pull/72675 tracks opting in the client to STJ.
+/// TODO - everything in this type should be deleted once the client side is using STJ.
+/// </summary>
+internal class DocumentSymbolNewtonsoft


I tried to contain the spread of newtonsoft here by just copying the types we need and adding the newtonsoft attributes to them.

These can be deleted as soon as we switch to STJ on the client side

ToddGrun · 2024-05-03T04:55:40Z

...guageServer/Microsoft.CommonLanguageServerProtocol.Framework/SystemTextJsonLanguageServer.cs

+        /// </summary>
+        private async Task<JsonElement?> ExecuteRequestAsync(JsonElement? request, CancellationToken cancellationToken = default)
+        {
+            var queue = target.GetRequestExecutionQueue();


target

@CyrusNajmabadi

Off topic: What's the general consensus on primary constructor usage?

I like them when they are accessed during field initializations (outside methods). However, I strongly like being able to easily differentiate class vs local data within methods. I guess if I had my druthers, for any primary ctor arg needed in a method we'd still have the field declaration using the underscore syntax that sets itself to the primary constructor arg.

currently no rules. there are mixed feelings. I personally have no issues with captures. but i'm fairly used to lambdas, local functions, and JS/TS-style capture development. I can understand the desire around fields though.

Maybe a general rule would be that it's ok for fairly simple types with few methods. But once it gets complex enough, we assign to fields?

dibarbet · 2024-05-06T23:42:14Z

blocked on microsoft/vs-streamjsonrpc#1038 (found in test insertion)

DoctorKrolic · 2024-05-10T20:37:19Z

Fixes #72867 or at least is related to that issue, if we consider that there are still Newtonsoft-based parts for razor compatibility

dibarbet · 2024-05-13T20:12:33Z

blocked on microsoft/vs-streamjsonrpc#1038 (found in test insertion)

unblocked. Now pending investigation of method JIT regressions

dibarbet · 2024-05-15T01:16:21Z

Updated PR validation is here https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/550816 (internal only)

JIT regressions are approved, will merge as soon as CI completes.

CyrusNajmabadi · 2024-05-15T19:48:19Z

Woohoo!

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 24, 2024

dibarbet force-pushed the lsp_stj branch from 2d9c4ea to 3b7f803 Compare April 24, 2024 01:34

kiminuo mentioned this pull request Apr 24, 2024

Use Json.NET instead of Newtonsoft.Json zkSNACKs/WalletWasabi#4223

Open

dibarbet force-pushed the lsp_stj branch 3 times, most recently from ff777c9 to ef5f44c Compare April 25, 2024 22:04

Switch Roslyn protocol types to System.Text.Json serialization

6046471

dibarbet force-pushed the lsp_stj branch 3 times, most recently from d0f8a35 to 84d40a8 Compare May 2, 2024 18:59

dibarbet added 2 commits May 2, 2024 13:37

Switch Roslyn language server to using System.Text.Json

b292a51

Use Newtonsoft for client side serialization in Document Outline as we

1a4c3f4

have not switched to STJ on the client side

dibarbet force-pushed the lsp_stj branch 2 times, most recently from 3fa1ec0 to 16e8689 Compare May 3, 2024 00:10

dibarbet changed the title ~~[WIP] Switch Roslyn protocol types to System.Text.Json serialization~~ Switch Roslyn protocol types to System.Text.Json serialization May 3, 2024

dibarbet marked this pull request as ready for review May 3, 2024 00:11

dibarbet requested review from a team as code owners May 3, 2024 00:11

dibarbet commented May 3, 2024

View reviewed changes

CyrusNajmabadi approved these changes May 3, 2024

View reviewed changes

dibarbet commented May 3, 2024

View reviewed changes

Cleanup a few more instances of newtonsoft and warnings

cade388

dibarbet force-pushed the lsp_stj branch from 16e8689 to cade388 Compare May 3, 2024 00:24

dibarbet commented May 3, 2024

View reviewed changes

ToddGrun reviewed May 3, 2024

View reviewed changes

dibarbet mentioned this pull request May 6, 2024

SystemTextJsonFormatter throws exceptions when ETW logging is enabled microsoft/vs-streamjsonrpc#1038

Closed

Merge branch 'main' into lsp_stj

c1fd7b3

dibarbet enabled auto-merge May 15, 2024 01:16

dibarbet merged commit e5d344a into dotnet:main May 15, 2024
28 checks passed

dibarbet deleted the lsp_stj branch May 15, 2024 18:05

dotnet-policy-service bot added this to the Next milestone May 15, 2024

333fred added a commit to 333fred/compiler-developer-sdk that referenced this pull request May 15, 2024

Add JsonPropertyName to react to dotnet/roslyn#73207.

950c240

333fred mentioned this pull request May 15, 2024

Add STJ Attributes 333fred/compiler-developer-sdk#7

Merged

333fred added a commit to 333fred/compiler-developer-sdk that referenced this pull request May 16, 2024

Add JsonPropertyName to react to dotnet/roslyn#73207.

0b11f71

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch Roslyn protocol types to System.Text.Json serialization #73207

Switch Roslyn protocol types to System.Text.Json serialization #73207

dibarbet commented Apr 24, 2024 •

edited

dibarbet May 3, 2024

CyrusNajmabadi commented May 3, 2024

dibarbet May 3, 2024

dibarbet May 3, 2024

dibarbet May 3, 2024 •

edited

dibarbet May 3, 2024

dibarbet May 3, 2024

dibarbet May 3, 2024 •

edited

dibarbet May 3, 2024

dibarbet May 3, 2024

dibarbet May 3, 2024

dibarbet May 3, 2024

ToddGrun May 3, 2024

CyrusNajmabadi May 3, 2024

dibarbet commented May 6, 2024

DoctorKrolic commented May 10, 2024

dibarbet commented May 13, 2024 •

edited

dibarbet commented May 15, 2024

CyrusNajmabadi commented May 15, 2024


		namespace Microsoft.CommonLanguageServerProtocol.Framework;

		internal abstract class SystemTextJsonLanguageServer<TRequestContext>(JsonRpc jsonRpc, JsonSerializerOptions options, ILspLogger logger) : AbstractLanguageServer<TRequestContext>(jsonRpc, logger)

Switch Roslyn protocol types to System.Text.Json serialization #73207

Switch Roslyn protocol types to System.Text.Json serialization #73207

Conversation

dibarbet commented Apr 24, 2024 • edited

Choose a reason for hiding this comment

CyrusNajmabadi commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet May 3, 2024 • edited

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

dibarbet commented May 6, 2024

DoctorKrolic commented May 10, 2024

dibarbet commented May 13, 2024 • edited

dibarbet commented May 15, 2024

CyrusNajmabadi commented May 15, 2024

dibarbet commented Apr 24, 2024 •

edited

dibarbet May 3, 2024 •

edited

dibarbet May 3, 2024 •

edited

dibarbet commented May 13, 2024 •

edited