Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history 
- Remove unnecessary code
- Simplify implementation to use execIdCall as BaseFunction inherits from IdScriptableObject
- Use Utils.assertEcmaErrorES6
  • Loading branch information
@0xe
0xe committed Jan 13, 2025
1 parent 4cea728 commit 836151a
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 101 deletions.
76 changes: 25 additions & 51 deletions rhino/src/main/java/org/mozilla/javascript/BaseFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,7 @@ static void init(Context cx, Scriptable scope, boolean sealed) {
if (cx.getLanguageVersion() >= Context.VERSION_ES6) {
obj.setStandardPropertyAttributes(READONLY | DONTENUM);
}
IdFunctionObject constructor = obj.exportAsJSClass(MAX_PROTOTYPE_ID, scope, sealed);
if (cx.getLanguageVersion() >= Context.VERSION_ES6) {
ScriptRuntimeES6.addSymbolHasInstance(cx, scope, constructor);
}
obj.exportAsJSClass(MAX_PROTOTYPE_ID, scope, sealed);
}

/**
Expand Down Expand Up @@ -271,7 +268,13 @@ protected void fillConstructorProperties(IdFunctionObject ctor) {
@Override
protected void initPrototypeId(int id) {
if (id == SymbolId_hasInstance) {
initPrototypeValue(id, SymbolKey.HAS_INSTANCE, makeHasInstance(), CONST | DONTENUM);
initPrototypeMethod(
FUNCTION_TAG,
id,
SymbolKey.HAS_INSTANCE,
String.valueOf(SymbolKey.HAS_INSTANCE),
1,
CONST | DONTENUM);
return;
}

Expand Down Expand Up @@ -323,52 +326,6 @@ static boolean isApplyOrCall(IdFunctionObject f) {
return false;
}

private Object makeHasInstance() {
Context cx = Context.getCurrentContext();
ScriptableObject obj = null;

if (cx != null) {
Scriptable scope = this.getParentScope();
obj =
new LambdaFunction(
scope,
0,
new Callable() {
@Override
public Object call(
Context cx,
Scriptable scope,
Scriptable thisObj,
Object[] args) {
if (thisObj != null
&& args.length == 1
&& args[0] instanceof Scriptable) {
Scriptable obj = (Scriptable) args[0];
Object protoProp = null;
if (thisObj instanceof BoundFunction)
protoProp =
((NativeFunction)
((BoundFunction) thisObj)
.getTargetFunction())
.getPrototypeProperty();
else
protoProp =
ScriptableObject.getProperty(
thisObj, "prototype");
if (protoProp instanceof IdScriptableObject) {
return ScriptRuntime.jsDelegatesTo(
obj, (Scriptable) protoProp);
}
throw ScriptRuntime.typeErrorById(
"msg.instanceof.bad.prototype", getFunctionName());
}
return false; // NOT_FOUND, null etc.
}
});
}
return obj;
}

@Override
public Object execIdCall(
IdFunctionObject f, Context cx, Scriptable scope, Scriptable thisObj, Object[] args) {
Expand Down Expand Up @@ -424,6 +381,23 @@ public Object execIdCall(
boundArgs = ScriptRuntime.emptyArgs;
}
return new BoundFunction(cx, scope, targetFunction, boundThis, boundArgs);

case SymbolId_hasInstance:
if (thisObj != null && args.length == 1 && args[0] instanceof Scriptable) {
Scriptable obj = (Scriptable) args[0];
Object protoProp = null;
if (thisObj instanceof BoundFunction)
protoProp =
((NativeFunction) ((BoundFunction) thisObj).getTargetFunction())
.getPrototypeProperty();
else protoProp = ScriptableObject.getProperty(thisObj, "prototype");
if (protoProp instanceof IdScriptableObject) {
return ScriptRuntime.jsDelegatesTo(obj, (Scriptable) protoProp);
}
throw ScriptRuntime.typeErrorById(
"msg.instanceof.bad.prototype", getFunctionName());
}
return false; // NOT_FOUND, null etc.
}
throw new IllegalArgumentException(String.valueOf(id));
}
Expand Down
8 changes: 8 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,14 @@ public final IdFunctionObject initPrototypeMethod(
return function;
}

public final IdFunctionObject initPrototypeMethod(
Object tag, int id, Symbol key, String functionName, int arity, int attributes) {
Scriptable scope = ScriptableObject.getTopLevelScope(this);
IdFunctionObject function = newIdFunction(tag, id, functionName, arity, scope);
prototypeValues.initValue(id, key, function, attributes);
return function;
}

public final void initPrototypeConstructor(IdFunctionObject f) {
int id = prototypeValues.constructorId;
if (id == 0) throw new IllegalStateException();
Expand Down
11 changes: 0 additions & 11 deletions rhino/src/main/java/org/mozilla/javascript/ScriptRuntimeES6.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,4 @@ public static void addSymbolUnscopables(
ScriptableObject.putProperty(unScopablesDescriptor, "writable", false);
constructor.defineOwnProperty(cx, SymbolKey.UNSCOPABLES, unScopablesDescriptor, false);
}

/** Registers the symbol <code>[Symbol.hasInstance]</code> on the given constructor function. */
public static void addSymbolHasInstance(
Context cx, Scriptable scope, IdScriptableObject constructor) {
ScriptableObject hasInstanceDescriptor = (ScriptableObject) cx.newObject(scope);
ScriptableObject.putProperty(hasInstanceDescriptor, "value", ScriptableObject.EMPTY);
ScriptableObject.putProperty(hasInstanceDescriptor, "enumerable", false);
ScriptableObject.putProperty(hasInstanceDescriptor, "configurable", false);
ScriptableObject.putProperty(hasInstanceDescriptor, "writable", false);
constructor.defineOwnProperty(cx, SymbolKey.HAS_INSTANCE, hasInstanceDescriptor, false);
}
}
4 changes: 2 additions & 2 deletions rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,8 +867,8 @@ public boolean hasInstance(Scriptable instance) {
Context cx = Context.getCurrentContext();
Object hasInstance = ScriptRuntime.getObjectElem(this, SymbolKey.HAS_INSTANCE, cx);
if (hasInstance instanceof Callable) {
return (boolean)
((Callable) hasInstance).call(cx, getParentScope(), this, new Object[] {this});
return ScriptRuntime.toBoolean(
((Callable) hasInstance).call(cx, getParentScope(), this, new Object[] {this}));
}
return ScriptRuntime.jsDelegatesTo(instance, this);
}
Expand Down
48 changes: 14 additions & 34 deletions rhino/src/test/java/org/mozilla/javascript/FunctionPrototypeSymbolHasInstanceTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.mozilla.javascript;

import org.junit.Assert;
import org.junit.Ignore;
import org.junit.Test;
import org.mozilla.javascript.tests.Utils;
Expand All @@ -16,7 +15,7 @@ public void testSymbolHasInstanceIsPresent() {
+ "};\n"
+ "var g = {};\n"
+ "`${f.hasOwnProperty(Symbol.hasInstance)}:${g.hasOwnProperty(Symbol.hasInstance)}`";
Utils.assertWithAllOptimizationLevelsES6("true:false", script);
Utils.assertWithAllModes("true:false", script);
}

@Test
Expand All @@ -29,7 +28,7 @@ public void testSymbolHasInstanceCanBeCalledLikeAnotherMethod() {
+ " }"
+ "};\n"
+ "f[Symbol.hasInstance]() == 42";
Utils.assertWithAllOptimizationLevelsES6(true, script);
Utils.assertWithAllModes(true, script);
}

// See: https://tc39.es/ecma262/#sec-function.prototype-%symbol.hasinstance%
Expand All @@ -38,7 +37,7 @@ public void testFunctionPrototypeSymbolHasInstanceHasAttributes() {
String script =
"var a = Object.getOwnPropertyDescriptor(Function.prototype, Symbol.hasInstance);\n"
+ "a.writable + ':' + a.configurable + ':' + a.enumerable";
Utils.assertWithAllOptimizationLevelsES6("false:false:false", script);
Utils.assertWithAllModes("false:false:false", script);
}

// See: https://tc39.es/ecma262/#sec-function.prototype-%symbol.hasinstance%
Expand All @@ -55,7 +54,7 @@ public void testFunctionPrototypeSymbolHasInstanceHasAttributesStrictMode() {
+ " typeErrorThrown = true \n"
+ "}\n"
+ "Object.prototype.hasOwnProperty.call(Function.prototype, Symbol.hasInstance) + ':' + typeErrorThrown + ':' + t + ':' + a.writable + ':' + a.configurable + ':' + a.enumerable; \n";
Utils.assertWithAllOptimizationLevelsES6("true:true:function:false:false:false", script);
Utils.assertWithAllModes("true:true:function:false:false:false", script);
}

@Test
Expand All @@ -68,17 +67,16 @@ public void testFunctionPrototypeSymbolHasInstanceHasProperties() {
String script2 =
"var a = Object.getOwnPropertyDescriptor(Function.prototype[Symbol.hasInstance], 'name');\n"
+ "a.value + ':' + a.writable + ':' + a.configurable + ':' + a.enumerable";
Utils.assertWithAllOptimizationLevelsES6("1:false:true:false", script);
Utils.assertWithAllOptimizationLevelsES6(
"Symbol(Symbol.hasInstance):false:true:false", script2);
Utils.assertWithAllModes("1:false:true:false", script);
Utils.assertWithAllModes("Symbol(Symbol.hasInstance):false:true:false", script2);
}

@Test
public void testFunctionPrototypeSymbolHasInstance() {
String script =
"(Function.prototype[Symbol.hasInstance] instanceof Function) + ':' + "
+ "Function.prototype[Symbol.hasInstance].call(Function, Object)\n";
Utils.assertWithAllOptimizationLevelsES6("true:true", script);
Utils.assertWithAllModes("true:true", script);
}

@Test
Expand All @@ -89,7 +87,7 @@ public void testFunctionPrototypeSymbolHasInstanceOnObjectReturnsTrue() {
+ "var o2 = Object.create(o);\n"
+ "(f[Symbol.hasInstance](o)) + ':' + "
+ "(f[Symbol.hasInstance](o2));\n";
Utils.assertWithAllOptimizationLevelsES6("true:true", script);
Utils.assertWithAllModes("true:true", script);
}

@Test
Expand All @@ -99,7 +97,7 @@ public void testFunctionPrototypeSymbolHasInstanceOnBoundTargetReturnsTrue() {
+ "var bc = new BC();\n"
+ "var bound = BC.bind();\n"
+ "bound[Symbol.hasInstance](bc);\n";
Utils.assertWithAllOptimizationLevelsES6(true, script);
Utils.assertWithAllModes(true, script);
}

@Test
Expand All @@ -112,15 +110,15 @@ public void testFunctionInstanceNullVoidEtc() {
+ "(null instanceof f) + ':' +\n"
+ "(void 0 instanceof f)\n"
+ "a";
Utils.assertWithAllOptimizationLevelsES6("false:false:false:false", script);
Utils.assertWithAllModes("false:false:false:false", script);
}

@Test
public void testFunctionPrototypeSymbolHasInstanceReturnsFalseOnUndefinedOrProtoypeNotFound() {
String script =
"Function.prototype[Symbol.hasInstance].call() + ':' +"
+ "Function.prototype[Symbol.hasInstance].call({});";
Utils.assertWithAllOptimizationLevelsES6("false:false", script);
Utils.assertWithAllModes("false:false", script);
}

@Test
Expand All @@ -138,7 +136,7 @@ public void testSymbolHasInstanceIsInvokedInInstanceOf() {
+ "Object.setPrototypeOf(g, f);\n"
+ "g instanceof f;"
+ "globalSet == 1";
Utils.assertWithAllOptimizationLevelsES6(true, script);
Utils.assertWithAllModes(true, script);
}

@Test
Expand All @@ -148,25 +146,7 @@ public void testThrowTypeErrorOnNonObjectIncludingSymbol() {
+ "var f = function() {}; \n"
+ "f.prototype = Symbol(); \n"
+ "f[Symbol.hasInstance]({})";

Utils.runWithAllOptimizationLevels(
(cx) -> {
cx.setLanguageVersion(Context.VERSION_ES6);
final Scriptable scope = cx.initStandardObjects();
var error =
Assert.assertThrows(
EcmaError.class,
() ->
cx.evaluateString(
scope,
script,
"testSymbolHasInstance",
0,
null));
Assert.assertTrue(
error.toString()
.contains("'prototype' property of is not an object."));
return null;
});
Utils.assertEcmaErrorES6(
"TypeError: 'prototype' property of is not an object. (test#3)", script);
}
}
4 changes: 1 addition & 3 deletions tests/testsrc/test262.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ built-ins/Error 5/41 (12.2%)

~built-ins/FinalizationRegistry

built-ins/Function 184/508 (36.22%)
built-ins/Function 175/508 (34.45%)
internals/Call 2/2 (100.0%)
internals/Construct 6/6 (100.0%)
length/S15.3.5.1_A1_T3.js strict
Expand Down Expand Up @@ -719,9 +719,7 @@ built-ins/Function 184/508 (36.22%)
prototype/call/S15.3.4.4_A6_T2.js compiled
prototype/call/S15.3.4.4_A6_T5.js compiled
prototype/call/S15.3.4.4_A6_T7.js compiled
prototype/Symbol.hasInstance/length.js
prototype/Symbol.hasInstance/name.js
prototype/Symbol.hasInstance/value-get-prototype-of-err.js {unsupported: [Proxy]}
prototype/toString/async-arrow-function.js {unsupported: [async-functions]}
prototype/toString/async-function-declaration.js {unsupported: [async-functions]}
prototype/toString/async-function-expression.js {unsupported: [async-functions]}
Expand Down

0 comments on commit 836151a

Please sign in to comment.