Load analyzers and generators in isolated ALCs in our OOP process #74780
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -15,20 +15,22 @@ namespace Microsoft.CodeAnalysis.Host; | |||
| internal abstract class AbstractAnalyzerAssemblyLoaderProvider : IAnalyzerAssemblyLoaderProvider | |||
There was a problem hiding this comment.
view with whitespace off.
src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.cs
Outdated
Show resolved
Hide resolved
| @@ -12,6 +12,10 @@ | |||
| using System.Runtime.InteropServices; | |||
There was a problem hiding this comment.
view with whitespace off.
If you haven't, you may want to run an RPS run to ensure the changing of paths in %TEMP% doesn't cause an unexplainable regression.| /// We include the <see cref="WorkspaceKind"/> of the workspace in the path we produce. That way we don't | ||
| /// collide in the common case of a normal host workspace and OOP workspace running together. This avoids an | ||
| /// annoying exception as each will try to clean up this directory, throwing exceptions because the other is | ||
| /// locking it. The exception is fine, since the cleanup is just hygienic and isn't intended to be needed for | ||
| /// correctness. But it is annoying and does cause noise in our perf test harness. |
There was a problem hiding this comment.
As an observation -- this code is relatively new. I don't understand why it ever would have been necessary, but we should be on the lookout for this problem coming back. Maybe give it a few RPS runs first or something?
src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public Assembly LoadFromPath(string fullPath) | ||
| => throw new InvalidOperationException(); |
There was a problem hiding this comment.
Consider putting a "This test should not be loading analyzer assemblies" so if somebody doesn't know what this is it'll be a bit clearer.
| // Load all analyzers into a fresh shadow copied load context. In the future, if we want to support reloading | ||
| // of solution-level analyzer references, we should just need to listen for changes to those analyzer paths and | ||
| // then call back into this method to update hte solution accordingly. | ||
| var analyzerLoader = loaderProvider.CreateNewShadowCopyLoader(); |
There was a problem hiding this comment.
Fine with me. Will have to reload VSCode to reload sln level analyzers, but those should be way way less common than project based ones.
src/Workspaces/Core/Portable/Workspace/IsolatedAnalyzerReferenceSet.Core.cs
Show resolved
Hide resolved
…stemProject.cs Co-authored-by: Jason Malinowski <[email protected]>
src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs
Outdated
Show resolved
Hide resolved
|
@CyrusNajmabadi Are they now never run in devenv and always run in ServiceHub.RoslynCodeAnalysisService? Does that mean they can start working in IDE and target net8.0+? |
No. Users can have their VS setup to run generators in proc.
That's not a decision for me. It's up to @jaredpar and the team. That said, nothing stops people from using our public APIs which load analzyer refs in proc. So as long as that's a supported scenario, then netstandard2 will need to be there. |
|
@Youssef1313: targeting net8.0 would still cause issues for anybody who won't have that change; at best this gets in to 17.12 so they'll be a lot of users that haven't upgraded. I don't imagine we'll be dropping netstandard use in the near future. |
Fixes #48083
This will allow reloading of them when they change on disk.