Skip to content

Commit

Permalink
Add nullable JS API parameter support
Browse files Browse the repository at this point in the history
This change allows to define JS APIs which receive nullable primitive
type parameters, by overriding NullabilityDetector interface.
E.g. clients can introduce Nullable-like annotation which they can
read at runtime, thus enabling passing null to JS function with string
parameter (by default such function receives a string "null")

One of the following commits will introduce KotlinNullabilityDetector,
which will rely on parsing Kotlin metadata and will allow a better
inter-operation of Rhino with Kotlin.

KotlinNullabilityDetector relies on Kotlin Metadata to detect parameter
nullability.

The limitation of this detector is that it cannot distinguish between
overloads with the same number of parameters. To avoid false positives,
KotlinNullabilityDetector returns array of 'false' values for methods
or constructors with such overloads.
  • Loading branch information
drubanovich-soti authored Jan 31, 2025
1 parent 631ef54 commit 07492b5
Show file tree
Hide file tree
Showing 21 changed files with 570 additions and 11 deletions.
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,16 @@ JavaDoc for all the APIs:

Rhino 1.7.15 and before were primarily used in a single JAR called "rhino.jar".

Newer releases now organize the code using Java modules. There are four primary modules:
Newer releases now organize the code using Java modules. There are four primary modules and one auxiliary module for Kotlin developers:

* **rhino**: The primary codebase necessary and sufficient to run JavaScript code. Required by everything that uses Rhino. In releases *after* 1.7.15, this module does not contain the "tools" or the XML implementation.
* **rhino-tools**: Contains the shell, debugger, and the "Global" object, which many tests and other Rhino-based tools use. Note that adding Global gives Rhino the ability to print to stdout, open files, and do other things that may be considered dangerous in a sensitive environment, so it only makes sense to include if you will use it.
* **rhino-xml**: Adds the implementation of the E4X XML standard. Only required if you are using that.
* **rhino-engine**: Adds the Rhino implementation of the standard Java *ScriptEngine* interface. Some projects use this to be able to switch between script execution engines, but for anything even moderately complex it is almost always easier and always more flexible to use Rhino's API directly.
* **rhino-all**: This creates an "all-in-one" JAR that includes *rhino-runtime*, *rhino-tools*, and *rhino-xml*. This is what's used if you want to run Rhino using "java jar".

* **rhino-kotlin**: Enhanced support for code written in Kotlin, [see the details.](./rhino-kotlin/README.md)

The release contains the following other modules, which are used while building and
testing but which are not published to Maven Central:

Expand Down
44 changes: 44 additions & 0 deletions rhino-kotlin/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Kotlin Support

The rhino-kotlin module uses Kotlin metadata to augment function and property information,
making JavaScript APIs more nuanced.

For example, the following code exposes setValue JavaScript function:
```
defineProperty(
topLevelScope,
"setValue",
Bundle::class.java.methods.find {
it.name == "setValue"
},
DONT_ENUM
)
```
which is backed by the following Kotlin class:
```
class Bundle() : ScriptableObject {
val valueMap = emptyMap<String, String?>()
fun setValue(key: String, value: String?) {
valueMap[key] = value
}
}
```
Imagine rhino-kotlin is not used and JavaScript code tries to call setValue with `null` value parameter:
```
setValue("key", null)
```
This will lead to unexpected result - a 4-char long string "null" will be inserted into the backing map.
This happens because Rhino engine doesn't know how to infer parameter nullability from pure Java code,
so it tries to convert `null` to String.

Adding rhino-kotlin dependency fixes the problem, allowing JavaScript functions to have nullable parameters.

At the moment, only parameter nullability is supported, but we might add more Kotlin-specific support in future.

**Note:** If building for Android, make sure the kotlin.Metadata class is not obfuscated by adding the following line
into the proguard configuration:
```
-keep class kotlin.Metadata
```
Without this line Rhino Kotlin support won't work in release apk.
48 changes: 48 additions & 0 deletions rhino-kotlin/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
plugins {
id 'rhino.library-conventions'
id 'org.jetbrains.kotlin.jvm'
}

dependencies {
implementation project(':rhino')
implementation "org.jetbrains.kotlin:kotlin-metadata-jvm:2.1.0"

testImplementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:2.1.0"
}

kotlin {
jvmToolchain(11)
}

publishing {
publications {
rhinokotlin(MavenPublication) {
from components.java
artifacts = [jar, sourceJar]
pom.withXml {
def root = asNode()

root.appendNode('description', "Rhino reflection interfaces implementations for Kotlin")
root.appendNode("url", "https://mozilla.github.io/rhino/")

def p = root.appendNode("parent")
p.appendNode("groupId", "org.sonatype.oss")
p.appendNode("artifactId", "oss-parent")
p.appendNode("version", "7")

def l = root.appendNode("licenses").appendNode("license")
l.appendNode("name", "Mozilla Public License, Version 2.0")
l.appendNode("url", "http://www.mozilla.org/MPL/2.0/index.txt")

def scm = root.appendNode("scm")
scm.appendNode("connection", "scm:git:[email protected]:mozilla/rhino.git")
scm.appendNode("developerConnection", "scm:git:[email protected]:mozilla/rhino.git")
scm.appendNode("url", "[email protected]:mozilla/rhino.git")

def o = root.appendNode("organization")
o.appendNode("name", "The Mozilla Foundation")
o.appendNode("url", "http://www.mozilla.org")
}
}
}
}
11 changes: 11 additions & 0 deletions rhino-kotlin/src/main/java/module-info.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import org.mozilla.javascript.NullabilityDetector;
import org.mozilla.kotlin.KotlinNullabilityDetector;

module org.mozilla.rhino.kotlin {
requires kotlin.metadata.jvm;
requires kotlin.stdlib;
requires org.mozilla.rhino;

provides NullabilityDetector with
KotlinNullabilityDetector;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package org.mozilla.kotlin;

import static kotlin.metadata.Attributes.isNullable;

import java.lang.reflect.Constructor;
import java.lang.reflect.Method;
import java.util.List;
import java.util.stream.Collectors;
import kotlin.Metadata;
import kotlin.metadata.KmClass;
import kotlin.metadata.KmConstructor;
import kotlin.metadata.KmFunction;
import kotlin.metadata.KmValueParameter;
import kotlin.metadata.jvm.KotlinClassMetadata;
import org.mozilla.javascript.NullabilityDetector;

public class KotlinNullabilityDetector implements NullabilityDetector {
@Override
public boolean[] getParameterNullability(Method method) {
int paramCount = method.getParameterTypes().length;
KmClass kmClass = getKmClassForJavaClass(method.getDeclaringClass());
return getMethodParameterNullabilityFromKotlinMetadata(
kmClass, method.getName(), paramCount);
}

@Override
public boolean[] getParameterNullability(Constructor<?> constructor) {
int paramCount = constructor.getParameterTypes().length;
KmClass kmClass = getKmClassForJavaClass(constructor.getDeclaringClass());
return getConstructorParameterNullabilityFromKotlinMetadata(kmClass, paramCount);
}

private KmClass getKmClassForJavaClass(Class<?> javaClass) {
Metadata metadata = javaClass.getAnnotation(Metadata.class);
if (metadata != null) {
KotlinClassMetadata.Class kMetadata =
(KotlinClassMetadata.Class) KotlinClassMetadata.readLenient(metadata);
return kMetadata.getKmClass();
} else {
return null;
}
}

private boolean[] getMethodParameterNullabilityFromKotlinMetadata(
KmClass clazz, String methodName, int paramCount) {
boolean[] fallback = createFallbackNullabilityArray(paramCount);
if (clazz == null) {
return fallback;
}
List<KmFunction> candidates =
clazz.getFunctions().stream()
.filter(
f ->
f.getName().equals(methodName)
&& f.getValueParameters().size() == paramCount)
.collect(Collectors.toList());
return candidates.size() == 1
? createNullabilityArray(candidates.get(0).getValueParameters())
: fallback;
}

private boolean[] getConstructorParameterNullabilityFromKotlinMetadata(
KmClass clazz, int paramCount) {
boolean[] fallback = createFallbackNullabilityArray(paramCount);
if (clazz == null) {
return fallback;
}
List<KmConstructor> candidates =
clazz.getConstructors().stream()
.filter(c -> c.getValueParameters().size() == paramCount)
.collect(Collectors.toList());
return candidates.size() == 1
? createNullabilityArray(candidates.get(0).getValueParameters())
: fallback;
}

private boolean[] createNullabilityArray(List<KmValueParameter> params) {
boolean[] result = new boolean[params.size()];
int index = 0;
for (KmValueParameter parameter : params) {
result[index++] = isNullable(parameter.getType());
}
return result;
}

private boolean[] createFallbackNullabilityArray(int paramCount) {
return new boolean[paramCount];
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
org.mozilla.kotlin.KotlinNullabilityDetector
15 changes: 15 additions & 0 deletions rhino-kotlin/src/test/java/org/mozilla/kotlin/JavaClass.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package org.mozilla.kotlin;

public class JavaClass {
private final String property1;
private final String property2;

public JavaClass(String param1, String param2) {
property1 = param1;
property2 = param2;
}

public void function(Integer param1, Long param2) {
// Do nothing
}
}
14 changes: 14 additions & 0 deletions rhino-kotlin/src/test/java/org/mozilla/kotlin/KotlinClass.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package org.mozilla.kotlin

class KotlinClass(
val nonNullProperty: String,
val nullableProperty: String?
) {
fun function(nullableParam: Int?, nonNullParam: Int, anotherNullableParam: KotlinClass?) {
// Do nothing
}

fun function(nonNullParam: Int, anotherNonNullParam: Int) {
// Do nothing
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package org.mozilla.kotlin

class KotlinClassWithOverloadedFunction {
fun function(nullableParam: Int?, nonNullParam: Int, anotherNullableParam: KotlinClass?) {
// Do nothing
}

fun function(nullableParam: Int?, nonNullParam: Int, anotherNonNullParam: String) {
// Do nothing
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package org.mozilla.kotlin;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;

import java.lang.reflect.Constructor;
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;
import org.junit.Test;

public class KotlinNullabilityDetectorTest {
private final KotlinNullabilityDetector detector = new KotlinNullabilityDetector();

@Test
public void testKotlinFunction() {
boolean[] nullability =
detector.getParameterNullability(findMethod(KotlinClass.class, "function", 3));

boolean[] expectedNullability = new boolean[] {true, false, true};
assertThat(nullability, is(expectedNullability));
}

@Test
public void testKotlinConstructor() {
boolean[] nullability =
detector.getParameterNullability(findConstructor(KotlinClass.class, 2));

boolean[] expectedNullability = new boolean[] {false, true};
assertThat(nullability, is(expectedNullability));
}

@Test
public void testJavaFunction() {
boolean[] nullability =
detector.getParameterNullability(findMethod(JavaClass.class, "function", 2));

boolean[] expectedNullability = new boolean[] {false, false};
assertThat(nullability, is(expectedNullability));
}

@Test
public void testJavaConstructor() {
boolean[] nullability =
detector.getParameterNullability(findConstructor(JavaClass.class, 2));

boolean[] expectedNullability = new boolean[] {false, false};
assertThat(nullability, is(expectedNullability));
}

@Test
public void testKotlinOverloadedFunction() {
List<Method> overloadedMethods =
findMethods(KotlinClassWithOverloadedFunction.class, "function");

assertThat(overloadedMethods.size(), is(2));
// Since we cannot distinguish overloads with same number of params, we have to fallback to
// no-op
boolean[] expectedNullability = new boolean[] {false, false, false};
overloadedMethods.forEach(
method ->
assertThat(
detector.getParameterNullability(method), is(expectedNullability)));
}

private Method findMethod(Class<?> clazz, String methodName, int paramCount) {
return Arrays.stream(clazz.getMethods())
.filter(
method ->
method.getName().equals(methodName)
&& method.getParameterTypes().length == paramCount)
.findFirst()
.orElse(null);
}

private Constructor<?> findConstructor(Class<?> clazz, int paramCount) {
return Arrays.stream(clazz.getConstructors())
.filter(constructor -> constructor.getParameterTypes().length == paramCount)
.findFirst()
.orElse(null);
}

private List<Method> findMethods(Class<?> clazz, String methodName) {
return Arrays.stream(clazz.getMethods())
.filter(method -> method.getName().equals(methodName))
.collect(Collectors.toList());
}
}
2 changes: 2 additions & 0 deletions rhino/src/main/java/module-info.java
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
module org.mozilla.rhino {
uses org.mozilla.javascript.NullabilityDetector;

exports org.mozilla.classfile;
exports org.mozilla.javascript;
exports org.mozilla.javascript.annotations;
Expand Down
3 changes: 2 additions & 1 deletion rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,9 @@ public boolean setValue(Object value, Scriptable owner, Scriptable start) {
// XXX: cache tag since it is already calculated in
// defineProperty ?
Class<?> valueType = pTypes[pTypes.length - 1];
boolean isNullable = member.argNullability[pTypes.length - 1];
int tag = FunctionObject.getTypeTag(valueType);
Object actualArg = FunctionObject.convertArg(cx, start, value, tag);
Object actualArg = FunctionObject.convertArg(cx, start, value, tag, isNullable);

if (member.delegateTo == null) {
member.invoke(start, new Object[] {actualArg});
Expand Down
Loading

4 comments on commit 07492b5

@rbri
Copy link
Collaborator

@rbri rbri commented on 07492b5 Feb 4, 2025

Choose a reason for hiding this comment

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

Looks like this commit drops the overall performance by a factor of 10 or more !!!!
At least the HtmlUnit test suite needs ages after this.

Will try to figure out what is going on but maybe the others (@gbrail) can try at least the benchmark test and prove my results.

@rbri
Copy link
Collaborator

@rbri rbri commented on 07492b5 Feb 4, 2025

Choose a reason for hiding this comment

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

As a first guess, loading the NullabilityDetector as service for each method might be not that optimal

Image

@gbrail
Copy link
Collaborator

@gbrail gbrail commented on 07492b5 Feb 4, 2025 via email

Choose a reason for hiding this comment

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

@rbri
Copy link
Collaborator

@rbri rbri commented on 07492b5 Feb 4, 2025

Choose a reason for hiding this comment

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

I usually obsess about performance

I know that :-) maybe that was the motivation of my comment.... ;-)

Please sign in to comment.