This looks good to me.
There's only one scenario where the only benefit of the current method is fewer allocations at the expense of a 2x slow-down in speed. That sounds like a good tradeoff to me.

@mbauman @timholy want to also sign off / review ?

This is rather surprising to me — I suppose it's entirely due to the ability to sum logical arrays and @inbounds over a pre-determined size array. I appreciate that this is better performance in many cases, but I dislike how this is harder to opt-out of than it is to opt-into. I suppose, though, that — by definition — you're doing vectorized allocat-y Julia by using findall... and if this were truly chasing peak allocation-free performance, you'd do the iterative find loop.

So with that rationale, I'll give this a ✅. I'll give it a bit more time, but barring further comments let's merge it tomorrow.

Thanks!

This change broke UniqueVectors.jl. My current plan is to issue a new release to fix it, but I am wondering: could there be other fallout throughout the package ecosystem? Seems somewhat unlikely, but perhaps worth considering/checking.

Usually before releasing a new version all package tests are run against it to detect any problems in advance, so I guess we'll find out at that point.

Would we avoid the ambiguity if it's findall(testf::Function, A::Union{AbstractArray, Tuple})? Broadcasting for tuples is likely faster too.

While we try to avoid ambiguity changes like this, it's really hard to avoid and is easy to fix.

Would we avoid the ambiguity if it's findall(testf::Function, A::Union{AbstractArray, Tuple})?

Do you mean if we were to replace the signature findall(testf::Function, A::AbstractArray) with findall(testf::Function, A::Union{AbstractArray, Tuple})? The method signature in UniqueVectors that is now ambiguous (following the change in this pull request) is findall(p::Base.Fix2{typeof(in),<:AbstractUniqueVector}, a::Union{Tuple, AbstractArray}). I believe that even if the change you are suggesting is made, the UniqueVector method would still be ambiguous with findall(pred::Fix2{typeof(in)}, x::AbstractArray) and findall(pred::Fix2{typeof(in)}, x::Tuple).

An alternative approach (and one which you might actually be implying) is to replace the three signatures of findall introduced in this PR with a single signature: the one you mentioned, findall(testf::Function, A::Union{AbstractArray, Tuple}). Then this method could e.g. call an inner _findall method, and the dispatching could be performed one level deeper. I think this would result in unambiguous methods, but haven't considered it carefully or tested it.

While we try to avoid ambiguity changes like this, it's really hard to avoid and is easy to fix.

Do you mean to fix it by adjusting julia before the release or to fix it in packages? I am on board either way. If there is no other fallout in the package ecosystem, then the above solution is almost certainly overkill -- the UniqueVectors package is probably the right place to fix it.

Yes, I meant the latter suggestion on both counts. Sorry for the brevity!