Run analyzer consistency checks on .NET Core #70517
Conversation
dotnet-issue-labeler
bot
added
Area-Analyzers
untriaged
| @@ -13,6 +13,15 @@ | |||
|
|
|||
| namespace Microsoft.CodeAnalysis | |||
| { | |||
| internal interface IAnalyzerAssemblyLoaderInternal : IAnalyzerAssemblyLoader | |||
There was a problem hiding this comment.
IAnalyzerAssemblyLoader is a public API and did not want to create new public API for this hence went with an internal API instead.
There was a problem hiding this comment.
We don't expect external users (at least those who lack IVT) to implement IAnalyzerAssemblyLoader, right?
There was a problem hiding this comment.
Correct. This is only used in the compiler server where external teams cannot provide implementations.
|
|
||
| private class InMemoryAssemblyLoader : IAnalyzerAssemblyLoader |
There was a problem hiding this comment.
This type muddied the waters a bit on how analyzer loading reacts in consistency checks because it doesn't load analyzers the same way as the actual implementations. So removed this and flipped to using the real implementations in order to make sure the tests are accurate.
|
@dotnet/roslyn-compiler PTAL |
|
@RikkiGibson, @jjonescz PTAL |
| @@ -13,6 +13,15 @@ | |||
|
|
|||
| namespace Microsoft.CodeAnalysis | |||
| { | |||
| internal interface IAnalyzerAssemblyLoaderInternal : IAnalyzerAssemblyLoader | |||
There was a problem hiding this comment.
We don't expect external users (at least those who lack IVT) to implement IAnalyzerAssemblyLoader, right?
| @@ -21,7 +30,7 @@ namespace Microsoft.CodeAnalysis | |||
| /// This type generally assumes that files on disk aren't changing, since it ensure that two calls to <see cref="LoadFromPath(string)"/> | |||
| /// will always return the same thing, per that interface's contract. | |||
| /// </remarks> | |||
| internal abstract partial class AnalyzerAssemblyLoader : IAnalyzerAssemblyLoader | |||
| internal abstract partial class AnalyzerAssemblyLoader : IAnalyzerAssemblyLoaderInternal | |||
There was a problem hiding this comment.
I was a little surprised I didn't see any changes which type-test or cast to IAnalyzerAssemblyLoaderInternal. I thought we had public API which permits users to pass in instances of IAnalyzerAssemblyLoader in order to use a common analyzer loader across compilations.
There was a problem hiding this comment.
You're correct that we have end points that provide IAnalyzerAssemblyLoader parameters. This particular API is only used in the compiler server where we do not provide a public API end point. It's not a re-hostable component but one where we provide all the implementations.
Pretty much the only reason that the compiler server takes IAnalyzerAssemblyLoader and not AnalyzerAssemblyLoader is that it limits the API surface area and it's easier to provide throwing implementations in unit tests so we can validate behavior better.
| @@ -91,27 +111,15 @@ internal static class AnalyzerConsistencyChecker | |||
| loadedAssemblies.Add(loader.LoadFromPath(resolvedPath)); | |||
| } | |||
|
|
|||
| // Third, check that the MVIDs of the files on disk match the MVIDs of the loaded assemblies. | |||
There was a problem hiding this comment.
Looks like this comment applies to the whole for loop below and should not be removed.
| { | ||
| // 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. |
There was a problem hiding this comment.
It's a bit weird that the API is named IsHostAssembly but the comments are talking about "consistency" - they made sense when inside the consistency checker but here are bit out of context. But I guess it's fine since it's an internal API (and has just one caller - the consistency checker).
ghost
added this to the 
The job of
AnalyzerConsistencyCheckeris to determine when the compiler server has loaded analyzers in a bad state and force a restart. One of the bad states it detects is when the analyzers on disk change. That is not handled byAnalyzerAssemblyLoaderbecause it has an invariant that the file system is unchanged. This was not running for .NET Core so this particular scenario was missed. This change updatesAnalyzerConsistencyCheckerto run on .NET Core too to catch these issues.closes #70053