LibJS: Bring Reference records a bit closer to the ECMAScript spec

Our Reference class now has the same fields as the spec:

- Base (a non-nullish value, an environment record, or `unresolvable`)
- Referenced Name (the name of the binding)
- Strict (whether the reference originated in strict mode code)
- ThisValue (if non-empty, the reference represents a `super` keyword)

The main difference from before is that we now resolve the environment
record that a reference interacts with. Previously we simply resolved
to either "local variable" or "global variable".

The associated abstract operations are still largely non-conforming,
since we don't yet implement proper variable bindings. But this patch
should at least fix a handful of test262 cases. :^)

There's one minor regression: some TypeError message strings get
a little worse due to doing a RequireObjectCoercible earlier in the
evaluation of MemberExpression.

Userland/Libraries/LibJS/AST.cpp

@@ -130,7 +130,7 @@ CallExpression::ThisAndCallee CallExpression::compute_this_and_callee(Interprete
     if (is<MemberExpression>(*m_callee)) {
         auto& member_expression = static_cast<MemberExpression const&>(*m_callee);
         Value callee;
-        Object* this_value = nullptr;
+        Value this_value;
 
         if (is<SuperExpression>(member_expression.object())) {
             auto super_base = interpreter.current_function_environment_record()->get_super_base();
@@ -141,7 +141,7 @@ CallExpression::ThisAndCallee CallExpression::compute_this_and_callee(Interprete
             auto property_name = member_expression.computed_property_name(interpreter, global_object);
             if (!property_name.is_valid())
                 return {};
-            auto reference = Reference(super_base, property_name);
+            auto reference = Reference { super_base, property_name, super_base };
             callee = reference.get(global_object);
             if (vm.exception())
                 return {};
@@ -153,9 +153,7 @@ CallExpression::ThisAndCallee CallExpression::compute_this_and_callee(Interprete
             callee = reference.get(global_object);
             if (vm.exception())
                 return {};
-            this_value = reference.base().to_object(global_object);
-            if (vm.exception())
-                return {};
+            this_value = reference.get_this_value();
         }
 
         return { this_value, callee };
@@ -667,9 +665,9 @@ Reference Expression::to_reference(Interpreter&, GlobalObject&) const
     return {};
 }
 
-Reference Identifier::to_reference(Interpreter& interpreter, GlobalObject&) const
+Reference Identifier::to_reference(Interpreter& interpreter, GlobalObject& global_object) const
 {
-    return interpreter.vm().resolve_binding(string());
+    return interpreter.vm().resolve_binding(global_object, string());
 }
 
 Reference MemberExpression::to_reference(Interpreter& interpreter, GlobalObject& global_object) const
@@ -677,10 +675,16 @@ Reference MemberExpression::to_reference(Interpreter& interpreter, GlobalObject&
     auto object_value = m_object->execute(interpreter, global_object);
     if (interpreter.exception())
         return {};
+
+    object_value = require_object_coercible(global_object, object_value);
+    if (interpreter.exception())
+        return {};
+
     auto property_name = computed_property_name(interpreter, global_object);
     if (!property_name.is_valid())
-        return {};
-    return { object_value, property_name };
+        return Reference {};
+
+    return Reference { object_value, property_name, object_value };
 }
 
 Value UnaryExpression::execute(Interpreter& interpreter, GlobalObject& global_object) const
@@ -701,12 +705,10 @@ Value UnaryExpression::execute(Interpreter& interpreter, GlobalObject& global_ob
         if (interpreter.exception()) {
             return {};
         }
-        // FIXME: standard recommends checking with is_unresolvable but it ALWAYS return false here
-        if (reference.is_local_variable() || reference.is_global_variable()) {
-            const auto& name = reference.name();
-            lhs_result = interpreter.vm().get_variable(name.to_string(), global_object).value_or(js_undefined());
-            if (interpreter.exception())
-                return {};
+        if (reference.is_unresolvable()) {
+            lhs_result = js_undefined();
+        } else {
+            lhs_result = reference.get(global_object, false);
         }
     } else {
         lhs_result = m_lhs->execute(interpreter, global_object);
@@ -1477,6 +1479,7 @@ Value UpdateExpression::execute(Interpreter& interpreter, GlobalObject& global_o
     InterpreterNodeScope node_scope { interpreter, *this };
 
     auto reference = m_argument->to_reference(interpreter, global_object);
+
     if (interpreter.exception())
         return {};
     auto old_value = reference.get(global_object);

Userland/Libraries/LibJS/Runtime/Reference.cpp

@@ -1,9 +1,10 @@
 /*
- * Copyright (c) 2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2020-2021, Andreas Kling <kling@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
 
+#include <LibJS/AST.h>
 #include <LibJS/Runtime/Error.h>
 #include <LibJS/Runtime/GlobalObject.h>
 #include <LibJS/Runtime/Reference.h>
@@ -15,39 +16,50 @@ void Reference::put(GlobalObject& global_object, Value value)
     auto& vm = global_object.vm();
 
     if (is_unresolvable()) {
-        throw_reference_error(global_object);
+        if (m_strict) {
+            throw_reference_error(global_object);
+            return;
+        }
+        global_object.put(m_name, value);
         return;
     }
 
-    if (is_local_variable() || is_global_variable()) {
-        if (is_local_variable())
-            vm.set_variable(m_name.to_string(), value, global_object);
-        else
-            global_object.put(m_name, value);
-        return;
-    }
+    if (is_property_reference()) {
+        // FIXME: This is an ad-hoc hack until we support proper variable bindings.
+        if (!m_base_value.is_object() && vm.in_strict_mode()) {
+            if (m_base_value.is_nullish())
+                vm.throw_exception<TypeError>(global_object, ErrorType::ReferenceNullishSetProperty, m_name.to_value(vm).to_string_without_side_effects(), m_base_value.to_string_without_side_effects());
+            else
+                vm.throw_exception<TypeError>(global_object, ErrorType::ReferencePrimitiveSetProperty, m_name.to_value(vm).to_string_without_side_effects(), m_base_value.typeof(), m_base_value.to_string_without_side_effects());
+            return;
+        }
 
-    auto base = this->base();
+        auto* base_obj = m_base_value.to_object(global_object);
+        if (!base_obj)
+            return;
 
-    if (!base.is_object() && vm.in_strict_mode()) {
-        if (base.is_nullish())
-            vm.throw_exception<TypeError>(global_object, ErrorType::ReferenceNullishSetProperty, m_name.to_value(vm).to_string_without_side_effects(), base.to_string_without_side_effects());
-        else
-            vm.throw_exception<TypeError>(global_object, ErrorType::ReferencePrimitiveSetProperty, m_name.to_value(vm).to_string_without_side_effects(), base.typeof(), base.to_string_without_side_effects());
+        bool succeeded = base_obj->put(m_name, value);
+        if (!succeeded && m_strict) {
+            vm.throw_exception<TypeError>(global_object, ErrorType::ReferenceNullishSetProperty, m_name.to_value(vm).to_string_without_side_effects(), m_base_value.to_string_without_side_effects());
+            return;
+        }
         return;
     }
 
-    if (base.is_nullish()) {
-        // This will always fail the to_object() call below, let's throw the TypeError ourselves with a nice message instead.
-        vm.throw_exception<TypeError>(global_object, ErrorType::ReferenceNullishSetProperty, m_name.to_value(vm).to_string_without_side_effects(), base.to_string_without_side_effects());
-        return;
-    }
+    VERIFY(m_base_type == BaseType::EnvironmentRecord);
+    auto existing_variable = m_base_environment_record->get_from_environment_record(m_name.as_string());
+    Variable variable {
+        .value = value,
+        .declaration_kind = existing_variable.has_value() ? existing_variable->declaration_kind : DeclarationKind::Var
+    };
 
-    auto* object = base.to_object(global_object);
-    if (!object)
+    // FIXME: This is a hack until we support proper variable bindings.
+    if (variable.declaration_kind == DeclarationKind::Const) {
+        vm.throw_exception<TypeError>(global_object, ErrorType::InvalidAssignToConst);
         return;
+    }
 
-    object->put(m_name, value);
+    m_base_environment_record->put_into_environment_record(m_name.as_string(), variable);
 }
 
 void Reference::throw_reference_error(GlobalObject& global_object)
@@ -59,71 +71,92 @@ void Reference::throw_reference_error(GlobalObject& global_object)
         vm.throw_exception<ReferenceError>(global_object, ErrorType::UnknownIdentifier, m_name.to_string_or_symbol().to_display_string());
 }
 
-Value Reference::get(GlobalObject& global_object)
+Value Reference::get(GlobalObject& global_object, bool throw_if_undefined)
 {
-    auto& vm = global_object.vm();
-
     if (is_unresolvable()) {
         throw_reference_error(global_object);
         return {};
     }
 
-    if (is_local_variable() || is_global_variable()) {
-        Value value;
-        if (is_local_variable())
-            value = vm.get_variable(m_name.to_string(), global_object);
-        else
-            value = global_object.get(m_name);
-        if (vm.exception())
-            return {};
-        if (value.is_empty()) {
-            throw_reference_error(global_object);
+    if (is_property_reference()) {
+        auto* base_obj = m_base_value.to_object(global_object);
+        if (!base_obj)
             return {};
-        }
-        return value;
+        return base_obj->get(m_name).value_or(js_undefined());
     }
 
-    auto base = this->base();
-
-    if (base.is_nullish()) {
-        // This will always fail the to_object() call below, let's throw the TypeError ourselves with a nice message instead.
-        vm.throw_exception<TypeError>(global_object, ErrorType::ReferenceNullishGetProperty, m_name.to_value(vm).to_string_without_side_effects(), base.to_string_without_side_effects());
+    VERIFY(m_base_type == BaseType::EnvironmentRecord);
+    auto value = m_base_environment_record->get_from_environment_record(m_name.as_string());
+    if (!value.has_value()) {
+        if (!throw_if_undefined) {
+            // FIXME: This is an ad-hoc hack for the `typeof` operator until we support proper variable bindings.
+            return js_undefined();
+        }
+        throw_reference_error(global_object);
         return {};
     }
-
-    auto* object = base.to_object(global_object);
-    if (!object)
-        return {};
-
-    return object->get(m_name).value_or(js_undefined());
+    return value->value;
 }
 
 bool Reference::delete_(GlobalObject& global_object)
 {
-    if (is_unresolvable())
+    if (is_unresolvable()) {
+        VERIFY(!m_strict);
         return true;
+    }
 
     auto& vm = global_object.vm();
 
-    if (is_local_variable() || is_global_variable()) {
-        if (is_local_variable())
-            return vm.delete_variable(m_name.to_string());
-        else
-            return global_object.delete_property(m_name);
+    if (is_property_reference()) {
+        auto* base_obj = m_base_value.to_object(global_object);
+        if (!base_obj)
+            return false;
+        bool succeeded = base_obj->delete_property(m_name);
+        if (!succeeded && m_strict) {
+            vm.throw_exception<TypeError>(global_object, ErrorType::ReferenceNullishDeleteProperty, m_name.to_value(vm).to_string_without_side_effects(), m_base_value.to_string_without_side_effects());
+            return false;
+        }
+        return succeeded;
     }
 
-    auto base = this->base();
+    VERIFY(m_base_type == BaseType::EnvironmentRecord);
+    return m_base_environment_record->delete_from_environment_record(m_name.as_string());
+}
 
-    if (base.is_nullish()) {
-        // This will always fail the to_object() call below, let's throw the TypeError ourselves with a nice message instead.
-        vm.throw_exception<TypeError>(global_object, ErrorType::ReferenceNullishDeleteProperty, m_name.to_value(vm).to_string_without_side_effects(), base.to_string_without_side_effects());
-        return false;
+String Reference::to_string() const
+{
+    StringBuilder builder;
+    builder.append("Reference { Base=");
+    switch (m_base_type) {
+    case BaseType::Unresolvable:
+        builder.append("Unresolvable");
+        break;
+    case BaseType::EnvironmentRecord:
+        builder.appendff("{}", base_environment().class_name());
+        break;
+    case BaseType::Value:
+        if (m_base_value.is_empty())
+            builder.append("<empty>");
+        else
+            builder.appendff("{}", m_base_value.to_string_without_side_effects());
+        break;
     }
+    builder.append(", ReferencedName=");
+    if (!m_name.is_valid())
+        builder.append("<invalid>");
+    else if (m_name.is_symbol())
+        builder.appendff("{}", m_name.as_symbol()->to_string());
+    else
+        builder.appendff("{}", m_name.to_string());
+    builder.appendff(", Strict={}", m_strict);
+    builder.appendff(", ThisValue=");
+    if (m_this_value.is_empty())
+        builder.append("<empty>");
+    else
+        builder.appendff("{}", m_this_value.to_string_without_side_effects());
 
-    auto* object = base.to_object(global_object);
-    VERIFY(object);
-
-    return object->delete_property(m_name);
+    builder.append(" }");
+    return builder.to_string();
 }
 
 }

Userland/Libraries/LibJS/Runtime/Reference.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2020-2021, Andreas Kling <kling@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -7,6 +7,7 @@
 #pragma once
 
 #include <AK/String.h>
+#include <LibJS/Runtime/EnvironmentRecord.h>
 #include <LibJS/Runtime/PropertyName.h>
 #include <LibJS/Runtime/Value.h>
 
@@ -14,69 +15,105 @@ namespace JS {
 
 class Reference {
 public:
+    enum class BaseType : u8 {
+        Unresolvable,
+        Value,
+        EnvironmentRecord,
+    };
+
     Reference() { }
-    Reference(Value base, const PropertyName& name, bool strict = false)
-        : m_base(base)
+    Reference(BaseType type, PropertyName const& name, bool strict)
+        : m_base_type(type)
         , m_name(name)
         , m_strict(strict)
     {
     }
 
-    enum LocalVariableTag { LocalVariable };
-    Reference(LocalVariableTag, const FlyString& name, bool strict = false)
-        : m_base(js_null())
+    Reference(Value base, PropertyName const& name, Value this_value, bool strict = false)
+        : m_base_type(BaseType::Value)
+        , m_base_value(base)
         , m_name(name)
+        , m_this_value(this_value)
         , m_strict(strict)
-        , m_local_variable(true)
     {
+        if (base.is_nullish()) {
+            m_base_type = BaseType::Unresolvable;
+            m_base_value = {};
+            m_this_value = {};
+            m_name = {};
+        }
     }
 
-    enum GlobalVariableTag { GlobalVariable };
-    Reference(GlobalVariableTag, const FlyString& name, bool strict = false)
-        : m_base(js_null())
-        , m_name(name)
+    Reference(EnvironmentRecord& base, FlyString const& referenced_name, bool strict = false)
+        : m_base_type(BaseType::EnvironmentRecord)
+        , m_base_environment_record(&base)
+        , m_name(referenced_name)
         , m_strict(strict)
-        , m_global_variable(true)
     {
     }
 
-    Value base() const { return m_base; }
-    const PropertyName& name() const { return m_name; }
-    bool is_strict() const { return m_strict; }
+    Value base() const
+    {
+        VERIFY(m_base_type == BaseType::Value);
+        return m_base_value;
+    }
 
-    bool is_unresolvable() const { return m_base.is_empty(); }
-    bool is_property() const
+    EnvironmentRecord& base_environment() const
     {
-        return m_base.is_object() || has_primitive_base();
+        VERIFY(m_base_type == BaseType::EnvironmentRecord);
+        return *m_base_environment_record;
     }
 
-    bool has_primitive_base() const
+    PropertyName const& name() const { return m_name; }
+    bool is_strict() const { return m_strict; }
+
+    // 6.2.4.2 IsUnresolvableReference ( V ), https://tc39.es/ecma262/#sec-isunresolvablereference
+    bool is_unresolvable() const { return m_base_type == BaseType::Unresolvable; }
+
+    // 6.2.4.1 IsPropertyReference ( V ), https://tc39.es/ecma262/#sec-ispropertyreference
+    bool is_property_reference() const
     {
-        return m_base.is_boolean() || m_base.is_string() || m_base.is_number();
+        if (is_unresolvable())
+            return false;
+        if (m_base_type == BaseType::EnvironmentRecord)
+            return false;
+        if (m_base_value.is_boolean() || m_base_value.is_string() || m_base_value.is_symbol() || m_base_value.is_bigint() || m_base_value.is_number() || m_base_value.is_object())
+            return true;
+        return false;
     }
 
-    bool is_local_variable() const
+    // 6.2.4.7 GetThisValue ( V ), https://tc39.es/ecma262/#sec-getthisvalue
+    Value get_this_value() const
     {
-        return m_local_variable;
+        VERIFY(is_property_reference());
+        if (is_super_reference())
+            return m_this_value;
+        return m_base_value;
     }
 
-    bool is_global_variable() const
+    // 6.2.4.3 IsSuperReference ( V ), https://tc39.es/ecma262/#sec-issuperreference
+    bool is_super_reference() const
     {
-        return m_global_variable;
+        return !m_this_value.is_empty();
     }
 
     void put(GlobalObject&, Value);
-    Value get(GlobalObject&);
+    Value get(GlobalObject&, bool throw_if_undefined = true);
     bool delete_(GlobalObject&);
 
+    String to_string() const;
+
 private:
     void throw_reference_error(GlobalObject&);
 
-    Value m_base;
+    BaseType m_base_type { BaseType::Unresolvable };
+    union {
+        Value m_base_value;
+        EnvironmentRecord* m_base_environment_record;
+    };
     PropertyName m_name;
+    Value m_this_value;
     bool m_strict { false };
-    bool m_local_variable { false };
-    bool m_global_variable { false };
 };
 
 }

Userland/Libraries/LibJS/Runtime/VM.cpp

@@ -393,16 +393,16 @@ Value VM::get_variable(const FlyString& name, GlobalObject& global_object)
 }
 
 // 9.4.2 ResolveBinding ( name [ , env ] ), https://tc39.es/ecma262/#sec-resolvebinding
-Reference VM::resolve_binding(FlyString const& name)
+Reference VM::resolve_binding(GlobalObject& global_object, FlyString const& name, EnvironmentRecord*)
 {
     // FIXME: This implementation of ResolveBinding is non-conforming.
 
     for (auto* environment_record = lexical_environment(); environment_record && environment_record->outer_environment(); environment_record = environment_record->outer_environment()) {
         auto possible_match = environment_record->get_from_environment_record(name);
         if (possible_match.has_value())
-            return { Reference::LocalVariable, name };
+            return Reference { *environment_record, name };
     }
-    return { Reference::GlobalVariable, name };
+    return Reference { global_object.environment_record(), name };
 }
 
 Value VM::construct(Function& function, Function& new_target, Optional<MarkedValueList> arguments)

Userland/Libraries/LibJS/Runtime/VM.h

@@ -204,7 +204,7 @@ public:
     void assign(const FlyString& target, Value, GlobalObject&, bool first_assignment = false, EnvironmentRecord* specific_scope = nullptr);
     void assign(const NonnullRefPtr<BindingPattern>& target, Value, GlobalObject&, bool first_assignment = false, EnvironmentRecord* specific_scope = nullptr);
 
-    Reference resolve_binding(FlyString const&);
+    Reference resolve_binding(GlobalObject&, FlyString const&, EnvironmentRecord* = nullptr);
 
     template<typename T, typename... Args>
     void throw_exception(GlobalObject& global_object, Args&&... args)

Userland/Libraries/LibJS/Tests/strict-mode-errors.js

@@ -18,12 +18,9 @@ test("basic functionality", () => {
     [null, undefined].forEach(primitive => {
         expect(() => {
             primitive.foo = "bar";
-        }).toThrowWithMessage(TypeError, `Cannot set property 'foo' of ${primitive}`);
+        }).toThrowWithMessage(TypeError, `${primitive} cannot be converted to an object`);
         expect(() => {
             primitive[Symbol.hasInstance] = 123;
-        }).toThrowWithMessage(
-            TypeError,
-            `Cannot set property 'Symbol(Symbol.hasInstance)' of ${primitive}`
-        );
+        }).toThrowWithMessage(TypeError, `${primitive} cannot be converted to an object`);
     });
 });