Fix type map assembly target traversal in ILC

[jkoritzinsky]

Type map assembly target references should only be followed for the provided type map group type (ie, type maps in target assemblies that aren't referenced by AssemblyTarget for that type group shouldn't preserve types).

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a bug in the ILC (IL Compiler) type map assembly target traversal logic. The issue was that when processing TypeMapAssemblyTarget attributes, the compiler would include ALL type maps from target assemblies, not just the type maps for the specific type map group referenced by the attribute.

Changes:

• Modified TypeMapMetadata.CreateFromAssembly to properly scope assembly traversal to specific type map groups
• Added TypeMapAssemblyTargetsMode enum to control whether target assemblies are traversed or just recorded
• Implemented a pending maps mechanism to handle cross-assembly type map references efficiently
• Added test coverage for the UsedTypeMapUniverse scenario to validate the fix

Reviewed changes

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

File	Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypeMapMetadata.cs	Core fix: tracks assembly+group pairs instead of just assemblies, passes type map group when enqueuing target assemblies, adds pending maps mechanism, adds TypeMapAssemblyTargetsMode enum
src/coreclr/tools/aot/ILCompiler/Program.cs	Updates three call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TrimmingDriver.cs	Updates two call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs	Adds test case for UsedTypeMapUniverse to verify type maps in library2.dll aren't included when traversing from library.dll's TypeMapAssemblyTarget

[jtschuster]

LGTM if/once we have negative tests.

[Sergio0694]

@jkoritzinsky is this something we'd hit too in CsWinRT 3.0 and that would be nice to get back ported after it's merged?

[jkoritzinsky]

I don't expect CsWinRT 3.0 to need this change as all of the different type map groups are in the same or similar assemblies and you are correctly emitting the assembly target attributes.

This is more a correctness fix to ensure that when an assembly target attribute is not present, that we won't pick up additional entries.

For the expected use cases, there's never a scenario where that would happen intentionally (ie this causing a behavior change in CsWinRT 3.0 would be exposing a bug there).

[Sergio0694]

I see, thank you for explaining! 🙂

[stephentoub]

🤖 Copilot Code Review — PR #124247

Holistic Assessment

Motivation: The PR addresses a real bug where TypeMapAssemblyTarget attributes were incorrectly including ALL type maps from target assemblies rather than only the type maps for the specific type map group referenced by the attribute. This is a valid correctness issue that needs fixing.

Approach: The fix correctly changes the tracking from just assemblies to (assembly, typeMapGroup) pairs. The pendingMaps mechanism handles the case where an assembly is scanned while looking for one group but contains maps for other groups that may be needed later. This is a sound approach.

Summary: ⚠️Needs Human Review. The core algorithm appears correct and the implementation properly scopes type map discovery per group. However, there are unused code additions that warrant discussion, and a reviewer noted that negative test cases are needed. The approved review is conditional on adding those tests.

---

Detailed Findings

✅ Correct pendingMaps Keying

The Gemini model initially raised a concern about pendingMaps potentially leaking type maps across groups. Upon verification, the implementation correctly keys pendingMaps by (EcmaAssembly assembly, TypeDesc typeMapGroup):

Dictionary<(EcmaAssemblyassembly,TypeDesctypeMapGroup),(TypeDescscannedDuringGroup,Mapmap)>pendingMaps=[];

This ensures that pending maps are only merged when the specific (assembly, group) pair is legitimately reached, preventing cross-group contamination.

✅ MergePendingMap Exception Handling

The MergePendingMap method correctly handles the exception stub priority:

• If the target map already has an exception stub, no changes are made (existing exception takes precedence)
• If the pending map has an exception stub, it's adopted
• Otherwise, entries are merged

The existing GetProxyTypeMapNode and GetExternalTypeMapNode methods correctly check the exception stub first and return failure nodes, ignoring partial entries.

⚠️ Unused Code — _targetModules, AddTargetModule, TargetModules, TypeMapAssemblyTargetsMode.Record

Several additions are unused in this PR:

• private readonly List<ModuleDesc> _targetModules = [];
• public void AddTargetModule(ModuleDesc targetModule)
• public IEnumerable<ModuleDesc> TargetModules
• TypeMapAssemblyTargetsMode.Record

Per review comments, the author confirmed these are for future R2R support. This is acceptable for a PR extracted from a larger change, but:

1. The documentation comment on TargetModules explains the purpose well
2. Consider whether dead code analysis tools will flag these

Recommendation: Acceptable given the explanation, but ensure the follow-up R2R PR arrives soon to avoid orphaned code.

⚠️ Test Coverage — Missing Negative Test Cases

Reviewer @jtschuster noted that negative test cases are needed to validate that TypeMap<T> attributes in "library" which aren't targeted by TypeMapAssemblyTarget<T>("library") are properly removed. The author committed to adding this test.

Status: The approval was conditional on this ("LGTM if/once we have negative tests"). Verify the test has been added before merging.

💡 Extra Blank Line

There's a double blank line after the scannedAssemblies.Add call:

scannedAssemblies.Add((currentAssembly,currentTypeMapGroup));}voidProcessTypeMapAssemblyTargetAttribute(...)

Minor style issue — consider removing one blank line.

✅ API Signature Change

The change from CompilerTypeSystemContext to the base TypeSystemContext and explicit ModuleDesc throwHelperEmitModule parameter improves flexibility and makes the dependency on the generated assembly explicit. This is a reasonable API change.

---

Cross-cutting Analysis

I verified that all callers of CreateFromAssembly have been updated:

• ILCompiler/Program.cs (3 call sites) — ✅ Updated
• TrimmingDriver.cs (2 call sites) — ✅ Updated

All callers pass TypeMapAssemblyTargetsMode.Traverse which is the active behavior. The Record mode is unused but documented for R2R support.

---

Summary

The core fix is correct and well-designed. The pendingMaps mechanism properly handles the complex case of type maps discovered during scans for different groups. Before merging:

1. Confirm negative test cases have been added as promised in the review comments
2. The unused code is acceptable given the R2R follow-up context

Verdict: ⚠️Needs Human Review — Verify the negative tests have been added per the conditional approval.

[Copilot]

Pull request overview

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

[jkoritzinsky]

Type map assembly target references should only be followed for the provided type map group type (ie, type maps in target assemblies that aren't referenced by AssemblyTarget for that type group shouldn't preserve types).

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a bug in the ILC (IL Compiler) type map assembly target traversal logic. The issue was that when processing TypeMapAssemblyTarget attributes, the compiler would include ALL type maps from target assemblies, not just the type maps for the specific type map group referenced by the attribute.

Changes:

• Modified TypeMapMetadata.CreateFromAssembly to properly scope assembly traversal to specific type map groups
• Added TypeMapAssemblyTargetsMode enum to control whether target assemblies are traversed or just recorded
• Implemented a pending maps mechanism to handle cross-assembly type map references efficiently
• Added test coverage for the UsedTypeMapUniverse scenario to validate the fix

Reviewed changes

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

File	Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypeMapMetadata.cs	Core fix: tracks assembly+group pairs instead of just assemblies, passes type map group when enqueuing target assemblies, adds pending maps mechanism, adds TypeMapAssemblyTargetsMode enum
src/coreclr/tools/aot/ILCompiler/Program.cs	Updates three call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TrimmingDriver.cs	Updates two call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs	Adds test case for UsedTypeMapUniverse to verify type maps in library2.dll aren't included when traversing from library.dll's TypeMapAssemblyTarget

[jtschuster]

LGTM if/once we have negative tests.

[Sergio0694]

@jkoritzinsky is this something we'd hit too in CsWinRT 3.0 and that would be nice to get back ported after it's merged?

[jkoritzinsky]

I don't expect CsWinRT 3.0 to need this change as all of the different type map groups are in the same or similar assemblies and you are correctly emitting the assembly target attributes.

This is more a correctness fix to ensure that when an assembly target attribute is not present, that we won't pick up additional entries.

For the expected use cases, there's never a scenario where that would happen intentionally (ie this causing a behavior change in CsWinRT 3.0 would be exposing a bug there).

[Sergio0694]

I see, thank you for explaining! 🙂

[stephentoub]

🤖 Copilot Code Review — PR #124247

Holistic Assessment

Motivation: The PR addresses a real bug where TypeMapAssemblyTarget attributes were incorrectly including ALL type maps from target assemblies rather than only the type maps for the specific type map group referenced by the attribute. This is a valid correctness issue that needs fixing.

Approach: The fix correctly changes the tracking from just assemblies to (assembly, typeMapGroup) pairs. The pendingMaps mechanism handles the case where an assembly is scanned while looking for one group but contains maps for other groups that may be needed later. This is a sound approach.

Summary: ⚠️Needs Human Review. The core algorithm appears correct and the implementation properly scopes type map discovery per group. However, there are unused code additions that warrant discussion, and a reviewer noted that negative test cases are needed. The approved review is conditional on adding those tests.

---

Detailed Findings

✅ Correct pendingMaps Keying

The Gemini model initially raised a concern about pendingMaps potentially leaking type maps across groups. Upon verification, the implementation correctly keys pendingMaps by (EcmaAssembly assembly, TypeDesc typeMapGroup):

Dictionary<(EcmaAssemblyassembly,TypeDesctypeMapGroup),(TypeDescscannedDuringGroup,Mapmap)>pendingMaps=[];

This ensures that pending maps are only merged when the specific (assembly, group) pair is legitimately reached, preventing cross-group contamination.

✅ MergePendingMap Exception Handling

The MergePendingMap method correctly handles the exception stub priority:

• If the target map already has an exception stub, no changes are made (existing exception takes precedence)
• If the pending map has an exception stub, it's adopted
• Otherwise, entries are merged

The existing GetProxyTypeMapNode and GetExternalTypeMapNode methods correctly check the exception stub first and return failure nodes, ignoring partial entries.

⚠️ Unused Code — _targetModules, AddTargetModule, TargetModules, TypeMapAssemblyTargetsMode.Record

Several additions are unused in this PR:

• private readonly List<ModuleDesc> _targetModules = [];
• public void AddTargetModule(ModuleDesc targetModule)
• public IEnumerable<ModuleDesc> TargetModules
• TypeMapAssemblyTargetsMode.Record

Per review comments, the author confirmed these are for future R2R support. This is acceptable for a PR extracted from a larger change, but:

1. The documentation comment on TargetModules explains the purpose well
2. Consider whether dead code analysis tools will flag these

Recommendation: Acceptable given the explanation, but ensure the follow-up R2R PR arrives soon to avoid orphaned code.

⚠️ Test Coverage — Missing Negative Test Cases

Reviewer @jtschuster noted that negative test cases are needed to validate that TypeMap<T> attributes in "library" which aren't targeted by TypeMapAssemblyTarget<T>("library") are properly removed. The author committed to adding this test.

Status: The approval was conditional on this ("LGTM if/once we have negative tests"). Verify the test has been added before merging.

💡 Extra Blank Line

There's a double blank line after the scannedAssemblies.Add call:

scannedAssemblies.Add((currentAssembly,currentTypeMapGroup));}voidProcessTypeMapAssemblyTargetAttribute(...)

Minor style issue — consider removing one blank line.

✅ API Signature Change

The change from CompilerTypeSystemContext to the base TypeSystemContext and explicit ModuleDesc throwHelperEmitModule parameter improves flexibility and makes the dependency on the generated assembly explicit. This is a reasonable API change.

---

Cross-cutting Analysis

I verified that all callers of CreateFromAssembly have been updated:

• ILCompiler/Program.cs (3 call sites) — ✅ Updated
• TrimmingDriver.cs (2 call sites) — ✅ Updated

All callers pass TypeMapAssemblyTargetsMode.Traverse which is the active behavior. The Record mode is unused but documented for R2R support.

---

Summary

The core fix is correct and well-designed. The pendingMaps mechanism properly handles the complex case of type maps discovered during scans for different groups. Before merging:

1. Confirm negative test cases have been added as promised in the review comments
2. The unused code is acceptable given the R2R follow-up context

Verdict: ⚠️Needs Human Review — Verify the negative tests have been added per the conditional approval.

[Copilot]

Pull request overview

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

[jkoritzinsky]

Type map assembly target references should only be followed for the provided type map group type (ie, type maps in target assemblies that aren't referenced by AssemblyTarget for that type group shouldn't preserve types).

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a bug in the ILC (IL Compiler) type map assembly target traversal logic. The issue was that when processing TypeMapAssemblyTarget attributes, the compiler would include ALL type maps from target assemblies, not just the type maps for the specific type map group referenced by the attribute.

Changes:

• Modified TypeMapMetadata.CreateFromAssembly to properly scope assembly traversal to specific type map groups
• Added TypeMapAssemblyTargetsMode enum to control whether target assemblies are traversed or just recorded
• Implemented a pending maps mechanism to handle cross-assembly type map references efficiently
• Added test coverage for the UsedTypeMapUniverse scenario to validate the fix

Reviewed changes

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

File	Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypeMapMetadata.cs	Core fix: tracks assembly+group pairs instead of just assemblies, passes type map group when enqueuing target assemblies, adds pending maps mechanism, adds TypeMapAssemblyTargetsMode enum
src/coreclr/tools/aot/ILCompiler/Program.cs	Updates three call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TrimmingDriver.cs	Updates two call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs	Adds test case for UsedTypeMapUniverse to verify type maps in library2.dll aren't included when traversing from library.dll's TypeMapAssemblyTarget

[jtschuster]

LGTM if/once we have negative tests.

[Sergio0694]

@jkoritzinsky is this something we'd hit too in CsWinRT 3.0 and that would be nice to get back ported after it's merged?

[jkoritzinsky]

I don't expect CsWinRT 3.0 to need this change as all of the different type map groups are in the same or similar assemblies and you are correctly emitting the assembly target attributes.

This is more a correctness fix to ensure that when an assembly target attribute is not present, that we won't pick up additional entries.

For the expected use cases, there's never a scenario where that would happen intentionally (ie this causing a behavior change in CsWinRT 3.0 would be exposing a bug there).

[Sergio0694]

I see, thank you for explaining! 🙂

[stephentoub]

🤖 Copilot Code Review — PR #124247

Holistic Assessment

Motivation: The PR addresses a real bug where TypeMapAssemblyTarget attributes were incorrectly including ALL type maps from target assemblies rather than only the type maps for the specific type map group referenced by the attribute. This is a valid correctness issue that needs fixing.

Approach: The fix correctly changes the tracking from just assemblies to (assembly, typeMapGroup) pairs. The pendingMaps mechanism handles the case where an assembly is scanned while looking for one group but contains maps for other groups that may be needed later. This is a sound approach.

Summary: ⚠️Needs Human Review. The core algorithm appears correct and the implementation properly scopes type map discovery per group. However, there are unused code additions that warrant discussion, and a reviewer noted that negative test cases are needed. The approved review is conditional on adding those tests.

---

Detailed Findings

✅ Correct pendingMaps Keying

The Gemini model initially raised a concern about pendingMaps potentially leaking type maps across groups. Upon verification, the implementation correctly keys pendingMaps by (EcmaAssembly assembly, TypeDesc typeMapGroup):

Dictionary<(EcmaAssemblyassembly,TypeDesctypeMapGroup),(TypeDescscannedDuringGroup,Mapmap)>pendingMaps=[];

This ensures that pending maps are only merged when the specific (assembly, group) pair is legitimately reached, preventing cross-group contamination.

✅ MergePendingMap Exception Handling

The MergePendingMap method correctly handles the exception stub priority:

• If the target map already has an exception stub, no changes are made (existing exception takes precedence)
• If the pending map has an exception stub, it's adopted
• Otherwise, entries are merged

The existing GetProxyTypeMapNode and GetExternalTypeMapNode methods correctly check the exception stub first and return failure nodes, ignoring partial entries.

⚠️ Unused Code — _targetModules, AddTargetModule, TargetModules, TypeMapAssemblyTargetsMode.Record

Several additions are unused in this PR:

• private readonly List<ModuleDesc> _targetModules = [];
• public void AddTargetModule(ModuleDesc targetModule)
• public IEnumerable<ModuleDesc> TargetModules
• TypeMapAssemblyTargetsMode.Record

Per review comments, the author confirmed these are for future R2R support. This is acceptable for a PR extracted from a larger change, but:

1. The documentation comment on TargetModules explains the purpose well
2. Consider whether dead code analysis tools will flag these

Recommendation: Acceptable given the explanation, but ensure the follow-up R2R PR arrives soon to avoid orphaned code.

⚠️ Test Coverage — Missing Negative Test Cases

Reviewer @jtschuster noted that negative test cases are needed to validate that TypeMap<T> attributes in "library" which aren't targeted by TypeMapAssemblyTarget<T>("library") are properly removed. The author committed to adding this test.

Status: The approval was conditional on this ("LGTM if/once we have negative tests"). Verify the test has been added before merging.

💡 Extra Blank Line

There's a double blank line after the scannedAssemblies.Add call:

scannedAssemblies.Add((currentAssembly,currentTypeMapGroup));}voidProcessTypeMapAssemblyTargetAttribute(...)

Minor style issue — consider removing one blank line.

✅ API Signature Change

The change from CompilerTypeSystemContext to the base TypeSystemContext and explicit ModuleDesc throwHelperEmitModule parameter improves flexibility and makes the dependency on the generated assembly explicit. This is a reasonable API change.

---

Cross-cutting Analysis

I verified that all callers of CreateFromAssembly have been updated:

• ILCompiler/Program.cs (3 call sites) — ✅ Updated
• TrimmingDriver.cs (2 call sites) — ✅ Updated

All callers pass TypeMapAssemblyTargetsMode.Traverse which is the active behavior. The Record mode is unused but documented for R2R support.

---

Summary

The core fix is correct and well-designed. The pendingMaps mechanism properly handles the complex case of type maps discovered during scans for different groups. Before merging:

1. Confirm negative test cases have been added as promised in the review comments
2. The unused code is acceptable given the R2R follow-up context

Verdict: ⚠️Needs Human Review — Verify the negative tests have been added per the conditional approval.

[Copilot]

Pull request overview

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

[jkoritzinsky]

Type map assembly target references should only be followed for the provided type map group type (ie, type maps in target assemblies that aren't referenced by AssemblyTarget for that type group shouldn't preserve types).

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a bug in the ILC (IL Compiler) type map assembly target traversal logic. The issue was that when processing TypeMapAssemblyTarget attributes, the compiler would include ALL type maps from target assemblies, not just the type maps for the specific type map group referenced by the attribute.

Changes:

• Modified TypeMapMetadata.CreateFromAssembly to properly scope assembly traversal to specific type map groups
• Added TypeMapAssemblyTargetsMode enum to control whether target assemblies are traversed or just recorded
• Implemented a pending maps mechanism to handle cross-assembly type map references efficiently
• Added test coverage for the UsedTypeMapUniverse scenario to validate the fix

Reviewed changes

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

File	Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypeMapMetadata.cs	Core fix: tracks assembly+group pairs instead of just assemblies, passes type map group when enqueuing target assemblies, adds pending maps mechanism, adds TypeMapAssemblyTargetsMode enum
src/coreclr/tools/aot/ILCompiler/Program.cs	Updates three call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TrimmingDriver.cs	Updates two call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs	Adds test case for UsedTypeMapUniverse to verify type maps in library2.dll aren't included when traversing from library.dll's TypeMapAssemblyTarget

[jtschuster]

LGTM if/once we have negative tests.

[Sergio0694]

@jkoritzinsky is this something we'd hit too in CsWinRT 3.0 and that would be nice to get back ported after it's merged?

[jkoritzinsky]

I don't expect CsWinRT 3.0 to need this change as all of the different type map groups are in the same or similar assemblies and you are correctly emitting the assembly target attributes.

This is more a correctness fix to ensure that when an assembly target attribute is not present, that we won't pick up additional entries.

For the expected use cases, there's never a scenario where that would happen intentionally (ie this causing a behavior change in CsWinRT 3.0 would be exposing a bug there).

[Sergio0694]

I see, thank you for explaining! 🙂

[stephentoub]

🤖 Copilot Code Review — PR #124247

Holistic Assessment

Motivation: The PR addresses a real bug where TypeMapAssemblyTarget attributes were incorrectly including ALL type maps from target assemblies rather than only the type maps for the specific type map group referenced by the attribute. This is a valid correctness issue that needs fixing.

Approach: The fix correctly changes the tracking from just assemblies to (assembly, typeMapGroup) pairs. The pendingMaps mechanism handles the case where an assembly is scanned while looking for one group but contains maps for other groups that may be needed later. This is a sound approach.

Summary: ⚠️Needs Human Review. The core algorithm appears correct and the implementation properly scopes type map discovery per group. However, there are unused code additions that warrant discussion, and a reviewer noted that negative test cases are needed. The approved review is conditional on adding those tests.

---

Detailed Findings

✅ Correct pendingMaps Keying

The Gemini model initially raised a concern about pendingMaps potentially leaking type maps across groups. Upon verification, the implementation correctly keys pendingMaps by (EcmaAssembly assembly, TypeDesc typeMapGroup):

Dictionary<(EcmaAssemblyassembly,TypeDesctypeMapGroup),(TypeDescscannedDuringGroup,Mapmap)>pendingMaps=[];

This ensures that pending maps are only merged when the specific (assembly, group) pair is legitimately reached, preventing cross-group contamination.

✅ MergePendingMap Exception Handling

The MergePendingMap method correctly handles the exception stub priority:

• If the target map already has an exception stub, no changes are made (existing exception takes precedence)
• If the pending map has an exception stub, it's adopted
• Otherwise, entries are merged

The existing GetProxyTypeMapNode and GetExternalTypeMapNode methods correctly check the exception stub first and return failure nodes, ignoring partial entries.

⚠️ Unused Code — _targetModules, AddTargetModule, TargetModules, TypeMapAssemblyTargetsMode.Record

Several additions are unused in this PR:

• private readonly List<ModuleDesc> _targetModules = [];
• public void AddTargetModule(ModuleDesc targetModule)
• public IEnumerable<ModuleDesc> TargetModules
• TypeMapAssemblyTargetsMode.Record

Per review comments, the author confirmed these are for future R2R support. This is acceptable for a PR extracted from a larger change, but:

1. The documentation comment on TargetModules explains the purpose well
2. Consider whether dead code analysis tools will flag these

Recommendation: Acceptable given the explanation, but ensure the follow-up R2R PR arrives soon to avoid orphaned code.

⚠️ Test Coverage — Missing Negative Test Cases

Reviewer @jtschuster noted that negative test cases are needed to validate that TypeMap<T> attributes in "library" which aren't targeted by TypeMapAssemblyTarget<T>("library") are properly removed. The author committed to adding this test.

Status: The approval was conditional on this ("LGTM if/once we have negative tests"). Verify the test has been added before merging.

💡 Extra Blank Line

There's a double blank line after the scannedAssemblies.Add call:

scannedAssemblies.Add((currentAssembly,currentTypeMapGroup));}voidProcessTypeMapAssemblyTargetAttribute(...)

Minor style issue — consider removing one blank line.

✅ API Signature Change

The change from CompilerTypeSystemContext to the base TypeSystemContext and explicit ModuleDesc throwHelperEmitModule parameter improves flexibility and makes the dependency on the generated assembly explicit. This is a reasonable API change.

---

Cross-cutting Analysis

I verified that all callers of CreateFromAssembly have been updated:

• ILCompiler/Program.cs (3 call sites) — ✅ Updated
• TrimmingDriver.cs (2 call sites) — ✅ Updated

All callers pass TypeMapAssemblyTargetsMode.Traverse which is the active behavior. The Record mode is unused but documented for R2R support.

---

Summary

The core fix is correct and well-designed. The pendingMaps mechanism properly handles the complex case of type maps discovered during scans for different groups. Before merging:

1. Confirm negative test cases have been added as promised in the review comments
2. The unused code is acceptable given the R2R follow-up context

Verdict: ⚠️Needs Human Review — Verify the negative tests have been added per the conditional approval.

[Copilot]

Pull request overview

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

[jkoritzinsky]

Type map assembly target references should only be followed for the provided type map group type (ie, type maps in target assemblies that aren't referenced by AssemblyTarget for that type group shouldn't preserve types).

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a bug in the ILC (IL Compiler) type map assembly target traversal logic. The issue was that when processing TypeMapAssemblyTarget attributes, the compiler would include ALL type maps from target assemblies, not just the type maps for the specific type map group referenced by the attribute.

Changes:

• Modified TypeMapMetadata.CreateFromAssembly to properly scope assembly traversal to specific type map groups
• Added TypeMapAssemblyTargetsMode enum to control whether target assemblies are traversed or just recorded
• Implemented a pending maps mechanism to handle cross-assembly type map references efficiently
• Added test coverage for the UsedTypeMapUniverse scenario to validate the fix

Reviewed changes

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

File	Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypeMapMetadata.cs	Core fix: tracks assembly+group pairs instead of just assemblies, passes type map group when enqueuing target assemblies, adds pending maps mechanism, adds TypeMapAssemblyTargetsMode enum
src/coreclr/tools/aot/ILCompiler/Program.cs	Updates three call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TrimmingDriver.cs	Updates two call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs	Adds test case for UsedTypeMapUniverse to verify type maps in library2.dll aren't included when traversing from library.dll's TypeMapAssemblyTarget

[jtschuster]

LGTM if/once we have negative tests.

[Sergio0694]

@jkoritzinsky is this something we'd hit too in CsWinRT 3.0 and that would be nice to get back ported after it's merged?

[jkoritzinsky]

I don't expect CsWinRT 3.0 to need this change as all of the different type map groups are in the same or similar assemblies and you are correctly emitting the assembly target attributes.

This is more a correctness fix to ensure that when an assembly target attribute is not present, that we won't pick up additional entries.

For the expected use cases, there's never a scenario where that would happen intentionally (ie this causing a behavior change in CsWinRT 3.0 would be exposing a bug there).

[Sergio0694]

I see, thank you for explaining! 🙂

[stephentoub]

🤖 Copilot Code Review — PR #124247

Holistic Assessment

Motivation: The PR addresses a real bug where TypeMapAssemblyTarget attributes were incorrectly including ALL type maps from target assemblies rather than only the type maps for the specific type map group referenced by the attribute. This is a valid correctness issue that needs fixing.

Approach: The fix correctly changes the tracking from just assemblies to (assembly, typeMapGroup) pairs. The pendingMaps mechanism handles the case where an assembly is scanned while looking for one group but contains maps for other groups that may be needed later. This is a sound approach.

Summary: ⚠️Needs Human Review. The core algorithm appears correct and the implementation properly scopes type map discovery per group. However, there are unused code additions that warrant discussion, and a reviewer noted that negative test cases are needed. The approved review is conditional on adding those tests.

---

Detailed Findings

✅ Correct pendingMaps Keying

The Gemini model initially raised a concern about pendingMaps potentially leaking type maps across groups. Upon verification, the implementation correctly keys pendingMaps by (EcmaAssembly assembly, TypeDesc typeMapGroup):

Dictionary<(EcmaAssemblyassembly,TypeDesctypeMapGroup),(TypeDescscannedDuringGroup,Mapmap)>pendingMaps=[];

This ensures that pending maps are only merged when the specific (assembly, group) pair is legitimately reached, preventing cross-group contamination.

✅ MergePendingMap Exception Handling

The MergePendingMap method correctly handles the exception stub priority:

• If the target map already has an exception stub, no changes are made (existing exception takes precedence)
• If the pending map has an exception stub, it's adopted
• Otherwise, entries are merged

The existing GetProxyTypeMapNode and GetExternalTypeMapNode methods correctly check the exception stub first and return failure nodes, ignoring partial entries.

⚠️ Unused Code — _targetModules, AddTargetModule, TargetModules, TypeMapAssemblyTargetsMode.Record

Several additions are unused in this PR:

• private readonly List<ModuleDesc> _targetModules = [];
• public void AddTargetModule(ModuleDesc targetModule)
• public IEnumerable<ModuleDesc> TargetModules
• TypeMapAssemblyTargetsMode.Record

Per review comments, the author confirmed these are for future R2R support. This is acceptable for a PR extracted from a larger change, but:

1. The documentation comment on TargetModules explains the purpose well
2. Consider whether dead code analysis tools will flag these

Recommendation: Acceptable given the explanation, but ensure the follow-up R2R PR arrives soon to avoid orphaned code.

⚠️ Test Coverage — Missing Negative Test Cases

Reviewer @jtschuster noted that negative test cases are needed to validate that TypeMap<T> attributes in "library" which aren't targeted by TypeMapAssemblyTarget<T>("library") are properly removed. The author committed to adding this test.

Status: The approval was conditional on this ("LGTM if/once we have negative tests"). Verify the test has been added before merging.

💡 Extra Blank Line

There's a double blank line after the scannedAssemblies.Add call:

scannedAssemblies.Add((currentAssembly,currentTypeMapGroup));}voidProcessTypeMapAssemblyTargetAttribute(...)

Minor style issue — consider removing one blank line.

✅ API Signature Change

The change from CompilerTypeSystemContext to the base TypeSystemContext and explicit ModuleDesc throwHelperEmitModule parameter improves flexibility and makes the dependency on the generated assembly explicit. This is a reasonable API change.

---

Cross-cutting Analysis

I verified that all callers of CreateFromAssembly have been updated:

• ILCompiler/Program.cs (3 call sites) — ✅ Updated
• TrimmingDriver.cs (2 call sites) — ✅ Updated

All callers pass TypeMapAssemblyTargetsMode.Traverse which is the active behavior. The Record mode is unused but documented for R2R support.

---

Summary

The core fix is correct and well-designed. The pendingMaps mechanism properly handles the complex case of type maps discovered during scans for different groups. Before merging:

1. Confirm negative test cases have been added as promised in the review comments
2. The unused code is acceptable given the R2R follow-up context

Verdict: ⚠️Needs Human Review — Verify the negative tests have been added per the conditional approval.

[Copilot]

Pull request overview

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

[jkoritzinsky]

Type map assembly target references should only be followed for the provided type map group type (ie, type maps in target assemblies that aren't referenced by AssemblyTarget for that type group shouldn't preserve types).

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a bug in the ILC (IL Compiler) type map assembly target traversal logic. The issue was that when processing TypeMapAssemblyTarget attributes, the compiler would include ALL type maps from target assemblies, not just the type maps for the specific type map group referenced by the attribute.

Changes:

• Modified TypeMapMetadata.CreateFromAssembly to properly scope assembly traversal to specific type map groups
• Added TypeMapAssemblyTargetsMode enum to control whether target assemblies are traversed or just recorded
• Implemented a pending maps mechanism to handle cross-assembly type map references efficiently
• Added test coverage for the UsedTypeMapUniverse scenario to validate the fix

Reviewed changes

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

File	Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypeMapMetadata.cs	Core fix: tracks assembly+group pairs instead of just assemblies, passes type map group when enqueuing target assemblies, adds pending maps mechanism, adds TypeMapAssemblyTargetsMode enum
src/coreclr/tools/aot/ILCompiler/Program.cs	Updates three call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TrimmingDriver.cs	Updates two call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs	Adds test case for UsedTypeMapUniverse to verify type maps in library2.dll aren't included when traversing from library.dll's TypeMapAssemblyTarget

[jtschuster]

LGTM if/once we have negative tests.

[Sergio0694]

@jkoritzinsky is this something we'd hit too in CsWinRT 3.0 and that would be nice to get back ported after it's merged?

[jkoritzinsky]

I don't expect CsWinRT 3.0 to need this change as all of the different type map groups are in the same or similar assemblies and you are correctly emitting the assembly target attributes.

This is more a correctness fix to ensure that when an assembly target attribute is not present, that we won't pick up additional entries.

For the expected use cases, there's never a scenario where that would happen intentionally (ie this causing a behavior change in CsWinRT 3.0 would be exposing a bug there).

[Sergio0694]

I see, thank you for explaining! 🙂

[stephentoub]

🤖 Copilot Code Review — PR #124247

Holistic Assessment

Motivation: The PR addresses a real bug where TypeMapAssemblyTarget attributes were incorrectly including ALL type maps from target assemblies rather than only the type maps for the specific type map group referenced by the attribute. This is a valid correctness issue that needs fixing.

Approach: The fix correctly changes the tracking from just assemblies to (assembly, typeMapGroup) pairs. The pendingMaps mechanism handles the case where an assembly is scanned while looking for one group but contains maps for other groups that may be needed later. This is a sound approach.

Summary: ⚠️Needs Human Review. The core algorithm appears correct and the implementation properly scopes type map discovery per group. However, there are unused code additions that warrant discussion, and a reviewer noted that negative test cases are needed. The approved review is conditional on adding those tests.

---

Detailed Findings

✅ Correct pendingMaps Keying

The Gemini model initially raised a concern about pendingMaps potentially leaking type maps across groups. Upon verification, the implementation correctly keys pendingMaps by (EcmaAssembly assembly, TypeDesc typeMapGroup):

Dictionary<(EcmaAssemblyassembly,TypeDesctypeMapGroup),(TypeDescscannedDuringGroup,Mapmap)>pendingMaps=[];

This ensures that pending maps are only merged when the specific (assembly, group) pair is legitimately reached, preventing cross-group contamination.

✅ MergePendingMap Exception Handling

The MergePendingMap method correctly handles the exception stub priority:

• If the target map already has an exception stub, no changes are made (existing exception takes precedence)
• If the pending map has an exception stub, it's adopted
• Otherwise, entries are merged

The existing GetProxyTypeMapNode and GetExternalTypeMapNode methods correctly check the exception stub first and return failure nodes, ignoring partial entries.

⚠️ Unused Code — _targetModules, AddTargetModule, TargetModules, TypeMapAssemblyTargetsMode.Record

Several additions are unused in this PR:

• private readonly List<ModuleDesc> _targetModules = [];
• public void AddTargetModule(ModuleDesc targetModule)
• public IEnumerable<ModuleDesc> TargetModules
• TypeMapAssemblyTargetsMode.Record

Per review comments, the author confirmed these are for future R2R support. This is acceptable for a PR extracted from a larger change, but:

1. The documentation comment on TargetModules explains the purpose well
2. Consider whether dead code analysis tools will flag these

Recommendation: Acceptable given the explanation, but ensure the follow-up R2R PR arrives soon to avoid orphaned code.

⚠️ Test Coverage — Missing Negative Test Cases

Reviewer @jtschuster noted that negative test cases are needed to validate that TypeMap<T> attributes in "library" which aren't targeted by TypeMapAssemblyTarget<T>("library") are properly removed. The author committed to adding this test.

Status: The approval was conditional on this ("LGTM if/once we have negative tests"). Verify the test has been added before merging.

💡 Extra Blank Line

There's a double blank line after the scannedAssemblies.Add call:

scannedAssemblies.Add((currentAssembly,currentTypeMapGroup));}voidProcessTypeMapAssemblyTargetAttribute(...)

Minor style issue — consider removing one blank line.

✅ API Signature Change

The change from CompilerTypeSystemContext to the base TypeSystemContext and explicit ModuleDesc throwHelperEmitModule parameter improves flexibility and makes the dependency on the generated assembly explicit. This is a reasonable API change.

---

Cross-cutting Analysis

I verified that all callers of CreateFromAssembly have been updated:

• ILCompiler/Program.cs (3 call sites) — ✅ Updated
• TrimmingDriver.cs (2 call sites) — ✅ Updated

All callers pass TypeMapAssemblyTargetsMode.Traverse which is the active behavior. The Record mode is unused but documented for R2R support.

---

Summary

The core fix is correct and well-designed. The pendingMaps mechanism properly handles the complex case of type maps discovered during scans for different groups. Before merging:

1. Confirm negative test cases have been added as promised in the review comments
2. The unused code is acceptable given the R2R follow-up context

Verdict: ⚠️Needs Human Review — Verify the negative tests have been added per the conditional approval.

[Copilot]

Pull request overview

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

[jkoritzinsky]

Type map assembly target references should only be followed for the provided type map group type (ie, type maps in target assemblies that aren't referenced by AssemblyTarget for that type group shouldn't preserve types).

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a bug in the ILC (IL Compiler) type map assembly target traversal logic. The issue was that when processing TypeMapAssemblyTarget attributes, the compiler would include ALL type maps from target assemblies, not just the type maps for the specific type map group referenced by the attribute.

Changes:

• Modified TypeMapMetadata.CreateFromAssembly to properly scope assembly traversal to specific type map groups
• Added TypeMapAssemblyTargetsMode enum to control whether target assemblies are traversed or just recorded
• Implemented a pending maps mechanism to handle cross-assembly type map references efficiently
• Added test coverage for the UsedTypeMapUniverse scenario to validate the fix

Reviewed changes

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

File	Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypeMapMetadata.cs	Core fix: tracks assembly+group pairs instead of just assemblies, passes type map group when enqueuing target assemblies, adds pending maps mechanism, adds TypeMapAssemblyTargetsMode enum
src/coreclr/tools/aot/ILCompiler/Program.cs	Updates three call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TrimmingDriver.cs	Updates two call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs	Adds test case for UsedTypeMapUniverse to verify type maps in library2.dll aren't included when traversing from library.dll's TypeMapAssemblyTarget

[jtschuster]

LGTM if/once we have negative tests.

[Sergio0694]

@jkoritzinsky is this something we'd hit too in CsWinRT 3.0 and that would be nice to get back ported after it's merged?

[jkoritzinsky]

I don't expect CsWinRT 3.0 to need this change as all of the different type map groups are in the same or similar assemblies and you are correctly emitting the assembly target attributes.

This is more a correctness fix to ensure that when an assembly target attribute is not present, that we won't pick up additional entries.

For the expected use cases, there's never a scenario where that would happen intentionally (ie this causing a behavior change in CsWinRT 3.0 would be exposing a bug there).

[Sergio0694]

I see, thank you for explaining! 🙂

[stephentoub]

🤖 Copilot Code Review — PR #124247

Holistic Assessment

Motivation: The PR addresses a real bug where TypeMapAssemblyTarget attributes were incorrectly including ALL type maps from target assemblies rather than only the type maps for the specific type map group referenced by the attribute. This is a valid correctness issue that needs fixing.

Approach: The fix correctly changes the tracking from just assemblies to (assembly, typeMapGroup) pairs. The pendingMaps mechanism handles the case where an assembly is scanned while looking for one group but contains maps for other groups that may be needed later. This is a sound approach.

Summary: ⚠️Needs Human Review. The core algorithm appears correct and the implementation properly scopes type map discovery per group. However, there are unused code additions that warrant discussion, and a reviewer noted that negative test cases are needed. The approved review is conditional on adding those tests.

---

Detailed Findings

✅ Correct pendingMaps Keying

The Gemini model initially raised a concern about pendingMaps potentially leaking type maps across groups. Upon verification, the implementation correctly keys pendingMaps by (EcmaAssembly assembly, TypeDesc typeMapGroup):

Dictionary<(EcmaAssemblyassembly,TypeDesctypeMapGroup),(TypeDescscannedDuringGroup,Mapmap)>pendingMaps=[];

This ensures that pending maps are only merged when the specific (assembly, group) pair is legitimately reached, preventing cross-group contamination.

✅ MergePendingMap Exception Handling

The MergePendingMap method correctly handles the exception stub priority:

• If the target map already has an exception stub, no changes are made (existing exception takes precedence)
• If the pending map has an exception stub, it's adopted
• Otherwise, entries are merged

The existing GetProxyTypeMapNode and GetExternalTypeMapNode methods correctly check the exception stub first and return failure nodes, ignoring partial entries.

⚠️ Unused Code — _targetModules, AddTargetModule, TargetModules, TypeMapAssemblyTargetsMode.Record

Several additions are unused in this PR:

• private readonly List<ModuleDesc> _targetModules = [];
• public void AddTargetModule(ModuleDesc targetModule)
• public IEnumerable<ModuleDesc> TargetModules
• TypeMapAssemblyTargetsMode.Record

Per review comments, the author confirmed these are for future R2R support. This is acceptable for a PR extracted from a larger change, but:

1. The documentation comment on TargetModules explains the purpose well
2. Consider whether dead code analysis tools will flag these

Recommendation: Acceptable given the explanation, but ensure the follow-up R2R PR arrives soon to avoid orphaned code.

⚠️ Test Coverage — Missing Negative Test Cases

Reviewer @jtschuster noted that negative test cases are needed to validate that TypeMap<T> attributes in "library" which aren't targeted by TypeMapAssemblyTarget<T>("library") are properly removed. The author committed to adding this test.

Status: The approval was conditional on this ("LGTM if/once we have negative tests"). Verify the test has been added before merging.

💡 Extra Blank Line

There's a double blank line after the scannedAssemblies.Add call:

scannedAssemblies.Add((currentAssembly,currentTypeMapGroup));}voidProcessTypeMapAssemblyTargetAttribute(...)

Minor style issue — consider removing one blank line.

✅ API Signature Change

The change from CompilerTypeSystemContext to the base TypeSystemContext and explicit ModuleDesc throwHelperEmitModule parameter improves flexibility and makes the dependency on the generated assembly explicit. This is a reasonable API change.

---

Cross-cutting Analysis

I verified that all callers of CreateFromAssembly have been updated:

• ILCompiler/Program.cs (3 call sites) — ✅ Updated
• TrimmingDriver.cs (2 call sites) — ✅ Updated

All callers pass TypeMapAssemblyTargetsMode.Traverse which is the active behavior. The Record mode is unused but documented for R2R support.

---

Summary

The core fix is correct and well-designed. The pendingMaps mechanism properly handles the complex case of type maps discovered during scans for different groups. Before merging:

1. Confirm negative test cases have been added as promised in the review comments
2. The unused code is acceptable given the R2R follow-up context

Verdict: ⚠️Needs Human Review — Verify the negative tests have been added per the conditional approval.

[Copilot]

Pull request overview

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

[jkoritzinsky]

Type map assembly target references should only be followed for the provided type map group type (ie, type maps in target assemblies that aren't referenced by AssemblyTarget for that type group shouldn't preserve types).

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a bug in the ILC (IL Compiler) type map assembly target traversal logic. The issue was that when processing TypeMapAssemblyTarget attributes, the compiler would include ALL type maps from target assemblies, not just the type maps for the specific type map group referenced by the attribute.

Changes:

• Modified TypeMapMetadata.CreateFromAssembly to properly scope assembly traversal to specific type map groups
• Added TypeMapAssemblyTargetsMode enum to control whether target assemblies are traversed or just recorded
• Implemented a pending maps mechanism to handle cross-assembly type map references efficiently
• Added test coverage for the UsedTypeMapUniverse scenario to validate the fix

Reviewed changes

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

File	Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypeMapMetadata.cs	Core fix: tracks assembly+group pairs instead of just assemblies, passes type map group when enqueuing target assemblies, adds pending maps mechanism, adds TypeMapAssemblyTargetsMode enum
src/coreclr/tools/aot/ILCompiler/Program.cs	Updates three call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TrimmingDriver.cs	Updates two call sites to pass new parameters (throwHelperEmitModule and TypeMapAssemblyTargetsMode.Traverse)
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs	Adds test case for UsedTypeMapUniverse to verify type maps in library2.dll aren't included when traversing from library.dll's TypeMapAssemblyTarget

[jtschuster]

LGTM if/once we have negative tests.

[Sergio0694]

@jkoritzinsky is this something we'd hit too in CsWinRT 3.0 and that would be nice to get back ported after it's merged?

[jkoritzinsky]

I don't expect CsWinRT 3.0 to need this change as all of the different type map groups are in the same or similar assemblies and you are correctly emitting the assembly target attributes.

This is more a correctness fix to ensure that when an assembly target attribute is not present, that we won't pick up additional entries.

For the expected use cases, there's never a scenario where that would happen intentionally (ie this causing a behavior change in CsWinRT 3.0 would be exposing a bug there).

[Sergio0694]

I see, thank you for explaining! 🙂

[stephentoub]

🤖 Copilot Code Review — PR #124247

Holistic Assessment

Motivation: The PR addresses a real bug where TypeMapAssemblyTarget attributes were incorrectly including ALL type maps from target assemblies rather than only the type maps for the specific type map group referenced by the attribute. This is a valid correctness issue that needs fixing.

Approach: The fix correctly changes the tracking from just assemblies to (assembly, typeMapGroup) pairs. The pendingMaps mechanism handles the case where an assembly is scanned while looking for one group but contains maps for other groups that may be needed later. This is a sound approach.

Summary: ⚠️Needs Human Review. The core algorithm appears correct and the implementation properly scopes type map discovery per group. However, there are unused code additions that warrant discussion, and a reviewer noted that negative test cases are needed. The approved review is conditional on adding those tests.

---

Detailed Findings

✅ Correct pendingMaps Keying

The Gemini model initially raised a concern about pendingMaps potentially leaking type maps across groups. Upon verification, the implementation correctly keys pendingMaps by (EcmaAssembly assembly, TypeDesc typeMapGroup):

Dictionary<(EcmaAssemblyassembly,TypeDesctypeMapGroup),(TypeDescscannedDuringGroup,Mapmap)>pendingMaps=[];

This ensures that pending maps are only merged when the specific (assembly, group) pair is legitimately reached, preventing cross-group contamination.

✅ MergePendingMap Exception Handling

The MergePendingMap method correctly handles the exception stub priority:

• If the target map already has an exception stub, no changes are made (existing exception takes precedence)
• If the pending map has an exception stub, it's adopted
• Otherwise, entries are merged

The existing GetProxyTypeMapNode and GetExternalTypeMapNode methods correctly check the exception stub first and return failure nodes, ignoring partial entries.

⚠️ Unused Code — _targetModules, AddTargetModule, TargetModules, TypeMapAssemblyTargetsMode.Record

Several additions are unused in this PR:

• private readonly List<ModuleDesc> _targetModules = [];
• public void AddTargetModule(ModuleDesc targetModule)
• public IEnumerable<ModuleDesc> TargetModules
• TypeMapAssemblyTargetsMode.Record

Per review comments, the author confirmed these are for future R2R support. This is acceptable for a PR extracted from a larger change, but:

1. The documentation comment on TargetModules explains the purpose well
2. Consider whether dead code analysis tools will flag these

Recommendation: Acceptable given the explanation, but ensure the follow-up R2R PR arrives soon to avoid orphaned code.

⚠️ Test Coverage — Missing Negative Test Cases

Reviewer @jtschuster noted that negative test cases are needed to validate that TypeMap<T> attributes in "library" which aren't targeted by TypeMapAssemblyTarget<T>("library") are properly removed. The author committed to adding this test.

Status: The approval was conditional on this ("LGTM if/once we have negative tests"). Verify the test has been added before merging.

💡 Extra Blank Line

There's a double blank line after the scannedAssemblies.Add call:

scannedAssemblies.Add((currentAssembly,currentTypeMapGroup));}voidProcessTypeMapAssemblyTargetAttribute(...)

Minor style issue — consider removing one blank line.

✅ API Signature Change

The change from CompilerTypeSystemContext to the base TypeSystemContext and explicit ModuleDesc throwHelperEmitModule parameter improves flexibility and makes the dependency on the generated assembly explicit. This is a reasonable API change.

---

Cross-cutting Analysis

I verified that all callers of CreateFromAssembly have been updated:

• ILCompiler/Program.cs (3 call sites) — ✅ Updated
• TrimmingDriver.cs (2 call sites) — ✅ Updated

All callers pass TypeMapAssemblyTargetsMode.Traverse which is the active behavior. The Record mode is unused but documented for R2R support.

---

Summary

The core fix is correct and well-designed. The pendingMaps mechanism properly handles the complex case of type maps discovered during scans for different groups. Before merging:

1. Confirm negative test cases have been added as promised in the review comments
2. The unused code is acceptable given the R2R follow-up context

Verdict: ⚠️Needs Human Review — Verify the negative tests have been added per the conditional approval.

[Copilot]

Pull request overview

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