mozilla / rhino

Rhino is an open-source implementation of JavaScript written entirely in Java
https://rhino.github.io
Other
4.18k stars 851 forks source link

NativeJavaClass: Add ".class" to access the associated Java Class #1679

Open camnwalter opened 1 month ago

camnwalter commented 1 month ago

First time making tests so let me know if there is some case that isn't covered or whatever else I should change.

This PR adds support for class literal syntax for NativeJavaClasses. Before this, an example would be trying to get the Class associated with java.lang.Object, where the user would have to do

java.lang.Object.__javaObject__

or

var objectClass = org.mozilla.javascript.Context.jsToJava(java.lang.Object, java.lang.Class)

This makes it so that

java.lang.Object.class

works as well, just like it looks like in Java. The work around I had to add was this, due to how Rhino automatically creates bean properties associated with the getters and setters of the same name. If there is a class that has a static setClass() method, a class field is created, which would conflict with the goal of this PR. Instead, we just ignore that case if it exists, since there can't be a static class getter due to getClass() being defined as an instance method inside Object, so there is no static class getter associated to the bean property.

Closes #757

p-bakker commented 1 month ago

Looks like at least one of the testcases you added fails

camnwalter commented 1 month ago

I think that should be fixed now. I guess it was running it multiple times and so the static count variable was higher than expected

gbrail commented 1 month ago

The code looks OK and I see the issue, but will people understand what we changed here? I think that at the very least the PR or commit should describe what we did and what people can do now -- since I'm not sure where the LiveConnect "spec" or docs are, I'm not sure how people will understand what is or isn't possible unless we update our docs.

camnwalter commented 1 month ago

Renamed the PR and added a more detailed description on what was changed. Not sure where else to put this (other than the commit description).

gbrail commented 1 month ago

IMO, this is basically adding a new feature to LiveConnect, which looks useful to me, but I could use some more opinions.

What do regular users of the Java interop think about this?

p-bakker commented 1 month ago

Did you see the discussion on #757?

This makes sense to me, to simplify access to the class

rPraml commented 1 month ago

Hello, we use LC a lot. But until now I can't remember that we were missing this feature. If a Java method expected a class as parameter, you could just call myJavaFunc(java.lang.String)

Nevertheless, I can imagine use cases where it would be necessary. But I also think we should just rename this __javaObject__ to class because

So, I would have nothing against simply replacing __javaObject__ with the more intuitive class. But perhaps others can give their opinion on whether we can afford this breaking change or if we should impmement the same functionality twice?

BTW: It was added 17 years ago with this commit https://github.com/mozilla/rhino/commit/d6233135d460a52d34663e2a57d53c0409e5a4f0 and then changed from __javaClass__ to __javaObject__ https://github.com/mozilla/rhino/commit/c782d036e849f94f655b52ea5fc54c80c9b7c738

camnwalter commented 1 month ago

Nevertheless, I can imagine use cases where it would be necessary.

Yeah the main reason I see it being useful is for reflection.

gbrail commented 1 month ago

This seems useful but given that so much of this stuff isn't documented we'll I'm kind of waiting to see if we have more opinions.

I am trying to resurrect some of the old docs, once I do that perhaps you can all take a look and see if that's a good place to update.

p-bakker commented 1 month ago

@gbrail which old docs are you referring to? Thought we did resurrect all (relevant) docs from the Mozilla website into rhino.github.io already