v3: print the operand values of failed asserts again - #28920
medvednikov wants to merge 5 commits into
Conversation
A failed `assert foo() == 'www'` outside of a test only printed
`V panic: Assertion failed...` and the source line. The C backend printed
operand values only for integer and float comparisons, reading the
children of the lowered condition. Those are not the source operands for
most other comparisons: string `==` becomes `string__eq(a, b)`, `>` swaps
the arguments of `string__lt`, struct `==` becomes a field by field `&&`
chain, array `==` gets a third argument. Test files printed the values of
such nodes, so they reported `false` for struct operands and evaluated
operands like `a.pop()` a second time.
The transformer now records the lowered form of both source operands while
it lowers an assert comparison, and uses the temp instead when the lowering
stored an operand in one. The lowering of the condition is unchanged. The
source spans of the operands are appended to the assert marker, for their
labels. The C backend reads operands without side effects again, and
stores operands with side effects in temps that the condition reads. An
operand is only reported when one of these works, so it is never evaluated
twice.
Outside of tests, a failure is printed in the format of V 0.5
(`builtin.__print_assert_failure`), which doc/docs.md also shows:
code.v:6: FAIL: fn main.main: assert foo() == 'www'
left value: foo() = zzz
right value: 'www' = www
V panic: Assertion failed...
`@[assert_continues]` functions no longer print the `V panic` line. In a
test build, asserts in helper functions use the test runner format too,
asserts without values print their message, and `sizeof()`/`__offsetof()`
types are qualified with their module again. Values whose `str` code does
not compile yet (C structs, functions, options inside other values) are
printed as `<value>`.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d1643d295e
ℹ️ 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".
The temps for assert operands with side effects are evaluated ahead of the condition. When only the right operand needed one, the left one was read afterwards, in the condition. So in `assert c.n == bump(mut c)`, `bump` ran first, and the assert passed as `1 == 1`. Capture the left operand too, when the right one is captured, unless it is a constant. When the left operand can not be copied into a temp, leave the right one in the condition, and report no values. Do the same when the condition does not read the temp of the left operand.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e1ea3546f
ℹ️ 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".
A failed assert reads the operands without side effects again, to report
them. Two kinds of operands looked like that, but call functions:
- struct literals, whose unset fields get their defaults from cgen,
including the fields of nested, generic and embedded structs, which are
not among the children of the literal, like `Outer{}` for an `Item`
field with `id int = next_id()`;
- index expressions on a type with a `[]` method.
Reading them again ran those calls a second time, and could report a
value that differs from the compared one. A struct literal can now be
read again only when the defaults of the fields it does not set have no
side effects, and an overloaded index never.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f8c4b0335
ℹ️ 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".
A failed assert reads the operands without side effects again, to report
them. When the comparison calls a user defined `==` or `<` method, also
for the fields, elements or variants of the operands, that method can
change what an operand reads, like a global `current` in
`assert current == Item{}`, whose `Item.==` changes `current`. The report
then showed the changed value, not the compared one.
Copy the operands of such comparisons into temps before the comparison
too, unless they are constants. When the lowered comparison does not
read those temps, like for arrays, whose elements are compared in a loop
ahead of the assert, report no values.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ea18d41d3
ℹ️ 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".
…lace The temps that keep the operands of an assert whose comparison calls a user defined operator method are plain C copies. For arrays, maps, pointers, channels, sum types and interfaces, and structs, fixed arrays and options that contain them, such a copy still shares storage with the operand, which the method can change in place, like a `Bag.==` method that increments `bag.items[0]`. The report then showed the changed value, not the compared one. Report no values for such comparisons.
This branch has not been deployed
1 participant
Summary
A failed
assert foo() == 'www'outside of a test only printedV panic: Assertion failed...and the source line. The C backend printed operand values only for integer and float comparisons, and it read them from the children of the lowered condition. For most other comparisons those children are not the source operands:==becomesstring__eq(a, b), and>/<=swap the arguments ofstring__lt;==becomes a field by field&&chain;==becomesarray_eq_raw(a, b, sizeof(T)).The test-file path used the same nodes. It printed
false/falsefor struct operands, nothing for arrays, swapped values for string>, and evaluated operands likea.pop()a second time to print them.Fix:
transform_assert_stmt): while it lowers an assert comparison, the transformer records the lowered form of both source operands (hooks intransform_exprandtransform_expr_for_type, active only for that comparison). If the lowering stored an operand in a temp (like__eq_rhs_0), the temp is used. The lowering of the condition itself is unchanged. The source spans of the operands are appended to the assert marker, for the labels..assert_stmt): operands with side effects are stored in temps before the condition, and the condition reads the temps. Operands without side effects are read again. An operand is only reported when one of these works, so the failure path never evaluates an operand twice. The temps keep the source order: when the right operand gets a temp, the left one gets one too (unless it is a constant), so inassert c.n == bump(mut c)the value ofc.nis read beforebumpruns. If the left operand can not be copied into a temp, the right one stays in the condition, and no values are printed. Struct literals count as having side effects when a field they do not set has a default with side effects. That includes the fields of nested, generic and embedded structs, which cgen fills in. Index expressions that call a[]method count as having side effects too. So neither is evaluated again for the report. Outside of tests, the failure is printed in the V 0.5 format ofbuiltin.__print_assert_failure, whichdoc/docs.mdalso shows for@[assert_continues].Before:
After:
The
code.v:6:NaN:in the issue is how the playground showscode.v:6: FAIL:; V 0.5 printedFAIL:there. Like V 0.5, an operand whose source text already is its value is printed once (right value: [1, 2]), and a message is printed asmessage: ....Other changes in the same code:
@[assert_continues]functions no longer printV panic: Assertion failed..., since they do not panic.V panicbefore the failure goes to the test runner.sizeof()/__offsetof()types in the printed expression are qualified again, whichvlib/v/tests/failing_tests_test.vexpects (it fails on master).strcode does not compile yet are printed as<value>: C structs (their V declaration need not match the C layout, like anonymous members), functions, and options with a non-scalar payload inside other values.none, and operands that are valueif/matchexpressions, still print no values.Fixes #28901
Validation
Issue repro prints the output above. Checked by hand, in programs and in test files: strings, string
</>, structs with and withoutstr(), arrays, nested arrays, fixed arrays, maps, sum types, enums, floats,u64, options and results with?/or, channel receive, interface method calls,sharedvalues,mutparameters and receivers,for mutvalues, generics, comptime$forfields, and asserts indefer,or,match,unsafe,$ifand loops. Operands with side effects are evaluated once. Also compiled with-cstrict -cc clang,-cc tcc,-autofree,-gc noneand-prod.New tests pass; they fail on master:
vlib/v/slow_tests/inout/v3_assert_failure_values.v3.v: the program format, with a call operand that must run once, structs, a customstr(), arrays, amutparameter, an enum, a float and messages.vlib/v/compiler_tests/assert_failure_values_test.v: the issue repro, and the test runner format (call operand run once, struct and array values, string>, message without values, assert in a helper), and the evaluation order of a plain left operand and a right operand that changes it (b.n == bump(mut b),b.s == grow(mut b)). On master, theintcase fails correctly but thestringcase passes. Also operands with calls that are not among their children:Outer{}.inner.id,Gen[int]{}.idandEmbed{}.idwith a field defaultnext_id(), andg[0]with a[]method. Without this they were evaluated again for the report.Re-run after the review fixes (
cgen: read the left assert operand before capturing the right one,cgen: do not read assert operands with hidden calls again), with the results below:./v -silent vlib/v/compiler_errors_test.v: 1720 passed, 5 skipped../v -silent vlib/v/slow_tests/inout/compiler_test.v: 88 ok, 9 expected panics, 1 skipped, 0 errors../v -silent test vlib/v/gen/c/ vlib/v/transform/plusassert_failure_values_test.vandtest_file_harness_codegen_test.v: 44 passed, 1 failed.test_file_harness_codegen_test.vstill fails on two unrelated checks (lines 401 and 617:Assertion failedforassert false, and the top-level statement error), which fail on master too. Master also fails its line 150, which this PR fixes../v -silent test vlib/v/tests/: 2277 passed, 14 failed, 7 skipped. The same 14 files fail on master:builtin_overflow_test,check_in_is_consistency_test,enum_bitfield_test,enum_from_generic_static_method_test,fn_call_mut_ref_args_test,fn_with_opt_or_res_of_multi_return_test,generic_muls_test,for_in_containers_of_fixed_array_test,for_in_ref_arr_test,option_generic_array_test,struct_aligned_test,struct_heap_large_fixed_array_test,vls/goto_def_test,vls/autocomplete_module_test.vlib/v/tests/failing_tests_test.vnow passes.vlib/v/compiler_tests/test_file_cli_run_test.vfails the same way as on master (it expectsAssertion failedforassert falsein a test).Other assert tests pass: the 15
vlib/v/tests/**/*assert*_test.vfiles,sum_smartcast_enum_codegen_test.v,assert_stderr_shadow_codegen_test.v,assert_wide_integer_literal_test.v,cmd/v/assert_compat_test.v,cmd/tools/vdoc/vdoc_run_examples_test.v.Updated expectations:
vlib/v/slow_tests/inout/v3_assert_operand_once.out,v3_assert_percent_label.out,v3_assert_unsigned_value.out: the header is nowfile:line: FAIL: fn main.main: assert ...andV panic: Assertion failed...is the last line, as in V 0.5. The values are unchanged.vlib/v/compiler_tests/test_file_harness_codegen_test.v:test_v3_assertion_operands_run_once_and_stats_count_executed_assertionsexpected the program format (left value: values.pop() = 1) for an assert in a test function, and failed on master since test functions use the test runner format. It now checksLeft value (len: 1): `1`.