Allow ExternalTypeMap to have duplicates entries with identical mapping but different trimTarget

[jtschuster]

Fixes#120160

[Copilot]

Pull Request Overview

This PR allows ExternalTypeMap to have duplicate entries with identical mapping but different trim targets, fixing issue #120160. The change enables multiple TypeMap attributes with the same key and target type but different trim targets to coexist without throwing duplicate key exceptions.

Key changes:

• Modified duplicate detection logic to only throw exceptions when mappings conflict (same key but different target types)
• Added test cases to verify duplicate mappings with different trim targets are properly handled
• Updated nullable annotations in test code for better type safety

Reviewed Changes

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

File	Description
TypeMapLazyDictionary.cs	Modified duplicate detection logic and added Equals method for TypeNameUtf8 comparison
TypeMap.cs	Added test cases for duplicate mappings and updated nullable annotations

[dotnet-policy-service]

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

[AaronRobinsonMSFT]

LGTM

[jtschuster]

Updated ILC to allow duplicates in the external typemap.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[jtschuster]

@simonrozsival Can you confirm if still required for Android?

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "ae2cf83c570cc68cb51e39d4b0d7d7abe5f5e7ef",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "ece7b4ad376abec5817c637ad4f0c1c11b537202",
"last_reviewed_commit": "ae2cf83c570cc68cb51e39d4b0d7d7abe5f5e7ef",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "ece7b4ad376abec5817c637ad4f0c1c11b537202",
"last_recorded_worker_run_id": "29679157065",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ae2cf83c570cc68cb51e39d4b0d7d7abe5f5e7ef",
"review_id": 4730524688
}
]
}

[github-actions]

Holistic Review

Motivation: The change addresses a real, filed bug (#120160): the ExternalTypeMap infrastructure rejected multiple TypeMap attributes that share the same key and target type but declare different trim targets, even though that is a legitimate authoring pattern. Relaxing this to only reject conflicting mappings is well-justified.

Approach: The approach is consistent across the three implementation surfaces. The AOT metadata layer (TypeMapMetadata) now stores a List<TypeDesc> of trimming targets per key and only throws when the target type genuinely differs; ExternalTypeMapNode fans out its dependency/marking logic over that list; and the lazy runtime dictionary tolerates duplicates whose target type-name bytes match. Tests were updated to cover both the newly-valid duplicate case and a still-invalid conflicting case, on both the AOT/trimming path and the runtime path.

Summary: ✅ LGTM. The logic is coherent and the semantics (dedupe identical, throw on conflict, treat a null trim target as unconditional) are preserved on each path. Tests meaningfully exercise both the valid and invalid duplicate scenarios. I raised one non-blocking inline observation about a subtle difference between the runtime path (compares target name bytes) and the AOT path (compares resolved TypeDesc identity); it is unlikely to matter in practice but worth a maintainer confirming is intentional. Two maintainers (AaronRobinsonMSFT, jkoritzinsky) have already approved.

---

Detailed Findings

✅ Correctness — Duplicate/conflict handling is consistent

TypeMapMetadata.AddExternalTypeMapEntry now appends to the trimming-target list when the key exists and the mapped type matches, and throws BadImageFormatException only on a true type conflict. MergePendingMap correctly re-adds each trimming target individually so cross-module merges preserve the same invariants. LazyExternalTypeDictionary.Add mirrors this at runtime: it returns silently on a matching duplicate and throws AddingDuplicateWithKeyArgumentException on a mismatch, while still checking the pre-cached dictionaries for the non-duplicate path.

✅ ExternalTypeMapNode — List fan-out preserves prior semantics

The three overrides correctly generalize the previous single-target logic: a null trimming target still means "unconditional" (target always kept), and GetConditionalStaticDependencies now skips emitting conditional dependencies entirely if any target in the list is unconditional. GetStaticDependencies and CompareToImpl each break after the first satisfying target, avoiding duplicate yields for the same key.

✅ Tests — Both valid and invalid duplicate cases covered

The runtime interop test (TypeMapApp.cs) splits the old DuplicateTypeNameKey into ValidDuplicateTypeNameKey (same target, extra trim targets — must succeed) and InvalidDuplicateTypeNameKey (conflicting targets — must throw), and adds Validate_ExternalTypeMapping_ValidDuplicateTypeKey. The trimming test adds a duplicate-key/different-trim-target mapping plus a TryGetValue assertion. This gives good coverage of the new branch on both paths.

💡 Minor — Runtime vs. AOT equality basis

See the inline comment on LazyExternalTypeDictionary.Add: the runtime dedupe compares target type-name UTF-8 bytes, whereas the AOT path compares resolved TypeDesc identity. Non-blocking, but worth confirming the byte-name comparison is a deliberate and sufficient equivalence.

Note

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

Generated by Holistic Review · 82.1 AIC · ⌖ 15.1 AIC · ⊞ 10K