[Java.Interop] Restore pure-reflection registration for JavaProxyObject

[simonrozsival]

Problem

dotnet/android's trimmable typemap scanner rejects any type carrying [JniAddNativeMethodRegistrationAttribute] with XA4251, and it currently fails on the built-in Java.Interop.JavaProxyObject — blocking the Java.Interop bump in dotnet/android#11622.

Root cause

#1441 added JniRuntime.JniTypeManager.TryRegisterBuiltInNativeMembers () — a reflection-free direct call to JavaProxyObject.RegisterNativeMembers — and changed that method from private to internal so the call would compile.

But that method is dead code: nothing in Java.Interop or dotnet/android ever calls TryRegisterBuiltInNativeMembers, and the base JniTypeManager.RegisterNativeMembers is a no-op. JavaProxyObject's natives are — and were before #1441 — registered purely via reflection in ReflectionJniTypeManager (FindAndCallRegisterMethod discovering the attributed method), which finds private methods fine via GetRuntimeMethods ().

The internal bump is what made the attribute leak into the reference assembly (private members are stripped from ref assemblies; internal ones are not), where the scanner sees it.

Fix

• Remove the unused TryRegisterBuiltInNativeMembers (+ its PublicAPI.Unshipped.txt entry).
• Restore JavaProxyObject.RegisterNativeMembers to private — its state before Split reflection-based JniTypeManager and JniValueManager behavior #1441.

Net change: 1 insertion, 18 deletions. No behavior change — registration still happens via reflection exactly as it did before #1441.

Verification

• ✅ Java.Interop.csproj builds clean (0/0); PublicApiAnalyzer consistent.
• ✅ Metadata inspection: the reference assembly now contains no Register* method on JavaProxyObject (the attributed method is stripped), so the trimmable scanner no longer trips XA4251. The implementation assembly keeps the private attributed method for reflection.

Unblocks dotnet/android#11622. cc @jonathanpeppers — review/approve once CI is green please? 🙏

[Copilot]

Pull request overview

Restores JavaProxyObject.RegisterNativeMembers to a private method to prevent [JniAddNativeMethodRegistrationAttribute] from appearing in Java.Interop’s reference assembly, while preserving the reflection-free built-in registration path via a new attribute-free internal entry point.

Changes:

• Update built-in registration flow to call JavaProxyObject.RegisterBuiltInNativeMembers (JniType) instead of the attribute-annotated method.
• Make JavaProxyObject.RegisterNativeMembers (JniNativeMethodRegistrationArguments) private again and add an internal wrapper that performs the same registrations.

Show a summary per file

File	Description
src/Java.Interop/Java.Interop/JniRuntime.JniTypeManager.cs	Switch built-in native member registration for JavaProxyObject to the new internal attribute-free entry point.
src/Java.Interop/Java.Interop/JavaProxyObject.cs	Make the attribute-annotated registration method private and introduce RegisterBuiltInNativeMembers (JniType) to keep built-in registration working without exposing the attribute in ref assemblies.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 0

[jonathanpeppers]

/review

[github-actions]

✅ Java.Interop PR Reviewer completed successfully!

[simonrozsival]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[github-actions]

⚠️ Needs Changes — 1 blocking issue

The XA4251 root-cause analysis is sound and the core fix is right: reverting JavaProxyObject.RegisterNativeMembers to private keeps it out of the reference assembly while reflection still discovers it (FindAndCallRegisterMethod enumerates GetRuntimeMethods (), which includes private methods). 👍

The blocking problem is the companion deletion of JniRuntime.JniTypeManager.TryRegisterBuiltInNativeMembers. The PR describes it as dead code with "no behavior change," but neither holds:

• ❌ It has two live callers in this repo — samples/Hello-NativeAOTFromJNI/NativeAotTypeManager.cs:124 and samples/Hello-NativeAOTFromAndroid/NativeAotTypeManager.cs:29. Both JniTypeManager subclasses call it, so they no longer compile (CS0103). Hello-NativeAOTFromJNI is in Java.Interop.sln and is published + run in CI — only continueOnError: true keeps that red build from failing the pipeline (so the GitHub checks, currently just license/cla, won't catch it).
• ❌ It's a functional regression for NativeAOT. NativeAOT type managers don't use the reflection path; this helper is the sole mechanism that registers JavaProxyObject's equals/hashCode/toString natives for them. Removing it drops those registrations.

Suggested path forward: keep the private revert, but instead of deleting the helper, give JavaProxyObject an internal, non-attributed registration helper that both the private attributed RegisterNativeMembers and TryRegisterBuiltInNativeMembers call. Then restore the method and its PublicAPI.Unshipped.txt entry. See the inline comment for specifics.

Please validate locally with dotnet build samples/Hello-NativeAOTFromJNI/Hello-NativeAOTFromJNI.csproj before merging, since CI won't flag this.

Generated by Java.Interop PR Reviewer for issue #1468 · 588.3 AIC · ⌖ 49.4 AIC · ⊞ 34.6K
_{Comment /review to run again}

[simonrozsival]

Thanks for the careful review — you're right on both counts, and I've pushed a fix (b2a2a15).

TryRegisterBuiltInNativeMembers is not dead code: it has two live callers, samples/Hello-NativeAOTFromJNI/NativeAotTypeManager.cs:124 and samples/Hello-NativeAOTFromAndroid/NativeAotTypeManager.cs:29, and it's the only path that registers JavaProxyObject's equals/hashCode/toString natives for the NativeAOT type managers (which don't go through the reflection-based FindAndCallRegisterMethod). Deleting it was both a CS0103 break and a NativeAOT functional regression; CI only hid it because those sample steps run with continueOnError: true. My "dead code" claim was wrong — I'd only grepped src/, not samples/.

Fix keeps the XA4251 change and restores the method, per your suggestion:

• RegisterNativeMembers (JniNativeMethodRegistrationArguments) stays private + [JniAddNativeMethodRegistration], so the attribute is stripped from the reference assembly.
• Extracted the registration list into a new attribute-free internal static JavaProxyObject.AddBuiltInRegistrations (ICollection<JniNativeMethodRegistration>).
• Both the private attributed RegisterNativeMembers (reflection path) and the restored TryRegisterBuiltInNativeMembers (NativeAOT path) call it.
• Restored the PublicAPI.Unshipped.txt entry.

Verified locally:

• Java.Interop.dll builds clean (0 warnings / 0 errors).
• Metadata scan of the produced reference assembly: no method carries [JniAddNativeMethodRegistration] (so XA4251 stays fixed); RegisterNativeMembers is stripped, AddBuiltInRegistrations is present but attribute-free.
• The sample's TryRegisterBuiltInNativeMembers (nativeClass, nativeClass.Name, methods) call resolves against the rebuilt assembly (no CS0103).

Addressed

[simonrozsival]

Reworked per review feedback — instead of carrying a registration helper in production, the NativeAOT samples now derive from JniRuntime.ReflectionJniTypeManager again (the behavior the base JniRuntime.JniTypeManager provided before #1441). Built-in types like JavaProxyObject/JavaProxyThrowable are then registered automatically via reflection, so no hand-written registration is needed.

Production is now smaller, not larger:

• JavaProxyObject.RegisterNativeMembers: internal → private (the XA4251 fix — strips the attribute from the ref assembly). The marshalers stay; the default Android reflection runtime discovers them via FindAndCallRegisterMethod → GetRuntimeMethods(), which returns private methods.
• Deleted JniRuntime.JniTypeManager.TryRegisterBuiltInNativeMembers (+ its PublicAPI.Unshipped.txt entry) — no longer needed by anything.

Samples: derive from ReflectionJniTypeManager, drop the reflection-free overrides + the hand-written JavaProxyObject registration. Since that base is [RequiresDynamicCode]/[RequiresUnreferencedCode], the constructor carries [UnconditionalSuppressMessage] for IL2026/IL3050.

A #pragma is not sufficient here — it silences the Roslyn analyzer but not the ILLink/ILC publish passes. I verified this against the real type manager + Java.Interop.dll:

• with [UnconditionalSuppressMessage] on the ctor → trim-publish clean;
• negative control (attrs removed) → ILLink emits IL2026: NativeAotTypeManager.NativeAotTypeManager() … ReflectionJniTypeManager() at the ctor.

Net: 47 insertions / 261 deletions.

Note: the two NativeAOT sample publish/run steps are continueOnError: true, so CI won't fail on a sample regression. I validated the Hello-NativeAOTFromJNI type manager locally via a trim-publish; Hello-NativeAOTFromAndroid is structurally identical.

[simonrozsival]

/review

[github-actions]

✅ Java.Interop PR Reviewer completed successfully!

[github-actions]

🤖 Review: ⚠️ Needs Changes

Thanks for iterating on this. The core change is right: making JavaProxyObject.RegisterNativeMembers private strips [JniAddNativeMethodRegistrationAttribute] from the reference assembly (fixing dotnet/android's XA4251 typemap-scanner rejection), while the runtime still discovers that private static method via reflection (ReflectionJniTypeManager.FindAndCallRegisterMethod → GetRuntimeMethods (), which finds non-public methods). Removing TryRegisterBuiltInNativeMembers is safe now that nothing calls it (it was unshipped API), and the previous review round's concerns (CS0103 from deleting a still-referenced helper / lost built-in registration) are genuinely resolved by switching the samples to ReflectionJniTypeManager. 👍

A few things to address before merge:

• ⚠️ Trimmer/AOT validation gap — the NativeAOT samples now derive from the AOT-incompatible ReflectionJniTypeManager and blanket-suppress IL2026/IL3050. The suppressions mean a warning-free trim/AOT publish no longer demonstrates the reflected-over registration survives trimming, and the sample build/run steps run under continueOnError, so CI won't catch a runtime regression. App.cs also never creates a JavaProxyObject, so the sample doesn't actually exercise the scenario being fixed. Please confirm by running the published native binary. (inline)
• 💡 Duplicate simple reference in Hello-NativeAOTFromJNI — GetSimpleReferences now yields example/ManagedType twice because ManagedType carries [JniTypeSignature] and sits in typeMappings. (inline)
• 💡 Dead typeMappings == null guard. (inline)

Process notes:

• The PR description is stale — it still describes the abandoned first approach ("Net change: 1 insertion, 18 deletions", "No behavior change", TryRegisterBuiltInNativeMembers is "dead code"). The actual change is +31 / −261 and rewrites both NativeAOT sample type managers to reflection-based registration. Please update it so reviewers and the merge history stay accurate.
• CI: only the license/cla check has reported; the Azure DevOps dotnet.java-interop build is still pending. Not mergeable until that build is green.

Verdict: ⚠️ Needs Changes — validate the AOT/runtime behavior, tidy the duplicate/dead code, and refresh the PR description; final sign-off also waits on the full CI build. The underlying fix itself looks correct.

Generated by Java.Interop PR Reviewer for issue #1468 · 1.3K AIC · ⌖ 48.9 AIC · ⊞ 34.6K
_{Comment /review to run again}

[simonrozsival]

Thanks — went through the review comments. Status against the current head (1fcc5ec2):

1. TryRegisterBuiltInNativeMembers "dead code" (JniTypeManager.cs:256) — ✅ Addressed. You were right that it had live callers. Rather than keep it, the samples now derive from JniRuntime.ReflectionJniTypeManager again (the pre-#1441 base behavior), so the helper and its two sample call sites are gone together — built-in registration is handled by the reflection base.

2. Unreachable if (typeMappings == null) guard — ✅ Fixed in 1fcc5ec2 (removed in the FromAndroid sample; FromJNI no longer has that method at all).

3. Duplicate simple reference from base + typeMappings — ✅ Resolved. FromJNI no longer overrides GetSimpleReferences (the base's [JniTypeSignature]-based reverse mapping already covers Example.ManagedType). FromAndroid still maps its own types, but Android binding types use [Register], not [JniTypeSignature], so base.GetSimpleReferences yields nothing for them — no duplicate.

4. Trim/AOT suppression hiding risk + sample not exercising the scenario — partly addressed, partly by-design:

• The IL2026/IL3050 suppression is only on the base() constructor call (the [RequiresDynamicCode]/[RequiresUnreferencedCode] is on ReflectionJniTypeManager's ctor). The actual registration does not depend on that suppression for correctness: RegisterNativeMembers(... Type type ...) is annotated [DynamicallyAccessedMembers(MethodsAndPrivateNested)] and FindAndCallRegisterMethod(Type marshalType, ...) is [DynamicallyAccessedMembers(Methods)] (Public+NonPublic methods). So when typeof(JavaProxyObject) flows through that path, the trimmer preserves its now-private RegisterNativeMembers, and reflection finds it. The private→ref-assembly-stripping (the XA4251 fix) and runtime trimming-preservation are independent.
• Re. "run the published binary": CI's RunJavaSample step does exactly that. The earlier run surfaced a real bug (type resolution), now fixed in a8f3f2b4 by overriding GetTypeForSimpleReference (singular) — post-Split reflection-based JniTypeManager and JniValueManager behavior #1441 GetType() dispatches through it, not the plural GetTypesForSimpleReference. The sample exercises the same reflection-registration mechanism (via example/ManagedType) that JavaProxyObject would use.
• Re. "App.cs only marshals a string": correct — FromJNI doesn't construct a JavaProxyObject. That's a pre-existing characteristic of the sample, not something this PR changes. I've kept scope minimal (this PR exists to unblock the dotnet/android Java.Interop bump); happy to add explicit proxy coverage as a follow-up if you'd prefer.

New build is running on 1fcc5ec2.

Addressed

[simonrozsival]

/review

[github-actions]

✅ Java.Interop PR Reviewer completed successfully!

[github-actions]

⚠️ Needs Changes (non-blocking) — core fix is correct

The XA4251 fix itself is sound and minimal, and I verified the key claims independently:

• ✅ JavaProxyObject.RegisterNativeMembers internal → private is safe. The default runtime discovers it through ReflectionJniTypeManager.FindAndCallRegisterMethod, which enumerates marshalType.GetRuntimeMethods() (returns private methods) and calls CreateDelegate (works on private statics). The [DynamicallyAccessedMembers] annotations on RegisterNativeMembers(... Type type ...) and FindAndCallRegisterMethod(Type marshalType, ...) preserve the method under trimming, so trimming-preservation and ref-assembly stripping are correctly independent.
• ✅ Removing TryRegisterBuiltInNativeMembers leaves no dangling callers in src/ (the two sample call sites were migrated), and the PublicAPI.Unshipped.txt entry was removed to match.
• ✅ Prior-round feedback (the retracted "dead code" claim, the typeMappings == null guard, the duplicate simple references, and the GetType()→singular GetTypeForSimpleReference resolution bug) all look addressed at the current head. No duplicate-yield in GetTypesForSimpleReference — the base plural path reads a separate built-in dictionary, not the overridden singular.

Please address before merge

1. ⚠️ Stale/inaccurate PR description. It still asserts TryRegisterBuiltInNativeMembers is "dead code" that "nothing... ever calls" (retracted in the thread — the samples called it) and states "Net change: 1 insertion, 18 deletions. No behavior change," whereas the actual diff is 44/-264 across 5 files and the samples' registration path changes from hand-rolled reflection-free to reflection-based. Since this becomes the squash-merge message, please update it to describe the final approach (samples derive from ReflectionJniTypeManager).
2. ⚠️ Untested behavioral change in the samples — see the inline comment on Hello-NativeAOTFromJNI. FromAndroid isn't built/run in CI, and FromJNI's publish + run steps are continueOnError: true, so neither sample's AOT correctness is gated. Please confirm FromAndroid was actually published/run.
3. 💡 Suppression justification accuracy — see the inline comment; the [UnconditionalSuppressMessage] text contradicts the attributes it suppresses.

CI

Only license/cla is visible and green; no build/test pipeline is reported on the PR (the /azp run bot replied "No pipelines are associated with this pull request"), and mergeable_state is blocked. I couldn't confirm a full green build from here — worth verifying the AzDO build is green before merge.

Nice, surgical fix overall — the comments above are about documentation accuracy and test coverage, not the correctness of the production change.

Generated by Java.Interop PR Reviewer for issue #1468 · 1.3K AIC · ⌖ 48.2 AIC · ⊞ 34.6K
_{Comment /review to run again}

[github-actions]

🤖 ⚠️ Trimmer/AOT — This PublishAot=true sample now derives from ReflectionJniTypeManager, which is explicitly [RequiresDynamicCode]/[RequiresUnreferencedCode] ("not compatible with Native AOT"). It works here only because the assembly is rooted via TrimmerRootAssembly and the [DynamicallyAccessedMembers] annotations on the RegisterNativeMembers(Type) → FindAndCallRegisterMethod path preserve the reflected members — but it inverts what these samples exist to demonstrate (a reflection-free AOT type manager) by depending on a base the runtime declares AOT-incompatible.

The safety net is also thin: Hello-NativeAOTFromAndroid isn't built or run in CI at all, and Hello-NativeAOTFromJNI's publish + RunJavaSample steps are continueOnError: true (build-tools/automation/templates/core-tests.yaml:83,90), so an AOT regression in either sample won't fail the build. Please confirm the FromAndroid sample was actually published/run (not just "structurally identical"), and consider keeping at least one sample on the reflection-free path so the AOT pattern stays demonstrated.

Rule: Consider trimmer/NativeAOT impact

[simonrozsival]

Acknowledged — and this is a deliberate trade-off, now documented in a class-level comment (4de18b34).

You're right that deriving from ReflectionJniTypeManager inverts the "reflection-free AOT type manager" pattern the samples originally demonstrated. The reason we're accepting that here: these are samples, not product. .NET for Android — what we actually ship — does not pair ReflectionJniTypeManager with NativeAOT; our NativeAOT path uses the trimmable typemap (reflection-free registration), which is exactly what making JavaProxyObject.RegisterNativeMembers private unblocks more of. Re-implementing a hand-written reflection-free registration in the java-interop samples (what #1441 originally did) is real work with no product benefit, so for now the suppression is the pragmatic choice. This also isn't a new AOT hole — pre-#1441 the equivalent suppressions lived inside JniTypeManager itself (justified NotUsedInAndroid).

On the CI safety net — you're correct, and I won't overclaim:

• Hello-NativeAOTFromAndroid is not built or run in CI (not in Java.Interop.sln; needs an Android device). I did not separately publish/run it — it's the same code shape as FromJNI, but I'm not asserting execution coverage for it.
• Hello-NativeAOTFromJNI is exercised: CI's RunJavaSample publishes + runs it on the JVM, and that step is what caught the real type-resolution bug I then fixed in a8f3f2b4. The latest green build (run Hello-NativeAOTFromJNI succeeded) confirms the published native binary actually runs.

If keeping one sample on the reflection-free path is something you'd like, I'm happy to do it as a follow-up — but I'd rather not expand this PR (its job is to unblock the dotnet/android bump).