Conversation
Fixes a parser bug that prevented recursive action calls by registering the action name before parsing its body. This allows functions like factorial to call themselves correctly. Updates `repeat while` loops in the interpreter to properly handle `break`, `continue`, `exit`, and `return` statements, ensuring correct control flow. Adds an extensive suite of test scripts to validate core language features, including recursion, various loop types, operator precedence, and string concatenation.
Adds a detailed bug report documenting a critical parser issue where binary operations in function call arguments are misparsed. This incorrect precedence handling leads to bugs like infinite recursion in recursive functions (e.g., `factorial with n minus 1` is parsed as `factorial(n) - 1`). The report outlines the root cause, impact, test cases, and a proposed solution to ensure expressions in arguments are parsed correctly.
WalkthroughThis PR addresses operator precedence issues in recursive function calls by enabling early action registration in the parser, adding special-case handling for loop variables in the typechecker, updating repeat-while loop control flow in the interpreter, and providing comprehensive test coverage through multiple Nexus workflow scripts validating recursion, loops, string concatenation, and control flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Claude finished @logbie's task —— View job PR Review: Parser Operator Precedence Bug DocumentationReviewing this pull request that documents a critical parser operator precedence bug. Todo List:
🔍 Review SummaryThis PR provides exceptional documentation of a critical parser bug and includes important foundational fixes. The work demonstrates thorough analysis and professional-quality bug reporting. ✅ StrengthsDocumentation Quality (Outstanding)
Test Coverage (Excellent)
Code Quality (Good)
📋 Detailed AnalysisCode Changes ReviewParser ( // Add the action name to our known actions BEFORE parsing the body
// This allows recursive calls to be recognized as action calls
self.known_actions.insert(name.clone());
Interpreter (
Typechecker (
Bug Documentation AssessmentThe
Test Strategy (Comprehensive)The test files cover critical scenarios:
|
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 (1)
src/interpreter/mod.rs (1)
2833-2876:RepeatWhileLoopdiscards_last_valueon normal completion (inconsistent with other loops)Inside the
RepeatWhileLooparm you now track_last_valueper iteration and correctly propagate it forExitandReturn, but the final return still usesValue::Null, so callers never see the last body value when the loop ends normally. The other loop variants (WhileLoop,RepeatUntilLoop,ForeverLoop,MainLoop) all return the last body value on normal completion.To make
RepeatWhileLoopconsistent with those semantics and with the new_last_valuetracking, consider:- Ok((Value::Null, ControlFlow::None)) + Ok((_last_value, ControlFlow::None))
🧹 Nitpick comments (8)
src/parser/mod.rs (1)
4224-4227: Earlyknown_actionsregistration correctly enables recursive action callsRegistering the action name before parsing the body is consistent with how
parse_binary_expressionusesknown_actionsto recognizename with ...asExpression::ActionCall, so this should unblock recursive calls inside the same action without impacting non‑recursive ones.One minor behavioral shift: if parsing the body later fails (e.g., missing
end action), the name will now remain inknown_actionseven though noActionDefinitionis produced. That only affects parsing heuristics, but if you wantknown_actionsto strictly mirror successfully parsed actions, you could either:
- defer insertion until after successfully consuming
end action, or- roll back the insert when
parse_action_definitionreturns an error.Also, since this changes when calls are classified as action calls, double‑check that any bytecode or interpreter logic that relies on the AST shape for actions behaves as expected for recursive definitions. As per coding guidelines, ensure any required bytecode updates/tests accompany this parser change.
Nexus/test_zero_concat.wfl (1)
3-15: Zero-concat regression test is sound; consider optional error handlingThe script correctly exercises concatenation of a numeric
0throughlog_test, writes to a log file, and closes the handle, so it should be effective as a focused regression test.If you want to align more strictly with the WFL guideline of comprehensive error handling around I/O, you could wrap the
open fileand logging calls in atry/when/otherwiseblock, but that’s optional for this small Nexus harness script.As per coding guidelines, WFL programs are encouraged to use structured
try/when/otherwisearound fallible file operations.parserbug.md (2)
15-18: Add language specifier to code block.The code block should specify the language for proper syntax highlighting and rendering.
Apply this diff:
-``` -(factorial with n) minus 1 -``` +```text +(factorial with n) minus 1 +```Based on learnings from static analysis tools.
22-25: Add language specifier to code block.The code block should specify the language for proper syntax highlighting and rendering.
Apply this diff:
-``` -factorial with (n minus 1) -``` +```text +factorial with (n minus 1) +```Based on learnings from static analysis tools.
Nexus/test_skip_loop.wfl (1)
22-23: Clarify expected result comment.The comment on line 23 is slightly confusing. The loop processes count2 values 1, 2, 3, 4 (while count2 < 5), and skips even numbers (2, 4). The odd numbers 1 and 3 sum to 4, which matches the stated expectation. However, the phrasing "loop only goes to 4" might be clearer as "loop processes values 1 through 4" to distinguish between the loop counter and the comparison value.
Apply this diff:
-display "Expected: 1+3+5 = 9, but loop only goes to 4, so actual should be 1+3 = 4" +display "Expected: Loop processes 1-4, skips evens (2,4), adds odds (1+3) = 4"Nexus/test_nested_loops.wfl (1)
1-26: Nested loop break behavior looks correct; header comment slightly misleadingThe loop logic and final check correctly verify that
breakexits only the inner loop and that the outer loop runs three times. The header comment mentions “break and exit”, but this script only covers thebreakcase; consider updating the comment or adding anexit loopvariant here for clarity and future readers.Nexus/test_factorial_parens.wfl (1)
1-14: Factorial implementation is correct; consider adding an assertion-style checkThe recursive definition using
n times (factorial with (n minus 1))cleanly exercises the precedence you want to validate. To make this a stronger regression test, consider adding acheck if result is equal to 120with PASS/FAIL output instead of only printing the raw result, so tooling can automatically detect regressions.Nexus/test_factorial.wfl (1)
1-22: Trace-style factorial test is sound; optional: add a concrete expected-value checkThe recursive implementation and logging of intermediate values are correct and useful for debugging. If you want this script to double as an automated test, you could add a small
check if fact3 is equal to 6with PASS/FAIL output after computingfactorial with 3, similar to other Nexus tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.claude/settings.local.json(1 hunks)Nexus/nexus.wfl(1 hunks)Nexus/nexus_partial.wfl(1 hunks)Nexus/test_concat.wfl(1 hunks)Nexus/test_even_check.wfl(1 hunks)Nexus/test_factorial.wfl(1 hunks)Nexus/test_factorial_inline.wfl(1 hunks)Nexus/test_factorial_parens.wfl(1 hunks)Nexus/test_inline_even.wfl(1 hunks)Nexus/test_nested_loops.wfl(1 hunks)Nexus/test_simple_call.wfl(1 hunks)Nexus/test_skip_loop.wfl(1 hunks)Nexus/test_zero_concat.wfl(1 hunks)parserbug.md(1 hunks)src/interpreter/mod.rs(2 hunks)src/parser/mod.rs(1 hunks)src/typechecker/mod.rs(2 hunks)temp1.txt(1 hunks)temp2.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.wfl
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.wfl: Use natural language syntax in WFL programs:store name as "value",check if x is greater than 5
Use comprehensive try/when/otherwise error handling in WFL programs
Utilize async/await in WFL programs for concurrent operations
Use containers (classes) in WFL for object-oriented programming when appropriate
Files:
Nexus/test_factorial_parens.wflNexus/test_factorial.wflNexus/test_even_check.wflNexus/nexus_partial.wflNexus/test_simple_call.wflNexus/nexus.wflNexus/test_concat.wflNexus/test_factorial_inline.wflNexus/test_zero_concat.wflNexus/test_skip_loop.wflNexus/test_inline_even.wflNexus/test_nested_loops.wfl
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingcargo fmt --all(see.rustfmt.toml)
Lint clean: runcargo clippy --all-targets --all-features -- -D warningswith no warnings
Usesnake_casefor function and file names in Rust
UseCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
ReviewSECURITY.md; avoid logging secrets and prefer zeroization for sensitive data in Rust codeUse Rust edition 2024 for all Rust source files
Files:
src/interpreter/mod.rssrc/parser/mod.rssrc/typechecker/mod.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.rs: Provide component documentation for all major modules in Rust source files
Implement comprehensive error diagnostics using codespan-reporting
Files:
src/interpreter/mod.rssrc/parser/mod.rssrc/typechecker/mod.rs
src/interpreter/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Apply security sanitization to subprocess execution in Rust implementation
Files:
src/interpreter/mod.rs
src/parser/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Update bytecode when modifying parser in Rust source code
Files:
src/parser/mod.rs
🧠 Learnings (3)
📚 Learning: 2025-12-05T10:17:06.447Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.447Z
Learning: Applies to TestPrograms/**/*.wfl : All TestPrograms/*.wfl files MUST pass after any change
Applied to files:
Nexus/nexus_partial.wflNexus/test_simple_call.wflNexus/test_concat.wflNexus/test_zero_concat.wflNexus/test_skip_loop.wflNexus/test_inline_even.wflNexus/test_nested_loops.wfl
📚 Learning: 2025-08-11T05:10:43.202Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 137
File: TestPrograms/test.wfl:1-2
Timestamp: 2025-08-11T05:10:43.202Z
Learning: Some test scripts in TestPrograms/ are deliberately designed to trigger errors as negative test cases to validate error handling and detection mechanisms. The file TestPrograms/test.wfl specifically tests variable redefinition error detection by attempting to use `store` twice on the same variable.
Applied to files:
Nexus/test_simple_call.wflNexus/test_zero_concat.wflNexus/test_inline_even.wflNexus/test_nested_loops.wfl
📚 Learning: 2025-08-12T09:39:16.504Z
Learnt from: logbie
Repo: WebFirstLanguage/wfl PR: 148
File: .claude/settings.local.json:41-44
Timestamp: 2025-08-12T09:39:16.504Z
Learning: The .claude/settings.local.json file is specifically formatted for Claude AI assistant usage and should not be reformatted to standard bash command syntax. The existing formatting is intentional and optimized for Claude's permission system.
Applied to files:
.claude/settings.local.json
🪛 markdownlint-cli2 (0.18.1)
parserbug.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Integration Tests (ubuntu-latest)
- GitHub Check: Build, Test, Clippy
- GitHub Check: Integration Tests (windows-latest)
- GitHub Check: claude-review
🔇 Additional comments (11)
.claude/settings.local.json (1)
33-34: Addition ofcargo runpermission is appropriate.The new
"Bash(cargo run:*)"permission aligns with existing cargo-related permissions and is appropriate for CI/test workflows. The pattern is consistent with other command patterns in the allow list.Nexus/test_even_check.wfl (1)
3-18: Even-check test script looks correct and idiomaticThe action and calls use the expected WFL arithmetic and
display ... with ...patterns, and should give clear diagnostics for numbers 1–5 without needing extra error handling.src/typechecker/mod.rs (2)
1521-1522: LGTM - Consistent special-case handling for loop variables.The extension of special-case handling to include "count" alongside "loopcounter" is consistent with the existing pattern. Both variables are correctly typed as Number and avoid undefined-variable errors during type inference.
648-651: The hardcoded "count" variable name is correct and follows WFL language specification.WFL documentation explicitly defines that count loops automatically provide a loop variable named
count, which is not user-configurable. The specification states: "The loop variable is always namedcountand is lexically scoped to the loop body. Currently, there is no syntax to rename or alias the loop variable - it's alwayscount." Unlike ForEachLoop where the item name is user-specified in syntax (for each <name> in <collection>), CountLoop has no such syntax parameter—the variable name is fixed by language design.Nexus/test_inline_even.wfl (1)
1-25: LGTM - Clear test validation of inline expressions.The test correctly validates even/odd detection using the inline expression
((num divided by 2) times 2) is equal to num. The test cases cover both even (2) and odd (1, 3) numbers with clear expected outcomes labeled in the display statements.Nexus/test_factorial_inline.wfl (1)
8-8: Test depends on parser bug fix for correct behavior.Line 8 uses the inline expression
n times factorial with n minus 1, which is the exact pattern described in parserbug.md as being misparsed. Without the parser fix, this will be incorrectly parsed as(n * factorial(n)) - 1, causing infinite recursion. This test validates the fix but will fail until the parser bug is addressed.Based on the PR objectives, this appears to be a documentation PR for the bug rather than a fix. Should this test be marked as expected-to-fail or skipped until the parser is fixed?
Nexus/nexus_partial.wfl (1)
1-44: LGTM - Pragmatic workaround test while parser bug exists.This test script provides valuable validation of loop constructs (repeat-until and forever with break) while explicitly skipping the problematic even number check that relies on inline expressions. The explicit SKIP marking (line 13) documents the known limitation until the parser bug is fixed.
parserbug.md (1)
1-339: Excellent bug documentation with comprehensive analysis.This is an exceptionally thorough bug report that includes:
- Clear problem statement and examples
- Detailed root cause analysis with step-by-step trace
- Impact analysis covering affected code patterns
- Multiple proposed fixes with trade-offs
- Test cases to validate the fix
- Practical workarounds for users
The document will serve as valuable reference for implementing the parser fix.
Nexus/test_concat.wfl (1)
1-20: LGTM - Clear concatenation test progression.The test systematically validates string concatenation with numbers through progressively complex examples:
- Simple string
- String with a number variable
- Multiple concatenations in one expression
This provides good coverage of concatenation behavior.
Nexus/test_simple_call.wfl (1)
11-19: Use documented first-class action syntax instead of simplified form.WFL supports first-class actions through its "First-Class Actions and Action Handles" feature, but Tests 2 and 3 use non-standard syntax. The standard pattern is
store handle as get action name from object, thenstore result as perform handle with args. Line 13's direct assignment (store f as sum_two) and line 18's direct call (store result2 as f with 10 and 20) do not follow documented syntax and may not work as intended. Either update to use the properget actionandperformkeywords, or confirm this test is validating error handling for unsupported syntax.Nexus/nexus.wfl (1)
295-303: Explicit parentheses in recursive factorial correctly enforce precedenceSwitching to
give back n times (factorial with (n minus 1))with the added comment makes the intended grouping unambiguous and avoids the precedence misparse forfactorial with n minus 1. This is a clear, low-risk workaround that keeps the integration test stable while the parser bug exists.
| @@ -0,0 +1 @@ | |||
| FileOneContent No newline at end of file | |||
There was a problem hiding this comment.
Remove temporary placeholder file.
temp1.txt appears to be a temporary development artifact with no clear purpose in the codebase. This file should not be committed to the main branch.
Remove this file from the PR or add it to .gitignore if it's required during development.
🤖 Prompt for AI Agents
In temp1.txt around lines 1 to 1, this file is a temporary placeholder and
should not be committed; either remove temp1.txt from the PR (delete the file
and stage the deletion) or, if it must exist for local development, revert its
addition in this branch and add an appropriate rule to .gitignore to prevent
future commits (ensure .gitignore entry is committed).
| @@ -0,0 +1 @@ | |||
| FileTwoContent No newline at end of file | |||
There was a problem hiding this comment.
Remove temporary placeholder file.
temp2.txt appears to be a temporary development artifact with no clear purpose in the codebase. This file should not be committed to the main branch.
Remove this file from the PR or add it to .gitignore if it's required during development.
🤖 Prompt for AI Agents
In temp2.txt around lines 1 to 1, this file is a temporary placeholder with no
purpose and should not be committed; remove temp2.txt from the PR (delete the
file and commit the deletion) or, if it is required during development, revert
its deletion but add an appropriate entry to .gitignore and commit that change
so the file is not tracked in the main branch.
Adds a detailed bug report documenting a critical parser issue related to operator precedence in function call arguments.
The current parser incorrectly groups binary operations in arguments. For example, an expression like
factorial with n minus 1is evaluated as(factorial(n)) - 1instead of the expectedfactorial(n - 1). This parsing error leads to incorrect behavior and can cause infinite recursion in recursive functions.This report provides a thorough analysis of the root cause, outlines the impact, and proposes potential solutions to inform an eventual fix.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.