LibJS: Refactor Accessor

This changes Accessor's m_{getter,setter} from Value to Function* which
seems like a better API to me - a getter/setter must either be a
function or missing, and the creation of an accessor with other values
must be prevented by the parser and Object.defineProperty() anyway.

Also add Accessor::set_{getter,setter}() so we can reuse an already
created accessor when evaluating an ObjectExpression with getter/setter
shorthand syntax.

Libraries/LibJS/AST.cpp

@@ -1175,20 +1175,22 @@ Value ObjectExpression::execute(Interpreter& interpreter) const
         update_function_name(value, name);
 
         if (property.type() == ObjectProperty::Type::Getter || property.type() == ObjectProperty::Type::Setter) {
-            Value getter;
-            Value setter;
-            auto existing_property_metadata = object->shape().lookup(key);
-            Value existing_property;
-            if (existing_property_metadata.has_value())
-                existing_property = object->get_direct(existing_property_metadata.value().offset);
-            if (property.type() == ObjectProperty::Type::Getter) {
-                getter = value;
-                setter = existing_property.is_accessor() ? existing_property.as_accessor().setter() : Value();
-            } else {
-                getter = existing_property.is_accessor() ? existing_property.as_accessor().getter() : Value();
-                setter = value;
+            ASSERT(value.is_function());
+            Accessor* accessor { nullptr };
+            auto property_metadata = object->shape().lookup(key);
+            if (property_metadata.has_value()) {
+                auto existing_property = object->get_direct(property_metadata.value().offset);
+                if (existing_property.is_accessor())
+                    accessor = &existing_property.as_accessor();
+            }
+            if (!accessor) {
+                accessor = Accessor::create(interpreter, nullptr, nullptr);
+                object->put_own_property(*object, key, Attribute::Configurable | Attribute::Enumerable, accessor, Object::PutOwnPropertyMode::DefineProperty);
             }
-            object->put_own_property(*object, key, Attribute::Configurable | Attribute::Enumerable, Accessor::create(interpreter, getter, setter), Object::PutOwnPropertyMode::DefineProperty);
+            if (property.type() == ObjectProperty::Type::Getter)
+                accessor->set_getter(&value.as_function());
+            else
+                accessor->set_setter(&value.as_function());
         } else {
             object->put(key, value);
         }

Libraries/LibJS/Runtime/Accessor.h

@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2020, Matthew Olsson <matthewcolsson@gmail.com>
+ * Copyright (c) 2020, Linus Groh <mail@linusgroh.de>
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -26,41 +27,43 @@
 
 #pragma once
 
-#include <LibJS/Runtime/Cell.h>
-#include <LibJS/Runtime/Value.h>
+#include <LibJS/Runtime/Function.h>
 
 namespace JS {
 
 class Accessor final : public Cell {
 public:
-    static Accessor* create(Interpreter& interpreter, Value getter, Value setter)
+    static Accessor* create(Interpreter& interpreter, Function* getter, Function* setter)
     {
         return interpreter.heap().allocate<Accessor>(getter, setter);
     }
 
-    Accessor(Value getter, Value setter)
+    Accessor(Function* getter, Function* setter)
         : m_getter(getter)
         , m_setter(setter)
     {
     }
 
-    Value getter() { return m_getter; }
-    Value setter() { return m_setter; }
+    Function* getter() const { return m_getter; }
+    void set_getter(Function* getter) { m_getter = getter; }
 
-    Value call_getter(Value this_object)
+    Function* setter() const { return m_setter; }
+    void set_setter(Function* setter) { m_setter = setter; }
+
+    Value call_getter(Value this_value)
     {
-        if (!getter().is_function())
+        if (!m_getter)
             return js_undefined();
-        return interpreter().call(getter().as_function(), this_object);
+        return interpreter().call(*m_getter, this_value);
     }
 
-    void call_setter(Value this_object, Value setter_value)
+    void call_setter(Value this_value, Value setter_value)
     {
-        if (!setter().is_function())
+        if (!m_setter)
             return;
         MarkedValueList arguments(interpreter().heap());
         arguments.values().append(setter_value);
-        interpreter().call(setter().as_function(), this_object, move(arguments));
+        interpreter().call(*m_setter, this_value, move(arguments));
     }
 
     void visit_children(Cell::Visitor& visitor) override
@@ -72,8 +75,8 @@ public:
 private:
     const char* class_name() const override { return "Accessor"; };
 
-    Value m_getter;
-    Value m_setter;
+    Function* m_getter { nullptr };
+    Function* m_setter { nullptr };
 };
 
 }

Libraries/LibJS/Runtime/Object.cpp

@@ -221,30 +221,35 @@ bool Object::define_property(const FlyString& property_name, const Object& descr
             return false;
         }
 
-        auto getter = descriptor.get("get");
+        auto getter = descriptor.get("get").value_or(js_undefined());
         if (interpreter().exception())
             return {};
-        auto setter = descriptor.get("set");
+        auto setter = descriptor.get("set").value_or(js_undefined());
         if (interpreter().exception())
             return {};
 
-        if (!(getter.is_empty() || getter.is_undefined() || getter.is_function())) {
+        Function* getter_function { nullptr };
+        Function* setter_function { nullptr };
+
+        if (getter.is_function()) {
+            getter_function = &getter.as_function();
+        } else if (!getter.is_undefined()) {
             interpreter().throw_exception<TypeError>("Accessor descriptor's 'get' field must be a function or undefined");
             return false;
         }
 
-        if (!(setter.is_empty() || setter.is_undefined() || setter.is_function())) {
+        if (setter.is_function()) {
+            setter_function = &setter.as_function();
+        } else if (!setter.is_undefined()) {
             interpreter().throw_exception<TypeError>("Accessor descriptor's 'set' field must be a function or undefined");
             return false;
         }
 
-        // FIXME: Throw a TypeError if the setter does not take any arguments
-
-        dbg() << "Defining new property " << property_name << " with accessor descriptor { attributes=" << attributes
-              << " , getter=" << (getter.is_empty() ? "<empty>" : getter.to_string_without_side_effects())
-              << ", setter=" << (setter.is_empty() ? "<empty>" : setter.to_string_without_side_effects()) << "}";
+        dbg() << "Defining new property " << property_name << " with accessor descriptor { attributes=" << attributes << ", "
+              << "getter=" << getter.to_string_without_side_effects() << ", "
+              << "setter=" << setter.to_string_without_side_effects() << "}";
 
-        return put_own_property(*this, property_name, attributes, Accessor::create(interpreter(), getter, setter), PutOwnPropertyMode::DefineProperty, throw_exceptions);
+        return put_own_property(*this, property_name, attributes, Accessor::create(interpreter(), getter_function, setter_function), PutOwnPropertyMode::DefineProperty, throw_exceptions);
     }
 
     auto value = descriptor.get("value");
@@ -267,9 +272,9 @@ bool Object::put_own_property(Object& this_object, const FlyString& property_nam
 
     if (value.is_accessor()) {
         auto& accessor = value.as_accessor();
-        if (accessor.getter().is_function())
+        if (accessor.getter())
             attributes |= Attribute::HasGet;
-        if (accessor.setter().is_function())
+        if (accessor.setter())
             attributes |= Attribute::HasSet;
     }