-
Notifications
You must be signed in to change notification settings - Fork 64
Split reflection-based JniTypeManager and JniValueManager behavior #1441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
simonrozsival
wants to merge
23
commits into
main
Choose a base branch
from
dev/simonrozsival/fix-aot-warnings-jni-type-manager
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
58f0dbd
WIP
simonrozsival fd4c112
Remove unnecessary suppression
simonrozsival f2246b3
Move DynamicJniTypeManager to its own separate file
simonrozsival 6d52b54
Update NativeAOT sample
simonrozsival fa49c9a
Simplify
simonrozsival 17e87cd
Fix dynamic JniTypeManager test regressions
simonrozsival 7f9798e
Use annotations for dynamic type manager tests
simonrozsival 6429366
Keep NativeAOT samples off dynamic type manager
simonrozsival c18bdc7
Revert extra JniTypeManager test assertions
simonrozsival a19e758
Move dynamic test annotations to classes
simonrozsival cd635d8
Remove extra generic invoker test
simonrozsival 6af232f
Use direct type references in NativeAOT samples
simonrozsival 906e8dd
Separate default JRE type manager from AOT path
simonrozsival ef5d9c8
Handle built-in proxy types in AOT type managers
simonrozsival a07fda2
Map primitive signatures for AOT constructor lookup
simonrozsival b1f75c8
Rename dynamic JNI type manager
simonrozsival 5877194
Split JniValueManager and ReflectionJniValueManager
simonrozsival 65e6c60
Undo unnecessary changes
simonrozsival 32f4f4f
Remove unnecessary changes
simonrozsival 8f644e5
Suppress warnigns in tests
simonrozsival 00d3a9d
Merge remote-tracking branch 'origin/main' into dev/simonrozsival/fix…
Copilot 48fbe33
Fix code review issues in ReflectionJniValueManager and ReflectionJni…
Copilot 5dbaa73
Address review comments: EnsureNotDisposed, spacing, Array.Empty, nul…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
125 changes: 108 additions & 17 deletions
125
samples/Hello-NativeAOTFromJNI/NativeAotTypeManager.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,37 +1,128 @@ | ||
| using Java.Interop; | ||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| namespace Hello_NativeAOTFromJNI; | ||
|
|
||
| class NativeAotTypeManager : JniRuntime.JniTypeManager { | ||
|
|
||
| #pragma warning disable IL2026 | ||
| Dictionary<string, Type> typeMappings = new () { | ||
| [Example.ManagedType.JniTypeName] = typeof (Example.ManagedType), | ||
| }; | ||
| #pragma warning restore IL2026 | ||
|
|
||
| internal const DynamicallyAccessedMemberTypes Methods = DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods; | ||
| internal const DynamicallyAccessedMemberTypes MethodsAndPrivateNested = Methods | DynamicallyAccessedMemberTypes.NonPublicNestedTypes; | ||
| internal const DynamicallyAccessedMemberTypes MethodsConstructors = MethodsAndPrivateNested | DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors; | ||
|
|
||
| protected override IEnumerable<Type> GetTypesForSimpleReference (string jniSimpleReference) | ||
| { | ||
| if (typeMappings.TryGetValue (jniSimpleReference, out var target)) | ||
| var target = GetTypeForSimpleReference (jniSimpleReference); | ||
| if (target != null) | ||
| yield return target; | ||
| foreach (var t in base.GetTypesForSimpleReference (jniSimpleReference)) | ||
| yield return t; | ||
| } | ||
|
|
||
| [return: DynamicallyAccessedMembers (MethodsConstructors)] | ||
| protected override Type? GetTypeForSimpleReference (string jniSimpleReference) | ||
| { | ||
| return jniSimpleReference switch { | ||
| Example.ManagedType.JniTypeName => typeof (Example.ManagedType), | ||
| "java/lang/Object" => typeof (Java.Lang.Object), | ||
| "java/lang/String" => typeof (Java.Lang.String), | ||
| _ => null, | ||
| }; | ||
| } | ||
|
|
||
| public override IEnumerable<Type> GetTypes (JniTypeSignature typeSignature) | ||
| { | ||
| if (!typeSignature.IsValid || typeSignature.ArrayRank != 0 || typeSignature.SimpleReference == null) | ||
| return []; | ||
| return GetTypesForSimpleReference (typeSignature.SimpleReference); | ||
| } | ||
|
|
||
| public override IEnumerable<JniRuntime.JniTypeManager.ReflectionConstructibleType> GetReflectionConstructibleTypes (JniTypeSignature typeSignature) | ||
| { | ||
| if (!typeSignature.IsValid || typeSignature.ArrayRank != 0 || typeSignature.SimpleReference == null) | ||
| yield break; | ||
| var target = GetTypeForSimpleReference (typeSignature.SimpleReference); | ||
| if (target != null) | ||
| yield return new JniRuntime.JniTypeManager.ReflectionConstructibleType (target); | ||
| } | ||
|
|
||
| protected override IEnumerable<string> GetSimpleReferences (Type type) | ||
| { | ||
| return base.GetSimpleReferences (type) | ||
| .Concat (CreateSimpleReferencesEnumerator (type)); | ||
| return CreateSimpleReferencesEnumerator (type); | ||
| } | ||
|
|
||
| IEnumerable<string> CreateSimpleReferencesEnumerator (Type type) | ||
| { | ||
| if (typeMappings == null) | ||
| yield break; | ||
| foreach (var e in typeMappings) { | ||
| if (e.Value == type) | ||
| yield return e.Key; | ||
| if (type == typeof (Example.ManagedType)) | ||
| yield return Example.ManagedType.JniTypeName; | ||
| else if (type == typeof (Java.Lang.Object)) | ||
| yield return "java/lang/Object"; | ||
| else if (type == typeof (Java.Lang.String)) | ||
| yield return "java/lang/String"; | ||
| } | ||
|
|
||
| protected override string? GetSimpleReference (Type type) | ||
| { | ||
| return GetSimpleReferences (type).FirstOrDefault (); | ||
| } | ||
|
|
||
| protected override JniTypeSignature GetTypeSignatureCore (Type type) | ||
| { | ||
| var simpleReference = GetSimpleReference (type); | ||
| return simpleReference == null ? default : new JniTypeSignature (simpleReference, 0, false); | ||
| } | ||
|
|
||
| protected override IEnumerable<JniTypeSignature> GetTypeSignaturesCore (Type type) | ||
| { | ||
| var signature = GetTypeSignatureCore (type); | ||
| if (signature.IsValid) | ||
| yield return signature; | ||
| } | ||
|
|
||
| [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)] | ||
| protected override Type? GetInvokerTypeCore ( | ||
| [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)] | ||
| Type type) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| protected override IReadOnlyList<string>? GetStaticMethodFallbackTypesCore (string jniSimpleReference) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| protected override string? GetReplacementTypeCore (string jniSimpleReference) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| protected override JniRuntime.ReplacementMethodInfo? GetReplacementMethodInfoCore (string jniSourceType, string jniMethodName, string jniMethodSignature) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| public override void RegisterNativeMembers ( | ||
| JniType nativeClass, | ||
| [DynamicallyAccessedMembers (MethodsAndPrivateNested)] | ||
| Type type, | ||
| ReadOnlySpan<char> methods) | ||
| { | ||
| if (type != typeof (Example.ManagedType)) { | ||
| if (!methods.IsEmpty) | ||
| throw new NotSupportedException ($"Could not register native members for type '{type.FullName}'."); | ||
| return; | ||
| } | ||
|
|
||
| var registrations = new List<JniNativeMethodRegistration> (); | ||
| Example.ManagedType.RegisterNativeMembers (new JniNativeMethodRegistrationArguments (registrations, null)); | ||
| if (registrations.Count > 0) | ||
| nativeClass.RegisterNativeMethods (registrations.ToArray ()); | ||
| } | ||
|
|
||
| [Obsolete ("Use RegisterNativeMembers(JniType, Type, ReadOnlySpan<char>)")] | ||
| public override void RegisterNativeMembers ( | ||
| JniType nativeClass, | ||
| [DynamicallyAccessedMembers (MethodsAndPrivateNested)] | ||
| Type type, | ||
| string? methods) | ||
| { | ||
| RegisterNativeMembers (nativeClass, type, methods.AsSpan ()); | ||
| } | ||
| } |
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖⚠️ Code organization — The type mappings in this
GetTypeForSimpleReferenceswitch statement duplicate thetypeMappingsdictionary on lines 13–21.GetTypeForSimpleReferencedoes forward lookups (string → Type) andCreateSimpleReferencesEnumeratordoes reverse lookups (Type → string) via the dictionary.If someone adds a type to one but not the other, forward/reverse resolution will silently diverge. The
Hello-NativeAOTFromJNIsample avoids this by encoding both directions directly in code without a dictionary.Consider either:
typeMappingsdictionary and using if/else type checks inCreateSimpleReferencesEnumerator(matching theHello-NativeAOTFromJNIpattern), orRule: Code duplication — single source of truth