Crystal: JSON dup is broken in 0.25.0

[bararchy]

0.24.2 example => https://play.crystal-lang.org/#/r/4br0
0.25.0 example => https://play.crystal-lang.org/#/r/4br2

[RX14]

It's not a regression, JSON::Any behaved just the same as always: https://play.crystal-lang.org/#/r/4bv4

This is a feature request to override dup in JSON::Any

[asterite]

It's not a bug.

JSON::Any is now a struct. Dupping a struct is just copying its bytes. The JSON::Any instance has a reference (pointer) to a hash, which remains the same (the reference stays the same).

In 0.24.0, the returned object was directly a Hash (inside the JSON::Any union), and dupping that dups the hash, which has a different semantic (gives another hash with same referenced contents).

[ysbaddaden]

You mean it should use clone instead of dup? That's still a bit confusing.

[RX14]

The most transparent way to do this is to define dup as JSON::Any.new(@raw.dup). But JSON::Any has always worked this way.

[asterite]

So you just need dup = JSON::Any.new(any.value.dup)...

But probably you know it's a hash and you want to deal directly with it, so why not work directly with the hash?

value = any.to_h
value.dup
value["baz"] = ...

Why is it confusing? JSON::Any is a struct. Dupping a struct copies bytes, not contents. clone is an alternative, but that's deep.

[ysbaddaden]

I'm easily fooled and confused with the Any wrappers. I'd naively expect the value to be dup'ed.

I understand why it doesn't, but I wonder whether it shouldn't dup the reference. After all, we want to manipulate the value; the wrapper helps deal with it nicely, but suddenly a simple dup seems to break this.

[asterite]

I guess forwarding dup might be good enough.

I just can see more issues saying "let's add "each", let's add "foo", let's add..." so JSON::Any becomes a monster combining all methods of the underlying types.

I see JSON::Any as a wrapper of values. Maybe you should even remove [] and size from it, only allowing to get the raw value as a union and casting it to the appropriate types when needed (maybe even removing as_a, as_i, etc.).

JSON::Any is just the way to implement a recursive alias type without the recursive type, which introduced a lot of inconsistencies in the language. I'd go with that.

[asterite]

(and of course I would eventually remove recursive aliases)

[kimyu92]

As an aside, imo, dup and clone are always confusing.
I would suggest to unify api to shallow_clone (alias to clone) and deep_clone

[RX14]

@asterite I think a good reasoning for why dup and not each is that dup and clone are defined on all objects, therefore defined on all types which JSON::Any can contain. So it makes sense to define it on JSON::Any. Whereas .each would have to do something different based on the contents of the Any, which means it's just better to use as_a or as_h yourself.

I'm in favour of doing this.

[asterite]

Yes, let's do this. But as you said, it's defined for all objects already, including JSON::Any, which is a struct, and dup for structs is defined and documented, so the above behaviour shouldn't be a surprise (only when you used to use the old json behaviour)