Implement setindex for ImmutableDict

[danielmatz]

This adds a method of Base.setindex for Base.ImmutableDict, as suggested in JuliaObjects/Accessors.jl#216.

For a key that is already in the ImmutableDict, rather than add it again and shadow it, as described in the ImmutableDict doc string, I opted to exclude the original key and then add in the new one.

[giordano]

This is a bit misleading since it isn't changing the original dictionary but creating a new one.

[vtjnash]

This makes the complexity of ImmutableDict worse than Vector{Pair}, which is never useful

[danielmatz]

This is a bit misleading since it isn't changing the original dictionary but creating a new one.

Did you perhaps misread this as setindex!? This change is about Base.setindex. It is already implemented for Tuple, NamedTuple, and CartesianIndex, where it creates a new value with the index changed.

This makes the complexity of ImmutableDict worse than Vector{Pair}, which is never useful

Whenever I make a PR for Julia, it never takes me long to get in over my head... So, take this all with a grain of salt. But the more I look at Base.ImmutableDict, the more questions I have.

When I saw ImmutableDict, I thought it would be a fairly general purpose, immutable version of Dict. But that doesn't seem to be the case. It really is an association list, right? Why wasn't it just called AssociationList? I understand that an immutable dictionary can be implemented as an association list, but the implementation details seem to be leaking out a lot here.

I think my main question is whether ImmutableDict wants to be an immutable dictionary or an association list.

The most apparent ways it violates the expectations of a dictionary is the behavior of keys, iterate, show, etc. All keys, including the shadowed ones, are included.

julia> d = Base.ImmutableDict(:a => 1, :b => 2, :a => 3)
Base.ImmutableDict{Symbol, Int64} with 3 entries:
  :a => 3
  :b => 2
  :a => 1

julia> keys(d)
KeySet for a Base.ImmutableDict{Symbol, Int64} with 3 entries. Keys:
  :a
  :b
  :a

julia> values(d)
ValueIterator for a Base.ImmutableDict{Symbol, Int64} with 3 entries. Values:
  3
  2
  1

julia> collect(d)
3-element Vector{Pair{Symbol, Int64}}:
 :a => 3
 :b => 2
 :a => 1

I worry that users will iterate over an ImmutableDict and accidentally operate on the shadowed entries. The doc string says that adding a key again makes the old value "partially overridden and hidden," but that's probably not enough detail to help users. And there's a comment in the code warning to not use iterate for certain operations, but that also doesn't help users.

As another example, consider this:

julia> d1 = Base.ImmutableDict(:a => 1, :a => 2)
Base.ImmutableDict{Symbol, Int64} with 2 entries:
  :a => 2
  :a => 1

julia> d2 = Base.ImmutableDict(:a => 2, :a => 1)
Base.ImmutableDict{Symbol, Int64} with 2 entries:
  :a => 1
  :a => 2

julia> d1[:a]
2

julia> d2[:a]
1

julia> d1 == d2
true

Is this a bug? No matter whether ImmutableDict wants to be a dictionary or an association list, this seems wrong.

Getting back to my PR. If ImmutableDict is meant to be an association list, then an implementation like

function setindex(d::ImmutableDict, value, key)
    ImmutableDict(d, key => value)
end

seems appropriate, and I can change this PR to do that.

But if it is meant to be an immutable dictionary, then my original implementation seems better, despite the higher complexity, because it actually behaves like a dictionary.

[vtjnash]

It could have been an AssociationList (taking the name from some lisps), to emphasize that implementation detail. But lisps seem to commonly emphasize implementation details rather than the abstract concepts when naming things. Elsewhere it is more common to refer to this as a generalization of a dict, e.g. https://pypi.org/project/multidict/ and https://en.cppreference.com/w/cpp/container/unordered_multimap/find.html and others in https://en.wikipedia.org/wiki/Multimap

[cstjean]

setindex can do its job (as a getindex inverse) efficiently without removing the previous entry.

The AbstractDict interface doesn't seem to be defined anywhere. I'm uncomfortable with keys() returning duplicates, but I can't really make an argument against it. Its docstring doesn't preclude it, at any rate.