Fix issue #560 residuals: returns in try blocks and container methods - #599
Merged
Conversation
…ethods (#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
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe type checker now traverses returns inside ChangesReturn-type residual fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/typechecker/mod.rs (1)
1652-1663: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnannotated static container methods are never refined —
static_methodsis destructured but unused.The new loop at Lines 1739-1781 only iterates
methods(instance methods).static_methodsis bound as_static_methods(Line 1660) and dropped entirely. Combined with the analyzer change (seeding both instance and static unannotated methods withType::Unknown), this means:
- Value-returning unannotated static methods stay
Unknownforever instead of getting the real inferred type (defeats the stated purpose for statics).- Void unannotated static methods, which previously defaulted to
Nothing, now also stayUnknown— a real strictness regression: indexing/misusing a void static method's result will no longer be caught.Static methods should get the same body-check + infer + write-back treatment as instance methods, updating
container_info.static_methodsviaget_container_mutafterward.🐛 Proposed fix to also process static methods
Statement::ContainerDefinition { name: _name, extends, implements, properties, methods, events: _events, static_properties: _static_properties, - static_methods: _static_methods, + static_methods, line, column, } => { @@ let mut inferred_method_returns: Vec<(String, Type)> = Vec::new(); - for method in methods { + 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, .. } = method { // Set container context for method body analysis let previous_container = self.current_container.clone(); self.current_container = Some(_name.clone()); @@ if return_type.is_none() { let inferred = self.infer_action_return_type(body); - inferred_method_returns.push((method_name.clone(), inferred)); + if is_static { + inferred_static_method_returns.push((method_name.clone(), inferred)); + } else { + inferred_method_returns.push((method_name.clone(), inferred)); + } } @@ 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; + } + } }Also applies to: 1739-1790
🤖 Prompt for 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. In `@src/typechecker/mod.rs` around lines 1652 - 1663, Process static methods alongside instance methods in the ContainerDefinition handling: bind static_methods instead of discarding it, run each unannotated static method through the same body-checking and type-inference logic as the methods loop, and write the inferred type back to container_info.static_methods using get_container_mut. Preserve annotated methods and ensure void static methods are refined to Nothing rather than remaining Unknown.src/analyzer/mod.rs (1)
1343-1352: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStatic methods are seeded as
Unknownhere but never refined downstream.This seed is consistent with the instance-method change, but per the review of
src/typechecker/mod.rs'sContainerDefinitionarm,static_methodsis never iterated to infer/write back the real return type. As a result, thisUnknownseed becomes permanent for every unannotated static method, including void ones that used to resolve toNothing. See the corresponding comment insrc/typechecker/mod.rsfor the fix.🤖 Prompt for 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. In `@src/analyzer/mod.rs` around lines 1343 - 1352, Update the static-method handling that creates the MethodInfo in the relevant analyzer branch and the corresponding ContainerDefinition logic in the typechecker: ensure static_methods are iterated for return-type inference and the inferred types are written back, so the provisional Type::Unknown seed is refined for unannotated methods, including void methods resolving to Nothing.
🧹 Nitpick comments (2)
src/typechecker/mod.rs (2)
1771-1774: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
check_return_statementsis never invoked for annotated container methods.Unlike the top-level
ActionDefinitionarm (Line 796-798:if let Some(ret_type) = return_type { self.check_return_statements(...) }), the container-method loop only handles thereturn_type.is_none()branch. Currently inert since WFL has no return-type annotation syntax, but worth mirroring for consistency/future-proofing given the arm explicitly aims to mirror the top-level handling.🤖 Prompt for 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. In `@src/typechecker/mod.rs` around lines 1771 - 1774, Update the container-method handling near infer_action_return_type to mirror the top-level ActionDefinition logic: when return_type is Some, invoke check_return_statements with the method body and annotated return type; retain inference and inferred_method_returns for the None branch.
3886-3911: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared
try/wait forsub-statement traversal.
collect_return_typesandcheck_return_statementsnow contain near-identicalTryStatement/WaitForStatementdestructuring and iteration logic, differing only in which recursive call they make. A small helper returning the list of contained statement slices (body, eachwhenbody,otherwise,finally, wait-for inner) would let both callers loop over it, reducing duplication and the risk of the two traversals drifting apart again in the future.Also applies to: 4002-4029
🤖 Prompt for 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. In `@src/typechecker/mod.rs` around lines 3886 - 3911, Extract the duplicated TryStatement/WaitForStatement traversal from collect_return_types and check_return_statements into a shared helper that returns or iterates over the contained statement slices: try body, each when clause body, otherwise, finally, and wait-for inner statement. Update both callers to use this helper while preserving their respective recursive operations, ensuring traversal behavior remains consistent.Source: Linters/SAST tools
🤖 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 `@Dev` diary/2026-07-10-issue-560-residuals-try-returns-container-methods.md:
- Around line 70-83: The ContainerDefinition type-checking logic refines only
instance methods, leaving unannotated static methods as Unknown. Update the
relevant ContainerDefinition handling in the typechecker to iterate and infer
return types for static_methods as well, including void methods resolving to
Nothing; then verify the diary accurately describes both instance and static
method refinement.
In `@tests/action_return_type_residuals_test.rs`:
- Around line 82-134: Add a regression test alongside the existing residual type
tests for an unannotated static container method invoked as
Container.static_method(). Define a container with a static action returning a
list, assign the call result, index it, and display the element via
assert_typechecks_clean; name the test to clearly indicate static container
method results are not typed Nothing.
---
Outside diff comments:
In `@src/analyzer/mod.rs`:
- Around line 1343-1352: Update the static-method handling that creates the
MethodInfo in the relevant analyzer branch and the corresponding
ContainerDefinition logic in the typechecker: ensure static_methods are iterated
for return-type inference and the inferred types are written back, so the
provisional Type::Unknown seed is refined for unannotated methods, including
void methods resolving to Nothing.
In `@src/typechecker/mod.rs`:
- Around line 1652-1663: Process static methods alongside instance methods in
the ContainerDefinition handling: bind static_methods instead of discarding it,
run each unannotated static method through the same body-checking and
type-inference logic as the methods loop, and write the inferred type back to
container_info.static_methods using get_container_mut. Preserve annotated
methods and ensure void static methods are refined to Nothing rather than
remaining Unknown.
---
Nitpick comments:
In `@src/typechecker/mod.rs`:
- Around line 1771-1774: Update the container-method handling near
infer_action_return_type to mirror the top-level ActionDefinition logic: when
return_type is Some, invoke check_return_statements with the method body and
annotated return type; retain inference and inferred_method_returns for the None
branch.
- Around line 3886-3911: Extract the duplicated TryStatement/WaitForStatement
traversal from collect_return_types and check_return_statements into a shared
helper that returns or iterates over the contained statement slices: try body,
each when clause body, otherwise, finally, and wait-for inner statement. Update
both callers to use this helper while preserving their respective recursive
operations, ensuring traversal behavior remains consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfb1581f-30fe-468c-b3a6-66fb47e0931f
📒 Files selected for processing (4)
Dev diary/2026-07-10-issue-560-residuals-try-returns-container-methods.mdsrc/analyzer/mod.rssrc/typechecker/mod.rstests/action_return_type_residuals_test.rs
…ed 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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two remaining false-positive "Cannot index into Nothing" diagnostics from issue #560, where unannotated actions and container methods that clearly return values were incorrectly typed as
Nothing:Returns inside
try:blocks were invisible to return-type inference —collect_return_typesdid not descend intotry:bodies,when errorclauses,otherwise, orfinallyblocks, causing actions whose only returns lived in these blocks to be inferred asNothing.Unannotated container methods were registered with
Nothingand never refined — mirroring the original Typechecker: unannotated action return types default to Nothing, causing false "Cannot index into Nothing" warnings #560 issue but for instance/static methods, which had no post-body inference pass.Key Changes
src/typechecker/mod.rs:collect_return_typesto descend intoTryStatement(body, allwhenclauses,otherwise,finally) andWaitForStatement, so returns in error-handling blocks are now visible to inference.check_return_statements(the annotated-method validation path) with the same arms to keep both traversals in sync.ContainerDefinitionarm: parameters are now in scope during body analysis (mirroring Type checker: more RHS forms fail inference inside included files — index / object-index / comparison /length of(follow-on to #551) #553 handling for top-level actions), and inferred return types are collected and written back to the registry viaget_container_mut.src/analyzer/mod.rs:Type::Unknown(instead ofType::Nothing), allowing graceful degradation before the type checker refines the type.get_container_mutmethod to allow the type checker to update inferred return types in the container registry after analyzing method bodies.tests/action_return_type_residuals_test.rs(new):try/when error, returns insidetry/when/otherwise, unannotated container method results, and inherited container method results all now type-check cleanly without false diagnostics.Dev diary entry documenting the investigation, root causes, and design rationale.
Implementation Details
Unknown, then refine via post-body inference.MethodCallparent-walk reads the same refined registry entries.https://claude.ai/code/session_016urN3UdbLEQaGtHvNtLC7k
Summary by CodeRabbit
try,when error,otherwise,finally, andwait forblocks so reachablereturnstatements are no longer ignored.Nothing; provisional types are refined to the actual inferred return type, while void methods remainNothing.Nothingresults at call sites.