Conversation
- handle modular java directly, get rid of `JavaMembers_jdk11` - More fine-tuned method deduplication that considers generic(`box.getArgTypes()`), accessibility of declared class - inaccessible methods will be discarded early
0e6cb61 to
d480240
Compare
|
I'm getting to this, thanks... With this change, is ReflectUtils.IS_MODULAR_JAVA no longer used? Can we get rid of it? |
|
Looks OK to me but the AI found a regression: The PR removed the tryToMakeAccessible retry from ExecutableBox.newInstance (ExecutableBox.java:185-189) on the premise that "JavaMembers should be responsible for ensuring methods visible to JS are accessible" — but that's only done in collectMethods for methods. getAccessibleConstructors(includePrivate=false) (JavaMembers.java:699) returns cl.getConstructors() without any setAccessible, so public constructors of non-public classes are no longer invokable. It also looks like, for static members, what method gets selected changed a bit. I'm happy to keep cleaning up this code, but do we have a place somewhere to document all of the edge cases for how we discover which method to call when there is a combination of public, package, and private, and different types? |
It's still being used by
Fixed in 7abbda1
Correct, this is due to
I think I can document this in |
Changes in this PR is actually cherry-picked from my draft branches, because apparently rewriting the whole JS-Java interop thing has caused my git commit history to be a completely chaotic mess.
JavaMembers_jdk11is removed, and modular java related handling is now integrated intoJavaMembersExecutableBoxaccepts inaccessible method, and will try to make it accessible and retry after failure, but nowJavaMemberswill 1. select more accessible method when collecting method, and 2. test method accessibility, and discard it if it's inaccessible anyway.This PR is stacked on #2452 , for
ReflectUtil.isClassExported(clazz)Analysis by AI
Analysis: Behavioral Differences Between Original and Rewritten Code
Equivalent Behaviors
Fast path for public classes: The new version directly calls
clazz.getMethods(), which is functionally equivalent to the original approach of going throughdiscoverPublicMethods→clazz.getMethods().Parameter type handling:
TypeInfo.asClass()returns the same values asmethod.getParameterTypes()for non-generic types, becauseTypeInfois created frommethod.getGenericParameterTypes()which, after type erasure, is equivalent togetParameterTypes().Behavioral Differences
Difference 1: Synthetic Method Filtering
method.isSynthetic())Impact: Synthetic methods (such as compiler-generated bridge methods and inner class access methods) are ignored in the new version. This is typically the desired behavior, as synthetic methods should not be exposed to scripts.
Difference 2: SecurityException Handling
SecurityExceptionand falls back todiscoverPublicMethodsSecurityExceptionandNoClassDefFoundError, thencontinues, skipping the classImpact: In security-restricted environments (e.g., applets), the original version attempts to obtain methods via
getMethods(), while the new version skips the class entirely. This may cause certain methods to be lost.Difference 3: Inheritance Hierarchy Traversal Order
LinkedHashSet, ensuring superclasses are processed before interfaces, with automatic deduplicationImpact: Method collection order may differ. The original version may process the same class multiple times (via different paths); the new version does not.
Difference 4: Non-exported Class Handling
JavaMembers_jdk11.discoverPublicMethodscallsfindAccessibleMethodfor non-exported classes to locate an accessible version!ReflectUtils.isExportedClass(parent))Impact: In modular Java environments, for non-exported classes, the original version attempts to find accessible methods from superclasses/interfaces, while the new version skips methods of that class entirely.
Difference 5: Deduplication Timing
discoverAccessibleMethods(based onMethodSignatureand return type comparison)JavaMembers.collectMethods(based onExecutableBoxand declaration class publicness + return type comparison)Impact: The new version adds "whether the declaring class is public" as an additional deduplication criterion, preferring methods whose declaring class is public.
Difference 6: Comparison Strategy
Impact: When two methods have the same parameter types but different declaring classes, the new version prefers the method whose declaring class is public.
Conclusion
The rewritten code works correctly in most cases but may exhibit behavioral differences in the following scenarios: