[TrimmableTypeMap] Fix inherited generic base typemap refs#11749
Conversation
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>
simonrozsival
changed the title
simonrozsival
added
copilot
There was a problem hiding this comment.
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.GenericArgumentsand generic-argument substitution when walking base hierarchies. - Update the generator/emitter to resolve constructed generic types as
TypeSpecand emit signatures using structuredTypeRefDatarather 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
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>
|
Follow-up after the review fixes: I kept |
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>
|
/review |
|
✅ Android PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 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 emitELEMENT_TYPE_CLASSand produce an unresolvable member ref. Rare for Java bindings, but the emitter already honorsIsEnumfor the declaring type, so closing the argument gap (or guarding it + filing a follow-up) is worthwhile.- 💡 2 suggestions — the now-dead
ResolveTypeReferencehelper, and therecordvalue-equality foot-gun introduced by the newIReadOnlyList<>member.
Details are in the inline comments — none are hard blockers, but the
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>
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 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 emittedTypeSpecblob shape, andTypeRefDatastructural equality. - ✅ Correct incremental-build fix — including
GenericArguments(plus UCO methods/ctors/native registrations) inComputeContentFingerprintmakesGenericBase(string)vsGenericBase(object)produce distinct MVIDs, pinned byGenerate_DifferentConstructedCallbackTypes_ProducesDifferentMVIDs. - ✅ Custom
Equals/GetHashCodecorrectly compensates forIReadOnlyListreference-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/!!Nhandling inWriteTypeSignaturelines 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
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
added
the
ready-to-review
| public override int GetHashCode () | ||
| { | ||
| unchecked { | ||
| int hash = 17; | ||
| hash = hash * 31 + StringComparer.Ordinal.GetHashCode (ManagedTypeName); | ||
| hash = hash * 31 + StringComparer.Ordinal.GetHashCode (AssemblyName); | ||
| hash = hash * 31 + IsValueType.GetHashCode (); | ||
| hash = hash * 31 + IsEnum.GetHashCode (); | ||
| foreach (var argument in GenericArguments) { | ||
| hash = hash * 31 + argument.GetHashCode (); | ||
| } | ||
| return hash; | ||
| } | ||
| } |
There was a problem hiding this comment.
Not a blocker, but supposedly we can do:
<PackageReference Include="Microsoft.Bcl.HashCode" Version="6.0.0" Condition="'$(TargetFramework)' == 'netstandard2.0'" />
Then use HashCode.Combine().
There was a problem hiding this comment.
I'll do that in a follow-up PR
#11798) ## Problem When R8 code shrinking is enabled, R8 removes any Java type that nothing keeps. .NET for Android keeps the Java callable wrappers (JCWs) of bound managed types by generating `-keep` rules from the **acw-map** (managed peer ↔ Java type). That works for generated JCWs, but **user-authored `AndroidJavaSource` `.java` files marked `Bind` != `true` have no managed peer**, so they never appear in the acw-map — and therefore get no keep rule. With shrinking on, R8 silently deletes them, and the app fails at runtime when it references those classes (often as a `NoClassDefFoundError` that only reproduces in Release/shrunk builds). ## Fix Pass user `AndroidJavaSource` (`Bind` != `True`) to the `R8` task and emit `-keep class <package>.<Type> { *; }` for each, so user Java survives shrinking: * **`D8.targets`** — collect `@(AndroidJavaSource)` items where `%(Bind) != 'True'` into `_R8KeepJavaSource` and pass them to `R8` via the new `JavaSourceFiles` property. * **`R8.cs`** — append keep rules in the same block that emits acw-map keeps. `GetUserJavaTypes()` derives the fully-qualified name as `<package>.<FileNameWithoutExtension>` (Java requires the public top-level type name to match the file name); `ReadJavaPackage()` parses the `package` declaration, skipping comments and stopping at the first `import`/type declaration. Names are de-duplicated. * **`R8Tests.cs`** — unit tests covering `ReadJavaPackage`: package present, trailing space before `;`, comment headers, no package, and `package` after `import`/type (ignored). ## Why split out This is being carved out of the trimmable typemap mega-PR #11617. Although the bug surfaced while bringing up the NativeAOT trimmable path, **it is not trimmable-specific** — the acw-map keep logic and R8 shrinking apply equally to legacy/MonoVM and CoreCLR, so user `AndroidJavaSource` was at risk of being shrunk there too. Landing it independently de-risks #11617 and ships a general correctness fix sooner. ## Context / related PRs - #11617 — parent mega-PR (reflection-free trimmable typemap) this is sliced from. - #11643 — removes the managed NativeAOT typemap in favor of the trimmable one (the path that exposed this gap). - Sibling slices already split out off `main`: #11794 (AndroidApplicationJavaClass/multidex), #11796 (manifest generation parity). Other merged groundwork: #11749, #11751, #11752, #11753, #11764, #11769. ## Verification 24/24 R8 unit tests pass (18 existing + 6 new). `BuildAfterMultiDexIsNotRequired` verified locally on NativeAOT and CoreCLR — user Java is retained after shrinking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…TypeManagers (#11799) ## Summary Bumps **external/Java.Interop** and aligns Mono.Android's reflection-based type and value managers with the new base-class contracts. The Java.Interop bump relaxes the `[DynamicallyAccessedMembers]` (DAM) requirements on the virtual members of `JniRuntime.ReflectionJniTypeManager` / `JniRuntime.ReflectionJniValueManager`, so the Mono.Android overrides no longer need to repeat those annotations and instead rely on targeted trimmer/AOT suppressions. This is a standalone slice of #11617 with no dependency on the trimmable type-map scanner/emitter or array codegen work. ## Changes - `external/Java.Interop` → `8d544738a` (from `70493645c`). - Drop now-redundant `[DynamicallyAccessedMembers]` annotations from the overrides in `AndroidTypeManager`, `ManagedTypeManager`, and `TrimmableTypeMapTypeManager` (`GetInvokerTypeCore`, `GetTypeForSimpleReference`, `RegisterNativeMembers`, `ActivatePeer`), replacing them with `[UnconditionalSuppressMessage]` where the trimmer still needs reassurance. - `JavaMarshalValueManager` now extends `JniRuntime.ReflectionJniValueManager` directly. It is marked `sealed` and carries `[RequiresDynamicCode]` / `[RequiresUnreferencedCode]`, uses the base `EnsureNotDisposed ()` helper, and drops its own dispose tracking and `ActivatePeer` override. - Remove the superseded `AndroidReflectionJniValueManager` and `SimpleValueManager` (and their `Mono.Android.csproj` entries). - `JNIEnvInit.CreateValueManager` creates the value manager through a local helper with the appropriate trimming/AOT suppressions for both the CoreCLR and NativeAOT paths. - No NativeAOT default change; the trimmable type/value managers are **not** part of this PR. ## Tests / baselines - Update the API-compatibility baseline: `Android.Graphics.ColorValueMarshaler.CreateGenericValue`'s `targetType` parameter no longer carries a DAM attribute (inherited from the Java.Interop base-class change). - Refresh `SimpleDotNet` CoreCLR/NativeAOT apkdesc size baselines and the NativeAOT `BuildHasNoWarnings` count, plus `BuildTest2`. ## Context Carved out of #11617. `JavaMarshalRegisteredPeers` extraction already merged via #11750. The trimmable managers, scanner/emitter, manifest, and R8 changes ship in their own PRs (#11749/#11751/#11753/#11769/#11794/#11796/#11798). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 exampleGenericSelectionHost<string>.SetSelection(int), could flow into the typemap model as if it were declared on the open generic typeGenericSelectionHost1`.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:
TypeRefData.GenericArgumentsand preserves generic instantiations decoded from metadata signatures.TypeRefDatafor inherited marshal methods and activation constructors.TypeSpecsignatures/member refs fromPEAssemblyBuilderinstead of flattening everything to plainTypeRefs.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:
TypeSpecmember reference for inherited callbacks.Local validation:
dotnet test tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests.csproj --no-restore