[Editor] Upgrade RolsynPad.Roslyn to v4.8.0 #2124

a-tsymbal · 2024-01-25T08:55:09Z

PR Details

Upgraded RoslynPad.Roslyn* packages to version 4.8.0

Description

Upgraded required packages, added ability for MEF to access internal implementations of contracts using IgnoresAccessChecksToGenerator.

To get access to some required unpublished as Nuget packages contracts from Microsoft.CodeAnalysis.LanguageServer.Protocol namespace added Stride.RestoreHelper. Copied GetLibReferences to other places where Stride.props and Stride.Core.props were not referenced to fix build.

The approach was the same as was used by RoslynPad in relation to Microsoft.CodeAnalysis.* internal dependencies. On Stride side same actions were required in relation to RoslynPad.Roslyn internal dependencies.

! Commented out RoslynWorkspace.EnableDiagnostics() (same happens with DiagnosticProvider.Enable()) call because of error:

  InvalidCastException: Unable to cast object of type 'Stride.Assets.Presentation.AssetEditors.ScriptEditor.StrideRoslynWorkspace' to type 'RoslynPad.Roslyn.RoslynWorkspace'.
     at RoslynPad.Roslyn.DocumentTrackingServiceFactory.DocumentTrackingService..ctor(Workspace workspace)
     at RoslynPad.Roslyn.DocumentTrackingServiceFactory.CreateService(HostWorkspaceServices workspaceServices)
     at Microsoft.CodeAnalysis.Host.Mef.MefWorkspaceServices.<>c__DisplayClass5_0.<.ctor>b__1()
     at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
     at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
     at System.Lazy`1.CreateValue()
     at Microsoft.CodeAnalysis.Host.Mef.MefWorkspaceServices.GetService[TWorkspaceService]()
     at Microsoft.CodeAnalysis.Host.HostWorkspaceServices.GetRequiredService[TWorkspaceService]()
     at Microsoft.CodeAnalysis.SolutionCrawler.SolutionCrawlerRegistrationService.WorkCoordinator..ctor(IAsynchronousOperationListener listener, IEnumerable`1 analyzerProviders, Boolean initializeLazily, Registration registration)
     at Microsoft.CodeAnalysis.SolutionCrawler.SolutionCrawlerRegistrationService.EnsureRegistration(Workspace workspace, Boolean initializeLazily)
     at Microsoft.CodeAnalysis.SolutionCrawler.SolutionCrawlerRegistrationService.Register(Workspace workspace)
     at Microsoft.CodeAnalysis.SolutionCrawler.SolutionCrawlerRegistrationService.Microsoft.CodeAnalysis.SolutionCrawler.ISolutionCrawlerRegistrationService.Register(Workspace workspace)
     at Microsoft.CodeAnalysis.DiagnosticProvider.Enable(Workspace workspace)
     at Stride.Assets.Presentation.AssetEditors.ScriptEditor.StrideRoslynWorkspace..ctor(RoslynHost host) in E:\github\stride\sources\editor\Stride.Assets.Presentation\AssetEditors\ScriptEditor\StrideRoslynWorkspace.cs:line 37
     at Stride.Assets.Presentation.AssetEditors.ScriptEditor.RoslynHost..ctor() in E:\github\stride\sources\editor\Stride.Assets.Presentation\AssetEditors\ScriptEditor\RoslynHost.cs:line 50

Probably MEF is resolving RoslynPad's workspace type instead of Stride's.

! So maybe some things related to diagnostics might stop working.

Related Issue

2043 Fixed issue mentioned in this comment

Motivation and Context

Fixes some functionality related to code analysis in script editor.

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My change requires a change to the documentation.
I have added tests to cover my changes.
All new and existing tests passed.
I have built and run the editor to try this change out.

Kryptos-FR

It might work but it feels very hackish. We shouldn't need all of that to make it work.

Instead, we should either properly reimplement the custom workspace/host or reuse the one provided by RoslynPad library.

Kryptos-FR · 2024-01-25T10:51:30Z

sources/editor/Stride.RestoreHelper/Stride.RestoreHelper.csproj

What is this file? It seems unrelated to Roslyn.

https://github.com/microsoft/MSBuildSdks/blob/main/src/NoTargets/README.md ("This can be useful for utility projects that just copy files, ...")

This is a helper project to copy requred dll from nuget folder
~.nuget\packages\microsoft.codeanalysis.languageserver.protocol\4.8.0-7.23558.1\lib\net7.0\Microsoft.CodeAnalysis.LanguageServer.Protocol.dll

We use it because now this dll is deprecated on nuget
https://www.nuget.org/packages/Microsoft.CodeAnalysis.LanguageServer.Protocol/3.9.0-5.21120.8

sources/editor/Stride.Assets.Presentation/AssetEditors/ScriptEditor/RoslynWorkspace.cs

sources/editor/Stride.Assets.Presentation/AssetEditors/ScriptEditor/RoslynHost.cs

global.json

sources/editor/Stride.RestoreHelper/Stride.RestoreHelper.csproj

Kryptos-FR · 2024-01-25T10:59:28Z

sources/editor/Stride.RestoreHelper/Stride.RestoreHelper.csproj

+    <RestoreSources>
+      https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json;
+      https://pkgs.dev.azure.com/azure-public/vside/_packaging/vssdk/nuget/v3/index.json;
+      https://pkgs.dev.azure.com/azure-public/vside/_packaging/vs-impl/nuget/v3/index.json;
+      https://api.nuget.org/v3/index.json
+    </RestoreSources>


What are those sources?

I took this from RoslynPad here https://github.com/roslynpad/roslynpad/blob/main/src/RestoreHelper/RestoreHelper.csproj

Kryptos-FR · 2024-01-25T10:59:51Z

sources/editor/Stride.Assets.Presentation/Stride.Assets.Presentation.csproj

+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.Features" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp.Features" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp.EditorFeatures" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.Scripting" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp.Scripting" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.Workspaces" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.CSharp.Workspaces" />
+    <InternalsAssemblyName Include="Microsoft.CodeAnalysis.LanguageServer.Protocol" />
+
+    <InternalsAssemblyName Include="RoslynPad.Roslyn" />
+    <InternalsAssemblyName Include="RoslynPad.Roslyn.Windows" />


Why is it needed, and what does it do?

It allows to use internal implementation from other assemblies https://github.com/aelij/IgnoresAccessChecksToGenerator.
E.g implementation of the IDiagnosticsService interface from RoslynPad looks like this https://github.com/roslynpad/roslynpad/blob/main/src/RoslynPad.Roslyn/Diagnostics/DiagnosticsService.cs#L7.
It is internal and I think MEF is not able to find it so we see this error: #2043 (comment)
System.Composition.Hosting.CompositionFailedException: 'No export was found for the contract 'IDiagnosticService'.

Same thing RoslynPad is doing with Microsoft.CodeAnalysis dependencies https://github.com/roslynpad/roslynpad/blob/main/src/RoslynPad.Roslyn/RoslynPad.Roslyn.csproj#L32.

Removed not used internals, left only required.

Kryptos-FR · 2024-01-25T11:01:02Z

sources/Directory.Packages.props

@@ -2,6 +2,9 @@
  <PropertyGroup>
    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
  </PropertyGroup>
+  <PropertyGroup>
+    <RoslynPrivateVersion>4.8.0-7.23558.1</RoslynPrivateVersion>


Shouldn't be needed if the <PackageVersion>version is defined in this file.

It is not defined in this file but we could move this to Stride.RestoreHelper.csproj and set it directly there.

Kryptos-FR · 2024-01-25T11:01:46Z

sources/editor/Stride.RestoreHelper/Stride.RestoreHelper.csproj

+  <Target Name="GetLibReferences" Outputs="@(Reference)">
+    <ItemGroup>
+      <Reference Include="$(PkgMicrosoft_CodeAnalysis_LanguageServer_Protocol)/lib/net7.0/*.dll" />
+    </ItemGroup>
+  </Target>


How does it work? Where is the reference (documentation) for this trick?

Answered in another comment, but here are more details https://duanenewman.net/blog/post/a-better-way-to-override-references-with-packagereference/

And one more here: https://learn.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#generatepathproperty

Ethereal77 · 2024-01-25T11:23:01Z

Instead, we should either properly reimplement the custom workspace/host or reuse the one provided by RoslynPad library.

That won't do. The RoslynPad workspace is not a MSBuild workspace operating on a solution with many projects like Stride does. RoslynPad creates a simple Roslyn workspace per document (as they are independent from one another), and is way simpler than what we have in Stride. Also it has no need to detect external changes, as RoslynPad has all of this "project" structure in memory.

At least, that's the way it was implemented the last time I checked (last year).

I have a prototype I made some time ago where I integrated RoslynPad inside Stride (i.e. not as an assembly reference but as code, to ease customization and get only the needed parts). I had to reimplement some parts of the Stride workspace mixing it with RoslynPad's.

a-tsymbal · 2024-01-25T21:08:30Z

Instead, we should either properly reimplement the custom workspace/host or reuse the one provided by RoslynPad library.

When I was working on this PR I wondered why some code looks almost the same as in RoslynPad repository and why it wasn't used. But from this #2124 (comment) explained it.

In this PR I just fixed issue by issue that was happening after upgrading RoslynPad.Roslyn* packages.

IXLLEGACYIXL · 2024-01-25T22:10:58Z

i cant run this PR , ive ran
git clean -xfd
dotnet restore

but i end up with these errors
it says that language server protocols 4.8 isnt found and that stride presentation dll wasnt found

https://gist.github.com/IXLLEGACYIXL/54ce8f77f4e0c8b52c29bc49e5f39a08

a-tsymbal · 2024-01-25T22:35:38Z

Probably you don't have this package in the .nuget folder.
It is located here: C:\Documents and Settings%user%.nuget\packages\microsoft.codeanalysis.languageserver.protocol\4.8.0-7.23558.1\lib\net7.0\Microsoft.CodeAnalysis.LanguageServer.Protocol.dll

I think it was added there by some of the .NET SDKs as it is not available on nuget website, here is my SDKs:

dotnet --info

.NET SDKs installed:
  7.0.115 [C:\Program Files\dotnet\sdk]
  8.0.101 [C:\Program Files\dotnet\sdk]

IXLLEGACYIXL · 2024-01-25T22:53:59Z

Probably you don't have this package in the .nuget folder. It is located here: C:\Documents and Settings%user%.nuget\packages\microsoft.codeanalysis.languageserver.protocol\4.8.0-7.23558.1\lib\net7.0\Microsoft.CodeAnalysis.LanguageServer.Protocol.dll

I think it was added there by some of the .NET SDKs as it is not available on nuget website, here is my SDKs:

dotnet --info
.NET SDKs installed:
  7.0.115 [C:\Program Files\dotnet\sdk]
  8.0.101 [C:\Program Files\dotnet\sdk]

i have these sdks too

a-tsymbal · 2024-01-28T16:49:24Z

Try to remove api.nuget.org source from the Stride.RestoreHelper.csproj

I see this package in another source: https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json

All versions of this package: https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/d1622942-d16f-48e5-bc83-96f4539e7601/nuget/v3/flat2/Microsoft.CodeAnalysis.LanguageServer.Protocol/index.json`

Version used by RolsynPad: https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/d1622942-d16f-48e5-bc83-96f4539e7601/nuget/v3/registrations2-semver2/Microsoft.CodeAnalysis.LanguageServer.Protocol/4.8.0-7.23558.1.json

In the response from the second link there is a direct link to nupkg file.

Upgrade RolsynPad to v4.8.0

62783ee

Kryptos-FR requested changes Jan 25, 2024

View reviewed changes

Remove unnecessary changes

2ac91b0

a-tsymbal marked this pull request as draft February 18, 2024 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Editor] Upgrade RolsynPad.Roslyn to v4.8.0 #2124

[Editor] Upgrade RolsynPad.Roslyn to v4.8.0 #2124

a-tsymbal commented Jan 25, 2024

Kryptos-FR left a comment

Kryptos-FR Jan 25, 2024

a-tsymbal Jan 25, 2024

Kryptos-FR Jan 25, 2024

a-tsymbal Jan 25, 2024

Kryptos-FR Jan 25, 2024

a-tsymbal Jan 25, 2024

a-tsymbal Jan 27, 2024

Kryptos-FR Jan 25, 2024

a-tsymbal Jan 25, 2024 •

edited

Kryptos-FR Jan 25, 2024

a-tsymbal Jan 25, 2024

a-tsymbal Jan 28, 2024

Ethereal77 commented Jan 25, 2024

a-tsymbal commented Jan 25, 2024

IXLLEGACYIXL commented Jan 25, 2024 •

edited

a-tsymbal commented Jan 25, 2024

IXLLEGACYIXL commented Jan 25, 2024

a-tsymbal commented Jan 28, 2024

[Editor] Upgrade RolsynPad.Roslyn to v4.8.0 #2124

Are you sure you want to change the base?

[Editor] Upgrade RolsynPad.Roslyn to v4.8.0 #2124

Conversation

a-tsymbal commented Jan 25, 2024

PR Details

Description

Related Issue

Motivation and Context

Types of changes

Checklist

Kryptos-FR 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

a-tsymbal Jan 25, 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

Ethereal77 commented Jan 25, 2024

a-tsymbal commented Jan 25, 2024

IXLLEGACYIXL commented Jan 25, 2024 • edited

a-tsymbal commented Jan 25, 2024

IXLLEGACYIXL commented Jan 25, 2024

a-tsymbal commented Jan 28, 2024

a-tsymbal Jan 25, 2024 •

edited

IXLLEGACYIXL commented Jan 25, 2024 •

edited