Add analyzer redirecting API #74820
Add analyzer redirecting API #74820
Conversation
jjonescz
commented
Aug 20, 2024
•
jjonescz
commented
- Part of Address analyzer and generator torn state with Visual Studio sdk#42087
- SDK counterpart: Add analyzer redirecting VSIX sdk#42861
dotnet-issue-labeler
bot
added
Area-Analyzers
untriaged
dotnet-policy-service
bot
added
the
Needs API Review
jjonescz
changed the title
IAnalyzerAssemblyResolver public API
jjonescz
changed the title
| { | ||
| redirectedPath = currentlyRedirectedPath; | ||
| } | ||
| else if (redirectedPath != currentlyRedirectedPath) |
There was a problem hiding this comment.
Given this is a path lets use string.Equals with an explicit comparer.
There was a problem hiding this comment.
== is equivalent to ordinal comparison; so did you mean to use ignore-case ordinal comparison, or just be more explicit?
| } | ||
| else if (redirectedPath != currentlyRedirectedPath) | ||
| { | ||
| throw new InvalidOperationException($"Multiple redirectors disagree on the path to redirect '{fullPath}' to ('{redirectedPath}' vs '{currentlyRedirectedPath}')."); |
There was a problem hiding this comment.
The catch below means this exception never manifests correct? It's just a Watson report mechanism?
There was a problem hiding this comment.
Yes, we don't want to continue if there's a mismatch, but we probably shouldn't crash either.
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Diagnostics.Redirecting; |
There was a problem hiding this comment.
Why did we put this into a separate namespace?
There was a problem hiding this comment.
So that we can use RestrictedInternalsVisibleTo to make this (and only this) namespace visible to the SDK counterpart.
| namespace Microsoft.CodeAnalysis.Diagnostics.Redirecting; | ||
|
|
||
| /// <summary> | ||
| /// Any MEF component implementing this interface will be used to redirect analyzer assemblies. |
There was a problem hiding this comment.
It feels a bit off to be mentioning MEF at the compiler layer. True this is shared code but still feels a bit off. Maybe we should move this to the remarks section?
Also .... are there any other cases in the compiler layer where interfaces are MEF imported in workspaces? Basically an analogous example to this? I'm considering if we should go this way or possibly make workspaces define a new interface that is specifically for MEF importing then uses an adapter to bridge to this.
There was a problem hiding this comment.
are there any other cases in the compiler layer where interfaces are MEF imported in workspaces?
IAnalyzerAssemblyResolver is exactly like that (it's what I based IAnalyzerAssemblyRedirector on). I don't know about any others.
|
|
||
| static string getAssemblyLocation(string fullPath, IAnalyzerAssemblyLoader assemblyLoader) | ||
| { | ||
| // We use reflection because AnalyzerAssemblyLoader can come from a different assembly (it's source-shared). |
There was a problem hiding this comment.
What is the case where AnalyzerFileReference doesn't have access to IAnalyzerAssemblyLoader members that are internal?
There was a problem hiding this comment.
AnalyzerFileReference doesn't have access to AnalyzerAssemblyLoader.RedirectAssemblyPathExternally because there are two AnalyzerAssemblyLoaders:
- one in Microsoft.CodeAnalysis, that one is fine and AnalyzerFileReference can access it,
- one in Microsoft.CodeAnalysis.Workspaces (it's a different type, only the source code is shared), this is the one actually passed here as the parameter
assemblyLoader, but AnalyzerFileReference cannot access it
