[TrimmableTypeMap] Generator correctness fixes#11988
Open
simonrozsival wants to merge 7 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a set of correctness and determinism fixes to the trimmable type map generator (-p:_AndroidTypeMapImplementation=trimmable), aligning generated output and resolution behavior with the native runtime and improving incremental/reproducible build behavior.
Changes:
- Adjust alias ordering so
Mono.Androidpeers sort first (matching the runtime’s first-writer-wins java→managed selection), and add a targeted regression test. - Extend scanning/model data to support emitting a
<layout>manifest child element from[Layout]attributes. - Add an
XA1010warning path for invalid$(AndroidManifestPlaceholders)and make generated typemap assemblies byte-deterministic via a deterministic PE content id.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapModelBuilderTests.cs | Adds regression test ensuring Mono.Android alias sorting wins for java/lang/Object. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TrimmableTypeMapGeneratorTests.cs | Extends test logger to capture invalid placeholder warnings. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateTrimmableTypeMap.cs | Implements XA1010 coded warning logging for invalid manifest placeholders. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/TrimmableTypeMapGenerator.cs | Wires invalid-placeholder warning callback into manifest generation. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerScanner.cs | Excludes Java.Interop.ManagedPeer from emission; propagates layout properties into component info. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/AssemblyIndex.cs | Parses [Layout] attribute named properties and attaches them to component attribute info. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/ITrimmableTypeMapLogger.cs | Adds logger API for invalid placeholder warning. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/PEAssemblyBuilder.cs | Adds deterministic PE content id provider for byte-stable generated assemblies. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ModelBuilder.cs | Implements Mono.Android-first alias sorting with deterministic tiebreaker. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 1
Member
Author
|
/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.
🤖 Code Review Summary
Five independent correctness fixes for the trimmable type-map generator. I read the full changed files and cross-checked each behavioral claim against the legacy ManifestDocument path before reading the PR description.
Verified correct
- ✅ Placeholder
eqIndex >= 0matches legacyManifestDocument.ReplacePlaceholdersexactly:"=val1".Split('=', 2)→["", "val1"], which replaces${}and does not warn. The old> 0silently dropped empty-key entries (they were neither replaced nor warned). Behavior and XA1010 reuse are consistent with the legacy +ManifestMergerpaths. - ✅ Activity-only
<layout>matches legacy —LayoutAttribute.FromTypeDefinitionis only consumed insideActivityFromTypeDefinition. Covered by the newService_LayoutAttributeElement_Ignoredregression test. - ✅ Alias comparator is a valid, deterministic total order (Mono.Android first → ordinal managed-name → ordinal assembly-name tie-break). Since peers are de-duped by
(ManagedName, AssemblyName), no two distinct peers compare equal, so the unstableList.Sortstill yields stable output. - ✅
ParseLayoutAttributecorrectly reads onlyNamedArguments—LayoutAttributehas a parameterless ctor and 5 named string properties, so there are no fixed args to miss. - ✅ PE content-id is the right fix and complete: the MVID was already deterministic (
DeterministicMvid), leaving the time-based PE stamp as the only remaining non-determinism. TheAppendData(byte[], offset, count)overload (rather than the span overload) is correct here because this project targets netstandard2.0.
Suggestions (posted inline)
⚠️ The byte-determinism fix lacks a regression test; the existing MVID-only test would still pass if the fix were reverted.- 💡 The
Java.Interop.ManagedPeerexclusion has no direct test. - 💡 The now-redundant
else if (eqIndex < 0)can be a plainelse.
CI
Completed checks (license/cla, Java.Interop Tests Windows/Mac) pass. The Linux/Mac/Windows builds and the umbrella dotnet-android check are still in progress/queued — no failures observed, but final green is pending.
Verdict:
Generated by Android PR Reviewer for #11988 · 331.8 AIC · ⌖ 22.7 AIC · ⊞ 6.8K
Comment /review to run again
simonrozsival
added
trimmable-type-map
ready-to-review
Java.Interop.ManagedPeer is a reflection-based helper marked
[RequiresUnreferencedCode] ("Uses reflection to find constructors and
invoke them."). The trimmable type map generator emitted a proxy for it
(_TypeMap.Proxies.Java_Interop_ManagedPeer_Proxy) whose constructor
references ManagedPeer's constructors, producing two IL2026 trim
warnings, aggregated by ILC into:
IL2104: Assembly '_Java.Interop.TypeMap' produced trim warnings
Because the default NativeAOT type map is now trimmable, this surfaced as
a real MSBuild warning and broke NativeAOT tests that assert no warnings
(e.g. BuildWithJavaToolOptions).
ManagedPeer is not supported by the trimmable type map: on the trimmable
path native registration goes through IAndroidCallableWrapper.RegisterNatives
and ManagedPeerNativeRegistration is disabled, so ManagedPeer is never
activated via the type map. Exclude it in the scanner so no proxy is
emitted.
Verified on an arm64 emulator: the HelloWorld NativeAOT (trimmable)
sample now builds with 0 warnings and still launches to MainActivity.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
(cherry picked from commit 8d424c0)
The trimmable manifest generator already had ComponentElementBuilder support for a <layout> child element, but the scanner never populated ComponentInfo.LayoutProperties, so [Layout(...)] on an activity produced no <layout> element (LayoutAttributeElement failed on NativeAOT). Parse the [Layout] attribute's named properties in AssemblyIndex.ParseAttributes (collected separately to tolerate attribute ordering, like [IntentFilter] and [MetaData]) and flow them through TypeAttributeInfo.LayoutProperties into ComponentInfo.LayoutProperties. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 968ac2c)
…ers) ManifestGenerator.ApplyPlaceholders already invoked a WarnInvalidPlaceholder callback for placeholder entries without '=', but the callback was never wired up, so the trimmable path silently dropped the XA1010 warning the legacy ManifestDocument emits (ManifestPlaceHoldersXA1010 failed on NativeAOT). Add ITrimmableTypeMapLogger.LogInvalidManifestPlaceholderWarning (logging XA1010 from the MSBuild logger) and wire the ManifestGenerator instance's WarnInvalidPlaceholder to it. The rooting-only PrepareManifestForRooting pass stays silent so the warning is not emitted twice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 518d9b4)
PEAssemblyBuilder.WritePE let ManagedPEBuilder fall back to a time-based PE content id, so every regeneration of a typemap assembly produced different bytes (different PE TimeDateStamp) even for identical input — the MVID was already deterministic, but the image was not. That churn broke incremental packaging on NativeAOT: an SDK CoreCompile rerun (e.g. touching a .csproj.user file, which the SDK treats as a compile input) rewrites the app assembly, reruns _GenerateTrimmableTypeMap, and — because the regenerated *.TypeMap.dll got a fresh timestamp — forced _BuildApkEmbed to repackage and _Sign to re-sign (CSProjUserFileChanges failed on NativeAOT). Supply a deterministic content-id provider (SHA-256 over the serialized image) so identical input yields byte-identical output; CopyIfStreamChanged then keeps the existing file/timestamp and downstream targets stay incremental. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 91204df)
For a JNI name mapped by multiple managed types (an alias group), the trimmable type map's GetTypeForSimpleReference returns the first (index [0]) alias. Order the aliases to match the native runtime's java->managed selection (clr_typemap_java_to_managed / monovm_typemap_java_to_managed, built by NativeTypeMappingData): the Mono.Android module is processed first and the first managed type to claim a Java name wins. So java/lang/Object must resolve to Java.Lang.Object (Mono.Android), not Java.Interop.JavaObject (Java.Interop). Order alias peers Mono.Android-assembly-first, with an ordinal managed-name tiebreak for deterministic proxy naming. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid null-forgiving PE blob access, keep layout emission activity-only, and trim explanatory comments while preserving legacy placeholder behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep Jon's requested Debug.Assert without the redundant runtime null guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
force-pushed
the
dev/simonrozsival/trimmable-generator-correctness
branch
from
21d1f46 to
20fed8b
Compare
simonrozsival
added
the
ready-to-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Independent correctness fixes for the trimmable type map generator (the opt-in
-p:_AndroidTypeMapImplementation=trimmablepath). These stand alone and do not depend on making the trimmable type map the NativeAOT default (#11822) — extracting them keeps that PR focused and lets these be reviewed on their own.Fixes
Java.Interop.ManagedPeerfrom the type map — it is a base infrastructure peer, not an app/binding type, and must not participate in java→managed resolution.<layout>element for the[Layout]attribute on activities — the scanner now surfaces[Layout]and the manifest generator matches the legacy activity-only emission behavior.$(AndroidManifestPlaceholders)— proper diagnostic instead of a silent/obscure failure, while preserving legacy placeholder replacement behavior.java/lang/ObjecttoJava.Lang.Object, notJava.Interop.JavaObject— order alias groups Mono.Android-assembly-first to match the native runtime's java→managed selection (clr_typemap_java_to_managed/monovm_typemap_java_to_managed, built byNativeTypeMappingData: Mono.Android module first, first-writer-wins), with ordinal managed-name and assembly-name tie-breakers for deterministic proxy naming.Testing
Microsoft.Android.Sdk.TrimmableTypeMap.Tests: 608/608 pass (includes alias-ordering, placeholder, and layout activity-only regression tests).Extracted from #11822.
Issue references
Contributes to #10933 and #10793.