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

0xe · 2024-12-09T08:37:24Z

Adds support for Function.prototype[Symbol.hasInstance].

rhino/src/main/java/org/mozilla/javascript/BaseFunction.java

rhino/src/test/java/org/mozilla/javascript/FunctionPrototypeSymbolHasInstanceTest.java

rhino/src/main/java/org/mozilla/javascript/BaseFunction.java

rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java

rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java

rhino/src/main/java/org/mozilla/javascript/BaseFunction.java

rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java

rbri · 2024-12-12T08:38:18Z

@0xe thanks for taking care of my comments

gbrail

Thanks for working on this!

Unless I'm missing something (and in JavaScript I'm usually missing something) there is an existing pattern to add a new symbol-keyed property to a native object -- see the implementation of "SymbolId_Iterator" in NativeString, for instance. This change seems to be doing some of that and some of something else. Can you please try to do it that way? Thanks!

gbrail · 2024-12-13T00:34:30Z

rhino/src/main/java/org/mozilla/javascript/ScriptRuntimeES6.java

@@ -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(


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).

You were right, those were unnecessary. I've removed them.

gbrail · 2024-12-13T00:36:07Z

rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java

+        Context cx = Context.getCurrentContext();
+        Object hasInstance = ScriptRuntime.getObjectElem(this, SymbolKey.HAS_INSTANCE, cx);
+        if (hasInstance instanceof Callable) {
+            return (boolean)


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".

gbrail · 2024-12-13T00:40:00Z

rhino/src/main/java/org/mozilla/javascript/BaseFunction.java

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


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.

You were right, those were unnecessary. I've removed them.

gbrail · 2024-12-13T00:41:05Z

rhino/src/main/java/org/mozilla/javascript/BaseFunction.java

+
+        if (cx != null) {
+            Scriptable scope = this.getParentScope();
+            obj =


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.

Yep, this is way simpler. Changed it to switch on execIdCall.

0xe · 2024-12-13T10:46:32Z

Thanks for working on this!

Unless I'm missing something (and in JavaScript I'm usually missing something) there is an existing pattern to add a new symbol-keyed property to a native object -- see the implementation of "SymbolId_Iterator" in NativeString, for instance. This change seems to be doing some of that and some of something else. Can you please try to do it that way? Thanks!

Thank you. I will try to address these in the coming days.

rbri · 2025-01-02T07:28:25Z

rhino/src/test/java/org/mozilla/javascript/FunctionPrototypeSymbolHasInstanceTest.java

+
+        Utils.runWithAllOptimizationLevels(
+                (cx) -> {
+                    cx.setLanguageVersion(Context.VERSION_ES6);


there are now some assertThrows.... methods in the Utils, maybe you can use one of these

Thanks! I've used Utils.assertEcmaErrorES6()

- Remove unnecessary code - Simplify implementation to use execIdCall as BaseFunction inherits from IdScriptableObject - Use Utils.assertEcmaErrorES6

0xe changed the title ~~FEAT: Implement Symbol.hasInstance~~ FEAT: Implement Symbol.hasInstance for Function.prototype Dec 9, 2024