v3: fix assert truncating a negated wide integer literal to 32 bits - #28051
v3: fix assert truncating a negated wide integer literal to 32 bits#28051hunterjsb wants to merge 3 commits into
Conversation
|
Two things I should disclose after reviewing this more adversarially. 1. A diagnostic change beyond the reported bug. Exempting For a sign-prefixed literal this is lossless ( More generally, a failing assert against any signed literal now prints 2. Scope is narrower than the issue title suggested. Positive wide literals were never affected — they are bare The bug does reach vlib's own suite, though — these all exceed 32 bits and truncate under v3 today:
Verification of no downstream effect: I compiled the three v3 assert fixtures ( |
eee243c to
5889122
Compare
Review result: request changesCI ignored as requested. I reviewed the current PR head, 1. [P2] Separate capture eligibility from diagnostic suppressionFile:
However, the same predicate is now used by becomes: Likewise, The capture and display decisions should use separate predicates. Keep the broad helper in 2. [P2] Make the regression test inspect the generated conditionFile: This check scans the entire generated C file: assert code.contains('== -123456789012345')The generator also embeds the original assertion source in the failure-detail string: v3_eprint_lit("... assert get() == -123456789012345 ...");Therefore, the positive assertion succeeds even when the actual generated condition still compares against a temporary. The preceding negative check only rejects the exact spelling: (int)(-123456789012345)A regression emitting this would pass both checks: int _t2 = -123456789012345;
if (!(_t1 == _t2)) {
// diagnostic string still contains == -123456789012345
}Extract the generated OverallThe capture-side change directly addresses the reported truncation and remains narrowly limited to side-effect-free constant expressions. I would hold approval for the diagnostic regression and the false-positive-prone regression test. This was a static source review; no CI results were considered. |
|
Both addressed in ac6e506 1. Split the predicates. fprintf(..., " right value", "~5", (long long)(~5)); // = -6
fprintf(..., " right value", "-(-1)", (long long)(-(-1))); // = 1
fprintf(..., " right value", "(5)", (long long)((5))); // = 5
v3_eprint_lit(" right value: -123456789012345\n"); // label is the valueConditions still compare inline in all four cases, so nothing regains a truncating temp. 2. The test was false-positive-prone exactly as described. It now extracts the generated condition rather than scanning the file: conditions := lines.filter(it.trim_space().starts_with('if (!(') && it.contains('123456789012345'))
assert conditions.len == 1
assert conditions[0].contains('== -123456789012345')
assert !lines.any(it.trim_space().starts_with('int _t') && it.contains('123456789012345'))Against pristine master it now fails on |
|
Pushed 2f3d857: re-checking adjacent shapes turned up a constant infix operand that was still captured and truncated — |
assert captures each numeric operand into a temp for its failure
diagnostic, exempting literals. A signed literal parses as a prefix over
the literal, so it missed that exemption, resolved to the default `int`,
and was emitted as `int _t = (int)(-123456789012345)`. The comparison
reads that temp, so the assert evaluated wrong rather than merely
reporting wrong:
assert get_plain() == -123456789012345
right value: -123456789012345 = 2045911175
This only surfaced on macOS, which is the sole platform that delegates to
v3 by default; the classic backend emits the literal with an LL suffix
and is unaffected.
Extend the exemption to sign/bit-not prefixes and parens over a numeric
literal, matching the constant folding already done in
enum_field_expr_value. Fixes vlang#28050.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback. The broad predicate was also deciding whether to omit the evaluated value, so `~5` and `-(-1)` lost their `= -6` and `= 1` output. Capture keeps the broad rule, since those expressions are side-effect free; display narrows to a bare or sign-prefixed literal, where the label already spells out the value. Tighten the regression test too: it scanned the whole C file, so the assert source embedded in the failure detail satisfied the positive check even when the condition still compared against a temp. Inspect the generated `if (!(` condition instead, and reject an int temp initialized from the wide literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A constant infix operand like `-9223372036854775807 - 1` was still captured as the default `int`, truncating the comparison the same way the negated literal was (math_test.v:1145 hits this exact shape). The classic backend never captures constant operands at all. Also shortens the predicate comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2f3d857 to
ddc37b2
Compare
Fixes #28050.
assertcaptures numeric operands into temps for its failure diagnostic, exempting literals. A signed literal parses as a.prefixover the literal, so it missed the exemption, resolved to the defaultint, and truncated:The comparison reads the temp, so the assert evaluated wrong, not just reported wrong. Only macOS delegates to v3 by default, which is why it looked platform-specific; driving v3 directly on Linux reproduces it.
Fix: exempt side-effect-free constant operands from capture: literals, sign/
~prefixes, parens, and constant infix —-9223372036854775807 - 1(math_test.v:1145) is an infix, so a literal-only exemption missed it. The classic backend never captures these shapes (assert_subexpression_to_ctempallowlists only side-effecting exprs), so this restores parity: assert evaluates constants exactly like a plainif. Non-constant operands still capture, preserving single evaluation of side effects.Diagnostics: per review, display suppression is a separate, narrower predicate — only a bare or sign-prefixed literal drops its
= value(the label already spells it out).~5,-(-1),(5),2 * 3keep their evaluated output; captured-then-truncated constants previously printed the truncated value and now print the true one.Affected in vlib today:
time_test.v:505,512,518,enum_explicit_size_big_and_small_test.v:78-80,math_test.v:1145.Tests:
vlib/v3/tests/assert_negated_literal_codegen_test.vinspects the emittedif (!(conditions for a negated literal and a constant product; each fails without its part of the fix. It asserts on emitted C rather than running a binary — v3's prelude omits<stdlib.h>and gcc 16 rejects the resulting implicitqsort, which also failsassert_stderr_shadow_codegen_test.von master. The threev3_assert_*inout fixtures emit byte-identical C with and without this patch.