Sema: fix comparison with undefined by wrongnull · Pull Request #17900 · ziglang/zig

wrongnull · 2023-11-06T19:05:41Z

Vexu

This is not the correct fix. The example Andrew gave doesn't show it well but the comparison is supposed to propagate the comptime known undefined and the conditional branch is supposed to complain about it.

test "undefined propagation in equality operators for bool" {
    var foo: bool = undefined;
    _ = foo == undefined; // no error
    // if (foo == undefined) {} // error: use of undefined value here causes undefined behavior
}

wrongnull · 2023-11-07T06:42:19Z

It turns out that the issue I'm trying to close is more complicated than I expected. So far I will convert this pr to draft and will notify you if I can fix this completely.

wrongnull · 2023-11-08T16:11:57Z

This is not the correct fix. The example Andrew gave doesn't show it well but the comparison is supposed to propagate the comptime known undefined and the conditional branch is supposed to complain about it.
test "undefined propagation in equality operators for bool" {
    var foo: bool = undefined;
    _ = foo == undefined; // no error
    // if (foo == undefined) {} // error: use of undefined value here causes undefined behavior
}

@Vexu I'm pretty sure now this fix is correct and ready for review

Vexu · 2023-11-08T16:17:28Z

-            .ty = result_ty.toIntern(),
-            .name = mod.intern_pool.indexToKey(val.toIntern()).error_union.val.err_name,
-        } })));
+        switch (mod.intern_pool.indexToKey(val.toIntern()).error_union.val) {


Can you explain this change?

I got segmentation fault caused by accessing an inactive union field in this test. zig test filename.zig

test "undefined propagation in equality operators for error union" { var foo: anyerror = undefined; if (foo == @as(anyerror!i32, undefined)) {} }

Can you explain this change?

@Vexu
I don't know why, but without this changes I got segmentation fault with this backtrace

thread 960 panic: access of union field 'err_name' while field 'payload' is active Analyzing test.zig: test.zig:test.undefined propagation in equality operators for error union %9 = load(%5) node_offset:3:9 to :3:12 %10 = block_comptime({ %11 = error_union_type(@anyerror_type, @i32_type) node_offset:3:20 to :3:32 %12 = break(%10, %11) }) node_offset:3:20 to :3:32 %13 = as_node(%10, @undef) node_offset:3:34 to :3:43 > %14 = cmp_eq(%9, %13) node_offset:3:9 to :3:44 %15 = as_node(@bool_type, %14) node_offset:3:9 to :3:44 %16 = condbr(%15, { %18 = dbg_block_begin() %19 = block({ %20 = restore_err_ret_index(%19.none) %21 = break(%19, @void_value) }) node_offset:3:46 to :3:46 %22 = dbg_block_end() %23 = break(%17, @void_value) }, { %24 = break(%17, @void_value) }) node_offset:3:5 to :3:5 For full context, use the command zig ast-check -t test.zig in test.zig: test.zig:test.undefined propagation in equality operators for error union > %17 = block({%9..%16}) node_offset:3:5 to :3:5 in test.zig: test.zig:test.undefined propagation in equality operators for error union > %2 = block({%3..%28}) node_offset:1:68 to :1:68 /home/wrongnull/projects/zig/src/Sema.zig:8916:77: 0x18fa3d0 in analyzeErrUnionCode (zig) .name = mod.intern_pool.indexToKey(val.toIntern()).error_union.val.err_name, ^ /home/wrongnull/projects/zig/src/Sema.zig:16446:56: 0x18f5255 in analyzeCmp (zig) const casted_rhs = try sema.analyzeErrUnionCode(block, rhs_src, rhs); ^ /home/wrongnull/projects/zig/src/Sema.zig:16346:27: 0x134c7aa in zirCmpEq (zig) return sema.analyzeCmp(block, src, lhs, rhs, op, lhs_src, rhs_src, true); ^ /home/wrongnull/projects/zig/src/Sema.zig:1033:63: 0xfee0cf in analyzeBodyInner (zig) .cmp_eq => try sema.zirCmpEq(block, inst, .eq, Air.Inst.Tag.fromCmpOp(.eq, block.float_mode == .Optimized)), ^ /home/wrongnull/projects/zig/src/Sema.zig:5857:34: 0x19a2b9b in resolveBlockBody (zig) if (sema.analyzeBodyInner(child_block, body)) |_| { ^ /home/wrongnull/projects/zig/src/Sema.zig:5840:33: 0x140fed8 in zirBlock (zig) return sema.resolveBlockBody(parent_block, src, &child_block, body, inst, &label.merges); ^ /home/wrongnull/projects/zig/src/Sema.zig:1577:49: 0xffe57a in analyzeBodyInner (zig) break :blk try sema.zirBlock(block, inst, tags[@intFromEnum(inst)] == .block_comptime); ^ /home/wrongnull/projects/zig/src/Sema.zig:5857:34: 0x19a2b9b in resolveBlockBody (zig) if (sema.analyzeBodyInner(child_block, body)) |_| { ^ /home/wrongnull/projects/zig/src/Sema.zig:5840:33: 0x140fed8 in zirBlock (zig) return sema.resolveBlockBody(parent_block, src, &child_block, body, inst, &label.merges); ^ /home/wrongnull/projects/zig/src/Sema.zig:1577:49: 0xffe57a in analyzeBodyInner (zig) break :blk try sema.zirBlock(block, inst, tags[@intFromEnum(inst)] == .block_comptime); ^ /home/wrongnull/projects/zig/src/Sema.zig:916:30: 0x12dcb68 in analyzeBody (zig) _ = sema.analyzeBodyInner(block, body) catch |err| switch (err) { ^ /home/wrongnull/projects/zig/src/Module.zig:4707:21: 0xfcb606 in analyzeFnBody (zig) sema.analyzeBody(&inner_block, fn_info.body) catch |err| switch (err) { ^ /home/wrongnull/projects/zig/src/Module.zig:3357:40: 0xd4a70d in ensureFuncBodyAnalyzed (zig) var air = mod.analyzeFnBody(func_index, sema_arena) catch |err| switch (err) { ^ /home/wrongnull/projects/zig/src/Compilation.zig:3531:42: 0xd488fd in processOneJob (zig) module.ensureFuncBodyAnalyzed(func) catch |err| switch (err) { ^ /home/wrongnull/projects/zig/src/Compilation.zig:3468:30: 0xb149ee in performAllTheWork (zig) try processOneJob(comp, work_item, main_progress_node); ^ /home/wrongnull/projects/zig/src/Compilation.zig:2243:31: 0xb1006c in update (zig) try comp.performAllTheWork(main_progress_node); ^ /home/wrongnull/projects/zig/src/main.zig:4279:24: 0xb3f5f2 in updateModule (zig) try comp.update(main_progress_node); ^ /home/wrongnull/projects/zig/src/main.zig:3680:17: 0xb621bc in buildOutputType (zig) updateModule(comp) catch |err| switch (err) { ^ /home/wrongnull/projects/zig/src/main.zig:285:31: 0x975557 in mainArgs (zig) return buildOutputType(gpa, arena, args, .zig_test); ^ /home/wrongnull/projects/zig/src/main.zig:223:20: 0x9725b5 in main (zig) return mainArgs(gpa, arena, args); ^ /home/wrongnull/projects/zig/lib/std/start.zig:585:37: 0x971fce in main (zig) const result = root.main() catch |err| { ^ ../sysdeps/nptl/libc_start_call_main.h:58:16: 0x7f9b1c9790cf in __libc_start_call_main (../sysdeps/x86/libc-start.c) ../csu/libc-start.c:360:3: 0x7f9b1c979188 in __libc_start_main_impl (../sysdeps/x86/libc-start.c) ???:?:?: 0x971bd4 in ??? (???) ???:?:?: 0x0 in ??? (???) Aborted

when I run zig test test.zig
test.zig:

test "undefined propagation in equality operators for error union" { var foo: anyerror = undefined; if (foo == @as(anyerror!i32, undefined)) {} }

Tbh, I can't yet figure out a proper fix for this segmentation fault other than the current one.

coerceExtra is wrapping undefined in an error union. I think the correct thing to do here to assert that the payload is undefined and then return it.

That's not an assertion.

Maybe I don't understood you well, but for me all the tests from original issue passed

I mean that you should chnage that if into an assertion.

I mean that you should chnage that if into an assertion.

I think now it's correct

andrewrk

In addition to the other review comment, the test cases from the closed issue are missing. You can't close the issue unless you add the test coverage.

Reverting.

andrewrk · 2023-11-12T21:31:51Z

            // For bools, we still check the other operand, because we can lower
            // bool eq/neq more efficiently.


This comment is not addressed in this diff nor in the writeup.

andrewrk · 2023-11-12T21:34:27Z

Reverted in 70d8baa.

wrongnull · 2023-11-13T02:39:59Z

Reverted in 70d8baa.

@andrewrk can I reopen this pr with same changes and test coverage? Also I'm not sure what should I do with comment

wrongnull and others added 2 commits November 6, 2023 22:04

fix comparison with undefined

5053f9e

Merge branch 'master' into fix-comparison-with-undefined

1ccb585

Vexu requested changes Nov 6, 2023

View reviewed changes

wrongnull and others added 4 commits November 6, 2023 23:36

Merge branch 'master' into fix-comparison-with-undefined

da7ef68

propagate 'undefined'

dea4956

remove unnecessary call

d277146

fix typo

a99ef52

wrongnull marked this pull request as draft November 7, 2023 06:42

wrongnull added 2 commits November 8, 2023 19:05

propogate 'undefined' in comparison

b1fc624

Merge branch 'master' into fix-comparison-with-undefined

eadaabe

wrongnull marked this pull request as ready for review November 8, 2023 16:11

Vexu reviewed Nov 8, 2023

View reviewed changes

apply more correct logic

691a41c

Vexu reviewed Nov 9, 2023

View reviewed changes

Comment thread src/Sema.zig

wrongnull added 5 commits November 10, 2023 10:53

apply undefined logic to vectors

1a65b36

propagate 'undefined' in error union

cafe1ff

add fmt

ed0891c

Merge branch 'master' into fix-comparison-with-undefined

d31fb9a

add assertion

942c15f

Vexu approved these changes Nov 11, 2023

View reviewed changes

Vexu merged commit 547481c into ziglang:master Nov 12, 2023

wrongnull deleted the fix-comparison-with-undefined branch November 12, 2023 09:06

andrewrk reviewed Nov 12, 2023

View reviewed changes

wrongnull mentioned this pull request Nov 13, 2023

Sema: Fix comparison with undefined 2 #17990

Closed

		// For bools, we still check the other operand, because we can lower
		// bool eq/neq more efficiently.

Uh oh!

Conversation

wrongnull commented Nov 6, 2023

Uh oh!

Vexu left a comment

Choose a reason for hiding this comment

Uh oh!

wrongnull commented Nov 7, 2023

Uh oh!

wrongnull commented Nov 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wrongnull Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewrk commented Nov 12, 2023

Uh oh!

wrongnull commented Nov 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wrongnull Nov 8, 2023 •

edited

Loading