checker: add an error when high overflows low in a for-in range #27854
checker: add an error when high overflows low in a for-in range #27854rilaaax wants to merge 3 commits into
high overflows low in a for-in range #27854Conversation
|
While testing the PR, I found two remaining edge cases:
These appear to be one newly rejected valid case and one overflow case that remains undetected. Could you please take a look? Thanks! |
|
Thanks. The remaining CI failure is only an outdated expectation in |
|
@codex review |
![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: c62ef19a35
ℹ️ 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".
| node.val_type = typ | ||
|
|
||
| if narrow_max := narrow_int_type_max(typ_idx) { | ||
| if high_const := c.eval_comptime_const_expr(node.high, 0) { |
There was a problem hiding this comment.
When a narrow range has erroneous constant arithmetic such as for i in u8(0) .. 1 / 0, c.expr first records the expected division-by-zero error, but this new call then evaluates the same AST again. eval_comptime_const_expr performs integer division directly in vlib/v/checker/comptime.v, so the zero denominator panics the compiler instead of allowing the normal diagnostic to be returned. Skip this evaluation when checking the bound added errors, or make constant evaluation return none for invalid arithmetic.
Useful? React with 👍 / 👎.
| ast.u8_type_idx { i64(max_u8) } | ||
| ast.u16_type_idx { i64(max_u16) } | ||
| ast.u32_type_idx { i64(max_u32) } |
There was a problem hiding this comment.
rune is an accepted range type and is emitted as a 32-bit unsigned value, but this helper omits it even though it handles u32. Consequently, a range such as for r in rune(4294967294) .. 4294967296 retains rune as its loop-variable type, bypasses the new diagnostic, and wraps after only two increments without ever reaching high. Handle ast.rune_type_idx with the same maximum as u32 and audit the other accepted finite-width integer types.
Useful? React with 👍 / 👎.
| if high_const := c.eval_comptime_const_expr(node.high, 0) { | ||
| if high_val := high_const.u64() { | ||
| if high_val > u64(narrow_max) { |
There was a problem hiding this comment.
For an explicitly overflowing cast such as for b in u8(0) .. int(4294967296), the normal 32-bit int runtime value is 0 (the checker currently only warns about that explicit cast), so the loop terminates immediately. However, eval_comptime_const_cast_value represents casts to int as an unconstrained i64, causing this new check to see 4294967296 and turn the previously compilable range into a hard overflow error. The fit check must use the cast expression's actual runtime-width value rather than the evaluator's mathematical value.
Useful? React with 👍 / 👎.
| node.val_type = typ | ||
|
|
||
| if narrow_max := narrow_int_type_max(typ_idx) { | ||
| if high_const := c.eval_comptime_const_expr(node.high, 0) { |
There was a problem hiding this comment.
The selected constant evaluator does not implement ast.PrefixExpr, so a statically known overflowing bound such as for b in u8(254) .. -(-256) returns none here and bypasses the new diagnostic. At runtime the bound is 256; the u8 loop variable reaches 255, wraps, and never terminates. Support unary constant expressions for this check, or use a constant-folding path that covers all compile-time integer expressions.
Useful? React with 👍 / 👎.
| if high_val > u64(narrow_max) { | ||
| c.error('`high` value `${high_val}` does not fit in the range value type `${c.table.type_to_str(typ)}` (max `${narrow_max}`); the loop variable would overflow and the loop would never terminate', | ||
| cond_pos.extend(high_pos)) |
There was a problem hiding this comment.
On the JavaScript backend, vlib/v/gen/js/js.v emits range increments as ${i}.val++; this mutates the numeric field directly without reapplying the narrow integer constructor, so a u8 range to 256 reaches 256 and terminates rather than wrapping. Because this checker error is unconditional, -b js now rejects ranges that executed correctly on that backend. Gate the diagnostic to backends whose generated loop variable actually wraps, or make all backends enforce the same narrow-type increment semantics.
Useful? React with 👍 / 👎.
| c.error('`high` value `${high_val}` does not fit in the range value type `${c.table.type_to_str(typ)}` (max `${narrow_max}`); the loop variable would overflow and the loop would never terminate', | ||
| cond_pos.extend(high_pos)) |
There was a problem hiding this comment.
This hard error changes which range loops are accepted and adds a user-visible diagnostic, but the commit only updates checker code and tests; the Range for documentation in doc/docs.md still describes exclusivity without explaining that an int/literal upper bound must fit the lower bound's narrow type. Add the new restriction and widening workaround to the relevant language documentation as required for public diagnostic and behavior changes.
AGENTS.md reference: AGENTS.md:L215-L225
Useful? React with 👍 / 👎.
2 participants
Fixes #27728
Problem
A
for i in low .. highloop wherelowhas an explicit narrow integer type (e.g.u8) andhighis a compile-time constant that exceeds that type's range (e.g.for b in min_u8 .. int(max_u8) + 1) compiles and runs, but results in an infinite loop: the loop variable's type silently defaults to the narrow type oflow, so it wraps around (255 -> 0 foru8) instead of ever reachinghigh, and the loop never terminates.Cause
In
for_in_stmt(vlib/v/checker/for.v), wheneverhigh's type isint/int_literal, the checker unconditionally sets the loop variable's type (node.val_type) to the type oflow, without checking whetherhigh's value actually fits in that (potentially narrower) type. The C code generator then emits a loop likefor (u8 b = 0; b < 256; ++b), wherebcan never reach256because it overflows and wraps back to0first.Fix
Added a check in
for_in_stmt: whenlow's type is a fixed-width integer type narrower thanint/i64/u64(i8,i16,i32,u8,u16,u32), andhighcan be evaluated as a compile-time constant (via the existingeval_comptime_const_expr, which already resolves builtin constants likemax_u8and simple casts/arithmetic), the checker now verifies that this constant value fits within the maximum representable value oflow's type. If it doesn't, a compile-time error is raised instead of silently generating an infinite loop. Ranges whose bounds cannot be evaluated statically (e.g. built from variables) are left unaffected, since this cannot be reliably checked at compile time.Test
Regression test added.