Transfer ownership in ReflectionMetadataHook.Register instead of cloning#9586
Conversation
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>
There was a problem hiding this comment.
Pull request overview
This PR updates the MSTest adapter’s source-generation registration hook (ReflectionMetadataHook.Register) to avoid defensive cloning of arrays and dictionaries, switching to an ownership-transfer contract so startup registration can be zero-copy.
Changes:
- Replaces defensive
Clone()patterns with direct storage of generator-provided collections (ownership transfer). - Adds
AsOwnedDictionaryto reuse concreteDictionary<,>inputs (and shallow-copy only non-DictionaryIReadOnlyDictionaryimplementations). - Stops cloning constructor-parameter type arrays when projecting
ConstructorInvokerInfointo internal constructor invokers.
Show a summary per file
| File | Description |
|---|---|
| src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs | Switches registration to ownership-transfer semantics, reuses dictionaries via AsOwnedDictionary, and updates XML docs to reflect the new contract. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Low
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>
|
/backport to rel/4.3 |
|
Started backporting to rel/4.3: https://github.com/microsoft/testfx/actions/runs/28668317883 |
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>
…ing by @Evangelink in #9586 (backport to rel/4.3) (#9589) Co-authored-by: Amaury Levé <amauryleve@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🧪 Test quality grade — PR #9586
This advisory comment was generated automatically. Grades are heuristic
|
What
The source-generated
ReflectionMetadataHook.Registerdefensively cloned every array and dictionary it received (types.Clone(), per-valueMethodInfo[]/Attribute[]clones,assemblyAttributes.Clone(), per-constructorParameterTypes.Clone(), plus shallow dictionary re-materialization). This PR replaces that with an ownership-transfer contract, eliminating the cloning at startup.Why
Registerispubliconly because the MSTest source generator emits a[ModuleInitializer]that calls it across the assembly boundary — it is documented as not for hand-written use. Both generators (the shippingReflectionMetadataEmitterand the AOTRuntimeRegistrationEmitter) build fresh, throwaway collections for each call and never touch them again. So the defensive copies were guarding against a caller mutation that the only sanctioned caller cannot perform.True caller-identity authentication isn't possible (generated code lives in the untrusted test assembly, so any token/sentinel is forgeable). But we don't need identity trust — we need mutation safety, which we get structurally by taking ownership of the fresh collections the generator hands over.
How
Type[]/object[]arrays by reference (no clone).Dictionary<,>(the shape both generators always emit) via a newAsOwnedDictionaryhelper; only exoticIReadOnlyDictionaryimplementations get a one-time shallow copy.ConstructorInvokerInfo→ internalConstructorInvokerprojection remains (a representation change, not a defensive copy) but no longer clones parameter arrays.Multi-TFM safe: uses only
is-pattern, capacity ctors, andforeach, so it compiles on the full matrix down to net462 / netstandard2.0 / uap10.0.16299 (avoids newer BCL APIs likenew Dictionary(IEnumerable<KVP>)).Verification
MSTestAdapter.PlatformServices.UnitTests— 893/893 pass.Registerpath, after-pack):SourceGenerationNonAotTests(2/2, 3-arg overload);InconclusiveTests(24/24) across Reflection, SourceGeneration, and AotSourceGeneration modes — the last exercises the modified 8-arg overload end-to-end.