From f04ada2d4424fa92ef614f48727b2820e36886d8 Mon Sep 17 00:00:00 2001 From: Markus Blatt Date: Sun, 15 Oct 2023 18:49:30 +0200 Subject: [PATCH 1/4] Better error handling for problems in conditions of ACTIONX. When encountering these (e.g. a number instead of an expression on the left hand side) the simulator would immediately abort with an error message like: ``` Error: An error occurred while creating the reservoir schedule Internal error: Extra unhandled data starting with token[0] = 135 Error: Unrecoverable errors while loading input: Extra unhandled data starting with token[0] = 135 ``` (The message above is for the number 135 on the left hand side) With this change we now use the usual way of handling errors and warnings in the parser and continue parsing. The error message for the problem above is now ``` Error: condition of action EX1 has the following error: Left side of comparsion (135) has to be an expression! Error: Problem with keyword ACTIONX In model.schedule line 562 condition of action EX1 has the following error: Left side of comparsion (135) has to be an expression! Error: Unrecoverable errors while loading input: Problem with keyword ACTIONX In model.schedule line 562 condition of action EX1 has the following error: Left side of comparsion (135) has to be an expression! --- opm/input/eclipse/Parser/ParseContext.hpp | 8 +++++++ opm/input/eclipse/Schedule/Action/ActionX.hpp | 3 ++- src/opm/input/eclipse/Parser/ParseContext.cpp | 4 ++++ .../eclipse/Schedule/Action/ActionParser.cpp | 14 ++++++++----- .../input/eclipse/Schedule/Action/ActionX.cpp | 21 +++++++++++++++---- src/opm/input/eclipse/Schedule/Schedule.cpp | 9 +++++++- tests/parser/ACTIONX.cpp | 19 ++++++++++++++++- 7 files changed, 66 insertions(+), 12 deletions(-) diff --git a/opm/input/eclipse/Parser/ParseContext.hpp b/opm/input/eclipse/Parser/ParseContext.hpp index 6b890b8e7..186cc5284 100644 --- a/opm/input/eclipse/Parser/ParseContext.hpp +++ b/opm/input/eclipse/Parser/ParseContext.hpp @@ -314,6 +314,14 @@ class KeywordLocation; */ const static std::string ACTIONX_ILLEGAL_KEYWORD; + /* + Error flag marking parser errors ic ACTIONX conditions + */ + const static std::string ACTIONX_CONDITION_ERROR; + /* + Error flag marking that an ACTIONX has no condition + */ + const static std::string ACTIONX_NO_CONDITION; /* The RPTSCH, RPTSOL and RPTSCHED keywords have two alternative forms, diff --git a/opm/input/eclipse/Schedule/Action/ActionX.hpp b/opm/input/eclipse/Schedule/Action/ActionX.hpp index 62238a470..5b86f7653 100644 --- a/opm/input/eclipse/Schedule/Action/ActionX.hpp +++ b/opm/input/eclipse/Schedule/Action/ActionX.hpp @@ -75,7 +75,8 @@ class ActionX { public: ActionX(); ActionX(const std::string& name, size_t max_run, double max_wait, std::time_t start_time); - ActionX(const DeckKeyword& kw, const Actdims& actimds, std::time_t start_time); + ActionX(const DeckKeyword& kw, const Actdims& actimds, std::time_t start_time, + std::vector>& condition_errors); ActionX(const DeckRecord& record, std::time_t start_time); explicit ActionX(const RestartIO::RstAction& rst_action); diff --git a/src/opm/input/eclipse/Parser/ParseContext.cpp b/src/opm/input/eclipse/Parser/ParseContext.cpp index 2cf4fb499..65f063eb2 100644 --- a/src/opm/input/eclipse/Parser/ParseContext.cpp +++ b/src/opm/input/eclipse/Parser/ParseContext.cpp @@ -114,6 +114,8 @@ namespace Opm { this->addKey(SUMMARY_REGION_TOO_LARGE, InputErrorAction::WARN); addKey(ACTIONX_ILLEGAL_KEYWORD, InputErrorAction::THROW_EXCEPTION); + addKey(ACTIONX_CONDITION_ERROR, InputErrorAction::THROW_EXCEPTION); + addKey(ACTIONX_NO_CONDITION, InputErrorAction::WARN); addKey(RPT_MIXED_STYLE, InputErrorAction::WARN); addKey(RPT_UNKNOWN_MNEMONIC, InputErrorAction::WARN); @@ -369,6 +371,8 @@ namespace Opm { const std::string ParseContext::SCHEDULE_INVALID_NAME = "SCHEDULE_INVALID_NAME"; const std::string ParseContext::ACTIONX_ILLEGAL_KEYWORD = "ACTIONX_ILLEGAL_KEYWORD"; + const std::string ParseContext::ACTIONX_CONDITION_ERROR = "ACTIONX_CONDITION_ERROR"; + const std::string ParseContext::ACTIONX_NO_CONDITION = "ACTIONX_NO_CONDITION"; const std::string ParseContext::SIMULATOR_KEYWORD_NOT_SUPPORTED = "SIMULATOR_KEYWORD_NOT_SUPPORTED"; const std::string ParseContext::SIMULATOR_KEYWORD_NOT_SUPPORTED_CRITICAL = "SIMULATOR_KEYWORD_NOT_SUPPORTED_CRITICAL"; diff --git a/src/opm/input/eclipse/Schedule/Action/ActionParser.cpp b/src/opm/input/eclipse/Schedule/Action/ActionParser.cpp index b7f0caed5..904f497dd 100644 --- a/src/opm/input/eclipse/Schedule/Action/ActionParser.cpp +++ b/src/opm/input/eclipse/Schedule/Action/ActionParser.cpp @@ -128,7 +128,9 @@ ParseNode Parser::current() const { Action::ASTNode Parser::parse_left() { auto current = this->current(); if (current.type != TokenType::ecl_expr) - return TokenType::error; + throw std::invalid_argument("Left side of comparison (" + + current.value + ") has to be " + "an expression!"); std::string func = current.value; FuncType func_type = get_func(current.value); @@ -270,15 +272,17 @@ Action::ASTNode Parser::parse(const std::vector& tokens) { return ASTNode( start_node.type ); auto tree = parser.parse_or(); + + if (tree.type == TokenType::error) + throw std::invalid_argument("Failed to parse ACTIONX condition."); + auto current = parser.current(); if (current.type != TokenType::end) { size_t index = parser.current_pos; - throw std::invalid_argument("Extra unhandled data starting with token[" + std::to_string(index) + "] = " + current.value); + throw std::invalid_argument("Extra unhandled data starting with token[" + std::to_string(index) + "] = " + current.value+ + " in ACTIONX condition."); } - if (tree.type == TokenType::error) - throw std::invalid_argument("Failed to parse"); - return tree; } } diff --git a/src/opm/input/eclipse/Schedule/Action/ActionX.cpp b/src/opm/input/eclipse/Schedule/Action/ActionX.cpp index b9554a2e5..ee47618d0 100755 --- a/src/opm/input/eclipse/Schedule/Action/ActionX.cpp +++ b/src/opm/input/eclipse/Schedule/Action/ActionX.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -119,7 +120,7 @@ ActionX::ActionX(const DeckRecord& record, std::time_t start_time) : -ActionX::ActionX(const DeckKeyword& kw, const Actdims& actdims, std::time_t start_time) : +ActionX::ActionX(const DeckKeyword& kw, const Actdims& actdims, std::time_t start_time, std::vector>& condition_errors) : ActionX(kw.getRecord(0), start_time) { std::vector tokens; @@ -132,10 +133,22 @@ ActionX::ActionX(const DeckKeyword& kw, const Actdims& actdims, std::time_t star this->m_conditions.emplace_back(cond_tokens, kw.location()); } - if (this->m_conditions.size() > actdims.max_conditions()) - throw OpmInputError(fmt::format("Action {} has too many conditions - adjust item 4 of ACTDIMS to at least {}", this->name(), this->m_conditions.size()), kw.location()); + if (this->m_conditions.empty()) + condition_errors.push_back({ParseContext::ACTIONX_NO_CONDITION, fmt::format("Action {} is missing a condition.", this->name())}); - this->condition = Action::AST(tokens); + if (this->m_conditions.size() > actdims.max_conditions()) + condition_errors.push_back({ ParseContext::ACTIONX_CONDITION_ERROR, + fmt::format("Action {} has too many conditions - adjust item 4 of ACTDIMS to at least {}", this->name(), this->m_conditions.size())}); + + try + { + this->condition = Action::AST(tokens); + } + catch(const std::invalid_argument& e) + { + condition_errors.push_back({ ParseContext::ACTIONX_CONDITION_ERROR, + fmt::format("condition of action {} has the following error: {}", this->name(), e.what())}); + } } diff --git a/src/opm/input/eclipse/Schedule/Schedule.cpp b/src/opm/input/eclipse/Schedule/Schedule.cpp index eee5219be..e3addcb02 100644 --- a/src/opm/input/eclipse/Schedule/Schedule.cpp +++ b/src/opm/input/eclipse/Schedule/Schedule.cpp @@ -665,9 +665,16 @@ void Schedule::iterateScheduleSection(std::size_t load_start, std::size_t load_e logger.location(location); if (keyword.is()) { + std::vector> condition_errors; //condition is parsed by ActionX constructor Action::ActionX action(keyword, this->m_static.m_runspec.actdims(), - std::chrono::system_clock::to_time_t(this->snapshots[report_step].start_time())); + std::chrono::system_clock::to_time_t(this->snapshots[report_step].start_time()), + condition_errors); + + for(const auto& [ marker, msg]: condition_errors) { + parseContext.handleError(marker, msg, keyword.location(), errors); + } + while (true) { keyword_index++; if (keyword_index == block.size()) diff --git a/tests/parser/ACTIONX.cpp b/tests/parser/ACTIONX.cpp index b4c05129f..dbde5205e 100644 --- a/tests/parser/ACTIONX.cpp +++ b/tests/parser/ACTIONX.cpp @@ -87,8 +87,25 @@ ACTIONX const auto deck = Parser{}.parseString( action_kw ); const auto& kw = deck["ACTIONX"].back(); - Action::ActionX action2(kw, {}, 0); + + std::vector> condition_errors; + Action::ActionX action2(kw, {}, 0, condition_errors); BOOST_CHECK_EQUAL(action2.name(), "ACTION"); + + // left hand side has to be an expression. + // Check whether we add an error to condition_errors + // if that is not the case + const auto action_kw_num_first = std::string{ R"( +ACTIONX + 'ACTION' / + 0.75 < WWCT OPX / +/ +)"}; + + condition_errors.clear(); + const auto deck1 = Parser{}.parseString( action_kw_num_first); + const auto action3 = Action::ActionX(deck1["ACTIONX"].back(), {}, 0, condition_errors); + BOOST_CHECK_EQUAL(condition_errors.size(), 1U); } From 25de42037f4f7b60d162ef8b539a04ff3c85ba76 Mon Sep 17 00:00:00 2001 From: Markus Blatt Date: Wed, 25 Oct 2023 13:55:24 +0200 Subject: [PATCH 2/4] Introduces method parseActionX returning the action and error strings. This way there is no constructor with an output parameter and we prevent introducing an additional member in ActionX that is only used in one constructor. --- opm/input/eclipse/Schedule/Action/ActionX.hpp | 15 +++- .../input/eclipse/Schedule/Action/ActionX.cpp | 81 ++++++++++++------- src/opm/input/eclipse/Schedule/Schedule.cpp | 9 +-- tests/parser/ACTIONX.cpp | 12 +-- 4 files changed, 75 insertions(+), 42 deletions(-) diff --git a/opm/input/eclipse/Schedule/Action/ActionX.hpp b/opm/input/eclipse/Schedule/Action/ActionX.hpp index 5b86f7653..7f5a4a44e 100644 --- a/opm/input/eclipse/Schedule/Action/ActionX.hpp +++ b/opm/input/eclipse/Schedule/Action/ActionX.hpp @@ -75,8 +75,10 @@ class ActionX { public: ActionX(); ActionX(const std::string& name, size_t max_run, double max_wait, std::time_t start_time); - ActionX(const DeckKeyword& kw, const Actdims& actimds, std::time_t start_time, - std::vector>& condition_errors); + ActionX(const std::string& name, size_t max_run, double max_wait, + std::time_t start_time, + const std::vector&& conditions, + const std::vector&& tokens); ActionX(const DeckRecord& record, std::time_t start_time); explicit ActionX(const RestartIO::RstAction& rst_action); @@ -132,6 +134,15 @@ private: std::vector m_conditions; }; +/// \brief Parse ActionX keyword. +/// \param kw The keyword representation of ActionX +/// \param actdims Dimensions for ActionX as specified in the deck. +/// \param start_time The first time that the ActionX should be evaluated +/// \return A tuple of the ActionX created and a vector that contains for each error experienced +/// during parsing the string indicating the type of error and the error string itself. +std::tuple>> +parseActionX(const DeckKeyword& kw, const Actdims& actimds, std::time_t start_time); + } } #endif /* WELL_HPP_ */ diff --git a/src/opm/input/eclipse/Schedule/Action/ActionX.cpp b/src/opm/input/eclipse/Schedule/Action/ActionX.cpp index ee47618d0..208a1795f 100755 --- a/src/opm/input/eclipse/Schedule/Action/ActionX.cpp +++ b/src/opm/input/eclipse/Schedule/Action/ActionX.cpp @@ -74,6 +74,50 @@ bool ActionX::valid_keyword(const std::string& keyword) { return (actionx_allowed_list.find(keyword) != actionx_allowed_list.end()); } +std::tuple>> +parseActionX(const DeckKeyword& kw, const Actdims& actdims, + std::time_t start_time) +{ + std::vector> condition_errors; + std::vector tokens; + std::vector conditions; + auto record = kw.getRecord(0); + const std::string name = record.getItem("NAME").getTrimmedString(0); + + for (size_t record_index = 1; record_index < kw.size(); record_index++) { + const auto& record = kw.getRecord(record_index); + const auto& cond_tokens = RawString::strings( record.getItem("CONDITION").getData() ); + + for (const auto& token : cond_tokens) + tokens.push_back(dequote(token, kw.location())); + + conditions.emplace_back(cond_tokens, kw.location()); + } + if (conditions.empty()) + condition_errors.push_back({ParseContext::ACTIONX_NO_CONDITION, + fmt::format("Action {} is missing a condition.", name)}); + + if (conditions.size() > actdims.max_conditions()) + condition_errors.push_back({ ParseContext::ACTIONX_CONDITION_ERROR, + fmt::format("Action {} has too many conditions - adjust item " + "4 of ACTDIMS to at least {}", + name, conditions.size())}); + + try + { + return { ActionX(name, + record.getItem("NUM").get(0), + record.getItem("MIN_WAIT").getSIDouble(0), + start_time, std::move(conditions), std::move(tokens)), + condition_errors}; + } + catch(const std::invalid_argument& e) + { + condition_errors.push_back({ ParseContext::ACTIONX_CONDITION_ERROR, + fmt::format("condition of action {} has the following error: {}", name, e.what())}); + return {ActionX(kw.getRecord(0), start_time), condition_errors}; + } +} ActionX::ActionX() : m_start_time(0) @@ -120,36 +164,13 @@ ActionX::ActionX(const DeckRecord& record, std::time_t start_time) : -ActionX::ActionX(const DeckKeyword& kw, const Actdims& actdims, std::time_t start_time, std::vector>& condition_errors) : - ActionX(kw.getRecord(0), start_time) -{ - std::vector tokens; - for (size_t record_index = 1; record_index < kw.size(); record_index++) { - const auto& record = kw.getRecord(record_index); - const auto& cond_tokens = RawString::strings( record.getItem("CONDITION").getData() ); - - for (const auto& token : cond_tokens) - tokens.push_back(dequote(token, kw.location())); - - this->m_conditions.emplace_back(cond_tokens, kw.location()); - } - if (this->m_conditions.empty()) - condition_errors.push_back({ParseContext::ACTIONX_NO_CONDITION, fmt::format("Action {} is missing a condition.", this->name())}); - - if (this->m_conditions.size() > actdims.max_conditions()) - condition_errors.push_back({ ParseContext::ACTIONX_CONDITION_ERROR, - fmt::format("Action {} has too many conditions - adjust item 4 of ACTDIMS to at least {}", this->name(), this->m_conditions.size())}); - - try - { - this->condition = Action::AST(tokens); - } - catch(const std::invalid_argument& e) - { - condition_errors.push_back({ ParseContext::ACTIONX_CONDITION_ERROR, - fmt::format("condition of action {} has the following error: {}", this->name(), e.what())}); - } -} +ActionX::ActionX(const std::string& name, size_t max_run, double min_wait, + std::time_t start_time, + const std::vector&& conditions, + const std::vector&& tokens) + : m_name(name), m_max_run(max_run), m_min_wait(min_wait), + m_start_time(start_time), condition(tokens), m_conditions(conditions) +{} ActionX ActionX::serializationTestObject() diff --git a/src/opm/input/eclipse/Schedule/Schedule.cpp b/src/opm/input/eclipse/Schedule/Schedule.cpp index e3addcb02..c684bce3e 100644 --- a/src/opm/input/eclipse/Schedule/Schedule.cpp +++ b/src/opm/input/eclipse/Schedule/Schedule.cpp @@ -665,11 +665,10 @@ void Schedule::iterateScheduleSection(std::size_t load_start, std::size_t load_e logger.location(location); if (keyword.is()) { - std::vector> condition_errors; //condition is parsed by ActionX constructor - Action::ActionX action(keyword, - this->m_static.m_runspec.actdims(), - std::chrono::system_clock::to_time_t(this->snapshots[report_step].start_time()), - condition_errors); + auto [action, condition_errors] = + Action::parseActionX(keyword, + this->m_static.m_runspec.actdims(), + std::chrono::system_clock::to_time_t(this->snapshots[report_step].start_time())); for(const auto& [ marker, msg]: condition_errors) { parseContext.handleError(marker, msg, keyword.location(), errors); diff --git a/tests/parser/ACTIONX.cpp b/tests/parser/ACTIONX.cpp index dbde5205e..ee5eb3152 100644 --- a/tests/parser/ACTIONX.cpp +++ b/tests/parser/ACTIONX.cpp @@ -88,9 +88,10 @@ ACTIONX const auto& kw = deck["ACTIONX"].back(); - std::vector> condition_errors; - Action::ActionX action2(kw, {}, 0, condition_errors); + const auto& [action2, condition_errors2 ] = + Action::parseActionX(kw, {}, 0); BOOST_CHECK_EQUAL(action2.name(), "ACTION"); + BOOST_CHECK_EQUAL(condition_errors2.size(), 0U); // left hand side has to be an expression. // Check whether we add an error to condition_errors @@ -102,10 +103,11 @@ ACTIONX / )"}; - condition_errors.clear(); + const auto deck1 = Parser{}.parseString( action_kw_num_first); - const auto action3 = Action::ActionX(deck1["ACTIONX"].back(), {}, 0, condition_errors); - BOOST_CHECK_EQUAL(condition_errors.size(), 1U); + const auto& [action3, condition_errors3] = + Action::parseActionX(deck1["ACTIONX"].back(), {}, 0); + BOOST_CHECK_EQUAL(condition_errors3.size(), 1U); } From 89f14594e1bd84c792f8ee0529c3ba4d8a6d0e7e Mon Sep 17 00:00:00 2001 From: Markus Blatt Date: Wed, 25 Oct 2023 14:00:37 +0200 Subject: [PATCH 3/4] Improved the "improved" error message even more. --- src/opm/input/eclipse/Schedule/Action/ActionParser.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/opm/input/eclipse/Schedule/Action/ActionParser.cpp b/src/opm/input/eclipse/Schedule/Action/ActionParser.cpp index 904f497dd..638091913 100644 --- a/src/opm/input/eclipse/Schedule/Action/ActionParser.cpp +++ b/src/opm/input/eclipse/Schedule/Action/ActionParser.cpp @@ -22,6 +22,8 @@ #include #include +#include + #include #include "ActionParser.hpp" @@ -128,9 +130,9 @@ ParseNode Parser::current() const { Action::ASTNode Parser::parse_left() { auto current = this->current(); if (current.type != TokenType::ecl_expr) - throw std::invalid_argument("Left side of comparison (" - + current.value + ") has to be " - "an expression!"); + throw std::invalid_argument(fmt::format("Expected expression as left hand side " + "of comparison, but got {} instead.", + current.value)); std::string func = current.value; FuncType func_type = get_func(current.value); From 574734680f272cdc8f50d5e7c806416c72638349 Mon Sep 17 00:00:00 2001 From: Markus Blatt Date: Wed, 25 Oct 2023 15:30:44 +0200 Subject: [PATCH 4/4] Fixed shadowing variable warning. --- src/opm/input/eclipse/Schedule/Action/ActionX.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/opm/input/eclipse/Schedule/Action/ActionX.cpp b/src/opm/input/eclipse/Schedule/Action/ActionX.cpp index 208a1795f..8216b97d0 100755 --- a/src/opm/input/eclipse/Schedule/Action/ActionX.cpp +++ b/src/opm/input/eclipse/Schedule/Action/ActionX.cpp @@ -85,8 +85,8 @@ parseActionX(const DeckKeyword& kw, const Actdims& actdims, const std::string name = record.getItem("NAME").getTrimmedString(0); for (size_t record_index = 1; record_index < kw.size(); record_index++) { - const auto& record = kw.getRecord(record_index); - const auto& cond_tokens = RawString::strings( record.getItem("CONDITION").getData() ); + const auto& cond_tokens = RawString::strings( kw.getRecord(record_index) + .getItem("CONDITION").getData() ); for (const auto& token : cond_tokens) tokens.push_back(dequote(token, kw.location()));