LibJS: Include identifier information in nullish property write access

When a PutById / PutByValue bytecode operation results in accessing a
nullish object, we now include the name of the property and the object
being accessed in the exception message (if available). This should make
it easier to debug live websites.

For example, the following errors would all previously produce a generic
error message of "ToObject on null or undefined":

  > foo = null
  > foo.bar = 1
  Uncaught exception:
  [TypeError] Cannot access property "bar" on null object "foo"
      at <unknown>

  > foo = { bar: undefined }
  > foo.bar.baz = 1
  Uncaught exception:
  [TypeError] Cannot access property "baz" on undefined object "foo.bar"
      at <unknown>

Note we certainly don't capture all possible nullish property write
accesses here. This just covers cases I've seen most on live websites;
we can cover more cases as they arise.

Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp

@@ -489,16 +489,17 @@ Bytecode::CodeGenerationErrorOr<Optional<Bytecode::Operand>> AssignmentExpressio
                     generator.emit_set_variable(identifier, rval);
                 } else if (is<MemberExpression>(*lhs)) {
                     auto& expression = static_cast<MemberExpression const&>(*lhs);
+                    auto base_identifier = generator.intern_identifier_for_expression(expression.object());
 
                     if (expression.is_computed()) {
                         if (!lhs_is_super_expression)
-                            generator.emit<Bytecode::Op::PutByValue>(*base, *computed_property, rval);
+                            generator.emit<Bytecode::Op::PutByValue>(*base, *computed_property, rval, Bytecode::Op::PropertyKind::KeyValue, move(base_identifier));
                         else
                             generator.emit<Bytecode::Op::PutByValueWithThis>(*base, *computed_property, *this_value, rval);
                     } else if (expression.property().is_identifier()) {
                         auto identifier_table_ref = generator.intern_identifier(verify_cast<Identifier>(expression.property()).string());
                         if (!lhs_is_super_expression)
-                            generator.emit<Bytecode::Op::PutById>(*base, identifier_table_ref, rval, Bytecode::Op::PropertyKind::KeyValue, generator.next_property_lookup_cache());
+                            generator.emit<Bytecode::Op::PutById>(*base, identifier_table_ref, rval, Bytecode::Op::PropertyKind::KeyValue, generator.next_property_lookup_cache(), move(base_identifier));
                         else
                             generator.emit<Bytecode::Op::PutByIdWithThis>(*base, *this_value, identifier_table_ref, rval, Bytecode::Op::PropertyKind::KeyValue, generator.next_property_lookup_cache());
                     } else if (expression.property().is_private_identifier()) {

Userland/Libraries/LibJS/Bytecode/CommonImplementations.h

@@ -248,14 +248,18 @@ inline ThrowCompletionOr<Value> get_global(Bytecode::Interpreter& interpreter, D
     return vm.throw_completion<ReferenceError>(ErrorType::UnknownIdentifier, identifier);
 }
 
-inline ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value, Value value, PropertyKey name, Op::PropertyKind kind, PropertyLookupCache* cache = nullptr)
+inline ThrowCompletionOr<void> put_by_property_key(VM& vm, Value base, Value this_value, Value value, Optional<DeprecatedFlyString const&> const& base_identifier, PropertyKey name, Op::PropertyKind kind, PropertyLookupCache* cache = nullptr)
 {
     // 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));
+    auto maybe_object = base.to_object(vm);
+    if (maybe_object.is_error())
+        return throw_null_or_undefined_property_access(vm, base, base_identifier, name);
+    auto object = maybe_object.release_value();
+
     if (kind == Op::PropertyKind::Getter || kind == Op::PropertyKind::Setter) {
         // The generator should only pass us functions for getters and setters.
         VERIFY(value.is_function());
@@ -411,7 +415,7 @@ inline Value new_function(VM& vm, FunctionExpression const& function_node, Optio
     return value;
 }
 
-inline ThrowCompletionOr<void> put_by_value(VM& vm, Value base, Value property_key_value, Value value, Op::PropertyKind kind)
+inline ThrowCompletionOr<void> put_by_value(VM& vm, Value base, Optional<DeprecatedFlyString const&> const& base_identifier, Value property_key_value, Value value, Op::PropertyKind kind)
 {
     // OPTIMIZATION: Fast path for simple Int32 indexes in array-like objects.
     if ((kind == Op::PropertyKind::KeyValue || kind == Op::PropertyKind::DirectKeyValue)
@@ -489,7 +493,7 @@ inline ThrowCompletionOr<void> put_by_value(VM& vm, Value base, Value property_k
     }
 
     auto property_key = kind != Op::PropertyKind::Spread ? TRY(property_key_value.to_property_key(vm)) : PropertyKey {};
-    TRY(put_by_property_key(vm, base, base, value, property_key, kind));
+    TRY(put_by_property_key(vm, base, base, value, base_identifier, property_key, kind));
     return {};
 }
 

Userland/Libraries/LibJS/Bytecode/Interpreter.cpp

@@ -1132,9 +1132,10 @@ ThrowCompletionOr<void> PutById::execute_impl(Bytecode::Interpreter& interpreter
     auto& vm = interpreter.vm();
     auto value = interpreter.get(m_src);
     auto base = interpreter.get(m_base);
+    auto base_identifier = interpreter.current_executable().get_identifier(m_base_identifier);
     PropertyKey name = interpreter.current_executable().get_identifier(m_property);
     auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index];
-    TRY(put_by_property_key(vm, base, base, value, name, m_kind, &cache));
+    TRY(put_by_property_key(vm, base, base, value, base_identifier, name, m_kind, &cache));
     return {};
 }
 
@@ -1145,7 +1146,7 @@ ThrowCompletionOr<void> PutByIdWithThis::execute_impl(Bytecode::Interpreter& int
     auto base = interpreter.get(m_base);
     PropertyKey name = interpreter.current_executable().get_identifier(m_property);
     auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index];
-    TRY(put_by_property_key(vm, base, interpreter.get(m_this_value), value, name, m_kind, &cache));
+    TRY(put_by_property_key(vm, base, interpreter.get(m_this_value), value, {}, name, m_kind, &cache));
     return {};
 }
 
@@ -1523,7 +1524,8 @@ ThrowCompletionOr<void> PutByValue::execute_impl(Bytecode::Interpreter& interpre
 {
     auto& vm = interpreter.vm();
     auto value = interpreter.get(m_src);
-    TRY(put_by_value(vm, interpreter.get(m_base), interpreter.get(m_property), value, m_kind));
+    auto base_identifier = interpreter.current_executable().get_identifier(m_base_identifier);
+    TRY(put_by_value(vm, interpreter.get(m_base), base_identifier, interpreter.get(m_property), value, m_kind));
     return {};
 }
 
@@ -1533,7 +1535,7 @@ ThrowCompletionOr<void> PutByValueWithThis::execute_impl(Bytecode::Interpreter&
     auto value = interpreter.get(m_src);
     auto base = interpreter.get(m_base);
     auto property_key = m_kind != PropertyKind::Spread ? TRY(interpreter.get(m_property).to_property_key(vm)) : PropertyKey {};
-    TRY(put_by_property_key(vm, base, interpreter.get(m_this_value), value, property_key, m_kind));
+    TRY(put_by_property_key(vm, base, interpreter.get(m_this_value), value, {}, property_key, m_kind));
     return {};
 }
 

Userland/Libraries/LibJS/Bytecode/Op.h

@@ -741,13 +741,14 @@ enum class PropertyKind {
 
 class PutById final : public Instruction {
 public:
-    explicit PutById(Operand base, IdentifierTableIndex property, Operand src, PropertyKind kind, u32 cache_index)
+    explicit PutById(Operand base, IdentifierTableIndex property, Operand src, PropertyKind kind, u32 cache_index, Optional<IdentifierTableIndex> base_identifier = {})
         : Instruction(Type::PutById, sizeof(*this))
         , m_base(base)
         , m_property(property)
         , m_src(src)
         , m_kind(kind)
         , m_cache_index(cache_index)
+        , m_base_identifier(move(base_identifier))
     {
     }
 
@@ -766,6 +767,7 @@ private:
     Operand m_src;
     PropertyKind m_kind;
     u32 m_cache_index { 0 };
+    Optional<IdentifierTableIndex> m_base_identifier {};
 };
 
 class PutByIdWithThis final : public Instruction {
@@ -929,12 +931,13 @@ private:
 
 class PutByValue final : public Instruction {
 public:
-    PutByValue(Operand base, Operand property, Operand src, PropertyKind kind = PropertyKind::KeyValue)
+    PutByValue(Operand base, Operand property, Operand src, PropertyKind kind = PropertyKind::KeyValue, Optional<IdentifierTableIndex> base_identifier = {})
         : Instruction(Type::PutByValue, sizeof(*this))
         , m_base(base)
         , m_property(property)
         , m_src(src)
         , m_kind(kind)
+        , m_base_identifier(move(base_identifier))
     {
     }
 
@@ -951,6 +954,7 @@ private:
     Operand m_property;
     Operand m_src;
     PropertyKind m_kind;
+    Optional<IdentifierTableIndex> m_base_identifier;
 };
 
 class PutByValueWithThis final : public Instruction {

Userland/Libraries/LibJS/Tests/null-or-undefined-access.js

@@ -6,9 +6,17 @@ test("null/undefined object", () => {
             foo.bar;
         }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object "foo"`);
 
+        expect(() => {
+            foo.bar = 1;
+        }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object "foo"`);
+
         expect(() => {
             foo[0];
         }).toThrowWithMessage(TypeError, `Cannot access property "0" on ${value} object "foo"`);
+
+        expect(() => {
+            foo[0] = 1;
+        }).toThrowWithMessage(TypeError, `Cannot access property "0" on ${value} object "foo"`);
     });
 });
 
@@ -22,6 +30,13 @@ test("null/undefined object key", () => {
             TypeError,
             `Cannot access property "baz" on ${value} object "foo.bar"`
         );
+
+        expect(() => {
+            foo.bar.baz = 1;
+        }).toThrowWithMessage(
+            TypeError,
+            `Cannot access property "baz" on ${value} object "foo.bar"`
+        );
     });
 });
 
@@ -32,5 +47,9 @@ test("null/undefined array index", () => {
         expect(() => {
             foo[0].bar;
         }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object`);
+
+        expect(() => {
+            foo[0].bar = 1;
+        }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object`);
     });
 });