Simplify and unify assembly loader provider #74895
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal sealed class VSAnalyzerAssemblyLoaderProvider( | ||
| [ImportMany] IEnumerable<IAnalyzerAssemblyResolver> externalResolvers) | ||
| : AbstractAnalyzerAssemblyLoaderProvider(externalResolvers.ToImmutableArray()); |
There was a problem hiding this comment.
we don't need this type. It was just exporting for the 'host' layer using the default behavior. But we don't need this since we already have a 'default' exported impl of IAnalyzerAssemblyLoaderProvider
| /// Abstract implementation of an anlyzer assembly loader that can be used by VS/VSCode to provide a | ||
| /// <see cref="IAnalyzerAssemblyLoader"/> with an appropriate path. | ||
| /// </summary> | ||
| internal abstract class AbstractAnalyzerAssemblyLoaderProvider : IAnalyzerAssemblyLoaderProvider |
There was a problem hiding this comment.
this code moved to be next to IAnalyzerAssemblyLoaderProvider
| [ExportWorkspaceServiceFactory(typeof(IAnalyzerAssemblyLoaderProvider)), Shared] | ||
| [method: ImportingConstructor] | ||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal sealed class DefaultAnalyzerAssemblyLoaderServiceFactory( |
There was a problem hiding this comment.
this code moved to be next to IAnalyzerAssemblyLoaderProvider. That's a standard pattern we have for workspace services. We have hte interface, and the default impl next to it.
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal sealed class DefaultAnalyzerAssemblyLoaderService( | ||
| [ImportMany] IEnumerable<IAnalyzerAssemblyResolver> externalResolvers) | ||
| : AbstractAnalyzerAssemblyLoaderProvider(externalResolvers.ToImmutableArray()); |
There was a problem hiding this comment.
Note: this default impl is what everything (except VSCode) uses. It is slightly different from the original logic in that it now just subclasses AbstractAnalyzerAssemblyLoaderProvider to pick up the logic from that.
| new(AbstractAnalyzerAssemblyLoaderProvider.GetDefaultShadowCopyPath(), externalResolvers.ToImmutableArray()); | ||
|
|
||
| public IAnalyzerAssemblyLoaderInternal GetShadowCopyLoader() | ||
| => _shadowCopyLoader; |
There was a problem hiding this comment.
we don't need a special REmoteWorkspace impl. The default impl does this work already properly.
| => new DefaultAnalyzerAssemblyLoaderProvider(this, workspaceServices.Workspace.Kind ?? "Default", _externalResolvers); | ||
|
|
||
| protected virtual IAnalyzerAssemblyLoaderInternal WrapLoader(IAnalyzerAssemblyLoaderInternal loader) | ||
| => loader; |
There was a problem hiding this comment.
no-op behavior. The VSCode version overrides this to customize behavior.
|
|
||
| public static string GetDefaultShadowCopyPath() | ||
| => Path.Combine(Path.GetTempPath(), "Roslyn", "AnalyzerAssemblyLoader"); | ||
| } |
There was a problem hiding this comment.
I'm a little confused by this comment. _workspaceKind is still factoring into the path passed into WrapLoader, no?
| => _shadowCopyLoader.Value; | ||
|
|
||
| private IAnalyzerAssemblyLoaderInternal CreateShadowCopyLoader() | ||
| => _factory.WrapLoader(DefaultAnalyzerAssemblyLoader.CreateNonLockingLoader( |
There was a problem hiding this comment.
There was a problem hiding this comment.
I mean, it's an abstract impl. It's sharing the logic we need for our two subclasses. That doesn't bother me.
yup. i got my branches mixed and hadn't pushed everythign properly. Good to go again @ToddGrun |
|
|
||
| private IAnalyzerAssemblyLoaderInternal CreateShadowCopyLoader() | ||
| => this.WrapLoader(DefaultAnalyzerAssemblyLoader.CreateNonLockingLoader( | ||
| Path.Combine(Path.GetTempPath(), nameof(Roslyn), "AnalyzerAssemblyLoader"), |
There was a problem hiding this comment.
It looks like we're now always using the same path now for VS/OOP - which I think is fine? I assume in VS we only make shadow copies in the remote workspace now? So devenv won't shadow copy analyzers and conflict with the remote workspace any more?
There was a problem hiding this comment.
correct. every shadow copy loader already puts their work in a unique guid-named subdirectory of the base directory passed in. So there's no conflicts ever across instances (within the same process, or other process, etc.). THey are always independent.
I've been touching this layer a lot in the reloadable-SG space, and this strange hierarchy has been bothering me majorly. This cleans things up a ton here.