diff --git a/CHANGELOG.md b/CHANGELOG.md index d8d63895b..89be5f4a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - special error message when trying to use a hidden symbol due to importing ### Changed +- avoid generating `LOAD_FAST_BY_INDEX` instructions when trying to fetch a potentially unreachable variable ### Removed diff --git a/include/Ark/Compiler/Lowerer/LocalsLocator.hpp b/include/Ark/Compiler/Lowerer/LocalsLocator.hpp index f589a778d..6972bd9de 100644 --- a/include/Ark/Compiler/Lowerer/LocalsLocator.hpp +++ b/include/Ark/Compiler/Lowerer/LocalsLocator.hpp @@ -66,13 +66,27 @@ namespace Ark::internal /** * @brief Drop potentially defined variables in the last saved branch + * + * @return true if vars were dropped, meaning that we created at least one variable in a branch + * @return false if nothing was dropped */ - void dropVarsForBranch(); + bool dropVarsForBranch(); + + /** + * @brief Mark the last variable of a scope as unreachable, blocking lookupLastScopeByName(..) from returning a value past that point + */ + void markLastLocalAsUnreachable(); private: struct Scope { - std::vector data; + struct Var + { + std::string name; + bool unreachable = false; + }; + + std::vector data; ScopeType type; }; diff --git a/src/arkreactor/Compiler/Lowerer/ASTLowerer.cpp b/src/arkreactor/Compiler/Lowerer/ASTLowerer.cpp index 2cde165c9..18ebec92d 100644 --- a/src/arkreactor/Compiler/Lowerer/ASTLowerer.cpp +++ b/src/arkreactor/Compiler/Lowerer/ASTLowerer.cpp @@ -474,13 +474,15 @@ namespace Ark::internal const auto label_then = IR::Entity::Label(m_current_label++); page(p).emplace_back(IR::Entity::GotoIf(label_then, true)); + bool created_vars = false; + // "false" branch code if (x.constList().size() == 4) // we have an else clause { m_locals_locator.saveScopeLengthForBranch(); compileExpression(x.list()[3], p, is_result_unused, is_terminal, can_use_ref); page(p).back().setSourceLocation(x.constList()[3].filename(), x.constList()[3].position().start.line); - m_locals_locator.dropVarsForBranch(); + created_vars = m_locals_locator.dropVarsForBranch(); } else { @@ -498,9 +500,15 @@ namespace Ark::internal m_locals_locator.saveScopeLengthForBranch(); compileExpression(x.list()[2], p, is_result_unused, is_terminal, can_use_ref); page(p).back().setSourceLocation(x.constList()[2].filename(), x.constList()[2].position().start.line); - m_locals_locator.dropVarsForBranch(); + created_vars = created_vars || m_locals_locator.dropVarsForBranch(); // set jump to end pos page(p).emplace_back(label_end); + + // if we have at least one branch that introduced a new variable, + // we have to mark the last variable in the current scope as unreachable + // to avoid generating bad indices for LOAD_FAST_BY_INDEX + if (created_vars) + m_locals_locator.markLastLocalAsUnreachable(); } void ASTLowerer::compileFunction(Node& x, const Page p, const bool is_result_unused) diff --git a/src/arkreactor/Compiler/Lowerer/LocalsLocator.cpp b/src/arkreactor/Compiler/Lowerer/LocalsLocator.cpp index 7611c200c..e320fb422 100644 --- a/src/arkreactor/Compiler/Lowerer/LocalsLocator.cpp +++ b/src/arkreactor/Compiler/Lowerer/LocalsLocator.cpp @@ -13,8 +13,12 @@ namespace Ark::internal void LocalsLocator::addLocal(const std::string& name) { auto& scope = m_scopes.back(); - if (std::ranges::find(scope.data, name) == scope.data.end()) - scope.data.push_back(name); + if (std::ranges::find_if( + scope.data, + [&name](const Scope::Var& v) { + return name == v.name; + }) == scope.data.end()) + scope.data.push_back(Scope::Var { .name = name, .unreachable = false }); } std::optional LocalsLocator::lookupLastScopeByName(const std::string& name) @@ -24,8 +28,14 @@ namespace Ark::internal if (type != ScopeType::Closure) { // Compute the index of the variable in the active scope from the end. - if (const auto it = std::ranges::find(data, name); it != data.end()) - return static_cast(std::distance(it, data.end())) - 1; + for (auto it = data.rbegin(); it != data.rend(); ++it) + { + // If a variable is marked unreachable, then anything before it can not be accessed as well + if (it->unreachable) + return std::nullopt; + if (it->name == name) + return static_cast(std::distance(data.rbegin(), it)); + } } return std::nullopt; @@ -48,15 +58,24 @@ namespace Ark::internal m_drop_for_conds.push_back(m_scopes.back().data.size()); } - void LocalsLocator::dropVarsForBranch() + bool LocalsLocator::dropVarsForBranch() { const auto old_length = m_drop_for_conds.back(); m_drop_for_conds.pop_back(); auto& back = m_scopes.back(); if (back.data.size() > old_length) + { back.data.erase( back.data.begin() + static_cast(old_length), back.data.end()); + return true; + } + return false; + } + + void LocalsLocator::markLastLocalAsUnreachable() + { + m_scopes.back().data.back().unreachable = true; } } diff --git a/tests/unittests/resources/CompilerSuite/ir/load_fast_by_index_after_cond.ark b/tests/unittests/resources/CompilerSuite/ir/load_fast_by_index_after_cond.ark new file mode 100644 index 000000000..45742bfc9 --- /dev/null +++ b/tests/unittests/resources/CompilerSuite/ir/load_fast_by_index_after_cond.ark @@ -0,0 +1,9 @@ +(let foo (fun (toggle) { + (let i 0) + (if toggle + (let a "error, I shouldn't be loaded!")) + (let b 5) + (print i b) })) + +(foo false) # 05 +(foo true) # 05 diff --git a/tests/unittests/resources/CompilerSuite/ir/load_fast_by_index_after_cond.expected b/tests/unittests/resources/CompilerSuite/ir/load_fast_by_index_after_cond.expected new file mode 100644 index 000000000..426903429 --- /dev/null +++ b/tests/unittests/resources/CompilerSuite/ir/load_fast_by_index_after_cond.expected @@ -0,0 +1,35 @@ +page_0 + LOAD_CONST 0 + STORE 0 + PUSH_RETURN_ADDRESS L3 + BUILTIN 0 + CALL_SYMBOL_BY_INDEX 0, 1 +.L3: + POP 0 + PUSH_RETURN_ADDRESS L4 + BUILTIN 1 + CALL_SYMBOL_BY_INDEX 0, 1 +.L4: + POP 0 + HALT 0 + +page_1 + STORE 1 + LOAD_CONST 1 + STORE 2 + LOAD_FAST_BY_INDEX 1 + POP_JUMP_IF_TRUE L0 + JUMP L1 +.L0: + LOAD_CONST 2 + STORE 3 +.L1: + LOAD_CONST 3 + STORE 4 + PUSH_RETURN_ADDRESS L2 + LOAD_FAST 2 + LOAD_FAST_BY_INDEX 0 + CALL_BUILTIN 9, 2 +.L2: + RET 0 + HALT 0 diff --git a/tests/unittests/resources/LangSuite/weird-tests.ark b/tests/unittests/resources/LangSuite/weird-tests.ark index 417439697..7ade70615 100644 --- a/tests/unittests/resources/LangSuite/weird-tests.ark +++ b/tests/unittests/resources/LangSuite/weird-tests.ark @@ -98,4 +98,16 @@ (i (or 0 b) (- a 1)) }))) (i 2 2) - (test:eq output [[2 2] [2 1] [1 1] [1 0]]) }) }) + (test:eq output [[2 2] [2 1] [1 1] [1 0]]) }) + + (test:case "use a variable after a condition" { + (let var_after_cond (fun (toggle) { + (let i 0) + (if toggle + (let a 1)) + (let b 5) + (let res (+ i b)) + (test:eq res 5) })) + + (var_after_cond false) + (var_after_cond true) }) })