Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
"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 thatf
is guaranteed to be called only once (which is currently a private detail of the implementation, though, the behavior is observable through fuzzing).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in the current docstring "store" refers to what the
get!
implementation does, not to whatf
does or is allowed to do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Dict
,IdDict
andWeakKeyDict
indicates that their implementations make mutatingf
safe. In particular,WeakKeyDict
handles the potential reentrancy with aReentrantLock
, which is neat.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withgettoken!
, callf
, and then callsettokenvalue!
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 aDictionaries.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?).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks. Then we should only list types from Base which guarantee this, and say nothing about
AbstractDic
in general.