Skip to content

Transfer ownership in ReflectionMetadataHook.Register instead of cloning#9586

Merged
Evangelink merged 3 commits into
mainfrom
dev/amauryleve/sourcegen-register-ownership-transfer
Jul 3, 2026
Merged

Transfer ownership in ReflectionMetadataHook.Register instead of cloning#9586
Evangelink merged 3 commits into
mainfrom
dev/amauryleve/sourcegen-register-ownership-transfer

Conversation

@Evangelink

Copy link
Copy Markdown
Member

What

The source-generated ReflectionMetadataHook.Register defensively cloned every array and dictionary it received (types.Clone(), per-value MethodInfo[]/Attribute[] clones, assemblyAttributes.Clone(), per-constructor ParameterTypes.Clone(), plus shallow dictionary re-materialization). This PR replaces that with an ownership-transfer contract, eliminating the cloning at startup.

Why

Register is public only 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 shipping ReflectionMetadataEmitter and the AOT RuntimeRegistrationEmitter) 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

  • Store the passed Type[] / object[] arrays by reference (no clone).
  • Reuse the caller's dictionary as-is when it's already a concrete Dictionary<,> (the shape both generators always emit) via a new AsOwnedDictionary helper; only exotic IReadOnlyDictionary implementations get a one-time shallow copy.
  • The ConstructorInvokerInfo → internal ConstructorInvoker projection remains (a representation change, not a defensive copy) but no longer clones parameter arrays.
  • Flips the XML-doc contract: the old "caller may mutate after the call" promise is replaced with an explicit ownership-transfer contract in the remarks.

Multi-TFM safe: uses only is-pattern, capacity ctors, and foreach, so it compiles on the full matrix down to net462 / netstandard2.0 / uap10.0.16299 (avoids newer BCL APIs like new Dictionary(IEnumerable<KVP>)).

Verification

  • Build: all TFMs (net462, net8.0, net9.0, net8.0-windows, net9.0-windows, uap10.0.16299) — 0 warnings, 0 errors.
  • Unit tests: MSTestAdapter.PlatformServices.UnitTests — 893/893 pass.
  • Acceptance (runtime Register path, 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.

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>
Copilot AI review requested due to automatic review settings July 3, 2026 14:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AsOwnedDictionary to reuse concrete Dictionary<,> inputs (and shallow-copy only non-Dictionary IReadOnlyDictionary implementations).
  • Stops cloning constructor-parameter type arrays when projecting ConstructorInvokerInfo into 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>
@Evangelink

Copy link
Copy Markdown
Member Author

/backport to rel/4.3

@Evangelink Evangelink enabled auto-merge (squash) July 3, 2026 14:57
@Evangelink Evangelink added the state/needs-review Awaiting review from the team. label Jul 3, 2026
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

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>
Copilot AI review requested due to automatic review settings July 3, 2026 15:11
Evangelink added a commit that referenced this pull request Jul 3, 2026
…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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Medium

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

🧪 Test quality grade — PR #9586

GradeTestNotes
B (80–89) mod SourceGeneratedReflectionOperationsTests.
GetConstructorInvoker_
CreatesInstance_
WithoutActivator
Tests both GetConstructorInvoker and CreateInstance in one body; consider splitting for per-method isolation.
B (80–89) mod SourceGeneratedReflectionOperationsTests.
GetTestMethodInvoker_
ReturnsRegisteredDelegate_
AndInvokesWithoutReflection
Tests delegate retrieval and invocation together; assertions are excellent — consider one test per behavior.
A (90–100) mod SourceGeneratedReflectionOperationsTests.
GetConstructorInvoker_
FallsBackToReflection_
WhenArgumentsDoNotMatchRegisteredConstructor
Value.Should().BeNull() correctly distinguishes the fallback path from the registered constructor. No issues found.
A (90–100) mod SourceGeneratedReflectionOperationsTests.
GetPropertySetter_
AssignsValue_
WithoutSetValue
Clean identity + side-effect assertion pair; concise and well-structured. No issues found.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 71.3 AIC · ⌖ 10.1 AIC · ⊞ 9.5K · [◷]( · )

@Evangelink Evangelink merged commit 0c48559 into main Jul 3, 2026
39 checks passed
@Evangelink Evangelink deleted the dev/amauryleve/sourcegen-register-ownership-transfer branch July 3, 2026 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state/needs-review Awaiting review from the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants