Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move csharp decompiler service down to features so it is available in vscode #69501

Merged
merged 4 commits into from
Aug 21, 2023

Conversation

dibarbet
Copy link
Member

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 14, 2023
@CyrusNajmabadi
Copy link
Member

We should make sure that ILSpy is listed in thirdpartynotices.txt to give them appropriate credit.

@@ -72,6 +72,7 @@
</ItemGroup>
<ItemGroup>
<PackageReference Include="Humanizer.Core" Version="$(HumanizerCoreVersion)" PrivateAssets="compile" />
<PackageReference Include="ICSharpCode.Decompiler" Version="$(ICSharpCodeDecompilerVersion)" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @jaredpar to advise if this will be a problem for source build

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing this will be a problem but checking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe this would be a problem. Features need to be source-buildable.

Copy link
Member

@tmat tmat Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add the reference to LanguageServer and MEF export the service there, no?

Copy link
Member Author

@dibarbet dibarbet Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmat Can't add an editor features reference to the language server project.

I was thinking of moving it to the Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj, but it requires dependencies on C# types. While we do technically depend on csharp there, it really isn't supposed to have language specific dependencies. Same issue occurs if I wanted to link it to the Microsoft.CodeAnalysis.LanguageServer project. I can technically do it, but violates language agnostic terms

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two options:

  1. Make this service language agnostic
  2. Add Microsoft.CodeAnalysis.LanguageServer.CSharp

I'd try [1] first. CSharpDecompiledSourceFormattingRule can be moved to Features.CSharp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "this service" I mean the actual decompilation. Could be a new service.
The glue around it, which might be C# specific could be moved to Features.CSharp

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated - moved the CSharp bits to CSharp.Features and added an abstraction over the decompilation, moved the actual decompilation implementation to LanguageServer.Protocol

@dibarbet dibarbet marked this pull request as ready for review August 16, 2023 22:27
@dibarbet dibarbet requested a review from a team as a code owner August 16, 2023 22:27
namespace Microsoft.CodeAnalysis.CSharp.DecompiledSource
{
[ExportLanguageService(typeof(IDecompilationService), LanguageNames.CSharp), Shared]
internal class CSharpCodeDecompilerDecompilationService : IDecompilationService
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSharpCodeDecompilerDecompilationService

Or just CSharpDecompilationService?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@dibarbet dibarbet requested a review from tmat August 21, 2023 20:20
@jasonmalinowski jasonmalinowski merged commit 531d12e into dotnet:main Aug 21, 2023
21 of 24 checks passed
@ghost ghost added this to the Next milestone Aug 21, 2023
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go To Definition doesn't show me the actuall implementation (VSCode)
7 participants