Support generic imports. - #717
Conversation
|
@microsoft-github-policy-service agree |
Andrew Arnott (AArnott)
left a comment
Andrew Arnott (AArnott)
left a comment
There was a problem hiding this comment.
Thanks for sharing this. I think it's shaping up.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Ah, thats a test failure that slipped through my local tests, due to the way of it being reported. I'll have a look, seems to be a genuine missed case. |
|
ImportMany attributes left some generic parameters which caused issues roundtripping through a serialized container. I've reverted the logic in the idividual AttributedPartDiscovery branches and instead have it now directly in |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for “parameterized generic imports” where an open-generic part imports a constructed generic type whose type arguments are the part’s own generic parameters (e.g. OptionsManager<TOptions> importing IOptionsFactory<TOptions>), enabling common Generic Host / Options DI patterns (issue #457) to compose in VS MEF.
Changes:
- Introduces
GenericParameterIndexesimport metadata and runtime substitution to close parameterized generic imports using the importing part’s concrete type arguments. - Updates runtime import handling to compute effective closed import-site types for direct,
Lazy<T>,ExportFactory<T>, andImportManycollection forms. - Adds/extends tests (including cache/reload) and documentation describing supported generic import scenarios.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.VisualStudio.Composition.Tests/Reflection/TypeRefTests.cs | Loosens an assertion on TypeLoadException message contents. |
| test/Microsoft.VisualStudio.Composition.Tests/GenericImportTests.cs | Adds coverage for parameterized generic imports across direct/Lazy/ExportFactory/ImportMany shapes. |
| test/Microsoft.VisualStudio.Composition.Tests/CacheAndReloadTests.cs | Verifies parameterized generic imports survive cache save/load. |
| src/Microsoft.VisualStudio.Composition/RuntimeExportProviderFactory+RuntimeExportProvider.cs | Computes effective closed import-site types/metadata at runtime for parameterized generic imports. |
| src/Microsoft.VisualStudio.Composition/ReflectionHelpers.cs | Adjusts open→closed generic constructor mapping for importing constructors with generic-parameter-containing parameter types. |
| src/Microsoft.VisualStudio.Composition/Reflection/TypeRef.cs | Avoids constructing TypeRef generic arguments for generic parameters. |
| src/Microsoft.VisualStudio.Composition/PartDiscovery.cs | Emits GenericParameterIndexes metadata and avoids type-identity constraints for parameterized generic import contracts. |
| src/Microsoft.VisualStudio.Composition/CompositionConstants.cs | Adds the GenericParameterIndexes metadata name constant. |
| src/Microsoft.VisualStudio.Composition/ComposedPart.cs | Exempts parameterized generic imports from the “generic type parameters not supported” validation diagnostic. |
| docfx/docs/mef_library_differences.md | Documents supported generic import patterns and explicitly calls out unsupported “bare type parameter” imports. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (ctorIndex >= 0) | ||
| { | ||
| var closedCtors = closedGeneric.GetConstructors(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); | ||
| if (ctorIndex < closedCtors.Length) | ||
| { | ||
| return closedCtors[ctorIndex]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Interesting change, can't tell which is better, as far as I'm concerned both sound like implementation details that in practice always hold. But the direct token matching for sure is simpler and allows to get rid of the dual loop, so I'd adopt it if there is no reason against it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.VisualStudio.Composition/RuntimeExportProviderFactory+RuntimeExportProvider.cs:157
- This wrapper reconstruction assumes Lazy/ExportFactory have exactly one generic argument. VS MEF also supports Lazy<T, TMetadata> and ExportFactory<T, TMetadata> (see MetadataType logic), so MakeGenericType(effectiveElementType) will throw for those cases. Preserve any existing metadata type argument when rebuilding the wrapper.
This issue also appears on line 168 of the same file.
// ImportingSiteTypeWithoutCollection is the wrapper type: Lazy<IFoo<TOptions>> or ExportFactory<IFoo<TOptions>>
// Reconstruct it as Lazy<IFoo<MyOptions>> or ExportFactory<IFoo<MyOptions>>
effectiveImportSiteWithoutCollection = import.ImportingSiteTypeWithoutCollection
.GetGenericTypeDefinition()
.MakeGenericType(effectiveElementType);
src/Microsoft.VisualStudio.Composition/RuntimeExportProviderFactory+RuntimeExportProvider.cs:172
- When computing effectiveImportSiteType for parameterized generic imports, the code uses effectiveElementType (the unwrapped contract type). For ImportMany, the collection element type is ImportingSiteTypeWithoutCollection, which may be Lazy/ExportFactory-wrapped; using the unwrapped type can make createability/assignment checks incorrect for collection types like ICollection<Lazy<T, TMetadata>> and custom collections.
}
else if (outerType.IsGenericType)
{
effectiveImportSiteType = outerType.GetGenericTypeDefinition().MakeGenericType(effectiveElementType);
}
|
Hmm, that copilot comment brought up some cases that weren't handled. Added tests for them. Also the documentation changes I suggested earlier apparently didn't apply, so I reapplied them manually. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.VisualStudio.Composition/Reflection/TypeRef.cs:118
- TypeRef now unconditionally filters out generic-parameter type arguments. If a constructed generic type has a mix of generic parameters and concrete type arguments (e.g. Foo<T,int>), this will produce a TypeRef whose GenericTypeArguments count is less than GenericTypeParameterCount. Later, Resolve() will call MakeGenericType with too few arguments and throw an ArgumentException, which is a behavior regression vs the previous early rejection. Consider only applying the filtering when all type arguments are generic parameters (the supported “parameterized generic import” case), and explicitly reject partially-open generic types to avoid creating an invalid TypeRef state.
this.FullName = (arrayElementType.GetTypeInfo().IsGenericType ? arrayElementType.GetGenericTypeDefinition() : arrayElementType).FullName ?? throw Assumes.NotReachable();
this.GenericTypeParameterCount = arrayElementType.GetTypeInfo().GenericTypeParameters.Length;
this.GenericTypeArguments = arrayElementType.GenericTypeArguments != null && arrayElementType.GenericTypeArguments.Length > 0
? arrayElementType.GenericTypeArguments.Where(t => !t.IsGenericParameter).Select(t => new TypeRef(resolver, t, shallow: true)).ToImmutableArray()
: ImmutableArray<TypeRef>.Empty;
| if (!importingPartTracker.ImportMetadata.TryGetValue(CompositionConstants.GenericParametersMetadataName, out var outerTypeArgsObj) || outerTypeArgsObj is not Type[] outerTypeArgs) | ||
| { | ||
| return importMetadata; | ||
| } | ||
|
|
||
| Type[] closedTypeArgs = indexes.Select(i => outerTypeArgs[i]).ToArray(); | ||
| return ImmutableDictionary.CreateRange(importMetadata) | ||
| .SetItem(CompositionConstants.GenericParametersMetadataName, closedTypeArgs); |
There was a problem hiding this comment.
Seems to be a false positive and not reproducable. Quote:
LazyMetadataWrapper.TryGetValue runs SubstituteValueIfRequired with Direction.ToOriginalValue, so reads through the dictionary always hand back Type[]. The substitution is only observable if you deliberately call LazyMetadataWrapper.TryUnwrap first — which is exactly what GetPartConstructedTypeRef does (to avoid forcing type loads), and why that method needs its two-form handling. GetEffectiveImportMetadata doesn't unwrap, so is not Type[] never trips there.
Fixes #457
As mentioned, mostly AI generated. The approach makes sense to me. I left the original generated comments intact during cleanup since they might help understanding during review, if further cleanup or other refactoring is desired let me know and I'll work on that. If the approach isn't desireable thats fine too, I just wanted to get this is out of my backlog ;)