diff --git a/clients/vscode-hlasmplugin/CHANGELOG.md b/clients/vscode-hlasmplugin/CHANGELOG.md index ff51573ce..525182855 100644 --- a/clients/vscode-hlasmplugin/CHANGELOG.md +++ b/clients/vscode-hlasmplugin/CHANGELOG.md @@ -20,6 +20,7 @@ - Empty arrays now behave similarly to other subscripted variables in macro tracer - Forward structured macro parameters correctly - Empty operands ignored by the SETx instructions +- Incorrect operator precedence in conditional assembly expressions ## [1.1.0](https://github.com/eclipse/che-che4z-lsp-for-hlasm/compare/1.0.0...1.1.0) (2022-03-29) diff --git a/language_server/test/virtual_file_provider_test.cpp b/language_server/test/virtual_file_provider_test.cpp index b8b26e0bb..eb382381b 100644 --- a/language_server/test/virtual_file_provider_test.cpp +++ b/language_server/test/virtual_file_provider_test.cpp @@ -46,7 +46,7 @@ TEST(virtual_file_provider, predicate) TEST(virtual_file_provider, file_missing) { - ws_mngr_mock ws_mngr; + NiceMock ws_mngr; json_sink_mock sink; virtual_file_provider vfp(ws_mngr, sink); @@ -75,7 +75,7 @@ TEST(virtual_file_provider, file_missing) TEST(virtual_file_provider, file_present) { - ws_mngr_mock ws_mngr; + NiceMock ws_mngr; json_sink_mock sink; virtual_file_provider vfp(ws_mngr, sink); diff --git a/parser_library/src/expressions/conditional_assembly/ca_expr_policy.cpp b/parser_library/src/expressions/conditional_assembly/ca_expr_policy.cpp index 03abde8b3..607e8147d 100644 --- a/parser_library/src/expressions/conditional_assembly/ca_expr_policy.cpp +++ b/parser_library/src/expressions/conditional_assembly/ca_expr_policy.cpp @@ -26,13 +26,6 @@ bool ca_character_policy::is_unary(ca_expr_ops op) || op == ca_expr_ops::UPPER; } -bool ca_arithmetic_policy::multiple_words(ca_expr_ops) { return false; } -bool ca_binary_policy::multiple_words(ca_expr_ops op) -{ - return op == ca_expr_ops::AND || op == ca_expr_ops::OR || op == ca_expr_ops::XOR; -} -bool ca_character_policy::multiple_words(ca_expr_ops) { return false; } - bool ca_arithmetic_policy::is_binary(ca_expr_ops op) { return op == ca_expr_ops::SLA || op == ca_expr_ops::SLL || op == ca_expr_ops::SRA || op == ca_expr_ops::SRL @@ -44,8 +37,7 @@ bool ca_binary_policy::is_binary(ca_expr_ops op) { return op == ca_expr_ops::EQ || op == ca_expr_ops::NE || op == ca_expr_ops::LE || op == ca_expr_ops::LT || op == ca_expr_ops::GE || op == ca_expr_ops::GT || op == ca_expr_ops::AND || op == ca_expr_ops::OR - || op == ca_expr_ops::XOR || op == ca_expr_ops::AND_NOT || op == ca_expr_ops::OR_NOT - || op == ca_expr_ops::XOR_NOT; + || op == ca_expr_ops::XOR; } bool ca_character_policy::is_binary(ca_expr_ops) { return false; } @@ -156,13 +148,10 @@ int ca_binary_policy::get_priority(ca_expr_ops op) case ca_expr_ops::NOT: return 1; case ca_expr_ops::AND: - case ca_expr_ops::AND_NOT: return 2; case ca_expr_ops::OR: - case ca_expr_ops::OR_NOT: return 3; case ca_expr_ops::XOR: - case ca_expr_ops::XOR_NOT: return 4; default: return 0; @@ -289,9 +278,6 @@ ca_expr_ops get_expr_operator(const std::string& op) S2O(SRL); S2O(FIND); S2O(INDEX); - S2O(AND_NOT); - S2O(OR_NOT); - S2O(XOR_NOT); S2O(EQ); S2O(NE); S2O(LE); @@ -312,8 +298,54 @@ ca_expr_ops get_expr_operator(const std::string& op) } ca_expr_ops ca_arithmetic_policy::get_operator(const std::string& symbol) { return get_expr_operator(symbol); } +std::variant ca_arithmetic_policy::get_operator_properties( + const std::string& symbol) +{ + auto o = get_operator(symbol); + if (o == ca_expr_ops::UNKNOWN) + return std::monostate(); + bool unary = is_unary(o); + bool binary = is_binary(o); + if (!unary && !binary) + return invalid_by_policy(); + return ca_expr_op { o, get_priority(o), binary, false, unary }; +} + ca_expr_ops ca_binary_policy::get_operator(const std::string& symbol) { return get_expr_operator(symbol); } +std::variant ca_binary_policy::get_operator_properties( + const std::string& symbol) +{ + auto o = get_operator(symbol); + if (o == ca_expr_ops::UNKNOWN) + return std::monostate(); + bool unary = is_unary(o); + bool binary = is_binary(o); + if (!unary && !binary) + return invalid_by_policy(); + return ca_expr_op { + o, + get_priority(o), + binary, + o == ca_expr_ops::EQ || o == ca_expr_ops::NE || o == ca_expr_ops::LE || o == ca_expr_ops::LT + || o == ca_expr_ops::GE || o == ca_expr_ops::GT, + unary, + }; +} + ca_expr_ops ca_character_policy::get_operator(const std::string& symbol) { return get_expr_operator(symbol); } +std::variant ca_character_policy::get_operator_properties( + const std::string& symbol) +{ + auto o = get_operator(symbol); + if (o == ca_expr_ops::UNKNOWN) + return std::monostate(); + bool unary = is_unary(o); + bool binary = is_binary(o); + if (!unary && !binary) + return invalid_by_policy(); + return ca_expr_op { o, get_priority(o), binary, false, unary }; +} + ca_expr_funcs ca_arithmetic_policy::get_function(const std::string& symbol) { diff --git a/parser_library/src/expressions/conditional_assembly/ca_expr_policy.h b/parser_library/src/expressions/conditional_assembly/ca_expr_policy.h index 7484ccb2c..c7693f013 100644 --- a/parser_library/src/expressions/conditional_assembly/ca_expr_policy.h +++ b/parser_library/src/expressions/conditional_assembly/ca_expr_policy.h @@ -15,6 +15,8 @@ #ifndef HLASMPLUGIN_PARSERLIBRARY_CA_EXPR_POLICY_H #define HLASMPLUGIN_PARSERLIBRARY_CA_EXPR_POLICY_H +#include + #include "context/common_types.h" #include "context/id_storage.h" @@ -34,9 +36,6 @@ enum class ca_expr_ops INDEX, // logical - AND_NOT, - OR_NOT, - XOR_NOT, EQ, NE, LE, @@ -60,6 +59,18 @@ enum class ca_expr_ops UNKNOWN }; +struct ca_expr_op +{ + ca_expr_ops op; + int priority; + bool binary; + bool requires_terms; + bool right_assoc; +}; + +struct invalid_by_policy +{}; + enum class ca_expr_funcs { // arithmetic @@ -120,9 +131,6 @@ class ca_arithmetic_policy // is binary arithmetic operation static bool is_binary(ca_expr_ops op); - // true if op can consist of multiple words (eg AND NOT) - static bool multiple_words(ca_expr_ops op); - // get priority relative to rest of arithmetic operators static int get_priority(ca_expr_ops op); @@ -135,6 +143,9 @@ class ca_arithmetic_policy // transforms string operator to enum static ca_expr_ops get_operator(const std::string& symbol); + static std::variant get_operator_properties( + const std::string& symbol); + // transforms string function to enum static ca_expr_funcs get_function(const std::string& symbol); @@ -157,9 +168,6 @@ class ca_binary_policy // is binary binary operation static bool is_binary(ca_expr_ops op); - // true if op can consist of multiple words (eg AND NOT) - static bool multiple_words(ca_expr_ops op); - // get priority relative to rest of binary operators static int get_priority(ca_expr_ops op); @@ -172,6 +180,9 @@ class ca_binary_policy // transforms string operator to enum static ca_expr_ops get_operator(const std::string& symbol); + static std::variant get_operator_properties( + const std::string& symbol); + // transforms string function to enum static ca_expr_funcs get_function(const std::string& symbol); @@ -194,9 +205,6 @@ class ca_character_policy // is binary character operation static bool is_binary(ca_expr_ops op); - // true if op can consist of multiple words (eg AND NOT) - static bool multiple_words(ca_expr_ops op); - // get priority relative to rest of character operators static int get_priority(ca_expr_ops op); @@ -209,6 +217,9 @@ class ca_character_policy // transforms string operator to enum static ca_expr_ops get_operator(const std::string& symbol); + static std::variant get_operator_properties( + const std::string& symbol); + // transforms string function to enum static ca_expr_funcs get_function(const std::string& symbol); diff --git a/parser_library/src/expressions/conditional_assembly/ca_operator_binary.cpp b/parser_library/src/expressions/conditional_assembly/ca_operator_binary.cpp index 63591cb79..d176e9ce3 100644 --- a/parser_library/src/expressions/conditional_assembly/ca_operator_binary.cpp +++ b/parser_library/src/expressions/conditional_assembly/ca_operator_binary.cpp @@ -205,12 +205,6 @@ context::SET_t ca_function_binary_operator::operation( return lhs.access_b() || rhs.access_b(); case ca_expr_ops::XOR: return lhs.access_b() != rhs.access_b(); - case ca_expr_ops::AND_NOT: - return lhs.access_b() && !rhs.access_b(); - case ca_expr_ops::OR_NOT: - return lhs.access_b() || !rhs.access_b(); - case ca_expr_ops::XOR_NOT: - return lhs.access_b() != !rhs.access_b(); default: break; } diff --git a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp index d7a4e619c..4e7cd1fd8 100644 --- a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp +++ b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp @@ -15,6 +15,7 @@ #include "ca_expr_list.h" #include +#include #include "../ca_operator_binary.h" #include "../ca_operator_unary.h" @@ -110,135 +111,213 @@ void ca_expr_list::unknown_functions_to_operators() } } +namespace { + +struct term_entry +{ + ca_expr_ptr term; + size_t i; + bool simple_term; +}; +struct op_entry +{ + size_t i; + ca_expr_ops op_type; + int priority; + bool binary; + bool right_assoc; + bool requires_terms; + range r; +}; + template -void ca_expr_list::resolve(diagnostic_op_consumer& diags) +struct resolve_stacks { - if (expr_list.empty()) - { - diags.add_diagnostic(diagnostic_op::error_CE003(expr_range)); - return; - } + using expr_policy = typename ca_expr_traits::policy_t; - size_t it = 0; - bool err = false; + std::stack terms; + std::stack op_stack; - ca_expr_ptr final_expr = retrieve_term::policy_t>(it, 0, diags); - err |= final_expr == nullptr; + void push_term(term_entry t) { terms.push(std::move(t)); } + void push_op(op_entry op) { op_stack.push(std::move(op)); } - while (it != expr_list.size() && !err) + bool reduce_binary(const op_entry& op, diagnostic_op_consumer& diags) { - auto op_range = expr_list[it]->expr_range; - - auto [prio, op_type] = retrieve_binary_operator::policy_t>(it, err, diags); + auto right = std::move(terms.top()); + terms.pop(); + auto left = std::move(terms.top()); + terms.pop(); - auto r_expr = retrieve_term::policy_t>(++it, prio, diags); - err |= r_expr == nullptr; + if (op.requires_terms && !left.simple_term || left.i > op.i) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.start))); + return false; + } + if (op.requires_terms && !right.simple_term || right.i < op.i) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); + return false; + } - final_expr = std::make_unique( - std::move(final_expr), std::move(r_expr), op_type, context::object_traits::type_enum, op_range); + terms.push({ + std::make_unique( + std::move(left.term), std::move(right.term), op.op_type, context::object_traits::type_enum, op.r), + left.i, + false, + }); + return true; } - if (err) + bool reduce_unary(const op_entry& op, diagnostic_op_consumer& diags) { - expr_list.clear(); - return; - } + auto right = std::move(terms.top()); + terms.pop(); - // resolve created tree - final_expr->resolve_expression_tree(context::object_traits::type_enum, diags); + if (op.requires_terms && !right.simple_term || right.i < op.i) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); + return false; + } - // move resolved tree to the front of the array - expr_list.clear(); - expr_list.emplace_back(std::move(final_expr)); -} + terms.push({ + std::make_unique( + std::move(right.term), op.op_type, expr_policy::set_type, op.r), + op.i, + false, + }); + return true; + } -template -ca_expr_ptr ca_expr_list::retrieve_term(size_t& it, int priority, diagnostic_op_consumer& diags) -{ - // list is exhausted - if (it == expr_list.size()) + bool reduce_stack_entry(diagnostic_op_consumer& diags) { - auto r = expr_list[it - 1]->expr_range; - r.start = r.end; - r.end.column++; - diags.add_diagnostic(diagnostic_op::error_CE003(r)); - return nullptr; + auto op = std::move(op_stack.top()); + op_stack.pop(); + if (terms.size() < 1 + op.binary) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(terms.size() < op.binary ? op.r.start : op.r.end))); + return false; + } + return op.binary ? reduce_binary(op, diags) : reduce_unary(op, diags); } - // first possible term - auto& curr_expr = expr_list[it]; - - // is unary op - if (is_symbol(curr_expr)) + bool reduce_stack(diagnostic_op_consumer& diags, int prio_limit, bool right_assoc) { - if (auto op_type = EXPR_POLICY::get_operator(get_symbol(curr_expr)); EXPR_POLICY::is_unary(op_type)) + while (!op_stack.empty()) { - auto new_expr = retrieve_term(++it, EXPR_POLICY::get_priority(op_type), diags); - if (!new_expr) - return nullptr; - - return std::make_unique( - std::move(new_expr), op_type, EXPR_POLICY::set_type, curr_expr->expr_range); + if (op_stack.top().priority > prio_limit) + break; + if (op_stack.top().priority == prio_limit && right_assoc) + break; + if (!reduce_stack_entry(diags)) + return false; } + return true; } - // is only term - if (it + 1 == expr_list.size()) - return std::move(expr_list[it++]); - - // tries to get binary operator - auto op_it = ++it; - auto op_range = expr_list[op_it]->expr_range; - bool err = false; - - auto [op_prio, op_type] = retrieve_binary_operator(op_it, err, diags); - if (err) - return nullptr; + bool reduce_stack_all(diagnostic_op_consumer& diags) + { + while (!op_stack.empty()) + { + if (!reduce_stack_entry(diags)) + return false; + } + return true; + } +}; - // if operator is of lower priority than the calling operator, finish - if (op_prio >= priority) - return std::move(curr_expr); - else - it = op_it; +} // namespace - auto right_expr = retrieve_term(++it, op_prio, diags); - if (!right_expr) - return nullptr; +template +void ca_expr_list::resolve(diagnostic_op_consumer& diags) +{ + using expr_policy = typename ca_expr_traits::policy_t; - return std::make_unique( - std::move(curr_expr), std::move(right_expr), op_type, EXPR_POLICY::set_type, op_range); -} + resolve_stacks stacks; -template -std::pair ca_expr_list::retrieve_binary_operator(size_t& it, bool& err, diagnostic_op_consumer& diags) -{ - const auto& op = expr_list[it]; + auto i = (size_t)-1; + bool prefer_next_term = true; + bool last_was_terminal = false; + for (auto& curr_expr : expr_list) + { + ++i; + if (is_symbol(curr_expr)) + { + const auto& symbol = get_symbol(curr_expr); + auto op_type_var = expr_policy::get_operator_properties(symbol); + if (std::holds_alternative(op_type_var)) + { + // fallback to term + } + else if (std::holds_alternative(op_type_var)) + { + if (!prefer_next_term) + { + diags.add_diagnostic(diagnostic_op::error_CE002(symbol, curr_expr->expr_range)); + expr_list.clear(); + return; + } + // fallback to term + } + else if (const auto& op_type = std::get(op_type_var); + !(prefer_next_term && op_type.binary)) // ... AND AND is interpreted as AND term, + // ... AND NOT ... is apparently not + { + if (op_type.requires_terms && !last_was_terminal) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(curr_expr->expr_range.start))); + expr_list.clear(); + return; + } + if (!stacks.reduce_stack(diags, op_type.priority, op_type.right_assoc)) + { + expr_list.clear(); + return; + } + stacks.push_op({ + i, + op_type.op, + op_type.priority, + op_type.binary, + op_type.right_assoc, + op_type.requires_terms, + curr_expr->expr_range, + }); + prefer_next_term = op_type.binary || op_type.requires_terms; + last_was_terminal = false; + continue; + } + } + stacks.push_term({ std::move(curr_expr), i, true }); + prefer_next_term = false; + last_was_terminal = true; + } - if (!is_symbol(op)) + if (!stacks.reduce_stack_all(diags)) { - diags.add_diagnostic(diagnostic_op::error_CE001(expr_range)); - err = true; - return std::make_pair(0, ca_expr_ops::UNKNOWN); + expr_list.clear(); + return; } - else if (!EXPR_POLICY::is_operator(EXPR_POLICY::get_operator(get_symbol(op)))) + if (stacks.terms.empty()) { - diags.add_diagnostic(diagnostic_op::error_CE002(get_symbol(op), expr_range)); - err = true; - return std::make_pair(0, ca_expr_ops::UNKNOWN); + diags.add_diagnostic(diagnostic_op::error_CE003(expr_range)); + expr_list.clear(); + return; } - - ca_expr_ops op_type = EXPR_POLICY::get_operator(get_symbol(expr_list[it])); - - if (EXPR_POLICY::multiple_words(op_type) && it + 1 < expr_list.size() && is_symbol(expr_list[it + 1]) - && get_symbol(expr_list[it + 1]) == "NOT") + if (stacks.terms.size() > 1) { - op_type = EXPR_POLICY::get_operator(get_symbol(expr_list[it]) + "_NOT"); - ++it; + diags.add_diagnostic(diagnostic_op::error_CE001(range(stacks.terms.top().term->expr_range))); + expr_list.clear(); + return; } - auto op_prio = EXPR_POLICY::get_priority(op_type); + ca_expr_ptr final_expr = std::move(stacks.terms.top().term); + + // resolve created tree + final_expr->resolve_expression_tree(context::object_traits::type_enum, diags); - return std::make_pair(op_prio, op_type); + // move resolved tree to the front of the array + expr_list.clear(); + expr_list.emplace_back(std::move(final_expr)); } } // namespace hlasm_plugin::parser_library::expressions diff --git a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.h b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.h index 72458bf7b..fa4591f66 100644 --- a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.h +++ b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.h @@ -57,15 +57,6 @@ class ca_expr_list final : public ca_expression // each loop iteration it pastes them together and continue until list is exhausted template void resolve(diagnostic_op_consumer& diags); - // retrieves single term with possible unary operators before it - // also checks for following binary operator, - // if it has higher prio than the current one, recursively calls retrieve_term for the second term for the higher - // priority operator - template - ca_expr_ptr retrieve_term(size_t& it, int priority, diagnostic_op_consumer& diags); - // retrieves following binary operator with its priority - template - std::pair retrieve_binary_operator(size_t& it, bool& err, diagnostic_op_consumer& diags); }; } // namespace hlasm_plugin::parser_library::expressions diff --git a/parser_library/test/expressions/ca_operator_test.cpp b/parser_library/test/expressions/ca_operator_test.cpp index e0cf17f47..c5818b196 100644 --- a/parser_library/test/expressions/ca_operator_test.cpp +++ b/parser_library/test/expressions/ca_operator_test.cpp @@ -117,11 +117,8 @@ INSTANTIATE_TEST_SUITE_P(op_parameters_suite, op_test_param { ca_expr_ops::NOT, SET_t_enum::A_TYPE, { 10 }, -11, "NOTA" }, op_test_param { ca_expr_ops::AND, SET_t_enum::B_TYPE, { false, true }, false, "ANDB" }, - op_test_param { ca_expr_ops::AND_NOT, SET_t_enum::B_TYPE, { true, false }, true, "AND_NOT" }, op_test_param { ca_expr_ops::OR, SET_t_enum::B_TYPE, { false, true }, true, "ORB" }, - op_test_param { ca_expr_ops::OR_NOT, SET_t_enum::B_TYPE, { false, true }, false, "OR_NOT" }, op_test_param { ca_expr_ops::XOR, SET_t_enum::B_TYPE, { false, true }, true, "XORB" }, - op_test_param { ca_expr_ops::XOR_NOT, SET_t_enum::B_TYPE, { true, false }, false, "XOR_NOT" }, op_test_param { ca_expr_ops::NOT, SET_t_enum::B_TYPE, { false }, true, "NOTB" }, op_test_param { ca_expr_ops::EQ, SET_t_enum::B_TYPE, { 1, 0 }, false, "EQA" }, diff --git a/parser_library/test/expressions/logical_expression_test.cpp b/parser_library/test/expressions/logical_expression_test.cpp index a84a322c3..cbd51b70f 100644 --- a/parser_library/test/expressions/logical_expression_test.cpp +++ b/parser_library/test/expressions/logical_expression_test.cpp @@ -273,3 +273,31 @@ TEST(logical_expressions, parenthesis_around_expressions) EXPECT_EQ(diags.size(), (size_t)2); EXPECT_TRUE(std::all_of(diags.begin(), diags.end(), [](const auto& d) { return d.code == "CE016"; })); } + +TEST(logical_expressions, operator_precedence) +{ + for (const auto& [args, expected] : std::initializer_list> { + { "0,0", true }, + { "0,1", true }, + { "1,0", false }, + { "1,1", true }, + }) + { + std::string input = + R"( + MACRO + TEST &A,&B + AIF (&A EQ 0 OR &A EQ 1 AND &B EQ 1).END + MNOTE 8,'ERROR' +.END ANOP + MEND + + TEST )" + + args; + analyzer a(input); + a.analyze(); + a.collect_diags(); + + EXPECT_EQ(a.diags().empty(), expected) << args; + } +}