microsoft / vs-mef

Managed Extensibility Framework (MEF) implementation used by Visual Studio
MIT License
417 stars 87 forks source link

Support generic imports (GenericHost/Options DI) #457

Open weltkante opened 4 months ago

weltkante commented 4 months ago

We've had Generic Host for a while now and I've been looking into using vs-mef as DI instead of the inbox implementation, to integrate better with existing codebases which already are MEF based. I've made good progress mapping the builder abstractions and constructing a dynamic vs-mef catalog from ServiceDescriptors during startup, but when I got to the options infrastructure of .NET Core I'm running into the problem that they adopted generic imports, something vs-mef doesn't support yet.

An example for exports added to the builder is here:

services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptions<>), typeof(UnnamedOptionsManager<>)));
services.TryAdd(ServiceDescriptor.Scoped(typeof(IOptionsSnapshot<>), typeof(OptionsManager<>)));
services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitor<>), typeof(OptionsMonitor<>)));
services.TryAdd(ServiceDescriptor.Transient(typeof(IOptionsFactory<>), typeof(OptionsFactory<>)));
services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitorCache<>), typeof(OptionsCache<>)));

So .NET is creating generic exports, which is fine, vs-mef supports that. However when we look ad e.g. UnnamedOptionsManager<TOptions> we can see its imports:

public UnnamedOptionsManager(IOptionsFactory<TOptions> factory)

i.e. the import depends on the generic parameter, which is something vs-mef does not support because the TypeRef infrastructure has no strategy to deal with it and just throws.

Conceptually I believe it should be fine if a generic export imports another generic export with a matching parameter? So the question is if there is any desire to have this support added to vs-mef.

AArnott commented 4 months ago

It's an interesting idea, but we have no plans to support this at this point. Open generic exports are easier to support because the generic type parameter can be ignored as it doesn't impact the composition graph. Generic imports on the other hand require a different representation in the composition for every anticipated type argument. I suppose supporting it is possible, but it's more difficult.

If I assume that System.ComponentModel.Composition's MEF engine supported this, I'd argue it's easier because they don't keep a static graph -- theirs is dynamic. They only figure out what a MEF part needs when it's activated. And they do it again the next time it's activated. It's why that MEF engine is so slow. VS-MEF is much faster at runtime because we compute the graph once and reuse it. But that means any dynamic element is unsupported.

I believe a dynamic component wouldn't be necessary to support this, now that I think about it. But I don't see us funding the work anytime in the future unless it became critical for a VS feature.

weltkante commented 4 months ago

If I assume that System.ComponentModel.Composition's MEF engine supported this [...]

I actually didn't consider checking System.ComponentModel.Composition but a quick test with mocked exports/imports to replicate the scenario seems to indicate it works there.

I believe a dynamic component wouldn't be necessary to support this, now that I think about it.

I don't believe this is any more dynamic than the existing open generic exports and this is mostly a question how to represent these dependencies. (That doesn't mean its easy, its probably a significant challenge, generics unfortunately aren't trivial.)

[...] we have no plans to support this at this point [...] I don't see us funding the work anytime in the future unless it became critical for a VS feature.

I assume that also means contributions of this kind are currently not desired either, as they likely will cause additional effort required to review and align with design of the library?

This was basically my main question, whether there's any interest in exploring how to add this feature, not necessarily asking you to provide it. I understand that it may be disruptive to the workings of the library and require effort for feedback/communications, so if this kind of high effort contributions are currently not desired I perfectly understand. Just wanted to ask before I invest any work myself, so far I have just identified the problem and no idea how a solution would look like.

Feel free to close as "not planned" or similar in this case, and thanks for the feedback 👍

AArnott commented 4 months ago

I think we'd take the feature if you offered a pull request that met the requirements we have for all our new features. Most importantly:

  1. No backward breaking changes.
  2. Thorough automated tested through the testing framework we already have in place.
  3. It can't regress performance for existing scenarios.

I bet we even have a test or two for this feature already, but disabled for the VS-MEF engine. So part of the work would be to look through our VS-MEF disabled tests to see if any apply that should be enabled. That would be easier than writing new tests, though depending on the coverage these offered, additional tests may (probably will) be needed.

Thanks for thinking about it!