Fix all remaining ParseError::new deprecation warnings - #195
Conversation
- Replace all ParseError::new() calls with ParseError::from_token() or ParseError::from_span() - Fix needless_borrow clippy warnings via cargo clippy --fix - Add #[allow(clippy::type_complexity)] for complex return type in containers.rs - Capture count_token at start of parse_count_loop for proper error reporting Files modified: - src/parser/expr/binary.rs (1 usage) - src/parser/expr/primary.rs (27 usages) - src/parser/mod.rs (8 usages) - src/parser/mod_complete.rs (13 usages) - src/parser/stmt/actions.rs (17 usages) - src/parser/stmt/collections.rs (needless_borrow fixes) - src/parser/stmt/containers.rs (31 usages + type_complexity) - src/parser/stmt/control_flow.rs (12 usages) - src/parser/stmt/errors.rs (needless_borrow fixes) - src/parser/stmt/io.rs (21 usages) - src/parser/stmt/patterns.rs (53 usages) - src/parser/stmt/processes.rs (needless_borrow fixes) This completes the migration from the deprecated ParseError::new() API to the modern from_token/from_span constructors, which provide proper span information for better error diagnostics. Co-Authored-By: Bradley Byrd <bsbyrd@logbie.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughThe PR refactors parser error handling across 12 files, replacing raw line/column coordinates with token-based error constructors. Error creation now consistently uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Areas requiring extra attention:
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/parser/stmt/actions.rs (1)
437-445: Lost location information in container action definition.The
ActionDefinitionat lines 437-444 hardcodesline: 0, column: 0, losing the location where the action was defined. Consider capturing the position when consuming "action" at line 333:fn parse_container_action_definition(&mut self) -> Result<Statement, ParseError> where Self: StmtParser<'a>, { - self.bump_sync(); // Consume "action" + let action_token = self.bump_sync().unwrap(); // Consume "action" // ... rest of the function ... Ok(Statement::ActionDefinition { name, parameters, body, return_type, - line: 0, - column: 0, + line: action_token.line, + column: action_token.column, })src/parser/mod_complete.rs (1)
231-234: Compilation error:self.cursordoes not exist in this parser.The
Parserstruct in this file usesself.tokens: Peekable<Iter<'a, TokenWithPosition>>, notself.cursor. Line 232 callsself.cursor.error(...)which will fail to compile.} else { - Err(self.cursor.error("Unexpected end of input".to_string())) + Err(ParseError::from_span( + "Unexpected end of input".to_string(), + crate::diagnostics::Span { start: 0, end: 0 }, + 0, + 0, + )) }src/parser/stmt/processes.rs (1)
181-191: Simplify token retrieval for "wait" statement.The current pattern using
map_orwith an inline temporaryTokenWithPositionis unclear and creates a reference to a short-lived default. Since the code expects a "wait" token to be present (it callsbump_sync()immediately after), usebump_sync().unwrap()instead to consume and retrieve the token directly:- let wait_token_pos = self.cursor.peek().map_or( - &TokenWithPosition { - token: Token::KeywordWait, - line: 0, - column: 0, - length: 0, - byte_start: 0, - byte_end: 0, - }, - |v| v, - ); - - self.bump_sync(); // Consume "wait" + let wait_token_pos = self.bump_sync().unwrap(); // Consume "wait"This simplifies the code and eliminates the problematic reference pattern seen elsewhere.
🧹 Nitpick comments (8)
src/parser/stmt/containers.rs (1)
361-384: Redundantextends_tokenclone and convoluted control flow.The
extends_tokenis cloned beforepeek()but theelse ifat line 372 can only be reached whenself.cursor.peek()returnedSome, makingextends_tokenalwaysSomeat that point. The fallback tofrom_spanat lines 378-383 is unreachable.Consider simplifying:
- let extends_token = self.cursor.peek().cloned(); if let Some(token) = self.cursor.peek() { if let Token::Identifier(id) = &token.token { extends = Some(id.clone()); self.bump_sync(); // Consume the identifier } else { return Err(ParseError::from_token( "Expected identifier after 'extends'".to_string(), token, )); } - } else if let Some(ref ext_tok) = extends_token { - return Err(ParseError::from_token( - "Expected identifier after 'extends'".to_string(), - ext_tok, - )); } else { return Err(ParseError::from_span( "Expected identifier after 'extends'".to_string(), crate::diagnostics::Span { start: 0, end: 0 }, 0, 0, )); }src/parser/stmt/errors.rs (1)
322-326: Verify ownership vs reference fortry_token.The
from_tokenmethod signature takes&TokenWithPosition, but line 324 passestry_tokenby value. While Rust's auto-ref will handle this, the inconsistency with other call sites (which pass&tokenortokendirectly on borrowed values) may cause confusion.For consistency with the rest of the codebase where owned tokens are passed:
return Err(ParseError::from_token( "Try statement must have at least one 'when' or 'catch' clause".to_string(), - try_token, + &try_token, ));src/parser/mod_complete.rs (1)
251-258: Inconsistent reference handling.Line 253 passes
token(borrowed frompeek()) directly, while line 257 passes&token_pos. For consistency when the token is borrowed frompeek():return Err(ParseError::from_token( format!("Expected identifier after 'as', found {:?}", token.token), - token, + &token, ));src/parser/expr/primary.rs (1)
89-94: Potential issue: Using cloned token after its original source.Line 90-93 uses
&tokenwheretokenwas cloned frompeek()at line 22. After consuming the left paren at line 76 and parsing an expression,&tokenstill references the originalLeftParentoken, not the current position. This is technically correct but may produce confusing error messages pointing to(when the issue is missing)at end of input.Consider using the last known position:
} else { - return Err(ParseError::from_token( + return Err(ParseError::from_span( "Expected closing parenthesis, found end of input".into(), - &token, + crate::diagnostics::Span { start: 0, end: 0 }, + token.line, + token.column, )); }src/parser/stmt/io.rs (1)
433-433: Inconsistent error construction pattern.Line 433 uses
self.cursor.error()while the rest of this function usesParseError::from_token()for end-of-input errors. For consistency with the PR's goal and to provide better span information, consider usingParseError::from_token()withopen_token.} else { - return Err(self.cursor.error("Unexpected end of input".to_string())); + return Err(ParseError::from_token( + "Unexpected end of input".to_string(), + open_token, + )); }src/parser/stmt/control_flow.rs (3)
268-293: Consider capturingfor_tokenfor better end-of-input error reporting.The
parse_count_loopfunction capturescount_tokenat line 364 for error reporting, butparse_for_each_loopdoes not capture the "for" token. This causes the end-of-input error at line 287-292 to use a placeholder span{start: 0, end: 0}instead of pointing to the "for" keyword.fn parse_for_each_loop(&mut self) -> Result<Statement, ParseError> where Self: StmtParser<'a>, { - self.bump_sync(); // Consume "for" + let for_token = self.bump_sync().unwrap(); // Consume "for" self.expect_token(Token::KeywordEach, "Expected 'each' after 'for'")?; let item_name = if let Some(token) = self.cursor.peek() { if let Token::Identifier(id) = &token.token { self.bump_sync(); id.clone() } else { return Err(ParseError::from_token( format!("Expected identifier after 'each', found {:?}", token.token), token, )); } } else { - return Err(ParseError::from_span( + return Err(ParseError::from_token( "Unexpected end of input after 'each'".to_string(), - crate::diagnostics::Span { start: 0, end: 0 }, - 0, - 0, + for_token, )); };
339-357: ForEachLoop statement uses post-loop token position.The
lineandcolumnfields are derived frompeek()after consuming "end for", which points to the token after the loop rather than the loop's start. If you capturefor_tokenas suggested above, use it here for consistent positioning:- let token_pos = self.cursor.peek().map_or( - &TokenWithPosition { - token: Token::KeywordFor, - line: 0, - column: 0, - length: 0, - byte_start: 0, - byte_end: 0, - }, - |v| v, - ); Ok(Statement::ForEachLoop { item_name, collection, reversed, body, - line: token_pos.line, - column: token_pos.column, + line: for_token.line, + column: for_token.column, })
476-496: CountLoop statement uses post-loop token position instead of count_token.You already capture
count_tokenat line 364. Use it for the statement's line/column instead of peeking at the token after "end count":- let token_pos = self.cursor.peek().map_or( - &TokenWithPosition { - token: Token::KeywordCount, - line: 0, - column: 0, - length: 0, - byte_start: 0, - byte_end: 0, - }, - |v| v, - ); Ok(Statement::CountLoop { start, end, step, downward, variable_name, body, - line: token_pos.line, - column: token_pos.column, + line: count_token.line, + column: count_token.column, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/parser/expr/binary.rs(1 hunks)src/parser/expr/primary.rs(16 hunks)src/parser/mod.rs(7 hunks)src/parser/mod_complete.rs(8 hunks)src/parser/stmt/actions.rs(8 hunks)src/parser/stmt/collections.rs(3 hunks)src/parser/stmt/containers.rs(16 hunks)src/parser/stmt/control_flow.rs(7 hunks)src/parser/stmt/errors.rs(1 hunks)src/parser/stmt/io.rs(12 hunks)src/parser/stmt/patterns.rs(32 hunks)src/parser/stmt/processes.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/parser/stmt/actions.rssrc/parser/stmt/errors.rssrc/parser/stmt/processes.rssrc/parser/mod_complete.rssrc/parser/expr/binary.rssrc/parser/stmt/containers.rssrc/parser/expr/primary.rssrc/parser/stmt/collections.rssrc/parser/stmt/patterns.rssrc/parser/stmt/io.rssrc/parser/stmt/control_flow.rssrc/parser/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/parser/stmt/actions.rssrc/parser/stmt/errors.rssrc/parser/stmt/processes.rssrc/parser/mod_complete.rssrc/parser/expr/binary.rssrc/parser/stmt/containers.rssrc/parser/expr/primary.rssrc/parser/stmt/collections.rssrc/parser/stmt/patterns.rssrc/parser/stmt/io.rssrc/parser/stmt/control_flow.rssrc/parser/mod.rs
src/parser/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Update bytecode when modifying parser in Rust source code
Files:
src/parser/stmt/actions.rssrc/parser/stmt/errors.rssrc/parser/stmt/processes.rssrc/parser/mod_complete.rssrc/parser/expr/binary.rssrc/parser/stmt/containers.rssrc/parser/expr/primary.rssrc/parser/stmt/collections.rssrc/parser/stmt/patterns.rssrc/parser/stmt/io.rssrc/parser/stmt/control_flow.rssrc/parser/mod.rs
🧠 Learnings (2)
📚 Learning: 2025-12-05T10:17:06.457Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.457Z
Learning: Applies to src/**/*.rs : Implement comprehensive error diagnostics using codespan-reporting
Applied to files:
src/parser/stmt/actions.rssrc/parser/mod_complete.rssrc/parser/expr/binary.rssrc/parser/expr/primary.rssrc/parser/stmt/patterns.rssrc/parser/stmt/control_flow.rssrc/parser/mod.rs
📚 Learning: 2025-12-05T10:17:06.457Z
Learnt from: CR
Repo: WebFirstLanguage/wfl PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T10:17:06.457Z
Learning: Applies to src/parser/**/*.rs : Update bytecode when modifying parser in Rust source code
Applied to files:
src/parser/stmt/actions.rssrc/parser/stmt/processes.rssrc/parser/mod_complete.rssrc/parser/expr/binary.rssrc/parser/expr/primary.rssrc/parser/stmt/patterns.rssrc/parser/mod.rs
🧬 Code graph analysis (11)
src/parser/stmt/actions.rs (1)
src/parser/ast.rs (2)
from_token(791-802)from_span(776-788)
src/parser/stmt/processes.rs (1)
src/parser/ast.rs (1)
from_token(791-802)
src/parser/mod_complete.rs (1)
src/parser/ast.rs (2)
from_token(791-802)from_span(776-788)
src/parser/expr/binary.rs (2)
src/parser/cursor.rs (1)
error(440-447)src/parser/ast.rs (1)
from_span(776-788)
src/parser/stmt/containers.rs (1)
src/parser/ast.rs (2)
from_token(791-802)from_span(776-788)
src/parser/expr/primary.rs (1)
src/parser/ast.rs (2)
from_token(791-802)from_span(776-788)
src/parser/stmt/collections.rs (1)
src/parser/ast.rs (1)
from_token(791-802)
src/parser/stmt/patterns.rs (1)
src/parser/ast.rs (2)
from_token(791-802)from_span(776-788)
src/parser/stmt/io.rs (1)
src/parser/ast.rs (1)
from_token(791-802)
src/parser/stmt/control_flow.rs (1)
src/parser/ast.rs (2)
from_token(791-802)from_span(776-788)
src/parser/mod.rs (1)
src/parser/ast.rs (2)
from_token(791-802)from_span(776-788)
🔇 Additional comments (27)
src/parser/stmt/containers.rs (2)
24-38: LGTM - Clippy suppression for complex return type.The
#[allow(clippy::type_complexity)]attribute is appropriate here given the 5-tuple return type. This aligns with the PR objective.
72-84: Consistent token-based error reporting.The migration to
from_tokencorrectly provides span context from the actual token for better diagnostics.src/parser/expr/binary.rs (1)
608-616: Appropriate use offrom_spanfor end-of-input error.Using
from_spanwithcall_lineandcall_columnpreserves location context from thecallkeyword when no token is available. The placeholder span matches prior behavior as noted in the PR description.src/parser/stmt/processes.rs (1)
53-64: Token ownership handling is correct.Both
token_pos(frombump_sync) andvar_token(frombump_sync) are owned, and Rust's auto-ref handles passing them tofrom_token(&TokenWithPosition).src/parser/stmt/collections.rs (2)
31-43: Correct token-based error reporting for list creation.The error paths correctly use
from_tokenwith the relevant token context - either the unexpected token or thecreate_tokenfor end-of-input scenarios.
175-187: Good error context for add operation failures.Using
add_tokenfor both missing "to" keyword and end-of-input provides consistent error location pointing to where theaddstatement began.src/parser/mod_complete.rs (1)
93-97: Correct error construction for orphaned 'end' tokens.Using
from_tokenwithfirst_tokenproperly captures the location of the unexpected 'end' keyword.src/parser/stmt/actions.rs (1)
49-56: Correct pattern for avoiding borrow conflicts.Cloning the token to
err_tokenbefore using it in the error message andfrom_tokencall correctly avoids borrow checker issues when the message formatserr_token.token.src/parser/expr/primary.rs (3)
59-73: Good error handling for list literal parsing.The errors correctly capture context - using the unexpected token for separator errors and
bracket_tokenfor end-of-input to point back to where the list started.
569-576: Correct clone-before-use pattern.The
err_token = next_token.clone()at line 570 correctly avoids borrowing issues when both formatting the message and passing tofrom_token.
1168-1174: Consistent end-of-input handling.Using
from_spanwith placeholder span and zeros for line/column matches the established pattern for end-of-input scenarios where no token context is available.src/parser/stmt/io.rs (3)
248-260: LGTM - Error construction correctly uses token context.The error handling properly uses
ParseError::from_token()with the appropriate token reference (&tokenfor the unexpected token case,open_tokenfor end-of-input). This provides accurate span information for diagnostics.
264-269: New HttpGetStatement variant looks good.The new
Statement::HttpGetStatementis correctly constructed with URL expression and variable name, usingopen_tokenfor line/column info.
704-716: LGTM - Proper token-based error handling in parse_delete_statement.The error construction correctly uses
from_token(next_token)for unexpected tokens andfrom_token(token_pos)for end-of-input, providing accurate diagnostic spans.src/parser/stmt/control_flow.rs (2)
121-143: LGTM - If statement error handling uses correct token context.The error paths correctly use
ParseError::from_token()with the appropriate token references:next_tokenfor unexpected tokens,tokenfor the "end" token when EOF follows, andcheck_tokenfor complete end-of-input scenarios.
364-364: LGTM - count_token captured and used for error reporting.The modification to capture
count_tokenat function start enables proper span information in error messages for end-of-input scenarios, as noted in the PR objectives.Also applies to: 380-398
src/parser/stmt/patterns.rs (5)
59-63: LGTM - Token clone correctly handles borrow scope.The
err_token = token.clone()pattern is necessary here becausetokenis borrowed from the match, and we need to use it in the error return after the match arm. This is the correct approach.
185-200: LGTM - Consistent error handling in parse_extension_filter.The error handling correctly uses
from_tokenwhen a token is available (lines 187-190) andfrom_spanwith placeholder values for end-of-input (lines 194-199).
211-218: Placeholder span for empty pattern definition is acceptable.Using
Span { start: 0, end: 0 }for an empty pattern definition is reasonable since there's no token to reference. This matches the PR's documented behavior for end-of-input errors.
860-866: LGTM - Lookahead/lookbehind brace matching errors use correct token.The unmatched brace errors correctly reference
&tokens[pattern_start - 1]which is the opening brace token, providing accurate span information for the diagnostic.Also applies to: 914-920
629-634: *Safe to approve: accessing tokens[i - 1] in error paths is protected by control flow.The pattern in lines 629–634, 651–656, and 673–678 is safe. At each of these error sites,
*ihas been incremented (lines 615, 637, 659 respectively) before the bounds check, guaranteeing*i > 0when the else branch executes andtokens[*i - 1]is accessed. The logic is sound given the code structure.src/parser/mod.rs (6)
187-190: LGTM - Orphaned 'end' error uses from_token correctly.The error for unexpected 'end' followed by unknown token now properly uses
ParseError::from_token()withfirst_token, providing accurate span information.
283-286: LGTM - Variable name parsing errors use correct token context.The error handling in
parse_variable_name_listcorrectly usesfrom_token(&token)for various error cases (number as variable name, reserved keyword, etc.).Also applies to: 295-301, 310-316
320-325: Placeholder span for end-of-input is consistent with PR approach.Using
from_spanwithSpan { start: 0, end: 0 }for the "end of input" case inparse_variable_name_listis consistent with other end-of-input handling in this PR.
381-393: LGTM - Comprehensive error handling for parse_variable_name_simple.The new if-else structure properly handles both cases: when a token is available (use
from_token) and when at end-of-input (usefrom_spanwith placeholder). This is the correct pattern.
486-497: LGTM - Read statement errors use from_token correctly.The error handling for unexpected 'read' statements properly uses
from_token(token_pos)with the peeked token.
535-536: Consistent use of cursor.error() for top-level end-of-input.The
parse_statementfunction usesself.cursor.error()for the end-of-input case. This is consistent with the existing pattern in this function and provides a reasonable fallback. The cursor likely implements appropriate error construction internally.
Merge pull request #195 from WebFirstLanguage/devin/1765135204-fix-clippy-warnings
1 participant
Fix all ParseError::new deprecation warnings in parser module
Summary
This PR completes the migration from the deprecated
ParseError::new()API to the modernParseError::from_token()andParseError::from_span()constructors across the entire parser module. The new API provides proper span information for better error diagnostics.Changes include:
ParseError::new()calls with the appropriate modern constructorcargo clippy --fix#[allow(clippy::type_complexity)]for a complex return type in containers.rsparse_count_loopto capturecount_tokenat function start for proper error reportingfor_tokenvariable inparse_for_each_loopReview & Testing Checklist for Human
parse_count_loopbehavior: The function now capturescount_token = self.bump_sync().unwrap()instead of justself.bump_sync(). Confirm this doesn't change parsing behavior for count loops.Span { start: 0, end: 0 }as a placeholder. This matches the deprecated API behavior but may warrant future improvement.Recommended test plan: Run the full test suite and verify clippy passes with
-D warnings. The pre-existingtest_complete_completion_workflowfailure in wfl-lsp is unrelated to these changes (confirmed by testing before/after).Notes
Span { start: 156, end: 157 }vsSpan { start: 0, end: 0 })Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.