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.