Handle parsing into ambiguous types in a union

[LewisGaul]

As reported in https://zigforum.org/t/zig-nestedtext-release-0-1-0/383/17:

test "typed parse: jeang3nie" {
    const Kind = union(enum) {
        color: []const u8,
        plain,
    };
    var p = Parser.init(testing.allocator, .{});
    const r = try p.parseTyped(Kind, "> null");
    defer p.parseTypedFree(r);
    std.debug.print("{s}\n", .{r});
}

This parses into color with the string null since this is the first field that succeeds, but parsing as a string should probably be the last resort (since this will always succeed).

[LewisGaul]

What about the more general problem of preferring the type that's 'hardest' to parse into? E.g. prefer a u8 field over a u16 field if the integer fits into u8, regardless of field ordering? Or should the user be required to place the u8 field before u16 in this case?

[nfisher1226]

Neither of the above mentioned solutions are going to cover all uses of tagged unions unfortunately. Any time two or more tags have the same payload or one which can be ambiguous to the parser, as in your above example of a u8 and a u16, there is a very real chance that the parser will not be able to preserve the original data.

There is a similar issue in Rust using an enum with an associated data type, which is solved in serde by using a cfg attribute:

#[derive(Clone, Deserialize, Debug, Serialize)]
#[serde(tag = "ColorType")]
pub enum Color {
    Hex(HexColor),
    Reduced(ReducedRGBA),
    Rgba(RGBA),
}

And this serializes to the following toml

ColorType = "Reduced"
red = 36
green = 31
blue = 49
alpha = 255

The two languages are very different but I bring it up because having an enum which stores a value is an almost identical concept to a tagged union. In both cases we're encoding a variant along with it's data using the type system.

That tag name itself can be vital to the program logic. I'm currently implementing gradient backgrounds in Zterm, and my gradient can be linear, radial or elliptical. Circular or elliptical variants take a starting position, while a linear variant can have an angular direction or an ending position. Currently this is all encoded into the Kind tagged union, but zig-nestedtext is unable to preserve which variant is being used except in the case of a linear gradient with an angular direction, as the other two variants do not have this possibility. Or it might accidentally get it correct because it was linear and that happens to be the first possibility.

const Gradient = struct {
    kind: Kind,
    // other fields omitted
};

const Kind = union(enum) {
    linear: Direction,
    circular: Position,
    elliptical: Position,
};

const Direction = union(enum) {
    position: Position,
    angle: f64,
};
// Position is a struct with x,y coordinates

I could work around it by storing the variant separately as an enum in my Gradient struct, but this then becomes duplicate information and the entire point (in my use case at least) is to keep the configuration struct as small as possible. It's not a large overhead, but it illustrates a weakness in the current implementation. I think a better solution would be storing the tag in the nestedtext.

[LewisGaul]

Pasting the corresponding zigforum message from @nfisher1226 here too, since zigforum currently seems to be down (and I have the message in my emails).

So I don’t know if you remember talking about this before, but there was a bug with this when using tagged unions if there were two items that had no payload. In that case it would just pick the first one when parsing back into a struct. I have another related problem and I think both could be fixed with the same change. Let me give some background.

pub const Kind = union(enum) {
    linear: Direction,
    radial: Placement,
    elliptical: Placement,
};

pub const VerticalPlacement = enum {
    top,
    center,
    bottom,
};

pub const HorizontalPlacement = enum {
    left,
    center,
    right,
};

pub const Placement = struct {
    vertical: VerticalPlacement,
    horizontal: HorizontalPlacement,
};

pub const Direction = union(enum) {
    angle: f64,
    edge: Placement,
};

pub const Gradient = struct {
    kind: Kind,
    /// more fields omitted
}

When I have a Gradient struct that is a linear gradient with a direction going to the top left, here’s what is saved.

background:
  kind:
    vertical: top
    horizontal: left

Basically, we’re saving the union payload but not the tag. You know the NestedText spec better than me, and also the codebase, so would it be appropriate to embed the tag, too, so that we get something more like this?

background:
  kind:
    linear:
      edge:
        vertical: top
        horizontal: left

Right now by only saving the payload there’s no way to parse that back into the original struct. This is really a different example of the same problem, there are now three union variants that can all potentially have the same data (a position) and there’s no way to tell them apart, just like if there were two branches with no payload. Saving the tag would fix that condition as well is my thinking.

I could completely rethink my struct layout of course, but I would love for this library to be able to handle anything that gets thrown it’s way. And as always I’m willing to help.

[LewisGaul]

@nfisher1226 thanks for the thoughts here and clear description of the problem. I'm increasingly convinced that this issue should be addressed in one way or other, and if we can agree on a way to do so I'd be happy to accept a PR, or may be able to find time to implement it myself.

Neither of the above mentioned solutions are going to cover all uses of tagged unions unfortunately. Any time two or more tags have the same payload or one which can be ambiguous to the parser, as in your above example of a u8 and a u16, there is a very real chance that the parser will not be able to preserve the original data.

Yeah, I'm not convinced my initial proposal is the best way forwards. Perhaps it's an approach that should be taken in addition to a full solution, but that can be assessed separately (I'll split into a separate issue if necessary, let's keep using this issue for this discussion now - I've changed the title).

There is a similar issue in Rust using an enum with an associated data type, which is solved in serde by using a cfg attribute:

#[derive(Clone, Deserialize, Debug, Serialize)]
#[serde(tag = "ColorType")]
pub enum Color {
    Hex(HexColor),
    Reduced(ReducedRGBA),
    Rgba(RGBA),
}

And this serializes to the following toml

ColorType = "Reduced"
red = 36
green = 31
blue = 49
alpha = 255

This is great to know about and worth taking into consideration. What I'm confused about is how this generalises - it looks like the ReducedRGBA must be a mapping object (struct?), and the 'tag' then gets embedded inside that mapping, but then what happens if the payload of an enum field is not a struct?

Basically, we’re saving the union payload but not the tag. You know the NestedText spec better than me, and also the codebase, so would it be appropriate to embed the tag, too, so that we get something more like this?

background:
  kind:
    linear:
      edge:
        vertical: top
        horizontal: left

I did find it a bit weird when implementing parsing into unions that the tag names are ignored and not dumped out (which is behaviour I took from Zig's std.json). I don't think the NestedText spec comes into this at all, this behaviour of parsing into/dumping out types is intentionally the responsibility of the library as NestedText consists only of containers and strings, so we're free to design our own solution here.

I think it should be fine to provide an option to the parse and dump functions to include union tags (i.e. just treat them the same as structs but with only one field expected to be present), which should give you the deeply nested output you suggested above. The only thing is that this would apply to all unions in the parse/dump.

An alternative option would be to check for an attribute on structs/unions to check whether direct union members should have their tags included, which would allow for controlling it per usage of union fields (not per union type - either would be possible but I think the former makes more sense).

For example, using your previous example, if you put const nestedtext_tag_child_union = true; on the Gradient struct then you would include one of the 'linear', 'radial' or 'elliptical' tags in the output. You would also have the option of placing this attribute on the Kind union to include the 'angle' or 'edge' tag in the 'linear' case, but you might not want that since that union is unambiguous anyway.

Thoughts? One or both of these options seem reasonable to me, but it's possible I'm overlooking something.

[nfisher1226]

Yeah, I'm not convinced my initial proposal is the best way forwards. Perhaps it's an approach that should be taken in addition to a full solution

Probably wise. As the code sees more usage other edge/corner cases may show themselves, so the extra approach might help. Not to mention, there are also untagged unions to account for.

it looks like the ReducedRGBA must be a mapping object (struct?), and the 'tag' then gets embedded inside that mapping

Exactly.

but then what happens if the payload of an enum field is not a struct?

Shouldn't matter actually. It's using the enum field name, not the name of the data type.

An alternative option would be to check for an attribute on structs/unions

I do kind of like this approach as it gives some fine grained control. However there is complexity to consider. The payoff must be weighed against the gain, and liking it doesn't necessarily make it worth it.

An argument can be made, however, that since NestedText is meant to be human readable it might be a good idea to have the tools to provide clarity where it is needed, without nesting things overly deep where that clarity isn't necessary.

I know that's not expressing a clear preference, I'm just thinking as I type right now.

[LewisGaul]

@nfisher1226 Please try out the include-union-tags branch, where you can place a nestedtext_include_tag attribute on unions to have the tag included in a single-field key-value object (parsing and dumping behaviour should match up).

See the tests in src/nestedtext.zig for how to use this:

test "typed parse: union, include tag" {
    var p = Parser.init(testing.allocator, .{});

    const MyUnion = union(enum) {
        foo: usize,
        bar: bool,
        baz,

        const nestedtext_include_tag = true;
    };

    try testing.expectEqual(MyUnion{ .foo = 1 }, try p.parseTyped(MyUnion, "foo: 1"));
    try testing.expectEqual(MyUnion{ .bar = false }, try p.parseTyped(MyUnion, "bar: false"));
    try testing.expectEqual(MyUnion{ .baz = {} }, try p.parseTyped(MyUnion, "baz: null"));
}

test "from type: union, include tag" {
    const MyUnion = union(enum) {
        foo: usize,
        bar: bool,
        baz,

        const nestedtext_include_tag = true;
    };

    {
        const tree = try fromArbitraryType(testing.allocator, MyUnion{ .foo = 123 });
        defer tree.deinit();
        try testStringify("foo: 123", tree);
    }
    {
        const tree = try fromArbitraryType(testing.allocator, MyUnion{ .bar = true });
        defer tree.deinit();
        try testStringify("bar: true", tree);
    }
    {
        const tree = try fromArbitraryType(testing.allocator, MyUnion{ .baz = {} });
        defer tree.deinit();
        try testStringify("baz: null", tree);
    }
}

Any feedback welcome. I can create a release if this seems like a good solution.

[nfisher1226]

I tried a simple test program but it doesn't seem to be working for me. Not sure if I'm missing something here.

const std = @import("std");
const nt = @import("nestedtext");
const allocator = std.heap.page_allocator;
const stdout = std.io.getStdOut().writer();

const Foo = union(enum) {
    bar: u8,
    baz: bool,
    buzz,

    const nestedtext_include_tag = true;
};

pub fn main() anyerror!void {
    var foo = Foo{ .bar = 42 };
    std.debug.print("{s}\n", .{foo});
    {
        const tree = try nt.fromArbitraryType(allocator, foo);
        defer tree.deinit();
        try tree.stringify(.{}, stdout);
    }
    std.debug.print("\n\n", .{});
    foo = Foo{ .baz = true };
    std.debug.print("{s}\n", .{foo});
    {
        const tree = try nt.fromArbitraryType(allocator, foo);
        defer tree.deinit();
        try tree.stringify(.{}, stdout);
    }
    std.debug.print("\n\n", .{});
}

Program output:

Foo{ .bar = 42 }
> 42

Foo{ .baz = true }
> true

What's odd is that the tests are passing, so it apparently is working in the test block but not in my code.

[nfisher1226]

Ah, figured it out. Needs to be pub to be seen outside that file, which means it's only visible to zig-nestedtext if it's pub. Should have thought of that right away.

pub const nestedtext_include_tag = true;

Tried it as above and it worked, tried it with the buzz field mapping to a struct and it worked nicely in that instance as well. Let me look some more things over and test a few more cases, but this looks good right now. That little gotcha about making the nestedtext_include_tag declaration pub should be documented so that it doesn't trip up others, but that's the only issue I currently see.

[LewisGaul]

Yeah, I thought it would be the missing pub. I don't currently have any documentation other than docstrings and tests... But yes, when documentation appears it would be good to include this, even though it could be considered a Zig gotcha more than a gotcha in the nestedtext library.

I'm thinking I might also add the ability to specify whether union tags should be included globally (e.g. when calling fromArbitraryType()), where the per-union field would override it either way.

[LewisGaul]

@nfisher1226 Would you like to try out the include-union-tags-global branch? This adds an options arg to fromArbitraryType() and Parser.parseTyped() (and Parser.parseTypedFree()).

My main question is whether this backwards incompatible API change of having to specify the options arg (as .{} to use the status quo default) is worth it or whether it's just annoying (especially considering Parser.init() already takes a set of options for handling the actual parsing from text).

It works as follows:

test "from type: union, with and without tags" {
    const PlainUnion = union(enum) {
        foo: usize,
        bar: bool,
        baz,
    };

    const AttrUnion = union(enum) {
        x: PlainUnion,
        y: PlainUnion,

        const nestedtext_include_tag = true;
    };

    {
        const tree = try fromArbitraryType(
            testing.allocator,
            AttrUnion{ .x = .{ .foo = 123 } },
            .{},
        );
        defer tree.deinit();
        try testStringify("x: 123", tree);
    }
    {
        const tree = try fromArbitraryType(
            testing.allocator,
            AttrUnion{ .x = .{ .foo = 123 } },
            .{ .include_union_tags = true },
        );
        defer tree.deinit();
        try testStringify("x:\n  foo: 123", tree);
    }
}

[nfisher1226]

I'll give it a try later tonight.