diff --git a/docs/SPEC.md b/docs/SPEC.md index ee1d65e..de53bb5 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -1032,8 +1032,15 @@ can read the loader's globals and call its functions, but they can never bind a write through to them — a bare `name is expr` inside a module function that doesn't refer to a local, a captured name, or the module's own top-level state creates a fresh local, regardless of what -happens to exist in the loader's scope. (Top-level statements in the -loaded file still execute directly in the current scope.) To share +happens to exist in the loader's scope. The same boundary applies one +level up to an **imported** module's own top-level statements: a bare +`name is expr` at an imported file's top level binds in *that module's +own scope*, never walking through to a same-named binding the importer +happens to already have — `counter is 0` at a module's top level can +never rebind an importer's pre-existing `counter`. `load_file` is the +one exception, per its older, documented contract above: its top-level +statements still execute directly in the current (caller's) scope, so +a same-named top-level assignment there *does* bind through. To share mutable state across files, put it in a dict or list and mutate fields — reads cross the boundary and field/index writes are value mutations, not bindings. The standard library's UI toolkit (`lib/ui.eigs`'s `_ui` diff --git a/src/compiler.c b/src/compiler.c index 87bf62d..50aa9fa 100644 --- a/src/compiler.c +++ b/src/compiler.c @@ -1617,7 +1617,14 @@ static void emit_assign_for_tos(Compiler *c, const char *name, uint32_t name_has } if (!picked) { int idx = add_string_constant(c, name); - if (local_only) { + /* #589: at an IMPORTED module's own top level, a bare write + * must never walk past mod_env to rebind whatever the + * importer's scope already holds under the same name — + * mirrors #373's function-body rule one level up. load_file + * doesn't set g_compile_import_toplevel, so its top-level + * writes keep walking the chain into the caller's scope, + * per its documented "current scope" contract. */ + if (local_only || g_compile_import_toplevel) { set_op = OP_SET_NAME_LOCAL; set_arg = (uint16_t)idx; } else { set_op = OP_SET_NAME; set_arg = (uint16_t)idx; diff --git a/src/eigenscript.h b/src/eigenscript.h index de5d73a..3954093 100644 --- a/src/eigenscript.h +++ b/src/eigenscript.h @@ -643,6 +643,14 @@ struct EigsThread { * global that happened to exist at compile time — the write * compiles as a fresh local instead. Reads stay dynamic. */ int compile_module_boundary; + /* #589: nonzero ONLY while compiling an `import`ed module's own + * top-level statements (never for load_file, whose documented + * contract is "runs directly in the current scope"). A bare + * `name is expr` at that top level must resolve/create in the + * module's own fresh env (mod_env) and never walk through to + * whatever the importer's scope happens to already bind — the + * top-level counterpart of #373's function-body boundary. */ + int compile_import_toplevel; /* Per-import resolution base (Phase 0b). Empty = fall back to * state->script_dir. OP_IMPORT saves/restores around module body. */ char import_resolve_dir[4096]; @@ -731,6 +739,7 @@ extern __thread EigsThread *eigs_current; #define g_exe_dir (eigs_current->state->exe_dir) #define g_load_env (eigs_current->load_env) #define g_compile_module_boundary (eigs_current->compile_module_boundary) +#define g_compile_import_toplevel (eigs_current->compile_import_toplevel) #define g_import_resolve_dir (eigs_current->import_resolve_dir) #define g_vm_multithreaded (eigs_current->state->multithreaded) #define g_gc_envs (eigs_current->state->gc_envs) diff --git a/src/vm.c b/src/vm.c index 8baf962..ac30ebe 100644 --- a/src/vm.c +++ b/src/vm.c @@ -4941,8 +4941,17 @@ static Value *vm_run_ex(EigsChunk *chunk, Env *env, Task *resume) { int saved_boundary = g_compile_module_boundary; g_compile_module_boundary = 1; /* #373 */ + /* #589: this module's OWN top-level statements compile here too + * (mod_env's parent is g_global_env — the importer's scope). + * Mark it so a bare top-level `name is expr` binds/updates only + * within mod_env and never walks through to the importer's + * global — load_file does NOT set this (its documented contract + * keeps top-level writes in the caller's current scope). */ + int saved_import_toplevel = g_compile_import_toplevel; + g_compile_import_toplevel = 1; EigsChunk *mod_chunk = compile_ast(ast, mod_env, source); g_compile_module_boundary = saved_boundary; + g_compile_import_toplevel = saved_import_toplevel; if (g_parse_errors > 0) { g_parse_errors = saved_errors; chunk_free(mod_chunk); diff --git a/tests/run_all_tests.sh b/tests/run_all_tests.sh index 09aadc3..2017d35 100755 --- a/tests/run_all_tests.sh +++ b/tests/run_all_tests.sh @@ -1709,6 +1709,7 @@ echo "" echo "[43a2] Builtin Argument Errors (26 checks)" check_eigs_suite "builtin argument errors" test_builtin_errors.eigs "All builtin_errors tests passed" 30 check_eigs_suite "module-boundary write insulation (#373)" test_module_scope.eigs "All module-scope tests passed" 9 +check_eigs_suite "import top-level scope insulation vs load_file current-scope contract (#589)" test_import_toplevel_scope.eigs "All import top-level scope tests passed" 11 echo "" # [43a3] EigenStore header-validation / corruption error paths (ext_store.c) diff --git a/tests/test_import_toplevel_scope.eigs b/tests/test_import_toplevel_scope.eigs new file mode 100644 index 0000000..1d08d30 --- /dev/null +++ b/tests/test_import_toplevel_scope.eigs @@ -0,0 +1,48 @@ +# #589: import's own top-level bare assignment must bind in the imported +# module's own scope, never walking through to a same-named binding that +# happens to already exist in the importer's scope — the top-level +# counterpart of #373's function-write boundary (test_module_scope.eigs). +# Before the fix, a module's `counter is 0` at its own top level rebound the +# IMPORTER's `counter` instead of creating module state, and every later use +# of `counter` inside the module's functions then resolved to the importer's +# variable (the exact cross-wire from the issue's minimal repro). +# +# load_file keeps its OLDER, documented "runs directly in the current scope" +# contract (docs/SPEC.md) completely unchanged — this file proves both +# directions explicitly, since a regression in either one is the failure +# mode #589 warns about. + +# --- Case 1 (import): module top-level state is insulated from a +# same-named importer variable, both at import time and across repeated +# calls into the module's own functions. --- +write_text of ["tmp_589_cmod.eigs", "counter is 0\ndefine bump() as:\n counter is counter + 1\n return counter\n"] + +counter is [9, 9, 9] +import tmp_589_cmod +assert of [(type of counter) == "list", "I1 importer's counter untouched by module top-level init"] +assert of [counter == [9, 9, 9], "I2 importer's counter value unchanged by import"] +assert of [(tmp_589_cmod.bump of null) == 1, "I3 module state starts at 1 in its own scope"] +assert of [(tmp_589_cmod.bump of null) == 2, "I4 module state accumulates in its own scope"] +assert of [(tmp_589_cmod.bump of null) == 3, "I5 module state keeps accumulating, still module-local"] +assert of [(type of counter) == "list", "I6 importer's counter still untouched after repeated module calls"] +assert of [counter == [9, 9, 9], "I7 importer's counter value still unchanged"] + +remove_file of "tmp_589_cmod.eigs" + +# --- Case 2 (load_file): documented contract is UNCHANGED — a load_file'd +# file's top-level statements still execute directly in the CURRENT +# (caller's) scope, so a same-named top-level assignment DOES bind through +# and DOES clobber the caller's existing binding. This is load_file's older, +# intentional semantics, not the #589 bug, and the fix must not touch it. --- +write_text of ["tmp_589_lfmod.eigs", "counterLF is 0\ndefine bumpLF() as:\n counterLF is counterLF + 1\n return counterLF\n"] + +counterLF is [9, 9, 9] +load_file of "tmp_589_lfmod.eigs" +assert of [(type of counterLF) == "num", "L1 load_file's top-level init binds through to the current scope"] +assert of [counterLF == 0, "L2 current-scope counterLF now holds the loaded file's value"] +assert of [(bumpLF of null) == 1, "L3 bumpLF reads/writes the (now shared) current-scope binding"] +assert of [counterLF == 1, "L4 the write is visible in the current scope too — same binding"] + +remove_file of "tmp_589_lfmod.eigs" + +print of "All import top-level scope tests passed"