Skip to content

ILLink: Don't preserve forwarder assemblies referenced by link-action root assemblies#126519

Open
Copilot wants to merge 10 commits intomainfrom
copilot/review-type-forwarder-behavior
Open

ILLink: Don't preserve forwarder assemblies referenced by link-action root assemblies#126519
Copilot wants to merge 10 commits intomainfrom
copilot/review-type-forwarder-behavior

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 3, 2026 •
edited
Loading

Description

MarkStep.MarkEntireAssembly unconditionally called TypeReferenceMarker.MarkTypeReferences, which walks all type references and calls MarkForwardedScope on each — causing forwarder assemblies to be preserved even for root assemblies with link action. For link-action assemblies this is unnecessary: SweepStep.AssemblyReferencesCorrector rewrites type references to point directly to implementation assemblies, making forwarders irrelevant at runtime.

Historically, TypeReferenceMarker was only ever intended for copy/save assemblies. It started running for link-action root assemblies as an unintended side effect of #96363 reusing MarkEntireAssembly for the new root-with-link-action path.

Changes:

  • MarkStep.cs:
    • New MarkExportedTypes(assembly, origin): marks the assembly's own ExportedType entries (via MarkExportedType) for all root assemblies — this records the export entry itself without preserving the forwarder assembly it points to; reuses the caller's MessageOrigin to avoid per-entry allocations
    • TypeReferenceMarker.MarkTypeReferences: moved out of MarkEntireAssembly and called directly in the copy/save branch of MarkAssembly — MarkEntireAssembly is now action-agnostic (marks all types, custom attributes, and exported type entries only)
    • TypeReferenceMarker.ProcessExportedType: calls MarkForwardedScope for each exported type — runs only for copy/save assemblies, so forwarder assemblies pointed to by exports are only preserved when their references won't be rewritten
    • CreateTypeReferenceForExportedTypeTarget: private instance helper inside TypeReferenceMarker, placed after ProcessExtra, that correctly handles nested exported types via DeclaringType and is used by ProcessExportedType
  • UsedForwarderIsRemovedWhenReferencedByRootedAssembly.cs — Regression test: verifies Forwarder.dll is removed when the consuming assembly is rooted with -a test (link action)
  • RootedForwarderWithExportedTypesIsHandled.cs — Test: verifies that a root linked assembly that is itself a type forwarder (rooted with -a Forwarder) has its exported type entries correctly preserved by MarkExportedTypes, the implementation assembly is kept, and the consuming assembly's reference to the forwarder is rewritten by the sweep

The key separation of concerns:

  • Marking the export entry (MarkExportedType) — always done for all root assemblies
  • Preserving the forwarder assembly (MarkForwardedScope) — done only for copy/save assemblies via TypeReferenceMarker

Customer Impact

Projects using -a to root an assembly with link action unnecessarily retain forwarder assemblies, increasing trimmed output size.

Regression

No — this is a pre-existing behavioral issue, not a regression from a recent release.

Testing

Added two new ILLink test cases:

  • UsedForwarderIsRemovedWhenReferencedByRootedAssembly: asserts Forwarder.dll is removed and the assembly reference is rewritten when the consuming assembly is rooted with -a test (link action)
  • RootedForwarderWithExportedTypesIsHandled: asserts that when a forwarder assembly is itself rooted with -a Forwarder (link action), its exported type entries are preserved by MarkExportedTypes, the implementation is kept because the forwarded type is used, and the consuming assembly's reference to the forwarder is rewritten away by the sweep

All 44 TypeForwarding tests and 49 LinkXml tests continue to pass.

Risk

Low. TypeReferenceMarker.MarkTypeReferences is now called only in the copy/save branch of MarkAssembly (its historically intended location), and MarkEntireAssembly is kept focused on marking all types and their members. Exported type entry marking (MarkExportedType) always runs for all root assemblies and reuses the caller's MessageOrigin for consistency, while forwarder assembly preservation (MarkForwardedScope for exported types) is scoped to copy/save assemblies via TypeReferenceMarker. The CreateTypeReferenceForExportedTypeTarget helper is encapsulated as an instance method inside TypeReferenceMarker (after ProcessExtra) since it is only used there. Existing tests cover copy/save, link, and rooted-forwarder scenarios.

Package authoring no longer needed in .NET 9

IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.

Original prompt [Chronological Review] 1. User asked which files are uncommitted in /home/sven/src/runtime 2. Agent ran `git status --short` and reported the results 3. User asked to review the changes around type forwarder behavior 4. Agent loaded the code-review skill, gathered diffs for all three changed files (MarkStep.cs modified, old test deleted, new test created) 5. Agent spawned a background explore task to gather deep context: MarkEntireAssembly method, TypeReferenceMarker class, MarkingHelpers methods, ProcessReferencesStep.IsFullyPreservedAction, test file listing, related tests, DependencyKind.ExportedType usage, git log 6. Agent spawned another sync explore task to understand type reference rewriting in SweepStep/ProcessReferencesStep 7. Agent delivered a comprehensive code review with holistic assessment and 4 detailed findings (2 warnings, 2 approvals) 8. User noted they filed issue https://github.com//issues/126518 9. Agent read the issue via GitHub MCP server 10. User asked to commit the current fix 11. Agent checked branch (main) and staged/committed the 3 files with a detailed commit message referencing #126518

[Intent Mapping]

[Technical Inventory]

  • Repository: dotnet/runtime at /home/sven/src/runtime, on branch main
  • Component: ILLink (IL Linker) - .NET's ahead-of-time trimming tool
  • Key files: MarkStep.cs, MarkingHelpers.cs, ProcessReferencesStep.cs, SweepStep.cs
  • Concepts: type forwarders, exported types, assembly actions (link/copy/save), type reference rewriting, rooted assemblies (-a flag)
  • Issue: ILLink: root assembly preserves referenced forwarders in other assemblies #126518 - "ILLink: root assembly preserves referenced forwarders in other assemblies"

[Code Archaeology]

  • MarkStep.cs: Modified MarkEntireAssembly to split exported type marking from type reference scope marking; added new MarkExportedTypes method; TypeReferenceMarker now only runs for copy/save assemblies
  • UsedForwarderIsKeptWhenReferencedByRootedAssembly.cs: Deleted (old test expecting forwarder kept)
  • UsedForwarderIsRemovedWhenReferencedByRootedAssembly.cs: New test expecting forwarder removed
  • ProcessReferencesStep.IsFullyPreservedAction: Returns true for Copy or Save actions only
  • MarkingHelpers.MarkForwardedScope: Resolves type reference scope and marks exported types in forwarder assemblies
  • SweepStep.AssemblyReferencesCorrector: Rewrites type reference scopes from forwarder to implementation assemblies

[Progress Assessment]

  • All tasks complete: review done, issue noted, commit made

[Context Validation]

  • Commit hash: c933d2f4d25
  • All three files committed successfully
  • Git recognized the test rename (64% similar)

[Recent Commands Analysis]

  • Last commands: git add + git commit with detailed message
  • Result: successful commit c933d2f4d25
  • Pre-summarization state: commit completed, conversation idle
1. Conversation Overview: - Primary Objectives: User asked to (1) check uncommitted files, (2) "review the changes around type forwarder behavior please", (3) note a filed GitHub issue, and (4) "Commit the current fix." - Session Context: Working in dotnet/runtime repo (/home/sven/src/runtime) on the `main` branch. The changes fix an ILLink bug where rooted assemblies with link action incorrectly preserved forwarder assemblies referenced through type references. The full workflow was: status check → deep code review → issue acknowledgment → commit. - User Intent Evolution: Straightforward progression from exploration → review → commit. User is "sbomer" (GitHub member, ILLink area).
  1. Technical Foundation:
    • Repository: dotnet/runtime at /home/sven/src/runtime, branch main
    • Component: ILLink (IL Linker) — .NET's trimming/linking tool in src/tools/illink/
    • Core Concept: Type forwarders are assemblies containing ExportedType entries that redirect type lookups to implementation assemblies. The linker can remove them when it rewrites type references to point directly at implementation assemblies.
    • Assembly Actions: link (rewrite/trim), copy (preserve as-is), save (preserve with minor changes). ProcessReferencesStep.IsFullyPreservedAction returns true only for Copy/Save.
    • Bug: MarkEntireAssembly unconditionally called TypeReferenceMarker.MarkTypeReferences, which marked forwarder scopes for ALL rooted assemblies. For link-action assemblies, this was unnecessary because SweepStep.AssemblyReferencesCorrector rewrites type references to point directly to implementation assemblies.
    • GitHub Issue: #126518 — "ILLink: root assembly preserves referenced forwarders in other assemblies", filed b...

Created from Copilot CLI via the copilot delegate command.

@sbomer sbomer added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed area-TypeSystem-coreclr labels Apr 3, 2026
@sbomer
Don't preserve forwarder assemblies referenced by link-action root as…
8378d26 
…semblies

When an assembly is rooted with `-a` (AllMembers) and has link action,
MarkStep.MarkEntireAssembly previously called TypeReferenceMarker.MarkTypeReferences
unconditionally. This walked all type references in the assembly and called
MarkForwardedScope for each, marking exported type entries in forwarder assemblies
and causing those assemblies to be preserved.

This is unnecessary for link-action assemblies because SweepStep rewrites type
references to point directly to implementation assemblies (via AssemblyReferencesCorrector),
making the forwarder assemblies unnecessary.

The fix splits MarkEntireAssembly into two concerns:
- MarkExportedTypes: always marks the assembly's own ExportedType entries and their
  forwarded scopes (needed even for link assemblies that are themselves forwarders)
- TypeReferenceMarker.MarkTypeReferences: now only called for copy/save assemblies
  where type references won't be rewritten

Also adds a regression test: UsedForwarderIsRemovedWhenReferencedByRootedAssembly.

Fixes #126518

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/620011f6-5d77-4684-9948-36a463ece85e

Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Apr 3, 2026
Copilot AI changed the title [WIP] Review changes related to type forwarder behavior ILLink: Don't preserve forwarder assemblies referenced by link-action root assemblies Apr 3, 2026
Copilot AI requested a review from sbomer April 3, 2026 22:08
sbomer
sbomer reviewed Apr 3, 2026
View reviewed changes
@sbomer
Separate exported type marking from TypeReferenceMarker
007698d 
Move the CreateTypeReferenceForExportedTypeTarget helper out of TypeReferenceMarker
and into MarkStep, used by MarkExportedTypes. TypeReferenceMarker.ProcessExportedType
is now a no-op since MarkExportedTypes handles all exported type marking (always called
before TypeReferenceMarker). This makes TypeReferenceMarker focused solely on marking
forwarded scopes of type references.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/3f923948-9778-4cb0-90d8-1fdfae2875d2

Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
Copilot AI requested a review from sbomer April 3, 2026 22:51
sbomer
sbomer reviewed Apr 3, 2026
View reviewed changes
sbomer
sbomer reviewed Apr 3, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

@copilot please address feedback.

@sbomer
Move TypeReferenceMarker call to copy/save branch in MarkAssembly
320ffb8 
MarkEntireAssembly now only marks all types, custom attributes, and
exported types — no action check needed. TypeReferenceMarker.MarkTypeReferences
is called directly in the copy/save branch of MarkAssembly, where it
conceptually belongs. The link-action root path calls MarkEntireAssembly
without the type reference scope walk, since SweepStep rewrites those
references anyway.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d97f6dea-e5d1-499b-b24e-384fc89ddca7

Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 3, 2026

@copilot please address feedback.

Addressed in 320ffb8 — TypeReferenceMarker.MarkTypeReferences is now called in the copy/save branch of MarkAssembly instead of inside MarkEntireAssembly.

Copilot AI requested a review from sbomer April 3, 2026 23:51
sbomer
sbomer reviewed Apr 6, 2026
View reviewed changes
@sbomer
Separate MarkExportedType from MarkForwardedScope in exported type ha…
878c7e6 
…ndling

MarkExportedTypes now only marks the export entry itself (MarkExportedType)
for all root assemblies. MarkForwardedScope for exported types is moved back
into TypeReferenceMarker.ProcessExportedType, which already only runs for
copy/save assemblies. This keeps the two concerns separate: marking the
forwarder entry vs. preserving the assembly it points to.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/a0ee3bc2-d2ba-46c6-9177-76b764d96d85

Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
Copilot AI requested a review from sbomer April 6, 2026 18:50
@sbomer
Revert CreateTypeReferenceForExportedTypeTarget to instance method
830933a 
Undo the static refactoring of CreateTypeReferenceForExportedTypeTarget,
keeping it as an instance method on TypeReferenceMarker that captures
assembly.MainModule from the enclosing class.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

This review was generated by Copilot.

ILLink Type-Forwarder Marking Review — copilot/review-type-forwarder-behavior vs 1d274840

Summary of Changes

The PR splits MarkEntireAssembly into two concerns:

  1. Exported type marking (MarkExportedTypes) — always runs inside MarkEntireAssembly for both copy/save and linked root assemblies.
  2. Forwarded scope / type-reference scope marking (TypeReferenceMarker) — now runs only for copy/save assemblies, not for linked root assemblies.

In TypeReferenceMarker.ProcessExportedType, the MarkExportedType call is removed (it's now handled by the standalone MarkExportedTypes method), leaving only MarkForwardedScope.

Finding 1 — Core behavior change is correct

Severity: Info

For linked assemblies (including root assemblies with link action), SweepStep.SweepAssembly → SweepAssemblyReferences → AssemblyReferencesCorrector rewrites all type-reference and exported-type scopes to point directly to the implementation assembly. Forwarding assemblies are bypassed in the rewritten metadata. Therefore, pre-marking forwarded scopes during the mark phase was unnecessary — it kept forwarder assemblies alive when the sweep would have removed them anyway. The new test (UsedForwarderIsRemovedWhenReferencedByRootedAssembly) correctly validates that the forwarder is now removed.

For copy/save assemblies, type references are not rewritten by SweepStep, so TypeReferenceMarker must still run. The PR correctly restricts the call to this path only.

Finding 2 — Missing test: root assembly that is itself a type forwarder

Severity: Low (test gap)

The new test covers a root assembly that references a forwarding assembly. It does not cover a root linked assembly that has its own exported types (i.e., is itself a forwarder). In this case:

  • MarkExportedTypes marks the assembly's own exported types.
  • TypeReferenceMarker no longer runs, so the forwarding chain from the root's exported types is not pre-walked.
  • AssemblyReferencesCorrector.ProcessExportedType in the sweep rewrites exported-type scopes to the implementation, so this should still work correctly.

A test confirming this path would strengthen confidence. Existing tests like TypeForwarderOnlyAssembliesRemoved likely cover this indirectly, but none explicitly combine -a <assembly> rooting with a forwarder-hosting assembly.

Finding 3 — Dependency-info source change for copy/save exported types

Severity: Info (no behavioral impact)

Before: MarkExportedType was first called from TypeReferenceMarker.ProcessExportedType, so the dependency-info source was the walker path. After: MarkExportedTypes marks them first with DependencyInfo(DependencyKind.ExportedType, assembly). Since MarkProcessed is idempotent (returns false on re-mark), the first caller wins. The recorded dependency source changes but both use DependencyKind.ExportedType with assembly, so the dependency graph is functionally identical.

Finding 4 — Ordering between MarkExportedTypesTarget and MarkExportedTypes

Severity: Info (no issue)

MarkExportedTypesTarget.ProcessAssembly (called early in MarkAssembly) only processes already-marked exported types for preserved-member resolution. The actual marking in MarkExportedTypes happens later inside MarkEntireAssembly. This ordering is correct because MarkExportedTypesTarget handles exported types marked by previous marking iterations (e.g., via link.xml), not the current MarkAssembly call.

Finding 5 — No regression risk for chained forwarding

Severity: Info

If root assembly A → forwarder B → implementation C, the change intentionally stops marking B from A's mark phase. When types are actually used, resolution follows A→B→C and marks C directly. During sweep, AssemblyReferencesCorrector rewrites A's scopes to C. B is correctly removable if nothing else references it.

Conclusion

The change is correct and well-motivated. It eliminates unnecessary preservation of forwarder assemblies when the linker will rewrite references anyway. The only actionable suggestion is adding a test for a root assembly that is itself a forwarder (Finding 2). No correctness issues or regressions found.

Generated by Code Review for issue #126519 · ◷

sbomer
sbomer reviewed Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

@copilot add a test for Finding 2.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 6, 2026

@copilot add a test for Finding 2.

Done in cbbf72e. Added RootedForwarderWithExportedTypesIsHandled.cs — it roots Forwarder.dll with -a Forwarder (link action) while Forwarder.dll is itself a type forwarder to Implementation.dll. The test asserts that the exported type entry in Forwarder.dll is kept (via MarkExportedTypes), Implementation.dll is preserved (because the type is used), and test.dll's reference to Forwarder is rewritten away by the sweep.

Copilot AI requested a review from sbomer April 6, 2026 20:40
This was referenced Apr 8, 2026
Open
Open
@sbomer sbomer marked this pull request as ready for review April 8, 2026 17:52
Copilot AI review requested due to automatic review settings April 8, 2026 17:52
Copilot AI reviewed Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adjusts ILLink’s marking behavior so that rooting a link-action assembly no longer unintentionally preserves referenced type-forwarder assemblies, relying instead on the sweep step’s reference rewriting. This aligns TypeReferenceMarker usage back with its historical intent (copy/save assemblies) while still ensuring exported type entries are recorded for rooted assemblies.

Changes:

  • Moved TypeReferenceMarker.MarkTypeReferences out of MarkEntireAssembly and into the copy/save path in MarkAssembly.
  • Added MarkExportedTypes so rooted assemblies always record their own ExportedType entries without preserving forwarder assemblies.
  • Added two TypeForwarding test cases to validate forwarder removal/rewrite for rooted link assemblies, including a rooted forwarder scenario.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs Splits exported-type entry marking from type-reference scope marking so forwarders aren’t preserved for rooted link assemblies.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/UsedForwarderIsRemovedWhenReferencedByRootedAssembly.cs Regression coverage: rooted consumer (-a test, link) shouldn’t keep Forwarder.dll, and reference should be rewritten away.
src/tools/illink/test/Mono.Linker.Tests.Cases/TypeForwarding/RootedForwarderWithExportedTypesIsHandled.cs Coverage for a rooted forwarder (-a Forwarder, link): exported type entries preserved, impl kept if used, and consumer reference rewritten.

@sbomer
Copy link
Copy Markdown
Member

sbomer commented Apr 8, 2026

@copilot apply changes based on the comments in this thread

@sbomer
Apply review feedback: fix indentation in test file and pass origin t…
c53905a 
…o MarkExportedTypes

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/0b1ef5fb-462b-4dc2-b733-efa071753b64

Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 8, 2026

@copilot apply changes based on the comments in this thread

Done in c53905a:

  • RootedForwarderWithExportedTypesIsHandled.cs: reformatted to use 4-space indentation and removed extra spaces in attribute/method invocations to match the repo style.
  • MarkExportedTypes: now accepts and reuses the origin parameter passed in from MarkEntireAssembly, eliminating the per-exported-type new MessageOrigin(assembly) allocation.

@sbomer sbomer requested a review from a team April 8, 2026 18:42
Copilot AI requested a review from sbomer April 8, 2026 18:43
This was referenced Apr 9, 2026
Open
Open
Open
Open
Open
Open
Copilot AI mentioned this pull request Apr 11, 2026
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

+1 more reviewer

@sbomer sbomer sbomer approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@sbomer sbomer

Copilot code review Copilot

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework

Projects

AppModel
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sbomer