Spreadsheet: Avoid using Value.to_string_without_side_effects()

We should use .to_string() and handle the possible exceptions.
This makes the displayed cell contents so much more informative than
'[object Object]' :^)

13 files changed, 73 insertions, 54 deletions

diff --git a/Userland/Applications/Spreadsheet/Cell.cpp b/Userland/Applications/Spreadsheet/Cell.cpp
index e3bd310b42..176887eb3b 100644
--- a/Userland/Applications/Spreadsheet/Cell.cpp
+++ b/Userland/Applications/Spreadsheet/Cell.cpp
@@ -74,12 +74,12 @@ const CellType& Cell::type() const
     return *CellType::get_by_name("Identity");
 }
 
-String Cell::typed_display() const
+JS::ThrowCompletionOr<String> Cell::typed_display() const
 {
     return type().display(const_cast<Cell&>(*this), m_type_metadata);
 }
 
-JS::Value Cell::typed_js_data() const
+JS::ThrowCompletionOr<JS::Value> Cell::typed_js_data() const
 {
     return type().js_value(const_cast<Cell&>(*this), m_type_metadata);
 }
diff --git a/Userland/Applications/Spreadsheet/Cell.h b/Userland/Applications/Spreadsheet/Cell.h
index d2d3541c1a..1e8830084e 100644
--- a/Userland/Applications/Spreadsheet/Cell.h
+++ b/Userland/Applications/Spreadsheet/Cell.h
@@ -79,8 +79,8 @@ struct Cell : public Weakable<Cell> {
         m_conditional_formats = move(fmts);
     }
 
-    String typed_display() const;
-    JS::Value typed_js_data() const;
+    JS::ThrowCompletionOr<String> typed_display() const;
+    JS::ThrowCompletionOr<JS::Value> typed_js_data() const;
 
     const CellType& type() const;
     const CellTypeMetadata& type_metadata() const { return m_type_metadata; }
diff --git a/Userland/Applications/Spreadsheet/CellType/Date.cpp b/Userland/Applications/Spreadsheet/CellType/Date.cpp
index 9c257dab65..9714ebb47d 100644
--- a/Userland/Applications/Spreadsheet/CellType/Date.cpp
+++ b/Userland/Applications/Spreadsheet/CellType/Date.cpp
@@ -21,7 +21,7 @@ DateCell::~DateCell()
 {
 }
 
-String DateCell::display(Cell& cell, const CellTypeMetadata& metadata) const
+JS::ThrowCompletionOr<String> DateCell::display(Cell& cell, const CellTypeMetadata& metadata) const
 {
     ScopeGuard propagate_exception { [&cell] {
         if (auto exc = cell.sheet().interpreter().exception()) {
@@ -29,8 +29,8 @@ String DateCell::display(Cell& cell, const CellTypeMetadata& metadata) const
             cell.set_exception(exc);
         }
     } };
-    auto timestamp = js_value(cell, metadata);
-    auto string = Core::DateTime::from_timestamp(TRY_OR_DISCARD(timestamp.to_i32(cell.sheet().global_object()))).to_string(metadata.format.is_empty() ? "%Y-%m-%d %H:%M:%S" : metadata.format.characters());
+    auto timestamp = TRY(js_value(cell, metadata));
+    auto string = Core::DateTime::from_timestamp(TRY(timestamp.to_i32(cell.sheet().global_object()))).to_string(metadata.format.is_empty() ? "%Y-%m-%d %H:%M:%S" : metadata.format.characters());
 
     if (metadata.length >= 0)
         return string.substring(0, metadata.length);
@@ -38,10 +38,10 @@ String DateCell::display(Cell& cell, const CellTypeMetadata& metadata) const
     return string;
 }
 
-JS::Value DateCell::js_value(Cell& cell, const CellTypeMetadata&) const
+JS::ThrowCompletionOr<JS::Value> DateCell::js_value(Cell& cell, const CellTypeMetadata&) const
 {
     auto js_data = cell.js_data();
-    auto value = TRY_OR_DISCARD(js_data.to_double(cell.sheet().global_object()));
+    auto value = TRY(js_data.to_double(cell.sheet().global_object()));
     return JS::Value(value / 1000); // Turn it to seconds
 }
 
diff --git a/Userland/Applications/Spreadsheet/CellType/Date.h b/Userland/Applications/Spreadsheet/CellType/Date.h
index 6e613a4380..b39583c371 100644
--- a/Userland/Applications/Spreadsheet/CellType/Date.h
+++ b/Userland/Applications/Spreadsheet/CellType/Date.h
@@ -15,8 +15,8 @@ class DateCell : public CellType {
 public:
     DateCell();
     virtual ~DateCell() override;
-    virtual String display(Cell&, const CellTypeMetadata&) const override;
-    virtual JS::Value js_value(Cell&, const CellTypeMetadata&) const override;
+    virtual JS::ThrowCompletionOr<String> display(Cell&, const CellTypeMetadata&) const override;
+    virtual JS::ThrowCompletionOr<JS::Value> js_value(Cell&, const CellTypeMetadata&) const override;
 };
 
 }
diff --git a/Userland/Applications/Spreadsheet/CellType/Identity.cpp b/Userland/Applications/Spreadsheet/CellType/Identity.cpp
index c93ea23c4e..70a43c8be0 100644
--- a/Userland/Applications/Spreadsheet/CellType/Identity.cpp
+++ b/Userland/Applications/Spreadsheet/CellType/Identity.cpp
@@ -19,12 +19,12 @@ IdentityCell::~IdentityCell()
 {
 }
 
-String IdentityCell::display(Cell& cell, const CellTypeMetadata&) const
+JS::ThrowCompletionOr<String> IdentityCell::display(Cell& cell, const CellTypeMetadata&) const
 {
-    return cell.js_data().to_string_without_side_effects();
+    return cell.js_data().to_string(cell.sheet().global_object());
 }
 
-JS::Value IdentityCell::js_value(Cell& cell, const CellTypeMetadata&) const
+JS::ThrowCompletionOr<JS::Value> IdentityCell::js_value(Cell& cell, const CellTypeMetadata&) const
 {
     return cell.js_data();
 }
diff --git a/Userland/Applications/Spreadsheet/CellType/Identity.h b/Userland/Applications/Spreadsheet/CellType/Identity.h
index 659ee270a1..5811a7305e 100644
--- a/Userland/Applications/Spreadsheet/CellType/Identity.h
+++ b/Userland/Applications/Spreadsheet/CellType/Identity.h
@@ -15,8 +15,8 @@ class IdentityCell : public CellType {
 public:
     IdentityCell();
     virtual ~IdentityCell() override;
-    virtual String display(Cell&, const CellTypeMetadata&) const override;
-    virtual JS::Value js_value(Cell&, const CellTypeMetadata&) const override;
+    virtual JS::ThrowCompletionOr<String> display(Cell&, const CellTypeMetadata&) const override;
+    virtual JS::ThrowCompletionOr<JS::Value> js_value(Cell&, const CellTypeMetadata&) const override;
 };
 
 }
diff --git a/Userland/Applications/Spreadsheet/CellType/Numeric.cpp b/Userland/Applications/Spreadsheet/CellType/Numeric.cpp
index 7b8f098a2a..9ebe1d8aa9 100644
--- a/Userland/Applications/Spreadsheet/CellType/Numeric.cpp
+++ b/Userland/Applications/Spreadsheet/CellType/Numeric.cpp
@@ -21,7 +21,7 @@ NumericCell::~NumericCell()
 {
 }
 
-String NumericCell::display(Cell& cell, const CellTypeMetadata& metadata) const
+JS::ThrowCompletionOr<String> NumericCell::display(Cell& cell, const CellTypeMetadata& metadata) const
 {
     ScopeGuard propagate_exception { [&cell] {
         if (auto exc = cell.sheet().interpreter().exception()) {
@@ -29,12 +29,12 @@ String NumericCell::display(Cell& cell, const CellTypeMetadata& metadata) const
             cell.set_exception(exc);
         }
     } };
-    auto value = js_value(cell, metadata);
+    auto value = TRY(js_value(cell, metadata));
     String string;
     if (metadata.format.is_empty())
-        string = value.to_string_without_side_effects();
+        string = TRY(value.to_string(cell.sheet().global_object()));
     else
-        string = format_double(metadata.format.characters(), TRY_OR_DISCARD(value.to_double(cell.sheet().global_object())));
+        string = format_double(metadata.format.characters(), TRY(value.to_double(cell.sheet().global_object())));
 
     if (metadata.length >= 0)
         return string.substring(0, metadata.length);
@@ -42,7 +42,7 @@ String NumericCell::display(Cell& cell, const CellTypeMetadata& metadata) const
     return string;
 }
 
-JS::Value NumericCell::js_value(Cell& cell, const CellTypeMetadata&) const
+JS::ThrowCompletionOr<JS::Value> NumericCell::js_value(Cell& cell, const CellTypeMetadata&) const
 {
     ScopeGuard propagate_exception { [&cell] {
         if (auto exc = cell.sheet().interpreter().exception()) {
@@ -50,7 +50,7 @@ JS::Value NumericCell::js_value(Cell& cell, const CellTypeMetadata&) const
             cell.set_exception(exc);
         }
     } };
-    return TRY_OR_DISCARD(cell.js_data().to_number(cell.sheet().global_object()));
+    return cell.js_data().to_number(cell.sheet().global_object());
 }
 
 }
diff --git a/Userland/Applications/Spreadsheet/CellType/Numeric.h b/Userland/Applications/Spreadsheet/CellType/Numeric.h
index eb5a991adc..ae49204e3d 100644
--- a/Userland/Applications/Spreadsheet/CellType/Numeric.h
+++ b/Userland/Applications/Spreadsheet/CellType/Numeric.h
@@ -15,8 +15,8 @@ class NumericCell : public CellType {
 public:
     NumericCell();
     virtual ~NumericCell() override;
-    virtual String display(Cell&, const CellTypeMetadata&) const override;
-    virtual JS::Value js_value(Cell&, const CellTypeMetadata&) const override;
+    virtual JS::ThrowCompletionOr<String> display(Cell&, const CellTypeMetadata&) const override;
+    virtual JS::ThrowCompletionOr<JS::Value> js_value(Cell&, const CellTypeMetadata&) const override;
 };
 
 }
diff --git a/Userland/Applications/Spreadsheet/CellType/String.cpp b/Userland/Applications/Spreadsheet/CellType/String.cpp
index fb1f360fc1..76a9be8782 100644
--- a/Userland/Applications/Spreadsheet/CellType/String.cpp
+++ b/Userland/Applications/Spreadsheet/CellType/String.cpp
@@ -19,18 +19,18 @@ StringCell::~StringCell()
 {
 }
 
-String StringCell::display(Cell& cell, const CellTypeMetadata& metadata) const
+JS::ThrowCompletionOr<String> StringCell::display(Cell& cell, const CellTypeMetadata& metadata) const
 {
-    auto string = cell.js_data().to_string_without_side_effects();
+    auto string = TRY(cell.js_data().to_string(cell.sheet().global_object()));
     if (metadata.length >= 0)
         return string.substring(0, metadata.length);
 
     return string;
 }
 
-JS::Value StringCell::js_value(Cell& cell, const CellTypeMetadata& metadata) const
+JS::ThrowCompletionOr<JS::Value> StringCell::js_value(Cell& cell, const CellTypeMetadata& metadata) const
 {
-    auto string = display(cell, metadata);
+    auto string = TRY(display(cell, metadata));
     return JS::js_string(cell.sheet().interpreter().heap(), string);
 }
 
diff --git a/Userland/Applications/Spreadsheet/CellType/String.h b/Userland/Applications/Spreadsheet/CellType/String.h
index 593bda55e2..15fcca374d 100644
--- a/Userland/Applications/Spreadsheet/CellType/String.h
+++ b/Userland/Applications/Spreadsheet/CellType/String.h
@@ -15,8 +15,8 @@ class StringCell : public CellType {
 public:
     StringCell();
     virtual ~StringCell() override;
-    virtual String display(Cell&, const CellTypeMetadata&) const override;
-    virtual JS::Value js_value(Cell&, const CellTypeMetadata&) const override;
+    virtual JS::ThrowCompletionOr<String> display(Cell&, const CellTypeMetadata&) const override;
+    virtual JS::ThrowCompletionOr<JS::Value> js_value(Cell&, const CellTypeMetadata&) const override;
 };
 
 }
diff --git a/Userland/Applications/Spreadsheet/CellType/Type.h b/Userland/Applications/Spreadsheet/CellType/Type.h
index 025b14650d..89ab6ee11a 100644
--- a/Userland/Applications/Spreadsheet/CellType/Type.h
+++ b/Userland/Applications/Spreadsheet/CellType/Type.h
@@ -28,8 +28,8 @@ public:
     static const CellType* get_by_name(StringView);
     static Vector<StringView> names();
 
-    virtual String display(Cell&, const CellTypeMetadata&) const = 0;
-    virtual JS::Value js_value(Cell&, const CellTypeMetadata&) const = 0;
+    virtual JS::ThrowCompletionOr<String> display(Cell&, const CellTypeMetadata&) const = 0;
+    virtual JS::ThrowCompletionOr<JS::Value> js_value(Cell&, const CellTypeMetadata&) const = 0;
     virtual ~CellType() { }
 
     const String& name() const { return m_name; }
diff --git a/Userland/Applications/Spreadsheet/Spreadsheet.cpp b/Userland/Applications/Spreadsheet/Spreadsheet.cpp
index 83f8296c54..67d8143ba5 100644
--- a/Userland/Applications/Spreadsheet/Spreadsheet.cpp
+++ b/Userland/Applications/Spreadsheet/Spreadsheet.cpp
@@ -598,8 +598,11 @@ Vector<Vector<String>> Sheet::to_xsv() const
         row.resize(column_count);
         for (size_t j = 0; j < column_count; ++j) {
             auto cell = at({ j, i });
-            if (cell)
-                row[j] = cell->typed_display();
+            if (cell) {
+                auto result = cell->typed_display();
+                if (result.has_value())
+                    row[j] = result.value();
+            }
         }
 
         data.append(move(row));
diff --git a/Userland/Applications/Spreadsheet/SpreadsheetModel.cpp b/Userland/Applications/Spreadsheet/SpreadsheetModel.cpp
index 986b992148..86e78bf91f 100644
--- a/Userland/Applications/Spreadsheet/SpreadsheetModel.cpp
+++ b/Userland/Applications/Spreadsheet/SpreadsheetModel.cpp
@@ -27,29 +27,46 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role)
         if (!cell)
             return String::empty();
 
-        if (cell->kind() == Spreadsheet::Cell::Formula) {
-            if (auto exception = cell->exception()) {
-                StringBuilder builder;
-                builder.append("Error: ");
-                auto value = exception->value();
-                if (value.is_object()) {
-                    auto& object = value.as_object();
-                    if (is<JS::Error>(object)) {
-                        auto error = object.get_without_side_effects("message").to_string_without_side_effects();
-                        builder.append(error);
-                        return builder.to_string();
-                    }
+        Function<String(JS::Value)> to_string_as_exception = [&](JS::Value value) {
+            ScopeGuard clear_exception {
+                [&] {
+                    cell->sheet().interpreter().vm().clear_exception();
+                }
+            };
+
+            StringBuilder builder;
+            builder.append("Error: "sv);
+            if (value.is_object()) {
+                auto& object = value.as_object();
+                if (is<JS::Error>(object)) {
+                    auto message = object.get_without_side_effects("message");
+                    auto error = message.to_string(cell->sheet().global_object());
+                    if (error.is_throw_completion())
+                        builder.append(message.to_string_without_side_effects());
+                    else
+                        builder.append(error.release_value());
+                    return builder.to_string();
                 }
-                auto error = value.to_string_without_side_effects();
-                // This is annoying, but whatever.
-                cell->sheet().interpreter().vm().clear_exception();
-
-                builder.append(error);
-                return builder.to_string();
             }
+            auto error_message = value.to_string(cell->sheet().global_object());
+
+            if (error_message.is_throw_completion())
+                return to_string_as_exception(error_message.release_error().value());
+
+            builder.append(error_message.release_value());
+            return builder.to_string();
+        };
+
+        if (cell->kind() == Spreadsheet::Cell::Formula) {
+            if (auto exception = cell->exception())
+                return to_string_as_exception(exception->value());
         }
 
-        return cell->typed_display();
+        auto display = cell->typed_display();
+        if (display.is_error())
+            return to_string_as_exception(display.release_error().value());
+
+        return display.release_value();
     }
 
     if (role == GUI::ModelRole::MimeData)
@@ -155,5 +172,4 @@ void SheetModel::update()
     m_sheet->update();
     did_update(UpdateFlag::DontInvalidateIndices);
 }
-
 }