Update TypeMap attribute handling #120477

jtschuster · 2025-10-07T18:31:15Z

Fixes #120394

Mark TypeMapAssemblyTargetAttributes when its TypeMapUniverse is marked
Recurse through TypeMapAssemblyTarget assemblies to find all TypeMapAttributes
Refactor TypeMapHandler construction and initialization
Use TypeReferenceEqualityComparer for Dictionaries
Mark TypeMap attribute origin / assembly when a TypeMap attribute is marked. Otherwise, assemblies with only TypeMapAttributes will not be kept.
Add more test coverage

- Mark TypeMapAssemblyTargetAttributes when a typemapuniverse is marked - Recurse through TypeMapAssemblyTarget assemblies to find all TypeMapAttributes - Refactor TypeMapHandler construction and initialization - Use TypeReferenceEqualityComparer for Dictionaries - Add more test coverage

Copilot

Pull Request Overview

This PR updates TypeMap attribute handling in the IL linker to fix issues with assembly preservation and attribute marking. The changes improve how TypeMapAttributes are handled across assemblies and ensure assemblies containing only TypeMap attributes are properly preserved.

Key changes:

Enhanced TypeMapHandler to recursively process TypeMapAssemblyTarget assemblies
Improved attribute marking logic to mark assemblies when TypeMap attributes need to be preserved
Added comprehensive test coverage for cross-assembly TypeMap scenarios

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs	Extended test case with cross-assembly TypeMap scenarios and dependency validation
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/Dependencies/TypeMapSecondOrderReference.cs	New test dependency assembly with TypeMap attributes for second-order reference testing
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/Dependencies/TypeMapReferencedAssembly.cs	New test dependency assembly with TypeMap attributes for primary reference testing
src/tools/illink/src/linker/Linker/TypeReferenceEqualityComparer.cs	Added obsolete marker for Default property to prevent misuse
src/tools/illink/src/linker/Linker/TypeMapHandler.cs	Major refactor of TypeMapHandler with improved initialization, recursive assembly processing, and proper assembly marking
src/tools/illink/src/linker/Linker.Steps/SweepStep.cs	Updated IsMarkedAssembly to check both assembly and main module marking
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs	Simplified TypeMapHandler initialization and made MarkAssembly method public

src/tools/illink/src/linker/Linker/TypeMapHandler.cs

src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs

dotnet-policy-service · 2025-10-07T18:32:21Z

Tagging subscribers to this area: @dotnet/illink
See info in area-owners.md if you want to be subscribed.

Sergio0694 · 2025-10-07T18:33:40Z

@jkoritzinsky does this need to be backported to .NET 10? As in, I assume this bug would break CsWinRT 3.0 too? 🤔

…ap.cs Co-authored-by: Copilot <[email protected]>

jtschuster · 2025-10-07T23:21:30Z

src/tools/illink/src/linker/Linker.Steps/SweepStep.cs

        bool IsMarkedAssembly(AssemblyDefinition assembly)
        {
-            return Annotations.IsMarked(assembly.MainModule);
+            return Annotations.IsMarked(assembly) || Annotations.IsMarked(assembly.MainModule);


An assembly that only had MarkAssembly called would be not be kept if the MainModule wasn't marked. If it makes more sense to mark the MainModule in MarkAssembly rather than have this check, I'm happy to change.

jtschuster · 2025-10-07T23:22:50Z

src/tools/illink/src/linker/Linker/TypeReferenceEqualityComparer.cs

    internal sealed class TypeReferenceEqualityComparer : EqualityComparer<TypeReference>
    {
+        [Obsolete("Default will point to default object equality comparer. Use the constructor to create an instance with a resolver.")]
+        public static new object Default => throw new InvalidOperationException();


Accidentally used TypeReferenceEqualityComparer.Default, which went to ObjectEqualityComparer.Default and required more debugging time than it should have. Adding this property to avoid doing that again.

sbomer · 2025-10-08T20:44:23Z

src/tools/illink/src/linker/Linker/TypeMapHandler.cs

            public void Resolve(LinkContext context, TypeMapHandler manager)
            {
-                foreach (AssemblyNameReference assemblyName in assemblies)
+                HashSet<AssemblyDefinition> visited = new();


nit: check _assembly here and return before allocating anything if it's null

sbomer · 2025-10-08T20:49:20Z

src/tools/illink/src/linker/Linker/TypeMapHandler.cs

+                                var nextAssemblyName = AssemblyNameReference.Parse(str);
+                                if (context.TryResolve(nextAssemblyName) is AssemblyDefinition nextAssembly)
+                                {
+                                    if (!visited.Contains(nextAssembly) && !toVisit.Contains(nextAssembly))


Add items to visited on enqueue above, instead of dequeue, then we can avoid the linear scan through toVisit here.

sbomer · 2025-10-08T21:32:02Z

src/tools/illink/src/linker/Linker/TypeMapHandler.cs

-                MarkTypeMapAttribute(entry, new DependencyInfo(DependencyKind.TypeMapEntry, callingMethod));
+                foreach (var entry in assemblyTargets)
+                {
+                    var info = new DependencyInfo(DependencyKind.TypeMapEntry, callingMethod);


nit: TypeMapEntry doesn't seem like it quite fits the usage - maybe add another DependencyKind?

sbomer · 2025-10-08T21:33:04Z

src/tools/illink/src/linker/Linker/TypeMapHandler.cs

+                {
+                    var info = new DependencyInfo(DependencyKind.TypeMapEntry, callingMethod);
+                    _markStep.MarkCustomAttribute(entry.Attribute, info, new MessageOrigin(entry.Origin));
+                    _markStep.MarkAssembly(entry.Origin, info, new MessageOrigin(entry.Origin));


Can this use MarkTypeMapAttribute instead? Same below.

src/tools/illink/src/linker/Linker/TypeMapHandler.cs

sbomer · 2025-10-08T21:43:20Z

src/tools/illink/src/linker/Linker/TypeMapHandler.cs

+            if (seenTypeGroups.Contains(group) && attr.DependencySourceRequiresTarget(_context, dependencyTypeDef))
            {
-                MarkTypeMapAttribute(attr, new DependencyInfo(DependencyKind.TypeMapEntry, trimTarget));
+                MarkTypeMapAttribute(attr, new DependencyInfo(DependencyKind.TypeMapEntry, dependencySource));


This will go to the else case even if the dependency source doesn't require the target - should the seenTypeGroups and the else below be inside the DependencySourceRequiresTarget check?

Yes, good catch. We should split first on DependencySourceRequiresTarget(), then seenTypeGroups.Contains() to match the other processing flow (pending dependency source -> pending typeMapGroup -> mark).

sbomer · 2025-10-08T21:49:26Z

src/tools/illink/src/linker/Linker/TypeMapHandler.cs

+                {
+                    "TypeMapAttribute`1" =>
+                        sourceType is null || context.Annotations.IsRelevantToVariantCasting(sourceType)
+                        || context.Annotations.IsInstantiated(sourceType),


I think for the external type map, the types aren't required to be instantiated or marked relevant to variant casting. I think the _typeMapHandler.ProcessType(type) in MarkInstruction is intended to keep type map entries in more cases.

Yes, I meant to leave a comment that it also should require the target when the source is a target of an IsInst instruction. "Target of IsInst" just doesn't have an associated annotation and I'm a little hesitant to add another cache for this. This should handle all "annotatable" ways that the target type is required at initialization. I think as long as this is only called during initialization and we keep the ProcessType() call for IsInst instructions we should be okay. I can add a comment to make that clear or refactor it to make it only visible to RecordTypeMapEntry.

sbomer · 2025-10-08T21:54:33Z

src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs

    {
        [Kept]
        [ExpectedWarning("IL2057", "Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String)'")]
+        [ExpectBodyModified] // Bug


What's the bug?

Thanks for noting this, I forgot to add a comment. It looks like the UnusedTypeChecks optimization is messing with the TypeMap attributes. The if (obj is UsedTrimTarget) checks are being modified. I'll a better example and a github issue for this.

jtschuster requested review from sbomer and jkoritzinsky October 7, 2025 18:31

jtschuster requested a review from marek-safar as a code owner October 7, 2025 18:31

Copilot AI review requested due to automatic review settings October 7, 2025 18:31

github-actions bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Oct 7, 2025

dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Oct 7, 2025

dotnet-policy-service bot assigned jtschuster Oct 7, 2025

Copilot AI reviewed Oct 7, 2025

View reviewed changes

jtschuster and others added 2 commits October 7, 2025 11:40

Fix compiler error for last minute change

620e1f7

Update src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeM…

781c154

…ap.cs Co-authored-by: Copilot <[email protected]>

build-analysis bot mentioned this pull request Oct 7, 2025

[wasm/browser] Failed to connect to socket /run/dbus/system_bus_socket #120176

Open

jtschuster commented Oct 7, 2025

View reviewed changes

jtschuster added this to AppModel Oct 8, 2025

AaronRobinsonMSFT added this to the 11.0.0 milestone Oct 8, 2025

sbomer reviewed Oct 8, 2025

View reviewed changes

Update TypeMap attribute handling #120477

Are you sure you want to change the base?

Update TypeMap attribute handling #120477

Uh oh!

Conversation

jtschuster commented Oct 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dotnet-policy-service bot commented Oct 7, 2025

Uh oh!

Sergio0694 commented Oct 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!