Implement json serialization/deserialization for LSP json messages. Part 2/N #68990

CyrusNajmabadi · 2023-07-11T21:34:05Z

Follows the same patterns as the existing library we use. However, while the types are the same, the namespace is different so as to not conflict.

Followup to #71141. Draft until that goes in. That PR introduced the types. This PR moves us onto them. We also remove our actual references entirely to the VS libs.

build: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=8820065&view=results
PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/517178

Follows the same patterns as the existing library we use. However, while the types are the same, the namespace is different so as to not conflict.

CyrusNajmabadi · 2023-07-11T21:35:27Z

...rFeatures/Core/Classification/Syntactic/SyntacticClassificationTaggerProvider.TagComputer.cs

 using Microsoft.VisualStudio.Text;
 using Microsoft.VisualStudio.Text.Tagging;
+using Roslyn.LanguageServer.Protocol;


a lot of fallout from switching from Microsoft.VisualStudio.LanguageServer.Protocol to Roslyn.LanguageServer.Protocol.

CyrusNajmabadi · 2023-07-11T21:36:03Z

src/EditorFeatures/Core/Extensions/LSPExtensions.cs

+            => new Roslyn.Text.Adornments.ClassifiedTextRun(run.ClassificationTypeName, run.Text, run.NavigationAction, run.Tooltip, (Roslyn.Text.Adornments.ClassifiedTextRunStyle)run.Style);
+
+        public static Roslyn.Text.Adornments.ClassifiedTextElement ToLSPElement(this VisualStudio.Text.Adornments.ClassifiedTextElement element)
+            => new Roslyn.Text.Adornments.ClassifiedTextElement(element.Runs.Select(r => r.ToLSPRun()));


a few helpers for when we have some VS types to convert to LSP types.

CyrusNajmabadi · 2023-07-11T21:38:13Z

...Server/Microsoft.CommonLanguageServerProtocol.Framework/Protocol/ApplyWorkspaceEditParams.cs

@@ -0,0 +1,39 @@
+// Licensed to the .NET Foundation under one or more agreements.


all our own versions of these lsp objects.

pros:

follows the same approach as MS.VS.LS.Protocol.

Is binary compatible for our code (modulo updating our namespace).

Can be tweaked as we see fit on our own schedule

Cons:

Newtonsoft based.

May diverge from VS as they add support for things. Will require manual upkeep.

Large.

Can be tweaked as we see fit on our own schedule

Razor has the same problem to solve with MS.VS.LS.Protocol. Would you rather us not use these so you are free to modify at will?

We need to do something here in regards to clasp / this with Razor. currently we basically cannot make any changes to clasp (of which I've wanted to make quite a fiew) without doing a dual insertion. I would like us to avoid more places where we need that, especially because LSP compatibility does not mean binary compat

without doing a dual insertion

I would block Razor using these types until we figure out how / if we can share it without dual insertions

I think we should also strongly consider using Karthik / Matteo's generation library for these (though doesn't have to be now). If we're using them,

yeah. we could potenitally make this a source package or something as well. it truly is just serialization goo.

does Karthik's lib handle all the extension cases?

Though - I would also avoid putting these definitions in clasp. It should probably be a different (source-only) package.

would having VS platform owned source package solve the issue we are tying to solve?

@olegtk yes. If the license for those files didn't conflict in some way.

wouldn't it cause PerfDDRIT regression though?

Not as a source package. It wouldn't make a new dll. It would literally just me code added to Roslyn. Just maintained in one location.

CyrusNajmabadi · 2023-07-11T21:42:03Z

@arkalyanms @dibarbet @jasonmalinowski . This should hopefully get us licensing clean. This is not my first choice at all. But absent approval and movement on unblocking these dependencies working with our license, this is the most expedient path forward. As we can see, there is literally nothing of interest here. It's all just DataContract + Newtonsoft.Json goop to give us an object model on top of the LSP protocol.

CyrusNajmabadi · 2023-07-11T21:54:34Z

also tagging @333fred as this may impact some downstream consumers (but in a way that should be easy to migrate to).

davidwengier · 2023-07-11T22:51:34Z

It seems odd that these are part of the public API of Microsoft.CommonLanguageServerProtocol.Framework, but namespaced with "Roslyn". Additionally, CLaSP was specifically written to avoid direct dependencies on LSP protocol POCOs. I think it would make sense to either move these out into their own/another project, or keep them in CLaSP, but namespace them accordingly (and in future, we could review whether the original design decision makes sense)

davidwengier · 2023-07-12T11:36:36Z

...ver/Microsoft.CommonLanguageServerProtocol.Framework/Protocol/LanguageServer.Protocol.csproj

@@ -0,0 +1,59 @@
+<Project>


Is this supposed to be here?

CyrusNajmabadi · 2023-07-12T15:52:56Z

It seems odd that these are part of the public API of Microsoft.CommonLanguageServerProtocol.Framework, but namespaced with "Roslyn". Additionally, CLaSP was specifically written to avoid direct dependencies on LSP protocol POCOs. I think it would make sense to either move these out into their own/another project, or keep them in CLaSP, but namespace them accordingly (and in future, we could review whether the original design decision makes sense)

All this is good feedback. I will say that my #1 goal above all else is to get us first to a clean licensing position. That delivers on our own licensing being sane, and making properly consumable by downstream (as well as abiding by everything we've promised).

After getting clean, i think we can update this aggressively to be in a place we want. That includes, but is not limited to:

deciding if these 'model types' should be in this lib, or in a separate lib.
deciding on the namespace for these types.
deciding on the general patterns practices for these types. For example, i would prefer them all to be records, with strong immutability guarantees around them.

But this can all come later IMO.

dibarbet · 2023-07-12T19:45:17Z

...osoft.CommonLanguageServerProtocol.Framework/Protocol/Internal/Text/ClassifiedTextElement.cs

+using System.Collections.Immutable;
+using Roslyn.Text.Adornments;
+
+namespace Roslyn.Text.Adornments


I would really like to avoid redefining the VS types if at all possible. Today using the protocol libs as-is, we can make the protocol project only depend on the public definitions by default (add more abstraction around the creation of LSP types).

However if we're using our own definitions I can see using the actual VS LSP library definitions could be hard, especially if we want a VSHover to be a subclass of an LSP Hover. The only workaround I can think of is object types, but that doesn't seem super nice.

Re-defining the vs only types is going to be annoying especially since that is where a lot of the changes go (so keeping in syc could be challenging).

arkalyanms

We have been asked to hold off on this PR in favor of a holistic solution on the LSP side.

CyrusNajmabadi · 2023-12-18T18:29:30Z

Moving forward. Speedometer showed no issues.

Implement json serialization/deserialization for LSP json messages.

8a5375e

Follows the same patterns as the existing library we use. However, while the types are the same, the namespace is different so as to not conflict.

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 11, 2023

CyrusNajmabadi commented Jul 11, 2023

View reviewed changes

CyrusNajmabadi force-pushed the lspApis2 branch 2 times, most recently from 3fb3596 to 4770338 Compare July 11, 2023 21:47

Merge branch 'main' into lspApis2

99570a1

CyrusNajmabadi force-pushed the lspApis2 branch from 4770338 to 99570a1 Compare July 11, 2023 21:54

davidwengier reviewed Jul 12, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into lspApis2

611c756

CyrusNajmabadi added 6 commits July 12, 2023 09:00

lint

7592fef

Lint

f74a4bc

Update

448194d

Marshalling

d387fca

Marshalling

80ed94e

Find refs

8681f96

dibarbet reviewed Jul 12, 2023

View reviewed changes

arkalyanms requested changes Jul 14, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into lspApis2

e6287b3

CyrusNajmabadi marked this pull request as ready for review December 7, 2023 00:43

CyrusNajmabadi requested review from 333fred and a team as code owners December 7, 2023 00:43

CyrusNajmabadi requested a review from arkalyanms December 7, 2023 00:43

CyrusNajmabadi changed the title ~~WIP: Implement json serialization/deserialization for LSP json messages.~~ Implement json serialization/deserialization for LSP json messages. Dec 7, 2023

CyrusNajmabadi added 4 commits December 6, 2023 19:16

Fix tests

0a447ec

Rename type

b6d97f2

Add new types in

31cb874

Remove file

aefdc5d

CyrusNajmabadi mentioned this pull request Dec 7, 2023

Implement json serialization/deserialization for LSP json messages. Part 1/N. #71141

Merged

Merge branch 'lspApis3' into lspApis2

f7d0c33

CyrusNajmabadi changed the title ~~Implement json serialization/deserialization for LSP json messages.~~ Implement json serialization/deserialization for LSP json messages. Part 2/N Dec 7, 2023

CyrusNajmabadi marked this pull request as draft December 7, 2023 03:33

CyrusNajmabadi added 2 commits December 6, 2023 19:42

Make internal

2cb528a

Merge branch 'lspApis3' into lspApis2

53b4d15

arkalyanms approved these changes Dec 14, 2023

View reviewed changes

CyrusNajmabadi added 10 commits December 15, 2023 10:54

Merge branch 'main' into lspApis3

232ebb1

Merge branch 'lspApis3' into lspApis2

6215db6

Downstream fallout

bbc3105

Fixes

4ad5db0

Accvessibility

fe289fc

fixes

cb0278c

fixes

1ff3c20

fixes

c22e296

fixes

19b6a4e

Fix

0c984c0

CyrusNajmabadi marked this pull request as ready for review December 18, 2023 18:29

CyrusNajmabadi merged commit 3988783 into dotnet:main Dec 18, 2023
27 checks passed

ghost added this to the Next milestone Dec 18, 2023

CyrusNajmabadi deleted the lspApis2 branch December 18, 2023 18:32

allisonchou mentioned this pull request Dec 31, 2023

[Automated] PRs inserted in VS build main-34431.06 #71430

Closed

Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024

333fred mentioned this pull request Jan 13, 2024

React to new extension changes 333fred/compiler-developer-sdk#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement json serialization/deserialization for LSP json messages. Part 2/N #68990

Implement json serialization/deserialization for LSP json messages. Part 2/N #68990

CyrusNajmabadi commented Jul 11, 2023 •

edited

CyrusNajmabadi Jul 11, 2023

CyrusNajmabadi Jul 11, 2023

CyrusNajmabadi Jul 11, 2023

davidwengier Jul 11, 2023 •

edited

dibarbet Jul 12, 2023

dibarbet Jul 12, 2023

CyrusNajmabadi Jul 12, 2023

dibarbet Jul 12, 2023

olegtk Jul 12, 2023

CyrusNajmabadi Jul 12, 2023

olegtk Jul 12, 2023

CyrusNajmabadi Jul 12, 2023

CyrusNajmabadi commented Jul 11, 2023

CyrusNajmabadi commented Jul 11, 2023

davidwengier commented Jul 11, 2023

davidwengier Jul 12, 2023

CyrusNajmabadi commented Jul 12, 2023

dibarbet Jul 12, 2023

arkalyanms left a comment

CyrusNajmabadi commented Dec 18, 2023

		@@ -0,0 +1,39 @@
		// Licensed to the .NET Foundation under one or more agreements.

Implement json serialization/deserialization for LSP json messages. Part 2/N #68990

Implement json serialization/deserialization for LSP json messages. Part 2/N #68990

Conversation

CyrusNajmabadi commented Jul 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier Jul 11, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jul 11, 2023

CyrusNajmabadi commented Jul 11, 2023

davidwengier commented Jul 11, 2023

Choose a reason for hiding this comment

CyrusNajmabadi commented Jul 12, 2023

Choose a reason for hiding this comment

arkalyanms left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Dec 18, 2023

CyrusNajmabadi commented Jul 11, 2023 •

edited

davidwengier Jul 11, 2023 •

edited