go-json-experiment / json

Experimental implementation of a proposed v2 encoding/json package
BSD 3-Clause "New" or "Revised" License
341 stars 11 forks source link

Undefined Behaviour in Error Messages #43

Closed lmittmann closed 1 week ago

lmittmann commented 1 week ago

I experience strange undefined behavior in regards to error messages when calling json.Unmarshal(..) and an error is returned.

Sometimes the error message prefix is json: cannot unmarshal Go value of type... , and sometimes it is json: unable to unmarshal Go value of type....

This undefined behavior causes e.g. google/go-cmp checks to panic sometimes.

I am using go1.22, and the latest master of this package.

j178 commented 1 week ago

I think this is an intended behavior, see https://github.com/go-json-experiment/json/blob/2d9f40f7385b5c44fa42ff92c05c12198e91e63b/errors.go#L46-L53

quasilyte commented 1 week ago

@j178 as impressive as it might look, I still think it's not good enough.

I believe returning an error in a random language (around 50 languages would be enough for the starters) plus some generated UUID would be much better and reliable. Shuffling every letter in the resulting error message could work too, but this step might be redundant.

non è possibile effettuare il marshalling dell'oggetto JSON nel valore Go di tipo 🤌🤌

dsnet commented 1 week ago

This undefined behavior causes e.g. google/go-cmp checks to panic sometimes.

I'm not sure what you're doing that's leading to a panic, but I don't recommend matching on the string representation of the error. The v2 experiment returns structured errors (i.e., SyntacticError and SemanticError) for a reason.

Tests that check error strings are inherently flaky and subject to breakage since error strings are not covered by any compatibility guarantees. While this module is still in development, the deliberate pseudo-randomization gives us more flexibility to change the error string since it prevents the introduction of flaky tests. Once the module is more stable, we can consider removing the randomization of error strings.

lmittmann commented 1 week ago

I think this is an intended behavior

@j178 thank you for pointing me here.

I'm not sure what you're doing that's leading to a panic

@dsnet I match on err.Error() as some of my possible errors are just error interfaces. google/go-cmp tests both x==y and y==x and if the results don't match it panics (panic: non-deterministic or non-symmetric function detected)

This is the first time I heard of Hyrum-proof - interesting concept. I guess this issue is proof it fulfills it's purpose. If this package makes it's way to the std 🤞🏻 it would be good to highlighting that behavior somewhere.

kortschak commented 1 week ago

While the Hyrum proofing is nice from a theoretical engineering perspective, this will make log spelunking potentially and security rule application more difficult.