From eaf8c2e398f197f24f27c444d59a2313315f8c3c Mon Sep 17 00:00:00 2001
From: Simon Wanner <simon+git@skyrising.xyz>
Date: Sun, 5 Nov 2023 15:21:01 +0100
Subject: [PATCH] LibJS: Improve error messages for primitive strict mode
 property access

Using ErrorType::ReferencePrimitiveSetProperty the errors for primitives
now look like "Cannot set property 'foo' of number '123'".

The strict-mode-errors test has been adjusted and re-enabled.
---
 .../LibJS/Bytecode/CommonImplementations.cpp  | 12 +++++++++--
 .../LibJS/Tests/builtins/Symbol/Symbol.js     |  2 +-
 .../LibJS/Tests/strict-mode-errors.js         | 20 ++++++++-----------
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/Userland/Libraries/LibJS/Bytecode/CommonImplementations.cpp b/Userland/Libraries/LibJS/Bytecode/CommonImplementations.cpp
index b435a1419d7..214a6e0c763 100644
--- a/Userland/Libraries/LibJS/Bytecode/CommonImplementations.cpp
+++ b/Userland/Libraries/LibJS/Bytecode/CommonImplementations.cpp
@@ -147,6 +147,11 @@ ThrowCompletionOr<Value> get_global(Bytecode::Interpreter& interpreter, Identifi
 
 ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value, Value value, PropertyKey name, Op::PropertyKind kind)
 {
+    // Better error message than to_object would give
+    if (vm.in_strict_mode() && base.is_nullish())
+        return vm.throw_completion<TypeError>(ErrorType::ReferenceNullishSetProperty, name, base.to_string_without_side_effects());
+
+    // a. Let baseObj be ? ToObject(V.[[Base]]).
     auto object = TRY(base.to_object(vm));
     if (kind == Op::PropertyKind::Getter || kind == Op::PropertyKind::Setter) {
         // The generator should only pass us functions for getters and setters.
@@ -169,8 +174,11 @@ ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value
     }
     case Op::PropertyKind::KeyValue: {
         bool succeeded = TRY(object->internal_set(name, value, this_value));
-        if (!succeeded && vm.in_strict_mode())
-            return vm.throw_completion<TypeError>(ErrorType::ReferenceNullishSetProperty, name, base.to_string_without_side_effects());
+        if (!succeeded && vm.in_strict_mode()) {
+            if (base.is_object())
+                return vm.throw_completion<TypeError>(ErrorType::ReferenceNullishSetProperty, name, base.to_string_without_side_effects());
+            return vm.throw_completion<TypeError>(ErrorType::ReferencePrimitiveSetProperty, name, base.typeof(), base.to_string_without_side_effects());
+        }
         break;
     }
     case Op::PropertyKind::DirectKeyValue:
diff --git a/Userland/Libraries/LibJS/Tests/builtins/Symbol/Symbol.js b/Userland/Libraries/LibJS/Tests/builtins/Symbol/Symbol.js
index b947d28f4c7..b82eab90ca4 100644
--- a/Userland/Libraries/LibJS/Tests/builtins/Symbol/Symbol.js
+++ b/Userland/Libraries/LibJS/Tests/builtins/Symbol/Symbol.js
@@ -23,5 +23,5 @@ test("setting new properties on a symbol is an error in strict mode", () => {
     var symbol = Symbol("foo");
     expect(() => {
         symbol.bar = 42;
-    }).toThrowWithMessage(TypeError, "Cannot set property 'bar' of Symbol(foo)");
+    }).toThrowWithMessage(TypeError, "Cannot set property 'bar' of symbol 'Symbol(foo)'");
 });
diff --git a/Userland/Libraries/LibJS/Tests/strict-mode-errors.js b/Userland/Libraries/LibJS/Tests/strict-mode-errors.js
index 26bb39dd4a3..4954581373a 100644
--- a/Userland/Libraries/LibJS/Tests/strict-mode-errors.js
+++ b/Userland/Libraries/LibJS/Tests/strict-mode-errors.js
@@ -1,23 +1,19 @@
 "use strict";
 
-test.xfail("basic functionality", () => {
-    [true, false, "foo", 123].forEach(primitive => {
+test("basic functionality", () => {
+    [true, false, "foo", 123, 123n, null, undefined].forEach(primitive => {
+        let description = `${typeof primitive} '${primitive}${
+            typeof primitive == "bigint" ? "n" : ""
+        }'`;
+        if (primitive == null) description = String(primitive);
         expect(() => {
             primitive.foo = "bar";
-        }).toThrowWithMessage(TypeError, `Cannot set property 'foo' of ${primitive}`);
+        }).toThrowWithMessage(TypeError, `Cannot set property 'foo' of ${description}`);
         expect(() => {
             primitive[Symbol.hasInstance] = 123;
         }).toThrowWithMessage(
             TypeError,
-            `Cannot set property 'Symbol(Symbol.hasInstance)' of ${primitive}`
+            `Cannot set property 'Symbol(Symbol.hasInstance)' of ${description}`
         );
     });
-    [null, undefined].forEach(primitive => {
-        expect(() => {
-            primitive.foo = "bar";
-        }).toThrowWithMessage(TypeError, `${primitive} cannot be converted to an object`);
-        expect(() => {
-            primitive[Symbol.hasInstance] = 123;
-        }).toThrowWithMessage(TypeError, `${primitive} cannot be converted to an object`);
-    });
 });