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

FEAT: Implement Symbol.hasInstance for Function.prototype #1751

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
69 changes: 66 additions & 3 deletions rhino/src/main/java/org/mozilla/javascript/BaseFunction.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ static void init(Context cx, Scriptable scope, boolean sealed) {
if (cx.getLanguageVersion() >= Context.VERSION_ES6) {
obj.setStandardPropertyAttributes(READONLY | DONTENUM);
}
obj.exportAsJSClass(MAX_PROTOTYPE_ID, scope, sealed);
IdFunctionObject constructor = obj.exportAsJSClass(MAX_PROTOTYPE_ID, scope, sealed);
if (cx.getLanguageVersion() >= Context.VERSION_ES6) {
ScriptRuntimeES6.addSymbolHasInstance(cx, scope, constructor);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to ask why you don't just call "defineProperty" here, but actually I'm wondering why we need this at all -- adding the new symbol case in the other places that you added it should be enough for this to work.

}
}

/**
Expand Down Expand Up @@ -80,7 +83,7 @@ protected boolean hasDefaultParameters() {
/**
* Gets the value returned by calling the typeof operator on this object.
*
* @see org.mozilla.javascript.ScriptableObject#getTypeOf()
* @see ScriptableObject#getTypeOf()
* @return "function" or "undefined" if {@link #avoidObjectDetection()} returns <code>true
* </code>
*/
Expand Down Expand Up @@ -156,6 +159,8 @@ protected int findInstanceIdInfo(String s) {
@Override
protected String getInstanceIdName(int id) {
switch (id) {
case SymbolId_hasInstance:
return "SymbolId_hasInstance";
case Id_length:
return "length";
case Id_arity:
Expand Down Expand Up @@ -265,6 +270,11 @@ protected void fillConstructorProperties(IdFunctionObject ctor) {

@Override
protected void initPrototypeId(int id) {
if (id == SymbolId_hasInstance) {
initPrototypeValue(id, SymbolKey.HAS_INSTANCE, makeHasInstance(), CONST | DONTENUM);
return;
}

String s;
int arity;
switch (id) {
Expand Down Expand Up @@ -313,6 +323,52 @@ 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 =
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's great to use a Lambda function here, but since this class still inherits from "IdScriptableObject", you should be able to make it work by adding a case to the switch in the "exec" function for your new "SymbolId_hasInstance" case.

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 @@ -627,6 +683,12 @@ private Object jsConstructor(Context cx, Scriptable scope, Object[] args) {
return cx.compileFunction(global, source, evaluator, reporter, sourceURI, 1, null);
}

@Override
protected int findPrototypeId(Symbol k) {
if (SymbolKey.HAS_INSTANCE.equals(k)) return SymbolId_hasInstance;
return 0;
}

@Override
protected int findPrototypeId(String s) {
int id;
Expand Down Expand Up @@ -670,7 +732,8 @@ public Scriptable getHomeObject() {
Id_apply = 4,
Id_call = 5,
Id_bind = 6,
MAX_PROTOTYPE_ID = Id_bind;
SymbolId_hasInstance = 7,
MAX_PROTOTYPE_ID = SymbolId_hasInstance;

private Object prototypeProperty;
private Object argumentsObj = NOT_FOUND;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,13 @@ final void delete(int id) {
Context cx = Context.getContext();
if (cx.isStrictMode()) {
int nameSlot = (id - 1) * SLOT_SPAN + NAME_SLOT;
String name = (String) valueArray[nameSlot];

String name = null;
if (valueArray[nameSlot] instanceof String)
name = (String) valueArray[nameSlot];
else if (valueArray[nameSlot] instanceof Symbol) {
name = valueArray[nameSlot].toString();
}
throw ScriptRuntime.typeErrorById(
"msg.delete.property.with.configurable.false", name);
}
Expand Down
2 changes: 1 addition & 1 deletion rhino/src/main/java/org/mozilla/javascript/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -1181,7 +1181,7 @@ private void toString(Map<Node, Integer> printIds, StringBuilder sb) {
Object[] a = (Object[]) x.objectValue;
sb.append("[");
for (int i = 0; i < a.length; i++) {
sb.append(a[i].toString());
if (a[i] != null) sb.append(a[i].toString());
if (i + 1 < a.length) sb.append(", ");
}
sb.append("]");
Expand Down
11 changes: 11 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/ScriptRuntimeES6.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,15 @@ 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(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get what you're trying to do here, but I don't think that we should add a new function to a "ScriptRuntime" class that we're only going to use in one place. I'd suggest that instead of all this, you just call "defineProperty" on the "constructor" object back in BaseFunction and pass it the bitset (I'll put a comment there too).

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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,12 @@ public boolean hasInstance(Scriptable instance) {
// chasing. This will be overridden in NativeFunction and non-JS
// objects.

Context cx = Context.getCurrentContext();
Object hasInstance = ScriptRuntime.getObjectElem(this, SymbolKey.HAS_INSTANCE, cx);
if (hasInstance instanceof Callable) {
return (boolean)
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 not sure that I know all the semantics of just casting to boolean here, but if I know anything about JavaScript, it's 90% edge cases.

So, since the function here could literally return anything, you should use "ScriptRuntime.toBoolean" here, which can cast any number of objects correctly to a "boolean".

((Callable) hasInstance).call(cx, getParentScope(), this, new Object[] {this});
}
return ScriptRuntime.jsDelegatesTo(instance, this);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
package org.mozilla.javascript;

import org.junit.Assert;
import org.junit.Ignore;
import org.junit.Test;
import org.mozilla.javascript.tests.Utils;

public class FunctionPrototypeSymbolHasInstanceTest {
@Test
public void testSymbolHasInstanceIsPresent() {
String script =
""
+ "var f = {\n"
+ " [Symbol.hasInstance](value) { "
+ " }"
+ "};\n"
+ "var g = {};\n"
+ "`${f.hasOwnProperty(Symbol.hasInstance)}:${g.hasOwnProperty(Symbol.hasInstance)}`";
Utils.assertWithAllOptimizationLevelsES6("true:false", script);
}

@Test
public void testSymbolHasInstanceCanBeCalledLikeAnotherMethod() {
String script =
""
+ "var f = {\n"
+ " [Symbol.hasInstance](value) { "
+ " return 42;"
+ " }"
+ "};\n"
+ "f[Symbol.hasInstance]() == 42";
Utils.assertWithAllOptimizationLevelsES6(true, script);
}

// See: https://tc39.es/ecma262/#sec-function.prototype-%symbol.hasinstance%
@Test
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);
}

// See: https://tc39.es/ecma262/#sec-function.prototype-%symbol.hasinstance%
@Test
public void testFunctionPrototypeSymbolHasInstanceHasAttributesStrictMode() {
String script =
"'use strict';\n"
+ "var t = typeof Function.prototype[Symbol.hasInstance];\n"
+ "var a = Object.getOwnPropertyDescriptor(Function.prototype, Symbol.hasInstance);\n"
+ "var typeErrorThrown = false;\n"
+ "try { \n"
+ " delete Function.prototype[Symbol.hasInstance] \n"
+ "} catch (e) { \n"
+ " 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);
}

@Test
@Ignore("name-length-params-prototype-set-incorrectly")
public void testFunctionPrototypeSymbolHasInstanceHasProperties() {
String script =
"var a = Object.getOwnPropertyDescriptor(Function.prototype[Symbol.hasInstance], 'length');\n"
+ "a.value + ':' + a.writable + ':' + a.configurable + ':' + a.enumerable";

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

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

@Test
public void testFunctionPrototypeSymbolHasInstanceOnObjectReturnsTrue() {
String script =
"var f = function() {};\n"
+ "var o = new f();\n"
+ "var o2 = Object.create(o);\n"
+ "(f[Symbol.hasInstance](o)) + ':' + "
+ "(f[Symbol.hasInstance](o2));\n";
Utils.assertWithAllOptimizationLevelsES6("true:true", script);
}

@Test
public void testFunctionPrototypeSymbolHasInstanceOnBoundTargetReturnsTrue() {
String script =
"var BC = function() {};\n"
+ "var bc = new BC();\n"
+ "var bound = BC.bind();\n"
+ "bound[Symbol.hasInstance](bc);\n";
Utils.assertWithAllOptimizationLevelsES6(true, script);
}

@Test
public void testFunctionInstanceNullVoidEtc() {
String script =
"var f = function() {};\n"
+ "var x;\n"
+ "a = (undefined instanceof f) + ':' +\n"
+ "(x instanceof f) + ':' +\n"
+ "(null instanceof f) + ':' +\n"
+ "(void 0 instanceof f)\n"
+ "a";
Utils.assertWithAllOptimizationLevelsES6("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);
}

@Test
public void testSymbolHasInstanceIsInvokedInInstanceOf() {
String script =
""
+ "var globalSet = 0;"
+ "var f = {\n"
+ " [Symbol.hasInstance](value) { "
+ " globalSet = 1;"
+ " return true;"
+ " }"
+ "}\n"
+ "var g = {}\n"
+ "Object.setPrototypeOf(g, f);\n"
+ "g instanceof f;"
+ "globalSet == 1";
Utils.assertWithAllOptimizationLevelsES6(true, script);
}

@Test
public void testThrowTypeErrorOnNonObjectIncludingSymbol() {
String script =
""
+ "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;
});
}
}
9 changes: 1 addition & 8 deletions tests/testsrc/test262.properties
Original file line number Diff line number Diff line change
Expand Up @@ -721,14 +721,7 @@ built-ins/Function 184/508 (36.22%)
prototype/call/S15.3.4.4_A6_T7.js non-interpreted
prototype/Symbol.hasInstance/length.js
prototype/Symbol.hasInstance/name.js
prototype/Symbol.hasInstance/prop-desc.js
prototype/Symbol.hasInstance/this-val-bound-target.js
prototype/Symbol.hasInstance/this-val-not-callable.js
prototype/Symbol.hasInstance/this-val-poisoned-prototype.js
prototype/Symbol.hasInstance/value-get-prototype-of-err.js
prototype/Symbol.hasInstance/value-negative.js
prototype/Symbol.hasInstance/value-non-obj.js
prototype/Symbol.hasInstance/value-positive.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
Loading