Skip to content

[TrimmableTypeMap] Fix inherited generic base typemap refs#11749

Open
simonrozsival wants to merge 8 commits into
dotnet:mainfrom
simonrozsival:android-fix-trimmable-typemap-constructed-generic-base-codegen
Open

[TrimmableTypeMap] Fix inherited generic base typemap refs#11749
simonrozsival wants to merge 8 commits into
dotnet:mainfrom
simonrozsival:android-fix-trimmable-typemap-constructed-generic-base-codegen

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Jun 26, 2026

Copy link
Copy Markdown
Member

Goal

Extract the generator/scanner bugfix from #11617 for inherited callbacks declared on constructed generic base types.

The full #11617 PR implements a much larger reflection-free trimmable typemap runtime path. This PR intentionally ports only the pieces needed to fix the generator build failure caused by constructed generic base types, so it can be reviewed and merged independently.

Problem

The scanner could resolve a generic base TypeSpec, but it flattened the result down to a string-like managed type name and lost the generic arguments. That meant an inherited registered callback declared on a closed generic base, for example GenericSelectionHost<string>.SetSelection(int), could flow into the typemap model as if it were declared on the open generic type GenericSelectionHost1`.

The generated typemap assembly then tried to emit callback/constructor member references without enough metadata to represent the constructed declaring type. In affected app hierarchies, this produced invalid or unresolvable generated metadata and surfaced as build errors.

Fix

This keeps exact constructed generic type information through the generator pipeline:

  • Adds TypeRefData.GenericArguments and preserves generic instantiations decoded from metadata signatures.
  • Substitutes generic type parameters while walking inherited base chains, including forwarding generic intermediate bases.
  • Carries exact declaring TypeRefData for inherited marshal methods and activation constructors.
  • Emits constructed generic TypeSpec signatures/member refs from PEAssemblyBuilder instead of flattening everything to plain TypeRefs.
  • Includes constructed generic type data in typemap content fingerprints so incremental generation stays correct.

Scope

This is deliberately narrower than #11617. It does not port the broader trimmable value-manager/type-manager runtime work, NativeAOT default changes, manifest parity fixes, or typemap proxy-association restoration from that branch.

Tests

Added regression coverage for:

  • Override detection across a non-generic MCW base that closes a generic registered base.
  • Override detection across a generic forwarding intermediate MCW base.
  • Generated typemap metadata using a constructed generic TypeSpec member reference for inherited callbacks.

Local validation:

dotnet test tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests.csproj --no-restore

Port the trimmable typemap generator fix from PR dotnet#11617 for constructed generic base types. Preserve constructed generic type refs through scanning and emit TypeSpec member refs for inherited callbacks.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 26, 2026 09:34
@simonrozsival simonrozsival changed the title Fix inherited generic base typemap refs [TrimmableTypeMap] Fix inherited generic base typemap refs Jun 26, 2026
@simonrozsival simonrozsival added copilot `copilot-cli` or other AIs were used to author this trimmable-type-map labels Jun 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes trimmable typemap scanning/generation for inherited callbacks declared on constructed generic base types (e.g., forwarding generic hierarchies), ensuring the generator preserves the constructed base type information and emits the correct metadata (TypeSpec-based member refs). It also adds regression coverage for generic/forwarding MCW base hierarchies.

Changes:

  • Extend the scanner to preserve/propagate constructed generic base type information via TypeRefData.GenericArguments and generic-argument substitution when walking base hierarchies.
  • Update the generator/emitter to resolve constructed generic types as TypeSpec and emit signatures using structured TypeRefData rather than parsing generic strings.
  • Add/extend tests covering override detection and verifying emitted member refs use constructed-generic parents.
Show a summary per file
File Description
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/TestFixtures/TestTypes.cs Adds new forwarding generic test fixture hierarchy for override detection/regression coverage.
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Scanner/OverrideDetectionTests.cs Adds assertions validating the constructed generic base declaring type is preserved.
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs Adds metadata-level test ensuring inherited generic-base callbacks use TypeSpec member refs.
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TrimmableTypeMapGeneratorTests.cs Updates generator tests’ assembly input wiring (currently introduces a compile-breaking AssemblyInput reference).
src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/SignatureTypeProvider.cs Changes generic instantiation representation to use TypeRefData.GenericArguments.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerScanner.cs Propagates constructed generic base type info through base-walk logic and caches/declaring-type resolution.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerInfo.cs Extends model to carry exact declaring type (TypeRefData) for callbacks/activation ctors.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs Uses structured TypeRefData signature emission for ctor member refs.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/PEAssemblyBuilder.cs Adds TypeSpec emission for constructed generics and centralizes type-signature writing.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ModelBuilder.cs Plumbs constructed declaring types into proxy/UCO callback models.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/Model/TypeMapAssemblyData.cs Adds GenericArguments and DisplayName to represent constructed generics without string-encoding.
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/MetadataHelper.cs Includes generic arguments (and array proxy metadata) in deterministic content fingerprinting.

Copilot's findings

  • Files reviewed: 12/12 changed files
  • Comments generated: 10

simonrozsival and others added 3 commits June 26, 2026 12:06
Use tuple inputs in generator tests for the current generator API and reject structured generic TypeRefData in export dispatch unsupported-type checks.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cover the new TypeRefData.GenericArguments shape in export dispatch unsupported-type validation so constructed generic signature types continue to be rejected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restore the AssemblyInput generator API from PR dotnet#11617 so this branch stays aligned with follow-up trimmable typemap work, while keeping tuple scanner compatibility for existing tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival

Copy link
Copy Markdown
Member Author

Follow-up after the review fixes: I kept AssemblyInput intentionally by adding src/Microsoft.Android.Sdk.TrimmableTypeMap/AssemblyInput.cs and wiring TrimmableTypeMapGenerator/GenerateTrimmableTypeMap/the generator tests to use it. This addresses the compile concern while keeping this PR aligned with #11617 and follow-up trimmable typemap work. The lower-level scanner tuple overload remains for existing tests.

simonrozsival and others added 2 commits June 26, 2026 13:37
Remove array-proxy fingerprinting from this narrower generator fix and add the missing LINQ import needed by the AssemblyInput scanner compatibility overload.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Substitute constructed generic base arguments before comparing inherited registered method and constructor signatures. Include UCO metadata in deterministic typemap fingerprints so constructed callback type changes produce distinct MVIDs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown

Android PR Reviewer completed successfully!

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Review: ⚠️ Needs Changes

Nicely-scoped extraction from #11617. The core fix is sound: threading TypeRefData.GenericArguments through the scanner (SubstituteGenericArguments, TryResolveBaseType, FindBaseRegisteredMethodInfo) and emitting constructed-generic TypeSpec member refs in PEAssemblyBuilder correctly preserves closed generic bases (e.g. inherited callbacks on GenericSelectionHost<string>), instead of flattening them to the open definition.

The fingerprint change in MetadataHelper is an important correctness fix on its own — recursing into GenericArguments and hashing the UCO methods/constructors/native registrations means GenericBase<string> vs GenericBase<object> no longer collide to the same MVID and cause stale incremental builds. Test coverage is strong: Generate_DifferentConstructedCallbackTypes_ProducesDifferentMVIDs, the substituted-parameter override-detection tests, and the unsupported-export cases all exercise the new paths.

A few items to consider before merge:

  • ⚠️ 1 warning — generic arguments are never marked value-type/enum, so a constructed base over an enum/struct argument would emit ELEMENT_TYPE_CLASS and produce an unresolvable member ref. Rare for Java bindings, but the emitter already honors IsEnum for the declaring type, so closing the argument gap (or guarding it + filing a follow-up) is worthwhile.
  • 💡 2 suggestions — the now-dead ResolveTypeReference helper, and the record value-equality foot-gun introduced by the new IReadOnlyList<> member.

Details are in the inline comments — none are hard blockers, but the ⚠️ is worth a decision before merge.

CI note: at review time the Azure DevOps build (Linux/macOS/Windows) was still in progress and mergeable_state is blocked; please confirm it goes green before merging. I could not build locally — the external/Java.Interop submodule isn't checked out in the review environment, so this review is static-analysis only.

Generated by Android PR Reviewer for issue #11749 · 2.6K AIC · ⌖ 47.9 AIC · ⊞ 37.9K
Comment /review to run again

Track value-type generic arguments when decoding TypeRefData so constructed generic TypeSpecs emit VALUETYPE where required. Add element-wise TypeRefData equality for generic arguments and remove the obsolete type-reference resolver helper.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown

Android PR Reviewer completed successfully!

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Code Review

Reviewed the change that threads exact constructed-generic type info (TypeRefData.GenericArguments / IsValueType / DisplayName) through the scanner and PE emitter, so inherited callbacks declared on a closed generic base emit valid TypeSpec member refs instead of collapsing to the open type (e.g. GenericSelectionHost\1). I traced the substitution logic (SubstituteGenericArguments), the override comparison (HaveIdenticalParameterTypes), the signature/TypeSpec emission (PEAssemblyBuilder.WriteTypeSignature/WriteGenericInstantiationSignature/ResolveTypeRef), and the structural TypeRefData.Equals/GetHashCode`.

Assessment: high-quality, well-targeted fix. Highlights:

  • Excellent regression coverage — the new fixtures/tests exercise the real failure modes directly: generic forwarding through multiple bases, substituted method parameters, value-type/enum arguments encoded as ELEMENT_TYPE_VALUETYPE (0x11), the emitted TypeSpec blob shape, and TypeRefData structural equality.
  • Correct incremental-build fix — including GenericArguments (plus UCO methods/ctors/native registrations) in ComputeContentFingerprint makes GenericBase(string) vs GenericBase(object) produce distinct MVIDs, pinned by Generate_DifferentConstructedCallbackTypes_ProducesDifferentMVIDs.
  • Custom Equals/GetHashCode correctly compensates for IReadOnlyList reference-equality in records, and the compared fields are consistent between the two.
  • EncodeAsValueType (IsValueType || IsEnum) is a nice generalization over the previous enum-only check, and the array/!N/!!N handling in WriteTypeSignature lines up with the substitution path.

I found no blocking issues. One low-severity, non-blocking maintainability note is posted inline (override-matching depends on two independent formatters — SignatureTypeProvider string output vs TypeRefData.DisplayName — staying in lockstep).

🛈 CI: the Azure DevOps builds (Linux / macOS / Windows) were still in progress at review time — not green yet. Worth confirming they pass before merge.

Generated by Android PR Reviewer for issue #11749 · 2.2K AIC · ⌖ 46.4 AIC · ⊞ 40K
Comment /review to run again

Comment thread src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerScanner.cs Outdated
Decode both inherited and derived method signatures as TypeRefData so override matching compares the shared structured representation instead of parallel string formatters.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival simonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

copilot `copilot-cli` or other AIs were used to author this ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). trimmable-type-map

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants