diff --git a/Docs/04-advanced-features/modules.md b/Docs/04-advanced-features/modules.md index 352281da..89d14fd7 100644 --- a/Docs/04-advanced-features/modules.md +++ b/Docs/04-advanced-features/modules.md @@ -75,6 +75,78 @@ display line All three forms work at the top level and inside your own action bodies. Because the analyzer does not read included files, it emits a **non-fatal** `Undefined action ''` note for a name it cannot see statically — the program still runs and the action resolves at runtime. +### Including the same file more than once (diamond includes) + +Larger programs often end up including a shared file from more than one +place. The classic shape is a **diamond**: two library files both include the +same helper file, and the main program includes both libraries. + +```text + util.wfl + / \ + auth.wfl render.wfl + \ / + main.wfl +``` + +```wfl +# util.wfl +define action called shout with parameters msg: + give back msg with "!" +end action + +# auth.wfl +include from "util.wfl" +define action called auth_check with parameters who: + give back shout of who +end action + +# render.wfl +include from "util.wfl" +define action called render_page with parameters title: + give back shout of title +end action + +# main.wfl +include from "auth.wfl" +include from "render.wfl" +display auth_check of "alice" # alice! +display render_page of "home" # home! +``` + +This works because a file's **definitions are included once per scope**. +`auth.wfl` brings `util.wfl` into the main program's scope. When +`render.wfl` then asks for `util.wfl` again, its definitions are already +visible in that scope, so the second `include from` does nothing — it does +not run the file a second time, and it does not raise an "already defined" +error. Each file can therefore honestly declare what it depends on, and the +order in which the main program includes its libraries does not matter. + +Three details worth knowing: + +- **The rule is about definitions.** A file that defines actions, + containers, or variables is brought in once per scope. A file that + defines nothing — it only displays something, writes a file, or changes + variables that already exist — has nothing a later include could collide + with, so it runs every time it is included, exactly as it always has. + (`load module from` remains the clearest way to say "run this file for + its side effects".) +- **"Same scope" includes scopes you can see.** An include inside an action + body is skipped when the file was already included by an enclosing scope + (its definitions are already reachable). A file included only inside an + action body or a loop body runs again on each call or iteration, because + each one starts with a fresh local scope that has not seen it. +- **A failed include is not remembered.** If the included file stops with + an error, a later include of it runs it again. Anything it defined before + the error stays in the scope, as it did before. + +The four files above are kept under `TestPrograms/docs_examples/modules/diamond/` +and validated with the rest of the documentation examples. + +A genuine cycle — `a.wfl` includes `b.wfl`, which includes `a.wfl` — is +still reported as a circular dependency (see +[Circular Dependency Protection](#circular-dependency-protection)). + ### Type Checking in Included Files Included files go through the same pipeline as the main program (parse, analyze, type check). Because `include from` runs the file in the parent scope — as if the code were written in the main program — type-check findings in an included file are reported the same way as in the main file: as **non-fatal warnings**. The program still runs. @@ -615,9 +687,12 @@ export constant VERSION - Must load entire module - Future: `load function1, function2 from "x.wfl"` planned -3. **No Module Caching** +3. **No Module Caching for `load module`** - Each `load module` re-parses and re-executes - Multiple loads of same file execute multiple times + - (`include from` is different: it brings a file's definitions in once + per scope, so diamond includes are safe — see + [Including the same file more than once](#including-the-same-file-more-than-once-diamond-includes)) - Future: Optional caching planned 4. **Export Foundation Only** @@ -850,7 +925,7 @@ load module from "expensive.wfl" # Uses cached version WFL's hybrid module system provides flexible code organization: - **`load module from "path.wfl"`** - Isolated execution for initialization and side effects -- **`include from "path.wfl"`** - Parent scope execution for shared libraries and containers +- **`include from "path.wfl"`** - Parent scope execution for shared libraries and containers; a file's definitions are included once per scope, so diamond includes are safe - **`export container/action/constant NAME`** - Foundation for future namespace system - Paths resolve relative to the including file - Circular dependencies are automatically detected diff --git a/History/dev-diary/2026/2026-09-04-diamond-includes.md b/History/dev-diary/2026/2026-09-04-diamond-includes.md new file mode 100644 index 00000000..587a2e1b --- /dev/null +++ b/History/dev-diary/2026/2026-09-04-diamond-includes.md @@ -0,0 +1,91 @@ +# 2026-09-04 — Diamond includes: the second branch always broke + +## Symptom + +A multi-file program where two library files both `include from` the same +shared file could not run: + +```text + util.wfl + / \ + auth.wfl render.wfl + \ / + main.wfl +``` + +```text +error[ERROR]: Semantic error in included file 'render.wfl': + Semantic error at line 3, column 21: 'shout' is not a function +``` + +The first branch (`auth.wfl`) worked; the second (`render.wfl`) failed while +being *analyzed*, before a single line of it ran. Verified on wfl 26.9.2. + +A downstream project had noticed this and concluded that "WFL includes form a +tree, and diamonds break", working around it by chaining every file into one +long line (`util <- db <- auth <- render <- site_ext <- main`). The chain works +only by accident: with every `include from` at the top of its file, nothing +is defined yet when each file is analyzed, so unknown names are downgraded to +warnings and resolve at runtime. + +## Root cause + +Two separate defects, one hiding the other. + +1. **Parent-scope actions were seeded into an included file's analyzer as + plain variables.** `extract_parent_variables` turned every runtime binding + into `SymbolKind::Variable`, actions included. Calling one of those + (`shout of title`) then hit the analyzer's "'shout' is not a function" + error, which is fatal for included files. This broke any second file that + used an action an earlier include had defined — the diamond, but also the + plain sibling order where `render.wfl` does not include `util.wfl` itself. + +2. **Re-including a file re-ran it into the same scope.** Once (1) is fixed, + the second arrival at `util.wfl` executes `store util_loads as 1` and + `define action called shout` again in a scope that already has both: + "Variable 'shout' has already been defined at line 0". The cycle check + only knew about files *currently* loading, not files already finished. + +## Fix + +- The interpreter now snapshots the enclosing scope as typed variables + **and** action signatures (`snapshot_parent_scope`), and the analyzer gets + the actions through `register_parent_actions` as real function symbols + with their true parameter lists. A same-name definition in the analyzed + file is treated as an overload under the existing distinctness rules, + which matches what the runtime already did. +- `Environment` records the canonical paths `include from` has completed in + that scope **and that installed at least one definition there**. An + include whose file is already recorded for the current scope (or an + ancestor it can see) is a no-op, decided before the import-depth ceiling + is charged. Recycled loop scopes clear the record along with their + values. A file that defines nothing is never recorded, so a side-effect- + only file keeps running on every include — no existing program changes + behavior (anything with a definition already failed). A failed include is + not recorded either; what it defined before failing stays in the scope, + as it always did. +- `load module` still runs in an isolated child scope. Its analyzer now + sees outer actions as callable functions (so calls resolve) but rejects a + same-name definition up front — the runtime would reject it anyway, and + the rejection must land before the module's earlier statements run. + +## Evidence + +- Red: `tests/include_diamond_test.rs` (9 tests, four added from review + findings: loop-scope recycling, side-effect-only re-include, the + import-depth boundary, and `load module` outer-action rejection) and + `TestPrograms/modules/include_diamond.wfl` fail on the unmodified + interpreter with the errors quoted above. +- Green: same tests pass after the change; `cargo test --workspace`, + `cargo clippy --all-targets --all-features -- -D warnings`, and the + gated `TestPrograms/` run are clean. +- Risk class R3 (backward compatibility). Negative paths covered: a genuine + include cycle is still rejected; an include inside an action body still + runs per call when nothing enclosing has included the file. + +## Residual + +The type checker does not follow includes, so a container defined in a +shared file still produces non-fatal "Container type 'X' not found" warnings +in a sibling file that instantiates it. That predates this change and the +program runs correctly; it is a separate issue. diff --git a/TestPrograms/docs_examples/_meta/manifest.json b/TestPrograms/docs_examples/_meta/manifest.json index 06dadad0..c0c5b7b4 100644 --- a/TestPrograms/docs_examples/_meta/manifest.json +++ b/TestPrograms/docs_examples/_meta/manifest.json @@ -673,5 +673,71 @@ "properties" ], "doc_purpose": "Demonstrates reading instance properties with object.property" + }, + "docs_examples/modules/diamond/util.wfl": { + "doc_section": "Docs/04-advanced-features/modules.md", + "type": "executable", + "validate_layers": [ + 1, + 2, + 3, + 4, + 5 + ], + "expected_exit_code": 0, + "tags": [ + "modules", + "include", + "diamond" + ], + "doc_purpose": "Shared leaf file of the diamond include example" + }, + "docs_examples/modules/diamond/auth.wfl": { + "doc_section": "Docs/04-advanced-features/modules.md", + "type": "executable", + "validate_layers": [ + 1, + 4, + 5 + ], + "expected_exit_code": 0, + "tags": [ + "modules", + "include", + "diamond" + ], + "doc_purpose": "First branch of the diamond include example. Layers 2-3 are skipped: it calls an action that lives in a file it includes, and single-file analysis reports that as a non-fatal warning with a nonzero exit; layer 5 runs the real multi-file program." + }, + "docs_examples/modules/diamond/render.wfl": { + "doc_section": "Docs/04-advanced-features/modules.md", + "type": "executable", + "validate_layers": [ + 1, + 4, + 5 + ], + "expected_exit_code": 0, + "tags": [ + "modules", + "include", + "diamond" + ], + "doc_purpose": "Second branch of the diamond include example. Layers 2-3 are skipped: it calls an action that lives in a file it includes, and single-file analysis reports that as a non-fatal warning with a nonzero exit; layer 5 runs the real multi-file program." + }, + "docs_examples/modules/diamond/main.wfl": { + "doc_section": "Docs/04-advanced-features/modules.md", + "type": "executable", + "validate_layers": [ + 1, + 4, + 5 + ], + "expected_exit_code": 0, + "tags": [ + "modules", + "include", + "diamond" + ], + "doc_purpose": "Top of the diamond include example: both branches include util.wfl once. Layers 2-3 are skipped: it calls an action that lives in a file it includes, and single-file analysis reports that as a non-fatal warning with a nonzero exit; layer 5 runs the real multi-file program." } } diff --git a/TestPrograms/docs_examples/modules/diamond/auth.wfl b/TestPrograms/docs_examples/modules/diamond/auth.wfl new file mode 100644 index 00000000..a868cebb --- /dev/null +++ b/TestPrograms/docs_examples/modules/diamond/auth.wfl @@ -0,0 +1,5 @@ +// One branch of the diamond: depends on util.wfl directly. +include from "util.wfl" +define action called auth_check with parameters who: + give back shout of who +end action diff --git a/TestPrograms/docs_examples/modules/diamond/main.wfl b/TestPrograms/docs_examples/modules/diamond/main.wfl new file mode 100644 index 00000000..b461e3b1 --- /dev/null +++ b/TestPrograms/docs_examples/modules/diamond/main.wfl @@ -0,0 +1,5 @@ +// Top of the diamond: both branches bring in util.wfl; it is included once. +include from "auth.wfl" +include from "render.wfl" +display auth_check of "alice" # alice! +display render_page of "home" # home! diff --git a/TestPrograms/docs_examples/modules/diamond/render.wfl b/TestPrograms/docs_examples/modules/diamond/render.wfl new file mode 100644 index 00000000..c16b5647 --- /dev/null +++ b/TestPrograms/docs_examples/modules/diamond/render.wfl @@ -0,0 +1,5 @@ +// The other branch of the diamond: also depends on util.wfl directly. +include from "util.wfl" +define action called render_page with parameters title: + give back shout of title +end action diff --git a/TestPrograms/docs_examples/modules/diamond/util.wfl b/TestPrograms/docs_examples/modules/diamond/util.wfl new file mode 100644 index 00000000..689012bb --- /dev/null +++ b/TestPrograms/docs_examples/modules/diamond/util.wfl @@ -0,0 +1,5 @@ +// Shared leaf of the include diamond documented in +// Docs/04-advanced-features/modules.md ("Including the same file more than once"). +define action called shout with parameters msg: + give back msg with "!" +end action diff --git a/TestPrograms/modules/include_diamond.wfl b/TestPrograms/modules/include_diamond.wfl new file mode 100644 index 00000000..e2e41ea0 --- /dev/null +++ b/TestPrograms/modules/include_diamond.wfl @@ -0,0 +1,16 @@ +// Diamond include: `auth.wfl` and `render.wfl` both `include from "util.wfl"`. +// The second arrival at `util.wfl` must be a no-op (its definitions are +// already in this scope), not an "already defined" failure, and both +// branches must be able to call the shared action. +include from "../../tests/fixtures/modules/diamond/auth.wfl" +include from "../../tests/fixtures/modules/diamond/render.wfl" + +describe "diamond include": + test "both branches see the shared action": + expect auth_check of "alice" to equal "alice!" + expect render_page of "home" to equal "home!" + end test + test "the shared file ran exactly once": + expect util_loads to equal 1 + end test +end describe diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index dd36c36d..322e67dc 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -364,6 +364,12 @@ pub struct Analyzer { /// before every independent program so prior declarations and diagnostics /// cannot leak across runs. baseline_symbols: HashMap, + /// Actions seeded from an enclosing runtime scope that the analyzed file + /// may call but must not redefine or overload: a `load module` file runs + /// in an isolated child scope whose runtime `define` rejects shadowing a + /// parent binding, so the analyzer rejects it first — before any of the + /// module's side effects run. + outer_actions: HashSet, /// Monotone lexical-scope identity source. Scope IDs remain stable through /// snapshots/clones and are never reused within one analyzer run. next_scope_id: u64, @@ -665,6 +671,7 @@ impl Analyzer { Analyzer { current_scope: global_scope, baseline_symbols, + outer_actions: HashSet::new(), next_scope_id: 1, errors: Vec::new(), warnings: Vec::new(), @@ -733,6 +740,55 @@ impl Analyzer { analyzer } + /// Seeds actions that already exist in the enclosing runtime scope (for + /// example, actions an earlier `include from` brought in) as real + /// function symbols with their true signatures. Seeding them as plain + /// variables would make every call to them a fatal "is not a function" + /// error in the file being analyzed, which is what broke diamond and + /// sibling includes. Like the other parent-scope symbols these carry + /// position 0:0. + /// + /// `mergeable` mirrors the runtime binding rule of the file being + /// analyzed: an `include from` file runs in the enclosing scope itself, + /// so a same-name definition there is an overload under the usual + /// distinctness rules; a `load module` file runs in an isolated child + /// scope where the runtime rejects any same-name definition, so the + /// analyzer rejects it too (see [`Self::outer_actions`]). + pub fn register_parent_actions( + &mut self, + actions: Vec<(String, Vec)>, + mergeable: bool, + ) { + for (name, signatures) in actions { + if !mergeable { + self.outer_actions.insert(name.clone()); + } + let param_types = signatures + .first() + .map(|sig| { + sig.parameters + .iter() + .map(|p| p.param_type.clone().unwrap_or(Type::Unknown)) + .collect() + }) + .unwrap_or_default(); + let symbol = Symbol { + name: name.clone(), + kind: SymbolKind::Function { signatures }, + symbol_type: Some(Type::Function { + parameters: param_types, + return_type: Box::new(Type::Unknown), + }), + line: 0, + column: 0, + }; + self.current_scope + .symbols + .insert(name.clone(), symbol.clone()); + self.baseline_symbols.insert(name, symbol); + } + } + pub fn is_builtin_function(name: &str) -> bool { crate::builtins::is_builtin_function(name) } @@ -3113,6 +3169,23 @@ impl Analyzer { return_type: return_type.clone(), }; + // An action seeded from the loading file's scope cannot be + // redefined or overloaded by an isolated module (the runtime + // would reject the shadowing after the module's earlier + // statements had already run). + if self.outer_actions.contains(name) { + self.errors.push(SemanticError::new( + format!( + "Action '{name}' has already been defined in an outer scope. \ + A file run with 'load module' cannot redefine or overload an action \ + from the file that loaded it; use a different name." + ), + *line, + *column, + )); + return; + } + // Same-scope redefinition of an existing action is the overload // path: accumulate the signature instead of erroring, unless the // pair is an exact duplicate or cannot be told apart at a call diff --git a/src/interpreter/environment.rs b/src/interpreter/environment.rs index 98c4cdbd..40456f02 100644 --- a/src/interpreter/environment.rs +++ b/src/interpreter/environment.rs @@ -1,6 +1,7 @@ use super::value::Value; use std::cell::RefCell; use std::collections::{HashMap, HashSet}; +use std::path::{Path, PathBuf}; use std::rc::{Rc, Weak}; #[derive(Debug)] @@ -11,6 +12,12 @@ pub struct Environment { /// When true, provides module isolation: values from parent scopes are deep cloned /// to prevent mutations, and assignment to parent variables is prevented. pub isolated: bool, + /// Canonical paths of the files `include from` has already run in this + /// scope. An include of a file whose definitions are already visible + /// here (recorded on this scope or an ancestor) is a no-op, which is + /// what makes diamond includes work: two files that both include the + /// same shared file reach it once, not twice. + pub included_files: HashSet, } impl Environment { @@ -23,6 +30,7 @@ impl Environment { constants: HashSet::new(), parent: None, isolated: false, + included_files: HashSet::new(), })) } @@ -35,6 +43,7 @@ impl Environment { constants: HashSet::new(), parent: Some(Rc::downgrade(parent)), isolated: false, + included_files: HashSet::new(), })) } @@ -48,6 +57,7 @@ impl Environment { constants: HashSet::new(), parent: Some(Rc::downgrade(parent)), isolated: false, + included_files: HashSet::new(), })) } @@ -64,9 +74,32 @@ impl Environment { constants: HashSet::new(), parent: Some(Rc::downgrade(parent)), isolated: true, + included_files: HashSet::new(), })) } + /// True when `path` was already included into this scope or into any + /// scope this one can see, so its definitions are already reachable. + pub fn has_included(&self, path: &Path) -> bool { + if self.included_files.contains(path) { + return true; + } + let mut parent = self.parent.as_ref().and_then(|weak| weak.upgrade()); + while let Some(env) = parent { + let env_ref = env.borrow(); + if env_ref.included_files.contains(path) { + return true; + } + parent = env_ref.parent.as_ref().and_then(|weak| weak.upgrade()); + } + false + } + + /// Records that `path` has been included into this scope. + pub fn mark_included(&mut self, path: PathBuf) { + self.included_files.insert(path); + } + pub fn define(&mut self, name: &str, value: Value) -> Result<(), String> { // Check if the variable already exists in current scope if self.values.contains_key(name) { @@ -351,6 +384,9 @@ impl Environment { pub fn clear(&mut self) { self.values.clear(); self.constants.clear(); + // A recycled loop scope starts logically fresh: the definitions an + // include installed are gone, so its marker must go with them. + self.included_files.clear(); // Parent, isolated status, and other flags remain unchanged } diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index d577b598..b8455700 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -1249,6 +1249,14 @@ impl Drop for OutboundStreamCleanup { } /// RAII guard that ensures module loading context is restored on scope exit. +/// What an included or loaded file can see from its enclosing scope, +/// split for the analyzer: typed variables and action signatures. +#[derive(Default)] +struct ParentScopeSnapshot { + variables: HashMap, + actions: Vec<(String, Vec)>, +} + /// Automatically removes its loading_stack entry and restores /// current_source_file when dropped. /// @@ -4913,42 +4921,80 @@ impl Interpreter { /// Extract variables from the environment for module analyzer /// Returns a HashMap of variable names to (inferred type, is_mutable) - fn extract_parent_variables( - env: &Rc>, - ) -> HashMap { - let mut vars = HashMap::new(); - let env_borrowed = env.borrow(); - - for (name, value) in &env_borrowed.values { - // Skip native builtins (e.g. `year`, `month`, `day`, `length`, ...). - // The analyzer already resolves these through `is_builtin_function`, - // so seeding them as parent *variables* only makes an included file - // stricter than the main file: an action-local `store year as ...` - // would fatally conflict with the builtin's outer-scope binding even - // though the same code runs fine in a main program (#557). Leaving - // them out lets locals shadow builtins consistently in both paths. - if matches!(value, Value::NativeFunction(_, _)) { - continue; - } - let inferred_type = Self::infer_type_from_value(value); - // Check if this variable is a constant (immutable) - let is_mutable = !env_borrowed.constants.contains(name); - vars.insert(name.clone(), (inferred_type, is_mutable)); - } - - // Also extract from parent scopes - if let Some(parent_weak) = &env_borrowed.parent - && let Some(parent_rc) = parent_weak.upgrade() - { - drop(env_borrowed); // Release borrow before recursive call - let parent_vars = Self::extract_parent_variables(&parent_rc); - // Parent variables are added first, can be shadowed by current scope - for (name, (ty, is_mut)) in parent_vars { - vars.entry(name).or_insert((ty, is_mut)); + /// Snapshot of what an included/loaded file can see from its enclosing + /// runtime scope, split the way the analyzer wants it: plain bindings + /// as typed variables and actions as function signatures. + fn snapshot_parent_scope(env: &Rc>) -> ParentScopeSnapshot { + let mut snapshot = ParentScopeSnapshot::default(); + // Nearest scope wins: a name bound here shadows the same name further + // out, whatever kind of binding each one is. + let mut seen: HashSet = HashSet::new(); + let mut current = Some(Rc::clone(env)); + while let Some(scope) = current { + let scope_ref = scope.borrow(); + for (name, value) in &scope_ref.values { + if !seen.insert(name.clone()) { + continue; + } + match value { + // Skip native builtins (e.g. `year`, `month`, `day`, `length`, ...). + // The analyzer already resolves these through `is_builtin_function`, + // so seeding them as parent *variables* only makes an included file + // stricter than the main file: an action-local `store year as ...` + // would fatally conflict with the builtin's outer-scope binding even + // though the same code runs fine in a main program (#557). Leaving + // them out lets locals shadow builtins consistently in both paths. + Value::NativeFunction(_, _) => {} + Value::Function(func) => { + snapshot + .actions + .push((name.clone(), vec![Self::signature_of(func)])); + } + Value::Overloaded(set) => { + let signatures = set + .overloads + .iter() + .map(|f| Self::signature_of(f)) + .collect(); + snapshot.actions.push((name.clone(), signatures)); + } + _ => { + let inferred_type = Self::infer_type_from_value(value); + // Check if this variable is a constant (immutable) + let is_mutable = !scope_ref.constants.contains(name); + snapshot + .variables + .insert(name.clone(), (inferred_type, is_mutable)); + } + } } + current = scope_ref.parent.as_ref().and_then(|weak| weak.upgrade()); } - vars + snapshot + } + + /// The analyzer-side signature of a runtime action value. + fn signature_of( + func: &crate::interpreter::value::FunctionValue, + ) -> crate::analyzer::FunctionSignature { + use crate::parser::ast::Parameter; + let parameters = func + .params + .iter() + .zip(func.param_types.iter().chain(std::iter::repeat(&None))) + .map(|(param_name, param_type)| Parameter { + name: param_name.clone(), + param_type: param_type.clone(), + default_value: None, + line: func.line, + column: func.column, + }) + .collect(); + crate::analyzer::FunctionSignature { + parameters, + return_type: None, + } } /// Infer AST Type from runtime Value @@ -8535,8 +8581,11 @@ impl Interpreter { use crate::analyzer::Analyzer; // Extract parent variables from current environment for module analyzer - let parent_vars = Self::extract_parent_variables(&env); - let mut analyzer = Analyzer::with_parent_variables(parent_vars); + let parent_scope = Self::snapshot_parent_scope(&env); + let mut analyzer = Analyzer::with_parent_variables(parent_scope.variables); + // The module runs in an isolated child scope: outer actions are + // callable but a same-name definition is rejected, as at runtime. + analyzer.register_parent_actions(parent_scope.actions, false); if let Err(errors) = analyzer.analyze(&program) { // Use the semantic error's position from the module file, not the load site let first_error = errors.first(); @@ -8669,9 +8718,22 @@ impl Interpreter { // 2. Resolve absolute path let resolved_path = self.resolve_module_path(&path_str, *line, *column).await?; - // 3. Check circular dependencies and the shared import-depth - // ceiling (loading_stack length is the depth already entered). + // 3. Reject a genuine cycle (the file is still being loaded). self.check_circular_dependency(&resolved_path, *line, *column)?; + + // 3b. Include definitions once per visible scope. When the + // file's definitions already live in this scope (or one it + // can see), running it again would only collide with them, + // so the include is a no-op. This is what lets two files + // both `include from` the same shared file (a diamond). It + // is decided before the depth ceiling because a no-op never + // enters another file. + if env.borrow().has_included(&resolved_path) { + return Ok((Value::Null, ControlFlow::None)); + } + + // 3c. Shared import-depth ceiling (loading_stack length is the + // depth already entered). if let Err(exceeded) = self .budget .check_import_depth(self.loading_stack.borrow().len()) @@ -8713,8 +8775,11 @@ impl Interpreter { // 6. Analyze semantics use crate::analyzer::Analyzer; - let parent_vars = Self::extract_parent_variables(&env); - let mut analyzer = Analyzer::with_parent_variables_mutable(parent_vars); + let parent_scope = Self::snapshot_parent_scope(&env); + let mut analyzer = Analyzer::with_parent_variables_mutable(parent_scope.variables); + // The file runs in this very scope, so a same-name definition + // merges into the overload set exactly as the runtime does. + analyzer.register_parent_actions(parent_scope.actions, true); if let Err(errors) = analyzer.analyze(&program) { let first_error = errors.first(); let (error_line, error_column) = @@ -8775,16 +8840,34 @@ impl Interpreter { // 9. Execute included file in PARENT scope (key difference from load module) // This allows containers/variables to be exposed to parent + let bindings_before = env.borrow().values.len(); let result = self .execute_block(&program.statements, Rc::clone(&env)) .await; - // 10. Handle result + // 10. Handle result. A file is recorded as included only when + // it ran to completion AND installed at least one definition + // in this scope: that is what a later include of it would + // collide with. A file that defines nothing (side effects + // only, or mutations of existing variables) keeps running on + // every include exactly as it always has. A failed include is + // never recorded; note that whatever it defined before failing + // stays in the scope, as it did before include-once existed. + let record_if_defined = |env: &Rc>| { + let mut env_mut = env.borrow_mut(); + if env_mut.values.len() > bindings_before { + env_mut.mark_included(resolved_path.clone()); + } + }; match result { - Ok((_, ControlFlow::None)) => Ok((Value::Null, ControlFlow::None)), + Ok((_, ControlFlow::None)) => { + record_if_defined(&env); + Ok((Value::Null, ControlFlow::None)) + } Ok((val, ControlFlow::Return(_))) => { // Return statements in included files are allowed and simply return the value // This enables utility functions in included files to use return statements + record_if_defined(&env); Ok((val, ControlFlow::None)) } Ok((_, ControlFlow::Break)) => Err(RuntimeError::new( diff --git a/tests/fixtures/modules/diamond/auth.wfl b/tests/fixtures/modules/diamond/auth.wfl new file mode 100644 index 00000000..6424fd78 --- /dev/null +++ b/tests/fixtures/modules/diamond/auth.wfl @@ -0,0 +1,4 @@ +include from "util.wfl" +define action called auth_check with parameters who: + give back shout of who +end action diff --git a/tests/fixtures/modules/diamond/render.wfl b/tests/fixtures/modules/diamond/render.wfl new file mode 100644 index 00000000..aa9229eb --- /dev/null +++ b/tests/fixtures/modules/diamond/render.wfl @@ -0,0 +1,4 @@ +include from "util.wfl" +define action called render_page with parameters title: + give back shout of title +end action diff --git a/tests/fixtures/modules/diamond/util.wfl b/tests/fixtures/modules/diamond/util.wfl new file mode 100644 index 00000000..d3d86dda --- /dev/null +++ b/tests/fixtures/modules/diamond/util.wfl @@ -0,0 +1,6 @@ +// Shared leaf of the include diamond. Both `auth.wfl` and `render.wfl` +// include this file; it must be safe to reach it twice. +store util_loads as 1 +define action called shout with parameters msg: + give back msg with "!" +end action diff --git a/tests/include_diamond_test.rs b/tests/include_diamond_test.rs new file mode 100644 index 00000000..629b3504 --- /dev/null +++ b/tests/include_diamond_test.rs @@ -0,0 +1,317 @@ +//! Diamond includes: two files that both `include from` the same shared file +//! must be usable together. The shared file's definitions already live in the +//! scope after the first include, so a second include of the same file into +//! that scope (or a scope that can already see it) is a no-op rather than a +//! fatal "already defined" error. A genuine cycle is still rejected. + +use std::fs; +use std::path::Path; +use std::sync::Arc; +use tempfile::TempDir; +use wfl::exec::budget::{BudgetLimits, ExecutionBudget}; +use wfl::interpreter::Interpreter; +use wfl::interpreter::value::Value; +use wfl::lexer::lex_wfl_with_positions; +use wfl::parser::Parser; + +async fn run(main_file: &Path) -> Result { + run_with(main_file, Interpreter::new()).await +} + +async fn run_with(main_file: &Path, mut interpreter: Interpreter) -> Result { + let source = fs::read_to_string(main_file).expect("read main"); + let tokens = lex_wfl_with_positions(&source); + let ast = Parser::new(&tokens) + .parse() + .unwrap_or_else(|e| panic!("Parse failed: {e:?}")); + interpreter.set_source_file(main_file.to_path_buf()); + match interpreter.interpret(&ast).await { + Ok(_) => Ok(interpreter), + Err(errors) => Err(errors + .iter() + .map(|e| e.message.clone()) + .collect::>() + .join("\n")), + } +} + +fn global(interpreter: &Interpreter, name: &str) -> Value { + interpreter + .global_env() + .borrow() + .get(name) + .unwrap_or_else(|| panic!("`{name}` is not defined in the global scope")) +} + +fn assert_text(interpreter: &Interpreter, name: &str, expected: &str) { + match global(interpreter, name) { + Value::Text(t) => assert_eq!(&*t, expected, "value of `{name}`"), + other => panic!("`{name}` should be text, got {other:?}"), + } +} + +fn assert_number(interpreter: &Interpreter, name: &str, expected: f64) { + match global(interpreter, name) { + Value::Number(n) => assert_eq!(n, expected, "value of `{name}`"), + other => panic!("`{name}` should be a number, got {other:?}"), + } +} + +fn write_diamond(dir: &Path) { + fs::write( + dir.join("util.wfl"), + r#" +store util_loads as 1 +define action called shout with parameters msg: + give back msg with "!" +end action +"#, + ) + .unwrap(); + fs::write( + dir.join("auth.wfl"), + r#" +include from "util.wfl" +define action called auth_check with parameters who: + give back shout of who +end action +"#, + ) + .unwrap(); + fs::write( + dir.join("render.wfl"), + r#" +include from "util.wfl" +define action called render_page with parameters title: + give back shout of title +end action +"#, + ) + .unwrap(); +} + +#[tokio::test] +async fn diamond_include_reaches_shared_file_twice_without_error() { + let dir = TempDir::new().unwrap(); + write_diamond(dir.path()); + let main = dir.path().join("main.wfl"); + fs::write( + &main, + r#" +include from "auth.wfl" +include from "render.wfl" +store a as auth_check of "alice" +store r as render_page of "home" +"#, + ) + .unwrap(); + let interpreter = run(&main) + .await + .unwrap_or_else(|e| panic!("diamond include failed: {e}")); + assert_text(&interpreter, "a", "alice!"); + assert_text(&interpreter, "r", "home!"); + // The shared leaf executed once: its `store` did not run a second time. + assert_number(&interpreter, "util_loads", 1.0); +} + +#[tokio::test] +async fn direct_double_include_of_same_file_is_a_no_op() { + let dir = TempDir::new().unwrap(); + write_diamond(dir.path()); + let main = dir.path().join("main.wfl"); + fs::write( + &main, + r#" +include from "util.wfl" +include from "util.wfl" +store s as shout of "x" +"#, + ) + .unwrap(); + let interpreter = run(&main) + .await + .unwrap_or_else(|e| panic!("double include failed: {e}")); + assert_text(&interpreter, "s", "x!"); +} + +#[tokio::test] +async fn include_inside_action_body_repeats_per_call_scope() { + // Each call of `prepare_greeting` gets a fresh local scope that cannot see the + // previous call's include, so the file must run again for that scope. + let dir = TempDir::new().unwrap(); + write_diamond(dir.path()); + let main = dir.path().join("main.wfl"); + fs::write( + &main, + r#" +define action called prepare_greeting with parameters who: + include from "util.wfl" + give back shout of who +end action +store first as prepare_greeting of "a" +store second as prepare_greeting of "b" +"#, + ) + .unwrap(); + let interpreter = run(&main) + .await + .unwrap_or_else(|e| panic!("per-call include failed: {e}")); + assert_text(&interpreter, "first", "a!"); + assert_text(&interpreter, "second", "b!"); +} + +#[tokio::test] +async fn include_already_visible_from_outer_scope_is_skipped_inside_action() { + // `util.wfl` is included at top level; an action body that includes it + // again can already see its definitions, so the include must not re-run + // the file into the local scope (which would collide with the outer one). + let dir = TempDir::new().unwrap(); + write_diamond(dir.path()); + let main = dir.path().join("main.wfl"); + fs::write( + &main, + r#" +include from "util.wfl" +define action called prepare_greeting with parameters who: + include from "util.wfl" + give back shout of who +end action +store first as prepare_greeting of "a" +"#, + ) + .unwrap(); + let interpreter = run(&main) + .await + .unwrap_or_else(|e| panic!("nested re-include failed: {e}")); + assert_text(&interpreter, "first", "a!"); + assert_number(&interpreter, "util_loads", 1.0); +} + +#[tokio::test] +async fn genuine_include_cycle_is_still_rejected() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("a.wfl"), "include from \"b.wfl\"\n").unwrap(); + fs::write(dir.path().join("b.wfl"), "include from \"a.wfl\"\n").unwrap(); + let err = run(&dir.path().join("a.wfl")) + .await + .err() + .expect("a.wfl <-> b.wfl cycle must fail"); + assert!( + err.contains("Circular dependency detected"), + "unexpected error: {err}" + ); +} + +#[tokio::test] +async fn include_in_a_loop_body_runs_again_in_each_recycled_iteration_scope() { + // Loop iteration scopes are recycled (cleared and reused). A file that + // installs only a plain variable leaves no weak references behind, so + // the recycled scope must forget the include along with the variable. + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("val.wfl"), "store loop_val as 7\n").unwrap(); + let main = dir.path().join("main.wfl"); + fs::write( + &main, + r#" +store total as 0 +count from 1 to 3: + include from "val.wfl" + change total to total plus loop_val +end count +"#, + ) + .unwrap(); + let interpreter = run(&main) + .await + .unwrap_or_else(|e| panic!("loop include failed: {e}")); + assert_number(&interpreter, "total", 21.0); +} + +#[tokio::test] +async fn side_effect_only_file_still_runs_on_every_include() { + // Backward compatibility: a file that defines nothing has nothing for a + // later include to collide with, so it keeps running each time. + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("bump.wfl"), "change n to n plus 1\n").unwrap(); + let main = dir.path().join("main.wfl"); + fs::write( + &main, + r#" +store n as 0 +include from "bump.wfl" +include from "bump.wfl" +"#, + ) + .unwrap(); + let interpreter = run(&main) + .await + .unwrap_or_else(|e| panic!("repeated side-effect include failed: {e}")); + assert_number(&interpreter, "n", 2.0); +} + +#[tokio::test] +async fn already_visible_include_is_a_no_op_even_at_the_import_depth_ceiling() { + // With a ceiling of one nested file, `wrapper.wfl` may not enter another + // file. Its include of `util.wfl` is already satisfied by the top-level + // include, so it must be a no-op rather than a depth error. + let dir = TempDir::new().unwrap(); + write_diamond(dir.path()); + fs::write( + dir.path().join("wrapper.wfl"), + "include from \"util.wfl\"\nstore via_wrapper as shout of \"w\"\n", + ) + .unwrap(); + let main = dir.path().join("main.wfl"); + fs::write( + &main, + "include from \"util.wfl\"\ninclude from \"wrapper.wfl\"\n", + ) + .unwrap(); + let mut interpreter = Interpreter::new(); + interpreter.set_budget(Arc::new(ExecutionBudget::new(BudgetLimits { + max_import_depth: 1, + ..BudgetLimits::default() + }))); + let interpreter = run_with(&main, interpreter) + .await + .unwrap_or_else(|e| panic!("no-op include must not consume import depth: {e}")); + assert_text(&interpreter, "via_wrapper", "w!"); +} + +#[tokio::test] +async fn load_module_cannot_overload_an_outer_action_and_is_rejected_before_running() { + // The module runs in an isolated scope where the runtime rejects any + // same-name definition. The analyzer must reject it first, so the + // module's earlier statements (here: a mutation of `ran`) never run. + let dir = TempDir::new().unwrap(); + fs::write( + dir.path().join("mod.wfl"), + r#" +change ran to "yes" +define action called greet with parameters a and b: + give back a with b +end action +"#, + ) + .unwrap(); + let main = dir.path().join("main.wfl"); + fs::write( + &main, + r#" +store ran as "no" +define action called greet with parameters who: + give back "hi " with who +end action +load module from "mod.wfl" +"#, + ) + .unwrap(); + let err = run(&main) + .await + .err() + .expect("a module redefining an outer action must fail"); + assert!( + err.contains("Semantic error in module") && err.contains("outer scope"), + "expected an analysis-time rejection, got: {err}" + ); +}