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

Restrict LanguageServer IVTs to a specific namespace #10183

Merged
merged 5 commits into from
Apr 1, 2024

Conversation

DustinCampbell
Copy link
Member

Fixes #10096

Microsoft.VisualStudio.Razor.LanguageServerClient has a reference to Microsoft.AspNetCore.Razor.LanguageServer along with InternalsVisibleTo access. This is unavoidable because we currently run the LanguageServer in-proc in VS when the LSP editor is active. However, over time LanguageServerClient has relied on more and more code in LanguageServer, which can cause MS.ANC.Razor.LanguageServer to be loaded in VS when it shouldn't be. This change addresses this issue by limiting the types that LanguageServerClient has access to.

Access is restricted using the RestrictedInternalsVisibleToAttribute, which is already supported by Microsoft.CodeAnalysis.CSharp.BannedApiAnalyzers. Other than adding our own copy of this attribute and updating the AssemblyInfo.cs for MS.ANC.Razor.LanguageServer, this change is entirely mechanical. Essentially, I've just moved types around, and those types fall into two categories:

  1. Types that are needed to host the language server. These have been moved to a Microsoft.AspNetCore.Razor.LanguageServer.Hosting namespace, and IVT is restricted to that namespace.
  2. Protocol types that are needed by both the client and server. These have been moved to MS.CA.Razor.Workspaces and organized in a Protocol folder.

@DustinCampbell DustinCampbell requested review from a team as code owners March 28, 2024 20:16
Copy link
Member Author

@DustinCampbell DustinCampbell Mar 28, 2024

Choose a reason for hiding this comment

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

👀 The real change is in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

👀 Here's our definition of RestrictedInternalsVisibleToAttribute

Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

I think there is probably more that could moved/abstracted out of Hosting and into workspaces (IClientConnection I think would be good, as it feels odd that so many endpoints have a using for the Hosting namespace), but this is still good as is.

@DustinCampbell
Copy link
Member Author

I'm going to be out tomorrow. So, rather than "merge and run", I'll wait until Monday.

@DustinCampbell DustinCampbell merged commit a5a56a0 into dotnet:main Apr 1, 2024
12 checks passed
@DustinCampbell DustinCampbell deleted the languageserver-ivt branch April 1, 2024 15:46
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 1, 2024
@Cosifne Cosifne modified the milestones: Next, 17.11 P2 May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency from on LanguageClient on LanguageServer
3 participants