From e60b1c3a5ce412e2f30e90648ae8b83eb88d7033 Mon Sep 17 00:00:00 2001 From: Alexandre Plateau Date: Fri, 28 Feb 2025 15:36:58 +0100 Subject: [PATCH 1/2] feat(closure): allow capture in nested scopes --- CHANGELOG.md | 1 + include/Ark/VM/VM.hpp | 4 +- src/arkreactor/VM/VM.cpp | 2 +- .../resources/LangSuite/builtins-tests.ark | 62 +++++++++++-------- 4 files changed, 41 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f7dadfb..dfb0c5a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,6 +103,7 @@ - re-enabled the AST optimizer, only used for the main `arkscript` executable (not enabled when embedding arkscript, so that one can grab variables from the VM) - loops have their own scope: variables created inside a loop won't leak outside it - upgraded fmtlib to 11.1.3-13 +- allow capture in nested scope (before it was targeting only the current scope) ### Removed - removed unused `NodeType::Closure` diff --git a/include/Ark/VM/VM.hpp b/include/Ark/VM/VM.hpp index 298b4b8a..75373227 100644 --- a/include/Ark/VM/VM.hpp +++ b/include/Ark/VM/VM.hpp @@ -186,8 +186,8 @@ namespace Ark // instruction helpers // ================================================ - inline Value* loadSymbol(uint16_t id, internal::ExecutionContext& context); - inline Value* loadConstAsPtr(uint16_t id) const; + [[nodiscard]] inline Value* loadSymbol(uint16_t id, internal::ExecutionContext& context); + [[nodiscard]] inline Value* loadConstAsPtr(uint16_t id) const; inline void store(uint16_t id, const Value* val, internal::ExecutionContext& context); inline void setVal(uint16_t id, const Value* val, internal::ExecutionContext& context); diff --git a/src/arkreactor/VM/VM.cpp b/src/arkreactor/VM/VM.cpp index 88b65fb1..8c1489fd 100644 --- a/src/arkreactor/VM/VM.cpp +++ b/src/arkreactor/VM/VM.cpp @@ -556,7 +556,7 @@ namespace Ark if (!context.saved_scope) context.saved_scope = Scope(); - Value* ptr = (context.locals.back())[arg]; + Value* ptr = findNearestVariable(arg, context); if (!ptr) throwVMError(ErrorKind::Scope, fmt::format("Couldn't capture `{}' as it is currently unbound", m_state.m_symbols[arg])); else diff --git a/tests/unittests/resources/LangSuite/builtins-tests.ark b/tests/unittests/resources/LangSuite/builtins-tests.ark index 53fac9a8..4e824b0e 100644 --- a/tests/unittests/resources/LangSuite/builtins-tests.ark +++ b/tests/unittests/resources/LangSuite/builtins-tests.ark @@ -5,31 +5,43 @@ (let closure (fun (&foo) ())) (test:suite builtin { - (test:eq (type []) "List") - (test:eq (type true) "Bool") - (test:eq (type false) "Bool") - (test:eq (type nil) "Nil") - (test:eq (type 1) "Number") - (test:eq (type "hello") "String") - (test:eq (type print) "CProc") - (test:eq (type foo) "Function") - (test:eq (type closure) "Closure") - - (test:expect (not (io:fileExists? "test.txt"))) - (io:writeFile "test.txt" "hello, world!") - (test:expect (io:fileExists? "test.txt")) - (test:eq (io:readFile "test.txt") "hello, world!") - (io:appendToFile "test.txt" "bis") - (test:eq (io:readFile "test.txt") "hello, world!bis") - (test:expect (> (len (io:listFiles "./")) 0)) - (test:expect (not (io:dir? "test.txt"))) - (test:expect (not (io:fileExists? "temp"))) - (io:makeDir "temp") - (test:expect (io:fileExists? "temp")) - (test:expect (io:dir? "temp")) - (let old (time)) - (sys:sleep 1) - (test:expect (< old (time))) + (test:case "types" { + (test:eq (type []) "List") + (test:eq (type true) "Bool") + (test:eq (type false) "Bool") + (test:eq (type nil) "Nil") + (test:eq (type 1) "Number") + (test:eq (type "hello") "String") + (test:eq (type print) "CProc") + (test:eq (type foo) "Function") + (test:eq (type closure) "Closure") }) + + (test:case "closures" { + (mut keep true) + (mut foo nil) + (while keep { + (set foo (fun (&keep) keep)) + (set keep false) }) + (test:expect foo.keep "capture inside a deeper scope works") }) + + (test:case "files" { + (test:expect (not (io:fileExists? "test.txt"))) + (io:writeFile "test.txt" "hello, world!") + (test:expect (io:fileExists? "test.txt")) + (test:eq (io:readFile "test.txt") "hello, world!") + (io:appendToFile "test.txt" "bis") + (test:eq (io:readFile "test.txt") "hello, world!bis") + (test:expect (> (len (io:listFiles "./")) 0)) + (test:expect (not (io:dir? "test.txt"))) + (test:expect (not (io:fileExists? "temp"))) + (io:makeDir "temp") + (test:expect (io:fileExists? "temp")) + (test:expect (io:dir? "temp")) }) + + (test:case "time" { + (let old (time)) + (sys:sleep 1) + (test:expect (< old (time))) }) (mut rands []) (mut i 0) From daa8e7230b96aaffc5c3c11b9b3d00fd93b4107c Mon Sep 17 00:00:00 2001 From: Alexandre Plateau Date: Fri, 28 Feb 2025 16:05:17 +0100 Subject: [PATCH 2/2] refactor(vm): mark pointers as const or pointer to const as recommended by cppcheck --- src/arkreactor/VM/VM.cpp | 50 +++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/src/arkreactor/VM/VM.cpp b/src/arkreactor/VM/VM.cpp index 8c1489fd..8233db3e 100644 --- a/src/arkreactor/VM/VM.cpp +++ b/src/arkreactor/VM/VM.cpp @@ -510,7 +510,7 @@ namespace Ark // value on the stack else [[likely]] { - Value* ip; + const Value* ip; do { ip = popAndResolveAsPtr(context); @@ -556,7 +556,7 @@ namespace Ark if (!context.saved_scope) context.saved_scope = Scope(); - Value* ptr = findNearestVariable(arg, context); + const Value* ptr = findNearestVariable(arg, context); if (!ptr) throwVMError(ErrorKind::Scope, fmt::format("Couldn't capture `{}' as it is currently unbound", m_state.m_symbols[arg])); else @@ -992,49 +992,49 @@ namespace Ark TARGET(GT) { - Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); + const Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); push((*a != *b && !(*a < *b)) ? Builtins::trueSym : Builtins::falseSym, context); DISPATCH(); } TARGET(LT) { - Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); + const Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); push((*a < *b) ? Builtins::trueSym : Builtins::falseSym, context); DISPATCH(); } TARGET(LE) { - Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); + const Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); push((((*a < *b) || (*a == *b)) ? Builtins::trueSym : Builtins::falseSym), context); DISPATCH(); } TARGET(GE) { - Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); + const Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); push(!(*a < *b) ? Builtins::trueSym : Builtins::falseSym, context); DISPATCH(); } TARGET(NEQ) { - Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); + const Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); push((*a != *b) ? Builtins::trueSym : Builtins::falseSym, context); DISPATCH(); } TARGET(EQ) { - Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); + const Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); push((*a == *b) ? Builtins::trueSym : Builtins::falseSym, context); DISPATCH(); } TARGET(LEN) { - Value* a = popAndResolveAsPtr(context); + const Value* a = popAndResolveAsPtr(context); if (a->valueType() == ValueType::List) push(Value(static_cast(a->constList().size())), context); @@ -1051,7 +1051,7 @@ namespace Ark TARGET(EMPTY) { - Value* a = popAndResolveAsPtr(context); + const Value* a = popAndResolveAsPtr(context); if (a->valueType() == ValueType::List) push(a->constList().empty() ? Builtins::trueSym : Builtins::falseSym, context); @@ -1068,28 +1068,29 @@ namespace Ark TARGET(TAIL) { - Value* a = popAndResolveAsPtr(context); + Value* const a = popAndResolveAsPtr(context); push(helper::tail(a), context); DISPATCH(); } TARGET(HEAD) { - Value* a = popAndResolveAsPtr(context); + Value* const a = popAndResolveAsPtr(context); push(helper::head(a), context); DISPATCH(); } TARGET(ISNIL) { - Value* a = popAndResolveAsPtr(context); + const Value* a = popAndResolveAsPtr(context); push((*a == Builtins::nil) ? Builtins::trueSym : Builtins::falseSym, context); DISPATCH(); } TARGET(ASSERT) { - Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); + Value* const b = popAndResolveAsPtr(context); + Value* const a = popAndResolveAsPtr(context); if (b->valueType() != ValueType::String) types::generateError( @@ -1104,7 +1105,7 @@ namespace Ark TARGET(TO_NUM) { - Value* a = popAndResolveAsPtr(context); + const Value* a = popAndResolveAsPtr(context); if (a->valueType() != ValueType::String) types::generateError( @@ -1122,7 +1123,7 @@ namespace Ark TARGET(TO_STR) { - Value* a = popAndResolveAsPtr(context); + const Value* a = popAndResolveAsPtr(context); push(Value(a->toString(*this)), context); DISPATCH(); } @@ -1130,7 +1131,7 @@ namespace Ark TARGET(AT) { { - Value* b = popAndResolveAsPtr(context); + const Value* b = popAndResolveAsPtr(context); Value a = *popAndResolveAsPtr(context); // be careful, it's not a pointer if (b->valueType() != ValueType::Number) @@ -1173,8 +1174,8 @@ namespace Ark TARGET(AT_AT) { { - Value* x = popAndResolveAsPtr(context); - Value* y = popAndResolveAsPtr(context); + const Value* x = popAndResolveAsPtr(context); + const Value* y = popAndResolveAsPtr(context); Value list = *popAndResolveAsPtr(context); // be careful, it's not a pointer if (y->valueType() != ValueType::Number || x->valueType() != ValueType::Number || @@ -1217,7 +1218,7 @@ namespace Ark TARGET(MOD) { - Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); + const Value *b = popAndResolveAsPtr(context), *a = popAndResolveAsPtr(context); if (a->valueType() != ValueType::Number || b->valueType() != ValueType::Number) types::generateError( "mod", @@ -1229,7 +1230,7 @@ namespace Ark TARGET(TYPE) { - Value* a = popAndResolveAsPtr(context); + const Value* a = popAndResolveAsPtr(context); if (a == &m_undefined_value) [[unlikely]] types::generateError( "type", @@ -1243,14 +1244,15 @@ namespace Ark TARGET(HASFIELD) { { - Value *field = popAndResolveAsPtr(context), *closure = popAndResolveAsPtr(context); + Value* const field = popAndResolveAsPtr(context); + Value* const closure = popAndResolveAsPtr(context); if (closure->valueType() != ValueType::Closure || field->valueType() != ValueType::String) types::generateError( "hasField", { { types::Contract { { types::Typedef("closure", ValueType::Closure), types::Typedef("field", ValueType::String) } } } }, { *closure, *field }); - auto it = std::find(m_state.m_symbols.begin(), m_state.m_symbols.end(), field->stringRef()); + auto it = std::ranges::find(m_state.m_symbols, field->stringRef()); if (it == m_state.m_symbols.end()) { push(Builtins::falseSym, context); @@ -1265,7 +1267,7 @@ namespace Ark TARGET(NOT) { - Value* a = popAndResolveAsPtr(context); + const Value* a = popAndResolveAsPtr(context); push(!(*a) ? Builtins::trueSym : Builtins::falseSym, context); DISPATCH(); }