diff --git a/Docs/04-advanced-features/modules.md b/Docs/04-advanced-features/modules.md index de59ec80..a513ad5c 100644 --- a/Docs/04-advanced-features/modules.md +++ b/Docs/04-advanced-features/modules.md @@ -14,10 +14,24 @@ WFL's module system allows you to organize code across multiple files, enabling ## Basic Module Loading -WFL provides two ways to include code from other files: +WFL provides two ways to pull in code from other files, and **choosing the right +one is the single most important thing to get right:** -1. **Load Module** - Isolated execution (existing behavior) -2. **Include** - Parent scope execution (NEW in V2) +| You want to… | Use | Why | +| --- | --- | --- | +| **Share a library** — call actions, use containers, or read constants defined in another file | **`include from "lib.wfl"`** | Runs the file *in your scope*, so its definitions become available to you | +| **Run a file for its side effects** — initialization, setup, logging — *without* exposing its definitions | **`load module from "setup.wfl"`** | Runs the file in an *isolated* scope; its actions/containers/variables stay private to that file | + +> **⚠️ Common mistake.** `load module` does **not** share the loaded file's +> actions, containers, or variables with the caller — that isolation is the +> whole point of `load module`. If you `load module` a library and then try to +> call one of its actions, you'll get an `is not defined` error (at analysis +> *and* at runtime). **To build a multi-file program out of shared library +> files, use `include from`.** WFL's diagnostics will remind you of this if you +> reach for the wrong one (see [What Modules Cannot Do](#what-modules-cannot-do)). + +1. **Include** - Parent scope execution; **use this to share libraries** +2. **Load Module** - Isolated execution; use this for side-effect-only files ## Load Module Statement @@ -162,7 +176,7 @@ create file at log_file with "Application started" ### What Modules Cannot Do -❌ **Define variables visible to parent:** +❌ **Define actions, containers, or variables visible to parent:** ```wfl # main.wfl load module from "setup.wfl" @@ -173,6 +187,24 @@ store utility_function as "some value" # This variable is local to the module ``` +Because these definitions never reach the caller, referencing one is an +undefined-name error — reported by the analyzer *and* raised at runtime. WFL's +diagnostics make this actionable: when a file uses `load module`, an +"is not defined" / "Undefined action" error carries a note reminding you that +`load module` is isolated and pointing you at `include from`, which *does* +share definitions across files: + +```text +error[ANALYZE-SEMANTIC]: Variable 'mod_double' is not defined + = If you expected this name to come from a file loaded with `load module`, + note that `load module from "..."` runs a file in an isolated scope and does + not expose its actions, containers, or variables to the caller. To share + definitions across files, use `include from "..."` instead. +``` + +If you meant to share the definition, switch that line to +`include from "setup.wfl"`. + ❌ **Modify parent variables:** ```wfl # main.wfl diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index fe4e0dde..7a5bc1e0 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -175,6 +175,21 @@ pub fn program_has_includes(program: &Program) -> bool { .any(|s| matches!(s, Statement::IncludeStatement { .. })) } +/// True when the program contains a top-level `load module from` statement. +/// +/// `load module` runs a file in an *isolated* child scope and does not expose +/// its actions/containers/variables to the caller (unlike `include from`). When +/// a caller references such a symbol it is genuinely undefined — fatal at +/// analysis *and* at runtime — so this is used to attach an actionable +/// "use `include from`" note to the undefined-name diagnostic rather than to +/// relax it (see issue #584). +pub fn program_has_load_module(program: &Program) -> bool { + program + .statements + .iter() + .any(|s| matches!(s, Statement::LoadModuleStatement { .. })) +} + pub struct Analyzer { current_scope: Scope, errors: Vec, diff --git a/src/analyzer/static_analyzer.rs b/src/analyzer/static_analyzer.rs index 2bb5b3f1..1f9ebb67 100644 --- a/src/analyzer/static_analyzer.rs +++ b/src/analyzer/static_analyzer.rs @@ -155,6 +155,14 @@ impl StaticAnalyzer for Analyzer { } if let Err(errors) = analyze_result { + // `load module` runs a file in an isolated scope and does not expose + // its actions/containers/variables to the caller — so an undefined + // action/variable here (whose definition lives in a loaded module) + // fails at runtime too, not only in the analyzer. Keep the error + // fatal, but point the user at `include from`, which actually shares + // definitions across files (issue #584). + let has_load_module = crate::analyzer::program_has_load_module(program); + for error in errors { // Skip errors about undefined variables that are actually action parameters if error.message.starts_with("Variable '") @@ -172,10 +180,31 @@ impl StaticAnalyzer for Analyzer { } } + let is_undefined_symbol = error.message.starts_with("Undefined action '") + || (error.message.starts_with("Variable '") + && error.message.ends_with("' is not defined")); + // The note fires for any undefined symbol when the file uses + // `load module` — the analyzer does not parse the loaded file to + // check whether this specific name is one of its exports, so the + // wording is conditional ("if you expected ... from a loaded + // module"). That keeps it a correct fix for a real module symbol + // while not misleading a plain typo in a side-effect-only load. + let note = if has_load_module && is_undefined_symbol { + Some( + "If you expected this name to come from a file loaded with `load module`, \ + note that `load module from \"...\"` runs a file in an isolated scope and \ + does not expose its actions, containers, or variables to the caller. To \ + share definitions across files, use `include from \"...\"` instead." + .to_string(), + ) + } else { + None:: + }; + diagnostics.push(WflDiagnostic::new( Severity::Error, error.message.clone(), - None::, + note, "ANALYZE-SEMANTIC".to_string(), file_id, error.line, diff --git a/tests/load_module_undefined_hint_test.rs b/tests/load_module_undefined_hint_test.rs new file mode 100644 index 00000000..64001361 --- /dev/null +++ b/tests/load_module_undefined_hint_test.rs @@ -0,0 +1,191 @@ +//! Issue #584 — referencing an action defined in a `load module` file. +//! +//! `load module from "..."` runs a file in an *isolated* child scope; by design +//! it does NOT expose the module's actions/containers/variables to the caller +//! (that is what `include from` is for — see `Docs/04-advanced-features/modules.md`). +//! Empirically, a caller that references a `load module`-defined action fails at +//! *runtime* too, not only in the static analyzer — so relaxing the analyzer to a +//! warning (as the include path does) would be wrong: it would let analysis pass +//! and then crash at runtime. +//! +//! The correct, backward-compatible fix keeps the fatal analyzer error (the +//! program genuinely cannot run) but makes it *actionable*: when a file uses +//! `load module`, an undefined action/variable diagnostic carries a note that +//! points the user at `include from`, the mechanism that actually shares +//! definitions. This suite pins that behavior and guards the boundaries. + +use std::fs; +use std::process::Command; +use tempfile::TempDir; + +fn wfl_exe() -> &'static str { + if cfg!(target_os = "windows") { + "target/release/wfl.exe" + } else { + "target/release/wfl" + } +} + +/// Run a WFL file inside `dir` (so module paths resolve to sibling files), +/// returning (combined stdout+stderr, exit code). +fn run_file_status(dir: &TempDir, name: &str, extra_args: &[&str]) -> (String, Option) { + let path = dir.path().join(name); + let output = Command::new(wfl_exe()) + .args(extra_args) + .arg(&path) + .output() + .expect("Failed to execute WFL"); + let combined = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + (combined, output.status.code()) +} + +/// Write a `lib_mod.wfl` exposing a one-arg `mod_double` action next to a +/// `main_mod.wfl` whose body is `main_body`, then run `main_mod.wfl`. +fn with_double_module(main_body: &str, extra_args: &[&str]) -> (String, Option) { + let dir = TempDir::new().expect("tempdir"); + fs::write( + dir.path().join("lib_mod.wfl"), + "define action called mod_double with parameters n:\n return n plus n\nend action\n", + ) + .unwrap(); + fs::write(dir.path().join("main_mod.wfl"), main_body).unwrap(); + let out = run_file_status(&dir, "main_mod.wfl", extra_args); + drop(dir); + out +} + +// --------------------------------------------------------------------------- +// The exact repro from issue #584 — `load module` + `of`-form call. +// --------------------------------------------------------------------------- + +/// The undefined-action error must stay fatal (the call cannot resolve at +/// runtime either), and it must carry an actionable note pointing at +/// `include from`. +#[test] +fn load_module_of_form_is_fatal_with_include_hint() { + let (out, code) = with_double_module( + "load module from \"lib_mod.wfl\"\ndisplay mod_double of 5\n", + &[], + ); + assert!( + out.contains("is not defined"), + "reference to a load-module action must stay fatal: {out}" + ); + assert!( + out.contains("include from"), + "the diagnostic should guide the user to `include from`: {out}" + ); + assert_eq!( + code, + Some(3), + "should exit 3 (fatal analyze error), not run: {out}" + ); +} + +/// Same for the `call ... with` form (which surfaces as "Undefined action"). +#[test] +fn load_module_call_form_is_fatal_with_include_hint() { + let (out, code) = with_double_module( + "load module from \"lib_mod.wfl\"\nstore d as call mod_double with 5\ndisplay d\n", + &[], + ); + assert!( + out.contains("Undefined action") || out.contains("is not defined"), + "call-form reference to a load-module action must stay fatal: {out}" + ); + assert!( + out.contains("include from"), + "the diagnostic should guide the user to `include from`: {out}" + ); + assert_eq!(code, Some(3), "should exit 3: {out}"); +} + +/// The hint also appears under `--analyze`. +#[test] +fn load_module_hint_under_analyze() { + let (out, _code) = with_double_module( + "load module from \"lib_mod.wfl\"\ndisplay mod_double of 5\n", + &["--analyze"], + ); + assert!( + out.contains("include from"), + "`--analyze` should also surface the `include from` guidance: {out}" + ); +} + +// --------------------------------------------------------------------------- +// The documented resolution: `include from` shares the action and runs. +// --------------------------------------------------------------------------- + +/// Swapping `load module` for `include from` makes the same program work — this +/// is the path the hint steers users toward. +#[test] +fn include_from_shares_the_action_and_runs() { + let (out, code) = with_double_module( + "include from \"lib_mod.wfl\"\ndisplay mod_double of 5\n", + &[], + ); + assert!( + out.contains("10"), + "include-from should share `mod_double` and print 10: {out}" + ); + assert_eq!(code, Some(0), "program should exit 0: {out}"); +} + +// --------------------------------------------------------------------------- +// Guard rails — the hint is specific to `load module` programs. +// --------------------------------------------------------------------------- + +/// A file that uses `load module` for side effects but has an *unrelated* +/// undefined reference still gets the hint. The analyzer does not parse the +/// loaded file to check whether the missing name is one of its exports, so the +/// trigger is deliberately coarse — but the note is conditionally worded +/// ("if you expected this name to come from a file loaded with `load module`"), +/// so it does not mislead when the real problem is a plain typo. This pins that +/// intentional behavior (CodeRabbit review on PR #586). +#[test] +fn load_module_plus_unrelated_typo_is_hinted_but_not_misleading() { + let (out, code) = with_double_module( + "load module from \"lib_mod.wfl\"\ndisplay totally_unrelated_typo\n", + &[], + ); + assert!( + out.contains("is not defined"), + "the unrelated typo must stay fatal: {out}" + ); + assert!( + out.contains("include from"), + "the hint fires whenever `load module` is present (coarse but safe): {out}" + ); + assert!( + out.to_lowercase().contains("if you expected"), + "the hint is conditionally framed so it does not mislead for a typo: {out}" + ); + assert_eq!(code, Some(3), "should exit 3: {out}"); +} + +/// Without any `load module` (or `include`), an undefined `of` callee stays +/// fatal but must NOT carry the `include from` hint (it would be misleading). +#[test] +fn undefined_without_load_module_has_no_include_hint() { + let dir = TempDir::new().expect("tempdir"); + fs::write( + dir.path().join("main.wfl"), + "display \"BEFORE\"\nstore g as missing_action of \"x\"\ndisplay g\n", + ) + .unwrap(); + let (out, code) = run_file_status(&dir, "main.wfl", &[]); + assert!( + out.contains("is not defined"), + "undefined callee must stay fatal: {out}" + ); + assert!( + !out.contains("include from"), + "no load module present, so no `include from` hint should appear: {out}" + ); + assert_eq!(code, Some(3), "should exit 3: {out}"); +}