Skip to content

Commit

Permalink
Fix analyzer consistency checks (#64831)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@jaredpar
jaredpar authored Oct 20, 2022
1 parent 36121e1 commit 158ba3a
Show file tree
Hide file tree
Showing 3 changed files with 121 additions and 32 deletions.
25 changes: 24 additions & 1 deletion src/Compilers/Server/VBCSCompiler/AnalyzerConsistencyChecker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#if NETFRAMEWORK

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
Expand Down Expand Up @@ -90,13 +92,32 @@ private static bool CheckCore(
}

// Third, check that the MVIDs of the files on disk match the MVIDs of the loaded assemblies.
var comparer = PathUtilities.Comparer;
var compilerDirectory = Path.GetDirectoryName(typeof(AnalyzerConsistencyChecker).Assembly.CodeBase);

for (int i = 0; i < resolvedPaths.Count; i++)
{
var resolvedPath = resolvedPaths[i];
var loadedAssembly = loadedAssemblies[i];

// When an assembly is loaded from the GAC then the load result would be the same if
// this ran on command line compiler. So there is no consistency issue here, this
// is just runtime rules expressing themselves.
if (loadedAssembly.GlobalAssemblyCache)
{
continue;
}

// When an assembly is loaded from the compiler directory then this means it's assembly
// binding redirects taking over. For example it's moving from an older version of System.Memory
// to the one shipping in the compiler. This is not a consistency issue.
if (PathUtilities.Comparer.Equals(compilerDirectory, Path.GetDirectoryName(loadedAssembly.CodeBase)))
{
continue;
}

var resolvedPathMvid = AssemblyUtilities.ReadMvid(resolvedPath);
var loadedAssemblyMvid = loadedAssembly.ManifestModule.ModuleVersionId;

if (resolvedPathMvid != loadedAssemblyMvid)
{
var message = $"analyzer assembly '{resolvedPath}' has MVID '{resolvedPathMvid}' but loaded assembly '{loadedAssembly.FullName}' has MVID '{loadedAssemblyMvid}'";
Expand All @@ -109,3 +130,5 @@ private static bool CheckCore(
}
}
}

#endif
11 changes: 4 additions & 7 deletions src/Compilers/Server/VBCSCompiler/CompilerRequestHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,6 @@ internal CompilerServerHost(string clientDirectory, string sdkDirectory, ICompil
Logger = logger;
}

private bool CheckAnalyzers(string baseDirectory, ImmutableArray<CommandLineAnalyzerReference> analyzers, [NotNullWhen(false)] out List<string>? errorMessages)
{
return AnalyzerConsistencyChecker.Check(baseDirectory, analyzers, AnalyzerAssemblyLoader, Logger, out errorMessages);
}

public bool TryCreateCompiler(in RunRequest request, BuildPaths buildPaths, [NotNullWhen(true)] out CommonCompiler? compiler)
{
switch (request.Language)
Expand Down Expand Up @@ -141,16 +136,18 @@ public BuildResponse RunCompilation(in RunRequest request, CancellationToken can
return new RejectedBuildResponse(message);
}

bool utf8output = compiler.Arguments.Utf8Output;
if (!CheckAnalyzers(request.WorkingDirectory, compiler.Arguments.AnalyzerReferences, out List<string>? errorMessages))
#if NETFRAMEWORK
if (!AnalyzerConsistencyChecker.Check(request.WorkingDirectory, compiler.Arguments.AnalyzerReferences, AnalyzerAssemblyLoader, Logger, out List<string?> errorMessages))
{
Logger.Log($"Rejected: {request.RequestId}: for analyzer load issues {string.Join(";", errorMessages)}");
return new AnalyzerInconsistencyBuildResponse(new ReadOnlyCollection<string>(errorMessages));
}
#endif

Logger.Log($"Begin {request.RequestId} {request.Language} compiler run");
try
{
bool utf8output = compiler.Arguments.Utf8Output;
TextWriter output = new StringWriter(CultureInfo.InvariantCulture);
int returnCode = compiler.Run(output, cancellationToken);
var outputString = output.ToString();
Expand Down
117 changes: 93 additions & 24 deletions src/Compilers/Server/VBCSCompilerTests/AnalyzerConsistencyCheckerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#if NETFRAMEWORK
#nullable disable

using System;
Expand Down Expand Up @@ -43,6 +44,39 @@ public AnalyzerConsistencyCheckerTests(ITestOutputHelper testOutputHelper, Assem
TestFixture = testFixture;
}

private TempFile CreateNetStandardDll(TempDirectory directory, string assemblyName, string version, ImmutableArray<byte> publicKey, string? extraSource = null)
{
var source = $$"""
using System;
using System.Reflection;

[assembly: AssemblyVersion("{{version}}")]
[assembly: AssemblyFileVersion("{{version}}")]
""";

var sources = extraSource is null
? new[] { CSharpTestSource.Parse(source) }
: new[] { CSharpTestSource.Parse(source), CSharpTestSource.Parse(extraSource) };

var options = new CSharpCompilationOptions(
OutputKind.DynamicallyLinkedLibrary,
warningLevel: Diagnostic.MaxWarningLevel,
cryptoPublicKey: publicKey,
deterministic: true,
publicSign: true);

var comp = CSharpCompilation.Create(
assemblyName,
sources,
references: NetStandard20.All,
options: options);

var file = directory.CreateFile($"{assemblyName}.dll");
var emitResult = comp.Emit(file.Path);
Assert.Empty(emitResult.Diagnostics.Where(x => x.Severity == DiagnosticSeverity.Error));
return file;
}

[Fact]
public void MissingReference()
{
Expand Down Expand Up @@ -73,25 +107,66 @@ public void DifferingMvids()
{
var directory = Temp.CreateDirectory();

// Load Beta.dll from the future Alpha.dll path to prime the assembly loader
var alphaDll = directory.CopyFile(TestFixture.Beta.Path, name: "Alpha.dll");
var key = NetStandard20.netstandard.GetAssemblyIdentity().PublicKey;
var mvidAlpha1 = CreateNetStandardDll(directory.CreateDirectory("mvid1"), "MvidAlpha", "1.0.0.0", key, "class C { }");
var mvidAlpha2 = CreateNetStandardDll(directory.CreateDirectory("mvid2"), "MvidAlpha", "1.0.0.0", key, "class D { }");

// Can't use InMemoryAssemblyLoader because that uses the None context which fakes paths
// to always be the currently executing application. That makes it look like everything
// is in the same directory
var assemblyLoader = new DefaultAnalyzerAssemblyLoader();
var analyzerReferences = ImmutableArray.Create(
new CommandLineAnalyzerReference(mvidAlpha1.Path),
new CommandLineAnalyzerReference(mvidAlpha2.Path));

var assemblyLoader = new InMemoryAssemblyLoader();
var betaAssembly = assemblyLoader.LoadFromPath(alphaDll.Path);
var result = AnalyzerConsistencyChecker.Check(directory.Path, analyzerReferences, assemblyLoader, Logger);
Assert.False(result);
}

// now overwrite the {directory}/Alpha.dll file with the content from our Alpha.dll test resource
alphaDll.CopyContentFrom(TestFixture.Alpha.Path);
directory.CopyFile(TestFixture.Gamma.Path);
directory.CopyFile(TestFixture.Delta1.Path);
/// <summary>
/// A differing MVID is okay when it's loading a DLL from the compiler directory. That is
/// considered an exchange type. For example if an analyzer has a reference to System.Memory
/// it will always load the copy the compiler used and that is not a consistency issue.
/// </summary>
[Fact]
[WorkItem(64826, "https://github.com/dotnet/roslyn/issues/64826")]
public void LoadingLibraryFromCompiler()
{
var directory = Temp.CreateDirectory();
var dllFile = CreateNetStandardDll(directory, "System.Memory", "2.0.0.0", NetStandard20.netstandard.GetAssemblyIdentity().PublicKey);

// This test must use the DefaultAnalyzerAssemblyLoader as we want assembly binding redirects
// to take affect here.
var assemblyLoader = new DefaultAnalyzerAssemblyLoader();
var analyzerReferences = ImmutableArray.Create(
new CommandLineAnalyzerReference("Alpha.dll"),
new CommandLineAnalyzerReference("Gamma.dll"),
new CommandLineAnalyzerReference("Delta.dll"));
new CommandLineAnalyzerReference("System.Memory.dll"));

var result = AnalyzerConsistencyChecker.Check(directory.Path, analyzerReferences, assemblyLoader, Logger);

Assert.False(result);
Assert.True(result);
}

/// <summary>
/// A differing MVID is okay when it's loading a DLL from the GAC. There is no reason that
/// falling back to csc would change the load result.
/// </summary>
[Fact]
[WorkItem(64826, "https://github.com/dotnet/roslyn/issues/64826")]
public void LoadingLibraryFromGAC()
{
var directory = Temp.CreateDirectory();
var dllFile = directory.CreateFile("System.Core.dll");
dllFile.WriteAllBytes(NetStandard20.Resources.SystemCore);

// This test must use the DefaultAnalyzerAssemblyLoader as we want assembly binding redirects
// to take affect here.
var assemblyLoader = new DefaultAnalyzerAssemblyLoader();
var analyzerReferences = ImmutableArray.Create(
new CommandLineAnalyzerReference("System.Core.dll"));

var result = AnalyzerConsistencyChecker.Check(directory.Path, analyzerReferences, assemblyLoader, Logger);

Assert.True(result);
}

[Fact]
Expand All @@ -109,22 +184,15 @@ public void AssemblyLoadException()
}

[Fact]
public void NetstandardIgnored()
public void LoadingSimpleLibrary()
{
var directory = Temp.CreateDirectory();
const string name = "netstandardRef";
var comp = CSharpCompilation.Create(
name,
new[] { CSharpTestSource.Parse("class C {}") },
references: new MetadataReference[] { NetStandard20.netstandard },
options: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary, warningLevel: Diagnostic.MaxWarningLevel));
var compFile = directory.CreateFile(name);
comp.Emit(compFile.Path);
var key = NetStandard20.netstandard.GetAssemblyIdentity().PublicKey;
var compFile = CreateNetStandardDll(directory, "netstandardRef", "1.0.0.0", key);

var analyzerReferences = ImmutableArray.Create(new CommandLineAnalyzerReference(compFile.Path));

var analyzerReferences = ImmutableArray.Create(new CommandLineAnalyzerReference(name));

var result = AnalyzerConsistencyChecker.Check(directory.Path, analyzerReferences, new InMemoryAssemblyLoader(), Logger);
var result = AnalyzerConsistencyChecker.Check(directory.Path, analyzerReferences, new DefaultAnalyzerAssemblyLoader(), Logger);

Assert.True(result);
}
Expand Down Expand Up @@ -152,3 +220,4 @@ public Assembly LoadFromPath(string fullPath)
}
}
}
#endif

0 comments on commit 158ba3a

Please sign in to comment.