Skip to content
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

Analyzer consistency check should not consider MVID on .NET Core #64826

Closed
jaredpar opened this issue Oct 19, 2022 · 0 comments · Fixed by #64831
Closed

Analyzer consistency check should not consider MVID on .NET Core #64826

jaredpar opened this issue Oct 19, 2022 · 0 comments · Fixed by #64831
Assignees
Milestone

Comments

@jaredpar
Copy link
Member

jaredpar commented Oct 19, 2022

An invariant of the compiler server is to produce the same results that would come from running csc / vbc directly. Analyzers are one place where it is hard to maintain this invariant. That is because the compiler server has to consider the totality of analyzers used for an entire build while csc / vbc just has to consider the analyzers for a specific project. Issues like address space pollution are an issue the server has to consider but csc / vbc can ignore.

One concrete case where this is a problem is when analyzer dependencies conflict. This is best illustrated with an example. Consider a solution which consisted of the following projects:

  • Project1.csproj
    • Uses Analyzer1 which has a dependency of Util.dll at version 1.0.0
  • Project2.csproj
    • Uses Analyzer2 which has a dependency of Util.dll at version 2.0.0

If these projects are build by csc / vbc both analyzers get their own clean address space and each load the correct version of Util.dll. In the compiler server though there are many cases which could cause one or the other to get the wrong copy of Util.dll and end up producing incorrect results. The easiest case is when the server runs on .NET Framework,. Util.dll is not strong named signed and then it's just a case of first load wins.

The analyzer consistency check in the compiler server was designed to spot cases where analyzer dependencies conflict and pre-emptively drop back to csc / vbc. This is done by just by loading all analyzer dependencies into the process, then comparing the MVID of the loaded DLL with the DLL on disk. If they're different then the compiler makes the conservative decision that we've hit address space pollution issues and drops back to csc / vbc.

This check is necessary but a few recent cases have shown the heuristic is too aggressive. The most common situation we've seen is when analyzers use Span<T> which brings System.Memory as a dependency. That will always be the netstandard2.0 version of System.Memory as analyzers must build against netstandard2.0. At the same time though it means the MVID check will always fail because at runtime we either load the net472 or netcoreapp version of System.Memory. Hence such analyzers, which are using Span<T> for perf, end up causing their compilations to always fall back to csc / vbc which is decidedly not performant.

The current analysis needs to be changed in the following ways:

  1. The consistency check should not run on .NET Core because each analyzer goes into it's own AssemblyLoadContext. That means there is no cross anlayzer pollution anymore.
  2. The consistency check should not consider DLLs that are loaded from the GAC or from the compiler directory. Such DLLs can reasonably be assumed to be a part of the compiler runtime. Falling back to csc / vbc will not change the load outcome.
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 19, 2022
@jaredpar jaredpar added Area-Compilers Bug and removed Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 19, 2022
@jaredpar jaredpar self-assigned this Oct 19, 2022
@jaredpar jaredpar added this to the 17.5 milestone Oct 19, 2022
jaredpar added a commit to jaredpar/roslyn that referenced this issue Oct 19, 2022
Issue dotnet#64826 outlines the core problem the analyzer consistency check is
hitting here and rationale for changing it. In short though this changes
our analyzer consistency check in the following ways:

1. It no longer applies on .NET Core. Analyzers get their own
   `AssemblyLoadContext` hence dependency consistency is not an issue
2. Do not fail consistency check when a dependency is loaded from the
   GAC
3. Do not fail consistency check when a dependency is loaded from the
   compiler directory

closes dotnet#64826
jaredpar added a commit that referenced this issue Oct 20, 2022
Issue #64826 outlines the core problem the analyzer consistency check is
hitting here and rationale for changing it. In short though this changes
our analyzer consistency check in the following ways:

1. It no longer applies on .NET Core. Analyzers get their own
   `AssemblyLoadContext` hence dependency consistency is not an issue
2. Do not fail consistency check when a dependency is loaded from the
   GAC
3. Do not fail consistency check when a dependency is loaded from the
   compiler directory

closes #64826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant