Skip to content

Commit

Permalink
Fix analyzer loading to prefer host (#66492)
Browse files Browse the repository at this point in the history
* Fix analyzer loading to prefer host

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

Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>
Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>
  • Loading branch information
Jared Parsons and cston authored Jan 30, 2023
1 parent 5a91423 commit db52ae3
Show file tree
Hide file tree
Showing 16 changed files with 874 additions and 744 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@

namespace Microsoft.CodeAnalysis.UnitTests
{
[CollectionDefinition(Name)]
public class AssemblyLoadTestFixtureCollection : ICollectionFixture<AssemblyLoadTestFixture>
{
public const string Name = nameof(AssemblyLoadTestFixtureCollection);
private AssemblyLoadTestFixtureCollection() { }
}

[Collection(AssemblyLoadTestFixtureCollection.Name)]
public class AnalyzerFileReferenceTests : TestBase
{
Expand Down
865 changes: 577 additions & 288 deletions src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs

Large diffs are not rendered by default.

This file was deleted.

Loading

0 comments on commit db52ae3

Please sign in to comment.