LibJS: Use empty value for Reference unresolvable state, not undefined

This fixes an issue where `undefined.foo = "bar"` would throw a
ReferenceError instead of a TypeError as undefined was also used for
truly unresolvable references (e.g. `foo() = "bar"`). I also made the
various error messages here a bit nicer, just "primitive value" is not
very helpful.

4 files changed, 39 insertions, 15 deletions

diff --git a/Userland/Libraries/LibJS/Runtime/ErrorTypes.h b/Userland/Libraries/LibJS/Runtime/ErrorTypes.h
index 8491890f75..6437d9d479 100644
--- a/Userland/Libraries/LibJS/Runtime/ErrorTypes.h
+++ b/Userland/Libraries/LibJS/Runtime/ErrorTypes.h
@@ -146,7 +146,8 @@
                                         "target is non-extensible")                                                                     \
     M(ProxyTwoArguments, "Proxy constructor requires at least two arguments")                                                           \
     M(ReduceNoInitial, "Reduce of empty array with no initial value")                                                                   \
-    M(ReferencePrimitiveAssignment, "Cannot assign property {} to primitive value")                                                     \
+    M(ReferenceNullishAssignment, "Cannot set property '{}' of {}")                                                                     \
+    M(ReferencePrimitiveAssignment, "Cannot set property '{}' of {} '{}'")                                                              \
     M(ReferenceUnresolvable, "Unresolvable reference")                                                                                  \
     M(ReflectArgumentMustBeAFunction, "First argument of Reflect.{}() must be a function")                                              \
     M(ReflectArgumentMustBeAnObject, "First argument of Reflect.{}() must be an object")                                                \
diff --git a/Userland/Libraries/LibJS/Runtime/Reference.cpp b/Userland/Libraries/LibJS/Runtime/Reference.cpp
index 6b4fff4baf..b567e38d23 100644
--- a/Userland/Libraries/LibJS/Runtime/Reference.cpp
+++ b/Userland/Libraries/LibJS/Runtime/Reference.cpp
@@ -47,12 +47,23 @@ void Reference::put(GlobalObject& global_object, Value value)
         return;
     }
 
-    if (!base().is_object() && vm.in_strict_mode()) {
-        vm.throw_exception<TypeError>(global_object, ErrorType::ReferencePrimitiveAssignment, m_name.to_value(global_object.vm()).to_string_without_side_effects());
+    auto base = this->base();
+
+    if (!base.is_object() && vm.in_strict_mode()) {
+        if (base.is_nullish())
+            vm.throw_exception<TypeError>(global_object, ErrorType::ReferenceNullishAssignment, m_name.to_value(global_object.vm()).to_string_without_side_effects(), base.to_string_without_side_effects());
+        else
+            vm.throw_exception<TypeError>(global_object, ErrorType::ReferencePrimitiveAssignment, m_name.to_value(global_object.vm()).to_string_without_side_effects(), base.typeof(), base.to_string_without_side_effects());
         return;
     }
 
-    auto* object = base().to_object(global_object);
+    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::ReferenceNullishAssignment, m_name.to_value(global_object.vm()).to_string_without_side_effects(), base.to_string_without_side_effects());
+        return;
+    }
+
+    auto* object = base.to_object(global_object);
     if (!object)
         return;
 
@@ -61,13 +72,11 @@ void Reference::put(GlobalObject& global_object, Value value)
 
 void Reference::throw_reference_error(GlobalObject& global_object)
 {
-    auto property_name = m_name.to_string();
-    String message;
-    if (property_name.is_empty()) {
-        global_object.vm().throw_exception<ReferenceError>(global_object, ErrorType::ReferenceUnresolvable);
-    } else {
-        global_object.vm().throw_exception<ReferenceError>(global_object, ErrorType::UnknownIdentifier, property_name);
-    }
+    auto& vm = global_object.vm();
+    if (!m_name.is_valid())
+        vm.throw_exception<ReferenceError>(global_object, ErrorType::ReferenceUnresolvable);
+    else
+        vm.throw_exception<ReferenceError>(global_object, ErrorType::UnknownIdentifier, m_name.to_string_or_symbol().to_display_string());
 }
 
 Value Reference::get(GlobalObject& global_object)
diff --git a/Userland/Libraries/LibJS/Runtime/Reference.h b/Userland/Libraries/LibJS/Runtime/Reference.h
index d5a8010916..ff94bd0359 100644
--- a/Userland/Libraries/LibJS/Runtime/Reference.h
+++ b/Userland/Libraries/LibJS/Runtime/Reference.h
@@ -64,7 +64,7 @@ public:
     const PropertyName& name() const { return m_name; }
     bool is_strict() const { return m_strict; }
 
-    bool is_unresolvable() const { return m_base.is_undefined(); }
+    bool is_unresolvable() const { return m_base.is_empty(); }
     bool is_property() const
     {
         return m_base.is_object() || has_primitive_base();
@@ -91,7 +91,7 @@ public:
 private:
     void throw_reference_error(GlobalObject&);
 
-    Value m_base { js_undefined() };
+    Value m_base;
     PropertyName m_name;
     bool m_strict { false };
     bool m_local_variable { false };
diff --git a/Userland/Libraries/LibJS/Tests/strict-mode-errors.js b/Userland/Libraries/LibJS/Tests/strict-mode-errors.js
index ada52e359f..8c8d756c4f 100644
--- a/Userland/Libraries/LibJS/Tests/strict-mode-errors.js
+++ b/Userland/Libraries/LibJS/Tests/strict-mode-errors.js
@@ -4,12 +4,26 @@ test("basic functionality", () => {
     [true, false, "foo", 123].forEach(primitive => {
         expect(() => {
             primitive.foo = "bar";
-        }).toThrowWithMessage(TypeError, "Cannot assign property foo to primitive value");
+        }).toThrowWithMessage(
+            TypeError,
+            `Cannot set property 'foo' of ${typeof primitive} '${primitive}'`
+        );
+        expect(() => {
+            primitive[Symbol.hasInstance] = 123;
+        }).toThrowWithMessage(
+            TypeError,
+            `Cannot set property 'Symbol(Symbol.hasInstance)' of ${typeof primitive} '${primitive}'`
+        );
+    });
+    [null, undefined].forEach(primitive => {
+        expect(() => {
+            primitive.foo = "bar";
+        }).toThrowWithMessage(TypeError, `Cannot set property 'foo' of ${primitive}`);
         expect(() => {
             primitive[Symbol.hasInstance] = 123;
         }).toThrowWithMessage(
             TypeError,
-            "Cannot assign property Symbol(Symbol.hasInstance) to primitive value"
+            `Cannot set property 'Symbol(Symbol.hasInstance)' of ${primitive}`
         );
     });
 });