Skip to content
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

Add support for covariant return types #1575

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions rhino/src/main/java/org/mozilla/javascript/JavaMembers.java
Original file line number Diff line number Diff line change
Expand Up @@ -326,11 +326,8 @@ private void discoverAccessibleMethods(
int mods = method.getModifiers();

if (isPublic(mods) || isProtected(mods) || includePrivate) {
MethodSignature sig = new MethodSignature(method);
if (!map.containsKey(sig)) {
if (includePrivate) method.trySetAccessible();
map.put(sig, method);
}
if (includePrivate) method.trySetAccessible();
registerMethod(map, method);
}
}
Class<?>[] interfaces = clazz.getInterfaces();
Expand All @@ -343,11 +340,7 @@ private void discoverAccessibleMethods(
// Some security settings (i.e., applets) disallow
// access to Class.getDeclaredMethods. Fall back to
// Class.getMethods.
Method[] methods = clazz.getMethods();
for (Method method : methods) {
MethodSignature sig = new MethodSignature(method);
if (!map.containsKey(sig)) map.put(sig, method);
}
discoverPublicMethods(clazz, map);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in discoverPublicMethods:

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

break; // getMethods gets superclass methods, no
// need to loop any more
}
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

// in JLS) we will take the best method
map.put(sig, method);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

package org.mozilla.javascript.tests;

import org.junit.Test;
import org.mozilla.javascript.tools.shell.Global;

/**
* If you inherit from an abstract class and narrow the return type, you will get both methods on
* Class.getMethods() and in the bytecode.
*
* <p>See explanation of {@link Class#getMethods()} <i> There may be more than one method with a
* particular name and parameter types in a class because while the Java language forbids a class to
* declare multiple methods with the same signature but different return types, the Java virtual
* machine does not. </i>
*
* <p>Note: It totally depends on the JVM, in which order the methods are returend, so we have a
* class here, that tries to simulate that 5 times to increase the probability that rhino will not
* detect at least one case as bean property
*
* @author Roland Praml
*/
public class CovariantReturnTypeTest {

public abstract static class A {
public abstract Object getValue1();

public abstract Object getValue2();

public abstract Object getValue3();

public abstract Object getValue4();

public abstract Object getValue5();
}

public static class B extends A {
private Integer value1;
private Integer value2;
private Integer value3;
private Integer value4;
private Integer value5;

// Note: Class B will inherit
@Override
public Integer getValue1() {
return value1;
}

public void setValue1(Integer value1) {
this.value1 = value1;
}

@Override
public Integer getValue2() {
return value2;
}

public void setValue2(Integer value2) {
this.value2 = value2;
}

@Override
public Integer getValue3() {
return value3;
}

public void setValue3(Integer value3) {
this.value3 = value3;
}

@Override
public Integer getValue4() {
return value4;
}

public void setValue4(Integer value4) {
this.value4 = value4;
}

@Override
public Integer getValue5() {
return value5;
}

public void setValue5(Integer value5) {
this.value5 = value5;
}
}

static final String LS = System.getProperty("line.separator");

@Test
public void checkIt() {
Utils.runWithAllOptimizationLevels(
cx -> {
final Global scope = new Global();
scope.init(cx);
scope.put("obj", scope, new B());
Object ret =
cx.evaluateString(
scope,
"obj.value1 = 1; obj.value2 = 2; obj.value3 = 3; obj.value4 = 4; obj.value5 = 5",
"myScript.js",
1,
null);
return null;
});
}
}
Loading