Fix ambiguities in any/all

[ararslan]

Note that these annotations used to be Base.Predicate, but they were removed when Predicate was removed from Base. Their removal is causing ambiguity warnings. This PR adds an annotation of Union{Function, DataType}, which should fix things.

cc @tkelman, @MikeInnes

[ararslan]

Bump

[MikeInnes]

Good to have the fix, but it shouldn't be necessary on 0.6 as things were cleaned up after predicate removal. Perhaps what makes sense is to revert the original patch conditionally on 0.4/5 while having the untyped version on 0.6.

Would be good to have a test case showing what was previously broken and now works.

[ararslan]

I agree a test would be great here, but I'm not sure the best way to go about it. Maybe @tkelman would know?

As it stands, this change should be functionally equivalent to the Predicate version on 0.4/0.5 and shouldn't make a difference on 0.6—what else could you pass to any/all that would make sense that isn't a function or type constructor? The annotation could always be removed anyway once 0.4/0.5 support is dropped. Though I'll gladly go with whatever you think is best.

[tkelman]

You could try loading the package in a separate process and check for ambiguity warnings on 0.4, or call an ambiguous method elsewhere. Though if you do the former, best to enable appveyor first since it's easy to write redirect code that freezes on windows.

[MikeInnes]

I think just calling one of the previously-ambiguous methods would be fine.

[tkelman]

Bump @ararslan

[ararslan]

Thanks for the reminder @tkelman, this slipped off my radar again. I'll add a test that calls one of the previously ambiguous methods.

[ararslan]

Things are now 100% ambiguity-free.

[MikeInnes]

Glad this is working. However, it doesn't look as though something like any(r"f.*o", ["foo", "bar"]) would currently dispatch correctly.

[ararslan]

I'm not sure what you mean. What would you expect that to do? It doesn't look like it's relevant to this PR, as this restricts the signature for lists to functions or data types, so unless there's a Base method for that, it wouldn't work anyway.

[MikeInnes]

On all Julia versions (0.4+) any(r"f.*o", ["foo", "bar"]) returns true. It's relevant to this PR because any(::Regex, ::List) should dispatch to the same method as any(::Function, ::List) in all cases, but currently it falls back to the generic implementation due to the way the types are set up in the PR.

[ararslan]

Oh, I see what you mean. IMO that's kind of bizarre that it works but since it does we shouldn't break it here. I'm open to any suggestions you may have to address that in this PR.

[tkelman]

Is a Regex a Predicate? How would that have worked before https://github.com/MikeInnes/Lazy.jl/pull/58/files?

[ararslan]

Assuming I understand the Predicate code that used to be in Base, functions were converted to Predicates, then the ::Predicate method was called. For some reason, Regex objects are callable (e.g. (r"f.*o")("foo") works) and return Bools, so the conversion to Predicate I guess worked? Could be misunderstanding the old logic though.

[ararslan]

Looks like this has never dispatched how you expect it to:

julia> Pkg.pin("Lazy", v"0.11.4") # prior to my previous PR
INFO: Creating Lazy branch pinned.ad69f5ac.tmp
INFO: No packages to install, update or remove

julia> using Lazy
INFO: Precompiling module Lazy.

julia> any(r"f.*o", ["foo", "bar"])
true

julia> @which any(r"f.*o", ["foo", "bar"])
any(f, itr) at reduce.jl:361

(Julia 0.4)

[ararslan]

Also with Lazy v0.11.4 on Julia 0.4:

julia> any(x -> x == 1, list(1, 2, 3))
ERROR: MethodError: `convert` has no method matching convert(::Type{Lazy.LinkedList}, ::Lazy.EmptyList)
This may have arisen from a call to the constructor Lazy.LinkedList(...),
since type constructors fall back to convert methods.
Closest candidates are:
  Lazy.LinkedList(::Any, ::Lazy.List)
  Lazy.LinkedList(::Any, ::Any)
  call{T}(::Type{T}, ::Any)
  ...
 in map at abstractarray.jl:1168
 in any at reduce.jl:361

[MikeInnes]

Ok so here's an example of the correct behaviour on both versions (with no Lazy.jl involved):

julia> type Foo end
julia> Base.any(f::Base.Predicate, ::Foo) = (@show(f); true)
julia> any(x -> x == 1, Foo())
f = Base.Predicate{##1#2}(#1)
true
julia> any(r"foo", Foo())
f = Base.Predicate{Regex}(r"foo")
true

julia> type Foo end
julia> any(f, ::Foo) = (@show(f); true)
any (generic function with 1 method)
julia> any(x -> x == 1, Foo())
f = #1
true
julia> any(r"foo", Foo())
f = r"foo"
true

[ararslan]

I made a change to allow this, but it's failing on 0.4 because, as I noted above, calling any or all on a Lazy.List has apparently never worked on 0.4 with the Predicate signature. At this point I have no idea why. 😕

[ararslan]

Come to think of it, unless there's really significant improvements in terms of performance or whatever when overloading any/all for Lists, why not just avoid a local definition entirely and let it fall back to the default in Base? That should work at least on 0.6, since any/all are just simple short-circuiting loops over the given iterator.

[MikeInnes]

The impact on memory usage would be significant; using lazy lists as iterates holds on to the head of the list which uses linear memory as you iterate, whereas the recursive version is able to use constant memory.

I don't mind just dropping support for 0.4 – or even 0.5 – at this stage.

[tkelman]

Please just get a release tagged for 0.4 that doesn't have ambiguity warnings.

[ararslan]

I'll go with whatever approach @MikeInnes thinks is best here.

[ararslan]

I kept the test for ambiguities but changed the tests for behavior to @pending, since the behavior seems not to have ever worked. This got the package tests working again and seems a decent stopgap until the behavior can be made to work, as it puts us in the same situation as before my previous PR, just with no ambiguities.

[MikeInnes]

LGTM, thanks.

[ararslan]

Thank you! Would you mind tagging a new version now to avoid the 0.4 ambiguity warnings?

[tkelman]

Bump, would greatly appreciate a tag, this is taking a lot longer than it needs to.

[ViralBShah]

Also, perhaps we can pick an org to have the project live in?

[ararslan]

Bump

[ViralBShah]

I thought this got done. @MikeInnes Can we move this package to an appropriate org?

[MikeInnes]

Sure. JuliaCollections? I don't have permissions but if someone adds me I can transfer.

[ararslan]

Sorry, I missed the release of v0.11.6. Thanks Mike! I'll add you to JuliaCollections.