Make DynamicData trimming/NativeAOT-safe via DynamicallyAccessedMembers#9832
Merged
Conversation
Annotate the declaring Type that DynamicData reflects over (constructor parameters, backing field, display-name declaring type, and the DynamicDataOperations.GetData parameter) with [DynamicallyAccessedMembers] so the trimmer preserves the members looked up at runtime, instead of leaving DynamicData silently trim/AOT-unsafe. The AutoDetect fallback to the test method's own class flows through a GetTestMethodDeclaringType helper carrying a narrow, truthful IL2073 suppression: the test method's declaring type is always rooted by discovery, so its members are preserved. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds trimming and NativeAOT annotations for reflection-based DynamicData member lookup.
Changes:
- Defines required reflected member types.
- Annotates declaring-type flows.
- Adds a suppressed helper for test-method declaring types.
Show a summary per file
| File | Description |
|---|---|
InternalAPI.Unshipped.txt |
Tracks new internal symbols. |
DynamicDataOperations.cs |
Adds trimming requirements and declaring-type helper. |
DynamicDataAttribute.cs |
Annotates constructors, storage, and display-name types. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary
The approach is sound — annotating Type parameters/fields with [DynamicallyAccessedMembers] is the correct pattern for making reflection-based member lookup trim-safe, and it mirrors the existing MemberConditionAttribute precedent in this repo.
Findings
| # | Dimension | Severity | Finding |
|---|---|---|---|
| 1 | Public API Surface | 🔴 Major | Adding [DynamicallyAccessedMembers] to 3 public constructor parameters and 1 public property (DynamicDataDisplayNameDeclaringType) changes the public API signature. These changes are not declared in PublicAPI.Unshipped.txt — only the internal symbols are tracked in InternalAPI.Unshipped.txt. Callers passing unannotated Type values will now receive new IL2067 trimmer warnings, which is a source-level breaking change for -WarnAsError users. |
| 2 | Trimming Correctness | 🟡 Suggestion | The IL2073 suppression on GetTestMethodDeclaringType is justified today because [TestClass] roots the type. Consider enriching the justification string to mention that invariant explicitly. |
| 3 | Over-preservation | i️ Info | RequiredMemberTypes preserves instance members too (since DynamicallyAccessedMemberTypes has no static-only flag), though DynamicData only supports static members. This is expected and unavoidable — no action needed. |
Dimensions with no findings (N/A or clean)
Algorithmic Correctness, Concurrency, Error Handling, Resource Management, Security, Performance, Cross-TFM, IPC, Localization, Naming, Style, Tests, Documentation, Backward Compat (aside from #1), Dependencies, Diagnostics, Logging, Serialization, Accessibility.
This comment has been minimized.
This comment has been minimized.
Address review feedback: clarify in RequiredMemberTypes docs that instance members are intentionally over-preserved (no static-only DAM flag) and that inherited members surfaced via FlattenHierarchy on a base type are outside DAM's granular reach. Enrich the IL2073 suppression justification with the [TestClass]-roots-the-type invariant breadcrumb. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
🔴 Build Failure AnalysisSummaryThe build failed with 4 CA1416 (platform compatibility) errors in Root Cause
Suggested Fix (for the base branch)Add // Option 1: Propagate the platform annotation
[UnsupportedOSPlatform("browser")]
[UnsupportedOSPlatform("wasi")]
public Task RunLongRunning(Func<Task> action, string name, CancellationToken cancellationToken)
=> _inner.RunLongRunning(action, name, cancellationToken);
// Option 2: Suppress the warning (preferred for test code)
#pragma warning disable CA1416 // Validate platform compatibility
public Task RunLongRunning(Func<Task> action, string name, CancellationToken cancellationToken)
=> _inner.RunLongRunning(action, name, cancellationToken);
#pragma warning restore CA1416Impact on This PRThis PR's changes (trimming annotations for
|
…rces Address review: the granular NonPublic* flags only preserve members declared directly on the annotated type, so an inherited (e.g. protected static) source surfaced via BindingFlags.FlattenHierarchy on a base type would still be trimmed away. This is a shipped, tested scenario (DynamicDataTest_Source*FromBase). Switch RequiredMemberTypes to All, which walks the whole base chain and matches the [DynamicDependency(All)] that MSTest.SourceGeneration already emits for test classes and their base types. The NonPublic*WithInherited flags are not a portable alternative (they are net9-only in the BCL). Also correct the IL2073 suppression justification: preservation comes from the source generator's [DynamicDependency(All)], not from [TestClass] (which carries no trimming annotations). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Evangelink
added a commit
that referenced
this pull request
Jul 11, 2026
…, BOMs, doc Tighten the source generator's [DynamicData] resolution so a reflection-free accessor is only emitted when it provably behaves like the runtime reflection lookup in DynamicDataOperations; otherwise fall back to the (DAM-safe) reflection path: - Default the source/display-name declaring type to the test method's containing type (matches methodInfo.DeclaringType), so inherited [DynamicData] resolves under the base type instead of the leaf. - Honor DynamicDataSourceType (explicit Property/Method/Field) and skip when the name maps to more than one member kind across the hierarchy (AutoDetect kind selection vs C# binding can diverge). - Validate the property getter itself (static + accessible from the consuming assembly), skip params/param-collection and by-ref source methods, and skip ambiguous method overloads. - Resolve display-name methods declared-only (GetDeclaredMethod), require RefKind.None params, and reject ambiguous overloads. - Guard declaring types to be closed + referenceable. - Escape emitted member/method identifiers so reserved-keyword names (e.g. @Class) compile. Also: add UTF-8 BOM to the four new .cs files, and correct the DynamicDataSourceResolver doc to describe the DAM-safe reflection fallback (#9832) rather than claiming it is unsafe. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f5797f94-192e-479e-a45f-2b2f8094c93b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What / why
DynamicDataAttributeresolves its source member (property/method/field) by name via reflection inDynamicDataOperations.GetData, reflecting over the declaringTypewithBindingFlags.Public | NonPublic | Instance | Static | FlattenHierarchy. That declaringTypecarried no trimming annotations, so under trimming/NativeAOT the members it looks up can be trimmed away and the lookup fails at runtime — silently, with no warning.This makes DynamicData genuinely trim/AOT-safe by telling the trimmer to preserve those members, following the exact pattern already used by
MemberConditionAttributein this same project ([DynamicallyAccessedMembers]on theType).Why this approach (and not the alternatives)
[RequiresUnreferencedCode]/[RequiresDynamicCode]would only warn, and — because the adapter consumes data sources through theITestDataSourceinterface, and users only ever apply the attribute — the warning would never actually reach the user; it would only fire inside MSTest's own build, tempting us into a suppression that would be a lie.ITestDataSourceinterface was rejected: the contract is trim-safe by design (e.g.DataRowAttributereturns static data), so the problem is the implementation, not the contract.[DynamicallyAccessedMembers]is the honest fix: it makes the code safe, emits no warning, needs no suppression, and — being polyfilled in this repo — works on all TFMs (netstandard2.0/net462/net8.0/net9.0) with no#ifguards.Changes
DynamicDataOperations.RequiredMemberTypesconstant (public+non-public properties/fields/methods, mirroring theBindingFlagsused).[DynamicallyAccessedMembers(RequiredMemberTypes)]to:Type-takingDynamicDataAttributeconstructors,_dynamicDataDeclaringTypefield,DynamicDataDisplayNameDeclaringTypeproperty,Type?parameter ofDynamicDataOperations.GetData.AutoDetectfallback to the test method's own class flows through a newGetTestMethodDeclaringTypehelper carrying a narrow, truthfulIL2073suppression —MethodInfo.DeclaringTypeisn't statically annotated, but the test class is always rooted by discovery, so its members are preserved.InternalAPI.Unshipped.txt.Verification
/p:EnableAotAnalyzers=true),DynamicDataOperations.csandDynamicDataAttribute.csproduce zero IL warnings — the annotations fully cover the reflection.methodInfo.DeclaringTypefallback).Out of scope / follow-up
Flipping the assembly-wide
EnableAotAnalyzersswitch is intentionally not done here. Enabling it surfaces 13 pre-existing, unrelated IL warnings inAssert.That.ExpressionDetailsandAssert.AreEquivalent.*(Expression.Lambda,Type.MakeGenericType,Type.GetInterfaces, …), which would break CI (-TreatWarningsAsErrors) and belong in a separate effort. This PR moves DynamicData one step toward that goal.