Crystal: `Hash#merge` vs `Set#merge`

[splattael]

Hi,

today I stumbled upon Hash#merge.

Expect

Hash#merge behaves exactly like Hash#merge! except it creates a copy.

Actual

Hash#merge merges the values (as expected) but also unifies the types (not expected).

Code

Hash(String, String).new.merge(Hash(Int32, Int32).new)
# NOT EXPECTED
# => Hash(Int32 | String, Int32 | String)


Hash(String, String).new.merge!(Hash(Int32, Int32).new)
# EXPECTED
# no overload matches 'Hash(String, String)#[]=' with types Int32, Int32
# Overloads are:
#  - Hash(K, V)#[]=(key : K, value : V)
# 
#       self[k] = v
#           ^

Additional example: Set#merge merges to sets of the same type without type unification:

p Set(String).new.merge(Set(Int32).new).class
# EXPECTED
# no overload matches 'Hash(String, String)#[]=' with types Int32, Int32
# Overloads are:
#  - Hash(K, V)#[]=(key : K, value : V)
# 
#       self[k] = v
#           ^

Personally, I'd expect Hash#merge to behave like Set#merge.

Having said that, the current implementation of Hash#merge could be renamed to Hash#union.
Additionally, we could introduce Set#union as well.

Thoughts?

Kind regards,
Peter

[RX14]

I think this is a bug in Set#merge not creating a union set. If you want to ensure that the resulting set is the same type, simple apply type restrictions to the Hash in your code before using #merge.

[splattael]

@RX14 Sorry, I don't get it. Could you please give some code examples?

[RX14]

Actually merge on set seems to be equivalent to merge! on Hash. I think it's a good idea to rename Set#merge to Set#merge! to avoid confusion.

@splattael If you use .as in the argument to merge it will fail at compile time if it's impossible: https://carc.in/#/r/1fe8. This allows you to ensure that the types are exactly the same.

[splattael]

@RX14 I find the following equivalent looking code example confusing:

hash = Hash(String, Int32).new
hash["foo"] = 42
hash["bar"] = 23.5 # => no overload matches 'Hash(String, Int32)#[]=' with types String, Float64
# OK

vs

hash = Hash(String, Int32).new.merge({"foo" => 42, "bar" => 23.5})
p hash # => Hash(String, Float64 | Int32)
# WAT?

[RX14]

@splattael the difference between those two examples is one is creating a new Hash while one is modifying the old one. This is why the behaviour is different.

[drosehn]

FWIW, I'm also someone fairly new to crystal (but with plenty of experience in ruby). And in thinking about this I do expect Hash#merge to behave the way it currently behaves. If I was merging two hashes (ahash and bhash) which had different types of elements, I'd find it irritating if I got errors for any element in bhash which was not acceptable to the types of ahash. After all, I'm creating a new hash from those two hashes, so why wouldn't I want a new hash which accepts all elements from both hashes?

I'd also want ahash.merge(bhash) to produce basically the same result as bhash.merge(ahash) (except for the ordering of the keys, and for which value is used for any key which exists in both hashes). That includes having the same types-accepted for the newly created hash, no matter which order the merge was done.

I would not expect ahash.merge!(bhash) to produce the same as bhash.merge!(ahash), because I wouldn't expect (or want!) merge! to change the types-accepted for an already-existing hash.

And if I wanted to get the behavior you described, then I'd immediately think of using .as() to do that, because I've had to use as() in similar situations. (situations which have nothing to do with Hashes.)

That's how I think about it, at least.

[asterite]

Yeah, we should probably rename merge to merge!. To create a set that is the combination of two sets we have |.

[splattael]

Thanks!