diff --git a/Dev diary/2026-07-10-issue-560-residuals-try-returns-container-methods.md b/Dev diary/2026-07-10-issue-560-residuals-try-returns-container-methods.md new file mode 100644 index 00000000..641e4fc0 --- /dev/null +++ b/Dev diary/2026-07-10-issue-560-residuals-try-returns-container-methods.md @@ -0,0 +1,115 @@ +# Issue #560 Residuals: Returns Inside `try` Blocks and Container Method Results No Longer Typed `Nothing` + +**Date:** 2026-07-10 + +## Background + +Issue #560 ("unannotated action return types default to `Nothing`") was fixed +in two rounds: #575 added post-body return-type inference for top-level +actions, and #591 seeded the provisional return type as `Unknown` so +self-recursive calls degrade gracefully. Working on Scriptorium surfaced the +same false `Cannot index into Nothing` diagnostic again, so this pass hunted +down the remaining shapes that still slipped through. Two were found and +fixed; both are static-diagnostics-only bugs (runtime was always correct). + +## Residual 1 — `return` inside a `try:` block was invisible to inference + +`collect_return_types` (`src/typechecker/mod.rs`) descends into conditionals +and loops to find an action's `return` statements, but it never descended into +`Statement::TryStatement`. An action whose only returns live inside a `try:` +body, a `when error` clause, an `otherwise` clause, or a `finally` block was +inferred as returning `Nothing`, and indexing its call result raised the false +error: + +```wfl +define action called load_data: + try: + return [1 and 2] + when error: + return [3 and 4] + end try +end action + +store xs as call load_data +store x0 as xs[0] // error[ERROR]: Cannot index into Nothing (false) +``` + +This is a very common shape — try blocks wrap exactly the file/database/parse +work that helper actions return values from, which is why Scriptorium hit it. + +**Fix:** `collect_return_types` now descends into `TryStatement` (body, every +`when` clause, `otherwise`, `finally`) and `WaitForStatement` (its wrapped +inner statement). Its sibling `check_return_statements` — the traversal used +when an annotation *is* present — got the same arms so the two stay in sync. + +## Residual 2 — container method results were registered as `Nothing` + +The analyzer registers container methods in its container registry with + +```rust +return_type: return_type.as_ref().cloned().unwrap_or(Type::Nothing), +``` + +and the type checker's `Expression::MethodCall` arm reads that registry to +type `instance.method()`. Unannotated value-returning methods (the norm) were +therefore typed `Nothing` at every call site — the container-flavored twin of +the original #560, which #575 only fixed for top-level actions: + +```wfl +create container Store: + property label: Text + action get_items: + return [1 and 2] + end +end +... +store xs as s.get_items() +store x0 as xs[0] // error[ERROR]: Cannot index into Nothing (false) +``` + +**Fix,** mirroring the #575 + #591 design for top-level actions: + +- The analyzer seeds unannotated instance *and* static methods with a + provisional `Type::Unknown` (degrades gracefully after #588/#589) instead of + `Type::Nothing`. +- The type checker's `ContainerDefinition` arm now checks each method body — + instance *and* static — with the method's parameters in scope (mirroring the + top-level action arm's #553 handling), infers the real return type from the + body's `return` statements, and writes it back into the registry through a + new `Analyzer::get_container_mut`. Void methods still end up `Nothing`, + exactly as before. +- Static methods matter here even though static method *calls* + (`Container.method()`) are still a future feature at runtime: the + refinement keeps `Container.method` member access reporting an accurate + function type, and without it a void static method would have stayed + `Unknown` forever instead of `Nothing` (a strictness regression flagged by + review on the first revision of this change). +- Annotated container methods (e.g. `action get_info: Text`) now also get + their `return` statements validated against the annotation via + `check_return_statements`, mirroring the top-level action arm. +- Inherited methods get the fix for free: the `MethodCall` parent-walk reads + the same registry entries. + +## Verification (TDD) + +`tests/action_return_type_residuals_test.rs` was written first and confirmed +failing (4/4) before the fix: + +- return inside `try`/`when error` infers the return type +- return inside `try`/`when`/`otherwise` infers the return type +- unannotated container method result is not typed `Nothing` +- inherited container method result is not typed `Nothing` + +All four pass after the fix, alongside the full `cargo test --all` suite, +`cargo clippy --all-targets --all-features -- -D warnings`, and the +`TestPrograms/` backward-compatibility run. A combined Scriptorium-shaped +stress case (recursive action indexing a map returned from inside `try`, +wrapped by a container method) type-checks completely clean. + +## Notes + +- No syntax, keyword, or runtime behavior changed — this is purely a + false-positive-diagnostics fix, so no user-facing docs needed updating. +- Remaining known limitation (unchanged by this pass): a method call typed + *before* its container's definition statement is reached in program order + still sees the provisional `Unknown` — safe (permissive) but not concrete. diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index d2666233..7b51a6d5 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -1277,7 +1277,14 @@ impl Analyzer { let method_info = MethodInfo { name: method_name.clone(), parameters: parameters.clone(), - return_type: return_type.as_ref().cloned().unwrap_or(Type::Nothing), + // Unannotated methods get a provisional `Unknown` + // (not `Nothing`) so uses of a method result checked + // before the type checker refines it degrade + // gracefully instead of raising false "found + // Nothing" errors (issue #560 residual). The type + // checker infers the real return type from the body + // and writes it back via `get_container_mut`. + return_type: return_type.as_ref().cloned().unwrap_or(Type::Unknown), is_public: true, // Default to public for now line: *method_line, column: *method_column, @@ -1336,7 +1343,9 @@ impl Analyzer { let method_info = MethodInfo { name: method_name.clone(), parameters: parameters.clone(), - return_type: return_type.as_ref().cloned().unwrap_or(Type::Nothing), + // Same provisional `Unknown` seed as instance + // methods above (issue #560 residual). + return_type: return_type.as_ref().cloned().unwrap_or(Type::Unknown), is_public: true, // Default to public for now line: *method_line, column: *method_column, @@ -2144,6 +2153,13 @@ impl Analyzer { self.containers.get(name) } + /// Mutable access to a registered container, used by the type checker to + /// refine an unannotated method's provisional return type after inferring + /// it from the method body (issue #560 residual). + pub fn get_container_mut(&mut self, name: &str) -> Option<&mut ContainerInfo> { + self.containers.get_mut(name) + } + pub fn get_containers(&self) -> &HashMap { &self.containers } diff --git a/src/typechecker/mod.rs b/src/typechecker/mod.rs index 0d1e3a41..a5d60c7f 100644 --- a/src/typechecker/mod.rs +++ b/src/typechecker/mod.rs @@ -1657,7 +1657,7 @@ impl TypeChecker { methods, events: _events, static_properties: _static_properties, - static_methods: _static_methods, + static_methods, line, column, } => { @@ -1726,21 +1726,96 @@ impl TypeChecker { } } - for method in methods { - if let Statement::ActionDefinition { body, .. } = method { + // Check method bodies (instance and static) and, for + // unannotated methods, infer the real return type from the + // body's `return` statements — the analyzer only registered a + // provisional `Unknown` (issue #560 residual: leaving the + // provisional type in place made `instance.method()` results + // degrade to `Unknown`, and the old `Nothing` default produced + // false "Cannot index into Nothing" errors). Static methods get + // the same refinement so `Container.method` member access + // reports an accurate function type and void statics resolve + // back to `Nothing`. Inferred types are collected first and + // written back after the loop so the registry borrow doesn't + // overlap the body checks. + let mut inferred_method_returns: Vec<(String, Type)> = Vec::new(); + let mut inferred_static_method_returns: Vec<(String, Type)> = Vec::new(); + for (method, is_static) in methods + .iter() + .map(|m| (m, false)) + .chain(static_methods.iter().map(|m| (m, true))) + { + if let Statement::ActionDefinition { + name: method_name, + parameters, + body, + return_type, + line: method_line, + column: method_column, + } = method + { // Set container context for method body analysis let previous_container = self.current_container.clone(); self.current_container = Some(_name.clone()); + // Parameters must be resolvable while checking the body + // and inferring return expressions, mirroring the + // top-level `ActionDefinition` arm (issue #553). + self.analyzer.push_scope(); + for param in parameters { + let param_symbol = Symbol { + name: param.name.clone(), + kind: SymbolKind::Variable { mutable: false }, + symbol_type: param.param_type.clone().or(Some(Type::Unknown)), + line: param.line, + column: param.column, + }; + let _ = self.analyzer.define_symbol(param_symbol); + } + for stmt in body { self.check_statement_types(stmt); } + if let Some(ret_type) = return_type { + self.check_return_statements( + body, + ret_type, + *method_line, + *method_column, + ); + } else { + let inferred = self.infer_action_return_type(body); + if is_static { + inferred_static_method_returns + .push((method_name.clone(), inferred)); + } else { + inferred_method_returns.push((method_name.clone(), inferred)); + } + } + + self.analyzer.pop_scope(); + // Restore previous container context self.current_container = previous_container; } } + if let Some(container_info) = self.analyzer.get_container_mut(_name) { + for (method_name, inferred) in inferred_method_returns { + if let Some(method_info) = container_info.methods.get_mut(&method_name) { + method_info.return_type = inferred; + } + } + for (method_name, inferred) in inferred_static_method_returns { + if let Some(method_info) = + container_info.static_methods.get_mut(&method_name) + { + method_info.return_type = inferred; + } + } + } + // Container type registration would be handled by analyzer } Statement::ContainerInstantiation { @@ -3836,6 +3911,32 @@ impl TypeChecker { | Statement::MainLoop { body, .. } => { self.collect_return_types(body, out); } + // Actions commonly return from inside error handling — a `try:` + // body, its `when error` clauses, `otherwise`, or `finally`. + // Skipping these blocks inferred such actions as `Nothing` and + // produced false "Cannot index into Nothing" diagnostics at the + // call site (issue #560 residual). + Statement::TryStatement { + body, + when_clauses, + otherwise_block, + finally_block, + .. + } => { + self.collect_return_types(body, out); + for clause in when_clauses { + self.collect_return_types(&clause.body, out); + } + if let Some(otherwise_stmts) = otherwise_block { + self.collect_return_types(otherwise_stmts, out); + } + if let Some(finally_stmts) = finally_block { + self.collect_return_types(finally_stmts, out); + } + } + Statement::WaitForStatement { inner, .. } => { + self.collect_return_types(std::slice::from_ref(inner), out); + } _ => {} } } @@ -3926,6 +4027,34 @@ impl TypeChecker { | Statement::MainLoop { body, .. } => { self.check_return_statements(body, expected_type, line, column); } + // Keep in sync with `collect_return_types`: returns inside + // `try:` blocks and `wait for` wrappers count too. + Statement::TryStatement { + body, + when_clauses, + otherwise_block, + finally_block, + .. + } => { + self.check_return_statements(body, expected_type, line, column); + for clause in when_clauses { + self.check_return_statements(&clause.body, expected_type, line, column); + } + if let Some(otherwise_stmts) = otherwise_block { + self.check_return_statements(otherwise_stmts, expected_type, line, column); + } + if let Some(finally_stmts) = finally_block { + self.check_return_statements(finally_stmts, expected_type, line, column); + } + } + Statement::WaitForStatement { inner, .. } => { + self.check_return_statements( + std::slice::from_ref(inner), + expected_type, + line, + column, + ); + } _ => {} } } @@ -4946,4 +5075,54 @@ mod tests { "Inferred type should be precisely Number (not widened to Any), got: {errors:?}" ); } + + /// Issue #560 residual: unannotated static container methods must be + /// refined in the container registry just like instance methods — a + /// value-returning one gets its inferred type (so `Container.method` + /// member access reports an accurate function type) and a void one goes + /// back to `Nothing` instead of keeping the provisional `Unknown` seed. + /// Static method *calls* are still a future feature at runtime, so the + /// registry is the observable surface to pin down here. + #[test] + fn test_static_method_return_types_refined_in_registry() { + let code = r#" +create container MathUtils: + static action get_pair: + return [1 and 2] + end + static action log_it: + display "hi" + end +end +"#; + let tokens = crate::lexer::lex_wfl_with_positions(code); + let mut parser = crate::parser::Parser::new(&tokens); + let program = parser.parse().expect("Should parse"); + + let mut type_checker = TypeChecker::new(); + let _ = type_checker.check_types(&program); + + let container = type_checker + .analyzer + .get_container("MathUtils") + .expect("container should be registered"); + let get_pair = container + .static_methods + .get("get_pair") + .expect("static method get_pair should be registered"); + assert!( + matches!(get_pair.return_type, Type::List(_)), + "value-returning static method should have an inferred List return type, got {:?}", + get_pair.return_type + ); + let log_it = container + .static_methods + .get("log_it") + .expect("static method log_it should be registered"); + assert_eq!( + log_it.return_type, + Type::Nothing, + "void static method should be refined to Nothing, not left as the Unknown seed" + ); + } } diff --git a/tests/action_return_type_residuals_test.rs b/tests/action_return_type_residuals_test.rs new file mode 100644 index 00000000..e66b0145 --- /dev/null +++ b/tests/action_return_type_residuals_test.rs @@ -0,0 +1,134 @@ +//! Regression tests for the remaining residuals of issue #560: an unannotated +//! action that clearly returns a value must not have its call result typed +//! `Nothing`. +//! +//! #575 added post-body return-type inference and #591 seeded recursion with +//! `Unknown`, but two shapes still slipped through: +//! +//! 1. `collect_return_types` did not descend into `try:` blocks (`try` body, +//! `when error` clauses, `otherwise`, `finally`), so an action whose only +//! `return`s live inside a `try:` block was inferred as `Nothing`, and +//! indexing its result raised a false `Cannot index into Nothing`. +//! 2. Container methods were registered with `return_type: Nothing` when +//! unannotated and never refined, so `instance.method()` results hit the +//! same false diagnostic. + +use wfl::lexer::lex_wfl_with_positions; +use wfl::parser::Parser; +use wfl::typechecker::TypeChecker; + +/// Type-check `code` and assert it produces zero diagnostics (see +/// `recursive_action_return_type_test.rs` for why fully-clean is the tighter +/// regression guard). +fn assert_typechecks_clean(code: &str) { + let tokens = lex_wfl_with_positions(code); + let mut parser = Parser::new(&tokens); + let program = parser.parse().expect("Should parse"); + + let mut type_checker = TypeChecker::new(); + let result = type_checker.check_types(&program); + + assert!( + result.is_ok(), + "Program should type-check clean; got: {:?}", + result.err() + ); +} + +/// An action whose only `return`s are inside a `try:` body / `when error` +/// clause must infer its return type from them, not default to `Nothing`. +#[test] +fn test_return_inside_try_block_infers_return_type() { + assert_typechecks_clean( + r#" +define action called load_data: + try: + return [1 and 2] + when error: + return [3 and 4] + end try +end action + +store xs as call load_data +store x0 as xs[0] +display x0 +"#, + ); +} + +/// Same, with `otherwise` (the success clause) also returning. +#[test] +fn test_return_inside_try_otherwise_infers_return_type() { + assert_typechecks_clean( + r#" +define action called risky: + try: + store vals as [1 and 2] + return vals + when error: + return [9 and 9] + otherwise: + return [7 and 7] + end try +end action + +store xs as call risky +store x0 as xs[0] +display x0 +"#, + ); +} + +/// An unannotated container method that returns a value must not have its +/// call result typed `Nothing`. +#[test] +fn test_container_method_result_not_typed_nothing() { + assert_typechecks_clean( + r#" +create container Store: + property label: Text + + action get_items: + return [1 and 2] + end +end + +create new Store as s: + label is "main" +end + +store xs as s.get_items() +store x0 as xs[0] +display x0 +"#, + ); +} + +/// The same holds for a method inherited from a parent container. +#[test] +fn test_inherited_container_method_result_not_typed_nothing() { + assert_typechecks_clean( + r#" +create container Base: + property label: Text + + action get_items: + return [1 and 2] + end +end + +create container Child extends Base: + property extra: Text +end + +create new Child as c: + label is "main" + extra is "x" +end + +store xs as c.get_items() +store x0 as xs[0] +display x0 +"#, + ); +}