-
Notifications
You must be signed in to change notification settings - Fork 173
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
fix-analyzers throwing error. #734
Comments
hmm, we may need to add some more complex logic to handle duplicate analyzer assemblies being loaded. |
@jmarolf Wouldn't a |
I guess the question is: What does the user want us to do? There are two different analyzers at play which may find different issues. try/catching just means the behavior for format is non-deterministic |
Which analyzer is failing? |
Since we can't load analyzers side-by-side on .NET Core (and we know dotnet-format is running on .NET Core, we should be using an assembly load context for the analyzers which allows them to be loaded even when there is more than one analyzer with the same name in the solution. |
I agree that assembly load context is likely the going to be the correct solution here |
I'm not very familiar with dynamic assembly loading. But is using AssemblyLoadContext is to replace: var assemblies = solution.Projects
.SelectMany(project => project.AnalyzerReferences.Select(reference => reference.FullPath))
.Distinct()
.Select(path => Assembly.LoadFrom(path)); with var context = new AssemblyLoadContext();
var assemblies = solution.Projects
.SelectMany(project => project.AnalyzerReferences.Select(reference => reference.FullPath))
.Distinct()
.Select(path => context.LoadFromAssemblyPath(path)); or I'm misunderstanding things? |
The easiest way would be either one ALC for each analyzer directory, or one ALC for each project. Going by analyzer directory puts the most power in the hands of each analyzer author to prepare a package with the necessary dependencies. |
One per project is likely the correct design moving forward |
@jmarolf, Something like that? var assemblies = new HashSet<Assembly>();
foreach (var project in solution.Projects)
{
var context = new AssemblyLoadContext();
set.AddRange(project.AnalyzerReferences.Select(reference => context.LoadFromAssemblyPath(reference.FullPath)));
}
return AnalyzerFinderHelpers.LoadAnalyzersAndFixers(assemblies); |
I'd definitely second that approach. As a workaround I've made sure that I've consolidated all nuget packages to the same version across the solution which has solved the problem, however this may not always be possible for all. |
We could do a better job of Analyzer isolation if we could use the AssemblyDependencyResolver, but that type was introduced in 3.1. https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblydependencyresolver?view=netcore-3.1&viewFallbackFrom=netcore-2.1 That being said, we can certainly use a separate AssemblyLoadContext. |
I believe this is fixed in version 5.0 |
When running the latest development build. --version 5.0.136601
It runs through until it hits, Running Formatters then throws an error. Fix-style is working fine.
Running formatters.
Unhandled exception: System.IO.FileLoadException: Assembly with same name is already loaded
at System.Runtime.Loader.AssemblyLoadContext.LoadFromPath(IntPtr ptrNativeAssemblyLoadContext, String ilPath, String niPath, ObjectHandleOnStack retAssembly)
at System.Runtime.Loader.AssemblyLoadContext.LoadFromAssemblyPath(String assemblyPath)
at System.Reflection.Assembly.LoadFrom(String assemblyFile)
at Microsoft.CodeAnalysis.Tools.Analyzers.AnalyzerReferenceInformationProvider.<>c.b__0_1(String path) in /_/src/Analyzers/AnalyzerReferenceInformationProvider.cs:line 28
The full set of analyzers returned are:
And the distinct list:
The text was updated successfully, but these errors were encountered: