-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Load Roslyn DevKit assemblies into the default ALC #71807
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this PR makes me want to clean up so much more of our assembly loading/MEF magic.
@@ -31,7 +24,7 @@ private AssemblyLoadContextWrapper(AssemblyLoadContext assemblyLoadContext, Immu | |||
_logger = logger; | |||
} | |||
|
|||
public static bool TryLoadExtension(string assemblyFilePath, ILogger? logger, [NotNullWhen(true)] out Assembly? assembly) | |||
public static bool TryLoadExtension(string assemblyFilePath, ILogger logger, [NotNullWhen(true)] out Assembly? assembly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this even take a ILogger if we also passed a logger into this class in the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is static - there is a wrapper instance for each extension, which gets its logger from the parameter here.
{ | ||
try | ||
{ | ||
var loadContext = CreateLoadContext(name); | ||
logger.LogTrace("[{name}] Loading assemblies in {assembliesDirectoryPath}", name, assembliesDirectoryPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't want to just use interpolated strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theres actually a diagnostic if you do that. See https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2254
I noticed that this file already did it the 'correct' way, so stuck with that here.
{ | ||
Description = "Full paths of extension assemblies to load (optional).", | ||
Required = false | ||
}; | ||
|
||
var devKitDependencyPathOption = new CliOption<string?>("--devKitDependencyPath") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but potentially confusing otherwise: should this be a devKitDependenciesPath? Since there's multiple things in the folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? I chose non-plural because there is a single dependency project. Not sure
src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/ServerConfigurationFactory.cs
Show resolved
Hide resolved
This allows extensions to use the host instance of an assembly if they do not ship their own copy
Resolves #71101
This allows extensions to use the host instance of an assembly if they do not ship their own copy. This did not work previously because the Roslyn DevKit assemblies were loaded into a separate ALC. Now that we ship devkit assemblies in the extension alongside the server, it is safe to load them in the default ALC.
For example, an extension can now access the same instance of VS telemetry that the host has created (and use its telemetry session).
![Untitled](https://private-user-images.githubusercontent.com/5749229/299772570-12ffeeb7-0a0d-4763-9073-62876d19bdd4.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk4Nzg1NzIsIm5iZiI6MTczOTg3ODI3MiwicGF0aCI6Ii81NzQ5MjI5LzI5OTc3MjU3MC0xMmZmZWViNy0wYTBkLTQ3NjMtOTA3My02Mjg3NmQxOWJkZDQucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxOCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMThUMTEzMTEyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZWViYTdkMWU0NzE3MGM0ZTI1OWVkYzU5Y2IzMDgwMjdmNjU0M2U0NjI3ZmViMDZkYzczZWQ3ZTQ2NWEwN2Y0ZiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.i1jZ9a6bttgljhy4hZi5dCzwkQWSLYh8X0x99tIoaho)
Requires client side PR dotnet/vscode-csharp#6831