[TrimmableTypeMap] Scanner and JCW edge-case fixes#11096
[TrimmableTypeMap] Scanner and JCW edge-case fixes#11096simonrozsival wants to merge 6 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes correctness gaps in the TrimmableTypeMap pipeline (scanner + generators) so the trimmable build path matches runtime behaviors and produces valid JCW/type-map outputs.
Changes:
- Align CRC64 package naming with the runtime by switching to the Jones CRC64 implementation (
Crc64Helper). - Improve override discovery by continuing base-hierarchy traversal past intermediate
DoNotGenerateAcwMCW base types (including generic-instantiated bases). - Adjust JCW Java generation for Application/Instrumentation to use lazy
__md_registerNatives()registration and sanitize proxy type names by removing generic arity backticks.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/TestFixtures/TestTypes.cs | Adds new fixtures for Application/Instrumentation and intermediate MCW-base override scenarios. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Scanner/OverrideDetectionTests.cs | Adds coverage for override detection across intermediate (including generic) MCW bases. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Scanner/JavaPeerScannerTests.cs | Adds CRC64 expectation test to match JavaNativeTypeManager behavior. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs | Verifies generated proxy type names don’t contain backticks and match sanitized naming. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/JcwJavaSourceGeneratorTests.cs | Updates/extends assertions for Application/Instrumentation lazy native registration and new override fixtures. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerScanner.cs | Removes DoNotGenerateAcw stop for method/property override discovery and switches CRC64 computation to Crc64Helper. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Microsoft.Android.Sdk.TrimmableTypeMap.csproj | Enables unsafe blocks and links in Java.Interop CRC64 helper sources. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ModelBuilder.cs | Sanitizes proxy type names by replacing generic arity backticks. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/Model/TypeMapAssemblyData.cs | Updates documentation to reference TrimmableTypeMap.ActivateInstance. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/JcwJavaSourceGenerator.cs | Implements lazy __md_registerNatives() path and injects registration before native callbacks when needed. |
src/Microsoft.Android.Sdk.TrimmableTypeMap/Microsoft.Android.Sdk.TrimmableTypeMap.csproj
Outdated
Show resolved
Hide resolved
d64ece1 to
6ccb7ca
Compare
64cbe4d to
7443456
Compare
6ccb7ca to
3aa9b44
Compare
7443456 to
0342566
Compare
3aa9b44 to
8fd237a
Compare
0342566 to
d68d7df
Compare
8fd237a to
b82f717
Compare
b82f717 to
0768a52
Compare
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
I didn’t spot a correctness bug in the scanner / JCW fixes themselves, but I did leave one targeted regression-coverage suggestion below. Public checks are green, however the internal Xamarin.Android-PR pipeline is still running, so this PR is not mergeable yet.
- 💡 Testing: cover the
ApplicationRegistration.registerApplications()half of the deferred-registration flow too (tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/JcwJavaSourceGeneratorTests.cs:315).
Nice cleanup overall — the override walk past intermediate MCW bases and the generic proxy-name sanitization both make sense.
Review generated by android-reviewer from review guidelines.
| }; | ||
|
|
||
| var java = GenerateToString (type); | ||
| AssertContainsLine ("__md_registerNatives ();\n\t\tn_OnCreate_Landroid_os_Bundle_ (p0);\n", java); |
There was a problem hiding this comment.
🤖 💡 Testing — This nicely locks in the per-callback __md_registerNatives() fallback. Since CannotRegisterInStaticConstructor still also depends on ApplicationRegistration.registerApplications() for the startup path, consider adding one regression test for result.ApplicationRegistrationTypes or the generated ApplicationRegistration.java as well. That would keep both halves of the deferred-registration contract covered if this generator gets refactored again.
Rule: Add regression coverage for the full bug-fix path.
- CRC64 fix: Use Jones algorithm (Crc64Helper) matching the runtime, not System.IO.Hashing.Crc64 - Inherited override detection: Walk past DoNotGenerateAcw intermediate MCW base types when detecting method overrides - JCW lazy registerNatives: Application/Instrumentation types use deferred __md_registerNatives() helper instead of static initializer - Backtick sanitization: Clean generic arity markers in proxy type names Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
682ebac to
2aeb243
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Fix scanner and JCW generation edge cases in the trimmable type map path.
This PR is now limited to the scanner/JCW fixes and associated regression tests.
Changes
Override detection across intermediate MCW bases
Deferred registerNatives() for Application / Instrumentation types
Proxy name sanitization cleanup
Regression tests