zig icon indicating copy to clipboard operation
zig copied to clipboard

change field names in `@typeInfo` to be `[:0]const u8` (null-terminated) instead of `[]const u8`

Open IntegratedQuantum opened this issue 1 year ago • 7 comments

Zig Version

0.11.0-dev.3655+a2f54fce5 worked in 0.11.0-dev.3395+1e7dcaa3a

Steps to Reproduce and Observed Behavior

const std = @import("std");
x: void,
pub fn main() !void {
	@compileLog(@typeInfo(@This()).Struct.fields[0].name[0..]);
}

Output:

$ zig run test.zig
Compile Log Output:
@as(*const [1]u8, "x")
$
$ ~/Downloads/zig-old/zig run test.zig
Compile Log Output:
@as(*const [1]u8, "x\x00")

In my project I was using field names to automatically generate calls to OpenGL's glGetUniformLocation.

Expected Behavior

I expect field names to be null-terminated, given that most other compiler-generated strings(string literals, @embedFile, @errorName) are also null-terminated for C compatibility.

IntegratedQuantum avatar Jun 17 '23 08:06 IntegratedQuantum

This is the current blocker for Bun to upgrade Zig

Unrelated: there's an issue with GenericPoison that hadn't happened in zig of a month ago when passing an anytype param to a function pointer when the type is known at comptime

Jarred-Sumner avatar Jun 17 '23 23:06 Jarred-Sumner

I think I've found the code responsible for this! I'm going to keep at it for a few more hours and if I don't have a fix tonight I'll post my findings.

ghost avatar Jun 18 '23 07:06 ghost

Well I haven't been able to get it to work. I was looking in Sema.zig: zirTypeInfo, under the .Struct section. For a while I tried to change .slice_const_u8_type to .slice_const_u8_sentinel_0_type and add .sentinel = .zero_u8 in the call to mod.arrayType, but this kept crashing on the assert at InternPool.zig:3880, where it says assert(ip.typeOf(elem) == field.ty.toIntern());. I think I found out this was because parts of the compiler use std.builtin.Type.StructField, which says the field name's type is []const u8, and I hadn't updated that definition. But, then I noticed that even in old versions where your code works as expected, std.builtin.Type.StructField still says []const u8.

The last patch I tried was this:

diff --git a/src/Sema.zig b/src/Sema.zig
index 36fe5a6ee..fc8dff605 100644
--- a/src/Sema.zig
+++ b/src/Sema.zig
@@ -17234,9 +17234,9 @@ fn zirTypeInfo(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Ai
                                 // TODO: write something like getCoercedInts to avoid needing to dupe
                                 const bytes = if (tuple.names.len != 0)
                                     // https://github.com/ziglang/zig/issues/15709
-                                    try sema.arena.dupe(u8, ip.stringToSlice(ip.indexToKey(struct_ty.toIntern()).anon_struct_type.names[i]))
+                                    try sema.arena.dupeZ(u8, ip.stringToSlice(ip.indexToKey(struct_ty.toIntern()).anon_struct_type.names[i]))
                                 else
-                                    try std.fmt.allocPrint(sema.arena, "{d}", .{i});
+                                    try std.fmt.allocPrintZ(sema.arena, "{d}", .{i});
                                 const new_decl_ty = try mod.arrayType(.{
                                     .len = bytes.len,
                                     .child = .u8_type,
@@ -17290,7 +17290,7 @@ fn zirTypeInfo(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Ai
                     struct_obj.fields.values(),
                 ) |*field_val, name_nts, field| {
                     // TODO: write something like getCoercedInts to avoid needing to dupe
-                    const name = try sema.arena.dupe(u8, ip.stringToSlice(name_nts));
+                    const name = try sema.arena.dupeZ(u8, ip.stringToSlice(name_nts));
                     const name_val = v: {
                         var anon_decl = try block.startAnonDecl();
                         defer anon_decl.deinit();

Trying to update the values without updating the types. But this seemed to have no effect.

ghost avatar Jun 18 '23 09:06 ghost

I'm passing field names to the win32 GetProcAddress function and plan on doing something similar to load Vulkan symbols. Null-terminated strings are very helpful here.

DaanVandenBosch avatar Jun 20 '23 18:06 DaanVandenBosch

I'm a bit confused here; the type of field names has always been []const u8, so I'm not sure where y'all think this null terminator was coming from.

https://github.com/ziglang/zig/blob/9eb008717b2786e885f4110503dc461d0bf2e682/lib/std/builtin.zig#L316

That said, if we consider this a proposal to change that to be name: [:0]const u8 instead, then I'm in favor of that.

andrewrk avatar Jun 20 '23 18:06 andrewrk

Sounds like a case of Hryum’s Law

On Tue, Jun 20, 2023 at 11:43 AM Andrew Kelley @.***> wrote:

I'm a bit confused here; the type of field names has always been []const u8, so I'm not sure where y'all think this null terminator was coming from.

https://github.com/ziglang/zig/blob/9eb008717b2786e885f4110503dc461d0bf2e682/lib/std/builtin.zig#L316

That said, if we consider this a proposal to change that to be name: [:0]const u8 instead, then I'm in favor of that.

— Reply to this email directly, view it on GitHub https://github.com/ziglang/zig/issues/16072#issuecomment-1599327911, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFNGS6END6J7MX4W2EYQALXMHVONANCNFSM6AAAAAAZKDIGJU . You are receiving this because you commented.Message ID: @.***>

Jarred-Sumner avatar Jun 20 '23 19:06 Jarred-Sumner

There is also #9182

Vexu avatar Jun 20 '23 22:06 Vexu