zig icon indicating copy to clipboard operation
zig copied to clipboard

ZON

Open MasonRemaley opened this issue 1 year ago • 24 comments

ZON

This PR implements ZON, or Zig Object Notation (think JSON, but with Zig syntax instead of Javascript.)

In particular, it implements:

  • Runtime parsing of ZON data
  • Runtime stringification of ZON data
  • Compile time importing of ZON data

A lot of files are added since I added a lot of test cases, the bulk of the work is in src/zon.zig and lib/std/zon. If there's anything I can do to make this easier to review, let me know.

Tests cover all new features.

Runtime

The runtime code can be found at lib/std/zon.

Parsing

lib/std/zon/parse.zig

Detailed doc comments are provided in the source. At a high level, this module provides the following functions and some configuration options:

  • parseFromSlice
  • parseFromAst
  • parseFromAstNoAlloc
  • parseFromAstNode
  • parseFromAstNodeNoAlloc
  • parseFree

Most people will just want parseFromSlice and maybe parseFree, but the more fine grained functions are available for those who need them.

Stringify

lib/std/zon/stringify.zig

Detailed doc comments are provided in the source. At a high level, this module provides the following functions and some configuration options:

  • stringify
  • stringifyMaxDepth
  • stringifyArbitraryDepth
  • stringifier

Most users will just need stringify, or one of its variants.

However, stringifier returns a much more fine grained interface that is used in the implementation of the other functions. It may be used directly if you need to serialize something piece by piece--perhaps to apply different configuration to different parts of the value, or because the value does not actually exist laid out in memory in the same form in which you want to stringify it.

Compile Time

Almost all of the logic for this is in src/zon.zig.

This PR also implements importing ZON at compile time:

const Foo = @import("foo.zon");

Things that may change later...

& In ZON syntax

~~Right now, ZON slices need to be prefixed with & to distinguish them from tuples. This is necessary because we don't know the result type when importing. In the future I think ZON should not distinguish between tuples and slices, it should just coerce to whatever the destination type is.~~

Update: & is not allowed in ZON, ZON will automatically coerce tuples to arrays or slices when given a destination type.

Untagged nonexhaustive enum values not yet supported

Zig does not currently have a way to represent an untagged enum value as a literal, and as such there's no way to represent one in ZON.

We could resolve this by adding a syntax for untagged enum literals to Zig, or by allowing ZON to coerce integers to untagged enum literals (provided the destination type is known.) Personally I prefer the former solution.

  • [ ] After this PR is merged I'll file an issue to track this if still relevant.

Type mismatches do not always point at helpful location

This is an existing issue, but it can make type errors in ZON imports annoying.

Consider this incorrect code:

    const Struct = struct {
        outer: struct { inner: u8 },
    };
    const some_value = .{ .outer = .{ .inner = 1.5 } };
    const casted: Struct = some_value;

It produces the following error:

src/main.zig:22:28: error: fractional component prevents float value '1.5' from coercion to type 'u8'
    const casted: Struct = some_value;

This isn't super helpful. The location information should be pointed to the struct literal, or a note should be added that points to the struct literal.

This comes up all the time with ZON, e.g. any time you do something like this and make a mistake:

    const Struct = struct {
        outer: struct { inner: u8 },
    };
    const casted: Struct = @import("foo.zon");
  • [ ] After this PR is merged I'll file an issue to track this.

Allowing eliding outer braces may make sense

There's case to be made to allow eliding braces on an outer struct in ZON, like this:

.x = 10,
.y = 20,
  • [ ] I'll file an issue with some motivating use cases after this is merged, easy change if we decide to make it

Divergences from Zig

Zig has no NaN or inf literals right now. Zon does, since otherwise it would have no way to represent these values.

IMO we should add the same literals to Zig.

  • [ ] After this PR is merged I'll file an issue to track this.

See Also

  • #15552
  • #17731
  • #14531
  • #14532
  • #14534
  • #5039
  • #14532
  • [x] #20275 (fixed, can probably parse directly as dest types now)
  • #20277
  • #20880
  • #20881

MasonRemaley avatar Jun 12 '24 01:06 MasonRemaley

Regarding & in ZON...

You're correct that ZON definitely shouldn't include this. Our main use of ZON today (build.zig.zon) already creates variable-length structures without this syntax. To avoid a divergence between ZON you can @import and build.zig.zon, I would consider this a merge blocker, but that's Andrew's call of course.

Implementation-wise, the ZIR instruction corresponding to @import should be modified to provide a result type if one is present (you can use .none to represent the case where there isn't one). Then, the ZON analysis logic should traverse into this result type as needed. This has the additional advantage of providing better source locations if the structure of an imported file doesn't match what's expected, since the type error will occur whilst parsing the relevant ZON expression.

Hmm I think I agree, better to knock it out now than to release it as is then immediately change the syntax. I would've done this up front but incorrectly thought it required major changes to the compiler.

I'll take a look at making this change. If no result type is specified, I'll have it default to a tuple rather than a slice unless there are any objections to that default.

MasonRemaley avatar Jun 12 '24 22:06 MasonRemaley

Yup. I agree that it makes sense to use a tuple when there's no result type.

By the way, in case you're unfamiliar with the AstGen code, you'll just want try rl.ri.resultType(gz, node) orelse .none in the corresponding case in AstGen.builtinCall.

mlugg avatar Jun 12 '24 23:06 mlugg

+ echo Looking for non-conforming code formatting...
+ stage3-debug/bin/zig fmt --check .. --exclude ../test/cases/ --exclude ../doc/ --exclude ../build-debug
Looking for non-conforming code formatting...
../test/behavior/zon/a_neg.zon:1:1: error: expected type expression, found '-'
../test/behavior/zon/slice-1.zon:1:1: error: expected type expression, found '&'
../test/behavior/zon/void.zon:1:1: error: expected type expression, found '{'
../test/behavior/zon/slice-empty.zon:1:1: error: expected type expression, found '&'
../test/behavior/zon/false.zon
../test/behavior/zon/slice-abc.zon:1:1: error: expected type expression, found '&'

In case you haven't found why this is happening, it's here:

https://github.com/ziglang/zig/blob/17f14e1d65b59ebb1a8b5b617cc31fb2614f0c6a/ci/x86_64-linux-release.sh#L60-L66

Edit: I'll address this in a PR shortly. Sit tight for a bit.

andrewrk avatar Jun 16 '24 17:06 andrewrk

After #20321 you should see the same failures locally when running zig build test, and you can check only code formatting conformance with zig build test-fmt.

Edit: I think if you rebase and resolve that build.zig conflict, those CI failures will disappear.

andrewrk avatar Jun 17 '24 02:06 andrewrk

Nice thanks! I'll do the rebase and get started on removing & from the syntax when I'm back in town next week.

MasonRemaley avatar Jun 19 '24 12:06 MasonRemaley

By the way, in case you're unfamiliar with the AstGen code, you'll just want try rl.ri.resultType(gz, node) orelse .none in the corresponding case in AstGen.builtinCall.

Thanks, I'm not familiar with this code so this is helpful!

I started implementing storing the result type on imports, but got a bit stuck.

AstGen used to use addStrTok(...) to store the import path. Since we now want to store both the import path and a result type, I store a Zir.Inst.Bin Heres's what AstGen does now:

// AstGen.builtinCall
const operand_node = params[0];
const rhs = try expr(gz, scope, .{ .rl = .none }, operand_node);
try gz.addPlNode(.import, node, Zir.Inst.Bin{
    .lhs = result_type,
    .rhs = rhs,
});

Zir used to use .str_tok to get the import path, that's also been updated:

// Zir.zirImport
const inst_data = sema.code.instructions.items(.data)[@intFromEnum(inst)].pl_node;
const extra = sema.code.extraData(Zir.Inst.Bin, inst_data.payload_index).data;
const src = block.nodeOffset(inst_data.src_node);
const operand = try sema.resolveConstString(
    block,
    src,
    extra.rhs,
    .{ .needed_comptime_reason = "import path must be comptime-known" },
);

When this code runs, resolveConstString fails:

thread 7410 panic: index out of bounds: index 123056708, len 16
Analyzing lib/std/std.zig
    > %344 = import(%3306982439, %123057044)
      %345 = break_inline(%343, %344)
    For full context, use the command
      zig ast-check -t lib/std/std.zig

  in lib/std/std.zig
    > %436 = decl_val("start")

/home/mason/Documents/zon/zig/src/Sema.zig:299:25: 0x6884afc in contains (zig)
        return map.items[@intFromEnum(key) - @intFromEnum(map.start)] != .none;
                        ^
/home/mason/Documents/zon/zig/src/Sema.zig:255:26: 0x649170c in get (zig)
        if (!map.contains(key)) return null;
                         ^
/home/mason/Documents/zon/zig/src/Sema.zig:1857:39: 0x6491605 in resolveInst (zig)
        const inst = sema.inst_map.get(i).?;
                                      ^
/home/mason/Documents/zon/zig/src/Sema.zig:1887:42: 0x6d84bcf in resolveConstString (zig)
    const air_inst = try sema.resolveInst(zir_ref);
                                         ^
/home/mason/Documents/zon/zig/src/Sema.zig:13928:48: 0x68ae4b4 in zirImport (zig)
    const operand = try sema.resolveConstString(

It appears that the value I get back in Sema for extra.rhs is garbage, instead of the one that I put in during AstGen.

Any pointers would be much appreciated--I can also push the WIP commit if that would be helpful, but I imagine that I'm probably missing something that will be obvious to people more familiar with this part of the codebase.

MasonRemaley avatar Jun 28 '24 23:06 MasonRemaley

You probably just need to clear your cache directory -- ZIR is cached based on zig version, so if you haven't created a commit with your changes, the compiler will use the old cached ZIR!

However, your lowering strategy for import isn't really suitable. Since the operand must be a string literal, it's just a waste of bytes and time to use expr to actually create a str instruction. So, you shouldn't use Zir.Inst.Bin as the payload here -- instead, create a new payload type which stores result_ty: Ref, name: NullTerminatedString, and use that as the payload. (For the union field, pl_node is fine; pl_tok would also be fine.)

mlugg avatar Jun 28 '24 23:06 mlugg

You probably just need to clear your cache directory -- ZIR is cached based on zig version, so if you haven't created a commit with your changes, the compiler will use the old cached ZIR!

Ha that was it, thanks! I made the same mistake yesterday and didn't quite understand how I fixed it--that had been bugging me glad to have it resolved.

However, your lowering strategy for import isn't really suitable. Since the operand must be a string literal, it's just a waste of bytes and time to use expr to actually create a str instruction. So, you shouldn't use Zir.Inst.Bin as the payload here -- instead, create a new payload type which stores result_ty: Ref, name: NullTerminatedString, and use that as the payload. (For the union field, pl_node is fine; pl_tok would also be fine.)

Makes sense, will do!

MasonRemaley avatar Jun 29 '24 00:06 MasonRemaley

I'm now starting to use the result type in my code that lowers zon. I'm attempting to inspect a struct type using Type.structFieldType, but it's failing because the field types are all currently set to .none.

I'll investigate more later today, but my guess is that I just need to call a helper function that finishes filling in the type or something before I call structFieldType.

MasonRemaley avatar Jul 04 '24 00:07 MasonRemaley

Does that need to be part of this changeset? I recommend to get the CI tests passing and your initial patch landed before embarking on another enhancement.

andrewrk avatar Jul 04 '24 01:07 andrewrk

@andrewrk, yes, using the result type is a necessary enhancement for this to be useful -- read earlier comments for details. TL;DR, it's necessary to handle slices correctly, because ZON doesn't need & to initialize a slice, but a tuple of course cannot coerce to a slice.

mlugg avatar Jul 04 '24 02:07 mlugg

@MasonRemaley, you probably want Sema.resolveTypeFields (name off the top of my head, could be misremembering!).

mlugg avatar Jul 04 '24 02:07 mlugg

Thanks--got it working! The & operator is no longer allowed in ZON.

When the lowering code recurses through the AST, it checks for each value it's emitting if the result location expects a pointer, and then adds any necessary indirection.

I need to add some more test coverage for some edge cases this makes possible, and then I'll update the runtime parser to match this behavior which should be trivial and then I think this will be ready.

[EDIT] Started adding test and realized the above change is overly general and results in edge cases that aren't worth addressing--I'll revise it to only apply this logic to convert arrays -> slices where needed.

MasonRemaley avatar Jul 06 '24 02:07 MasonRemaley

I got to the point in my project where I wanna start using ZON, which gave me some good motivation to get through the remaining TODOs I had.

This is ready for review!

MasonRemaley avatar Jul 20 '24 03:07 MasonRemaley

CI is failing due to some changes I made to string and integer literal error handling that incorrectly didn't show notes in some places--will fix later today or tomorrow.

MasonRemaley avatar Jul 20 '24 04:07 MasonRemaley

Fixed the previous failure, now getting a CI failure with the C backend. I'm able to reproduce it locally with:

zig test lib/std/std.zig -target x86_64-linux -ofmt=c

Here's the failure from the CI server:

error: thread 1170000 panic: reached unreachable code
/home/ci/actions-runner1/_work/zig/zig/lib/std/debug.zig:404:14: 0x4cd378f in assert (zig)
    if (!ok) unreachable; // assertion failure
             ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:8089:15: 0x651673b in freeLocal (zig)
        assert(!f.allocs.contains(local_index));
              ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:8066:18: 0x6516a13 in die (zig)
    try freeLocal(f, inst, local_index, ref_inst);
                 ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:8105:16: 0x680501f in feed (zig)
        try die(bt.f, bt.inst, op_ref);
               ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:7122:24: 0x683fe33 in airAggregateInit (zig)
            try bt.feed(element);
                       ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:3257:54: 0x651be3b in genBodyInner (zig)
            .aggregate_init   => try airAggregateInit(f, inst),
                                                     ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:3032:25: 0x651e4b3 in genBody (zig)
        try genBodyInner(f, body);
                        ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:3073:20: 0x618d5fb in genBodyResolveState (zig)
        try genBody(f, body);
                   ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:4941:28: 0x6825e9b in airCondBr (zig)
    try genBodyResolveState(f, inst, liveness_condbr.then_deaths, then_body, false);
                           ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:3232:47: 0x651ad0f in genBodyInner (zig)
            .cond_br          => try airCondBr(f, inst),
                                              ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:3071:25: 0x618d5d7 in genBodyResolveState (zig)
        try genBodyInner(f, body);
                        ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:4605:28: 0x680d943 in lowerBlock (zig)
    try genBodyResolveState(f, inst, &.{}, body, true);
                           ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:4582:22: 0x680dfc7 in airBlock (zig)
    return lowerBlock(f, inst, @ptrCast(f.air.extra[extra.end..][0..extra.data.body_len]));
                     ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:3220:46: 0x651a657 in genBodyInner (zig)
            .block            => try airBlock(f, inst),
                                             ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:3032:25: 0x651e4b3 in genBody (zig)
        try genBodyInner(f, body);
                        ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:3073:20: 0x618d5fb in genBodyResolveState (zig)
        try genBody(f, body);
                   ^
/home/ci/actions-runner1/_work/zig/zig/src/codegen/c.zig:2833:28: 0x618c5a3 in genFunc (zig)
    try genBodyResolveState(f, undefined, &.{}, main_body, false);
                           ^
/home/ci/actions-runner1/_work/zig/zig/src/link/C.zig:252:20: 0x618e427 in updateFunc (zig)
    codegen.genFunc(&function) catch |err| switch (err) {
                   ^
/home/ci/actions-runner1/_work/zig/zig/src/link.zig:425:82: 0x5c9336b in updateFunc (zig)
                return @as(*tag.Type(), @fieldParentPtr("base", base)).updateFunc(pt, func_index, air, liveness);
                                                                                 ^
/home/ci/actions-runner1/_work/zig/zig/src/Zcu/PerThread.zig:826:22: 0x5872f1b in linkerUpdateFunc (zig)
        lf.updateFunc(pt, func_index, air, liveness) catch |err| switch (err) {
                     ^
/home/ci/actions-runner1/_work/zig/zig/src/Compilation.zig:4033:36: 0x5434f63 in processOneCodegenJob (zig)
            try pt.linkerUpdateFunc(func.func, func.air);
                                   ^
/home/ci/actions-runner1/_work/zig/zig/src/Compilation.zig:4005:33: 0x5434157 in codegenThread (zig)
            processOneCodegenJob(tid, comp, codegen_job) catch |job_error| {
                                ^
/home/ci/actions-runner1/_work/zig/zig/lib/std/Thread/Pool.zig:178:50: 0x543425b in runFn (zig)
            @call(.auto, func, .{id.?} ++ closure.arguments);
                                                 ^
/home/ci/actions-runner1/_work/zig/zig/lib/std/Thread/Pool.zig:291:32: 0x53e67a3 in worker (zig)
            run_node.data.runFn(&run_node.data, id);
                               ^
/home/ci/actions-runner1/_work/zig/zig/lib/std/Thread.zig:409:13: 0x5138a13 in callFn__anon_74518 (zig)
            @call(.auto, f, args);
            ^
/home/ci/actions-runner1/_work/zig/zig/lib/std/Thread.zig:678:30: 0x4ee7d63 in entryFn (zig)
                return callFn(f, args_ptr.*);
                             ^
lib/libc/musl/src/thread/pthread_create.c:207:17: 0xc9d36db in start (lib/libc/musl/src/thread/pthread_create.c)
???:?:?: 0x0 in ??? (???)

If I comment out the tests for my standard library contributions, it passes. IIUC this means that my standard library contributions are failing to compile on the C backend for some reason.

I'm not sure the best way to debug this--I think the first step is probably to figure out what code it's failing on. Any tips on how to do that would be appreciated!

MasonRemaley avatar Jul 22 '24 23:07 MasonRemaley

Update on this--I've narrowed the issue down to a three of the tests in std/zon:

  • stringify primitives
  • arrays and slices
  • free on error

If these are disabled, zig test lib/std/std.zig -target x86_64-linux -ofmt=c succeeds. Next step is to look into why.

MasonRemaley avatar Jul 26 '24 05:07 MasonRemaley

Here's a repro for the first of the ~two~three C backend failures this PR triggers that doesn't call into any ZON stuff:

var buffer = std.ArrayList(u8).init(std.testing.allocator);
defer buffer.deinit();
const writer = buffer.writer();
try std.fmt.formatInt(680564733841876926926749214863536422912, 10, .lower, .{}, writer);
try std.testing.expectEqualSlices(u8, "680564733841876926926749214863536422912", buffer.items);

I suspect that this is a preexisitng bug in the C backend that didn't have test coverage until now. I tried to verify this by running this test on master, however, the C backend tests fail on master for me even without any changes: gist

Is master failing these tests right now, or am I doing something wrong?

MasonRemaley avatar Jul 30 '24 22:07 MasonRemaley

@MasonRemaley I get the same error as you running build/stage4/bin/zig test lib/std/std.zig -target x86_64-linux -ofmt=c on a debug build of master (0.14.0-dev.739+a69d403cb), but it works fine when I add -lc (as the Zig tests are doing: https://github.com/ziglang/zig/blob/a69d403cb2c82ce6257bfa1ee7eba52f895c14e7/test/tests.zig#L85).

I tried running your test case in a standalone file using this approach (zig test -target x86_64-linux -ofmt=c -lc test.zig), and I didn't get any compiler crash, but I did get a bunch of "call to undeclared function 'zig_lo_big'", so it's likely I'm doing something wrong. However, this test case does reproduce the same crash observed in CI for me when tested with the same command:

test {
    try @import("std").testing.expectError(error.E, f());
}

fn f() error{E}![0]u8 {
    return error.E;
}
Stack trace
thread 358310 panic: reached unreachable code
/var/home/ian/src/zig/lib/std/debug.zig:404:14: 0x17dca2c in assert (zig)
    if (!ok) unreachable; // assertion failure
             ^
/var/home/ian/src/zig/src/codegen/c.zig:8089:15: 0x31c6c1d in freeLocal (zig)
        assert(!f.allocs.contains(local_index));
              ^
/var/home/ian/src/zig/src/codegen/c.zig:8066:18: 0x31c6ebd in die (zig)
    try freeLocal(f, inst, local_index, ref_inst);
                 ^
/var/home/ian/src/zig/src/codegen/c.zig:8105:16: 0x3505ebd in feed (zig)
        try die(bt.f, bt.inst, op_ref);
               ^
/var/home/ian/src/zig/src/codegen/c.zig:7122:24: 0x354af3a in airAggregateInit (zig)
            try bt.feed(element);
                       ^
/var/home/ian/src/zig/src/codegen/c.zig:3257:54: 0x31cd0a0 in genBodyInner (zig)
            .aggregate_init   => try airAggregateInit(f, inst),
                                                     ^
/var/home/ian/src/zig/src/codegen/c.zig:3032:25: 0x31cfbc6 in genBody (zig)
        try genBodyInner(f, body);
                        ^
/var/home/ian/src/zig/src/codegen/c.zig:3073:20: 0x2db5183 in genBodyResolveState (zig)
        try genBody(f, body);
                   ^
/var/home/ian/src/zig/src/codegen/c.zig:4941:28: 0x352c0b2 in airCondBr (zig)
    try genBodyResolveState(f, inst, liveness_condbr.then_deaths, then_body, false);
                           ^
/var/home/ian/src/zig/src/codegen/c.zig:3232:47: 0x31cbd3d in genBodyInner (zig)
            .cond_br          => try airCondBr(f, inst),
                                              ^
/var/home/ian/src/zig/src/codegen/c.zig:3071:25: 0x2db514d in genBodyResolveState (zig)
        try genBodyInner(f, body);
                        ^
/var/home/ian/src/zig/src/codegen/c.zig:4605:28: 0x350f70e in lowerBlock (zig)
    try genBodyResolveState(f, inst, &.{}, body, true);
                           ^
/var/home/ian/src/zig/src/codegen/c.zig:4582:22: 0x350fe72 in airBlock (zig)
    return lowerBlock(f, inst, @ptrCast(f.air.extra[extra.end..][0..extra.data.body_len]));
                     ^
/var/home/ian/src/zig/src/codegen/c.zig:3220:46: 0x31cb554 in genBodyInner (zig)
            .block            => try airBlock(f, inst),
                                             ^
/var/home/ian/src/zig/src/codegen/c.zig:3032:25: 0x31cfbc6 in genBody (zig)
        try genBodyInner(f, body);
                        ^
/var/home/ian/src/zig/src/codegen/c.zig:3073:20: 0x2db5183 in genBodyResolveState (zig)
        try genBody(f, body);
                   ^
/var/home/ian/src/zig/src/codegen/c.zig:2833:28: 0x2db3dbe in genFunc (zig)
    try genBodyResolveState(f, undefined, &.{}, main_body, false);
                           ^
/var/home/ian/src/zig/src/link/C.zig:252:20: 0x2db6227 in updateFunc (zig)
    codegen.genFunc(&function) catch |err| switch (err) {
                   ^
/var/home/ian/src/zig/src/link.zig:426:82: 0x29002e1 in updateFunc (zig)
                return @as(*tag.Type(), @fieldParentPtr("base", base)).updateFunc(pt, func_index, air, liveness);
                                                                                 ^
/var/home/ian/src/zig/src/Zcu/PerThread.zig:826:22: 0x246e8f9 in linkerUpdateFunc (zig)
        lf.updateFunc(pt, func_index, air, liveness) catch |err| switch (err) {
                     ^
/var/home/ian/src/zig/src/Compilation.zig:4070:36: 0x1fe8d30 in processOneCodegenJob (zig)
            try pt.linkerUpdateFunc(func.func, func.air);
                                   ^
/var/home/ian/src/zig/src/Compilation.zig:4042:33: 0x1fe7e6f in codegenThread (zig)
            processOneCodegenJob(tid, comp, codegen_job) catch |job_error| {
                                ^
/var/home/ian/src/zig/lib/std/Thread/Pool.zig:178:50: 0x1fe7fb1 in runFn (zig)
            @call(.auto, func, .{id.?} ++ closure.arguments);
                                                 ^
/var/home/ian/src/zig/lib/std/Thread/Pool.zig:291:32: 0x1f9639d in worker (zig)
            run_node.data.runFn(&run_node.data, id);
                               ^
/var/home/ian/src/zig/lib/std/Thread.zig:409:13: 0x1cb3ffa in callFn__anon_74738 (zig)
            @call(.auto, f, args);
            ^
/var/home/ian/src/zig/lib/std/Thread.zig:678:30: 0x1a31a12 in entryFn (zig)
                return callFn(f, args_ptr.*);
                             ^
???:?:?: 0x7fbe01f501b6 in ??? (libc.so.6)
Unwind information for `libc.so.6:0x7fbe01f501b6` was not available, trace may be incomplete

???:?:?: 0x7fbe01fd23cb in ??? (libc.so.6)
cAborted (core dumped)

This example still needs to be reduced further (the code ultimately triggering this is probably buried deep within std.fmt somewhere), but hopefully it helps with the analysis. I'll give a shot later this week at trying to reduce it further (and maybe learning how to use zig reduce) unless someone else finds the root cause sooner.

ianprime0509 avatar Jul 31 '24 03:07 ianprime0509

@ianprime0509 thank you very much--realizing I was missing that flag unblocked me on this and that smaller repro is very helpful!

One of the three failing tests actually was a bug on my end, I fixed that one. The other two appear to be preexisting issues to I've filed separate issues to track them here:

  • #20880
  • #20881

MasonRemaley avatar Jul 31 '24 07:07 MasonRemaley

Alright, this should be ready for review now!

MasonRemaley avatar Jul 31 '24 19:07 MasonRemaley

@mlugg Thanks for looking this over!

It gives better error messages; if a single field is wrong somewhere deep in the ZON, this will give the error at that field, rather than a less-than-helpful error at the @import.

I agree, the error messages are very poor. You can also trigger the same issue without ZON like this:

const Struct = struct {
    outer: struct { inner: u8 },
};
const some_value = .{ .outer = .{ .inner = 1.5 } };
const casted: Struct = some_value;
src/main.zig:22:28: error: fractional component prevents float value '1.5' from coercion to type 'u8'
    const casted: Struct = some_value;

My plan was to get this merged, and then make a separate PR that fixes the handling of this case, thereby improving error handling overall instead of just fixing it for ZON. This also saves me from duplicating logic for type checking that already exists.

However...

Am I correct in understanding that after #16865 it won't even be possible to trigger this example case? If anonymous structs are no longer special, then you're not even allowed to attempt this coercion in the first place right? Instead of getting an error about the field with unhelpful line number info, you'd get an error just saying those are two completely different struct types.

If so, then my plan doesn't make sense, because I'd just be fixing the handling of a case that won't even be possible in Zig in the near future, and relying on existing error handling that will have been removed. If that's the case then I think it's better to do the type checking here as you suggested.

MasonRemaley avatar Aug 06 '24 21:08 MasonRemaley

@MasonRemaley yes, your understanding of #16865 is correct. Sorry, I don't know why I focused on pointers in my explanation!

So, yeah, that example will fail after #16865 is implemented, because some_value will have a concrete struct type which doesn't coerce to Struct. Your thinking is completely right, and I reach the same conclusion that the type checks / coercions must happen during lowering.

mlugg avatar Aug 07 '24 00:08 mlugg

@andrewrk Thanks for the feedback! I'll work through all the review comments, and then ping here again when I'm done.

MasonRemaley avatar Aug 17 '24 05:08 MasonRemaley

I hope this PR isn't abandoned! Excited to finally have a ZON parser in the stdlib

VisenDev avatar Sep 14 '24 00:09 VisenDev

@VisenDev it's not abandoned--this feature is very important to my work. I'm working on some other stuff w/ tight deadlines right now, but I'm planning on getting back to this in about a week and a half.

MasonRemaley avatar Sep 14 '24 02:09 MasonRemaley

FYI the last several CI runs have all failed the same way: compile errors when targeting a 32-bit system. You can easily reproduce with zig build -Dtarget=arm-linux.

andrewrk avatar Nov 13 '24 03:11 andrewrk

Thanks for the explanation! I don't expect the CI to succeed right now as I'm partway through refactoring to parse imports to a known result type.

(Locally, the tests that I expect to pass at this point are all passing, and I expect all of them will on CI too once I'm done--if not I'll try this.)

[EDIT] Totally missed that you said compile error. You explained in person, my last commit likely fixes the issue, if it doesn't I'll test it locally to sort it out.

MasonRemaley avatar Nov 13 '24 03:11 MasonRemaley

All the ZON related tests now pass locally without reliance on anon struct types! Will see whether CI passes or not.

Major todos: improve error messages in the compile time parser, address existing code review feedback.

MasonRemaley avatar Dec 03 '24 03:12 MasonRemaley

Needs a rebase to kick off the CI

andrewrk avatar Dec 03 '24 03:12 andrewrk