missing compile error for modification of comptime value in runtime branch

[tgschultz]

fn valGetMax(comptime T: type) usize {
    var t = @typeId(T);
    
    var s = switch(t) {
        builtin.TypeId.Float => 32,
        else => @maxValue(T),
    };
    
    return s;
}


test "type" {
    var x = valGetMax(i32);
    x = valGetMax(f64);
}

TODO analyze_min_max_value float

I imagine other builtins will have the same problem.

[tgschultz]

I'm adding this here as I'm pretty sure it's the same problem manifesting in a different way.

In the code below, the problem disappears if If I do the check directly instead of calling isSliceType, however in the original code, where this check is always a few function calls deep, it doesn't seem to matter.

I feel I should also note that in the original code, replacing the check with

if(@typeName(T)[0] == '[' and @typeName(T)[1] == '[')

also produces incorrect results.

const std = @import("std");
const builtin = @import("builtin");
const debug = std.debug;

fn isSliceType(comptime T: type) bool {
    if (@typeId(T) == builtin.TypeId.Pointer) {
        if (@typeInfo(T).Pointer.size == builtin.TypeInfo.Pointer.Size.Slice) {
            return true;
        }
    }
    return false;
}

fn countSliceMembers(inp: var) usize {
    comptime var count = usize(0);
    const type_info = @typeInfo(@typeOf(inp));
    
    inline for (type_info.Struct.fields) |field_info| {
        const FieldType = field_info.field_type;
        if (isSliceType(FieldType)) count += 1;
    }
    return count;
} 

test "incorrectly caching... something" {
    const String = []const u8;
    const Number = u32;
    
    const S = struct {
        string: String,
        number: Number,
    };
    
    debug.assert(isSliceType(String) == true);
    debug.assert(isSliceType(Number) == false);
    
    var s = S{ .string = "", .number = 10, };
    debug.assert(countSliceMembers(s) == 1);
}

Test 1/1 incorrectly caching result of function...assertion failure
Unable to dump stack trace: Unable to open debug info: TodoSupportCoffDebugInfo

[andrewrk]

Note the original code is fixed if you use const instead of var:

fn valGetMax(comptime T: type) usize {
    const t = @typeId(T);
    
    const s = switch(t) {
        builtin.TypeId.Float => 32,
        else => @maxValue(T),
    };
    
    return s;
}


test "type" {
    var x = valGetMax(i32);
    x = valGetMax(f64);
}

[andrewrk]

The first example is not actually a bug - it's working as designed. When you use var, the switch is evaluated at runtime (or at least before the optimizer gets to it). So @maxValue(f64) has to be analyzed in the second call to valGetMax. (Let me see about finishing up that TODO for max value though).

Also I'll have a look at your second example.

[tgschultz]

The second example is probably the same when you drill down into it, due to taking var instead of type in countSliceMembers.

At the time, I probably did this as a convenience so the caller wouldn't need to pass @typeOf(x). While that would be nice, I think it would also probably be anti-zen in that it obfuscates what's happening.

Something to consider from this though is that it is somewhat difficult to reason about without a good working knowledge of how comptime behaves.

[andrewrk]

Something to consider from this though is that it is somewhat difficult to reason about without a good working knowledge of how comptime behaves.

Is it though? If @maxValue(f64) was working correctly, this code would have worked fine. When you compiled in ReleaseFast mode, LLVM definitely would have optimized out the dead code. So it would be unclear whether code has happening at compile-time or runtime. And that unclarity is exactly where the optimizer lives - for all your code (and in most code of most programming languages). But the moment that your semantics depend on compile-time values, you'll get compile errors to ensure things are happening at compile-time.

[tgschultz]

The unclarity in this case came from not understanding comptime well enough to see how the implementation was failing. I was aware that @maxValue didn't work on floats, but I couldn't interpret why that code would even be encountered. In my mind, the fact that T is a comptime argument meant that the compiler should generate different values for @typeId(T), and since the switch was applied to @typeId(T) it would be evaluated at comptime.

Now that I have written a lot more comptime code I think I can see the problem when you point it out, but a month or two ago I don't know if I would have. Maybe I have had an especially bad mental model for comptime and my experience is not indicative of the average programmer, though.

[andrewrk]

For the second test case -

count is a comptime var. This means that every store to count must be a comptime known value, and every load from count must be to a comptime value. However, the expression isSliceType(FieldType) is a runtime known value, and so when we do if (isSliceType(FieldType)) count += 1, there's a load and store with a comptime value inside a runtime branch.

This is unsound, so there should be a compile error for it, but there is not. In other words we need to do count += 1 at comptime, but we're actually doing it at runtime. That should be impossible.

It actually fixes the test case if you remove comptime from var count = usize(0).

So this is a great test case for a new compile error that zig needs to have.

[andrewrk]

I thought about this some more and I think this is also working as designed. The count += 1 instructs the compiler to do a compile time variable modification, which Zig is doing. This can be useful in other places, even in runtime branches. For example, auto hash:

zig/std/hash_map.zig

Lines 445 to 502 in bc88ef2

	pub fn autoHash(key: var, comptime rng: *std.rand.Random, comptime HashInt: type) HashInt {
	switch (@typeInfo(@typeOf(key))) {
	builtin.TypeId.NoReturn,
	builtin.TypeId.Opaque,
	builtin.TypeId.Undefined,
	builtin.TypeId.ArgTuple,
	=> @compileError("cannot hash this type"),
	builtin.TypeId.Void,
	builtin.TypeId.Null,
	=> return 0,
	builtin.TypeId.Int => |info| {
	const unsigned_x = @bitCast(@IntType(false, info.bits), key);
	if (info.bits <= HashInt.bit_count) {
	return HashInt(unsigned_x) ^ comptime rng.scalar(HashInt);
	} else {
	return @truncate(HashInt, unsigned_x ^ comptime rng.scalar(@typeOf(unsigned_x)));
	}
	},
	builtin.TypeId.Float => |info| {
	return autoHash(@bitCast(@IntType(false, info.bits), key), rng);
	},
	builtin.TypeId.Bool => return autoHash(@boolToInt(key), rng),
	builtin.TypeId.Enum => return autoHash(@enumToInt(key), rng),
	builtin.TypeId.ErrorSet => return autoHash(@errorToInt(key), rng),
	builtin.TypeId.Promise, builtin.TypeId.Fn => return autoHash(@ptrToInt(key), rng),
	builtin.TypeId.Namespace,
	builtin.TypeId.Block,
	builtin.TypeId.BoundFn,
	builtin.TypeId.ComptimeFloat,
	builtin.TypeId.ComptimeInt,
	builtin.TypeId.Type,
	=> return 0,
	builtin.TypeId.Pointer => |info| switch (info.size) {
	builtin.TypeInfo.Pointer.Size.One => @compileError("TODO auto hash for single item pointers"),
	builtin.TypeInfo.Pointer.Size.Many => @compileError("TODO auto hash for many item pointers"),
	builtin.TypeInfo.Pointer.Size.Slice => {
	const interval = std.math.max(1, key.len / 256);
	var i: usize = 0;
	var h = comptime rng.scalar(HashInt);
	while (i < key.len) : (i += interval) {
	h ^= autoHash(key[i], rng, HashInt);
	}
	return h;
	},
	},
	builtin.TypeId.Optional => @compileError("TODO auto hash for optionals"),
	builtin.TypeId.Array => @compileError("TODO auto hash for arrays"),
	builtin.TypeId.Struct => @compileError("TODO auto hash for structs"),
	builtin.TypeId.Union => @compileError("TODO auto hash for unions"),
	builtin.TypeId.ErrorUnion => @compileError("TODO auto hash for unions"),
	}
	}

[tgschultz]

The results still don't make sense to me. If you change S to

const S = struct {
    number: Number,
    num2: Number,
};

It no longer has any slice members. isSliceType(FieldType) should resolve to false no matter how the loop is unrolled or what order the fields are evaluated in. Yet if we warn on the result of countSliceMembers() instead of asserting we will find that it is always equal to the number of members in S.

At the very least I think my statement that this behavior is difficult to reason about is accurate.

[andrewrk]

At the very least I think my statement that this behavior is difficult to reason about is accurate.

I'll definitely agree with you there.

It no longer has any slice members. isSliceType(FieldType) should resolve to false no matter how the loop is unrolled or what order the fields are evaluated in. Yet if we warn on the result of countSliceMembers() instead of asserting we will find that it is always equal to the number of members in S.

This happens because, just like with @compileLog, the compile-time side effects happen when the block is analyzed. When the if statement has a runtime condition, both the then and the else blocks are analyzed for every iteration of the inlined while loop, so count += 1 happens 2 times. However when the if statement has a comptime condition, only one of (then block, else block) is analyzed.