Fix parser, analyzer, and typechecker for documented forms and includes - #550
Merged
Conversation
…, #548, #549) Addresses five reported issues: - #549 (parser): accept the documented/natural spellings `substring of X from START length LEN`, `split of X by DELIM`, and `check if n is not equal to N`. The `of ... and ...` builtin-call collector now treats `from`, `by`, and `length` as argument separators; the `split` keyword handler optionally consumes a leading `of`; and the `is not` comparison branch consumes an optional trailing `equal`/`to`. - #547 (analyzer): stdlib functions called inside an included file's action were reported as "'X' is not a function". The FunctionCall analysis branch now recognizes builtin functions before checking scope symbols, matching the Variable and ActionCall branches. - #548 (analyzer/typechecker): calling an include-exposed action from a top-level statement raised a fatal "Undefined action" that aborted the program before the include ran. Both the analyzer and type checker now detect `include from` and suppress undefined-action errors, since included files expose actions dynamically at runtime. Undefined actions in programs without includes still error as before. - #468 (analyzer): `add ... to X`, `respond ... with X`, and uses inside `main loop`/`try`/`describe`/`test` blocks were not counted as variable uses, producing false ANALYZE-UNUSED warnings. Added the missing arms to the use-tracking visitor. - #467 was already fixed on disk (chained `respond ... and status N and content_type "X"`); verified end-to-end and covered by existing tests. Adds tests/docs_parser_and_include_fixes_test.rs covering all cases plus regression guards. All existing tests pass; fmt and clippy are clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUG4otZGqWhSAvoamQ9G9L
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds include-aware diagnostics to semantic analysis and type checking, extends unused-variable tracking into more nested statements, broadens parser acceptance for several phrasings, and adds regression coverage for the updated behavior. ChangesLanguage analysis and parser fixes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Program
participant Analyzer
participant TypeChecker
Program->>Analyzer: analyze(statements)
Analyzer->>Analyzer: scan for include from
Analyzer->>Analyzer: resolve function and action calls
alt include present
Analyzer-->>Program: record warning for undefined action
else no include
Analyzer-->>Program: report fatal undefined action
end
Program->>TypeChecker: check_types(statements)
TypeChecker->>TypeChecker: scan for include from
TypeChecker->>TypeChecker: infer action call type
alt include present
TypeChecker-->>Program: return Type::Any
else no include
TypeChecker-->>Program: report undefined action error
end
Related issues: 🚥 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/parser/expr/primary.rs (1)
1110-1137: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"length" separator check is case-sensitive, unlike similar bareword checks elsewhere in this file.
Line 1121 compares
id == "length"directly, whereas the existing"with"bareword check at line 230 usesid.to_lowercase() == "with". A user writingLengthinstead oflengthinsubstring of X from START length LENwill silently fail to match this separator, breaking argument parsing for that call.🔧 Proposed fix
) || matches!( &sep_token.token, - Token::Identifier(id) if id == "length" + Token::Identifier(id) if id.to_lowercase() == "length" );🤖 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/parser/expr/primary.rs` around lines 1110 - 1137, The separator handling in parse_primary_expression treats the bareword "length" case-sensitively, unlike the other bareword checks in this file. Update the Identifier(id) branch in the separator match to compare case-insensitively the same way the "with" check does, so variants like "Length" are accepted when parsing stdlib-style calls such as substring of X from START length LEN.
🧹 Nitpick comments (3)
src/analyzer/static_analyzer.rs (1)
667-731: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDeclaration collection doesn't mirror new usage recursion.
mark_used_variablesnow recurses intoMainLoop/ForeverLoop/SingleLineIf/TryStatement/DescribeBlock/TestBlockbodies to mark variables as used, butcollect_variable_declarations(lines 372-459) still only recurses intoIfStatement,WhileLoop/ForEachLoop/CountLoop, andActionDefinition. Variables declared inside amain loop,tryblock,describe/testblock, or a single-lineifbody are never inserted intousagesin the first place, so they will silently escape unused-variable detection (a false negative) rather than being properly tracked. This asymmetry means the fix only helps "used elsewhere, declared outside" cases but leaves declarations inside these new block types unchecked.♻️ Suggested fix: mirror the new recursion in `collect_variable_declarations`
Statement::WhileLoop { body, .. } | Statement::ForEachLoop { body, .. } | Statement::CountLoop { body, .. } => { for stmt in body { self.collect_variable_declarations(stmt, usages); } } + Statement::MainLoop { body, .. } | Statement::ForeverLoop { body, .. } => { + for stmt in body { + self.collect_variable_declarations(stmt, usages); + } + } + Statement::SingleLineIf { + then_stmt, + else_stmt, + .. + } => { + self.collect_variable_declarations(then_stmt, usages); + if let Some(else_stmt) = else_stmt { + self.collect_variable_declarations(else_stmt, usages); + } + } + Statement::TryStatement { + body, + when_clauses, + otherwise_block, + .. + } => { + for stmt in body { + self.collect_variable_declarations(stmt, usages); + } + for clause in when_clauses { + for stmt in &clause.body { + self.collect_variable_declarations(stmt, usages); + } + } + if let Some(otherwise) = otherwise_block { + for stmt in otherwise { + self.collect_variable_declarations(stmt, usages); + } + } + }🤖 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/static_analyzer.rs` around lines 667 - 731, `collect_variable_declarations` is missing the same nested-block recursion now added in `mark_used_variables`, so declarations inside `MainLoop`, `ForeverLoop`, `SingleLineIf`, `TryStatement`, `DescribeBlock`, and `TestBlock` are never tracked for unused-variable checks. Update `collect_variable_declarations` in `static_analyzer.rs` to mirror the traversal logic used by `mark_used_variables`, recursing into those statement variants and their bodies/clauses so variables declared there are inserted into `usages` and analyzed consistently.src/parser/expr/binary.rs (1)
442-449: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated "optional of" consumption logic.
The same optional-
of-consumption block is duplicated insrc/parser/expr/primary.rs(lines 819-827). Consider extracting a small shared helper (e.g.consume_optional_of(&mut self)) to avoid keeping two copies in sync.♻️ Example shared helper
+fn consume_optional_of(&mut self) { + if let Some(of_token) = self.cursor.peek() + && matches!(&of_token.token, Token::KeywordOf) + { + self.bump_sync(); + } +}Then call
self.consume_optional_of();at both call sites.🤖 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/parser/expr/binary.rs` around lines 442 - 449, The optional "of" consumption logic is duplicated in both parsing paths, so extract it into a small shared helper on the parser, such as consume_optional_of, and call that helper from the existing split-handling code in binary and primary parsing. Keep the helper responsible only for peeking Token::KeywordOf and bumping it, so both call sites stay consistent and the logic is maintained in one place.src/analyzer/mod.rs (1)
372-382: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude-detection scan is duplicated between Analyzer and TypeChecker.
The same
program.statements.iter().any(|s| matches!(s, Statement::IncludeStatement { .. }))scan appears independently in bothAnalyzer::analyzeandTypeChecker::check_types(src/typechecker/mod.rs). Consider extracting a shared helper (e.g.,fn program_has_includes(program: &Program) -> bool) to avoid drift if the include-detection logic ever needs to change (e.g., to detect nested includes).🤖 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 372 - 382, The include-statement scan is duplicated in Analyzer::analyze and TypeChecker::check_types, so factor the shared `program.statements.iter().any(|s| matches!(s, Statement::IncludeStatement { .. }))` logic into a common helper such as `program_has_includes(&Program) -> bool`. Update both Analyzer and TypeChecker to call the helper so include detection stays consistent if the rule changes later, and keep the existing `has_includes` handling intact.
🤖 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/analyzer/mod.rs`:
- Around line 164-168: The undefined-action handling in analyze() is too broad
because has_includes suppresses every Expression::ActionCall “Undefined action”
error whenever any Statement::IncludeStatement exists. Update the logic around
has_includes and the ActionCall diagnostic so include presence only lowers
severity (for example to a warning) or narrows suppression to cases that could
plausibly come from includes, instead of blanket-skipping all undefined actions.
Use the existing analyze(), has_includes, and Expression::ActionCall paths to
keep genuine typos reported while still accounting for dynamically resolved
includes.
---
Outside diff comments:
In `@src/parser/expr/primary.rs`:
- Around line 1110-1137: The separator handling in parse_primary_expression
treats the bareword "length" case-sensitively, unlike the other bareword checks
in this file. Update the Identifier(id) branch in the separator match to compare
case-insensitively the same way the "with" check does, so variants like "Length"
are accepted when parsing stdlib-style calls such as substring of X from START
length LEN.
---
Nitpick comments:
In `@src/analyzer/mod.rs`:
- Around line 372-382: The include-statement scan is duplicated in
Analyzer::analyze and TypeChecker::check_types, so factor the shared
`program.statements.iter().any(|s| matches!(s, Statement::IncludeStatement { ..
}))` logic into a common helper such as `program_has_includes(&Program) ->
bool`. Update both Analyzer and TypeChecker to call the helper so include
detection stays consistent if the rule changes later, and keep the existing
`has_includes` handling intact.
In `@src/analyzer/static_analyzer.rs`:
- Around line 667-731: `collect_variable_declarations` is missing the same
nested-block recursion now added in `mark_used_variables`, so declarations
inside `MainLoop`, `ForeverLoop`, `SingleLineIf`, `TryStatement`,
`DescribeBlock`, and `TestBlock` are never tracked for unused-variable checks.
Update `collect_variable_declarations` in `static_analyzer.rs` to mirror the
traversal logic used by `mark_used_variables`, recursing into those statement
variants and their bodies/clauses so variables declared there are inserted into
`usages` and analyzed consistently.
In `@src/parser/expr/binary.rs`:
- Around line 442-449: The optional "of" consumption logic is duplicated in both
parsing paths, so extract it into a small shared helper on the parser, such as
consume_optional_of, and call that helper from the existing split-handling code
in binary and primary parsing. Keep the helper responsible only for peeking
Token::KeywordOf and bumping it, so both call sites stay consistent and the
logic is maintained in one place.
🪄 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: c3837e0e-cc84-4ead-a22b-e982e1280db8
📒 Files selected for processing (6)
src/analyzer/mod.rssrc/analyzer/static_analyzer.rssrc/parser/expr/binary.rssrc/parser/expr/primary.rssrc/typechecker/mod.rstests/docs_parser_and_include_fixes_test.rs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns WFL’s parser/analyzer/typechecker behavior with documented natural-language syntax and improves include-related semantics, while also fixing static analyzer variable-usage tracking to reduce false unused-variable warnings.
Changes:
- Parser: accepts documented forms for
substring ... from ... length ...,split of ... by ..., andis not equal to. - Analyzer/TypeChecker: detect
include fromusage and relax undefined-action reporting/inference to avoid include-driven false failures. - Static analyzer + tests: recursively count variable usage in nested blocks and additional statement kinds; adds regression tests covering all four reported issues.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/docs_parser_and_include_fixes_test.rs | Adds regression tests for parser forms, include behavior, and unused-variable analysis fixes. |
| src/typechecker/mod.rs | Tracks presence of includes to suppress undefined-action type errors and infer Any for runtime-provided actions. |
| src/parser/expr/primary.rs | Extends call-argument separator parsing and adds optional of handling for split. |
| src/parser/expr/binary.rs | Supports is not equal to token sequence; adds optional of handling for split in binary parsing. |
| src/analyzer/static_analyzer.rs | Recursively marks variables used inside nested statement blocks and additional statement kinds (incl. respond, list ops, loops). |
| src/analyzer/mod.rs | Detects includes; suppresses undefined-action errors when includes are present; adds builtin-guard for include scopes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2079
to
+2083
| if Self::is_builtin_function(name) { | ||
| // Built-in stdlib functions (touppercase, wflhash256, | ||
| // parse_json, ...) are always callable. When an included | ||
| // file is analyzed, parent-scope bindings for these | ||
| // natives are injected as plain variables, so without this |
Follow-ups from CodeRabbit and Copilot review of PR #550: - analyzer: the builtin-function guard in the FunctionCall branch no longer runs before symbol resolution. It now only applies when the resolved symbol is a non-function (the include-injected variable case) or is unresolved, so arity/signature validation is preserved for builtins registered as real Function symbols. - parser: the `length` argument separator is matched case-insensitively (via eq_ignore_ascii_case), consistent with other bareword pseudo-keywords. - parser: extracted the duplicated optional-`of` consumption into a shared Parser::consume_optional_of helper used by both split handlers. - analyzer/typechecker: extracted the duplicated include-detection scan into a shared analyzer::program_has_includes helper. - static analyzer: collect_variable_declarations now mirrors the recursion in mark_used_variables (main loop, forever loop, single-line if, try, describe, test blocks) so variables declared inside those blocks are tracked for unused-variable analysis. Adds a regression test for the case-insensitive `length` separator. All tests, fmt, and clippy pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUG4otZGqWhSAvoamQ9G9L
Previously, when a program used `include from`, all "Undefined action" errors were fully suppressed to avoid fatally aborting before the include ran (issue #548). CodeRabbit correctly noted this is too broad: a genuine typo (e.g. `call grret` for `greet`) in a file that also includes a module would go completely unreported. The analyzer now collects such cases as non-fatal warnings instead of suppressing them. The program still runs to completion (the action may be provided by an included module at runtime), but genuine typos are surfaced with a note explaining both possibilities. Programs without includes still report undefined actions as fatal errors, unchanged. The type checker continues to infer Type::Any silently so the warning is not duplicated. Adds a warnings channel + getter to the Analyzer and emits them as Severity::Warning diagnostics from analyze_static. Updates the include regression test and adds one asserting typos are surfaced as warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUG4otZGqWhSAvoamQ9G9L
| // callable; only genuinely non-callable symbols are | ||
| // an error. Real builtin Function symbols take the | ||
| // arm above, so their arity checks are preserved. | ||
| if Self::is_builtin_function(name) { |
Comment on lines
+195
to
+198
| // Detect includes: their exposed actions are only known at runtime. | ||
| if crate::analyzer::program_has_includes(program) { | ||
| self.has_includes = true; | ||
| } |
Comment on lines
+2760
to
+2764
| } else if self.has_includes { | ||
| // Action may be provided by an included file at runtime; | ||
| // its result type is unknowable statically, so treat it as | ||
| // Any to avoid cascading "could not infer type" errors. | ||
| return Type::Any; |
Three follow-ups from Copilot's review of PR #550: - analyzer/typechecker: `has_includes` is now assigned directly from program_has_includes() on each run instead of only being set to true, so a reused analyzer/typechecker instance (e.g. an editor session) can't carry a stale flag from a previous program that used includes. - analyzer: the builtin-call relaxation is restricted to include-injected parent-scope symbols (defined at position 0:0). A user who shadows a builtin name with a non-function value (e.g. `store touppercase as "x"`) and then calls it still gets an "is not a function" error instead of a silent pass. - typechecker: in the include-present undefined-action path, each argument expression is now inferred before returning Type::Any, so type errors inside the arguments are not missed in include-using programs. Adds a regression test for the builtin-shadowing case. All tests, fmt, and clippy pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUG4otZGqWhSAvoamQ9G9L
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
logbie
added a commit
that referenced
this pull request
Jul 3, 2026
* fix(typechecker): infer builtin return types inside included files (#551) Follow-on to #550: the analyzer half was fixed, but the type checker still aborted with a fatal "Could not infer type for variable 'X'" when an included file bound a builtin-function result to a variable (store full as wflhash256 of s). Root cause: the include path injects every parent-environment binding into the included file's analyzer scope — including stdlib builtins, whose runtime value is a NativeFunction and therefore gets recorded as a plain variable symbol with type Unknown at position 0:0. That symbol shadowed the type checker's builtin-signature fallback, so the call expression inferred Unknown and the variable declaration errored. Fixes: - infer_expression_type's Variable branch now resolves include-injected builtin symbols (position 0:0, Unknown/absent type, name registered as a builtin) to their real builtin Function signature, mirroring the analyzer's #550 treatment. A user who shadows a builtin with a concrete value keeps that value's type and the existing behavior. - get_builtin_function_type gains explicit entries for parse_json (Any), stringify_json/stringify_json_pretty (Text), and string_split (List<Text>), and its fallback for registered-but-unlisted builtins is now Any instead of Unknown, so builtin results stay inferable (previously parse_json produced a spurious "Could not infer type" even in the main file). Adds four regression tests to docs_parser_and_include_fixes_test.rs: wflhash256 and parse_json results stored in variables inside included actions, a top-level store in an included file, and a main-file parse_json inference guard. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AhKSguwesKWJAGL6ez1Bue * refactor(typechecker): explicit return types for known builtins Review follow-up on PR #552: the blanket `_ => Type::Any` fallback in get_builtin_function_type made builtins with known return types (print, to_uppercase, generate_uuid, ...) appear to return Any, weakening type checking — a Nothing-returning call could look usable as a value. Add explicit entries mirroring the stdlib registrations and runtime implementations: void functions (print, sleep, foreach, filesystem mutations) return Nothing; underscore text aliases, generate_uuid, generate_csrf_token, current_date, format_datetime, and path helpers return Text; predicates (every, some, pattern_matches, path_matches, path_exists, ...) return Boolean; sizes/counters return Number; directory listings and pattern_find_all return lists; path_params returns a Text->Text map. Genuinely dynamic results (parse_json, query parsers, DateTime values) are explicitly Any. The Any fallback now only covers test helpers and not-yet-implemented placeholder names. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AhKSguwesKWJAGL6ez1Bue --------- Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Jul 13, 2026
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
This PR fixes four reported issues affecting the parser, analyzer, and typechecker:
substring ... from ... length,split ... by ..., andis not equal to#549: Parser now accepts documented natural-language forms forsubstring,split, and comparison operatorsadd 1 to Xnot counted as a use ofX(ANALYZE-UNUSED false positive) #468: Variables used inadd/respondstatements and nested blocks are now properly counted as usedKey Changes
Parser (
src/parser/expr/primary.rsandsrc/parser/expr/binary.rs)substring of X from START length LENby accepting optionalfromandlengthseparators in function call argumentssplit of X by DELIMby optionally consumingofkeyword before the text expressionis not equal toform by consuming optionalequalandtokeywords afternotin binary expressionsAnalyzer (
src/analyzer/mod.rsandsrc/analyzer/static_analyzer.rs)has_includesflag to detectinclude fromstatements upfrontmark_used_variables()to recursively traverse nested statement blocks (MainLoop,ForeverLoop,SingleLineIf,TryStatement,DescribeBlock,TestBlock) and compound-assignment statements (AddToListStatement,RemoveFromListStatement,RespondStatement, etc.) to properly count variables as used (fixes Analyzer:add 1 to Xnot counted as a use ofX(ANALYZE-UNUSED false positive) #468)TypeChecker (
src/typechecker/mod.rs)has_includesflag matching analyzer behaviorType::Anyfor actions that may be provided by included files at runtime, avoiding cascading type inference errorsTests (
tests/docs_parser_and_include_fixes_test.rs)https://claude.ai/code/session_01VUG4otZGqWhSAvoamQ9G9L
Summary by CodeRabbit
split of X by ...,is not equal to ..., and additional member-call argument separators (includinglength).include frombehavior so undefined actions can be reported as non-fatal warnings (and type-checking aligns), while truly undefined actions still error when no includes exist.