Reject overflowing numeric literals safely - #620
Conversation
📝 WalkthroughWalkthroughNumeric literal lexing now converts integer parse failures and non-finite float values into lexer errors instead of panics. Regression tests cover both standard and position-aware lexer APIs. ChangesNumeric lexing
Estimated code review effort: 3 (Moderate) | ~15 minutes 🚥 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn parse_int_literal(lex: &mut logos::Lexer<Token>) -> Option<i64> { | ||
| lex.slice().parse::<i64>().ok() | ||
| } | ||
|
|
||
| /// WFL numbers must be finite. Rust accepts very large decimal floats as | ||
| /// positive infinity, so reject both parse failures and non-finite results at | ||
| /// the token boundary instead of injecting an infinity value into the runtime. | ||
| fn parse_float_literal(lex: &mut logos::Lexer<Token>) -> Option<f64> { | ||
| lex.slice() | ||
| .parse::<f64>() | ||
| .ok() | ||
| .filter(|value| value.is_finite()) | ||
| } |
There was a problem hiding this comment.
🟡 Behavior change ships without the required documentation or Dev Diary entry
A user-facing behavior change is introduced (parse_int_literal/parse_float_literal at src/lexer/token.rs:484-496) that now rejects out-of-range integer and non-finite float literals, but the change ships with no documentation or Dev Diary entry, so the repository's mandatory docs-with-feature rule is not met.
Impact: Contributors and users have no record that oversized numeric literals are now rejected, leaving documentation out of sync with actual runtime behavior.
Mandatory documentation policy in CLAUDE.md/AGENTS.md
CLAUDE.md states under "Documentation Development" that every change altering user-facing behavior MUST update or add corresponding documentation and add a Dev Diary entry in the same change. The PR's own description confirms a compatibility/behavior change ("out-of-range integers and non-finite float literals are now rejected instead of crashing or entering the runtime"), yet git diff for this PR touches only src/lexer/tests.rs and src/lexer/token.rs — no Docs/ update and no Dev diary/ entry.
Prompt for agents
This PR changes user-facing behavior (out-of-range integer literals and non-finite float literals are now rejected as lexer errors instead of panicking or producing infinity). Per CLAUDE.md and AGENTS.md the repository requires documentation to ship in the same change, plus a Dev Diary entry for non-trivial work. Add a short note to the relevant Docs section describing that numeric literals must fit in i64 and floats must be finite, and add a Dev Diary entry under the 'Dev diary/' folder describing this reliability/security fix and its compatibility impact.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
Hardens the WFL lexer against attacker-controlled numeric literals by replacing panic-prone parsing with fallible parsing, ensuring oversized integers and non-finite floats become normal lexing errors rather than unwinding CLI/LSP execution paths.
Changes:
- Replace
parse::<i64/f64>().unwrap()lexer callbacks with fallible helpers that returnOption, letting Logos surface failures as lex errors. - Reject integers outside
i64and reject float literals that parse to non-finite values (±inf,NaN) at the token boundary. - Add regression tests covering oversized integer literals (including unwind safety for public lexing entry points) and non-finite float literals.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/lexer/token.rs |
Switch numeric literal callbacks to safe, fallible parsing; reject non-finite float literals. |
src/lexer/tests.rs |
Add regressions ensuring oversized integers and overflowing floats are lex errors and don’t trigger panics in lexer collection paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let source = format!("{}.0", "9".repeat(400)); | ||
| let mut lexer = Token::lexer(&source); | ||
| assert!(matches!(lexer.next(), Some(Err(_)))); | ||
| assert!(lexer.next().is_none()); | ||
| assert!(std::panic::catch_unwind(|| lex_wfl_with_positions(&source)).is_ok()); | ||
| } |
![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: 09056da318
ℹ️ 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".
| /// lexer. Logos treats `None` as a normal lexing error, which the positioned | ||
| /// lexer reports with the literal's span and the parser then rejects. | ||
| fn parse_int_literal(lex: &mut logos::Lexer<Token>) -> Option<i64> { | ||
| lex.slice().parse::<i64>().ok() |
There was a problem hiding this comment.
When this callback returns None, the collection path turns the Logos Err into a stderr message and drops the span instead of giving the parser a fatal token/error. In contexts where the rest of the line is already valid, e.g. store x as 1 9223372036854775808, the checked CLI lexer produces the same token stream as store x as 1 plus EOL, so parsing/analyzing can succeed and execute the truncated program instead of rejecting the malformed literal; the same issue applies to the new non-finite float path. Please preserve a lexer error in the returned token stream/result rather than relying on a dropped Err to make the parser reject it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lexer/tests.rs (1)
187-187: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueInclude the standard lexer path in the panic regression test.
For consistency with the integer test, consider verifying that
lex_wflalso handles the non-finite float without unwinding.♻️ Proposed refactor
+ assert!(std::panic::catch_unwind(|| lex_wfl(&source)).is_ok()); assert!(std::panic::catch_unwind(|| lex_wfl_with_positions(&source)).is_ok());🤖 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/lexer/tests.rs` at line 187, Extend the panic regression test around lex_wfl_with_positions to also invoke the standard lex_wfl path with the same source and assert it completes without unwinding, matching the existing integer test coverage.
🤖 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/lexer/tests.rs`:
- Line 187: Extend the panic regression test around lex_wfl_with_positions to
also invoke the standard lex_wfl path with the same source and assert it
completes without unwinding, matching the existing integer test coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d509d3e-d55a-483d-8017-2bfb06233dbb
📒 Files selected for processing (2)
src/lexer/tests.rssrc/lexer/token.rs
|
Superseded by #632, which preserves this security fix in the consolidated Rust-source hardening PR. The combined head is mergeable and all required CI checks are green. |
Summary
i64representationSecurity impact
Previously, attacker-controlled WFL source containing an oversized integer reached
parse::<i64>().unwrap()and could unwind CLI or LSP processing. Invalid numeric input now becomes a normal lexer error.Validation
git diff --checkcatch_unwindcoverageProduction readiness
Summary by CodeRabbit