Document that get! may mutate Dict

[nalimilan]

This is guaranteed since #9595 but not documented.

[kshyatt]

Is there a way we could add a simple doctest showing this?

[nalimilan]

It's hard to come up with a simple example that isn't an anti-pattern, so I'm not sure it's really useful to show an example. Here's the less contrived one I could find though if you think it's worth showing:

  # Existing doctest
  julia> squares = Dict{Int, Int}();
  
  julia> function get_square!(d, i)
             get!(d, i) do
                 i^2
             end
         end
  get_square! (generic function with 1 method)
  
  julia> get_square!(squares, 2)
  4
  
  julia> squares
  Dict{Int64, Int64} with 1 entry:
    2 => 4

  # Added
  julia> function get_square_recursive!(d, i)
             get!(d, i) do
                 i > 0 && get_square_recursive!(d, i-1)
                 i^2
             end
         end

  julia> get_square_recursive!(squares, 5)
  25

  julia> squares
  Dict{Int64, Int64} with 4 entries:
    5 => 25
    4 => 16
    2 => 4
    3 => 9

[andrewjradcliffe]

The current docstring explicitly states that the function is permitted to mutate the receiver. The function can do anything it pleases; the only constraint is that the type of the value returned by the function call can be coerced to that of the value type of the collection.

To quote the relevant statement:

Return the value stored for the given key, or if no mapping for the key is present, store key => f(), and return f()

"store" implies mutation of the receiver. Any other mutation that occurs as the result of the function call is a design decision left to the programmer.

I fail to see how the proposed change clarifies the use of get!.

The current implementation calls f a single time, which, if Julia had function traits like Rust, could be expressed as FnOnce. This is the only place that clarification is worthwhile, but, it would lock down the public API so that f is guaranteed to be called only once (which is currently a private detail of the implementation, though, the behavior is observable through fuzzing).

[KristofferC]

It is not obvious that f itself is allowed to mutate the dict (because you have a mutation of the dict "in the middle", after you have checked that the key exists but before you have inserted the new one). This clarifies that it is ok.

[nalimilan]

Yes, in the current docstring "store" refers to what the get! implementation does, not to what f does or is allowed to do.

[andrewjradcliffe]

This clarifies that it is ok.

Now I see the need for the clarification, namely, that the internal state of the dict will not be irreparably mangled by f that mutates the receiver.

• By documenting the behavior, it becomes part of the public interface -- is that desirable? (N.B. I am not opposed, and, in fact am in favor of better interface documentation).
• Inspection of the code for Dict, IdDict and WeakKeyDict indicates that their implementations make mutating f safe. In particular, WeakKeyDict handles the potential reentrancy with a ReentrantLock, which is neat.

[nalimilan]

Good questions. I think it's worth being part of the public API, otherwise it doesn't really make sense to implement this behavior if we don't guarantee it. But I'm not sure whether we can say it's required for any AbstractDict or whether we should only mention concrete types implemented in Base.

@andyferris Do you think that guarantee would be OK for Dictionaries.jl?

[andyferris]

In Dictionaries.jl we can guarantee that f is called exactly once (iff the key is missing).

However, get! is optimized via tokens - it will generate a token with gettoken!, call f, and then call settokenvalue! with the token from the first step. This is pretty much the entire premise behind Dictionaries.jl (plus some exploration of what happens if we iterate values not pairs by default).

If f mutates the keys of a Dictionaries.Dictionary for example, then the token will be invalid and all sorts of badness can occur (we even use @inbounds so this is real undefined behavior territory).

This could be patched up by using and checking age counters; these were stripped for a few % of performance since I figured they were only there for WeakKeyDict (and concurrent but not parallel mutation in general I suppose?).

[andyferris]

(note the get! method is generic in Dictionaries.jl, using only the token interface; we'd need to add a generic age interface to make this happen).

[nalimilan]

OK, thanks. Then we should only list types from Base which guarantee this, and say nothing about AbstractDic in general.

Suggested change

	When `collection` is a `Dict`, `f` is allowed to add or remove keys and/or change values.
	When `collection` is a `Dict`, `IdDict` or `WeakKeyDict`, `f` is allowed to add or remove
	keys and/or change values.