Fix analyzer loading to prefer host #66492

jaredpar · 2023-01-19T22:27:42Z

This changes the design of our analyzer loading policy to give hosts more control over dependencies. Whenever possible analyzer dependencies will be unified with the compiler host environment where previously we only tried to unify dependencies owned directly by the compiler. This will be achieved by using standard assembly loading practices that give the host first shot at determining an Assembly to load for a given AssemblyName.

For a concrete example of this problem consider some analyzers deploy copies of System.Memory.dll in their analyzer folder. That means there are two copies of System.Memory.dll in play: the one the compiler deploys and the analyzer copy. The compiler prefers its copy because so exchange types unify to a single place.

This won't change the designed behavior for the command line compilers. In those scenarios the compiler is the host and the intent compiler dependencies to be unified (this PR does fix a bug in this area). It will make the implementation more rigorous though.

This will potentially change our behavior when loaded in hosts such as Visual Studio. That environment has a much larger set of DLLs that can be loaded into the primary load context. This is important though to ensure propery type / dll unification happens where as today it can result in duplicate types and assemblies.

This change also substantially updates our tests. Most of our tests were focused on the default loading strategy (load in place, no shadow copies). That is actually the least common loading strategy. The most common is shadow copy loading. As such I revamped our test suite such that the same set of tests run for all four of our main configurations:

default + .NET Core
default + .NET Framework
shadow + .NET Core
shadow + .NET Framework

The results are illuminating. There are more differences between these scenarios than most people, likely, suspect. Later changes may try and unify a few scenarios but for now wanted to mostly document the behavior so it's not accidentally changed.

closes #66104
closes #60763
https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1692992

jaredpar · 2023-01-19T22:27:57Z

@dotnet/roslyn-compiler PTAL, @RikkiGibson

jaredpar · 2023-01-19T22:30:43Z

src/Compilers/Core/CodeAnalysisTest/ShadowCopyAnalyzerAssemblyLoaderTests.cs

@@ -1,146 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.


These tests were merged into DefaultAnalyzerAssemblyLoaderTestsBase. That way they run on the four major configurations that we care about.

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerAssemblyLoader.cs

src/Compilers/Core/Portable/Collections/DictionaryExtensions.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

RikkiGibson · 2023-01-23T18:51:03Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

+                }
+
+                bool isMatch;
+#if NETCOREAPP


I am noticing that more logic is being pushed up into the base. I am wondering if it would make sense to eliminate the distinction between the AnalyzerAssemblyLoader and DefaultAnalyzerAssemblyLoader, and simply use partial methods instead, where we stub some partial methods in this class and implement them in the .Core.cs and .Desktop.cs partials. Obviously, things that need to be virtual for ShadowCopyAnalyzerAssemblyLoader would remain virtual.

It's a sensible idea, I'm not sure I want to lump it into this PR though. It will cause fairly significant rename on top of all the churn already present here. May want to push that off to a follow up so I can get this bug fix in.

I went ahead and spiked this out here

jaredpar@c3fa753

I agree we should do this but I'm leery to add it to the current PR. Feel like it's complicated enough. Think it's better to get this merged then follow up with that change quickly after. Open to suggestions.

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

RikkiGibson · 2023-01-23T22:27:26Z

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs

    {
-#if NETCOREAPP
-        private sealed class InvokeUtil
+        public void Exec(Action<string> testOutputHelper, AssemblyLoadContext alc, bool shadowLoad, string typeName, string methodName)


Consider renaming alc to something like compilerContext

cleaned this up in the next PR where i fixed up names quite a bit.

RikkiGibson · 2023-01-23T22:29:16Z

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs

+            }
+            finally
+            {
+                testOutputHelper($"Test fixture root: {fixture.TempDirectory.Path}");


It wasn't clear to me why we use this action instead of passing the ITestOutputHelper itself and calling WriteLine.

It's complicated ...

The way the AssemblyLoadContext are setup today there are two copies of ITestOutputHelper because XUnit.Abstractions.dll is loaded twice: once in the default ALC and once in the custom ALC. That means if you try and pass ITestOutputHelper directly here you get a type mismatch at runtime because they are different types.

To pass it around I need to move all of the xUnit loading into the default ALC. That is a bit challenging given how it's setup. I'm trying experiments to do that but may push that off into a follow up PR.

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs

RikkiGibson · 2023-01-23T23:55:20Z

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs


-                try
+                if (loader is ShadowCopyAnalyzerAssemblyLoader)


|| !ExecutionConditionUtil.IsWindows?

BTW, is there any reason to use the shadow copy loader on non-Windows platforms? Is the usage of it conditioned accordingly?

I think it can be but need to do more digging. Yes we can delete the files today but is that an accident of implementation or intentional design decision? Also are there other side effects we're not thinking of here? Have to discuss a bit with runtime team.

Will add it to the list of follow ups in #66532.

RikkiGibson · 2023-01-24T05:47:09Z

The Linux test run crashed when running the test DefaultAnalyzerAssemblyLoaderTests.AssemblyLoading_DependencyInDifferentDirectory.

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-roslyn-refs-pull-66492-merge-7a66d1718b8a4600b6/Microsoft.CodeAnalysis.Rebuild.UnitTests_Microsoft.CodeAnalysis.Scripting.UnitTests_Microsoft.CodeAnalysis.UnitTests_34/1/console.d7424587.log?helixlogtype=result

It looks like there are dumps but might be most straightforward to try and repro on a linux machine locally.

jaredpar · 2023-01-27T18:58:55Z

Sorry, I know we have a lot of active convos happening here but I need to rebase and force push. Until I do this change is going to be blocked by the Correctness build bug in our infra.

jcouv

LGTM Thanks (iteration 11)

This changes the design of our analyzer loading policy to give hosts more control over dependencies. Whenever possible analyzer dependencies will be unified with the compiler host environment where previously we only tried to unify dependencies owned directly by the compiler. This will be achieved by using standard assembly loading practices that give the host first shot at determining an `Assembly` to load for a given `AssemblyName`. For a concrete example of this problem consider some analyzers deploy copies of `System.Memory.dll` in their analyzer folder. That means there are two copies of `System.Memory.dll` in play: the one the compiler deploys and the analyzer copy. The compiler prefers its copy because so exchange types unify to a single place. This won't change the designed behavior for the command line compilers. In those scenarios the compiler is the host and the intent compiler dependencies to be unified (this PR does fix a bug in this area). It will make the implementation more rigorous though. This will potentially change our behavior when loaded in hosts such as Visual Studio. That environment has a much larger set of DLLs that can be loaded into the primary load context. This is important though to ensure propery type / dll unification happens where as today it can result in duplicate types and assemblies. This change also substantially updates our tests. Most of our tests were focused on the default loading strategy (load in place, no shadow copies). That is actually the least common loading strategy. The most common is shadow copy loading. As such I revamped our test suite such that the same set of tests run for all four of our main configurations: - default + .NET Core - default + .NET Framework - shadow + .NET Core - shadow + .NET Framework The results are illuminating. There are more differences between these scenarios than most people, likely, suspect. Later changes may try and unify a few scenarios but for now wanted to mostly document the behavior so it's not accidentally changed. closes dotnet#66104 closes dotnet#60763 https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1692992

Co-authored-by: Charles Stoner <[email protected]>

This change responds to several concerns raised in dotnet#66492: - Collapse the `AnalyzerAssembyLoader` hierarchy by one level and move more logic into the base type. - Fix `AssemblyLoadContext` in tests to not replicate types. That means we can safely pass `ITestOutputHelper` into the custom `AssemblyLoadContext` - Further strengthen the test separation I have deliberately not changed the file names to reflect the new type names because I wanted the review process to be smooth. Once I have two sign offs for the change I will send another commit which does the following: - Rename - DefaultAnalyzerAssemblyLoaderTests.cs -> AnalyzerAssemblyLoaderTests.cs - DefaultAnalyzerAssemblyLoader.Core.cs -> AnalyzerAssemblyLoader.Core.cs - DefaultAnalyzerAssemblyLoader.Desktop.cs -> AnalyzerAssemblyLoader.Desktop.cs - DefaultAnalyzerAssemblyLoader.cs -> AnalyzerAssemblyLoader.cs - Move - `DefaultAnalyzerAssemblyLoader` into DefaultAnalyzerAssemblyLoader.cs - `InvokeUtil` to InvokeUtil.cs. As constructed every edit to this type causes Test Explorer to lose context on the theory branches. Doesn't come back until rebuild. Will be smoother editting in its own file. That should get is into the style we expect here. After that I will merge once tests are green.

The invariant that our `AnalyzerAssemblyLoader` depends on is the underlying file system is static. Once the contents of an assembly are read off of disk, even if the content is changed and re-registered, the loader will make no attempt to load the new assembly The change in dotnet#66492 missed this part of the contract. Upon a new registration via `AddDependencyLocation` it would reset the data for the location and allow new loads to occur. This caused a spike in shadow load copies during VS testing and regressed RPS. This change restores the previous copy behavior and adds test to ensure it's not regressed again.

* Reduce copies in shadow load The invariant that our `AnalyzerAssemblyLoader` depends on is the underlying file system is static. Once the contents of an assembly are read off of disk, even if the content is changed and re-registered, the loader will make no attempt to load the new assembly The change in #66492 missed this part of the contract. Upon a new registration via `AddDependencyLocation` it would reset the data for the location and allow new loads to occur. This caused a spike in shadow load copies during VS testing and regressed RPS. This change restores the previous copy behavior and adds test to ensure it's not regressed again. * PR feedback

This change responds to several concerns raised in dotnet#66492: - Collapse the `AnalyzerAssembyLoader` hierarchy by one level and move more logic into the base type. - Fix `AssemblyLoadContext` in tests to not replicate types. That means we can safely pass `ITestOutputHelper` into the custom `AssemblyLoadContext` - Further strengthen the test separation I have deliberately not changed the file names to reflect the new type names because I wanted the review process to be smooth. Once I have two sign offs for the change I will send another commit which does the following: - Rename - DefaultAnalyzerAssemblyLoaderTests.cs -> AnalyzerAssemblyLoaderTests.cs - DefaultAnalyzerAssemblyLoader.Core.cs -> AnalyzerAssemblyLoader.Core.cs - DefaultAnalyzerAssemblyLoader.Desktop.cs -> AnalyzerAssemblyLoader.Desktop.cs - DefaultAnalyzerAssemblyLoader.cs -> AnalyzerAssemblyLoader.cs - Move - `DefaultAnalyzerAssemblyLoader` into DefaultAnalyzerAssemblyLoader.cs - `InvokeUtil` to InvokeUtil.cs. As constructed every edit to this type causes Test Explorer to lose context on the theory branches. Doesn't come back until rebuild. Will be smoother editting in its own file. That should get is into the style we expect here. After that I will merge once tests are green.

* Continued analyzer assembly loading work This change responds to several concerns raised in #66492: - Collapse the `AnalyzerAssembyLoader` hierarchy by one level and move more logic into the base type. - Fix `AssemblyLoadContext` in tests to not replicate types. That means we can safely pass `ITestOutputHelper` into the custom `AssemblyLoadContext` - Further strengthen the test separation I have deliberately not changed the file names to reflect the new type names because I wanted the review process to be smooth. Once I have two sign offs for the change I will send another commit which does the following: - Rename - DefaultAnalyzerAssemblyLoaderTests.cs -> AnalyzerAssemblyLoaderTests.cs - DefaultAnalyzerAssemblyLoader.Core.cs -> AnalyzerAssemblyLoader.Core.cs - DefaultAnalyzerAssemblyLoader.Desktop.cs -> AnalyzerAssemblyLoader.Desktop.cs - DefaultAnalyzerAssemblyLoader.cs -> AnalyzerAssemblyLoader.cs - Move - `DefaultAnalyzerAssemblyLoader` into DefaultAnalyzerAssemblyLoader.cs - `InvokeUtil` to InvokeUtil.cs. As constructed every edit to this type causes Test Explorer to lose context on the theory branches. Doesn't come back until rebuild. Will be smoother editting in its own file. That should get is into the style we expect here. After that I will merge once tests are green. * Unload AssemblyLoadContext after test completion * Reuse AssemblyLoadTestFixture across test runs This also has the added benefit that the entire .NET Core set of tests run in less than one second. Previously it close to half a second per test. This will hopefully alleviate the pain for the runtime issue we're hittnig running `AssemblyLoadContext` tests in CI. This reduces the amount of compilations on this code path which seem to be the trigger for the failure. - #66621 - dotnet/runtime#81108 * Apply suggestions from code review Co-authored-by: Rikki Gibson <[email protected]> * PR feedback * rename the files * Fixup --------- Co-authored-by: Rikki Gibson <[email protected]>

jaredpar added the Area-Compilers label Jan 19, 2023

jaredpar requested a review from a team as a code owner January 19, 2023 22:27

RikkiGibson self-assigned this Jan 19, 2023

jaredpar commented Jan 19, 2023

View reviewed changes

RikkiGibson reviewed Jan 19, 2023

View reviewed changes

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed Jan 19, 2023

View reviewed changes

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed Jan 19, 2023

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed Jan 19, 2023

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed Jan 19, 2023

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs Show resolved Hide resolved

RikkiGibson reviewed Jan 19, 2023

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerAssemblyLoader.cs Outdated Show resolved Hide resolved

jaredpar requested a review from a team as a code owner January 20, 2023 00:35

sharwell reviewed Jan 20, 2023

View reviewed changes

src/Compilers/Core/Portable/Collections/DictionaryExtensions.cs Outdated Show resolved Hide resolved

jaredpar force-pushed the load2 branch from 1fa3ba1 to 91fb269 Compare January 23, 2023 17:07