LibJS: Make Object.prototype.toString() fully spec compliant

- Fix evaluation order: IsArray(O) should always be called and before
  Get(O, @@toStringTag), previously it was the other way around and
  IsArray would only be called if @@toStringTag is not a string
- Add missing exception checks to both function calls
- Add missing builtin tag for arguments object

Also, while we're here:
- Update variable names to match spec
- Add spec step comments

5 files changed, 87 insertions, 36 deletions

diff --git a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp
index 919752d03c..0f73cef6a7 100644
--- a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp
+++ b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp
@@ -414,6 +414,7 @@ Object* create_unmapped_arguments_object(GlobalObject& global_object, Vector<Val
     // 2. Let obj be ! OrdinaryObjectCreate(%Object.prototype%, « [[ParameterMap]] »).
     // 3. Set obj.[[ParameterMap]] to undefined.
     auto* object = Object::create(global_object, global_object.object_prototype());
+    object->set_has_parameter_map();
 
     // 4. Perform DefinePropertyOrThrow(obj, "length", PropertyDescriptor { [[Value]]: 𝔽(len), [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true }).
     object->define_property_or_throw(vm.names.length, { .value = Value(length), .writable = true, .enumerable = false, .configurable = true });
diff --git a/Userland/Libraries/LibJS/Runtime/ArgumentsObject.cpp b/Userland/Libraries/LibJS/Runtime/ArgumentsObject.cpp
index ea2b682227..908524f02b 100644
--- a/Userland/Libraries/LibJS/Runtime/ArgumentsObject.cpp
+++ b/Userland/Libraries/LibJS/Runtime/ArgumentsObject.cpp
@@ -18,6 +18,7 @@ ArgumentsObject::ArgumentsObject(GlobalObject& global_object, Environment& envir
 void ArgumentsObject::initialize(GlobalObject& global_object)
 {
     Base::initialize(global_object);
+    set_has_parameter_map();
     m_parameter_map = Object::create(global_object, nullptr);
 }
 
diff --git a/Userland/Libraries/LibJS/Runtime/Object.h b/Userland/Libraries/LibJS/Runtime/Object.h
index 86374cfb48..ee9c9fe3e7 100644
--- a/Userland/Libraries/LibJS/Runtime/Object.h
+++ b/Userland/Libraries/LibJS/Runtime/Object.h
@@ -151,6 +151,9 @@ public:
     // B.3.7 The [[IsHTMLDDA]] Internal Slot, https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot
     virtual bool is_htmldda() const { return false; }
 
+    bool has_parameter_map() const { return m_has_parameter_map; }
+    void set_has_parameter_map() { m_has_parameter_map = true; }
+
     virtual const char* class_name() const override { return "Object"; }
     virtual void visit_edges(Cell::Visitor&) override;
     virtual Value value_of() const { return Value(const_cast<Object*>(this)); }
@@ -194,8 +197,12 @@ protected:
     explicit Object(GlobalObjectTag);
     Object(ConstructWithoutPrototypeTag, GlobalObject&);
 
+    // [[Extensible]]
     bool m_is_extensible { true };
 
+    // [[ParameterMap]]
+    bool m_has_parameter_map { false };
+
 private:
     Value call_native_property_getter(NativeProperty& property, Value this_value) const;
     void call_native_property_setter(NativeProperty& property, Value this_value, Value) const;
diff --git a/Userland/Libraries/LibJS/Runtime/ObjectPrototype.cpp b/Userland/Libraries/LibJS/Runtime/ObjectPrototype.cpp
index c6a2f51c9a..21a17bd9ce 100644
--- a/Userland/Libraries/LibJS/Runtime/ObjectPrototype.cpp
+++ b/Userland/Libraries/LibJS/Runtime/ObjectPrototype.cpp
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2020-2021, Linus Groh <linusg@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -73,39 +74,71 @@ JS_DEFINE_NATIVE_FUNCTION(ObjectPrototype::to_string)
 {
     auto this_value = vm.this_value(global_object);
 
+    // 1. If the this value is undefined, return "[object Undefined]".
     if (this_value.is_undefined())
         return js_string(vm, "[object Undefined]");
+
+    // 2. If the this value is null, return "[object Null]".
     if (this_value.is_null())
         return js_string(vm, "[object Null]");
 
-    auto* this_object = this_value.to_object(global_object);
-    VERIFY(this_object);
+    // 3. Let O be ! ToObject(this value).
+    auto* object = this_value.to_object(global_object);
+    VERIFY(object);
+
+    // 4. Let isArray be ? IsArray(O).
+    auto is_array = Value(object).is_array(global_object);
+    if (vm.exception())
+        return {};
+
+    String builtin_tag;
+
+    // 5. If isArray is true, let builtinTag be "Array".
+    if (is_array)
+        builtin_tag = "Array";
+    // 6. Else if O has a [[ParameterMap]] internal slot, let builtinTag be "Arguments".
+    else if (object->has_parameter_map())
+        builtin_tag = "Arguments";
+    // 7. Else if O has a [[Call]] internal method, let builtinTag be "Function".
+    else if (object->is_function())
+        builtin_tag = "Function";
+    // 8. Else if O has an [[ErrorData]] internal slot, let builtinTag be "Error".
+    else if (is<Error>(object))
+        builtin_tag = "Error";
+    // 9. Else if O has a [[BooleanData]] internal slot, let builtinTag be "Boolean".
+    else if (is<BooleanObject>(object))
+        builtin_tag = "Boolean";
+    // 10. Else if O has a [[NumberData]] internal slot, let builtinTag be "Number".
+    else if (is<NumberObject>(object))
+        builtin_tag = "Number";
+    // 11. Else if O has a [[StringData]] internal slot, let builtinTag be "String".
+    else if (is<StringObject>(object))
+        builtin_tag = "String";
+    // 12. Else if O has a [[DateValue]] internal slot, let builtinTag be "Date".
+    else if (is<Date>(object))
+        builtin_tag = "Date";
+    // 13. Else if O has a [[RegExpMatcher]] internal slot, let builtinTag be "RegExp".
+    else if (is<RegExpObject>(object))
+        builtin_tag = "RegExp";
+    // 14. Else, let builtinTag be "Object".
+    else
+        builtin_tag = "Object";
+
+    // 15. Let tag be ? Get(O, @@toStringTag).
+    auto to_string_tag = object->get(*vm.well_known_symbol_to_string_tag());
+    if (vm.exception())
+        return {};
 
+    // Optimization: Instead of creating another PrimitiveString from builtin_tag, we separate tag and to_string_tag and add an additional branch to step 16.
     String tag;
-    auto to_string_tag = this_object->get(*vm.well_known_symbol_to_string_tag());
 
-    if (to_string_tag.is_string()) {
+    // 16. If Type(tag) is not String, set tag to builtinTag.
+    if (!to_string_tag.is_string())
+        tag = move(builtin_tag);
+    else
         tag = to_string_tag.as_string().string();
-    } else if (this_object->is_array()) {
-        tag = "Array";
-    } else if (this_object->is_function()) {
-        tag = "Function";
-    } else if (is<Error>(this_object)) {
-        tag = "Error";
-    } else if (is<BooleanObject>(this_object)) {
-        tag = "Boolean";
-    } else if (is<NumberObject>(this_object)) {
-        tag = "Number";
-    } else if (is<StringObject>(this_object)) {
-        tag = "String";
-    } else if (is<Date>(this_object)) {
-        tag = "Date";
-    } else if (is<RegExpObject>(this_object)) {
-        tag = "RegExp";
-    } else {
-        tag = "Object";
-    }
 
+    // 17. Return the string-concatenation of "[object ", tag, and "]".
     return js_string(vm, String::formatted("[object {}]", tag));
 }
 
diff --git a/Userland/Libraries/LibJS/Tests/builtins/Object/Object.prototype.toString.js b/Userland/Libraries/LibJS/Tests/builtins/Object/Object.prototype.toString.js
index fc16b9984d..70e1898886 100644
--- a/Userland/Libraries/LibJS/Tests/builtins/Object/Object.prototype.toString.js
+++ b/Userland/Libraries/LibJS/Tests/builtins/Object/Object.prototype.toString.js
@@ -3,20 +3,29 @@ test("length", () => {
 });
 
 test("result for various object types", () => {
-    const oToString = o => Object.prototype.toString.call(o);
+    const arrayProxy = new Proxy([], {});
+    const customToStringTag = {
+        [Symbol.toStringTag]: "Foo",
+    };
+    const arguments = (function () {
+        return arguments;
+    })();
 
-    expect(oToString(undefined)).toBe("[object Undefined]");
-    expect(oToString(null)).toBe("[object Null]");
-    expect(oToString([])).toBe("[object Array]");
-    expect(oToString(function () {})).toBe("[object Function]");
-    expect(oToString(new Error())).toBe("[object Error]");
-    expect(oToString(new TypeError())).toBe("[object Error]");
-    expect(oToString(new AggregateError([]))).toBe("[object Error]");
-    expect(oToString(new Boolean())).toBe("[object Boolean]");
-    expect(oToString(new Number())).toBe("[object Number]");
-    expect(oToString(new Date())).toBe("[object Date]");
-    expect(oToString(new RegExp())).toBe("[object RegExp]");
-    expect(oToString({})).toBe("[object Object]");
+    expect(Object.prototype.toString.call(undefined)).toBe("[object Undefined]");
+    expect(Object.prototype.toString.call(null)).toBe("[object Null]");
+    expect(Object.prototype.toString.call([])).toBe("[object Array]");
+    expect(Object.prototype.toString.call(arguments)).toBe("[object Arguments]");
+    expect(Object.prototype.toString.call(function () {})).toBe("[object Function]");
+    expect(Object.prototype.toString.call(new Error())).toBe("[object Error]");
+    expect(Object.prototype.toString.call(new TypeError())).toBe("[object Error]");
+    expect(Object.prototype.toString.call(new AggregateError([]))).toBe("[object Error]");
+    expect(Object.prototype.toString.call(new Boolean())).toBe("[object Boolean]");
+    expect(Object.prototype.toString.call(new Number())).toBe("[object Number]");
+    expect(Object.prototype.toString.call(new Date())).toBe("[object Date]");
+    expect(Object.prototype.toString.call(new RegExp())).toBe("[object RegExp]");
+    expect(Object.prototype.toString.call({})).toBe("[object Object]");
+    expect(Object.prototype.toString.call(arrayProxy)).toBe("[object Array]");
+    expect(Object.prototype.toString.call(customToStringTag)).toBe("[object Foo]");
 
     expect(globalThis.toString()).toBe("[object Object]");
 });