WIP Fix #9349#9354
Conversation
…pect Top-level `expect foo(Dynamite) == 5` where the argument is ill-typed turns into a `runtime_error` LIR node; LIR codegen of the surrounding `num_is_eq` then calls `exprLayout` on it and panics with "LIR/codegen invariant violated: exprLayout called on non-value expression runtime_error". The same expression inside `main!` reports the type mismatch cleanly, so the bug is specific to `expect` codegen. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Anton-4
left a comment
Anton-4
left a comment
There was a problem hiding this comment.
Thanks @niclas-ahden :)
This looks good in general, can you add:
- the issue number to the test description, so
test "issue #9349: top-level expect with - Can you alter the test so that it expects the panic for now with the explanation in a comment? That way we can merge this in and make sure the code to run the test stays up to date.
|
@Anton-4 Thanks for the feedback!
I went with skipping the test for now with a TODO, which I think is what you meant, but let me know if you meant a literal expect of a panic. If this is good I'll continue this way for the future tests :) |
I thought about skipping at first but I believe that will lead to zig not building the code after the skip so it could become stale without us knowing. So I do think a literal expect of the panic is recommended here. |
When `lowerLowLevel` lowered an arg to a `runtime_error` LIR expression, that noreturn expression slipped through ANF (it was marked atomic) and landed directly in a low-level op's arg slot. The dev backend's `exprLayout` invariant forbids being called on noreturn exprs, so querying the operand layout for `num_is_eq` panicked. Bubble up `runtime_error` in `lowerLowLevel`: if any lowered arg is a `runtime_error`, replace the entire low-level call with a single `runtime_error` carrying the call's ret layout. Accumulated let-bindings for prior args (and their side effects) are preserved by `acc.finish`. Also handle a top-level expression that returns `.noreturn` in dev `generateCode` so we skip the result store when the trap has already been emitted. Unskip the previously-skipped repro in comptime_eval_test.zig. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Expecting a panic in our case was not straightforward so I just fixed the issue :p |
|
@Anton-4 Yeah, I also got stuck on expecting the panic. I wonder how tests like these should be written going forward? I did verify that skipping the test also skips compiling it, so it can rot as you said. |
2 participants
Add failing test for #9349 as per Zulip thread. This looks good to me, but I'd appreciate any feedback so that I can bring that into the future ones I'll create as well :)