Compatibility with new iteration protocol

[martinholters]

This makes DataStructures functional in face of the new iteration protocol and resolves all the occurring ambiguities.

[codecov]

Codecov Report

Merging #382 into master will decrease coverage by 0.08%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #382      +/-   ##
==========================================
- Coverage   96.58%   96.49%   -0.09%     
==========================================
  Files          31       31              
  Lines        2312     2314       +2     
==========================================
  Hits         2233     2233              
- Misses         79       81       +2

Impacted Files	Coverage Δ
src/accumulator.jl	100% <ø> (+2.5%)	⬆️
src/classified_collections.jl	85.71% <ø> (ø)	⬆️
src/default_dict.jl	97.72% <ø> (ø)	⬆️
src/multi_dict.jl	98.3% <ø> (ø)	⬆️
src/trie.jl	93.22% <ø> (ø)	⬆️
src/priorityqueue.jl	97.43% <0%> (-1.7%)	⬇️
src/circ_deque.jl	100% <100%> (ø)	⬆️
src/queue.jl	100% <100%> (ø)	⬆️
src/int_set.jl	100% <100%> (ø)	⬆️
src/deque.jl	100% <100%> (ø)	⬆️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 691caea...5ea4fcc. Read the comment docs.

[StephenVavasis]

Could someone post a link to the "new iteration protocol"? I don't know about this, and I'd like to check that the sorted-container iterators are ok.

[martinholters]

https://docs.julialang.org/en/latest/manual/interfaces/#man-interface-iteration-1

But there is a relatively good compatibility layer; the only downside to it is that it defines a next(itr::I, state::LegacyIterationCompat{I,T,S}) where {I,T,S} (and similar for done) that is ambiguous to many of the methods as they were defined here. Things still work (the ambiguity isn't hit), but the detect_ambiguities test fails. I've resolved those by either putting a type on the state parameter or introducing an ambiguity-resolving method were the state type is defined by Base (and might vary between before and after the iteration protocol change). This makes up 95% of this PR.

The only thing not due to ambiguity resolution (and what got me working on this) is 5ea4fcc:

-done(t::OrderedDict, i::Int) = done(t.keys, i)
+done(t::OrderedDict, i::Int) = i > length(t.keys)

Note that t.keys here is an array. It works on 0.6:

julia> done([1,2,3], 3)
false

julia> done([1,2,3], 4)
true

But fails on current master:

julia> done([1,2,3], 3)
ERROR: MethodError: no method matching done(::Array{Int64,1}, ::Int64)

because the expected state is not an Int any more:

julia> start([1,2,3])
Base.LegacyIterationCompat{Array{Int64,1},Int64,Int64}(false, 1, 2)

But actually, using done(t.keys,i) to determine whether i is still inbounds for t.keys[i] looks like dangerously relying on the internals of Array iteration anyway.

[martinholters]

That said, it might be a good idea to start using the new iteration protocol directly here. I just wanted to make it working and pass its tests quickly.

[StephenVavasis]

Thanks! So if I put the new iteration protocol in code and precede it with @compat, will this also work for 0.6.2? I'm wondering how @compat handles the case when nothing is returned. Or is it better to have a branch in the code based on VERSION.

[martinholters]

There is no Compat support at the moment and I'd imagine it to be quite complicated to do, so I wouldn't expect it anytime soon (if at all).

[StephenVavasis]

Thanks! Can you post the appropriate if VERSION >= statement to distinguish between usage of the old and new iteration protocols?
I got this question answered here: JuliaLang/julia#18823

[martinholters]

An alternative to the VERSION check is isdefined(Base, :iterate).

Meanwhile, can you give this a go? Or at least 5ea4fcc, which I could separate out if preferred, so that OrderedDict can be iterated on Julia master?

[StephenVavasis]

I had not previously looked at the code for OrderDict.jl, and now that I see it for the first time, I am concerned about its ongoing maintainability. It appears that someone started from the code for Dict and made modifications in various places. The problem with this is that Dict is very complicated code and uses low-level Julia interfaces. In fact, two bugs have been found in Dict just in the past year. So, really, the only people qualified to maintain this code for OrderedDict would be the maintainers of Dict, but I'm not sure if they are interested.

A maintainable alternative would be to reimplement OrderedDict from scratch using existing Base data structures. For example, OrderedDict{K,V} could be implemented via: d1, which is a Dict from K to Tuple{V,Int} and a1, a Vector of Tuple{K,Bool}. On the ith insertion, say k=>v, one pushes k=>(v,i) into d1 and sets a1[i]=(k,true). To delete k=>v, one pops k=>(v,i) from d and sets a1[i]=(k,false). Finally, to enumerate entries in order, loop over i and return the k=>v for each i such that a1[i][2] holds. (Clearly, this would be slower than the existing implementation due to overhead in maintaining a1.)

[martinholters]

I agree with your assessment of the OrderedDict code. Using instead of copying the Dict code from base seems a much saner approach. However, such a complete rewrite will require some thought. E.g. in the idea you've sketched, repeated insertion and deletion will make a1 grow without bounds, even while keeping the stored elements bounded. That sounds like a property to be avoided.

While this may not be the right place to discuss what a possible future OrderedDict implementation might look like, do you think it will materialize in time for Julia 0.7?

[ExpandingMan]

It would be really great if this could get merged, currently this package is basically unusable on 0.7 due to this issue.

[StephenVavasis]

In #386 I have replaced OrderedDict with a completely new implementation (described in my comment earlier in this thread). This should be mergeable with 0.7. This new implementation is less performant than the old implementation, so if this PR is accepted, I will open issues on github on recovering some of the lost performance (to be done later).

[kmsquire]

Tested locally. LGTM.