LibJS: Ensure we only call toString on computed properties once

Committer: https://github.com/linusg
Commit: https://github.com/SerenityOS/serenity/commit/a97b5393d0b

Userland/Libraries/LibJS/AST.cpp

@@ -81,13 +81,11 @@ static void update_function_name(Value value, FlyString const& name)
         static_cast<ECMAScriptFunctionObject&>(function).set_name(name);
 }
 
-static ThrowCompletionOr<String> get_function_name(GlobalObject& global_object, Value value)
+static ThrowCompletionOr<String> get_function_property_name(PropertyKey key)
 {
-    if (value.is_symbol())
-        return String::formatted("[{}]", value.as_symbol().description());
-    if (value.is_string())
-        return value.as_string().string();
-    return value.to_string(global_object);
+    if (key.is_symbol())
+        return String::formatted("[{}]", key.as_symbol()->description());
+    return key.to_string();
 }
 
 // 14.2.2 Runtime Semantics: Evaluation, https://tc39.es/ecma262/#sec-block-runtime-semantics-evaluation
@@ -2999,7 +2997,8 @@ Completion ObjectExpression::execute(Interpreter& interpreter, GlobalObject& glo
         if (value.is_function() && property.is_method())
             static_cast<ECMAScriptFunctionObject&>(value.as_function()).set_home_object(object);
 
-        auto name = TRY(get_function_name(global_object, key));
+        auto property_key = TRY(PropertyKey::from_value(global_object, key));
+        auto name = TRY(get_function_property_name(property_key));
         if (property.type() == ObjectProperty::Type::Getter) {
             name = String::formatted("get {}", name);
         } else if (property.type() == ObjectProperty::Type::Setter) {
@@ -3011,14 +3010,14 @@ Completion ObjectExpression::execute(Interpreter& interpreter, GlobalObject& glo
         switch (property.type()) {
         case ObjectProperty::Type::Getter:
             VERIFY(value.is_function());
-            object->define_direct_accessor(TRY(PropertyKey::from_value(global_object, key)), &value.as_function(), nullptr, Attribute::Configurable | Attribute::Enumerable);
+            object->define_direct_accessor(property_key, &value.as_function(), nullptr, Attribute::Configurable | Attribute::Enumerable);
             break;
         case ObjectProperty::Type::Setter:
             VERIFY(value.is_function());
-            object->define_direct_accessor(TRY(PropertyKey::from_value(global_object, key)), nullptr, &value.as_function(), Attribute::Configurable | Attribute::Enumerable);
+            object->define_direct_accessor(property_key, nullptr, &value.as_function(), Attribute::Configurable | Attribute::Enumerable);
             break;
         case ObjectProperty::Type::KeyValue:
-            object->define_direct_property(TRY(PropertyKey::from_value(global_object, key)), value, JS::default_attributes);
+            object->define_direct_property(property_key, value, JS::default_attributes);
             break;
         case ObjectProperty::Type::Spread:
         default:

Userland/Libraries/LibJS/Tests/computed-property-sideeffects.js

@@ -0,0 +1,145 @@
+let calledToStringError = {};
+let throwingToString = {
+    toString: () => {
+        throw calledToStringError;
+    },
+};
+let calledValueOfError = {};
+let throwingValueOf = {
+    toString: undefined,
+    valueOf: () => {
+        throw calledValueOfError;
+    },
+};
+let calledToStringAccessorError = {};
+let throwingToStringAccessor = {
+    get toString() {
+        throw calledToStringAccessorError;
+    },
+};
+let calledValueOfAccessorError = {};
+let throwingValueOfAccessor = {
+    toString: undefined,
+    get valueOf() {
+        throw calledValueOfAccessorError;
+    },
+};
+
+test("Exceptions thrown by computed properties are caught", () => {
+    var i = 0;
+    var j = 0;
+    var k = 0;
+    expect(() => {
+        return { first: k++, [throwingToString]: i++, second: j++ };
+    }).toThrow(calledToStringError);
+    expect(i).toBe(1);
+    expect(j).toBe(0);
+    expect(k).toBe(1);
+    expect(() => {
+        return { first: k++, [throwingValueOf]: i++, second: j++ };
+    }).toThrow(calledValueOfError);
+    expect(i).toBe(2);
+    expect(j).toBe(0);
+    expect(k).toBe(2);
+    expect(() => {
+        return { first: k++, [throwingToStringAccessor]: i++, second: j++ };
+    }).toThrow(calledToStringAccessorError);
+    expect(i).toBe(3);
+    expect(j).toBe(0);
+    expect(k).toBe(3);
+    expect(() => {
+        return { first: k++, [throwingValueOfAccessor]: i++, second: j++ };
+    }).toThrow(calledValueOfAccessorError);
+    expect(i).toBe(4);
+    expect(j).toBe(0);
+    expect(k).toBe(4);
+});
+
+test("Test toString and valueOf are only called once", () => {
+    var counter = 0;
+    var key1 = {
+        valueOf: function () {
+            expect(counter++).toBe(0);
+            return "a";
+        },
+        toString: null,
+    };
+    var key2 = {
+        valueOf: function () {
+            expect(counter++).toBe(1);
+            return "b";
+        },
+        toString: null,
+    };
+    var key3 = {
+        get toString() {
+            expect(counter++).toBe(2);
+            return function () {
+                return "c";
+            };
+        },
+    };
+    var key4 = {
+        get valueOf() {
+            expect(counter++).toBe(3);
+            return function () {
+                return "d";
+            };
+        },
+        toString: null,
+    };
+    var key5 = {
+        valueOf: function () {
+            expect(counter++).toBe(4);
+            return 0;
+        },
+        toString: null,
+    };
+    var key6 = {
+        valueOf: function () {
+            expect(counter++).toBe(5);
+            return 2;
+        },
+        toString: null,
+    };
+    var key7 = {
+        get toString() {
+            expect(counter++).toBe(6);
+            return function () {
+                return 4;
+            };
+        },
+    };
+    var key8 = {
+        get valueOf() {
+            expect(counter++).toBe(7);
+            return function () {
+                return 8;
+            };
+        },
+        toString: null,
+    };
+
+    var object = {
+        [key1]: "a",
+        [key2]: "b",
+        [key3]: "c",
+        [key4]: "d",
+        [key5]: "e",
+        [key6]: "f",
+        [key7]: "g",
+        [key8]: "h",
+    };
+    expect(counter).toBe(8);
+    expect(object.a).toBe("a");
+    expect(object.b).toBe("b");
+    expect(object.c).toBe("c");
+    expect(object.d).toBe("d");
+    expect(object[0]).toBe("e");
+    expect(object[2]).toBe("f");
+    expect(object[4]).toBe("g");
+    expect(object[8]).toBe("h");
+    expect(Object.getOwnPropertyNames(object) + "").toBe(
+        ["0", "2", "4", "8", "a", "b", "c", "d"] + ""
+    );
+});