Fix Error constructors to return rather than throw (#749)

* Refactor: Error constructors to return instead of "throwing"

* Test: Add a test case for Error Objects in Object.prototype.toString

* Fix: Make Error.prototype.toString spec compliant

* Test: Add tests for Error.prototype.toString

boa/src/builtins/error/mod.rs

@@ -25,6 +25,9 @@ pub(crate) mod r#type;
 // pub(crate) mod eval;
 // pub(crate) mod uri;
 
+#[cfg(test)]
+mod tests;
+
 pub(crate) use self::r#type::TypeError;
 pub(crate) use self::range::RangeError;
 pub(crate) use self::reference::ReferenceError;
@@ -78,7 +81,7 @@ impl Error {
         // This value is used by console.log and other routines to match Object type
         // to its Javascript Identifier (global constructor method name)
         this.set_data(ObjectData::Error);
-        Err(this.clone())
+        Ok(this.clone())
     }
 
     /// `Error.prototype.toString()`
@@ -93,8 +96,33 @@ impl Error {
     /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/toString
     #[allow(clippy::wrong_self_convention)]
     pub(crate) fn to_string(this: &Value, _: &[Value], context: &mut Context) -> Result<Value> {
-        let name = this.get_field("name").to_string(context)?;
-        let message = this.get_field("message").to_string(context)?;
-        Ok(format!("{}: {}", name, message).into())
+        if !this.is_object() {
+            return context.throw_type_error("'this' is not an Object");
+        }
+        let name = this.get_field("name");
+        let name_to_string;
+        let name = if name.is_undefined() {
+            "Error"
+        } else {
+            name_to_string = name.to_string(context)?;
+            name_to_string.as_str()
+        };
+
+        let message = this.get_field("message");
+        let message_to_string;
+        let message = if message.is_undefined() {
+            ""
+        } else {
+            message_to_string = message.to_string(context)?;
+            message_to_string.as_str()
+        };
+
+        if name.is_empty() {
+            Ok(message.into())
+        } else if message.is_empty() {
+            Ok(name.into())
+        } else {
+            Ok(format!("{}: {}", name, message).into())
+        }
     }
 }

boa/src/builtins/error/range.rs

@@ -62,6 +62,6 @@ impl RangeError {
         // This value is used by console.log and other routines to match Object type
         // to its Javascript Identifier (global constructor method name)
         this.set_data(ObjectData::Error);
-        Err(this.clone())
+        Ok(this.clone())
     }
 }

boa/src/builtins/error/reference.rs

@@ -61,6 +61,6 @@ impl ReferenceError {
         // This value is used by console.log and other routines to match Object type
         // to its Javascript Identifier (global constructor method name)
         this.set_data(ObjectData::Error);
-        Err(this.clone())
+        Ok(this.clone())
     }
 }

boa/src/builtins/error/syntax.rs

@@ -64,6 +64,6 @@ impl SyntaxError {
         // This value is used by console.log and other routines to match Object type
         // to its Javascript Identifier (global constructor method name)
         this.set_data(ObjectData::Error);
-        Err(this.clone())
+        Ok(this.clone())
     }
 }

boa/src/builtins/error/tests.rs

@@ -0,0 +1,30 @@
+use crate::{forward, Context};
+
+#[test]
+fn error_to_string() {
+    let mut ctx = Context::new();
+    let init = r#"
+        let e = new Error('1');
+        let name = new Error();
+        let message = new Error('message');
+        message.name = '';
+        let range_e = new RangeError('2');
+        let ref_e = new ReferenceError('3');
+        let syntax_e = new SyntaxError('4');
+        let type_e = new TypeError('5');
+    "#;
+    forward(&mut ctx, init);
+    assert_eq!(forward(&mut ctx, "e.toString()"), "\"Error: 1\"");
+    assert_eq!(forward(&mut ctx, "name.toString()"), "\"Error\"");
+    assert_eq!(forward(&mut ctx, "message.toString()"), "\"message\"");
+    assert_eq!(forward(&mut ctx, "range_e.toString()"), "\"RangeError: 2\"");
+    assert_eq!(
+        forward(&mut ctx, "ref_e.toString()"),
+        "\"ReferenceError: 3\""
+    );
+    assert_eq!(
+        forward(&mut ctx, "syntax_e.toString()"),
+        "\"SyntaxError: 4\""
+    );
+    assert_eq!(forward(&mut ctx, "type_e.toString()"), "\"TypeError: 5\"");
+}

boa/src/builtins/error/type.rs

@@ -67,6 +67,6 @@ impl TypeError {
         // This value is used by console.log and other routines to match Object type
         // to its Javascript Identifier (global constructor method name)
         this.set_data(ObjectData::Error);
-        Err(this.clone())
+        Ok(this.clone())
     }
 }

boa/src/builtins/object/tests.rs

@@ -146,6 +146,8 @@ fn object_to_string() {
         Array.prototype.toString = Object.prototype.toString;
         let f = () => {};
         Function.prototype.toString = Object.prototype.toString;
+        let e = new Error('test');
+        Error.prototype.toString = Object.prototype.toString;
         let b = Boolean();
         Boolean.prototype.toString = Object.prototype.toString;
         let i = Number(42);
@@ -170,6 +172,7 @@ fn object_to_string() {
     // );
     assert_eq!(forward(&mut ctx, "a.toString()"), "\"[object Array]\"");
     assert_eq!(forward(&mut ctx, "f.toString()"), "\"[object Function]\"");
+    assert_eq!(forward(&mut ctx, "e.toString()"), "\"[object Error]\"");
     assert_eq!(forward(&mut ctx, "b.toString()"), "\"[object Boolean]\"");
     assert_eq!(forward(&mut ctx, "i.toString()"), "\"[object Number]\"");
     assert_eq!(forward(&mut ctx, "s.toString()"), "\"[object String]\"");

boa/src/context.rs

@@ -293,7 +293,7 @@ impl Context {
             vec![Const::from(message.into()).into()],
         ))
         .run(self)
-        .expect_err("RangeError should always throw")
+        .expect("Into<String> used as message")
     }
 
     /// Throws a `RangeError` with the specified message.
@@ -315,7 +315,7 @@ impl Context {
             vec![Const::from(message.into()).into()],
         ))
         .run(self)
-        .expect_err("TypeError should always throw")
+        .expect("Into<String> used as message")
     }
 
     /// Throws a `TypeError` with the specified message.
@@ -336,7 +336,7 @@ impl Context {
             vec![Const::from(message.into() + " is not defined").into()],
         ))
         .run(self)
-        .expect_err("ReferenceError should always throw")
+        .expect("Into<String> used as message")
     }
 
     /// Throws a `ReferenceError` with the specified message.
@@ -357,7 +357,7 @@ impl Context {
             vec![Const::from(message.into()).into()],
         ))
         .run(self)
-        .expect_err("SyntaxError should always throw")
+        .expect("Into<String> used as message")
     }
 
     /// Throws a `SyntaxError` with the specified message.