From d59aac47d19e3a8c1d421c85eca5222d6b05e078 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 3 Jul 2026 16:49:21 +0200 Subject: [PATCH 1/3] Transfer ownership in ReflectionMetadataHook.Register instead of cloning The source-gen Register hook defensively cloned every array and dictionary it received. The only sanctioned caller is the MSTest source generator's [ModuleInitializer], which emits fresh, throwaway collections it never mutates, so the copies guarded against a mutation that cannot happen. Store the passed arrays by reference and reuse caller dictionaries when already concrete (AsOwnedDictionary), only materializing exotic IReadOnlyDictionary implementations. The ConstructorInvokerInfo -> ConstructorInvoker projection remains (a representation change, not a defensive copy) but no longer clones parameter arrays. Documents the ownership-transfer contract on the hook. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ReflectionMetadataHook.cs | 99 +++++++++++-------- 1 file changed, 58 insertions(+), 41 deletions(-) diff --git a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs index cae9e3cf02..a54e8354d7 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs @@ -52,8 +52,8 @@ public static class ReflectionMetadataHook /// All types directly annotated with [TestClass] in the assembly. /// /// A map from each test class to its [TestMethod]-annotated - /// set. The dictionary and arrays are copied defensively; the caller may mutate the inputs - /// after the call. + /// set. Ownership transfers to the adapter: the source generator hands over freshly-built + /// collections and must not mutate them after the call (see the remarks on the full overload). /// /// /// Do not call this method from hand-written code; it is meant to be invoked exclusively from @@ -120,8 +120,19 @@ public static void Register( /// assigns it directly. /// /// + /// /// Do not call this method from hand-written code; it is meant to be invoked exclusively from /// the [ModuleInitializer] emitted by the MSTest source generator. + /// + /// + /// Ownership transfer. The adapter takes ownership of every collection passed in + /// (the array, the array, and + /// each dictionary with its value arrays) and stores them without cloning. The generator emits + /// fresh, throwaway collections for each call, so the caller MUST NOT mutate any of these + /// inputs after the call returns. This trades the previous defensive copies for zero-copy + /// startup, relying on the single trusted caller (the generator, versioned in lockstep with + /// this adapter) rather than defending a public surface that is documented as not for direct use. + /// /// [EditorBrowsable(EditorBrowsableState.Never)] public static void Register( @@ -174,29 +185,20 @@ public static void Register( throw new ArgumentNullException(nameof(propertySetters)); } - var typesCopy = (Type[])types.Clone(); + // Ownership transfer (see the remarks on this method): the source generator hands over + // freshly-built, throwaway collections and never mutates them after the call, so we store + // them directly instead of cloning. Arrays are kept by reference; dictionaries are reused + // as-is when already concrete and only materialized when the caller passed some other + // IReadOnlyDictionary implementation. + Dictionary testMethodsMap = AsOwnedDictionary(testMethods); + Dictionary typeAttributesMap = AsOwnedDictionary(typeAttributes); + Dictionary> methodInvokersMap = AsOwnedDictionary(methodInvokers); + Dictionary> propertySettersMap = AsOwnedDictionary(propertySetters); - var testMethodsCopy = new Dictionary(testMethods.Count); - foreach (KeyValuePair kvp in testMethods) - { - testMethodsCopy[kvp.Key] = (MethodInfo[])kvp.Value.Clone(); - } - - var typeAttributesCopy = new Dictionary(typeAttributes.Count); - foreach (KeyValuePair kvp in typeAttributes) - { - typeAttributesCopy[kvp.Key] = (Attribute[])kvp.Value.Clone(); - } - - object[] assemblyAttributesCopy = (object[])assemblyAttributes.Clone(); - - var methodInvokersCopy = new Dictionary>(methodInvokers.Count); - foreach (KeyValuePair> kvp in methodInvokers) - { - methodInvokersCopy[kvp.Key] = kvp.Value; - } - - var constructorInvokersCopy = new Dictionary(constructorInvokers.Count); + // ConstructorInvokerInfo (public struct) has to be projected onto the adapter's internal + // ConstructorInvoker type; this is a representation change, not a defensive copy, and the + // parameter-type arrays are taken by reference. + var constructorInvokersMap = new Dictionary(constructorInvokers.Count); foreach (KeyValuePair kvp in constructorInvokers) { var invokers = new SourceGeneratedReflectionDataProvider.ConstructorInvoker[kvp.Value.Length]; @@ -205,26 +207,20 @@ public static void Register( ConstructorInvokerInfo info = kvp.Value[i]; invokers[i] = new SourceGeneratedReflectionDataProvider.ConstructorInvoker { - Parameters = (Type[])info.ParameterTypes.Clone(), + Parameters = info.ParameterTypes, Invoker = info.Invoker, }; } - constructorInvokersCopy[kvp.Key] = invokers; - } - - var propertySettersCopy = new Dictionary>(propertySetters.Count); - foreach (KeyValuePair> kvp in propertySetters) - { - propertySettersCopy[kvp.Key] = kvp.Value; + constructorInvokersMap[kvp.Key] = invokers; } // TypesByName must always match Type.FullName at runtime (see comment in the source // generator emitter): compute it on the runtime side from typeof(T).FullName so the // generator emits less code and the same FullName conventions are honored for nested // and generic types. - var typesByName = new Dictionary(typesCopy.Length, StringComparer.Ordinal); - foreach (Type type in typesCopy) + var typesByName = new Dictionary(types.Length, StringComparer.Ordinal); + foreach (Type type in types) { if (type.FullName is { } fullName) { @@ -236,14 +232,14 @@ public static void Register( { Assembly = assembly, AssemblyName = assembly.GetName().Name ?? string.Empty, - Types = typesCopy, + Types = types, TypesByName = typesByName, - TypeMethods = testMethodsCopy, - TypeAttributes = typeAttributesCopy, - AssemblyAttributes = assemblyAttributesCopy, - TypeMethodInvokers = methodInvokersCopy, - TypeConstructorsInvoker = constructorInvokersCopy, - TypePropertySetters = propertySettersCopy, + TypeMethods = testMethodsMap, + TypeAttributes = typeAttributesMap, + AssemblyAttributes = assemblyAttributes, + TypeMethodInvokers = methodInvokersMap, + TypeConstructorsInvoker = constructorInvokersMap, + TypePropertySetters = propertySettersMap, }; lock (Lock) @@ -260,6 +256,27 @@ public static void Register( } } + // Reuses the caller-provided dictionary when it is already a concrete Dictionary<,> (the shape + // the source generator always emits), honoring the ownership-transfer contract with zero + // copying. Any other IReadOnlyDictionary implementation is materialized once so the provider + // still owns a concrete instance. Values are always taken by reference. + private static Dictionary AsOwnedDictionary(IReadOnlyDictionary source) + where TKey : notnull + { + if (source is Dictionary concrete) + { + return concrete; + } + + var copy = new Dictionary(source.Count); + foreach (KeyValuePair kvp in source) + { + copy[kvp.Key] = kvp.Value; + } + + return copy; + } + private static readonly Dictionary EmptyTypeAttributes = []; private static readonly Dictionary> EmptyMethodInvokers = []; From 9e1a1b55fb18e9b1faa60867808be405b703b349 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 3 Jul 2026 16:57:12 +0200 Subject: [PATCH 2/3] Reword ownership-transfer remarks to avoid implying a trust boundary The API is public and callable from any code; the contract is about ownership and mutation, not caller identity. Clarify accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../SourceGeneration/ReflectionMetadataHook.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs index a54e8354d7..fbe2e97b88 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs @@ -127,11 +127,12 @@ public static void Register( /// /// Ownership transfer. The adapter takes ownership of every collection passed in /// (the array, the array, and - /// each dictionary with its value arrays) and stores them without cloning. The generator emits - /// fresh, throwaway collections for each call, so the caller MUST NOT mutate any of these - /// inputs after the call returns. This trades the previous defensive copies for zero-copy - /// startup, relying on the single trusted caller (the generator, versioned in lockstep with - /// this adapter) rather than defending a public surface that is documented as not for direct use. + /// each dictionary with its value arrays) and stores them without cloning. Callers MUST hand + /// over freshly-built collections and MUST NOT mutate them after the call returns; the source + /// generator (the only intended caller) already emits fresh, throwaway collections that satisfy + /// this. This is a contract about ownership and mutation, not caller identity: it trades the + /// previous defensive copies for zero-copy startup on the understanding that the inputs are the + /// adapter's to keep. /// /// [EditorBrowsable(EditorBrowsableState.Never)] From 7fe90ef5ec33cf17d6f528b4c585ee596e324d30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 3 Jul 2026 17:11:19 +0200 Subject: [PATCH 3/3] Flow read-only dictionaries through instead of materializing them The provider is documented as an immutable metadata snapshot, so its dictionary members are now typed IReadOnlyDictionary<,>. Register stores the read-only inputs directly (true ownership transfer, zero copy), removing the AsOwnedDictionary shim; MergeInto widens its source parameter to IReadOnlyDictionary. Unit-test initializers that used indexer syntax now construct a concrete Dictionary explicitly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...teSourceGeneratedReflectionDataProvider.cs | 2 +- .../ReflectionMetadataHook.cs | 58 +++++-------------- .../SourceGeneratedReflectionDataProvider.cs | 22 +++---- ...ourceGeneratedReflectionOperationsTests.cs | 8 +-- 4 files changed, 32 insertions(+), 58 deletions(-) diff --git a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/CompositeSourceGeneratedReflectionDataProvider.cs b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/CompositeSourceGeneratedReflectionDataProvider.cs index b361211b2d..7dd61b80cd 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/CompositeSourceGeneratedReflectionDataProvider.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/CompositeSourceGeneratedReflectionDataProvider.cs @@ -222,7 +222,7 @@ private static SourceGeneratedReflectionDataProvider BuildMergedSnapshot(IReadOn }; } - private static void MergeInto(Dictionary target, Dictionary source) + private static void MergeInto(Dictionary target, IReadOnlyDictionary source) where TKey : notnull { foreach (KeyValuePair kvp in source) diff --git a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs index fbe2e97b88..7f5fcd5ef2 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs @@ -127,12 +127,13 @@ public static void Register( /// /// Ownership transfer. The adapter takes ownership of every collection passed in /// (the array, the array, and - /// each dictionary with its value arrays) and stores them without cloning. Callers MUST hand - /// over freshly-built collections and MUST NOT mutate them after the call returns; the source - /// generator (the only intended caller) already emits fresh, throwaway collections that satisfy - /// this. This is a contract about ownership and mutation, not caller identity: it trades the - /// previous defensive copies for zero-copy startup on the understanding that the inputs are the - /// adapter's to keep. + /// each dictionary with its value arrays) and stores them without cloning; the read-only + /// dictionaries are held as-is behind . Callers + /// MUST hand over freshly-built collections and MUST NOT mutate them after the call returns; + /// the source generator (the only intended caller) already emits fresh, throwaway collections + /// that satisfy this. This is a contract about ownership and mutation, not caller identity: it + /// trades the previous defensive copies for zero-copy startup on the understanding that the + /// inputs are the adapter's to keep. /// /// [EditorBrowsable(EditorBrowsableState.Never)] @@ -188,17 +189,11 @@ public static void Register( // Ownership transfer (see the remarks on this method): the source generator hands over // freshly-built, throwaway collections and never mutates them after the call, so we store - // them directly instead of cloning. Arrays are kept by reference; dictionaries are reused - // as-is when already concrete and only materialized when the caller passed some other - // IReadOnlyDictionary implementation. - Dictionary testMethodsMap = AsOwnedDictionary(testMethods); - Dictionary typeAttributesMap = AsOwnedDictionary(typeAttributes); - Dictionary> methodInvokersMap = AsOwnedDictionary(methodInvokers); - Dictionary> propertySettersMap = AsOwnedDictionary(propertySetters); - - // ConstructorInvokerInfo (public struct) has to be projected onto the adapter's internal - // ConstructorInvoker type; this is a representation change, not a defensive copy, and the - // parameter-type arrays are taken by reference. + // the passed arrays and read-only dictionaries directly instead of copying them. + // + // ConstructorInvokerInfo (public struct) still has to be projected onto the adapter's + // internal ConstructorInvoker type; this is a representation change, not a defensive copy, + // and the parameter-type arrays are taken by reference. var constructorInvokersMap = new Dictionary(constructorInvokers.Count); foreach (KeyValuePair kvp in constructorInvokers) { @@ -235,12 +230,12 @@ public static void Register( AssemblyName = assembly.GetName().Name ?? string.Empty, Types = types, TypesByName = typesByName, - TypeMethods = testMethodsMap, - TypeAttributes = typeAttributesMap, + TypeMethods = testMethods, + TypeAttributes = typeAttributes, AssemblyAttributes = assemblyAttributes, - TypeMethodInvokers = methodInvokersMap, + TypeMethodInvokers = methodInvokers, TypeConstructorsInvoker = constructorInvokersMap, - TypePropertySetters = propertySettersMap, + TypePropertySetters = propertySetters, }; lock (Lock) @@ -257,27 +252,6 @@ public static void Register( } } - // Reuses the caller-provided dictionary when it is already a concrete Dictionary<,> (the shape - // the source generator always emits), honoring the ownership-transfer contract with zero - // copying. Any other IReadOnlyDictionary implementation is materialized once so the provider - // still owns a concrete instance. Values are always taken by reference. - private static Dictionary AsOwnedDictionary(IReadOnlyDictionary source) - where TKey : notnull - { - if (source is Dictionary concrete) - { - return concrete; - } - - var copy = new Dictionary(source.Count); - foreach (KeyValuePair kvp in source) - { - copy[kvp.Key] = kvp.Value; - } - - return copy; - } - private static readonly Dictionary EmptyTypeAttributes = []; private static readonly Dictionary> EmptyMethodInvokers = []; diff --git a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/SourceGeneratedReflectionDataProvider.cs b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/SourceGeneratedReflectionDataProvider.cs index a841f81ad6..bccb969498 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/SourceGeneratedReflectionDataProvider.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/SourceGeneratedReflectionDataProvider.cs @@ -33,13 +33,13 @@ internal class SourceGeneratedReflectionDataProvider /// /// Gets a lookup of types by full name. /// - public Dictionary TypesByName { get; init; } = []; + public IReadOnlyDictionary TypesByName { get; init; } = new Dictionary(); /// /// Gets attributes declared on each type. The array contains attribute instances /// already inflated by the source generator so no reflection call is required to read them. /// - public Dictionary TypeAttributes { get; init; } = []; + public IReadOnlyDictionary TypeAttributes { get; init; } = new Dictionary(); /// /// Gets attribute instances declared at the assembly level. @@ -50,7 +50,7 @@ internal class SourceGeneratedReflectionDataProvider /// Gets the properties declared on each type that MSTest may inspect (for example /// TestContext properties or properties referenced by DynamicData). /// - public Dictionary TypeProperties { get; init; } = []; + public IReadOnlyDictionary TypeProperties { get; init; } = new Dictionary(); /// /// Gets the methods declared on each type that the source generator was able to surface @@ -62,36 +62,36 @@ internal class SourceGeneratedReflectionDataProvider /// source for BindingFlags.DeclaredOnly-style enumerations; the reflection-backed /// fallback is responsible for completeness. /// - public Dictionary TypeMethods { get; init; } = []; + public IReadOnlyDictionary TypeMethods { get; init; } = new Dictionary(); /// /// Gets source-location data for each type's methods so navigation in the IDE works /// without a PDB round-trip. /// - public Dictionary TypeMethodLocations { get; init; } = []; + public IReadOnlyDictionary TypeMethodLocations { get; init; } = new Dictionary(); /// /// Gets attributes declared on each method, keyed by the instance /// that the source-generator resolved at startup. Keying by /// (rather than method name) preserves the ability to distinguish overloaded methods. /// - public Dictionary TypeMethodAttributes { get; init; } = []; + public IReadOnlyDictionary TypeMethodAttributes { get; init; } = new Dictionary(); /// /// Gets constructors declared on each type. These are returned by /// GetDeclaredConstructors. /// - public Dictionary TypeConstructors { get; init; } = []; + public IReadOnlyDictionary TypeConstructors { get; init; } = new Dictionary(); /// /// Gets a lookup of properties on a type by property name. /// - public Dictionary> TypePropertiesByName { get; init; } = []; + public IReadOnlyDictionary> TypePropertiesByName { get; init; } = new Dictionary>(); /// /// Gets the constructor invokers that allow instantiating types without reflection. /// - public Dictionary TypeConstructorsInvoker { get; init; } = []; + public IReadOnlyDictionary TypeConstructorsInvoker { get; init; } = new Dictionary(); /// /// Gets the delegate-based invokers for test methods and fixtures, keyed by the @@ -102,14 +102,14 @@ internal class SourceGeneratedReflectionDataProvider /// a is converted with AsTask(), and any /// return value is discarded — so callers can simply await the result. /// - public Dictionary> TypeMethodInvokers { get; init; } = []; + public IReadOnlyDictionary> TypeMethodInvokers { get; init; } = new Dictionary>(); /// /// Gets the delegate-based property setters, keyed by the the /// adapter holds (today: the TestContext property). Each delegate assigns the value /// directly instead of calling . /// - public Dictionary> TypePropertySetters { get; init; } = []; + public IReadOnlyDictionary> TypePropertySetters { get; init; } = new Dictionary>(); /// /// Returns the snapshot of merged metadata that callers should read. Single-assembly diff --git a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/SourceGeneration/SourceGeneratedReflectionOperationsTests.cs b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/SourceGeneration/SourceGeneratedReflectionOperationsTests.cs index efeae1bc29..83a675df15 100644 --- a/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/SourceGeneration/SourceGeneratedReflectionOperationsTests.cs +++ b/test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/SourceGeneration/SourceGeneratedReflectionOperationsTests.cs @@ -34,7 +34,7 @@ public void GetTestMethodInvoker_ReturnsRegisteredDelegate_AndInvokesWithoutRefl var provider = new SourceGeneratedReflectionDataProvider { - TypeMethodInvokers = { [add] = invoker }, + TypeMethodInvokers = new Dictionary> { [add] = invoker }, }; var operations = new SourceGeneratedReflectionOperations(provider); @@ -67,7 +67,7 @@ public void GetConstructorInvoker_CreatesInstance_WithoutActivator() ]; var provider = new SourceGeneratedReflectionDataProvider { - TypeConstructorsInvoker = { [typeof(Sample)] = invokers }, + TypeConstructorsInvoker = new Dictionary { [typeof(Sample)] = invokers }, }; var operations = new SourceGeneratedReflectionOperations(provider); @@ -99,7 +99,7 @@ public void GetConstructorInvoker_FallsBackToReflection_WhenArgumentsDoNotMatchR ]; var provider = new SourceGeneratedReflectionDataProvider { - TypeConstructorsInvoker = { [typeof(Sample)] = invokers }, + TypeConstructorsInvoker = new Dictionary { [typeof(Sample)] = invokers }, }; var operations = new SourceGeneratedReflectionOperations(provider); @@ -119,7 +119,7 @@ public void GetPropertySetter_AssignsValue_WithoutSetValue() var provider = new SourceGeneratedReflectionDataProvider { - TypePropertySetters = { [property] = setter }, + TypePropertySetters = new Dictionary> { [property] = setter }, }; var operations = new SourceGeneratedReflectionOperations(provider);