Fix of form to resolve include-exposed actions in all contexts - #581
Conversation
Issue #548 relaxed the fatal "Undefined action" for include-exposed actions, but only for the `call <action> with <arg>` form. The idiomatic `<action> of <arg>` form parses to `Expression::FunctionCall` whose callee is a bare `Variable`, a path that never reached #548's relaxation — so it stayed fatal at top level and inside action bodies (while incidentally "working" inside `main loop`/`describe`/`test`, whose bodies the analyzer does not descend into). This made natural multi-file libraries unusable through their idiomatic top-level API. Analyzer (`src/analyzer/mod.rs`): in the `FunctionCall` arm, stop recursing into a bare-`Variable` callee (which reported it as fatal before the block could relax it) and resolve it inline. When the callee is unresolved, not a builtin/parameter, and the program uses `include from`, emit the same non-fatal "Undefined action" warning as the `ActionCall` path; otherwise fall back to the pre-existing `report_undefined_name` behavior (preserving the `try_depth` downgrade and the no-include fatal). Type checker (`src/typechecker/mod.rs`): mirror the `ActionCall` relaxation in the `FunctionCall` arm — an unresolved-`Variable` callee in an include-using program yields `Type::Any` instead of `Unknown`, avoiding the spurious "could not infer type" cascade the `call` form already avoids. The same change fixes the #547-class nested-include case: a file that references (via `of`) an action from a file it itself includes now passes its isolated analysis, because it has a top-level `include` and so takes the relaxation. Adds `tests/include_of_form_resolution_test.rs`: the `of` form across top-level / action body / container action / main loop / describe-test, multi-arg and chained `of`, the nested-include scenario, plus guard tests (undefined-without-include stays fatal, typo-with-include is a warning, undefined argument still reported, `call ... with` unregressed) and a snapshot pinning that main-loop bodies are currently not statically analyzed (recommended follow-up). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj9BVovsRf5DdNWkeuf3wd
|
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: 31 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. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe analyzer defers callee analysis for bare-variable FunctionCall expressions, relaxing undefined-name errors to warnings when ChangesInclude-aware of-form resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SourceProgram
participant Analyzer
participant TypeChecker
participant IncludeState as "has_includes"
SourceProgram->>Analyzer: analyze_expression(FunctionCall with bare-variable callee)
Analyzer->>Analyzer: check scope for callee name
alt name resolved in scope
Analyzer->>Analyzer: analyze arguments
else name is builtin, action param, or count
Analyzer->>Analyzer: treat as callable, analyze arguments
else name unresolved
Analyzer->>IncludeState: check include from present
alt include from present
Analyzer->>Analyzer: emit "Undefined action" warning
else no includes
Analyzer->>Analyzer: report_undefined_name (fatal)
end
Analyzer->>Analyzer: analyze arguments
end
SourceProgram->>TypeChecker: infer_expression_type(FunctionCall)
TypeChecker->>TypeChecker: check if callee is unresolvable bare variable
TypeChecker->>IncludeState: check include from present
alt include from present
TypeChecker->>TypeChecker: infer argument types
TypeChecker-->>SourceProgram: return Type::Any
else no includes
TypeChecker->>TypeChecker: standard function-type inference
TypeChecker-->>SourceProgram: return inferred type or error
end
Possibly related issues
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/analyzer/mod.rs (1)
2274-2374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared include-aware relaxation logic.
This fallback block (builtin/param/count pass-through →
has_includeswarning → fatal otherwise) is structurally near-identical to theActionCallrelaxation block at Lines 2593-2613. Duplicating this pattern is exactly how issue#580happened in the first place:call ... withgot the#548relaxation but theofform's separate code path didn't. Extracting a shared helper (e.g.fn resolve_undefined_callee(&mut self, name: &str, line, column, arguments)) would prevent a similar path from drifting out of sync again in the future.♻️ Sketch of a shared helper
fn report_undefined_callee_relaxed( &mut self, label: &str, // "Variable" or "action" name: &str, line: usize, column: usize, ) { if self.has_includes { self.warnings.push(SemanticError::new( format!("Undefined {label} '{name}'"), line, column, )); } else { self.report_undefined_name(format!("{label} '{name}' is not defined"), line, column); } }🤖 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 2274 - 2374, The include-aware undefined-callee handling is duplicated here and in the ActionCall path, so extract the shared fallback into a helper to keep the builtin/parameter/count pass-through, has_includes warning, and fatal-otherwise behavior in one place. Move the common logic out of the current `Expression::Variable` function-call branch and the `ActionCall` relaxation block into a shared method such as `resolve_undefined_callee` or `report_undefined_callee_relaxed`, then have both paths call it so future changes stay in sync.src/typechecker/mod.rs (1)
2662-2681: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect fix; mirrors the analyzer relaxation.
The early-return correctly avoids downgrading the call result to
Unknown(which would otherwise trigger a spurious "Could not infer type" at thestoresite) by returningAnyinstead, while still inferring argument types so nested type errors aren't lost.One maintainability note:
is_knownhere duplicates the equivalent check insideActionCall's inference path (Lines 3062-3097). Since this exact kind of duplication is what let issue#580slip through in the analyzer, consider extracting a small shared helper (e.g.fn is_statically_known_callee(&self, name: &str) -> bool) used by bothFunctionCallandActionCallinference to keep them from diverging again.🤖 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 2662 - 2681, The fix is correct, but the callee-knowledge check in the FunctionCall path duplicates the same logic used in ActionCall inference, which can drift over time. Extract the shared “is this callee statically known?” check into a small helper on the analyzer or typechecker (for example, a method used by both FunctionCall and ActionCall handling), and update both paths in mod.rs to call it so the `get_symbol`/builtin/action-parameter logic stays consistent in one place.
🤖 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.
Nitpick comments:
In `@src/analyzer/mod.rs`:
- Around line 2274-2374: The include-aware undefined-callee handling is
duplicated here and in the ActionCall path, so extract the shared fallback into
a helper to keep the builtin/parameter/count pass-through, has_includes warning,
and fatal-otherwise behavior in one place. Move the common logic out of the
current `Expression::Variable` function-call branch and the `ActionCall`
relaxation block into a shared method such as `resolve_undefined_callee` or
`report_undefined_callee_relaxed`, then have both paths call it so future
changes stay in sync.
In `@src/typechecker/mod.rs`:
- Around line 2662-2681: The fix is correct, but the callee-knowledge check in
the FunctionCall path duplicates the same logic used in ActionCall inference,
which can drift over time. Extract the shared “is this callee statically known?”
check into a small helper on the analyzer or typechecker (for example, a method
used by both FunctionCall and ActionCall handling), and update both paths in
mod.rs to call it so the `get_symbol`/builtin/action-parameter logic stays
consistent in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a7b00a5-ed0e-4ce7-85b4-cb05b1a18f75
📒 Files selected for processing (3)
src/analyzer/mod.rssrc/typechecker/mod.rstests/include_of_form_resolution_test.rs
…580) Addresses CodeRabbit review feedback on PR #581. Issue #580 was caused by the `of` form (`FunctionCall`) and `call ... with` form (`ActionCall`) maintaining separate copies of the include-aware undefined-callee logic, so #548's relaxation reached only one of them. Consolidate the shared decision so the two paths cannot drift apart again. No behavior change. Analyzer (`src/analyzer/mod.rs`): extract `warn_undefined_callee_if_includes`, which owns the identical part — the `has_includes` check and the non-fatal "Undefined action" warning — and returns whether the caller must still emit its own fatal diagnostic. Both the `FunctionCall` bare-Variable-callee tail and the `ActionCall` tail route through it; each keeps its distinct fatal path (the `of` form's try_depth-aware `report_undefined_name` "Variable '…' is not defined" vs. the `call` form's "Undefined action" error). Type checker (`src/typechecker/mod.rs`): extract `is_callable_without_symbol` (builtins, action parameters, internal test stubs) and use it in both the `FunctionCall` and `ActionCall` "is this callee statically known?" checks. Verified unchanged: analyzer/typechecker lib unit tests (465), `docs_parser_and_include_fixes_test` (#547/#548/#551/#553), `include_of_form_resolution_test` (13), full suite, clippy -D warnings, fmt, and 102/102 TestPrograms integration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj9BVovsRf5DdNWkeuf3wd
Summary
Fixes issue #580 where the idiomatic
ofform (e.g.,greet of "bob") failed to resolve actions exposed byinclude fromstatements in top-level code and action bodies, while thecall ... withform worked correctly. The root cause was that theofform parses asFunctionCall { function: Variable(..), .. }and was being analyzed as a fatal undefined variable before the include-aware relaxation could apply.Key Changes
Analyzer (
src/analyzer/mod.rs)Variablecallees inFunctionCallto prevent premature fatal "undefined variable" errorsVariablecallees that mirrors theActionCallrelaxation:has_includesis true, emit a non-fatal warning instead of a fatal error (allowing runtime resolution by included files)countloop variableType Checker (
src/typechecker/mod.rs)Variablecallees whenhas_includesis true and the callee is not statically knownType::Anyfor such cases (matching theActionCallbehavior) to avoid cascading "could not infer type" errorsTest Coverage
Comprehensive regression test suite (
tests/include_of_form_resolution_test.rs) covering:ofform in top-level statements, action bodies, container actions, main loops, and test blocksofcallscall ... withform continues to workThis fix ensures the
ofform is as capable as thecall ... withform while maintaining type safety and error reporting for genuinely undefined names.https://claude.ai/code/session_01Kj9BVovsRf5DdNWkeuf3wd
Summary by CodeRabbit
of-style function calls so valid calls are no longer flagged as undefined too early.