Add analyzer redirecting API #74820

jjonescz · 2024-08-20T13:41:00Z

Part of Address analyzer and generator torn state with Visual Studio sdk#42087
SDK counterpart: Add analyzer redirecting VSIX sdk#42861
Design: Add analyzer redirecting proposal sdk#42437 (markdown)

jaredpar · 2024-10-22T12:04:59Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

+                            {
+                                redirectedPath = currentlyRedirectedPath;
+                            }
+                            else if (redirectedPath != currentlyRedirectedPath)


Given this is a path lets use string.Equals with an explicit comparer.

== is equivalent to ordinal comparison; so did you mean to use ignore-case ordinal comparison, or just be more explicit?

jaredpar · 2024-10-22T12:05:45Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

+                            }
+                            else if (redirectedPath != currentlyRedirectedPath)
+                            {
+                                throw new InvalidOperationException($"Multiple redirectors disagree on the path to redirect '{fullPath}' to ('{redirectedPath}' vs '{currentlyRedirectedPath}').");


The catch below means this exception never manifests correct? It's just a Watson report mechanism?

Yes, we don't want to continue if there's a mismatch, but we probably shouldn't crash either.

jaredpar · 2024-10-22T12:06:41Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/IAnalyzerAssemblyRedirector.cs

+// 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;


Why did we put this into a separate namespace?

So that we can use RestrictedInternalsVisibleTo to make this (and only this) namespace visible to the SDK counterpart.

jaredpar · 2024-10-22T12:09:28Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/IAnalyzerAssemblyRedirector.cs

+namespace Microsoft.CodeAnalysis.Diagnostics.Redirecting;
+
+/// <summary>
+/// Any MEF component implementing this interface will be used to redirect analyzer assemblies.


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.

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.

jaredpar · 2024-10-22T12:10:50Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerFileReference.cs

+
+            static string getAssemblyLocation(string fullPath, IAnalyzerAssemblyLoader assemblyLoader)
+            {
+                // We use reflection because AnalyzerAssemblyLoader can come from a different assembly (it's source-shared).


What is the case where AnalyzerFileReference doesn't have access to IAnalyzerAssemblyLoader members that are internal?

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

jaredpar · 2025-01-09T22:48:33Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

@@ -77,6 +78,8 @@ internal abstract partial class AnalyzerAssemblyLoader : IAnalyzerAssemblyLoader
        /// <see cref="Assembly"/> winning.</remarks>
        private readonly ImmutableArray<IAnalyzerAssemblyResolver> _externalResolvers;

+        private readonly ImmutableArray<IAnalyzerAssemblyRedirector> _externalRedirectors;


Have a layering question. The high level view of this change is that when Visual Studio is loading analyzers / generators it is able to redirect from paths in the .NET SDK to paths inside of Visual Studio. Basically just change the path that we are loading from.

Why are we doing this work in the compiler vs. say the workspaces layer? That seems like the more natural fit to my eye. Is there a reason that it needs to be done here instead?

Note: appologies cause I think we talked about this before but it's been paged out of my brain.

Note that this file is actually source-shared between Compiler and Workspaces.

But yes, we could implement this somewhere else in the IDE layers, outside these shared components. I've tried that at first, but it was difficult (more so as I'm unfamiliar with the IDE codebase).

The current implementation changes AnalyzerFileReference.FullPath to the redirected one. And AnalyzerFileReference.FullPath is then used by everything else, so we are basically done.

If this was done in IDE, we would keep the FullPath pointing to the original path and then we would need to change the IDE to know about redirecting, e.g.,

the assembly loaders,

Solution Explorer analyzer UI,

some data serialization / checksums also needed changing IIRC,

maybe more?

(And doing this in the compiler doesn't seem bad? Theoretically, the same redirecting could be done by the command line compiler if we ever wanted that for some reason.)

I think it's reasonable for the IDE to know about redirecting as they are the ones that actually do it. It also seems like it should be reflected in the IDE that they are loading the VS copy of analyzers vs. it being hidden by the compiler.

Hm, I can move this into ProjectSystemProject.GetMappedAnalyzerPaths. That's what CodeStyle analyzer redirecting uses as well. I thought @chsienki said we don't want to use that API, but now I think the reason was only that Razor needs to do more complex assembly resolution which we don't need here.

I'm not sure if moving there will change anything functionally except layering, but I will try to do that to see what happens.

I think it's reasonable for the IDE to know about redirecting as they are the ones that actually do it. It also seems like it should be reflected in the IDE that they are loading the VS copy of analyzers vs. it being hidden by the compiler.

You mean it should be reflected in the UI somehow? Technically I think that's orthogonal to where this lives - even if it's in the compiler layer, it can expose some APIs which the IDE could read to see which analyzers were redirected.

Btw, IAnalyzerAssemblyResolver also lives in the compiler layer even though it's also meant to be used just by IDEs I think.

Tried locally moving the functionality into GetMappedAnalyzerPaths and it seems to work fine. I can push the changes tomorrow when I polish them a bit.

@jjonescz Would we be against updating the Resolver interface to support Razor's use case? I think moving CodeStyle and Razor redirection to this interface would be preferable.

Would we be against updating the Resolver interface to support Razor's use case?

IAnalyzerAssemblyResolver supports Razor's use case since it was added for Razor. Although it's true that Razor redirection currently lives in both the resolver interface and GetMappedAnalyzerPaths, not sure why, perhaps backcompat.

I think moving CodeStyle and Razor redirection to this interface would be preferable.

I guess CodeStyle could be moved to use the Resolver interface, not sure, but also seems unrelated to this PR.

Note that this PR adds another interface - IAnalyzerAssemblyRedirector - which is a bit different than the Resolver one - it comes from external VSIX via MEF, that's why I made it a separate interface.

The logic of both interfaces (Redirector and Resolver) could be similar - and indeed it was before this thread where Jared asked me to move the new one (Redirector) out of compiler layer. The Resolver still lives in the compiler layer - and it needs to because it intercepts assembly loading inside AssemblyLoadContext which currently is shared between compiler and IDE so technically lives in the compiler layer. Whereas Redirector only redirects paths, so it can live in IDE layer.

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerFileReference.cs

333fred · 2025-01-21T17:44:59Z

@RikkiGibson @jaredpar for reviews

jaredpar · 2025-01-23T21:21:59Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/IAnalyzerAssemblyRedirector.cs

+namespace Microsoft.CodeAnalysis.Workspaces.AnalyzerRedirecting;
+
+/// <summary>
+/// Any MEF component implementing this interface will be used to redirect analyzer assemblies.


Can we add a bit here that drives home this happens before shadow copy? Essentially this redirects the path that we pass to the compiler and the compiler will then do as it normally does with the path?

jaredpar · 2025-01-24T16:13:13Z

@dotnet/roslyn-ide, @jasonmalinowski PTAL

Add IAnalyzerAssemblyResolver public API

1a317da

dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 20, 2024

dotnet-policy-service bot added the Needs API Review Needs to be reviewed by the API review council label Aug 20, 2024

Load analyzers directly in ServiceHub

bebcf7d

dotnet-policy-service bot added the VSCode label Aug 26, 2024

Merge branch 'main' into analyzer-resolver-api

f108d19

jjonescz removed the VSCode label Aug 26, 2024

dotnet-policy-service bot added the VSCode label Aug 26, 2024

jjonescz added 2 commits August 27, 2024 10:36

Handle missing assembly on disk

659d124

Merge branch 'main' into analyzer-resolver-api

93b8e10

jjonescz removed the VSCode label Aug 27, 2024

jjonescz added 2 commits August 28, 2024 13:28

Merge branch 'main' into analyzer-resolver-api

9c68c45

Redirect path only for serialization

10f96ea

dotnet-policy-service bot added the VSCode label Aug 28, 2024

build-analysis bot mentioned this pull request Aug 28, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

Redirect full path on creation

7e3faba

jjonescz mentioned this pull request Aug 29, 2024

Add analyzer redirecting VSIX dotnet/sdk#42861

Open

jjonescz marked this pull request as ready for review August 29, 2024 16:32

jjonescz requested review from a team as code owners August 29, 2024 16:32

jjonescz mentioned this pull request Aug 30, 2024

Add analyzer redirecting proposal dotnet/sdk#42437

Draft

jjonescz changed the title ~~Add IAnalyzerAssemblyResolver public API~~ Add analyzer redirecting public API Aug 30, 2024

jjonescz added 6 commits August 30, 2024 14:40

Add separate redirector API

0f3bb48

Remove superfluous methods

1481640

Report caught exceptions

7f7eda4

Make the API RestrictedInternalsVisibleTo

74f9a76

Merge branch 'main' into analyzer-resolver-api

1261e70

Remove unnecessary resolver modification

61f96b6

jjonescz changed the title ~~Add analyzer redirecting public API~~ Add analyzer redirecting API Sep 11, 2024

chsienki mentioned this pull request Sep 27, 2024

Do not load CodeStyle analyzers added by the SDK #75250

Merged

jjonescz added 2 commits October 22, 2024 10:45

Merge branch 'main' into analyzer-resolver-api

6315300

Fixup after merge

d489573

jaredpar reviewed Oct 22, 2024

View reviewed changes

JoeRobich mentioned this pull request Nov 26, 2024

Load Roslyn Analyzers from VS #74941

Closed

jaredpar reviewed Jan 9, 2025

View reviewed changes

Merge branch 'main' into analyzer-resolver-api

23e0f47

RikkiGibson self-assigned this Jan 13, 2025

jjonescz added 2 commits January 14, 2025 11:40

Remove redirecting from AnalyzerFileReference

fbf85ed

Redirect analyzers in ProjectSystemProject

889d6fe

333fred approved these changes Jan 14, 2025

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerFileReference.cs Show resolved Hide resolved

jaredpar approved these changes Jan 23, 2025

View reviewed changes

Expand doc comment

bc99564

jasonmalinowski self-requested a review January 24, 2025 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add analyzer redirecting API #74820

Add analyzer redirecting API #74820

jjonescz commented Aug 20, 2024 •

edited

Loading

jaredpar Oct 22, 2024

jjonescz Oct 22, 2024

jaredpar Oct 22, 2024

jjonescz Oct 22, 2024

jaredpar Oct 22, 2024

jjonescz Oct 22, 2024 •

edited

Loading

jaredpar Oct 22, 2024

jjonescz Oct 22, 2024

jaredpar Oct 22, 2024

jjonescz Oct 22, 2024 •

edited

Loading

jaredpar Jan 9, 2025

jjonescz Jan 10, 2025 •

edited

Loading

jaredpar Jan 13, 2025

jjonescz Jan 13, 2025

jjonescz Jan 13, 2025

JoeRobich Jan 15, 2025

jjonescz Jan 15, 2025

333fred commented Jan 21, 2025

jaredpar Jan 23, 2025

jaredpar commented Jan 24, 2025

Add analyzer redirecting API #74820

Are you sure you want to change the base?

Add analyzer redirecting API #74820

Conversation

jjonescz commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jan 21, 2025

Choose a reason for hiding this comment

jaredpar commented Jan 24, 2025

jjonescz commented Aug 20, 2024 •

edited

Loading

jjonescz Oct 22, 2024 •

edited

Loading

jjonescz Oct 22, 2024 •

edited

Loading

jjonescz Jan 10, 2025 •

edited

Loading