ILLink: mark forwarded type targets when forwarders are marked

[Copilot]

Marked type forwarders could survive trimming while their target type definitions were removed, leaving dangling forwarders in rooted and copied assemblies. This change makes forwarder marking also mark the resolved target type unconditionally, so preserved forwarders remain functional.

• Behavior change

  • Marking an ExportedType now also marks its resolved target TypeDefinition.
  • This applies to both direct exported-type marking and exported types processed later from the pending mark queue.

• Implementation

  • MarkingHelpers.MarkExportedType now marks the forwarder target after marking the forwarder metadata row.
  • Added a shared helper for marking the resolved target and propagating any preserved-members state from the ExportedType to the target type.
  • Updated pending ExportedType processing in MarkStep to use the same target-marking path.

• Test updates

  • Updated existing linker tests that previously asserted dangling-forwarder behavior.
  • Rooted/copy library cases now expect the forwarded target type to be kept.
  • Copy-action type-forwarding cases now expect linked target assemblies/types to remain when referenced by kept forwarders.

// Before: preserved forwarder metadata could outlive the target type.// After: marking the forwarder also marks the forwarded-to type.[KeptTypeInAssembly("Forwarder.dll",typeof(ImplementationLibrary))][KeptTypeInAssembly("Implementation.dll",typeof(ImplementationLibrary))]

[Copilot]

Pull request overview

This PR updates ILLink’s marking behavior so that when a type forwarder (ExportedType) is preserved, the forwarded-to TypeDefinition is also marked, preventing preserved forwarders from becoming dangling after trimming/copying.

Changes:

• Update MarkingHelpers.MarkExportedType to also mark the resolved target TypeDefinition for the exported type.
• Update pending ExportedType processing in MarkStep to mark the exported type target consistently.
• Adjust existing linker tests to expect forwarded-to target types/assemblies to remain when forwarders are preserved.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tools/illink/src/linker/Linker/MarkingHelpers.cs	Adds shared helper to mark exported-type targets and propagate preserved-members state.
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs	Ensures pending exported types also mark their resolved target types during processing.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UsedAndUnusedForwarderWithAssemblyCopy.cs	Updates expectations so linked target type is kept when copied forwarders are preserved.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UnusedForwarderWithAssemblyCopyIsKept.cs	Updates expectations so forwarded-to implementation type is kept.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryCopyBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for copy behavior.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for root-all behavior.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_reviewed_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_recorded_worker_run_id": "29674628683",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"review_id": 4730179324
}
]
}

[github-actions]

Holistic Review

Motivation: Marked type forwarders (ExportedType metadata rows) could survive trimming while the target TypeDefinition they point to was removed, producing dangling forwarders in rooted (RootAllLibrary) and copy assemblies. A forwarder whose target type no longer exists is broken metadata: a consumer resolving the forwarded type at runtime would fail. This is a real correctness gap, confirmed by the existing tests that previously asserted [RemovedAssembly(...)] for the forwarded-to target.

Approach: MarkingHelpers.MarkExportedType now, after marking the forwarder metadata row, also resolves and marks the target TypeDefinition via a new shared MarkExportedTypeTarget helper, propagating any preserved-members state from the ExportedType to the target. MarkStep.ProcessMarkedPending routes pending ExportedType items through the same helper instead of the previous no-op. This is a clean, minimal, and consistent fix: the two code paths that can mark a forwarder now converge on one method, and the resolve is guarded (TryResolve returning a non-TypeDefinition is a safe no-op), so unresolvable forwarders degrade gracefully. The test updates correctly flip the previously-asserted dangling behavior to [KeptTypeInAssembly(...)].

Summary: A focused, correct bug fix with matching test coverage; I found no blocking issues. Verdict: LGTM.

Detailed Findings

• suggestion (non-blocking, src/tools/illink/src/linker/Linker/MarkingHelpers.cs): MarkExportedTypeTarget marks the target with new DependencyInfo(DependencyKind.ExportedType, exportedType). DependencyKind.ExportedType is documented in DependencyInfo.cs as // type -> exported type, but here the dependency runs the other direction (exported type -> target type). Reusing this kind is harmless for trimming behavior and only affects dependency-graph / --dump-dependencies reporting, but the arrow is semantically reversed. Consider a more accurate kind or a short clarifying comment. Not a merge blocker.

• I verified the other copy/copyused forwarder tests are unaffected: cases like UnusedForwarderWithAssemblyCopyUsed, UsedForwarderWithAssemblyCopyUsedAndUnusedReference, and UsedAndUnusedForwarderReferencedFromCopyUsedAssembly use copyused, where the forwarder assembly itself is removed as unused, so no ExportedType is marked and their [RemovedAssembly(...)] expectations still hold. The four updated tests are the only ones where a forwarder is actually marked. Because these expectations can only be validated by running the linker, CI (the ILLink test suite) should confirm no other test regresses.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 89 AIC · ⌖ 10.6 AIC · ⊞ 10K

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Marked type forwarders could survive trimming while their target type definitions were removed, leaving dangling forwarders in rooted and copied assemblies. This change makes forwarder marking also mark the resolved target type unconditionally, so preserved forwarders remain functional.

• Behavior change

  • Marking an ExportedType now also marks its resolved target TypeDefinition.
  • This applies to both direct exported-type marking and exported types processed later from the pending mark queue.

• Implementation

  • MarkingHelpers.MarkExportedType now marks the forwarder target after marking the forwarder metadata row.
  • Added a shared helper for marking the resolved target and propagating any preserved-members state from the ExportedType to the target type.
  • Updated pending ExportedType processing in MarkStep to use the same target-marking path.

• Test updates

  • Updated existing linker tests that previously asserted dangling-forwarder behavior.
  • Rooted/copy library cases now expect the forwarded target type to be kept.
  • Copy-action type-forwarding cases now expect linked target assemblies/types to remain when referenced by kept forwarders.

// Before: preserved forwarder metadata could outlive the target type.// After: marking the forwarder also marks the forwarded-to type.[KeptTypeInAssembly("Forwarder.dll",typeof(ImplementationLibrary))][KeptTypeInAssembly("Implementation.dll",typeof(ImplementationLibrary))]

[Copilot]

Pull request overview

This PR updates ILLink’s marking behavior so that when a type forwarder (ExportedType) is preserved, the forwarded-to TypeDefinition is also marked, preventing preserved forwarders from becoming dangling after trimming/copying.

Changes:

• Update MarkingHelpers.MarkExportedType to also mark the resolved target TypeDefinition for the exported type.
• Update pending ExportedType processing in MarkStep to mark the exported type target consistently.
• Adjust existing linker tests to expect forwarded-to target types/assemblies to remain when forwarders are preserved.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tools/illink/src/linker/Linker/MarkingHelpers.cs	Adds shared helper to mark exported-type targets and propagate preserved-members state.
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs	Ensures pending exported types also mark their resolved target types during processing.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UsedAndUnusedForwarderWithAssemblyCopy.cs	Updates expectations so linked target type is kept when copied forwarders are preserved.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UnusedForwarderWithAssemblyCopyIsKept.cs	Updates expectations so forwarded-to implementation type is kept.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryCopyBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for copy behavior.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for root-all behavior.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_reviewed_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_recorded_worker_run_id": "29674628683",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"review_id": 4730179324
}
]
}

[github-actions]

Holistic Review

Motivation: Marked type forwarders (ExportedType metadata rows) could survive trimming while the target TypeDefinition they point to was removed, producing dangling forwarders in rooted (RootAllLibrary) and copy assemblies. A forwarder whose target type no longer exists is broken metadata: a consumer resolving the forwarded type at runtime would fail. This is a real correctness gap, confirmed by the existing tests that previously asserted [RemovedAssembly(...)] for the forwarded-to target.

Approach: MarkingHelpers.MarkExportedType now, after marking the forwarder metadata row, also resolves and marks the target TypeDefinition via a new shared MarkExportedTypeTarget helper, propagating any preserved-members state from the ExportedType to the target. MarkStep.ProcessMarkedPending routes pending ExportedType items through the same helper instead of the previous no-op. This is a clean, minimal, and consistent fix: the two code paths that can mark a forwarder now converge on one method, and the resolve is guarded (TryResolve returning a non-TypeDefinition is a safe no-op), so unresolvable forwarders degrade gracefully. The test updates correctly flip the previously-asserted dangling behavior to [KeptTypeInAssembly(...)].

Summary: A focused, correct bug fix with matching test coverage; I found no blocking issues. Verdict: LGTM.

Detailed Findings

• suggestion (non-blocking, src/tools/illink/src/linker/Linker/MarkingHelpers.cs): MarkExportedTypeTarget marks the target with new DependencyInfo(DependencyKind.ExportedType, exportedType). DependencyKind.ExportedType is documented in DependencyInfo.cs as // type -> exported type, but here the dependency runs the other direction (exported type -> target type). Reusing this kind is harmless for trimming behavior and only affects dependency-graph / --dump-dependencies reporting, but the arrow is semantically reversed. Consider a more accurate kind or a short clarifying comment. Not a merge blocker.

• I verified the other copy/copyused forwarder tests are unaffected: cases like UnusedForwarderWithAssemblyCopyUsed, UsedForwarderWithAssemblyCopyUsedAndUnusedReference, and UsedAndUnusedForwarderReferencedFromCopyUsedAssembly use copyused, where the forwarder assembly itself is removed as unused, so no ExportedType is marked and their [RemovedAssembly(...)] expectations still hold. The four updated tests are the only ones where a forwarder is actually marked. Because these expectations can only be validated by running the linker, CI (the ILLink test suite) should confirm no other test regresses.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 89 AIC · ⌖ 10.6 AIC · ⊞ 10K

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Marked type forwarders could survive trimming while their target type definitions were removed, leaving dangling forwarders in rooted and copied assemblies. This change makes forwarder marking also mark the resolved target type unconditionally, so preserved forwarders remain functional.

• Behavior change

  • Marking an ExportedType now also marks its resolved target TypeDefinition.
  • This applies to both direct exported-type marking and exported types processed later from the pending mark queue.

• Implementation

  • MarkingHelpers.MarkExportedType now marks the forwarder target after marking the forwarder metadata row.
  • Added a shared helper for marking the resolved target and propagating any preserved-members state from the ExportedType to the target type.
  • Updated pending ExportedType processing in MarkStep to use the same target-marking path.

• Test updates

  • Updated existing linker tests that previously asserted dangling-forwarder behavior.
  • Rooted/copy library cases now expect the forwarded target type to be kept.
  • Copy-action type-forwarding cases now expect linked target assemblies/types to remain when referenced by kept forwarders.

// Before: preserved forwarder metadata could outlive the target type.// After: marking the forwarder also marks the forwarded-to type.[KeptTypeInAssembly("Forwarder.dll",typeof(ImplementationLibrary))][KeptTypeInAssembly("Implementation.dll",typeof(ImplementationLibrary))]

[Copilot]

Pull request overview

This PR updates ILLink’s marking behavior so that when a type forwarder (ExportedType) is preserved, the forwarded-to TypeDefinition is also marked, preventing preserved forwarders from becoming dangling after trimming/copying.

Changes:

• Update MarkingHelpers.MarkExportedType to also mark the resolved target TypeDefinition for the exported type.
• Update pending ExportedType processing in MarkStep to mark the exported type target consistently.
• Adjust existing linker tests to expect forwarded-to target types/assemblies to remain when forwarders are preserved.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tools/illink/src/linker/Linker/MarkingHelpers.cs	Adds shared helper to mark exported-type targets and propagate preserved-members state.
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs	Ensures pending exported types also mark their resolved target types during processing.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UsedAndUnusedForwarderWithAssemblyCopy.cs	Updates expectations so linked target type is kept when copied forwarders are preserved.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UnusedForwarderWithAssemblyCopyIsKept.cs	Updates expectations so forwarded-to implementation type is kept.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryCopyBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for copy behavior.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for root-all behavior.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_reviewed_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_recorded_worker_run_id": "29674628683",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"review_id": 4730179324
}
]
}

[github-actions]

Holistic Review

Motivation: Marked type forwarders (ExportedType metadata rows) could survive trimming while the target TypeDefinition they point to was removed, producing dangling forwarders in rooted (RootAllLibrary) and copy assemblies. A forwarder whose target type no longer exists is broken metadata: a consumer resolving the forwarded type at runtime would fail. This is a real correctness gap, confirmed by the existing tests that previously asserted [RemovedAssembly(...)] for the forwarded-to target.

Approach: MarkingHelpers.MarkExportedType now, after marking the forwarder metadata row, also resolves and marks the target TypeDefinition via a new shared MarkExportedTypeTarget helper, propagating any preserved-members state from the ExportedType to the target. MarkStep.ProcessMarkedPending routes pending ExportedType items through the same helper instead of the previous no-op. This is a clean, minimal, and consistent fix: the two code paths that can mark a forwarder now converge on one method, and the resolve is guarded (TryResolve returning a non-TypeDefinition is a safe no-op), so unresolvable forwarders degrade gracefully. The test updates correctly flip the previously-asserted dangling behavior to [KeptTypeInAssembly(...)].

Summary: A focused, correct bug fix with matching test coverage; I found no blocking issues. Verdict: LGTM.

Detailed Findings

• suggestion (non-blocking, src/tools/illink/src/linker/Linker/MarkingHelpers.cs): MarkExportedTypeTarget marks the target with new DependencyInfo(DependencyKind.ExportedType, exportedType). DependencyKind.ExportedType is documented in DependencyInfo.cs as // type -> exported type, but here the dependency runs the other direction (exported type -> target type). Reusing this kind is harmless for trimming behavior and only affects dependency-graph / --dump-dependencies reporting, but the arrow is semantically reversed. Consider a more accurate kind or a short clarifying comment. Not a merge blocker.

• I verified the other copy/copyused forwarder tests are unaffected: cases like UnusedForwarderWithAssemblyCopyUsed, UsedForwarderWithAssemblyCopyUsedAndUnusedReference, and UsedAndUnusedForwarderReferencedFromCopyUsedAssembly use copyused, where the forwarder assembly itself is removed as unused, so no ExportedType is marked and their [RemovedAssembly(...)] expectations still hold. The four updated tests are the only ones where a forwarder is actually marked. Because these expectations can only be validated by running the linker, CI (the ILLink test suite) should confirm no other test regresses.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 89 AIC · ⌖ 10.6 AIC · ⊞ 10K

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Marked type forwarders could survive trimming while their target type definitions were removed, leaving dangling forwarders in rooted and copied assemblies. This change makes forwarder marking also mark the resolved target type unconditionally, so preserved forwarders remain functional.

• Behavior change

  • Marking an ExportedType now also marks its resolved target TypeDefinition.
  • This applies to both direct exported-type marking and exported types processed later from the pending mark queue.

• Implementation

  • MarkingHelpers.MarkExportedType now marks the forwarder target after marking the forwarder metadata row.
  • Added a shared helper for marking the resolved target and propagating any preserved-members state from the ExportedType to the target type.
  • Updated pending ExportedType processing in MarkStep to use the same target-marking path.

• Test updates

  • Updated existing linker tests that previously asserted dangling-forwarder behavior.
  • Rooted/copy library cases now expect the forwarded target type to be kept.
  • Copy-action type-forwarding cases now expect linked target assemblies/types to remain when referenced by kept forwarders.

// Before: preserved forwarder metadata could outlive the target type.// After: marking the forwarder also marks the forwarded-to type.[KeptTypeInAssembly("Forwarder.dll",typeof(ImplementationLibrary))][KeptTypeInAssembly("Implementation.dll",typeof(ImplementationLibrary))]

[Copilot]

Pull request overview

This PR updates ILLink’s marking behavior so that when a type forwarder (ExportedType) is preserved, the forwarded-to TypeDefinition is also marked, preventing preserved forwarders from becoming dangling after trimming/copying.

Changes:

• Update MarkingHelpers.MarkExportedType to also mark the resolved target TypeDefinition for the exported type.
• Update pending ExportedType processing in MarkStep to mark the exported type target consistently.
• Adjust existing linker tests to expect forwarded-to target types/assemblies to remain when forwarders are preserved.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tools/illink/src/linker/Linker/MarkingHelpers.cs	Adds shared helper to mark exported-type targets and propagate preserved-members state.
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs	Ensures pending exported types also mark their resolved target types during processing.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UsedAndUnusedForwarderWithAssemblyCopy.cs	Updates expectations so linked target type is kept when copied forwarders are preserved.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UnusedForwarderWithAssemblyCopyIsKept.cs	Updates expectations so forwarded-to implementation type is kept.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryCopyBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for copy behavior.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for root-all behavior.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_reviewed_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_recorded_worker_run_id": "29674628683",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"review_id": 4730179324
}
]
}

[github-actions]

Holistic Review

Motivation: Marked type forwarders (ExportedType metadata rows) could survive trimming while the target TypeDefinition they point to was removed, producing dangling forwarders in rooted (RootAllLibrary) and copy assemblies. A forwarder whose target type no longer exists is broken metadata: a consumer resolving the forwarded type at runtime would fail. This is a real correctness gap, confirmed by the existing tests that previously asserted [RemovedAssembly(...)] for the forwarded-to target.

Approach: MarkingHelpers.MarkExportedType now, after marking the forwarder metadata row, also resolves and marks the target TypeDefinition via a new shared MarkExportedTypeTarget helper, propagating any preserved-members state from the ExportedType to the target. MarkStep.ProcessMarkedPending routes pending ExportedType items through the same helper instead of the previous no-op. This is a clean, minimal, and consistent fix: the two code paths that can mark a forwarder now converge on one method, and the resolve is guarded (TryResolve returning a non-TypeDefinition is a safe no-op), so unresolvable forwarders degrade gracefully. The test updates correctly flip the previously-asserted dangling behavior to [KeptTypeInAssembly(...)].

Summary: A focused, correct bug fix with matching test coverage; I found no blocking issues. Verdict: LGTM.

Detailed Findings

• suggestion (non-blocking, src/tools/illink/src/linker/Linker/MarkingHelpers.cs): MarkExportedTypeTarget marks the target with new DependencyInfo(DependencyKind.ExportedType, exportedType). DependencyKind.ExportedType is documented in DependencyInfo.cs as // type -> exported type, but here the dependency runs the other direction (exported type -> target type). Reusing this kind is harmless for trimming behavior and only affects dependency-graph / --dump-dependencies reporting, but the arrow is semantically reversed. Consider a more accurate kind or a short clarifying comment. Not a merge blocker.

• I verified the other copy/copyused forwarder tests are unaffected: cases like UnusedForwarderWithAssemblyCopyUsed, UsedForwarderWithAssemblyCopyUsedAndUnusedReference, and UsedAndUnusedForwarderReferencedFromCopyUsedAssembly use copyused, where the forwarder assembly itself is removed as unused, so no ExportedType is marked and their [RemovedAssembly(...)] expectations still hold. The four updated tests are the only ones where a forwarder is actually marked. Because these expectations can only be validated by running the linker, CI (the ILLink test suite) should confirm no other test regresses.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 89 AIC · ⌖ 10.6 AIC · ⊞ 10K

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Marked type forwarders could survive trimming while their target type definitions were removed, leaving dangling forwarders in rooted and copied assemblies. This change makes forwarder marking also mark the resolved target type unconditionally, so preserved forwarders remain functional.

• Behavior change

  • Marking an ExportedType now also marks its resolved target TypeDefinition.
  • This applies to both direct exported-type marking and exported types processed later from the pending mark queue.

• Implementation

  • MarkingHelpers.MarkExportedType now marks the forwarder target after marking the forwarder metadata row.
  • Added a shared helper for marking the resolved target and propagating any preserved-members state from the ExportedType to the target type.
  • Updated pending ExportedType processing in MarkStep to use the same target-marking path.

• Test updates

  • Updated existing linker tests that previously asserted dangling-forwarder behavior.
  • Rooted/copy library cases now expect the forwarded target type to be kept.
  • Copy-action type-forwarding cases now expect linked target assemblies/types to remain when referenced by kept forwarders.

// Before: preserved forwarder metadata could outlive the target type.// After: marking the forwarder also marks the forwarded-to type.[KeptTypeInAssembly("Forwarder.dll",typeof(ImplementationLibrary))][KeptTypeInAssembly("Implementation.dll",typeof(ImplementationLibrary))]

[Copilot]

Pull request overview

This PR updates ILLink’s marking behavior so that when a type forwarder (ExportedType) is preserved, the forwarded-to TypeDefinition is also marked, preventing preserved forwarders from becoming dangling after trimming/copying.

Changes:

• Update MarkingHelpers.MarkExportedType to also mark the resolved target TypeDefinition for the exported type.
• Update pending ExportedType processing in MarkStep to mark the exported type target consistently.
• Adjust existing linker tests to expect forwarded-to target types/assemblies to remain when forwarders are preserved.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tools/illink/src/linker/Linker/MarkingHelpers.cs	Adds shared helper to mark exported-type targets and propagate preserved-members state.
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs	Ensures pending exported types also mark their resolved target types during processing.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UsedAndUnusedForwarderWithAssemblyCopy.cs	Updates expectations so linked target type is kept when copied forwarders are preserved.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UnusedForwarderWithAssemblyCopyIsKept.cs	Updates expectations so forwarded-to implementation type is kept.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryCopyBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for copy behavior.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for root-all behavior.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_reviewed_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_recorded_worker_run_id": "29674628683",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"review_id": 4730179324
}
]
}

[github-actions]

Holistic Review

Motivation: Marked type forwarders (ExportedType metadata rows) could survive trimming while the target TypeDefinition they point to was removed, producing dangling forwarders in rooted (RootAllLibrary) and copy assemblies. A forwarder whose target type no longer exists is broken metadata: a consumer resolving the forwarded type at runtime would fail. This is a real correctness gap, confirmed by the existing tests that previously asserted [RemovedAssembly(...)] for the forwarded-to target.

Approach: MarkingHelpers.MarkExportedType now, after marking the forwarder metadata row, also resolves and marks the target TypeDefinition via a new shared MarkExportedTypeTarget helper, propagating any preserved-members state from the ExportedType to the target. MarkStep.ProcessMarkedPending routes pending ExportedType items through the same helper instead of the previous no-op. This is a clean, minimal, and consistent fix: the two code paths that can mark a forwarder now converge on one method, and the resolve is guarded (TryResolve returning a non-TypeDefinition is a safe no-op), so unresolvable forwarders degrade gracefully. The test updates correctly flip the previously-asserted dangling behavior to [KeptTypeInAssembly(...)].

Summary: A focused, correct bug fix with matching test coverage; I found no blocking issues. Verdict: LGTM.

Detailed Findings

• suggestion (non-blocking, src/tools/illink/src/linker/Linker/MarkingHelpers.cs): MarkExportedTypeTarget marks the target with new DependencyInfo(DependencyKind.ExportedType, exportedType). DependencyKind.ExportedType is documented in DependencyInfo.cs as // type -> exported type, but here the dependency runs the other direction (exported type -> target type). Reusing this kind is harmless for trimming behavior and only affects dependency-graph / --dump-dependencies reporting, but the arrow is semantically reversed. Consider a more accurate kind or a short clarifying comment. Not a merge blocker.

• I verified the other copy/copyused forwarder tests are unaffected: cases like UnusedForwarderWithAssemblyCopyUsed, UsedForwarderWithAssemblyCopyUsedAndUnusedReference, and UsedAndUnusedForwarderReferencedFromCopyUsedAssembly use copyused, where the forwarder assembly itself is removed as unused, so no ExportedType is marked and their [RemovedAssembly(...)] expectations still hold. The four updated tests are the only ones where a forwarder is actually marked. Because these expectations can only be validated by running the linker, CI (the ILLink test suite) should confirm no other test regresses.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 89 AIC · ⌖ 10.6 AIC · ⊞ 10K

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Marked type forwarders could survive trimming while their target type definitions were removed, leaving dangling forwarders in rooted and copied assemblies. This change makes forwarder marking also mark the resolved target type unconditionally, so preserved forwarders remain functional.

• Behavior change

  • Marking an ExportedType now also marks its resolved target TypeDefinition.
  • This applies to both direct exported-type marking and exported types processed later from the pending mark queue.

• Implementation

  • MarkingHelpers.MarkExportedType now marks the forwarder target after marking the forwarder metadata row.
  • Added a shared helper for marking the resolved target and propagating any preserved-members state from the ExportedType to the target type.
  • Updated pending ExportedType processing in MarkStep to use the same target-marking path.

• Test updates

  • Updated existing linker tests that previously asserted dangling-forwarder behavior.
  • Rooted/copy library cases now expect the forwarded target type to be kept.
  • Copy-action type-forwarding cases now expect linked target assemblies/types to remain when referenced by kept forwarders.

// Before: preserved forwarder metadata could outlive the target type.// After: marking the forwarder also marks the forwarded-to type.[KeptTypeInAssembly("Forwarder.dll",typeof(ImplementationLibrary))][KeptTypeInAssembly("Implementation.dll",typeof(ImplementationLibrary))]

[Copilot]

Pull request overview

This PR updates ILLink’s marking behavior so that when a type forwarder (ExportedType) is preserved, the forwarded-to TypeDefinition is also marked, preventing preserved forwarders from becoming dangling after trimming/copying.

Changes:

• Update MarkingHelpers.MarkExportedType to also mark the resolved target TypeDefinition for the exported type.
• Update pending ExportedType processing in MarkStep to mark the exported type target consistently.
• Adjust existing linker tests to expect forwarded-to target types/assemblies to remain when forwarders are preserved.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tools/illink/src/linker/Linker/MarkingHelpers.cs	Adds shared helper to mark exported-type targets and propagate preserved-members state.
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs	Ensures pending exported types also mark their resolved target types during processing.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UsedAndUnusedForwarderWithAssemblyCopy.cs	Updates expectations so linked target type is kept when copied forwarders are preserved.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UnusedForwarderWithAssemblyCopyIsKept.cs	Updates expectations so forwarded-to implementation type is kept.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryCopyBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for copy behavior.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for root-all behavior.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_reviewed_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_recorded_worker_run_id": "29674628683",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"review_id": 4730179324
}
]
}

[github-actions]

Holistic Review

Motivation: Marked type forwarders (ExportedType metadata rows) could survive trimming while the target TypeDefinition they point to was removed, producing dangling forwarders in rooted (RootAllLibrary) and copy assemblies. A forwarder whose target type no longer exists is broken metadata: a consumer resolving the forwarded type at runtime would fail. This is a real correctness gap, confirmed by the existing tests that previously asserted [RemovedAssembly(...)] for the forwarded-to target.

Approach: MarkingHelpers.MarkExportedType now, after marking the forwarder metadata row, also resolves and marks the target TypeDefinition via a new shared MarkExportedTypeTarget helper, propagating any preserved-members state from the ExportedType to the target. MarkStep.ProcessMarkedPending routes pending ExportedType items through the same helper instead of the previous no-op. This is a clean, minimal, and consistent fix: the two code paths that can mark a forwarder now converge on one method, and the resolve is guarded (TryResolve returning a non-TypeDefinition is a safe no-op), so unresolvable forwarders degrade gracefully. The test updates correctly flip the previously-asserted dangling behavior to [KeptTypeInAssembly(...)].

Summary: A focused, correct bug fix with matching test coverage; I found no blocking issues. Verdict: LGTM.

Detailed Findings

• suggestion (non-blocking, src/tools/illink/src/linker/Linker/MarkingHelpers.cs): MarkExportedTypeTarget marks the target with new DependencyInfo(DependencyKind.ExportedType, exportedType). DependencyKind.ExportedType is documented in DependencyInfo.cs as // type -> exported type, but here the dependency runs the other direction (exported type -> target type). Reusing this kind is harmless for trimming behavior and only affects dependency-graph / --dump-dependencies reporting, but the arrow is semantically reversed. Consider a more accurate kind or a short clarifying comment. Not a merge blocker.

• I verified the other copy/copyused forwarder tests are unaffected: cases like UnusedForwarderWithAssemblyCopyUsed, UsedForwarderWithAssemblyCopyUsedAndUnusedReference, and UsedAndUnusedForwarderReferencedFromCopyUsedAssembly use copyused, where the forwarder assembly itself is removed as unused, so no ExportedType is marked and their [RemovedAssembly(...)] expectations still hold. The four updated tests are the only ones where a forwarder is actually marked. Because these expectations can only be validated by running the linker, CI (the ILLink test suite) should confirm no other test regresses.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 89 AIC · ⌖ 10.6 AIC · ⊞ 10K

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Marked type forwarders could survive trimming while their target type definitions were removed, leaving dangling forwarders in rooted and copied assemblies. This change makes forwarder marking also mark the resolved target type unconditionally, so preserved forwarders remain functional.

• Behavior change

  • Marking an ExportedType now also marks its resolved target TypeDefinition.
  • This applies to both direct exported-type marking and exported types processed later from the pending mark queue.

• Implementation

  • MarkingHelpers.MarkExportedType now marks the forwarder target after marking the forwarder metadata row.
  • Added a shared helper for marking the resolved target and propagating any preserved-members state from the ExportedType to the target type.
  • Updated pending ExportedType processing in MarkStep to use the same target-marking path.

• Test updates

  • Updated existing linker tests that previously asserted dangling-forwarder behavior.
  • Rooted/copy library cases now expect the forwarded target type to be kept.
  • Copy-action type-forwarding cases now expect linked target assemblies/types to remain when referenced by kept forwarders.

// Before: preserved forwarder metadata could outlive the target type.// After: marking the forwarder also marks the forwarded-to type.[KeptTypeInAssembly("Forwarder.dll",typeof(ImplementationLibrary))][KeptTypeInAssembly("Implementation.dll",typeof(ImplementationLibrary))]

[Copilot]

Pull request overview

This PR updates ILLink’s marking behavior so that when a type forwarder (ExportedType) is preserved, the forwarded-to TypeDefinition is also marked, preventing preserved forwarders from becoming dangling after trimming/copying.

Changes:

• Update MarkingHelpers.MarkExportedType to also mark the resolved target TypeDefinition for the exported type.
• Update pending ExportedType processing in MarkStep to mark the exported type target consistently.
• Adjust existing linker tests to expect forwarded-to target types/assemblies to remain when forwarders are preserved.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tools/illink/src/linker/Linker/MarkingHelpers.cs	Adds shared helper to mark exported-type targets and propagate preserved-members state.
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs	Ensures pending exported types also mark their resolved target types during processing.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UsedAndUnusedForwarderWithAssemblyCopy.cs	Updates expectations so linked target type is kept when copied forwarders are preserved.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UnusedForwarderWithAssemblyCopyIsKept.cs	Updates expectations so forwarded-to implementation type is kept.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryCopyBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for copy behavior.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for root-all behavior.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_reviewed_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_recorded_worker_run_id": "29674628683",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"review_id": 4730179324
}
]
}

[github-actions]

Holistic Review

Motivation: Marked type forwarders (ExportedType metadata rows) could survive trimming while the target TypeDefinition they point to was removed, producing dangling forwarders in rooted (RootAllLibrary) and copy assemblies. A forwarder whose target type no longer exists is broken metadata: a consumer resolving the forwarded type at runtime would fail. This is a real correctness gap, confirmed by the existing tests that previously asserted [RemovedAssembly(...)] for the forwarded-to target.

Approach: MarkingHelpers.MarkExportedType now, after marking the forwarder metadata row, also resolves and marks the target TypeDefinition via a new shared MarkExportedTypeTarget helper, propagating any preserved-members state from the ExportedType to the target. MarkStep.ProcessMarkedPending routes pending ExportedType items through the same helper instead of the previous no-op. This is a clean, minimal, and consistent fix: the two code paths that can mark a forwarder now converge on one method, and the resolve is guarded (TryResolve returning a non-TypeDefinition is a safe no-op), so unresolvable forwarders degrade gracefully. The test updates correctly flip the previously-asserted dangling behavior to [KeptTypeInAssembly(...)].

Summary: A focused, correct bug fix with matching test coverage; I found no blocking issues. Verdict: LGTM.

Detailed Findings

• suggestion (non-blocking, src/tools/illink/src/linker/Linker/MarkingHelpers.cs): MarkExportedTypeTarget marks the target with new DependencyInfo(DependencyKind.ExportedType, exportedType). DependencyKind.ExportedType is documented in DependencyInfo.cs as // type -> exported type, but here the dependency runs the other direction (exported type -> target type). Reusing this kind is harmless for trimming behavior and only affects dependency-graph / --dump-dependencies reporting, but the arrow is semantically reversed. Consider a more accurate kind or a short clarifying comment. Not a merge blocker.

• I verified the other copy/copyused forwarder tests are unaffected: cases like UnusedForwarderWithAssemblyCopyUsed, UsedForwarderWithAssemblyCopyUsedAndUnusedReference, and UsedAndUnusedForwarderReferencedFromCopyUsedAssembly use copyused, where the forwarder assembly itself is removed as unused, so no ExportedType is marked and their [RemovedAssembly(...)] expectations still hold. The four updated tests are the only ones where a forwarder is actually marked. Because these expectations can only be validated by running the linker, CI (the ILLink test suite) should confirm no other test regresses.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 89 AIC · ⌖ 10.6 AIC · ⊞ 10K

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Marked type forwarders could survive trimming while their target type definitions were removed, leaving dangling forwarders in rooted and copied assemblies. This change makes forwarder marking also mark the resolved target type unconditionally, so preserved forwarders remain functional.

• Behavior change

  • Marking an ExportedType now also marks its resolved target TypeDefinition.
  • This applies to both direct exported-type marking and exported types processed later from the pending mark queue.

• Implementation

  • MarkingHelpers.MarkExportedType now marks the forwarder target after marking the forwarder metadata row.
  • Added a shared helper for marking the resolved target and propagating any preserved-members state from the ExportedType to the target type.
  • Updated pending ExportedType processing in MarkStep to use the same target-marking path.

• Test updates

  • Updated existing linker tests that previously asserted dangling-forwarder behavior.
  • Rooted/copy library cases now expect the forwarded target type to be kept.
  • Copy-action type-forwarding cases now expect linked target assemblies/types to remain when referenced by kept forwarders.

// Before: preserved forwarder metadata could outlive the target type.// After: marking the forwarder also marks the forwarded-to type.[KeptTypeInAssembly("Forwarder.dll",typeof(ImplementationLibrary))][KeptTypeInAssembly("Implementation.dll",typeof(ImplementationLibrary))]

[Copilot]

Pull request overview

This PR updates ILLink’s marking behavior so that when a type forwarder (ExportedType) is preserved, the forwarded-to TypeDefinition is also marked, preventing preserved forwarders from becoming dangling after trimming/copying.

Changes:

• Update MarkingHelpers.MarkExportedType to also mark the resolved target TypeDefinition for the exported type.
• Update pending ExportedType processing in MarkStep to mark the exported type target consistently.
• Adjust existing linker tests to expect forwarded-to target types/assemblies to remain when forwarders are preserved.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tools/illink/src/linker/Linker/MarkingHelpers.cs	Adds shared helper to mark exported-type targets and propagate preserved-members state.
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs	Ensures pending exported types also mark their resolved target types during processing.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UsedAndUnusedForwarderWithAssemblyCopy.cs	Updates expectations so linked target type is kept when copied forwarders are preserved.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UnusedForwarderWithAssemblyCopyIsKept.cs	Updates expectations so forwarded-to implementation type is kept.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryCopyBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for copy behavior.
src/tools/illink/test/Mono.Linker.Tests.Cases/Libraries/RootAllLibraryBehavior.cs	Updates expectations so forwarded-to exportedtype assembly/type is kept for root-all behavior.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_reviewed_commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "9d24c4d12540aca6019a5e212b7e170f5f3a0207",
"last_recorded_worker_run_id": "29674628683",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "118f2229fc4d3f2a358bc7c7a89b2e6e9cab45b5",
"review_id": 4730179324
}
]
}

[github-actions]

Holistic Review

Motivation: Marked type forwarders (ExportedType metadata rows) could survive trimming while the target TypeDefinition they point to was removed, producing dangling forwarders in rooted (RootAllLibrary) and copy assemblies. A forwarder whose target type no longer exists is broken metadata: a consumer resolving the forwarded type at runtime would fail. This is a real correctness gap, confirmed by the existing tests that previously asserted [RemovedAssembly(...)] for the forwarded-to target.

Approach: MarkingHelpers.MarkExportedType now, after marking the forwarder metadata row, also resolves and marks the target TypeDefinition via a new shared MarkExportedTypeTarget helper, propagating any preserved-members state from the ExportedType to the target. MarkStep.ProcessMarkedPending routes pending ExportedType items through the same helper instead of the previous no-op. This is a clean, minimal, and consistent fix: the two code paths that can mark a forwarder now converge on one method, and the resolve is guarded (TryResolve returning a non-TypeDefinition is a safe no-op), so unresolvable forwarders degrade gracefully. The test updates correctly flip the previously-asserted dangling behavior to [KeptTypeInAssembly(...)].

Summary: A focused, correct bug fix with matching test coverage; I found no blocking issues. Verdict: LGTM.

Detailed Findings

• suggestion (non-blocking, src/tools/illink/src/linker/Linker/MarkingHelpers.cs): MarkExportedTypeTarget marks the target with new DependencyInfo(DependencyKind.ExportedType, exportedType). DependencyKind.ExportedType is documented in DependencyInfo.cs as // type -> exported type, but here the dependency runs the other direction (exported type -> target type). Reusing this kind is harmless for trimming behavior and only affects dependency-graph / --dump-dependencies reporting, but the arrow is semantically reversed. Consider a more accurate kind or a short clarifying comment. Not a merge blocker.

• I verified the other copy/copyused forwarder tests are unaffected: cases like UnusedForwarderWithAssemblyCopyUsed, UsedForwarderWithAssemblyCopyUsedAndUnusedReference, and UsedAndUnusedForwarderReferencedFromCopyUsedAssembly use copyused, where the forwarder assembly itself is removed as unused, so no ExportedType is marked and their [RemovedAssembly(...)] expectations still hold. The four updated tests are the only ones where a forwarder is actually marked. Because these expectations can only be validated by running the linker, CI (the ILLink test suite) should confirm no other test regresses.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 89 AIC · ⌖ 10.6 AIC · ⊞ 10K

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite