From 4ef1e8f226caff2a0d9002cb5e30fbd7bc99b7b3 Mon Sep 17 00:00:00 2001 From: davidot Date: Mon, 7 Feb 2022 15:00:40 +0100 Subject: [PATCH] Spreadsheet: No longer use vm.exception() to signal exception state Instead, use the completions which are returned directly. This means we no longer have to worry about the global VM state when running code. --- Userland/Applications/Spreadsheet/Cell.cpp | 26 +++++++------- Userland/Applications/Spreadsheet/Cell.h | 11 ++++-- .../Spreadsheet/CellSyntaxHighlighter.cpp | 33 +++++++++-------- .../Spreadsheet/CellType/Date.cpp | 18 ++++------ .../Applications/Spreadsheet/CellType/Date.h | 1 + .../Spreadsheet/CellType/Numeric.cpp | 36 ++++++++----------- .../Spreadsheet/CellType/Numeric.h | 11 ++++++ .../Spreadsheet/JSIntegration.cpp | 5 +-- .../Applications/Spreadsheet/Spreadsheet.cpp | 36 +++++++++---------- .../Applications/Spreadsheet/Spreadsheet.h | 6 +--- .../Spreadsheet/SpreadsheetModel.cpp | 12 ++----- 11 files changed, 98 insertions(+), 97 deletions(-) diff --git a/Userland/Applications/Spreadsheet/Cell.cpp b/Userland/Applications/Spreadsheet/Cell.cpp index 9a1a4816c25..32e98270e29 100644 --- a/Userland/Applications/Spreadsheet/Cell.cpp +++ b/Userland/Applications/Spreadsheet/Cell.cpp @@ -96,15 +96,18 @@ void Cell::update_data(Badge) if (!m_dirty) return; - m_js_exception = {}; - if (m_dirty) { m_dirty = false; if (m_kind == Formula) { if (!m_evaluated_externally) { - auto [value, exception] = m_sheet->evaluate(m_data, this); - m_evaluated_data = value; - m_js_exception = move(exception); + auto value_or_error = m_sheet->evaluate(m_data, this); + if (value_or_error.is_error()) { + m_evaluated_data = JS::js_undefined(); + m_thrown_value = *value_or_error.release_error().release_value(); + } else { + m_evaluated_data = value_or_error.release_value(); + m_thrown_value = {}; + } } } @@ -118,14 +121,14 @@ void Cell::update_data(Badge) m_evaluated_formats.background_color.clear(); m_evaluated_formats.foreground_color.clear(); - if (!m_js_exception) { + if (m_thrown_value.is_empty()) { for (auto& fmt : m_conditional_formats) { if (!fmt.condition.is_empty()) { - auto [value, exception] = m_sheet->evaluate(fmt.condition, this); - if (exception) { - m_js_exception = move(exception); + auto value_or_error = m_sheet->evaluate(fmt.condition, this); + if (value_or_error.is_error()) { + m_thrown_value = *value_or_error.release_error().release_value(); } else { - if (value.to_boolean()) { + if (value_or_error.release_value().to_boolean()) { if (fmt.background_color.has_value()) m_evaluated_formats.background_color = fmt.background_color; if (fmt.foreground_color.has_value()) @@ -185,8 +188,7 @@ void Cell::copy_from(const Cell& other) m_type_metadata = other.m_type_metadata; m_conditional_formats = other.m_conditional_formats; m_evaluated_formats = other.m_evaluated_formats; - if (!other.m_js_exception) - m_js_exception = other.m_js_exception; + m_thrown_value = other.m_thrown_value; } } diff --git a/Userland/Applications/Spreadsheet/Cell.h b/Userland/Applications/Spreadsheet/Cell.h index 1e8830084e4..231bb92738b 100644 --- a/Userland/Applications/Spreadsheet/Cell.h +++ b/Userland/Applications/Spreadsheet/Cell.h @@ -49,8 +49,13 @@ struct Cell : public Weakable { bool dirty() const { return m_dirty; } void clear_dirty() { m_dirty = false; } - void set_exception(JS::Exception* exc) { m_js_exception = exc; } - JS::Exception* exception() const { return m_js_exception; } + void set_thrown_value(JS::Value value) { m_thrown_value = value; } + Optional thrown_value() const + { + if (m_thrown_value.is_empty()) + return {}; + return m_thrown_value; + } const String& data() const { return m_data; } const JS::Value& evaluated_data() const { return m_evaluated_data; } @@ -103,7 +108,7 @@ private: bool m_evaluated_externally { false }; String m_data; JS::Value m_evaluated_data; - JS::Exception* m_js_exception { nullptr }; + JS::Value m_thrown_value; Kind m_kind { LiteralString }; WeakPtr m_sheet; Vector> m_referencing_cells; diff --git a/Userland/Applications/Spreadsheet/CellSyntaxHighlighter.cpp b/Userland/Applications/Spreadsheet/CellSyntaxHighlighter.cpp index 533d17d1087..c6bdde6bea4 100644 --- a/Userland/Applications/Spreadsheet/CellSyntaxHighlighter.cpp +++ b/Userland/Applications/Spreadsheet/CellSyntaxHighlighter.cpp @@ -34,21 +34,24 @@ void CellSyntaxHighlighter::rehighlight(const Palette& palette) (u64)-1, false); - if (m_cell && m_cell->exception()) { - auto& traceback = m_cell->exception()->traceback(); - auto& range = traceback.first().source_range; - GUI::TextRange text_range { { range.start.line - 1, range.start.column }, { range.end.line - 1, range.end.column } }; - m_client->spans().prepend( - GUI::TextDocumentSpan { - text_range, - Gfx::TextAttributes { - Color::Black, - Color::Red, - false, - false, - }, - (u64)-1, - false }); + if (m_cell && m_cell->thrown_value().has_value()) { + if (auto value = m_cell->thrown_value().value(); value.is_object() && is(value.as_object())) { + auto& error = static_cast(value.as_object()); + auto& traceback = error.traceback(); + auto& range = traceback.first().source_range; + GUI::TextRange text_range { { range.start.line - 1, range.start.column }, { range.end.line - 1, range.end.column } }; + m_client->spans().prepend( + GUI::TextDocumentSpan { + text_range, + Gfx::TextAttributes { + Color::Black, + Color::Red, + false, + false, + }, + (u64)-1, + false }); + } } m_client->do_update(); } diff --git a/Userland/Applications/Spreadsheet/CellType/Date.cpp b/Userland/Applications/Spreadsheet/CellType/Date.cpp index 9714ebb47d4..65ce76d6a95 100644 --- a/Userland/Applications/Spreadsheet/CellType/Date.cpp +++ b/Userland/Applications/Spreadsheet/CellType/Date.cpp @@ -23,19 +23,15 @@ DateCell::~DateCell() JS::ThrowCompletionOr DateCell::display(Cell& cell, const CellTypeMetadata& metadata) const { - ScopeGuard propagate_exception { [&cell] { - if (auto exc = cell.sheet().interpreter().exception()) { - cell.sheet().interpreter().vm().clear_exception(); - cell.set_exception(exc); - } - } }; - 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()); + return propagate_failure(cell, [&]() -> JS::ThrowCompletionOr { + 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); + if (metadata.length >= 0) + return string.substring(0, metadata.length); - return string; + return string; + }); } JS::ThrowCompletionOr DateCell::js_value(Cell& cell, const CellTypeMetadata&) const diff --git a/Userland/Applications/Spreadsheet/CellType/Date.h b/Userland/Applications/Spreadsheet/CellType/Date.h index b39583c3710..41f6b3ec46e 100644 --- a/Userland/Applications/Spreadsheet/CellType/Date.h +++ b/Userland/Applications/Spreadsheet/CellType/Date.h @@ -6,6 +6,7 @@ #pragma once +#include "Numeric.h" #include "Type.h" namespace Spreadsheet { diff --git a/Userland/Applications/Spreadsheet/CellType/Numeric.cpp b/Userland/Applications/Spreadsheet/CellType/Numeric.cpp index 9ebe1d8aa9c..186eea277e6 100644 --- a/Userland/Applications/Spreadsheet/CellType/Numeric.cpp +++ b/Userland/Applications/Spreadsheet/CellType/Numeric.cpp @@ -23,34 +23,26 @@ NumericCell::~NumericCell() JS::ThrowCompletionOr NumericCell::display(Cell& cell, const CellTypeMetadata& metadata) const { - ScopeGuard propagate_exception { [&cell] { - if (auto exc = cell.sheet().interpreter().exception()) { - cell.sheet().interpreter().vm().clear_exception(); - cell.set_exception(exc); - } - } }; - auto value = TRY(js_value(cell, metadata)); - String string; - if (metadata.format.is_empty()) - string = TRY(value.to_string(cell.sheet().global_object())); - else - string = format_double(metadata.format.characters(), TRY(value.to_double(cell.sheet().global_object()))); + return propagate_failure(cell, [&]() -> JS::ThrowCompletionOr { + auto value = TRY(js_value(cell, metadata)); + String string; + if (metadata.format.is_empty()) + string = TRY(value.to_string(cell.sheet().global_object())); + else + string = format_double(metadata.format.characters(), TRY(value.to_double(cell.sheet().global_object()))); - if (metadata.length >= 0) - return string.substring(0, metadata.length); + if (metadata.length >= 0) + return string.substring(0, metadata.length); - return string; + return string; + }); } JS::ThrowCompletionOr NumericCell::js_value(Cell& cell, const CellTypeMetadata&) const { - ScopeGuard propagate_exception { [&cell] { - if (auto exc = cell.sheet().interpreter().exception()) { - cell.sheet().interpreter().vm().clear_exception(); - cell.set_exception(exc); - } - } }; - return cell.js_data().to_number(cell.sheet().global_object()); + return propagate_failure(cell, [&]() { + 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 ae49204e3de..928bdf6d812 100644 --- a/Userland/Applications/Spreadsheet/CellType/Numeric.h +++ b/Userland/Applications/Spreadsheet/CellType/Numeric.h @@ -6,10 +6,21 @@ #pragma once +#include "../Cell.h" #include "Type.h" namespace Spreadsheet { +template +static auto propagate_failure(Cell& cell, Callable&& steps) +{ + auto result_or_error = steps(); + if (result_or_error.is_error()) + cell.set_thrown_value(*result_or_error.throw_completion().value()); + + return result_or_error; +} + class NumericCell : public CellType { public: diff --git a/Userland/Applications/Spreadsheet/JSIntegration.cpp b/Userland/Applications/Spreadsheet/JSIntegration.cpp index 5ddc2ffb468..d4dda776aab 100644 --- a/Userland/Applications/Spreadsheet/JSIntegration.cpp +++ b/Userland/Applications/Spreadsheet/JSIntegration.cpp @@ -164,8 +164,9 @@ void SheetGlobalObject::visit_edges(Visitor& visitor) { Base::visit_edges(visitor); for (auto& it : m_sheet.cells()) { - if (it.value->exception()) - visitor.visit(it.value->exception()); + if (auto opt_thrown_value = it.value->thrown_value(); opt_thrown_value.has_value()) + visitor.visit(*opt_thrown_value); + visitor.visit(it.value->evaluated_data()); } } diff --git a/Userland/Applications/Spreadsheet/Spreadsheet.cpp b/Userland/Applications/Spreadsheet/Spreadsheet.cpp index cc0a3721b87..4c28433d0ec 100644 --- a/Userland/Applications/Spreadsheet/Spreadsheet.cpp +++ b/Userland/Applications/Spreadsheet/Spreadsheet.cpp @@ -60,15 +60,22 @@ Sheet::Sheet(Workbook& workbook) warnln("SyntaxError: {}", error.to_string()); } } else { - (void)interpreter().run(script_or_error.value()); - if (auto* exception = interpreter().exception()) { + auto result = interpreter().run(script_or_error.value()); + if (result.is_error()) { warnln("Spreadsheet: Failed to run runtime code:"); - for (auto& traceback_frame : exception->traceback()) { - auto& function_name = traceback_frame.function_name; - auto& source_range = traceback_frame.source_range; - dbgln(" {} at {}:{}:{}", function_name, source_range.filename, source_range.start.line, source_range.start.column); + auto thrown_value = *result.throw_completion().value(); + warn("Threw: {}", thrown_value.to_string_without_side_effects()); + if (thrown_value.is_object() && is(thrown_value.as_object())) { + auto& error = static_cast(thrown_value.as_object()); + warnln(" with message '{}'", error.get_without_side_effects(interpreter().vm().names.message)); + for (auto& traceback_frame : error.traceback()) { + auto& function_name = traceback_frame.function_name; + auto& source_range = traceback_frame.source_range; + dbgln(" {} at {}:{}:{}", function_name, source_range.filename, source_range.start.line, source_range.start.column); + } + } else { + warnln(); } - interpreter().vm().clear_exception(); } } } @@ -159,22 +166,15 @@ void Sheet::update(Cell& cell) } } -Sheet::ValueAndException Sheet::evaluate(StringView source, Cell* on_behalf_of) +JS::ThrowCompletionOr Sheet::evaluate(StringView source, Cell* on_behalf_of) { TemporaryChange cell_change { m_current_cell_being_evaluated, on_behalf_of }; - ScopeGuard clear_exception { [&] { interpreter().vm().clear_exception(); } }; auto script_or_error = JS::Script::parse(source, interpreter().realm()); - // FIXME: Convert parser errors to exceptions to show them to the user. - if (script_or_error.is_error() || interpreter().exception()) - return { JS::js_undefined(), interpreter().exception() }; + if (script_or_error.is_error()) + return interpreter().vm().throw_completion(interpreter().global_object(), script_or_error.error().first().to_string()); - auto result = interpreter().run(script_or_error.value()); - if (result.is_error()) { - auto* exc = interpreter().exception(); - return { JS::js_undefined(), exc }; - } - return { result.value(), {} }; + return interpreter().run(script_or_error.value()); } Cell* Sheet::at(StringView name) diff --git a/Userland/Applications/Spreadsheet/Spreadsheet.h b/Userland/Applications/Spreadsheet/Spreadsheet.h index 213131185df..0d4a36c5e1c 100644 --- a/Userland/Applications/Spreadsheet/Spreadsheet.h +++ b/Userland/Applications/Spreadsheet/Spreadsheet.h @@ -107,11 +107,7 @@ public: } } - struct ValueAndException { - JS::Value value; - JS::Exception* exception { nullptr }; - }; - ValueAndException evaluate(StringView, Cell* = nullptr); + JS::ThrowCompletionOr evaluate(StringView, Cell* = nullptr); JS::Interpreter& interpreter() const; SheetGlobalObject& global_object() const { return *m_global_object; } diff --git a/Userland/Applications/Spreadsheet/SpreadsheetModel.cpp b/Userland/Applications/Spreadsheet/SpreadsheetModel.cpp index 8be1f77dc7d..8d97b575168 100644 --- a/Userland/Applications/Spreadsheet/SpreadsheetModel.cpp +++ b/Userland/Applications/Spreadsheet/SpreadsheetModel.cpp @@ -28,12 +28,6 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) return String::empty(); Function 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()) { @@ -58,8 +52,8 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) }; if (cell->kind() == Spreadsheet::Cell::Formula) { - if (auto exception = cell->exception()) - return to_string_as_exception(exception->value()); + if (auto opt_throw_value = cell->thrown_value(); opt_throw_value.has_value()) + return to_string_as_exception(*opt_throw_value); } auto display = cell->typed_display(); @@ -86,7 +80,7 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) return {}; if (cell->kind() == Spreadsheet::Cell::Formula) { - if (cell->exception()) + if (cell->thrown_value().has_value()) return Color(Color::Red); }