close
The Wayback Machine - https://web.archive.org/web/20230623180925/https://github.com/ziglang/zig/issues/15131
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

possible miscompilation with comptime switch on struct field types #15131

Closed
nurpax opened this issue Mar 31, 2023 · 4 comments
Closed

possible miscompilation with comptime switch on struct field types #15131

nurpax opened this issue Mar 31, 2023 · 4 comments
Labels

Comments

@nurpax
Copy link
Contributor

nurpax commented Mar 31, 2023

Zig Version

0.11.0-dev.1893+277015960

(Windows 10, 64-bit, default zig build options)

Steps to Reproduce and Observed Behavior

I'd expect the following program to never hit the "invalid field type" @compileError in the typeinfo switch. This particular @compileError is conditioned under if (!isIdField(...)) but the if-body runs regardless of what isIdField function returns.

Here's the program:

const std = @import("std");

pub fn Outer(comptime Components: []const type) type {
    return struct {
        fn compTagBits(comptime C: type) u32 {
            inline for (Components, 0..) |c, i| {
                if (c == C) {
                    return 1 << i;
                }
            }
            @compileError("unhandled component type " ++ @typeName(C));
        }

        fn isIdField(comptime field: std.builtin.Type.StructField) bool {
            _ = field;
            // compileError happens regardless of what this function returns
            return true;
        }

        fn compTagBitsStruct(comptime C: type) u32 {
            if (@typeInfo(C) != .Struct) {
                @compileError("Expecting struct type, got " ++ @typeName(C));
            }
            var mask: u32 = 0;
            const fields = std.meta.fields(C);
            inline for (fields) |field| {
                switch (@typeInfo(field.type)) {
                    .Optional => {}, // do nothing for optional fields
                    .Pointer => |info| {
                        mask |= compTagBits(info.child);
                    }, // required component
                    else => {
                        if (!isIdField(field)) {
                            // NOTE why do we hit this @compileError when isIdField is
                            // hardcoded to return true?  Replacing isIdField(field)
                            // with literal "false" doesn't hit this path.
                            @compileError("invalid field type " ++ field.name ++ ":" ++ @typeName(field.type));
                        }
                    },
                }
            }
            return mask;
        }
    };
}

test "weird behavior" {
    const Pos = struct { p: @Vector(2, f32) };
    const Velocity = struct { v: @Vector(2, f32) };
    const Ret = struct {
        id: u32,
        p: *Pos,
        v: *Velocity,
    };
    const O = Outer(&[_]type{ Pos, Velocity });
    const bits = O.compTagBitsStruct(Ret);
    std.debug.print("bits {b}\n", .{bits});
}

Saving this into file repro.zig and running zig test src\repro.zig, I get the below unexpected error message:

src\repro.zig:37:29: error: invalid field type id:u32
                            @compileError("invalid field type " ++ field.name ++ ":" ++ @typeName(field.type));
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
referenced by:
    test.weird behavior: src\repro.zig:56:19
    remaining reference traces hidden; use '-freference-trace' to see all reference traces

Expected Behavior

I'd expect this if-body to be never taken:

                        if (!isIdField(field)) {
                            // NOTE why do we hit this @compileError when isIdField is
                            // hardcoded to return true?  Replacing isIdField(field)
                            // with literal "false" doesn't hit this path.
                            @compileError("invalid field type " ++ field.name ++ ":" ++ @typeName(field.type));
                        }

This is an extract from a bigger program, the contents of isIdField were originally more complicated until I noticed that it doesn't really matter what it returns.

I noticed #15122 and this bug kind of has a similar energy.

@nurpax nurpax added the bug Observed behavior contradicts documented or intended behavior label Mar 31, 2023
@Vexu
Copy link
Member

Vexu commented Mar 31, 2023

The !isIdField(field) check is executed at runtime, you can get your desired behavior by prefixing the expression with comptime:

if (comptime !isIdField(field)) {
    @compileError("invalid field type " ++ field.name ++ ":" ++ @typeName(field.type));
}

@Vexu Vexu added question and removed bug Observed behavior contradicts documented or intended behavior labels Mar 31, 2023
@nurpax
Copy link
Contributor Author

nurpax commented Apr 1, 2023

Thanks for the reply! I confirm that this indeed fixes it.

I guess I don't quite understand what happens if I don't specify comptime: the whole if around the @compileError gets thrown away without warning.

@Vexu
Copy link
Member

Vexu commented Apr 1, 2023

The condition is not known at comptime so the body gets analyzed triggering the compile error.

@nurpax
Copy link
Contributor Author

nurpax commented Apr 1, 2023

OK, makes sense now, thanks! Closing the bug :)

@nurpax nurpax closed this as completed Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants