LibJS: Implement Sets using Maps

This implements ordered sets using Maps with a sentinel value, and
includes some extra set tests.
Fixes #11004.

Co-Authored-By: davidot <davidot@serenityos.org>

Userland/Libraries/LibJS/Runtime/Set.cpp

@@ -15,6 +15,7 @@ Set* Set::create(GlobalObject& global_object)
 
 Set::Set(Object& prototype)
     : Object(prototype)
+    , m_values(*prototype.global_object().map_prototype())
 {
 }
 
@@ -25,8 +26,7 @@ Set::~Set()
 void Set::visit_edges(Cell::Visitor& visitor)
 {
     Base::visit_edges(visitor);
-    for (auto& value : m_values)
+    static_cast<Object&>(m_values).visit_edges(visitor);
-        visitor.visit(value);
 }
 
 }

Userland/Libraries/LibJS/Runtime/Set.h

@@ -6,8 +6,8 @@
 
 #pragma once
 
-#include <AK/HashTable.h>
 #include <LibJS/Runtime/GlobalObject.h>
+#include <LibJS/Runtime/Map.h>
 #include <LibJS/Runtime/Object.h>
 #include <LibJS/Runtime/Value.h>
 
@@ -22,13 +22,24 @@ public:
     explicit Set(Object& prototype);
     virtual ~Set() override;
 
-    OrderedHashTable<Value, ValueTraits> const& values() const { return m_values; };
+    // NOTE: Unlike what the spec says, we implement Sets using an underlying map,
-    OrderedHashTable<Value, ValueTraits>& values() { return m_values; };
+    //       so all the functions below do not directly implement the operations as
+    //       defined by the specification.
+
+    void set_clear() { m_values.map_clear(); }
+    bool set_remove(Value const& value) { return m_values.map_remove(value); }
+    bool set_has(Value const& key) const { return m_values.map_has(key); }
+    void set_add(Value const& key) { m_values.map_set(key, js_undefined()); }
+    size_t set_size() const { return m_values.map_size(); }
+
+    auto begin() const { return m_values.begin(); }
+    auto begin() { return m_values.begin(); }
+    auto end() const { return m_values.end(); }
 
 private:
     virtual void visit_edges(Visitor& visitor) override;
 
-    OrderedHashTable<Value, ValueTraits> m_values;
+    Map m_values;
 };
 
 }

Userland/Libraries/LibJS/Runtime/SetIterator.cpp

@@ -18,7 +18,7 @@ SetIterator::SetIterator(Set& set, Object::PropertyKind iteration_kind, Object&
     : Object(prototype)
     , m_set(set)
     , m_iteration_kind(iteration_kind)
-    , m_iterator(set.values().begin())
+    , m_iterator(static_cast<Set const&>(set).begin())
 {
 }
 

Userland/Libraries/LibJS/Runtime/SetIterator.h

@@ -33,7 +33,7 @@ private:
     Set& m_set;
     bool m_done { false };
     Object::PropertyKind m_iteration_kind;
-    OrderedHashTable<Value, ValueTraits>::Iterator m_iterator;
+    Map::ConstIterator m_iterator;
 };
 
 }

Userland/Libraries/LibJS/Runtime/SetIteratorPrototype.cpp

@@ -41,7 +41,7 @@ JS_DEFINE_NATIVE_FUNCTION(SetIteratorPrototype::next)
         return create_iterator_result_object(global_object, js_undefined(), true);
 
     auto& set = set_iterator->set();
-    if (set_iterator->m_iterator == set.values().end()) {
+    if (set_iterator->m_iterator == set.end()) {
         set_iterator->m_done = true;
         return create_iterator_result_object(global_object, js_undefined(), true);
     }
@@ -49,7 +49,7 @@ JS_DEFINE_NATIVE_FUNCTION(SetIteratorPrototype::next)
     auto iteration_kind = set_iterator->iteration_kind();
     VERIFY(iteration_kind != Object::PropertyKind::Key);
 
-    auto value = *set_iterator->m_iterator;
+    auto value = (*set_iterator->m_iterator).key;
     ++set_iterator->m_iterator;
     if (iteration_kind == Object::PropertyKind::Value)
         return create_iterator_result_object(global_object, value, false);

Userland/Libraries/LibJS/Runtime/SetPrototype.cpp

@@ -52,7 +52,7 @@ JS_DEFINE_NATIVE_FUNCTION(SetPrototype::add)
     auto value = vm.argument(0);
     if (value.is_negative_zero())
         value = Value(0);
-    set->values().set(value, AK::HashSetExistingEntryBehavior::Keep);
+    set->set_add(value);
     return set;
 }
 
@@ -60,7 +60,7 @@ JS_DEFINE_NATIVE_FUNCTION(SetPrototype::add)
 JS_DEFINE_NATIVE_FUNCTION(SetPrototype::clear)
 {
     auto* set = TRY(typed_this_object(global_object));
-    set->values().clear();
+    set->set_clear();
     return js_undefined();
 }
 
@@ -68,7 +68,7 @@ JS_DEFINE_NATIVE_FUNCTION(SetPrototype::clear)
 JS_DEFINE_NATIVE_FUNCTION(SetPrototype::delete_)
 {
     auto* set = TRY(typed_this_object(global_object));
-    return Value(set->values().remove(vm.argument(0)));
+    return Value(set->set_remove(vm.argument(0)));
 }
 
 // 24.2.3.5 Set.prototype.entries ( ), https://tc39.es/ecma262/#sec-set.prototype.entries
@@ -86,8 +86,8 @@ JS_DEFINE_NATIVE_FUNCTION(SetPrototype::for_each)
     if (!vm.argument(0).is_function())
         return vm.throw_completion<TypeError>(global_object, ErrorType::NotAFunction, vm.argument(0).to_string_without_side_effects());
     auto this_value = vm.this_value(global_object);
-    for (auto& value : set->values())
+    for (auto& entry : *set)
-        TRY(call(global_object, vm.argument(0).as_function(), vm.argument(1), value, value, this_value));
+        TRY(call(global_object, vm.argument(0).as_function(), vm.argument(1), entry.key, entry.key, this_value));
     return js_undefined();
 }
 
@@ -95,8 +95,7 @@ JS_DEFINE_NATIVE_FUNCTION(SetPrototype::for_each)
 JS_DEFINE_NATIVE_FUNCTION(SetPrototype::has)
 {
     auto* set = TRY(typed_this_object(global_object));
-    auto& values = set->values();
+    return Value(set->set_has(vm.argument(0)));
-    return Value(values.find(vm.argument(0)) != values.end());
 }
 
 // 24.2.3.10 Set.prototype.values ( ), https://tc39.es/ecma262/#sec-set.prototype.values
@@ -111,7 +110,7 @@ JS_DEFINE_NATIVE_FUNCTION(SetPrototype::values)
 JS_DEFINE_NATIVE_FUNCTION(SetPrototype::size_getter)
 {
     auto* set = TRY(typed_this_object(global_object));
-    return Value(set->values().size());
+    return Value(set->set_size());
 }
 
 }

Userland/Libraries/LibJS/Tests/builtins/Set/Set.prototype.forEach.js

@@ -46,3 +46,106 @@ describe("normal behavior", () => {
         });
     });
 });
+
+describe("modification during iteration", () => {
+    test("adding items during forEach also get visited", () => {
+        const set = new Set([1, 2]);
+        const visited = [];
+        set.forEach(val => {
+            if (val <= 2) set.add(4 * val);
+
+            visited.push(val);
+        });
+        expect(set).toHaveSize(4);
+
+        expect(visited).toEqual([1, 2, 4, 8]);
+    });
+
+    test("removing an item before it is visited means it doesn't get visited", () => {
+        const set = new Set([1, 2, 3]);
+        const visited = [];
+        set.forEach(val => {
+            visited.push(val);
+            if (val === 1) {
+                expect(set.delete(2)).toBeTrue();
+            } else {
+                expect(val).toBe(3);
+                expect(set.delete(2)).toBeFalse();
+            }
+        });
+        expect(set).toHaveSize(2);
+        expect(visited).toEqual([1, 3]);
+    });
+
+    test("removing an item after it was visited and adding it again means it gets visited twice", () => {
+        const set = new Set([1, 2, 3]);
+        const visited = [];
+        set.forEach(val => {
+            visited.push(val);
+            if (val === 2) {
+                expect(set.delete(1)).toBeTrue();
+            } else if (val === 3) {
+                expect(set).toHaveSize(2);
+                set.add(1);
+                expect(set).toHaveSize(3);
+            }
+        });
+        expect(set).toHaveSize(3);
+        expect(visited).toEqual([1, 2, 3, 1]);
+    });
+
+    test("adding a new item and removing it before it gets visited means it never gets visited", () => {
+        const set = new Set([1, 2]);
+        const visited = [];
+        set.forEach(val => {
+            visited.push(val);
+            if (val === 1) {
+                set.add(3);
+                expect(set).toHaveSize(3);
+            } else if (val === 2) {
+                expect(set).toHaveSize(3);
+                expect(set.delete(3)).toBeTrue();
+                expect(set).toHaveSize(2);
+            }
+            expect(val).not.toBe(3);
+        });
+        expect(set).toHaveSize(2);
+        expect(visited).toEqual([1, 2]);
+    });
+
+    test("removing and adding in the same iterations", () => {
+        const set = new Set([1, 2, 3]);
+        const visited = [];
+        let first = true;
+        set.forEach(val => {
+            visited.push(val);
+            if (val === 1 && first) {
+                expect(set.delete(1)).toBeTrue();
+                set.add(1);
+            }
+
+            first = false;
+        });
+        expect(set).toHaveSize(3);
+
+        expect(visited).toEqual([1, 2, 3, 1]);
+    });
+
+    test("removing and readding the same item means it can get visited n times", () => {
+        let n = 3;
+
+        const set = new Set([1, 2]);
+
+        const visited = [];
+        set.forEach(val => {
+            visited.push(val);
+            if (n-- > 0) {
+                expect(set.delete(val)).toBeTrue();
+                set.add(val);
+            }
+        });
+
+        expect(set).toHaveSize(2);
+        expect(visited).toEqual([1, 2, 1, 2, 1]);
+    });
+});

Userland/Utilities/js.cpp

@@ -375,13 +375,12 @@ static void print_map(JS::Object const& object, HashTable<JS::Object*>& seen_obj
 static void print_set(JS::Object const& object, HashTable<JS::Object*>& seen_objects)
 {
     auto& set = static_cast<JS::Set const&>(object);
-    auto& values = set.values();
     print_type("Set");
     js_out(" {{");
     bool first = true;
-    for (auto& value : values) {
+    for (auto& entry : set) {
         print_separator(first);
-        print_value(value, seen_objects);
+        print_value(entry.key, seen_objects);
     }
     if (!first)
         js_out(" ");