fix: infer user-defined action return types in the type checker - #575
Conversation
The type checker defaulted every action's return type to `Nothing` (WFL has no return-type annotation syntax), so a `call <action>` result was typed `Nothing` at the call site. When that result fed a builtin position requiring `Text` — `respond to req with ...`, `open file at ...`, list indexing, etc. — the checker emitted a spurious "Expected Text but found Nothing" error even though the value was correct at runtime. This produced a wall of false positives in any program that factors request handling / file reading into actions (issue #569). Infer an action's return type from its `return` expressions: collect the type of every reachable return, merge them (identical types collapse; differing/un-inferrable types widen to a permissive type), and update the action's symbol so call sites carry the real result type. Inference runs after the body is checked (so body-locals and parameters are in scope) and the symbol update is deferred until the parameter scope is popped, since `get_symbol_mut` only reaches the innermost scope. Genuine mismatches are still reported: an action returning `Number` used where `Text` is required now correctly reports `Number` (with a helpful conversion hint) instead of `Nothing`. Verified against all TestPrograms: the only change in type-check output is the removal of false positives (e.g. database_sqlite_test.wfl no longer reports "Cannot index into Nothing" on action-returned query rows); no new diagnostics or regressions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYZY9RXYzxp4WHhZxqVjZt
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe typechecker now provisions actions with a provisional ChangesAction return-type inference
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TypeChecker
participant Scope
participant ActionSymbol
TypeChecker->>ActionSymbol: register with provisional Nothing return type
TypeChecker->>Scope: push params/body-local scope
TypeChecker->>TypeChecker: check_statement_types on action body
TypeChecker->>TypeChecker: collect_return_types over control flow
TypeChecker->>TypeChecker: infer_action_return_type (merge/widen)
TypeChecker->>Scope: pop scope
TypeChecker->>ActionSymbol: update with inferred Function return type (skip if Nothing)
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/typechecker/mod.rs`:
- Around line 3686-3697: Stop collecting return types from unreachable sibling
statements after an unconditional return. Update the statement-list traversal in
the typechecker logic around infer/collect returns so that once a
ReturnStatement is encountered, later statements in the same block are ignored
for return-type aggregation. Keep only reachable returns contributing to the
merged type so helpers that infer from statements no longer widen precise types
like Text/Number to Any.
- Around line 3718-3725: The return-type collector in `collect_return_types` is
missing `Statement::RepeatWhileLoop`, so returns inside `repeat while` bodies
are skipped and infer `Nothing`. Update the existing loop-match group alongside
`Statement::ForEachLoop`, `CountLoop`, `WhileLoop`, `RepeatUntilLoop`,
`ForeverLoop`, and `MainLoop` to also recurse into `RepeatWhileLoop.body` so its
returns are included in inference.
- Around line 780-809: Forward action calls are still rejected because top-level
action symbols are registered without a return type until the definition is
analyzed, so early call sites see symbol_type as missing. Update the
action-signature setup in src/typechecker/mod.rs so action symbols are seeded
with a provisional Type::Function (using the known parameters and a placeholder
return type) before the statement walk, or otherwise defer call-site validation
until after all action bodies are analyzed; use the existing
infer_action_return_type, get_symbol_mut, and check_return_statements flow as
the reference points.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Address two correctness gaps in the return-type collector (PR #575 review): - Include `repeat while` bodies: `collect_return_types` (and the parallel `check_return_statements`) omitted `Statement::RepeatWhileLoop`, so an action whose only `return` sits inside a `repeat while` loop still inferred `Nothing`. Add it to the loop-recursion arm. - Ignore unreachable returns: a bare `return` unconditionally exits the action, so sibling statements after it in the same block are dead code. The collector now stops at the first top-level `return`, preventing an unreachable sibling from widening a precise type (e.g. `Text`) to `Any` and masking a genuine mismatch at the call site. Adds regression tests for both. Not changed: forward action calls (calling an action before its definition) remain rejected — WFL executes top-to-bottom and such a call fails at runtime with "Undefined action", so the type error correctly mirrors real behavior rather than being a false positive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYZY9RXYzxp4WHhZxqVjZt
|
@coderabbitai review |
✅ Action performedReview finished.
|
* fix: seed action provisional return type as Unknown so self-recursion type-checks (#590) A self-recursive action that used its own recursive result inside its body (e.g. indexed it) got a false `Cannot index into Nothing` diagnostic. The body is type-checked before the real return type is inferred (#575's ordering), and the provisional return type was seeded as `Nothing`, so a self-reference in the body resolved to `Nothing` and any use/indexing of it raised strict "found Nothing" errors. Seed the provisional return type as `Unknown` instead. After #588/#589 an `Unknown`-typed value degrades gracefully, so self-references resolve cleanly during the body check while post-body inference (#575) still records the concrete return type for external callers. Void actions are still recorded as `Nothing` externally, preserving existing behavior. Adds regression tests covering the reported repro and the Scribe `scribe_p_unary` shape that negates its recursive result. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Qykg1eQ2bJKx2uoJNBGPj * test: consolidate recursive-action regression tests and tighten assertion (#590) Address review feedback on PR #591: extract the shared lex/parse/typecheck flow into `assert_typechecks_clean`, and assert the programs type-check with zero diagnostics (`result.is_ok()`) instead of only checking for the absence of one error substring. The tighter guard catches both a re-introduced "Cannot index into Nothing" error and any new spurious diagnostic on the recursive path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Qykg1eQ2bJKx2uoJNBGPj --------- Co-authored-by: Claude <noreply@anthropic.com>
…#599) * fix: infer action return types through try blocks and for container methods (#560) Two residual shapes of issue #560 still produced false 'Cannot index into Nothing' diagnostics after #575/#591: - collect_return_types never descended into try statements, so an action whose only returns live inside a try body, when-error clause, otherwise, or finally block was inferred as returning Nothing. It now traverses TryStatement and WaitForStatement (check_return_statements kept in sync). - Container methods were registered with return_type Nothing when unannotated and never refined, so instance.method() results hit the same false error. The analyzer now seeds unannotated methods with a provisional Unknown, and the type checker infers the real return type from each method body (parameters in scope, mirroring the top-level action arm) and writes it back to the container registry via a new Analyzer::get_container_mut. Inherited method calls read the same registry entries, so they are fixed too. Static-diagnostics-only change; runtime behavior is unchanged. TDD: tests/action_return_type_residuals_test.rs was confirmed failing (4/4) before the fix and passes after, alongside the full test suite and all TestPrograms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016urN3UdbLEQaGtHvNtLC7k * fix: refine static container method return types and validate annotated method returns Address CodeRabbit review on #599: - Static methods were seeded with the provisional Unknown but never refined: the ContainerDefinition arm only iterated instance methods. Value-returning statics stayed Unknown forever and void statics lost their previous Nothing type. Static methods now go through the same body-check + infer + write-back loop, updating container_info.static_methods. (Static method calls remain a runtime future feature; the registry refinement keeps Container.method member access accurate and restores Nothing for void statics.) - Annotated container methods (action name: Type) now have their return statements validated against the annotation via check_return_statements, mirroring the top-level action arm. - Added a registry-level unit test pinning both static cases (inferred List for a value-returning static, Nothing for a void static), since a typecheck-clean integration test cannot observe static calls that the runtime rejects. Dev diary updated to match the implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016urN3UdbLEQaGtHvNtLC7k * chore: remove stray test artifacts accidentally committed flush_test_*.txt, test_output.txt, and a google_index.html overwrite were produced by running the TestPrograms suite locally and swept in by git add -A. Remove the artifacts and restore google_index.html to its prior content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016urN3UdbLEQaGtHvNtLC7k --------- Co-authored-by: Claude <noreply@anthropic.com>
The type checker defaulted every action's return type to
Nothing(WFL has no return-type annotation syntax), so a
call <action>resultwas typed
Nothingat the call site. When that result fed a builtinposition requiring
Text—respond to req with ...,open file at ...,list indexing, etc. — the checker emitted a spurious
"Expected Text but found Nothing" error even though the value was correct
at runtime. This produced a wall of false positives in any program that
factors request handling / file reading into actions (issue #569).
Infer an action's return type from its
returnexpressions: collect thetype of every reachable return, merge them (identical types collapse;
differing/un-inferrable types widen to a permissive type), and update the
action's symbol so call sites carry the real result type. Inference runs
after the body is checked (so body-locals and parameters are in scope) and
the symbol update is deferred until the parameter scope is popped, since
get_symbol_mutonly reaches the innermost scope.Genuine mismatches are still reported: an action returning
Numberusedwhere
Textis required now correctly reportsNumber(with a helpfulconversion hint) instead of
Nothing.Verified against all TestPrograms: the only change in type-check output is
the removal of false positives (e.g. database_sqlite_test.wfl no longer
reports "Cannot index into Nothing" on action-returned query rows); no new
diagnostics or regressions.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01KYZY9RXYzxp4WHhZxqVjZt
Summary by CodeRabbit
New Features
Bug Fixes