Skip to content

@typeInfo strings cannot be null terminated. #9182

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

Closed
Vexu opened this issue Jun 20, 2021 · 4 comments · Fixed by #18470
Closed

@typeInfo strings cannot be null terminated. #9182

Vexu opened this issue Jun 20, 2021 · 4 comments · Fixed by #18470
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@Vexu
Copy link
Member

Vexu commented Jun 20, 2021

Taken from #8636.

Looks good so far, do you also want to make the change to @typeInfo and @src?

I have local changes ready to update string fields contained in structures returned by these two builtins to [:0]const u8.

But while applying changes to @typeInfo() I realized its std.builtin.TypeInfo output is also an input to @Type() and that could break existing code. For instance, this assignment fails since it uses a []const u8 as returned by std.mem.tokenize() and that can't implicitly cast to [:0]const u8:

I see three ways to fix this.

  1. Have separate types for @typeInfo and @Type, which would break even more code.
  2. Leave things as they are, which leaves the awkwardness from the issue that the pr was trying to solve.
  3. Add casts from []T -> [:t]T and *[N]T -> *[N:t]T when the value is comptime known.

The last option would likely solve some other awkwardness about dealing with sentinel terminated values but includes a lot of implicitness.

@Vexu Vexu added enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Jun 20, 2021
@Vexu Vexu added this to the 0.9.0 milestone Jun 20, 2021
@jmc-88
Copy link
Contributor

jmc-88 commented Jun 23, 2021

The code that broke with the local changes I had for amending std.builtin.TypeInfo were due to the fact that the failing testcase is using std.mem.tokenize() at comptime. The problem here is that there's no way to make the slice returned by the TokenIterator into a null-terminated slice without allocating dedicated memory for it, which is impossible at comptime.

So, another option: introduce a comptime allocator to unbreak code that follows a similar pattern even if @Type() starts requiring null-terminated slices. This would be unblocked by issue #1291.

But it'd still make its use a bit more awkward, potentially requiring the user to allocate/deallocate memory whereas now no such dance is required.

@ifreund
Copy link
Member

ifreund commented Jun 23, 2021

The problem here is that there's no way to make the slice returned by the TokenIterator into a null-terminated slice without allocating dedicated memory for it, which is impossible at comptime.

Actually, this can be done:

const std = @import("std");
const testing = std.testing;

const text =
    \\f1
    \\f2
    \\f3
;

test "issue 6456" {
    comptime var gen_fields: []const [:0]const u8 = &[_][:0]const u8{};
    comptime {
        var it = std.mem.tokenize(text, "\n");
        while (it.next()) |name| {
            const name_z = name ++ "\x00";
            const name_z_slice: [:0]const u8 = name_z[0..name.len :0];
            gen_fields = gen_fields ++ [_][:0]const u8{name_z_slice};
        }
    }
    try testing.expectEqualStrings("f1", gen_fields[0]);
    try testing.expectEqualStrings("f2", gen_fields[1]);
    try testing.expectEqualStrings("f3", gen_fields[2]);
    try testing.expectEqual(@as(u8, 0), gen_fields[0][gen_fields[0].len]);
    try testing.expectEqual(@as(u8, 0), gen_fields[1][gen_fields[1].len]);
    try testing.expectEqual(@as(u8, 0), gen_fields[2][gen_fields[2].len]);

}

Edit: here's a function to do this that would be suitable for std.mem:

fn addZ(comptime T: type, comptime s: []const T) [:0]T {
    var arr: [s.len + 1]T = undefined;
    std.mem.copy(T, &arr, s);
    arr[s.len] = 0;
    return arr[0..s.len :0];
}

@jmc-88
Copy link
Contributor

jmc-88 commented Jun 23, 2021

Hmm, I suppose you're right. That's definitely more awkward code though, but manageable. Especially if something like addZ() (or, I don't know std.mem.tokenizeZ()) is provided by the stdlib so that users don't have to come up with its implementation each time.

@castholm
Copy link
Contributor

A very easy way to convert a comptime-known []const u8 value to [:0]const u8 is to simply concatenate it with "". Using the example from above:

const std = @import("std");
const testing = std.testing;

const text =
    \\f1
    \\f2
    \\f3
;

test "issue 6456" {
    comptime var gen_fields: []const [:0]const u8 = &[_][:0]const u8{};
    comptime {
        var it = std.mem.tokenizeScalar(u8, text, '\n');
        while (it.next()) |name| {
            gen_fields = gen_fields ++ .{name ++ ""};
        }
    }
    try testing.expectEqualStrings("f1", gen_fields[0]);
    try testing.expectEqualStrings("f2", gen_fields[1]);
    try testing.expectEqualStrings("f3", gen_fields[2]);
    try testing.expectEqual(@as(u8, 0), gen_fields[0][gen_fields[0].len]);
    try testing.expectEqual(@as(u8, 0), gen_fields[1][gen_fields[1].len]);
    try testing.expectEqual(@as(u8, 0), gen_fields[2][gen_fields[2].len]);
}

If we can accept the slight awkwardness of it, it might be an acceptable tradeoff to break code that reifies types from strings tokenized at comptime (suggesting StructField{ .name = name ++ "" } as a fix) in exchange for null-terminated @typeInfo() names.

@andrewrk andrewrk modified the milestones: 0.13.0, 0.12.0 Jan 8, 2024
@andrewrk andrewrk added the accepted This proposal is planned. label Jan 8, 2024
@jacobly0 jacobly0 marked this as a duplicate of #16116 Feb 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants