From a80d3fdf491af03e3edbea0243b97edc615fc10c Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Wed, 22 Jun 2022 23:08:12 +0300 Subject: [PATCH] LibJS: Implement WeakMap changes from 'Symbol as WeakMap Keys Proposal' --- Tests/LibJS/test-js.cpp | 6 +++--- Userland/Libraries/LibJS/Runtime/ErrorTypes.h | 1 + .../LibJS/Runtime/WeakMapPrototype.cpp | 21 ++++++++++--------- .../LibJS/Tests/builtins/WeakMap/WeakMap.js | 2 +- .../WeakMap/WeakMap.prototype.delete.js | 3 +++ .../builtins/WeakMap/WeakMap.prototype.get.js | 2 ++ .../builtins/WeakMap/WeakMap.prototype.has.js | 2 ++ .../builtins/WeakMap/WeakMap.prototype.set.js | 20 ++++++++++++++---- 8 files changed, 39 insertions(+), 18 deletions(-) diff --git a/Tests/LibJS/test-js.cpp b/Tests/LibJS/test-js.cpp index aeffa8515b..7bfc47754a 100644 --- a/Tests/LibJS/test-js.cpp +++ b/Tests/LibJS/test-js.cpp @@ -68,10 +68,10 @@ TESTJS_GLOBAL_FUNCTION(mark_as_garbage, markAsGarbage) auto value = TRY(reference.get_value(global_object)); - if (!value.is_object()) - return vm.throw_completion(global_object, JS::ErrorType::NotAnObject, String::formatted("Variable with name {}", variable_name.string())); + if (!can_be_held_weakly(value)) + return vm.throw_completion(global_object, JS::ErrorType::CannotBeHeldWeakly, String::formatted("Variable with name {}", variable_name.string())); - vm.heap().uproot_cell(&value.as_object()); + vm.heap().uproot_cell(&value.as_cell()); TRY(reference.delete_(global_object)); return JS::js_undefined(); diff --git a/Userland/Libraries/LibJS/Runtime/ErrorTypes.h b/Userland/Libraries/LibJS/Runtime/ErrorTypes.h index e6b0ca34f5..d875c668c3 100644 --- a/Userland/Libraries/LibJS/Runtime/ErrorTypes.h +++ b/Userland/Libraries/LibJS/Runtime/ErrorTypes.h @@ -19,6 +19,7 @@ M(CallStackSizeExceeded, "Call stack size limit exceeded") \ M(CannotDeclareGlobalFunction, "Cannot declare global function of name '{}'") \ M(CannotDeclareGlobalVariable, "Cannot declare global variable of name '{}'") \ + M(CannotBeHeldWeakly, "{} cannot be held weakly") \ M(ClassConstructorWithoutNew, "Class constructor {} must be called with 'new'") \ M(ClassExtendsValueNotAConstructorOrNull, "Class extends value {} is not a constructor or null") \ M(ClassExtendsValueInvalidPrototype, "Class extends value has an invalid prototype {}") \ diff --git a/Userland/Libraries/LibJS/Runtime/WeakMapPrototype.cpp b/Userland/Libraries/LibJS/Runtime/WeakMapPrototype.cpp index e74f2a2545..c84239e2f2 100644 --- a/Userland/Libraries/LibJS/Runtime/WeakMapPrototype.cpp +++ b/Userland/Libraries/LibJS/Runtime/WeakMapPrototype.cpp @@ -1,11 +1,12 @@ /* - * Copyright (c) 2021, Idan Horowitz + * Copyright (c) 2021-2022, Idan Horowitz * * SPDX-License-Identifier: BSD-2-Clause */ #include #include +#include #include namespace JS { @@ -35,9 +36,9 @@ JS_DEFINE_NATIVE_FUNCTION(WeakMapPrototype::delete_) { auto* weak_map = TRY(typed_this_object(global_object)); auto value = vm.argument(0); - if (!value.is_object()) + if (!can_be_held_weakly(value)) return Value(false); - return Value(weak_map->values().remove(&value.as_object())); + return Value(weak_map->values().remove(&value.as_cell())); } // 24.3.3.3 WeakMap.prototype.get ( key ), https://tc39.es/ecma262/#sec-weakmap.prototype.get @@ -45,10 +46,10 @@ JS_DEFINE_NATIVE_FUNCTION(WeakMapPrototype::get) { auto* weak_map = TRY(typed_this_object(global_object)); auto value = vm.argument(0); - if (!value.is_object()) + if (!can_be_held_weakly(value)) return js_undefined(); auto& values = weak_map->values(); - auto result = values.find(&value.as_object()); + auto result = values.find(&value.as_cell()); if (result == values.end()) return js_undefined(); return result->value; @@ -59,10 +60,10 @@ JS_DEFINE_NATIVE_FUNCTION(WeakMapPrototype::has) { auto* weak_map = TRY(typed_this_object(global_object)); auto value = vm.argument(0); - if (!value.is_object()) + if (!can_be_held_weakly(value)) return Value(false); auto& values = weak_map->values(); - return Value(values.find(&value.as_object()) != values.end()); + return Value(values.find(&value.as_cell()) != values.end()); } // 24.3.3.5 WeakMap.prototype.set ( key, value ), https://tc39.es/ecma262/#sec-weakmap.prototype.set @@ -70,9 +71,9 @@ JS_DEFINE_NATIVE_FUNCTION(WeakMapPrototype::set) { auto* weak_map = TRY(typed_this_object(global_object)); auto value = vm.argument(0); - if (!value.is_object()) - return vm.throw_completion(global_object, ErrorType::NotAnObject, value.to_string_without_side_effects()); - weak_map->values().set(&value.as_object(), vm.argument(1)); + if (!can_be_held_weakly(value)) + return vm.throw_completion(global_object, ErrorType::CannotBeHeldWeakly, value.to_string_without_side_effects()); + weak_map->values().set(&value.as_cell(), vm.argument(1)); return weak_map; } diff --git a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.js b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.js index 78c8ad79a9..37c601c3b4 100644 --- a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.js +++ b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.js @@ -37,6 +37,6 @@ describe("regressions", () => { test("missing key/value properties on iterable entry", () => { expect(() => { new WeakMap([{}]); - }).toThrowWithMessage(TypeError, "undefined is not an object"); + }).toThrowWithMessage(TypeError, "undefined cannot be held weakly"); }); }); diff --git a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.delete.js b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.delete.js index dd12bd8b1a..4133b168aa 100644 --- a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.delete.js +++ b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.delete.js @@ -5,9 +5,12 @@ test("basic functionality", () => { [{ a: 1 }, 1], [{ a: 2 }, 2], [{ a: 3 }, 3], + [Symbol("foo"), "bar"], ]; const weakMap = new WeakMap(original); expect(weakMap.delete(original[0][0])).toBeTrue(); expect(weakMap.delete(original[0][0])).toBeFalse(); + expect(weakMap.delete(original[3][0])).toBeTrue(); + expect(weakMap.delete(original[3][0])).toBeFalse(); expect(weakMap.delete(null)).toBeFalse(); }); diff --git a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.get.js b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.get.js index 09cd06c970..33270ad7ec 100644 --- a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.get.js +++ b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.get.js @@ -5,8 +5,10 @@ test("basic functionality", () => { [{ a: 1 }, 1], [{ a: 2 }, 2], [{ a: 3 }, 3], + [Symbol("foo"), "bar"], ]; const weakMap = new WeakMap(original); expect(weakMap.get(original[0][0])).toBe(original[0][1]); + expect(weakMap.get(original[3][0])).toBe(original[3][1]); expect(weakMap.get(null)).toBe(undefined); }); diff --git a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.has.js b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.has.js index b0d67baa54..81dca8697e 100644 --- a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.has.js +++ b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.has.js @@ -7,10 +7,12 @@ test("basic functionality", () => { [{ a: 1 }, 1], [{ a: 2 }, 2], [{ a: 3 }, 3], + [Symbol("foo"), "bar"], ]; var weakMap = new WeakMap(original); expect(new WeakMap().has()).toBeFalse(); expect(weakMap.has(original[0][0])).toBeTrue(); + expect(weakMap.has(original[3][0])).toBeTrue(); expect(weakMap.has({ a: 1 })).toBeFalse(); }); diff --git a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.set.js b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.set.js index 9bc6afabbe..14d38d6512 100644 --- a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.set.js +++ b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.set.js @@ -5,9 +5,11 @@ test("basic functionality", () => { [{ a: 1 }, 1], [{ a: 2 }, 2], [{ a: 3 }, 3], + [Symbol("foo"), "bar"], ]); expect(weakMap.set({ a: 4 }, 4)).toBe(weakMap); expect(weakMap.set({ a: 1 }, 2)).toBe(weakMap); + expect(weakMap.set(Symbol("hello"), "friends")).toBe(weakMap); }); test("invalid values", () => { @@ -15,18 +17,28 @@ test("invalid values", () => { [-100, Infinity, NaN, "hello", 152n].forEach(value => { expect(() => { weakMap.set(value, value); - }).toThrowWithMessage(TypeError, "is not an object"); + }).toThrowWithMessage(TypeError, "cannot be held weakly"); }); }); test("automatic removal of garbage-collected values", () => { const weakMap = new WeakMap(); - const key = { e: 3 }; + const objectKey = { e: 3 }; - expect(weakMap.set(key, 1)).toBe(weakMap); + expect(weakMap.set(objectKey, 1)).toBe(weakMap); expect(getWeakMapSize(weakMap)).toBe(1); - markAsGarbage("key"); + markAsGarbage("objectKey"); + gc(); + + expect(getWeakMapSize(weakMap)).toBe(0); + + const symbolKey = Symbol("foo"); + + expect(weakMap.set(symbolKey, "bar")).toBe(weakMap); + expect(getWeakMapSize(weakMap)).toBe(1); + + markAsGarbage("symbolKey"); gc(); expect(getWeakMapSize(weakMap)).toBe(0);