Enable hosts to provide custom assembly resolution #73185

chsienki · 2024-04-22T21:49:41Z

Today, in Roslyn we have some Razor specific override code that when the razor assembly is requested, we redirect it to the generator supplied by the tooling. As part of the razor cohosting effort, we need a way of not only changing the path, but the actual instance of the assembly that is loaded, so that we can return one from a different ALC/AppDomain rather than loading a second copy.

Rather than adding on more razor specific code, this PR adds a new feature where a host can provide an IAnalyzerAssemblyResolver implementation that allows the host to intercept the assembly load and provide its own implementation of the given assembly.

Nothing here is public, and currently we only provide a single, razor specific resolver, which Razor tooling can interact with via the existing external access mechanism, but we could potentially open this up to other hosts in the future should the need arise.

The code to remove the specific loading code will be removed later on, as we need to land the Razor side of this too, but they can co-exist: Roslyn will override the path, but when the Razor side is present it will then get the option to override it.

Note this code also doesn't yet expose this to Omnisharp, so we will need to make changes to support VSCode to use this mechanism, but we also have extra Razor specific code to handle that, so nothing will break by taking this change.

…mblies

- Add MEF imports for the resolvers and pass them into the loader - Add empty resolver arrays for varios test scenarios

chsienki · 2024-04-22T21:51:41Z

@dotnet/roslyn-compiler for review please.

@dotnet/roslyn-ide as this adds MEF imports to the top-level assembly loaders.

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

RikkiGibson · 2024-04-24T21:35:12Z

src/Compilers/Core/CodeAnalysisTest/InvokeUtil.cs

-                AnalyzerTestKind.LoadDirect => new DefaultAnalyzerAssemblyLoader(compilerContext, AnalyzerLoadOption.LoadFromDisk),
-                AnalyzerTestKind.LoadStream => new DefaultAnalyzerAssemblyLoader(compilerContext, AnalyzerLoadOption.LoadFromStream),
-                AnalyzerTestKind.ShadowLoad => new ShadowCopyAnalyzerAssemblyLoader(compilerContext, tempRoot.CreateDirectory().Path),
+                AnalyzerTestKind.LoadDirect => new DefaultAnalyzerAssemblyLoader(compilerContext, AnalyzerLoadOption.LoadFromDisk, externalResolvers.ToImmutableArray()),


micro-nit: it feels reasonable for these APIs to take ImmutableArray if this input array is invariably going to be copied to immutable.

You can't because this call is used via .net remoting which doesn't support ImmutableArray :(

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

RikkiGibson · 2024-04-24T22:23:09Z

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

+    [Export(typeof(IAnalyzerAssemblyResolver)), Shared]
+    internal class RazorAnalyzerAssemblyResolver : IAnalyzerAssemblyResolver
+    {
+        internal static Func<AssemblyName, Assembly?>? AssemblyResolver;


In the past, we have had difficulty shipping updates to EA due to need to load newer Roslyn into old versions of Razor. e.g. #72129.

It seems useful to limit the surface which we expose through the EA as much as possible, in order to give ourselves more flexibiltiy to change. In that vein I think we should consider a few options like the following:

Don't declare this type on the Roslyn side, have Razor declare it instead, and take ownership of the shape of it. (I am assuming the MEF composition will still work, I could be wrong.)

Declare a property instead of a field here, in case we ever want further customization of behavior which cannot be provided by a field.

I am assuming the MEF composition will still work, I could be wrong

That's the kicker here. In OOP the razor and roslyn MEF catalogs don't have any visibility to each other. The usual pattern would be to declare the concrete type in Roslyn, along with an external access interface that Razor provides the impl of via MEF, but because of the above restriction we can't.

I frankly hate this static, it's an ugly hack, but it is at least limited to the contract between razor and Roslyn.

More than happy to make the EA contract more resilient to changes if we can.

I wonder if we might be ok in this instance though, this particular bit of EA should only be used by the razor tooling, which (I think?) should always be sim-shipping with roslyn. That might not be true for VSCode/devkit though, will need to find that out.

In the past, we have had difficulty shipping updates to EA due to need to load newer Roslyn into old versions of Razor. e.g. #72129.

The Razor EA problems I've been aware of have all been for the EA layer that shipped with the compiler. That lesson we learned from that is an EA layer that can see the compiler is a compiler DLL. It must be shipped next to the compiler in all deployments and loaded from that location. That was very problematic because it meant that every host of the compiler (workspaces, VS, VS code, etc ...) had to know to deploy + load that DLL.

My understanding is that this EA assembly is in the VS / VS Code layer. That is a much more limited set of hosts and they have already solved the problem of making sure the EA assembly is deployed correctly.

I frankly hate this static, it's an ugly hack, but it is at least limited to the contract between razor and Roslyn.

My biggest worry is that there is a load order dependency here. Essentially this static must be set before the assembly resolve happens. If it happens the other way then we just silently fail here.

At the least can we add some guards to tell us if this happened in the wrong order? Basically if the static is set after the resolve event for razor occurs that should be a red flag. At least a Debug.Assert failure.

src/Compilers/Core/CodeAnalysisTest/AnalyzerAssemblyLoaderTests.cs

src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj

jaredpar · 2024-04-25T17:40:19Z

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

@@ -42,15 +44,16 @@ internal sealed class ShadowCopyAnalyzerAssemblyLoader : AnalyzerAssemblyLoader
        internal int CopyCount => _mvidPathMap.Count;

 #if NETCOREAPP
-        public ShadowCopyAnalyzerAssemblyLoader(string baseDirectory)
-            : this(null, baseDirectory)
+        public ShadowCopyAnalyzerAssemblyLoader(string baseDirectory, ImmutableArray<IAnalyzerAssemblyResolver>? externalResolvers = null)


I would consider making the reslovers non-optional here. Virtually every code path is already passing these values. For the two remaining they can simply use , [] in the ctor.

After this change having the right set of external resolvers is an important part of this type being correct hence I would prefer to avoid making this optional.

jaredpar · 2024-04-25T17:50:38Z

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

@@ -56,6 +58,11 @@ public bool IsHostAssembly(Assembly assembly)

        private partial Assembly? Load(AssemblyName assemblyName, string assemblyOriginalPath)
        {
+            if (ResolveAssemblyExternally(assemblyName) is { } externallyResolvedAssembly)


Think this needs to go after EnsureReslovedHooked. Consider the case where:

myanalyzer.dll and util.dll are passed via /analyzer:

myanalyzer.dll depends on util.dll such that it's required when roslyn reads types from the assembly

There is a resolver that hooks myanalyzer.dll but nothing else

This version of the code would end up throwing. That is because the load from myanalyzer.dll would come from an external location. Normal assembly resolution won't find util.dll (becuase the resolver didn't load myanalyzer.dll from a place that had it). The AssemblyResolve method would find it but it won't run because resolution hasn't been hooked.

I can't manage to construct a test in a way that allows us to show this behavior, but I think I can convince myself that hooking first is the correct order.

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

jaredpar · 2024-04-25T18:46:08Z

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

+    [Export(typeof(IAnalyzerAssemblyResolver)), Shared]
+    internal class RazorAnalyzerAssemblyResolver : IAnalyzerAssemblyResolver
+    {
+        internal static Func<AssemblyName, Assembly?>? AssemblyResolver;


In the past, we have had difficulty shipping updates to EA due to need to load newer Roslyn into old versions of Razor. e.g. #72129.

The Razor EA problems I've been aware of have all been for the EA layer that shipped with the compiler. That lesson we learned from that is an EA layer that can see the compiler is a compiler DLL. It must be shipped next to the compiler in all deployments and loaded from that location. That was very problematic because it meant that every host of the compiler (workspaces, VS, VS code, etc ...) had to know to deploy + load that DLL.

My understanding is that this EA assembly is in the VS / VS Code layer. That is a much more limited set of hosts and they have already solved the problem of making sure the EA assembly is deployed correctly.

I frankly hate this static, it's an ugly hack, but it is at least limited to the contract between razor and Roslyn.

My biggest worry is that there is a load order dependency here. Essentially this static must be set before the assembly resolve happens. If it happens the other way then we just silently fail here.

At the least can we add some guards to tell us if this happened in the wrong order? Basically if the static is set after the resolve event for razor occurs that should be a red flag. At least a Debug.Assert failure.

- Add extra tests - Update hookResolve order - Add asserts in RazorAnalyzerAssemblyLoader - Add doc comments

sharwell · 2024-04-29T15:22:33Z

It's not clear to me why we would need to create a new ALC when we already create a new ALC for each directory containing an assembly. Is Razor trying to load more than one assembly with the same name from the same folder, but with different contents?

jaredpar · 2024-04-29T21:29:45Z

It's not clear to me why we would need to create a new ALC when we already create a new ALC for each directory containing an assembly

This change doesn't result in any new ALC getting created. Instead it just changes which of the existing ALC loads a given assembly.

Is Razor trying to load more than one assembly with the same name from the same folder, but with different contents?

The issue is service hub. Today Razor and Roslyn exist in different service hub entries which in turn means they end up in different ALC. The problem is that Microsoft.CodeAnalysis.Razor.Compiler.dll has two different load paths:

It's part of the Razor ALC by virtue of Service Hub construction.
It's loaded into the Roslyn ALC by name because it's just another source generator entry and thus has an /analyzer reference.

This change is attempting to unify the two copies of M.C.R.C to be just (1). This is very similar to how the VS IDE unifies it by picking the copy shipped in VS.

Other avenues are being discussed on how to make this more of a first class Service Hub concept but they're not likely to manifest solutions anytime soon.

RikkiGibson

Please link any issues, when/if they are created, which are requesting a long-term solution for the underlying issue motivating this change. Thanks

dibarbet

The code to remove the specific loading code will be removed later on, as we need to land the Razor side of this too, but they can co-exist: Roslyn will override the path, but when the Razor side is present it will then get the option to override it.

Is this the code you're referring to that we can remove later on?
https://github.com/dotnet/roslyn/blob/main/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs#L1004

chsienki · 2024-05-09T23:30:52Z

@dibarbet Yep, thats the code.

chsienki added 4 commits April 22, 2024 11:42

Allow a host to override how the AnalyzerAssemblyLoader resolves asse…

b157716

…mblies

Add tests

e9b1a25

Fixup workspaces layer

706bea9

- Add MEF imports for the resolvers and pass them into the loader - Add empty resolver arrays for varios test scenarios

Add a specific Razor assembly resolver in external accces

55af5cf

chsienki requested review from a team as code owners April 22, 2024 21:49

chsienki added the Area-Compilers label Apr 22, 2024

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 22, 2024

chsienki added the Area-Analyzers label Apr 22, 2024

chsienki added 2 commits April 22, 2024 16:22

Fix top level imports

ebb7260

Formatting

7c8ef76

jaredpar reviewed Apr 23, 2024

View reviewed changes

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

Make compiler assembly loading a resolver in core loader

30ab5d4

chsienki force-pushed the analyzer_loading_interception branch from 13781c6 to 30ab5d4 Compare April 24, 2024 18:50

RikkiGibson self-assigned this Apr 24, 2024

Remove unused alc parameter

5544942

RikkiGibson reviewed Apr 24, 2024

View reviewed changes

jaredpar self-assigned this Apr 25, 2024

Remove uncessary default value from ctor

8aa64f0

jaredpar reviewed Apr 25, 2024

View reviewed changes

PR Feedback:

7ae01a3

- Add extra tests - Update hookResolve order - Add asserts in RazorAnalyzerAssemblyLoader - Add doc comments

jaredpar approved these changes Apr 26, 2024

View reviewed changes

RikkiGibson approved these changes May 8, 2024

View reviewed changes

dibarbet approved these changes May 9, 2024

View reviewed changes

chsienki merged commit 095bde2 into dotnet:main May 9, 2024
28 checks passed

dotnet-policy-service bot added this to the Next milestone May 9, 2024

dotnet-bot mentioned this pull request May 14, 2024

[Automated] PRs inserted in VS build main-34913.43 #73460

Closed

dotnet-bot mentioned this pull request May 21, 2024

[Automated] PRs inserted in VS build feature.debugger.main-34920.221 #73610

Closed

Cosifne modified the milestones: Next, 17.11 P2 May 28, 2024

dotnet-bot mentioned this pull request Jun 27, 2024

[Automated] PRs inserted in VS build feature.dotnetVS-35026.97 #74176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable hosts to provide custom assembly resolution #73185

Enable hosts to provide custom assembly resolution #73185

chsienki commented Apr 22, 2024

chsienki commented Apr 22, 2024

RikkiGibson Apr 24, 2024

chsienki Apr 25, 2024

RikkiGibson Apr 24, 2024

chsienki Apr 25, 2024

chsienki Apr 25, 2024

jaredpar Apr 25, 2024

jaredpar Apr 25, 2024

jaredpar Apr 25, 2024

chsienki Apr 26, 2024

jaredpar Apr 25, 2024

sharwell commented Apr 29, 2024

jaredpar commented Apr 29, 2024

RikkiGibson left a comment

dibarbet left a comment

chsienki commented May 9, 2024

Enable hosts to provide custom assembly resolution #73185

Enable hosts to provide custom assembly resolution #73185

Conversation

chsienki commented Apr 22, 2024

chsienki commented Apr 22, 2024

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

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

sharwell commented Apr 29, 2024

jaredpar commented Apr 29, 2024

RikkiGibson left a comment

Choose a reason for hiding this comment

dibarbet left a comment

Choose a reason for hiding this comment

chsienki commented May 9, 2024