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

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

No description provided.

rbri · 2024-12-09T09:23:11Z

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

@@ -31,7 +31,8 @@ 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);
+        ScriptRuntimeES6.addSymbolHasInstance(cx, scope, constructor);


do we need a language version check here (cx.getLanguageVersion() >= Context.VERSION_ES6)

rbri · 2024-12-09T09:40:14Z

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

+                        + "};\n"
+                        + "var g = {};\n"
+                        + "`${f.hasOwnProperty(Symbol.hasInstance)}:${g.hasOwnProperty(Symbol.hasInstance)}`";
+        Utils.runWithAllOptimizationLevels(


maybe you can use assertWithAllOptimizationLevelsES6 here

rbri · 2024-12-09T09:41:50Z

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

@@ -265,6 +268,11 @@ protected void fillConstructorProperties(IdFunctionObject ctor) {

    @Override
    protected void initPrototypeId(int id) {
+        if (id == SymbolId_hasInstance) {
+            initPrototypeValue(id, SymbolKey.HAS_INSTANCE, makeHasInstance(), 0x0F);


please use the real attributes instead of 0x0F for readability

rbri · 2024-12-09T09:42:22Z

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

+            int instanceIdInfo, Scriptable scope, Object value, int attr) {
+        if (instanceIdInfo == SymbolId_hasInstance) {
+            return buildDataDescriptor(scope, value, attr, SymbolKey.HAS_INSTANCE.toString(), 1);
+        } else {


else not requred if you return before

rbri · 2024-12-09T09:42:50Z

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

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


else not required

rbri · 2024-12-09T09:44:48Z

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

+     *
+     * @return ScriptableObject
+     */
+    ScriptableObject buildDataDescriptorHelper(


please add @OverRide... same for the next and maybe you can also copy the comment

Removed these as they don't actually solve the problem with name and length attributes.

rbri · 2024-12-09T09:48:53Z

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

        ScriptableObject desc = new NativeObject();
        ScriptRuntime.setBuiltinProtoAndParent(desc, scope, TopLevel.Builtins.Object);
        desc.defineProperty("value", value, EMPTY);
-        desc.setCommonDescriptorProperties(attributes, true);
+        desc.setCommonDescriptorProperties(attributes, true, name, length);


is it a good idea to introduce this new function with two (at least for me magic parameters) including all the forwarder for the old calls if we need the additional functionality only at one place?

Removed these as they don't actually solve the problem with name and length attributes.

rbri · 2024-12-09T09:50:26Z

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

+                                        }
+                                        throw ScriptRuntime.typeErrorById(
+                                                "msg.instanceof.bad.prototype", getFunctionName());
+                                    } else {


else not needed because of the throw before

andreabergia · 2024-12-11T16:04:19Z

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

+                    String name = null;
+                    if (valueArray[nameSlot] instanceof String)
+                        name = (String) valueArray[nameSlot];
+                    else if (valueArray[nameSlot] instanceof SymbolKey) {


Should be Symbol and not SymbolKey I think

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

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.

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.

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.

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

rbri reviewed Dec 9, 2024

View reviewed changes

andreabergia reviewed Dec 11, 2024

View reviewed changes

0xe requested review from rbri and andreabergia December 12, 2024 01:24

0xe force-pushed the scratch/satish/function-prototype-hasinstance branch 2 times, most recently from 732d044 to 5aa5f4a Compare December 12, 2024 01:38

0xe marked this pull request as ready for review December 12, 2024 01:40

0xe force-pushed the scratch/satish/function-prototype-hasinstance branch 2 times, most recently from 013b02b to 680b2a2 Compare December 12, 2024 05:13

gbrail requested changes Dec 13, 2024

View reviewed changes

FEAT: Implement Symbol.hasInstance

5e7ebb0

0xe force-pushed the scratch/satish/function-prototype-hasinstance branch from 680b2a2 to 5e7ebb0 Compare December 18, 2024 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

0xe commented Dec 9, 2024

rbri Dec 9, 2024

rbri Dec 9, 2024

rbri Dec 9, 2024

rbri Dec 9, 2024

rbri Dec 9, 2024

rbri Dec 9, 2024 •

edited

Loading

0xe Dec 12, 2024

rbri Dec 9, 2024

0xe Dec 12, 2024

rbri Dec 9, 2024

andreabergia Dec 11, 2024

rbri commented Dec 12, 2024

gbrail left a comment

gbrail Dec 13, 2024

gbrail Dec 13, 2024

gbrail Dec 13, 2024

gbrail Dec 13, 2024

0xe commented Dec 13, 2024

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

Are you sure you want to change the base?

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

Conversation

0xe commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbri Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbri commented Dec 12, 2024

gbrail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xe commented Dec 13, 2024

rbri Dec 9, 2024 •

edited

Loading