encoding/json: unmarshal uses the first struct in a union of structs

[Feoramund]

Context

json.unmarshal seems to be ignoring alternative structs in a union, because if it doesn't find the fields it's looking for on the first struct variant it tries, it parses it as any value, skipping the other possibilities.

Odin/core/encoding/json/unmarshal.odin

Lines 461 to 474 in 5b6c96c

	} else {
	// allows skipping unused struct fields
	// NOTE(bill): prevent possible memory leak if a string is unquoted
	allocator := p.allocator
	defer p.allocator = allocator
	p.allocator = mem.nil_allocator()
	parse_value(p) or_return
	if parse_comma(p) {
	break struct_loop
	}
	continue struct_loop
	}

Odin:    dev-2024-04:2416380f3
OS:      Arch Linux, Linux 6.8.7-arch1-1
CPU:     12th Gen Intel(R) Core(TM) i7-12700K
RAM:     31916 MiB
Backend: LLVM 14.0.6

Expected Behavior

JSON unmarshalling to B instead of A, or some sort of error/advisory/workaround.

Current Behavior

Invalid type assertion from U to B, actual type: A

Failure Information (for bugs)

Steps to Reproduce

package main

import "core:encoding/json"

A :: struct {
    a: int,
}

B :: struct {
    b: string,
}

U :: union {
    rawptr,
    A,
    B,
}

main :: proc() {
    u: U

    error := json.unmarshal_string(`{"b": "Hellope"}`, &u)
    assert(error == nil)
    assert(u.(B).b == "Hellope")
}

[gingerBill]

So this is actually tricky to handle correctly because it has to do a best fit search, and currently it does the "first fit" search. In the case other structs, a missing field might not be an error, so it just gets ignored.

[Spooky309]

Other JSON implementations I've seen use a "$Variant" key in the JSON, usually with a string name of the variant (to prevent changes in the union declaration from causing the IDs to shift and break the JSON), using the $ specifically because it's illegal in the language to begin a field name with $ so there won't be any conflicts with actual field names. There's a closed PR I saw a couple of months ago that did something similar, but it wasn't quite right.

Would something like this be at all appropriate? I feel it would beat having a field called this_is_a_button in my structs 😄

[karl-zylinski]

There was a discussion on the discord about making it configurable.
Default behavior is the current "best fit"
and then you can set it into a "variant type name" mode, where the union would be stored like you say:

{
    data: {
        // the variant data
    },
    variant: "B",
}

the variant key could use the dollar sign you mentioned.

Then there is the question if the variant should store the type name of the variant or the union tag number. I've seen situations where both are more practical:

• If you use type name then you cannot change type names (or you need some json tag that says the old type name)
• If you use union tag number then you can change type name, but you cannot reorder the union.

[Spooky309]

Now that I think about it, the tag number is probably more practical in general, since there's no real reason to reorder an enum (that I can think of?), but there are very good reasons to rename things, but surely it could also be configurable? There's an argument against not "bloating" the configuration though, or providing defaults that could catch someone off-guard.

Maybe I need to hop on the Discord 😃

[karl-zylinski]

Did a draft PR related to this: #4293

In it I added an option to make the marshaller write out the variant name and tag and the unmarshaller is able to look at that data in a backwards compatible way.

[MalteJanz]

Hi👋 ,

I'm just getting started with Odin and also ran into this issue. I'm coming from Rust + web development and just want to give some feedback on how other ecosystems deal with this.

In Rust the tagged unions are just called enums and there is a very popular library for serializing and deserializing data called serde. Also keep in mind that the json data might come from an external API server and could contain multiple tagged unions that are represented differently and thus each need their own handling. That's why I think going with some kind of annotations on the union definition makes more sense then a "global" marshal config option that applies to the whole json data.

In serde there are four different representations for tagged unions, see their documentation: https://serde.rs/enum-representations.html

• Externally tagged (the default in serde)
• Internally tagged
• Adjacently tagged
• Untagged

To me it looks like the current implementation marshals into the untagged representation just fine but right now has difficulties unmarshaling that again to the same data, as it doesn't consider all variants. One awesome first step would be to just fix the unmarshalling to consider all variants, e.g. serde defines it like this:

There is no explicit tag identifying which variant the data contains. Serde will try to match the data against each variant in order and the first one that deserializes successfully is the one returned.

I don't know if you want to keep the current behaviour for backwards compatability but in my personal opinion I would expected that a marshal and then unmarshal gives me the same data back (at least where possible) and not suddenly a different union variant, which caught me by suprise 😅.

In the future it would also be great to support the other representations in a configurable per union way as that would provide more flexiblity especially when reading json from external sources. If you want I can create another issue for that 🙂

[MalteJanz]

I opened up a draft PR with the goal of just fixing the unmarshall logic to consider other union variants as well here:
#5758

That fixes the reproduction example for me without any change needed to the marshalling logic. But I haven't done extensive testing yet. Feel free to give it a try 🙂