Fix WFL syntax constraints in action blocks - #266
Conversation
…ifiers This commit addresses two syntax issues that were discovered: 1. Support "otherwise check if" else-if chain pattern in conditionals - Previously required nested "otherwise: check if" syntax - Now supports flat "otherwise check if" pattern (like else-if in other languages) - Works correctly inside action definitions - Added parse_if_statement_chained() method for recursive else-if parsing 2. Handle identifiers containing keywords (like content_type) - Previously "content_type" would be split into KeywordContent + error - Now keywords followed by underscore are treated as identifier starts - The underscore and following characters are appended to form the full identifier - Both lex_wfl() and lex_wfl_with_positions() updated Tests added: - test_keyword_with_underscore_becomes_identifier - test_multiple_keywords_with_underscores - test_keyword_without_underscore_stays_keyword - test_keyword_in_context_vs_identifier - test_otherwise_check_if_chain - test_otherwise_check_if_in_action - test_otherwise_colon_check_if_still_works - test_deep_else_if_chain
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughLexer: keywords immediately followed by underscores are now accumulated into multi-word Identifiers with correct position spans. Parser: added parse_if_statement_chained to parse else-if chains without consuming outer end-check. New tests cover both lexer and parser behaviors. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/lexer/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2026-01-14T18:06:04.095ZApplied to files:
📚 Learning: 2026-01-14T18:06:04.095ZApplied to files:
📚 Learning: 2026-01-14T18:06:04.095ZApplied to files:
📚 Learning: 2026-01-14T18:06:04.095ZApplied to files:
📚 Learning: 2026-01-14T18:05:40.917ZApplied to files:
🧬 Code graph analysis (1)src/lexer/mod.rs (1)
⏰ 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)
✏️ Tip: You can disable this entire section by setting 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 Code Review for WFL Syntax Constraints FixesTodo List
Review Summary ✅This PR successfully addresses two important syntax issues in the WFL language compiler. The implementation is well-engineered, thoroughly tested, and follows good practices. Key Improvements1. Else-If Chain Support (
|
There was a problem hiding this comment.
Pull request overview
This PR fixes two syntax parsing issues in the WFL language parser: supporting flat "otherwise check if" else-if chains (similar to else-if in other languages) and correctly handling identifiers that contain keywords (like content_type).
Changes:
- Added support for "otherwise check if" pattern as an alternative to nested "otherwise: check if" syntax
- Fixed lexer to treat keywords followed by underscores as identifier starts rather than separate keyword tokens
- Added comprehensive test coverage for both fixes
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/parser/stmt/control_flow.rs | Implements chained else-if parsing with new parse_if_statement_chained() method and updates parse_if_statement() to handle "otherwise check if" pattern |
| src/lexer/mod.rs | Updates both lex_wfl() and lex_wfl_with_positions() to correctly tokenize keyword+underscore combinations as identifiers |
| src/parser/tests.rs | Adds 4 tests validating else-if chain parsing in various contexts |
| src/lexer/tests.rs | Adds 4 tests validating keyword+underscore identifier tokenization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This keyword is followed by underscore - treat as identifier start | ||
| let keyword_str = &input[span.start..span.end]; | ||
| if let Some(ref mut id) = current_id { | ||
| id.push(' '); |
There was a problem hiding this comment.
When a keyword is followed by an underscore and being accumulated into an identifier, the code pushes a space character instead of the underscore. This will create incorrect identifiers. For example, content_type would become content type instead of content_type. The underscore should be appended in the next iteration when the Err(_) branch handles it, but the space here is incorrect.
| id.push(' '); |
| id.push(' '); | ||
| id.push_str(keyword_str); | ||
| current_id_length += 1 + token_length; |
There was a problem hiding this comment.
Same issue as in lex_wfl(): when a keyword is followed by an underscore, a space is pushed instead of preserving the underscore. This will create malformed identifiers like content type instead of content_type.
| id.push(' '); | |
| id.push_str(keyword_str); | |
| current_id_length += 1 + token_length; | |
| // Append the keyword segment directly; underscores are handled | |
| // separately in the Err("_") branch, so we must not insert spaces. | |
| id.push_str(keyword_str); | |
| current_id_length += token_length; |
| let nested_if = self.parse_if_statement_chained()?; | ||
| Some(vec![nested_if]) |
There was a problem hiding this comment.
This code is duplicated from lines 89-90. Consider extracting the recursive else-if chain parsing logic into a helper method to reduce duplication between parse_if_statement() and parse_if_statement_chained().
| if let Some(token) = self.cursor.peek() | ||
| && matches!(token.token, Token::Colon) | ||
| { | ||
| self.bump_sync(); // Consume the colon if present | ||
| } | ||
|
|
||
| // Skip any Eol tokens after the colon | ||
| self.skip_eol(); | ||
|
|
||
| let mut else_stmts = Vec::with_capacity(8); | ||
|
|
||
| while let Some(token) = self.cursor.peek().cloned() { | ||
| if matches!(token.token, Token::KeywordEnd) { | ||
| break; | ||
| } | ||
| if matches!(token.token, Token::Eol) { | ||
| self.bump_sync(); // Skip Eol between statements | ||
| continue; | ||
| } | ||
|
|
||
| match self.parse_statement() { | ||
| Ok(stmt) => else_stmts.push(stmt), | ||
| Err(e) => return Err(e), | ||
| } | ||
| } | ||
|
|
||
| Some(else_stmts) |
There was a problem hiding this comment.
This else block parsing logic (lines 223-249) is duplicated from lines 93-119 in parse_if_statement(). The duplication makes maintenance harder and increases the risk of inconsistencies. Consider extracting this into a shared helper method.
1. test_report_failure_message: Skip test when running as root
- Root bypasses filesystem permission checks, so setting a
directory to read-only (0o444) doesn't prevent writes
- Added check using libc::geteuid() to skip test when running as root
2. test_timeout_happy_path: Increase timeout from 1 to 5 seconds
- 1 second was too tight and caused flakiness under heavy test load
- Added descriptive assertion message for better debugging
|
Claude finished @logbie's task —— View job Code Review for WFL Syntax Constraints FixesTodo List
Review Summary ✅This PR successfully addresses two important syntax issues in the WFL language compiler. The implementation is well-engineered, thoroughly tested, and follows good practices. Key Improvements1. Else-If Chain Support (
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4725376c70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if slice == "_" && current_id.is_some() { | ||
| // Append underscore to current identifier | ||
| if let Some(ref mut id) = current_id { | ||
| id.push('_'); | ||
| } |
There was a problem hiding this comment.
In the new keyword+underscore path you only append _ to current_id, but the next identifier token is still merged using the existing Token::Identifier logic, which always inserts a space. That means inputs like content_type (the case this change targets) become content_ type, so the parsed identifier no longer matches the source name and variable lookups will fail. This happens whenever the lexer emits KeywordContent, then _, then Identifier(type). Consider tracking that the last joiner was _ (or directly concatenating the next identifier without a space) so the combined identifier stays content_type.
Useful? React with 👍 / 👎.
|
Claude finished @logbie's task —— View job Code Review for WFL Syntax Constraints Fixes ✅Todo List
Review Summary ✅This PR successfully addresses two important syntax issues in the WFL language compiler. The implementation is well-engineered, thoroughly tested, and follows good practices. Key Improvements1. Else-If Chain Support (
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let nested_if = self.parse_if_statement_chained()?; | ||
| Some(vec![nested_if]) | ||
| } else { | ||
| // Standard else block with colon |
There was a problem hiding this comment.
This comment is misleading because the colon is optional (checked with if let Some(token) on the next line). Consider updating to 'Standard else block (with optional colon)' to accurately reflect the implementation.
| // Standard else block with colon | |
| // Standard else block (with optional colon) |
| let nested_if = self.parse_if_statement_chained()?; | ||
| Some(vec![nested_if]) | ||
| } else { | ||
| // Standard else block with colon |
There was a problem hiding this comment.
This comment is misleading because the colon is optional (checked with if let Some(token) on the next line). Consider updating to 'Standard else block (with optional colon)' to accurately reflect the implementation.
| // Standard else block with colon | |
| // Standard else block (with optional colon) |
| if let Some(ref mut id) = current_id { | ||
| id.push(' '); | ||
| id.push_str(keyword_str); |
There was a problem hiding this comment.
When a keyword is followed by an underscore, this code appends it to an existing identifier with a space separator. However, this creates incorrect identifiers like 'some_var content_type' instead of treating 'content_type' as a separate identifier. The underscore should start a new identifier, not continue the previous one.
| // This keyword is followed by underscore - treat as identifier start | ||
| let keyword_str = &input[span.start..span.end]; | ||
| if let Some(ref mut id) = current_id { | ||
| id.push(' '); | ||
| id.push_str(keyword_str); | ||
| current_id_length += 1 + token_length; | ||
| current_id_byte_end = span.end; | ||
| } else { | ||
| current_id = Some(keyword_str.to_string()); | ||
| current_id_start_line = token_line; | ||
| current_id_start_column = token_column; | ||
| current_id_length = token_length; | ||
| current_id_byte_start = span.start; | ||
| current_id_byte_end = span.end; | ||
| } |
There was a problem hiding this comment.
When a keyword is followed by an underscore, this code appends it to an existing identifier with a space separator. However, this creates incorrect identifiers like 'some_var content_type' instead of treating 'content_type' as a separate identifier. The underscore should start a new identifier, not continue the previous one.
| // This keyword is followed by underscore - treat as identifier start | |
| let keyword_str = &input[span.start..span.end]; | |
| if let Some(ref mut id) = current_id { | |
| id.push(' '); | |
| id.push_str(keyword_str); | |
| current_id_length += 1 + token_length; | |
| current_id_byte_end = span.end; | |
| } else { | |
| current_id = Some(keyword_str.to_string()); | |
| current_id_start_line = token_line; | |
| current_id_start_column = token_column; | |
| current_id_length = token_length; | |
| current_id_byte_start = span.start; | |
| current_id_byte_end = span.end; | |
| } | |
| // This keyword is followed by underscore - treat it as the start of a new identifier, | |
| // not a continuation of any previous identifier. | |
| let keyword_str = &input[span.start..span.end]; | |
| // Flush any existing multi-word identifier before starting a new one. | |
| if let Some(id) = current_id.take() { | |
| tokens.push(TokenWithPosition::with_span( | |
| Token::Identifier(id), | |
| current_id_start_line, | |
| current_id_start_column, | |
| current_id_length, | |
| current_id_byte_start, | |
| current_id_byte_end, | |
| )); | |
| } | |
| // Start a new identifier with the keyword text. | |
| current_id = Some(keyword_str.to_string()); | |
| current_id_start_line = token_line; | |
| current_id_start_column = token_column; | |
| current_id_length = token_length; | |
| current_id_byte_start = span.start; | |
| current_id_byte_end = span.end; |
|
Fix formatting |
|
Claude finished @logbie's task —— View job Fixing Code Formatting ✅Todo List
Completed: Fixed formatting issue in Changes Made
All formatting is now clean and compliant with the project's rustfmt configuration. --- • Branch: |
- Consolidate eprintln! arguments on same line in lexer error handling Co-authored-by: logbie <logbie@users.noreply.github.com>
|
Claude finished @logbie's task —— View job Code Review for WFL Syntax Constraints Fixes ✅Todo List
Review Summary ✅This PR successfully addresses two important syntax issues in the WFL language compiler. The implementation is well-engineered, thoroughly tested, and follows good practices. Key Improvements1. Else-If Chain Support (
|
…ifiers
This commit addresses two syntax issues that were discovered:
Support "otherwise check if" else-if chain pattern in conditionals
Handle identifiers containing keywords (like content_type)
Tests added:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.