checker: detect empty ranges with comptime constant bounds#27890
checker: detect empty ranges with comptime constant bounds#27890rilaaax wants to merge 4 commits into
Conversation
|
@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: 8b7b1f37ad
ℹ️ 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".
|
Please remember to add a regression test whenever you submit a valid codex review fix. Thanks! |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5368b548f0
ℹ️ 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".
| else if left_i := left.i64() { | ||
| if right_i := right.i64() { | ||
| match expr.op { | ||
| .plus { return left_i + right_i } |
There was a problem hiding this comment.
For same-typed narrow integers this fallback changes the evaluated value from the expression's narrow result to an unbounded i64; for example u8(255) + u8(1) is cast back to u8 by the generated expression, so for i in u8(255) + u8(1) .. 1 has a low bound of 0 and should execute once, but this evaluator returns 256 and the new range check rejects it as an empty range. Keep the narrow integer cases or cast the folded result back to the promoted expression type before using it for diagnostics.
Useful? React with 👍 / 👎.
| if low_val != none && high_val != none { | ||
| low_i := low_val.i64() | ||
| high_i := high_val.i64() |
There was a problem hiding this comment.
This block still runs after the earlier c.error(...) branches, so invalid ranges with constant non-integer bounds now get an extra empty-range diagnostic: the existing for i in 10 .. 10.5 checker test will have 10.5 evaluated as f64, then .i64() truncates it to 10 and emits empty range: 10 .. 10 in addition to range types do not match. That changes exact output-test expectations in this repo, so skip this check unless both checked bound types are integers.
AGENTS.md reference: AGENTS.md:L713-L713
Useful? React with 👍 / 👎.
| high_val := node.high.val.i64() | ||
| // At this point, it is guaranteed that both node.cond and node.high are integers | ||
| low_val := c.eval_comptime_const_expr(node.cond, 0) | ||
| high_val := c.eval_comptime_const_expr(node.high, 0) |
There was a problem hiding this comment.
When a bound contains a constant shift that c.expr has already rejected, such as for _ in 0 .. 1 << 99 {}, check_shift still returns an integer type, so this new evaluation path computes the whole bound anyway. The constant evaluator performs the shift directly (u64(left_i) << right_i) without validating the count first, which can make the checker execute an out-of-range shift while processing an already-invalid program; skip this range evaluation after bound errors or make the evaluator reject invalid shifts before executing them.
Useful? React with 👍 / 👎.
| if left_f := left.f64() { | ||
| if right_f := right.f64() { | ||
| match expr.op { | ||
| .plus { | ||
| return left_f + right_f |
There was a problem hiding this comment.
This folds every float operation in f64, but expressions whose promoted type is f32 are rounded after each operation in generated code. For example, int(f32(16777216.0) + f32(1.0)) .. 16777217 should start at 16777216 because the f32 addition cannot represent the +1, but the evaluator computes 16777217 in f64 and the new empty-range check rejects a loop that would execute once. Keep f32 operations rounded to f32 before returning the folded value.
Useful? React with 👍 / 👎.
3 participants
Fixes #27889
Problem
The compile-time check that detects empty numeric ranges (e.g.
for i in 4 .. 2) only worked when both bounds were raw integer literals. Ranges whose bounds were comptime constant cast expressions (int(2),u8(2),u16(4)) or operation results (4 - 2,int(4) - int(2)) silently compiled even when the range could never execute.Cause
checker/for.vmatched the bounds againstast.IntegerLiteralonly, so any other AST node type (ast.CastExpr, etc.) bypassed the emptiness check entirely, even though its value was fully known at compile time.Fix
Replaced the
ast.IntegerLiteralpattern match withc.eval_comptime_const_expr(), the same comptime evaluator already used elsewhere in the checker, to resolve both bounds to concrete values before comparing them. The check is restricted to cases where both bounds resolve to actual integer types, so it doesn't misfire alongside the existing "range types do not match" / "range type can only be an integer type" errors (e.g.10 .. 10.5,'a' .. 'b'). A fallback unsigned comparison handles literals that overflowi64.Known remaining limitations (left as TODOs, out of scope for this fix):
0 .. max_u8 + 1)4 .. int(2) + u8(1))Test
Coverage + regression tests added.