Add support for covariant return types #1575

rPraml · 2024-08-22T06:52:08Z

A few days ago, we discovered also a strange behaviour in the js<->java liveconnect code.

Normally, rhino will detect getters/setters in java classes and allows you, to access them as properties.

e.g. if you have a class like

public class MyBean {
  public String getValue() {return null;}
  public void setValue(String value) {...}
}

you can read/write the value with myBean.value = "foo" in javascript. This works if there are matching getters/setters found. (If there is an incompatible setter like setValue(Integer value) it will no longer work, as Integer is not an instance of String.
BTW: It will work if there is a setValue(Object value), as this setter accepts also strings.

If you now inherit and narrow the return type, you will get multiple methods with covariant return types, e.g.

public interface ValueHolder {
   Object getValue();
}
public class MyBean implements ValueHolder {...}

will result in the following bytecode:

public getValue()Ljava/lang/String;
public synthetic bridge getValue()Ljava/lang/Object;

Note: You get the same method with same signature, but different return types. This is not allowed by the JLS, but at VM/bytecode level and it is explicitly mentioned in https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Class.html#getMethods()

What happens in Rhino now:

Rhino takes the FIRST method of each signature (signature = method name + parameters)
So if there are covariant methods, it will either take getValue()Ljava/lang/String or getValue()Ljava/lang/Object
After that, it tries to find a compatible setter.
-- If getValue()Ljava/lang/String was found, it will find also the setValue(String) setter and generates a String-BeanProperty
-- If getValue()Ljava/lang/Object was found, it will the setValue(String) setter, but cannot use it, because rhino thinks the property is of type Object and the setter is incompatible
The main problem is, that Class.getMethods() does not return the methods in a particular order. (What happened: In our unit tests, the correct method was preferred, in production the wrong method was preferred)

This PR changes the method discovery in a way that if a method with same signature was found, but a "better" return type, this method is used.

gbrail · 2024-08-23T05:24:17Z

Looks like a good quality thing to fix to me but I don't work with the LiveConnect stuff very much.

Does anyone else who uses this stuff extensively want to have a look?

rbri · 2024-08-23T06:02:39Z

Does anyone else who uses this stuff extensively want to have a look?

sorry i do not use it

p-bakker · 2024-08-23T06:04:46Z

@andreabergia care to have a look at this one? Seems like your team also uses Java interop a fair bit

p-bakker · 2024-08-24T05:25:32Z

Don't use it either to the extend that we would run into this, but reading the explanation and the linked Java succes, I think the PR makes sense

rPraml · 2024-08-22T06:53:16Z

rhino/src/main/java/org/mozilla/javascript/JavaMembers.java

+        if (existing == null) {
+            map.put(sig, method);
+        } else if (existing.getReturnType().isAssignableFrom(method.getReturnType())) {
+            // if there are multiple methods with same name (which is allowed in bytecode, but not


e.g. if the existing method has a return type of Object but new method will return String, we will use this method.

I'm on a Map efficiency kick this week, so I apologize in advance -- but is there a way that you could replace the "map.get" / "map.put" pair with "putIfAbsent" or something like that? I'm not 100% sure but I think that if it works you could have fewer hash map traversals.

I think the Map calls are actually ok the way that they are. The default implementation of putIfAbsent does exactly the same thing, and this way he already has a reference to the existing value for the else condition without needing to get it again.

Ignore my last comment. HashMap does not use the default Map implementation, so there may be a slight improvement. @gbrail, would it need to look something like this instead?

Method putResult = map.putIfAbsent(sig, method); if (putResult != method && putResult.getReturnType().isAssignableFrom(method.getReturnType())) { map.put(sig, method); }

Waiting for feedback of #1644, because I'll expect a merge conflict here

rPraml · 2024-08-22T06:56:01Z

rhino/src/main/java/org/mozilla/javascript/JavaMembers.java

-                                MethodSignature sig = new MethodSignature(method);
-                                if (!map.containsKey(sig)) map.put(sig, method);
-                            }
+                            discoverPublicMethods(clazz, map);


The code in discoverPublicMethods:

void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map) { Method[] methods = clazz.getMethods(); for (Method method : methods) { registerMethod(map, method); } }

gbrail · 2024-09-08T21:23:00Z

I would like to move forward with this but I'd like to know if there is a way to optimize the map usage like I commented above. Thanks!

gbrail · 2024-09-12T05:52:03Z

Yep -- that saves us from traversing the hash table twice (once for the get and again for the put) and that can really add up if this happens in a place that gets called a lot!

…

On Wed, Sep 11, 2024 at 10:46 AM Tony Germano ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In rhino/src/main/java/org/mozilla/javascript/JavaMembers.java <#1575 (comment)>: > @@ -387,7 +380,12 @@ void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map) { static void registerMethod(Map<MethodSignature, Method> map, Method method) { MethodSignature sig = new MethodSignature(method); // Array may contain methods with same signature but different return value! - if (!map.containsKey(sig)) { + Method existing = map.get(sig); + if (existing == null) { + map.put(sig, method); + } else if (existing.getReturnType().isAssignableFrom(method.getReturnType())) { + // if there are multiple methods with same name (which is allowed in bytecode, but not Ignore my last comment. HashMap does not use the default Map implementation, so there may be a slight improvement. @gbrail <https://github.com/gbrail>, would it need to look something like this instead? Method putResult = map.putIfAbsent(sig, method);if (putResult != method && putResult.getReturnType().isAssignableFrom(method.getReturnType())) { map.put(sig, method); } — Reply to this email directly, view it on GitHub <#1575 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I26IIFHR6RVAQ4ZV3NLZWB6VZAVCNFSM6AAAAABM5O532SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOJYGEZDAMZTHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

rPraml · 2024-09-18T19:47:04Z

Can take a look at the PR next week or so

rPraml · 2024-09-24T11:57:54Z

@gbrail FYI: This PR is IMHO ready for merge now.

gbrail · 2024-09-24T23:31:23Z

Thanks, but sorry -- three different people changed JavaMembers.java over the last two weeks and rebasing this to master is complicated. Can you please rebase this to the latest master branch and make sure that when done this reflects what you want? Thanks!

rPraml

Thanks, but sorry -- three different people changed JavaMembers.java over the last two weeks and rebasing this to master is complicated. Can you please rebase this to the latest master branch and make sure that when done this reflects what you want? Thanks!

I've added some explaining comments merged back the develop branch.
Or would you prefer a rebase + force-push in these cases?

rPraml · 2024-09-25T06:52:22Z

rhino/src/main/java/org/mozilla/javascript/JavaMembers.java

@@ -393,10 +382,21 @@ void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map) {
        }
    }

-    static void registerMethod(Map<MethodSignature, Method> map, Method method) {
+    static Method registerMethod(Map<MethodSignature, Method> map, Method method) {


registerMethod retuns the added method (required to make it accessible)

rPraml · 2024-09-25T06:52:46Z

rhino/src/main/java/org/mozilla/javascript/JavaMembers.java

+        if (oldValue.getReturnType().equals(newValue.getReturnType())) {
+            return oldValue; // same return type. Do not overwrite existing method
+        } else if (oldValue.getReturnType().isAssignableFrom(newValue.getReturnType())) {
+            return newValue; // more concrete return type. Replace method


I could merge the two ifs, but that makes the code less readable

rPraml · 2024-09-25T06:56:35Z

rhino/src/main/java/org/mozilla/javascript/JavaMembers.java

-                                    } else {
-                                        map.putIfAbsent(sig, method);
+                                    if (includePrivate && !registered.isAccessible()) {
+                                        registered.setAccessible(true);


@gbrail There was a recent change. This code makes the registered method accessible once, without adding much complexity with computeIfAbsent
That's why I've made registerMethod to return the current used method

gbrail · 2024-09-27T22:49:50Z

Sorry, another hairy rebase is required here. It seems that everyone wants to make changes to the JavaAdapter class!

Can you please try to rebase this on master? Thanks!
_

rPraml · 2024-09-28T10:45:37Z

No problem, I can do a rebase (probably not until Monday)

@gbrail I assume you want a rebase so that the GIT history is clean (no "gitar" hero), right?

In my company and in a few other projects, we have switched to selecting "squash" when merging. Then all changes are summarized in one commit. This has the advantage that each pull request is exactly one commit. This also makes it easier to revert a PR. However, you lose the history of the individual commits in the PR.

I just wanted to mention this because I don't know if you know about the feature and it would ultimately be easier for everyone involved (Contributors do not need to rebase, you get one commit for each feature and you don't have to ask people to do another roundtrip), but as I said, it's no problem for me to do a rebase next week. There may be also more disadvantages, why you don't want to do a sqash merge.

gbrail · 2024-09-28T18:15:42Z

Yes -- I'd like to keep the commit history clean and I think I even enabled a branch protection rule to prevent merge commits in the master branch.

I have sometimes done squash merges, but I haven't always taken the time. Let's try to squash more merges going forward and see if that helps with all the rebasing.

With that said, if multiple developers have PRs open that change the same files, even if I squash everything, I fear we'll still have to deal with conflicts -- let's see what comes up. Thanks!

p-bakker · 2024-09-29T11:49:13Z

Imho whether to squash commits or not it's something that needs to be determined on a case-by-case basis: if a PR contains only one isolated change but a lot of cleanup/fix commits, then a squash makes sense.

However, if a larger change consists of a few distinct changes to come to an end result and separating the distinct changes into separate commits makes the larger change easier to understand, now and in the future, then squashing doesn't make sense imho.

The challenge is when a larger change consists of a mix of change, fix & cleanup commits. Personally, in such a case I prefer to do an interactive rebase, reordering and/or merging commits into a logical sequence of distinct changes that make sense to (re)view in isolation, now and in the future.

But this approach does mean more work on the dev to create a clean commit history, cause it requires to go through an interactive rebase if just having to do a small stylistic change, but in the long run it is worth it imho

Unfortunately, it seems GitHub didn't allow PR authors to specific whether to squash merge or not...

rPraml · 2024-09-30T06:50:35Z

PR is up to date again ;)

gbrail · 2024-10-01T04:08:40Z

This is great -- thanks!

p-bakker added the Java Interop Issues related to the interaction between Java and JavaScript label Aug 24, 2024

rPraml commented Aug 24, 2024

View reviewed changes

This was referenced Sep 23, 2024

Optimize map accesses in several places #1644

Merged

fix: bytecode version unspecified & NPE and trySetAccessible method does not exist on Android #1636

Closed

rPraml commented Sep 25, 2024

View reviewed changes

Add support for covariant return types

96ae158

rPraml force-pushed the covariant-methods branch from 1a2bbd9 to 96ae158 Compare September 30, 2024 06:49

gbrail merged commit ff46060 into mozilla:master Oct 1, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for covariant return types #1575

Add support for covariant return types #1575

rPraml commented Aug 22, 2024

gbrail commented Aug 23, 2024

rbri commented Aug 23, 2024

p-bakker commented Aug 23, 2024

p-bakker commented Aug 24, 2024

rPraml Aug 22, 2024

gbrail Aug 24, 2024

tonygermano Sep 11, 2024

tonygermano Sep 11, 2024

rPraml Sep 23, 2024

rPraml Aug 22, 2024

gbrail commented Sep 8, 2024

gbrail commented Sep 12, 2024 via email

rPraml commented Sep 18, 2024

rPraml commented Sep 24, 2024

gbrail commented Sep 24, 2024

rPraml left a comment

rPraml Sep 25, 2024

rPraml Sep 25, 2024

rPraml Sep 25, 2024

gbrail commented Sep 27, 2024

rPraml commented Sep 28, 2024

gbrail commented Sep 28, 2024

p-bakker commented Sep 29, 2024 •

edited

Loading

rPraml commented Sep 30, 2024

gbrail commented Oct 1, 2024

Add support for covariant return types #1575

Add support for covariant return types #1575

Conversation

rPraml commented Aug 22, 2024

gbrail commented Aug 23, 2024

rbri commented Aug 23, 2024

p-bakker commented Aug 23, 2024

p-bakker commented Aug 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbrail commented Sep 8, 2024

gbrail commented Sep 12, 2024 via email

rPraml commented Sep 18, 2024

rPraml commented Sep 24, 2024

gbrail commented Sep 24, 2024

rPraml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbrail commented Sep 27, 2024

rPraml commented Sep 28, 2024

gbrail commented Sep 28, 2024

p-bakker commented Sep 29, 2024 • edited Loading

rPraml commented Sep 30, 2024

gbrail commented Oct 1, 2024

p-bakker commented Sep 29, 2024 •

edited

Loading