Update versions to use most recent LanguageServer changes. #61496

akhera99 · 2022-05-24T23:10:24Z

This is a helper PR for #61491

genlu · 2022-05-24T23:21:20Z

eng/Versions.props

@@ -25,10 +25,10 @@
    <MicrosoftVisualStudioExtensibilityTestingVersion>0.1.132-beta</MicrosoftVisualStudioExtensibilityTestingVersion>
    <!-- CodeStyleAnalyzerVersion should we updated together with version of dotnet-format in dotnet-tools.json -->
    <CodeStyleAnalyzerVersion>4.2.0-2.final</CodeStyleAnalyzerVersion>
-    <VisualStudioEditorPackagesVersion>17.2.3194</VisualStudioEditorPackagesVersion>
+    <VisualStudioEditorPackagesVersion>17.3.37-preview</VisualStudioEditorPackagesVersion>


Would this break integration tests?

It did in the other PR I made, what can I do to fix that?

We'd need to reinstate -vs-deps branch, so this won't affect people works in main. I also have a change requires new Editor package.

is this in preview1 or preview2? Preview1 should be rolling out to machines soonish, so if its present there we can avoid a -vs-deps branch

hmm, looks like rel/d17.3 has 17.3.41-preview, so I guess it will be available soon?
https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/VS?path=%2F.corext%2FConfigs%2FVS-Platform%2FVS-Platform.packageconfig&version=GBrel%2Fd17.3&_a=contents

I think p1 should be rolling out to machines next week, so it might be reasonable to wait

genlu · 2022-05-24T23:22:50Z

#61494 this just merged, so you'd need a sync :)

ryzngard · 2022-05-24T23:39:34Z

src/Features/Lsif/GeneratorTest/FoldingRangeTests.vb

@@ -78,4 +78,4 @@ using System.Linq;|}", "imports")>
            Return foldingRange
        End Function
    End Class
-End Namespace
+End Namespace


revert this file :)

Cosifne · 2022-05-25T00:07:34Z

@akhera99
Let me know when you are ready to get this in, and I am going to create main-vs-deps for you

davidwengier · 2022-05-25T00:29:01Z

src/EditorFeatures/Core/Microsoft.CodeAnalysis.EditorFeatures.csproj

@@ -39,6 +41,7 @@
    <PackageReference Include="Microsoft.VisualStudio.Language" Version="$(MicrosoftVisualStudioLanguageVersion)" />
    <!-- No warn on NU1701 until the LSP client targets netstandard2.0 https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1369985/ -->
    <PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client" Version="$(MicrosoftVisualStudioLanguageServerClientVersion)" PrivateAssets="all" NoWarn="NU1701" />
+    <PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client.Implementation" Version="$(MicrosoftVisualStudioLanguageServerClientImplementationVersion)" PrivateAssets="all" NoWarn="NU1701" />


@gundermanc Does this pose a problem for VS Mac? Currently Microsoft.VisualStudio.LanguageServer.Client.Implementation is excluded from the VS Mac MEF composition, but I'm not sure if thats due to technical limitations or just history.

Currently Microsoft.VisualStudio.LanguageServer.Client.Implementation is excluded from the VS Mac MEF composition

It is? My most recent understanding is that it should be part of the composition as it contains the LSP client implementation on Mac.

Note that though they share the same name, Microsoft.VisualStudio.LanguageServer.Client.Implementation on VS Mac is a VS Mac and Cocoa specific build with different implementations. The public surface area is similar to that of VS Windows and the assembly reference will resolve but some public types and members available via the Windows implementation do not exist in the Mac one.

My most recent understanding is that it should be part of the composition

Maybe I'm reading the XML wrong. Its in the composition, but won't be scanned. Perhaps that just means it won't be scanned for [Import]s but can still participate in [Export]s? @KirillOsenkov probably knows.

The public surface area is similar to that of VS Windows

How similar? Is it a bad idea to reference it from a platform agnostic library? ie, is there a chance that this will break VS Mac because the snippet expansion provider isn't present there?

Maybe I'm reading the XML wrong. Its in the composition, but won't be scanned. Perhaps that just means it won't be scanned for [Import]s but can still participate in [Export]s

IDK what that means specifically. On my Mac it appears to light up correctly.

How similar?

The goal is that all non-obsolete public members and features should eventually be present on both the Mac and Windows hosts but it may take some time to get there. Here's an example of legacy stuff that's excluded: https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient?path=/src/product/RemoteLanguage/Def/ILanguageServiceBroker.cs&version=GBdevelop&line=29&lineEnd=53&lineStartColumn=1&lineEndColumn=7&lineStyle=plain&_a=contents

It looks like LanguageServerSnippetExpander does indeed exist on both Mac and Windows builds. The one thing to be aware of is that @akhera99 made a breaking change to this API May 12th and it probably hasn't yet been inserted to Mac, so we'll have to rev the LSP client before we can insert this version of Roslyn: https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/398270

If it's in the /MonoDevelop/Ide/Composition, it will be in MEF. You can only be in MEF or not in MEF, so can't have Exports without Imports :)

Scanning is for Mono.Addins, I believe (??) and should be unrelated for our purposes.

Also worth mention that all of the 'APIs' on Microsoft.VisualStudio.LanguageServer.Client.Implementation.dll are extremely crude and/or suspect. They were originally introduced for LiveShare support and haven't aged well with broader adoption. @MariaSolOs has a thread open with Editor team about its redesign: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1542016

Okay, false alarm, sorry. I saw an exclude in MonoDevelop.Ide.addin.xml for it here but thats not the MEF composition. Looks like it is in MEF here

so we'll have to rev the LSP client before we can insert this version of Roslyn:

Thank you, this is very good to know. How does that process work?

The other option, potentially, is of course to rearchitect the Roslyn bits here and move this dependency to EditorFeatures.Wpf, but thats worse long term, and probably will take longer anyway.

How does that process work?

LSP in VSfM is still new and I'm still working on the process. At the moment it's just rev to a consistent set of new packages. Kirill usually recommends picking a package version that matches those inserted in VS Windows.

Okay cool, so we can do that when the appropriate Roslyn insertion for VS Mac demands it.

JoeRobich · 2022-05-25T22:47:29Z

@akhera99 I see the change to main-vs-deps branch. Are we trying to get this in before CI updates with 17.3 P1 images? Or would it no longer work with P1?

davidwengier · 2022-05-25T23:11:27Z

src/EditorFeatures/Core/Microsoft.CodeAnalysis.EditorFeatures.csproj

-    <PackageReference Include="Microsoft.VisualStudio.Shell.15.0" Version="$(MicrosoftVisualStudioShell150Version)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" />
-    <PackageReference Include="Microsoft.VisualStudio.Shell.Framework" Version="$(MicrosoftVisualStudioShellFrameworkVersion)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" />
-    <PackageReference Include="Microsoft.VisualStudio.GraphModel" Version="$(MicrosoftVisualStudioGraphModelVersion)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" />
-    <PackageReference Include="Microsoft.VisualStudio.Imaging" Version="$(MicrosoftVisualStudioImagingVersion)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" />


😍🥰💖

This is amazing

update versioning to use languageserver.client.implementation

16c8bb5

akhera99 requested review from a team as code owners May 24, 2022 23:10

dotnet-issue-labeler bot added the Area-IDE label May 24, 2022

genlu reviewed May 24, 2022

View reviewed changes

merge main

f9aa60b

ryzngard reviewed May 24, 2022

View reviewed changes

fix

c0e2ecb

davidwengier reviewed May 25, 2022

View reviewed changes

davidwengier mentioned this pull request May 25, 2022

Semantic Snippets - Remove reflection and call the LanguageServerSnippetExpander directly #61491

Merged

akhera99 changed the base branch from main to main-vs-deps May 25, 2022 22:11

Simplify LSP reference update

ac5bd25

akhera99 requested a review from a team as a code owner May 25, 2022 22:43

davidwengier reviewed May 25, 2022

View reviewed changes

davidwengier approved these changes May 26, 2022

View reviewed changes

runfoapp bot mentioned this pull request May 26, 2022

Syntax_Provider_Doesnt_Attribute_Incorrect_Timing flaky on MacOS #61474

Closed

akhera99 merged commit 4e7a34d into dotnet:main-vs-deps May 26, 2022

ghost added this to the Next milestone May 26, 2022

Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update versions to use most recent LanguageServer changes. #61496

Update versions to use most recent LanguageServer changes. #61496

akhera99 commented May 24, 2022

genlu May 24, 2022

akhera99 May 24, 2022

genlu May 24, 2022

dibarbet May 25, 2022

genlu May 25, 2022 •

edited

Loading

dibarbet May 25, 2022

genlu commented May 24, 2022

ryzngard May 24, 2022

Cosifne commented May 25, 2022

davidwengier May 25, 2022

gundermanc May 25, 2022

davidwengier May 25, 2022

gundermanc May 25, 2022 •

edited

Loading

KirillOsenkov May 25, 2022

gundermanc May 25, 2022 •

edited

Loading

davidwengier May 25, 2022

gundermanc May 25, 2022

davidwengier May 25, 2022

JoeRobich commented May 25, 2022

davidwengier May 25, 2022

KirillOsenkov May 26, 2022

Update versions to use most recent LanguageServer changes. #61496

Update versions to use most recent LanguageServer changes. #61496

Conversation

akhera99 commented May 24, 2022

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

genlu May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu commented May 24, 2022

Choose a reason for hiding this comment

Cosifne commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gundermanc May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gundermanc May 25, 2022 • edited Loading

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

JoeRobich commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu May 25, 2022 •

edited

Loading

gundermanc May 25, 2022 •

edited

Loading

gundermanc May 25, 2022 •

edited

Loading