Repository navigation
[Mono.Android] Enable C# memory-safety rules v2 - #13539
simonrozsival wants to merge 5 commits into
Conversation
The normal Java producer reports "[options] location of system modules is not set in conjunction with -source 17" when compiling with matching source and target versions. This prevents a warning-free Mono.Android build even when managed compilation and API analysis are clean. Use --release when the configured source and target versions agree, normalizing 1.8 to 8. Preserve the existing -source/-target behavior for intentional mismatches rather than suppressing the warning or changing Java sources. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: 635cd0f Under memory-safety rules v2, an unsafe declaration describes a caller contract rather than merely permitting unchecked implementation code. Enabling the rules without adjusting existing declarations introduces RequiresUnsafe metadata on released managed wrappers, raw-handle constructors, and public JNI entrypoints. Set $(MemorySafetyRulesVersion) to 2 and retain preview language mode. Move implementation-only unsafe contexts into method bodies, preserve existing JNI ownership and cleanup ordering, and update the canonical JNI emitter and typed-marshaling template together with their output. Keep PublicAPI inventories unchanged and leave independent Java.Interop caller contracts intact. Retain the generated nullability, obsolete-member, and listener corrections needed by normal Mono.Android generation. Include generator.dll in binding-generation inputs so an updated producer invalidates its output, and keep the generator test harness consistent with the production fixup pipeline without adding test cases. Removing declaration-level unsafe from public pointer and function-pointer methods intentionally broadens checked-call acceptance. Their documented ABI, lifetime, thread, and ownership obligations still apply; preserving public member metadata does not preserve the exact compiler-accepted call set. The pre-rebase API-37 control and feature-flag candidate built with zero warnings and errors, full API analysis, and ApiCompat. Semantic comparisons covered public, protected, and nested-visible members in implementation and true reference assemblies; the intentional module-rules marker was checked separately. That evidence uses SDK 26523.103 and does not validate this rebased SDK 26556.115/property-based revision, which still needs a fresh build. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: 11ca91b The rebase adds five UTF-8 allocation and constructor overloads with declaration-level unsafe. Under memory-safety rules v2 these acquire RequiresUnsafe metadata, producing five RS0016/RS0017 pairs against the unchanged API inventories. Remove only those five declaration qualifiers, retaining their local unsafe operations and documented caller preconditions. Update the single existing Streams IOException snapshot to reflect the intended safe-callable raw-constructor compatibility policy; do not add tests or alter API inventories. The current API-37 build passes with zero warnings and errors. All 478 generator tests and 173 focused CoreCLR arm64 on-device tests pass. Implementation and true-reference semantic public APIs match the current-source v1 compiler control; the expected v2 module marker is checked separately. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Targeted regression coverage is missing for override and deprecation inference, inherited nullability normalization, and eventDefaultReturn behavior in the generated code.
3 open issues
What changed in this PR
Enables C# memory-safety rules v2 for Mono.Android while preserving existing public member metadata and documenting caller obligations that remain unenforced.
Changes:
- Moves implementation-only unsafe contexts into method bodies and updates JNI emitters and marshaling templates.
- Adjusts binding generation for inherited nullability, abstract interface slots, listener assignment, and fallback return values.
- Updates generation inputs, Java compiler options, compatibility documentation, and the existing IOException snapshot.
| File | Description |
|---|---|
tests/generator-Tests/Unit-Tests/CodeGeneratorTestBase.cs |
Runs interface nullability normalization in the test pipeline. |
tests/generator-Tests/expected.xaji/Streams/Java.IO.IOException.cs |
Updates raw-constructor declarations and compatibility remarks. |
src/Mono.Android/Mono.Android.targets |
Tracks generator changes as binding-generation inputs. |
src/Mono.Android/Mono.Android.csproj |
Enables memory-safety rules v2. |
src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs |
Adds local unsafe contexts and UTF-8 JNI lookups. |
src/Mono.Android/metadata |
Defines fallback returns for two listener methods. |
src/Mono.Android/Java.Lang/Throwable.cs |
Preserves constructor callability and scopes reference cleanup. |
src/Mono.Android/Java.Lang/Object.cs |
Preserves public contracts and scopes reference cleanup. |
src/Mono.Android/Java.Interop/SafeJavaCollectionFactory.cs |
Adds local unsafe contexts to collection converters. |
src/Mono.Android/Java.Interop/Runtime.cs |
Moves unsafe context into the raw-reference type check. |
src/Mono.Android/Java.Interop/JniMarshalTyped.tt |
Updates typed-marshaling declarations and compatibility documentation. |
src/Mono.Android/Java.Interop/JniMarshalTyped.cs |
Synchronizes generated marshaling methods with the template. |
src/Mono.Android/Java.Interop/JavaConvert.cs |
Adds local unsafe contexts around lookup and cleanup operations. |
src/Mono.Android/Android.Runtime/JNIEnvInit.cs |
Moves pointer validation into the existing unsafe block. |
src/Mono.Android/Android.Runtime/JNIEnv.cs |
Preserves public JNI declarations under v2 rules. |
src/Mono.Android/Android.Runtime/JavaSet.cs |
Preserves conversion callability and scopes reference release. |
src/Mono.Android/Android.Runtime/JavaList.cs |
Preserves conversion callability and scopes reference release. |
src/Mono.Android/Android.Runtime/JavaDictionary.cs |
Preserves conversion callability and scopes reference release. |
src/Mono.Android/Android.Runtime/JavaCollection.cs |
Preserves conversion callability and scopes reference release. |
src/Mono.Android/Android.Runtime/JavaArray.cs |
Preserves conversion callability and scopes reference release. |
src/Mono.Android/Android.Graphics/Bitmap.cs |
Keeps pixel unlocking callable without declaration-level unsafe. |
src/Mono.Android/Android.Bluetooth/BluetoothDevice.cs |
Removes unsafe qualifiers from managed forwarding overloads. |
src/Mono.Android/Android.Bluetooth/AdvertisingSetParametersBuilder.cs |
Removes unsafe qualifiers from managed forwarding methods. |
src/java-runtime/java-runtime.targets |
Uses javac --release for equal source/target versions. |
src/generator/SourceWriters/MethodCallback.cs |
Emits null checks for inherited default-interface contracts. |
src/generator/SourceWriters/JavaLangObjectConstructor.cs |
Generates safe-callable Throwable raw constructors. |
src/generator/SourceWriters/InterfaceListenerProperty.cs |
Assigns the first handler to newly created implementors. |
src/generator/SourceWriters/InterfaceEventHandlerImplClass.cs |
Emits configurable listener fallback returns. |
src/generator/SourceWriters/GenericExplicitInterfaceImplementationMethod.cs |
Adds null checks for non-null string arguments. |
src/generator/SourceWriters/Extensions/SourceWriterExtensions.cs |
Refines listener selection and inherited return checks. |
src/generator/SourceWriters/ClassInvokerClass.cs |
Preserves raw invoker-constructor callability. |
src/generator/Java.Interop.Tools.Generator.ObjectModel/Method.cs |
Stores and clones listener fallback expressions. |
src/generator/Java.Interop.Tools.Generator.ObjectModel/GenBase.cs |
Infers abstract interface slots and normalizes inherited nullability. |
src/generator/Java.Interop.Tools.Generator.Importers/XmlApiImporter.cs |
Imports eventDefaultReturn metadata. |
src/generator/CodeGenerator.cs |
Runs interface nullability normalization during generation. |
Documentation/Java.Interop/Architecture.md |
Explains project-scoped activation and compatibility exceptions. |
Documentation/docs-mobile/binding-libs/customizing-bindings/java-bindings-metadata.md |
Documents listener fallback expressions. |
build-tools/jnienv-gen/Generator.cs |
Omits public unsafe declaration qualifiers in JNI output. |
Review effort: Balanced
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Reviewed all 38 changed files and traced the generator changes through their consumers. New findings: 0 errors, 1 warning, 0 suggestions. The inline warning identifies a behavioral regression: normalizing a default-interface override's parameter to its base annotation can remove an explicitly nullable input allowance, and the newly generated callback null check then rejects previously accepted calls. Existing inline test-coverage findings are not duplicated here.
The project-scoped v2 activation, documented compatibility exceptions, and corresponding JNI-emitter/typed-marshaling-template changes are consistent with the stated compatibility approach. The documentation correctly distinguishes unchanged member metadata from broadened compiler-accepted calls.
CI: GitHub reports Azure build 1679195 still in progress, with no failed checks reported at inspection time; Linux/macOS build checks and Android Tools checks have passed. CI is not yet confirmed green. The standalone Azure build-status query was blocked by an authentication requirement, so the underlying Azure result/timeline could not be independently confirmed.
Local validation: Attempted the focused DefaultInterfaceMethods generator tests using dotnet test tests/generator-Tests/generator-Tests.csproj --no-restore -v minimal --filter 'FullyQualifiedName~DefaultInterfaceMethods' with intermediate/output paths redirected under /tmp/gh-aw/agent/. The .NET CLI failed before project evaluation with a first-run NuGet-Migrations mutex IOException; no tests executed. The finding is based on the source/data-flow trace, not an executed reproduction. No repository files were modified.
Generated by Android PR Reviewer for #13539 · copilot · gpt61 · 121.9 AIC · ⌖ 5.06 AIC · ⊞ 20.2K
Comment /review to run again
Context: #13539 Copying the root interface's non-null parameter annotation onto an overriding default method erased an explicit nullable allowance. The callback writer then rejected null with NullPointerException instead of forwarding it. Combine parameter contracts contravariantly and return contracts covariantly across every override level, retaining intermediate annotations instead of consulting only the root. Callback dispatch still binds to the original interface signature. Reuse the existing generated null-forgiveness mechanism only when that signature is stricter than the DIM's nullable input, without rejecting null at runtime or changing the public interface contract. The requested skipInterfaceMethods regression also exposed two older interface-aware override lookups that bypassed the skip-aware abstract-slot inference. Restrict those fallback lookups to actual base-class members; interface-only abstract slots continue through the existing filtered inference. Add five focused regression cases for the nullable DIM chain, interface-only override/obsolete inference with and without skipped slots, and imported eventDefaultReturn fallback/handler dispatch. Compile the generated DIM and event code with the existing Roslyn compiler and an opt-in strict warning check, without changing existing compilation defaults. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "169.254.169.254"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ LGTM — no blocking code findings
Findings: 0 errors, 0 warnings, 1 non-blocking inline suggestion. Reviewed head d23cb4f37eaa5f712e6d2a3e886f62ff7b8c9309, the changed files and surrounding source, generator consumers, and the previously resolved review threads. The project-scoped v2 opt-in, local implementation unsafe contexts, canonical JNI emitter and typed-marshaling template/output changes are consistent with the documented compatibility exceptions. The new regressions specifically cover multi-level DIM nullability, skipped interface-only abstract slots, and listener fallback/handler dispatch. PublicAPI inventories are unchanged in the diff.
CI: All 40 head-commit GitHub checks report success, including the aggregate dotnet-android check for Azure build 1679319. The direct Azure build-details query required unavailable authentication, so I did not independently verify its build result or timeline beyond the GitHub checks.
Local validation limit: Attempted dotnet test tests/generator-Tests/generator-Tests.csproj --no-restore --filter 'Name~DefaultInterfaceNullabilityPreservesNullableOverridesAcrossLevels|Name~InterfaceOnlyAbstractBaseInfersOverrideAndObsoleteUnlessSkipped|Name~EventDefaultReturnPreservesFallbackAndHandlerDispatch' -v minimal. The .NET CLI failed during first-run setup with DirectoryNotFoundException, before compilation or test execution. No local test pass, independent metadata-equivalence check, or device validation is claimed by this review.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
169.254.169.254
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "169.254.169.254"See Network Configuration for more information.
Generated by Android PR Reviewer for #13539 · copilot · gpt61 · 169.3 AIC · ⌖ 5.03 AIC · ⊞ 20.2K
Comment /review to run again
| SetBody.Add ($"\timpl = new {opt.GetOutputName (iface.FullName)}Implementor ({(iface.NeedsSender ? "this" : string.Empty)});"); | ||
| SetBody.Add ($"\tImpl{name} = impl;"); | ||
| SetBody.Add ($"}} else"); | ||
| SetBody.Add ("}"); |
There was a problem hiding this comment.
🤖 💡 Testing — Consider adding a regression that exercises the generated delegate property on a fresh owner: assign the first handler through the property, verify it is returned/invoked, then replace and clear it. EventDefaultReturnPreservesFallbackAndHandlerDispatch assigns listener.Handler directly on the implementor, so it would still pass if the old else were restored here and the first property assignment were silently dropped again. A focused source-writer test or a pure-managed consumer with a stub implementor would protect this separate fix.
{Rule: Bug fixes need regression tests}
Context: #13539 (comment) Calling eventDefaultReturn a return expression did not explain that the binding generator inserts its value verbatim without expression-only validation. Calls can have side effects and crafted metadata can inject additional source, so this is not a sandbox for untrusted configuration. Document the source-code trust boundary, the no-handler execution timing, and ordinary compiler/runtime failure behavior. Recommend small fallbacks and hand-written listeners for complex logic. Keep the existing listener behavior and generator contract unchanged; the window-insets fallback needs a method call, and expression-only parsing would not make that call side-effect-free. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
Review assessment: no new blocking code defects found; validation remains limited. New findings: 0 ❌ errors, 0
The project-scoped v2 opt-in, local implementation unsafe contexts, and corresponding JNI emitter/T4 changes are consistent with the stated compatibility policy. The documentation clearly distinguishes preserved member metadata from broadened compiler-accepted calls. The added regressions cover the previously reported nullable DIM inheritance, skipped interface slots, and fallback/handler dispatch cases.
CI: All 40 GitHub check runs report success, including the aggregate dotnet-android check for Azure build 1679889. The direct Azure build query required authentication, so its build result and timeline could not be independently confirmed; this is not an unconditional CI-green verdict.
Local validation: Attempted the focused generator tests with dotnet test tests/generator-Tests/generator-Tests.csproj --filter 'Name~DefaultInterfaceNullabilityPreservesNullableOverridesAcrossLevels|Name~InterfaceOnlyAbstractBaseInfersOverrideAndObsoleteUnlessSkipped|Name~EventDefaultReturnPreservesFallbackAndHandlerDispatch' -v minimal --results-directory /tmp/gh-aw/agent/review-results. The command failed during .NET CLI initialization with a NuGet-Migrations IOException, before tests ran. No local build/device-test or API-metadata validation is claimed by this review. No repository files were changed.
Generated by Android PR Reviewer for #13539 · copilot · gpt61 · 175 AIC · ⌖ 4.78 AIC · ⊞ 21K
Comment /review to run again
| } | ||
|
|
||
| [TestCase (null, 0)] | ||
| [TestCase ("value + 1", 6)] |
There was a problem hiding this comment.
🤖 💡 Testing — Consider adding a reference-returning listener case with nullable-reference-types enabled and return-not-null='true'. Both product uses of eventDefaultReturn return reference types, whereas this regression exercises only int. A generated consumer that returns the original argument as its fallback (and checks reference identity), plus a nullable expression guarded by ?? throw, would protect the return-nullability behavior that motivated the metadata entries as well as handler dispatch.
{Rule: Test edge cases and compile generated consumers}
Enable
MemorySafetyRulesVersion=2for Mono.Android with preview language mode. The base is380fbcbbcc6bfa8515fa513c443bb3ac83bd06cb, using SDK12.0.100-alpha.1.26556.115and compiler5.14.0-1.26556.115.Under v2, declaration-level
unsafeintroduces caller-visibleRequiresUnsafemetadata. Move implementation-only contexts into method bodies so released managed wrappers, raw-handle constructors, and public JNI entrypoints retain their member contracts. Update the canonical JNI emitter, typed-marshaling template/checked-in output, generated nullability/obsolete/listener handling, and binding-generation inputs. The rebase introduced five UTF-8 constructor/allocation overloads requiring the same qualifier correction; their original API analysis failed with five RS0016/RS0017 pairs.No PublicAPI inventories, dependency pins, or other project activations are changed. Five review-requested generator regression cases cover nullable DIM inheritance, interface-only override/obsolete inference (including skipped slots), and imported event fallback/handler dispatch. One existing Streams IOException snapshot is updated, with explicit user approval, to reflect the intended safe-callable raw-constructor policy. The separate Java prerequisite commit uses
javac --releasefor equal source/target versions, including1.8normalization, and preserves intentional mismatches.Compatibility: pointer/function-pointer methods intentionally become callable from more checked contexts. Their documented ABI, ownership, lifetime, and threading preconditions still apply. Unchanged public member metadata does not mean identical compiler-accepted calls. The v2 module marker is intentional; independent Java.Interop raw contracts are unchanged.
Validation
Current review fix:
d23cb4f37eaa5f712e6d2a3e886f62ff7b8c9309. Preserve nullable DIM inputs across all override levels, and prevent skipped interface slots from being re-marked as overrides. Callback dispatch uses the existing generated null-forgiveness mechanism only where a nullable DIM argument meets the original interface's stricter annotation, with explicit user approval; null is still forwarded and public contracts are not rewritten. Without that narrow adaptation the normal product build reported seven CS8604 warnings; that evidence is retained.$Vdenotes the retained session-localmono-pr13539-validationdirectory. Itsrun.pyinvokes normal targets using the owned pinned SDK/private state, one CPU, no node reuse/shared compilation, and unchanged analyzers/API inventories. Complete expanded commands and environment/input guards are recorded in its receipts.python3 "$V/run.py" review-generator-suite-fixed test tests/generator-Tests/generator-Tests.csproj --logger 'trx;LogFileName=review-generator-suite-fixed.trx' --results-directory "$V/results"python3 "$V/run.py" review-mono-api37-fixed build src/Mono.Android/Mono.Android.csproj/memorysafetyrulesversion:2.Both newly built implementation and true-reference public/protected/nested-visible semantic APIs match the exact retained pre-fix products: 10,331 types, 78,439 methods, 45,841 fields, 26,294 properties, and 249 events; no signature/flag/constant/parameter/generic/full custom-attribute differences or public member
RequiresUnsafeattributes. Both actual module markers remain[2]. All 12 API inventories are unchanged. The tested tree wase0bf1874plus the exact five-file patch committed asd23cb4f37e; products were not rebuilt merely to change commit/SourceLink provenance after committing. No device or extra-profile rerun was performed for this generator-only review fix. Owned helpers are released.Earlier SDK/runtime validation (e0bf187 source; historical after the review fix)
These earlier receipts are preserved, not substituted for the current-source checks above. The source was built as
93467a1plus the exact UTF-8/snapshot patch subsequently committed ase0bf1874.python3 "$V/run.py" prepare-qualified-git build build-tools/scripts/Prepare.proj -t:Preparepython3 "$V/run.py" mono-api37-fixed build src/Mono.Android/Mono.Android.csproj/memorysafetyrulesversion:2.python3 "$V/run.py" sdk-solution-isolated-native-assets build Microsoft.Android.slnxpython3 "$V/run.py" extra-api-levels build Microsoft.Android.slnx -t:BuildExtraApiLevelspython3 "$V/run.py" configure-workload-final build build-tools/create-packs/Microsoft.Android.Sdk.proj -t:ConfigureLocalWorkloadpython3 "$V/run.py" generator-tests-fixed test tests/generator-Tests/generator-Tests.csproj --logger 'trx;LogFileName=generator-tests-fixed.trx' --results-directory "$V/results"git diff --check 380fbcbbcc6bfa8515fa513c443bb3ac83bd06cb...HEADgit diff --exit-code 380fbcbbcc6bfa8515fa513c443bb3ac83bd06cb...HEAD -- src/Mono.Android/PublicAPIRuntime: the normal SDK
Build/Installtargets and project-localdotnet test --no-build --report-trxpassed 173 existing tests, 0 failed/skipped, on the isolated API35 arm64 VM (1 CPU/2.5 GiB RAM). Configuration: Release,-r android-arm64,PublishAot=false; CoreCLR runtime pack12.0.0-alpha.1.26556.115. The exact recorded commands are inreceipts/device-apk-single-rid.json,device-install.json, anddevice-test.json. Included categories areTransferredReferences,InvokerActivation,InterfaceCollections,TrimmableTypeMapRuntimeCoverage,JniRemapping,ConstructorParity,ManagedConstruction,JniUtf8Construction,JavaList,JNIObjectArray, not the removed activation-test additions. The TRX contains 173 definitions and 173 passing results.Actual installed base/arm64 splits were pulled before/after MTP and matched the normal
.apksentries byte-for-byte. Installedlibcoreclr.somatches the selected net12 runtime pack. Mono.Android was decoded from the actual installed assembly store, matched that validation's normal R2R PE byte-for-byte, and contains module rules[2]. VM, private ADB, and owned build/helper processes were released.API metadata: a warning-free e0bf187-source v1 compiler-only control, using the successful normal candidate's exact sources/references/analyzers/inventories with only the rules version and output destinations changed, matched both implementation and true-reference public/protected/nested-visible surfaces: 10,331 types, 78,439 methods, 45,841 fields, 26,294 properties, and 249 events. No signature/flag/constant/parameter/generic/full custom-attribute differences or public member
RequiresUnsafeattributes were found. Module rules changed separately from[]to[2]. This is not claimed as a separate ordinary v1 MSBuild build. The initial CS2023 warning from putting/noconfiginside the response file remains preserved separately; the corrected invocation put it outside the response file.Visible warnings and limits
The earlier full SDK's 15 warnings are native narrowing diagnostics and CS2002 in files outside this change. API37.1/37.2 report four duplicate-symbol RS0025 warnings in untouched Shipped inventories. The earlier runtime test APK/Install retains 38 visible fixture trim/AppDomain warnings; these are not a warning-free full-SDK/device-build claim. No warning IDs were disabled, analyzers skipped, or inventories rewritten. Existing negative generator fixtures still print their expected BG diagnostics.
The first cold plural-
RuntimeIdentifiersdevice build exposed a separate reference-output-path failure (XAGTT7023). The successful earlier device validation uses the supported single-RID configuration; this PR does not claim to fix the plural-RID cold graph. No NativeAOT/other-ABI runtime matrix or T4-engine execution is claimed. Initial failures and helper-authoring corrections are retained separately; historical pre-rebase proofs are not substituted for these results.