-
Notifications
You must be signed in to change notification settings - Fork 858
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -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> | ||
*/ | ||
|
@@ -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: | ||
|
@@ -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) { | ||
|
@@ -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 = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
||
|
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; | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
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.