Skip to content

Rewrite method collecting of JavaMembers - #2474

Open
ZZZank wants to merge 5 commits into
mozilla:masterfrom
ZZZank:method-collect
Open

ZZZank wants to merge 5 commits into
mozilla:masterfrom
ZZZank:method-collect

Conversation

@ZZZank

@ZZZank ZZZank commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

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_jdk11 is removed, and modular java related handling is now integrated into JavaMembers
  • More polished method filtering and deduplication, that considers synthetic method, generci param types, and the accessibility of declaring class
  • Inaccessible method will now be discarded early. Previously ExecutableBox accepts inaccessible method, and will try to make it accessible and retry after failure, but now JavaMembers will 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

  1. Fast path for public classes: The new version directly calls clazz.getMethods(), which is functionally equivalent to the original approach of going through discoverPublicMethods → clazz.getMethods().

  2. Parameter type handling: TypeInfo.asClass() returns the same values as method.getParameterTypes() for non-generic types, because TypeInfo is created from method.getGenericParameterTypes() which, after type erasure, is equivalent to getParameterTypes().

Behavioral Differences

Difference 1: Synthetic Method Filtering

  • Original: Does not filter synthetic methods
  • New: Filters out synthetic methods (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

  • Original: Catches SecurityException and falls back to discoverPublicMethods
  • New: Catches SecurityException and NoClassDefFoundError, then continues, skipping the class

Impact: 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

  • Original: Depth-first recursion; interface processing order may interleave with superclasses
  • New: Uses LinkedHashSet, ensuring superclasses are processed before interfaces, with automatic deduplication

Impact: 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

  • Original: JavaMembers_jdk11.discoverPublicMethods calls findAccessibleMethod for non-exported classes to locate an accessible version
  • New: Skips non-exported classes entirely (!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

  • Original: Deduplicates during discoverAccessibleMethods (based on MethodSignature and return type comparison)
  • New: Deduplicates during JavaMembers.collectMethods (based on ExecutableBox and 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

  • Original: Only compares return types, keeping the more concrete return type
  • New: First compares whether the declaring class is public, then compares return types

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:

  1. Classes that use synthetic methods
  2. Security-restricted environments (SecurityManager)
  3. Non-exported classes in modular Java
  4. When methods with identical parameter types exist but have different declaring classes

- 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
@gbrail

gbrail commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

I'm getting to this, thanks...

With this change, is ReflectUtils.IS_MODULAR_JAVA no longer used? Can we get rid of it?

@gbrail

gbrail commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

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?

@ZZZank

ZZZank commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

is ReflectUtils.IS_MODULAR_JAVA no longer used? Can we get rid of it?

It's still being used by ReflectUtils.isExportedClass(...) to keep compatibility with JVMs without module system, aka Android. So probably no.

public constructors of non-public classes are no longer invokable

Fixed in 7abbda1

for static members, what method gets selected changed a bit

Correct, this is due to .compareAmbiguousMethod() assuming that every methods particapates in inheritance, should fixed in 29dc1b8

a place somewhere to document all of the edge cases

I think I can document this in ./docs/lc/member-collect.md or ./docs/reflect/member-collect.md, or is there a dedicated doc repo for Rhino?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants