Use more generic approach for findfirst and findlast

Also add tests, and remove @inferred calls which did not actually check
whether functions were inferred, but only whether the equality test was.
Change the _pairs() fallback to use Iterators.countfrom() so that an error
is raised by findlast() rather than assuming the last index is typemax(Int),
and use length() for iterators which support it so that findlast() works.

Author: nalimilan

base/array.jl

@@ -1492,6 +1492,11 @@ cat(n::Integer, x::Integer...) = reshape([x...], (ntuple(x->1, n-1)..., length(x
 
 ## find ##
 
+_pairs(A::Union{AbstractArray, AbstractDict, AbstractString, Tuple, NamedTuple}) = pairs(A)
+_pairs(iter) = _pairs(IteratorSize(iter), iter)
+_pairs(::Union{HasLength, HasShape}, iter) = zip(1:length(iter), iter)
+_pairs(::Union{SizeUnknown, IsInfinite}, iter) = zip(Iterators.countfrom(1), iter)
+
 """
     findnext(A, i::Integer)
 
@@ -1545,12 +1550,14 @@ end
 """
     findfirst(A)
 
-Return the index of the first `true` value in `A`.
+Return the index or key of the first `true` value in `A`.
 Return `nothing` if no such value is found.
 To search for other kinds of values, pass a predicate as the first argument.
 
-Indices are of the same type as those returned by [`keys(A)`](@ref)
-and [`pairs(A)`](@ref).
+Indices or keys are of the same type as those returned by [`keys(A)`](@ref)
+and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString`
+`Tuple` and `NamedTuple` objects, and are linear indices starting at `1`
+for other iterables.
 
 # Examples
 ```jldoctest
@@ -1576,7 +1583,22 @@ julia> findfirst(A)
 CartesianIndex(2, 1)
 ```
 """
-findfirst(A) = isempty(A) ? nothing : findnext(A, first(keys(A)))
+function findfirst(A)
+    warned = false
+    for (i, a) in _pairs(A)
+        if !warned && !(a isa Bool)
+            depwarn("In the future `findfirst` will only work on boolean collections. Use `findfirst(x->x!=0, A)` instead.", :findfirst)
+            warned = true
+        end
+        if a != 0
+            return i
+        end
+    end
+    return nothing
+end
+
+# Needed for bootstrap, and allows defining only an optimized findnext method
+findfirst(A::Union{AbstractArray, AbstractString}) = findnext(A, first(keys(A)))
 
 """
     findnext(predicate::Function, A, i)
@@ -1626,11 +1648,13 @@ end
 """
     findfirst(predicate::Function, A)
 
-Return the index of the first element of `A` for which `predicate` returns `true`.
+Return the index or key of the first element of `A` for which `predicate` returns `true`.
 Return `nothing` if there is no such element.
 
-Indices are of the same type as those returned by [`keys(A)`](@ref)
-and [`pairs(A)`](@ref).
+Indices or keys are of the same type as those returned by [`keys(A)`](@ref)
+and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString`
+`Tuple` and `NamedTuple` objects, and are linear indices starting at `1`
+for other iterables.
 
 # Examples
 ```jldoctest
@@ -1659,7 +1683,16 @@ julia> findfirst(iseven, A)
 CartesianIndex(2, 1)
 ```
 """
-findfirst(testf::Function, A) = isempty(A) ? nothing : findnext(testf, A, first(keys(A)))
+function findfirst(testf::Function, A)
+    for (i, a) in _pairs(A)
+        testf(a) && return i
+    end
+    return nothing
+end
+
+# Needed for bootstrap, and allows defining only an optimized findnext method
+findfirst(testf::Function, A::Union{AbstractArray, AbstractString}) =
+    findnext(testf, A, first(keys(A)))
 
 """
     findprev(A, i)
@@ -1711,11 +1744,13 @@ end
 """
     findlast(A)
 
-Return the index of the last `true` value in `A`.
+Return the index or key of the last `true` value in `A`.
 Return `nothing` if there is no `true` value in `A`.
 
-Indices are of the same type as those returned by [`keys(A)`](@ref)
-and [`pairs(A)`](@ref).
+Indices or keys are of the same type as those returned by [`keys(A)`](@ref)
+and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString`
+`Tuple` and `NamedTuple` objects, and are linear indices starting at `1`
+for other iterables.
 
 # Examples
 ```jldoctest
@@ -1743,7 +1778,22 @@ julia> findlast(A)
 CartesianIndex(2, 1)
 ```
 """
-findlast(A) = isempty(A) ? nothing : findprev(A, last(keys(A)))
+function findlast(A)
+    warned = false
+    for (i, a) in Iterators.reverse(_pairs(A))
+        if !warned && !(a isa Bool)
+            depwarn("In the future `findlast` will only work on boolean collections. Use `findlast(x->x!=0, A)` instead.", :findlast)
+            warned = true
+        end
+        if a != 0
+            return i
+        end
+    end
+    return nothing
+end
+
+# Needed for bootstrap, and allows defining only an optimized findprev method
+findlast(A::Union{AbstractArray, AbstractString}) = findprev(A, last(keys(A)))
 
 """
     findprev(predicate::Function, A, i)
@@ -1790,11 +1840,13 @@ end
 """
     findlast(predicate::Function, A)
 
-Return the index of the last element of `A` for which `predicate` returns `true`.
+Return the index or key of the last element of `A` for which `predicate` returns `true`.
 Return `nothing` if there is no such element.
 
-Indices are of the same type as those returned by [`keys(A)`](@ref)
-and [`pairs(A)`](@ref).
+Indices or keys are of the same type as those returned by [`keys(A)`](@ref)
+and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString`
+`Tuple` and `NamedTuple` objects, and are linear indices starting at `1`
+for other iterables.
 
 # Examples
 ```jldoctest
@@ -1820,16 +1872,27 @@ julia> findlast(isodd, A)
 CartesianIndex(2, 1)
 ```
 """
-findlast(testf::Function, A) = isempty(A) ? nothing : findprev(testf, A, last(keys(A)))
+function findlast(testf::Function, A)
+    for (i, a) in Iterators.reverse(_pairs(A))
+        testf(a) && return i
+    end
+    return nothing
+end
+
+# Needed for bootstrap, and allows defining only an optimized findprev method
+findlast(testf::Function, A::Union{AbstractArray, AbstractString}) =
+    findprev(testf, A, last(keys(A)))
 
 """
     findall(f::Function, A)
 
 Return a vector `I` of the indices or keys of `A` where `f(A[I])` returns `true`.
 If there are no such elements of `A`, return an empty array.
 
-Indices are of the same type as those returned by [`keys(A)`](@ref)
-and [`pairs(A)`](@ref).
+Indices or keys are of the same type as those returned by [`keys(A)`](@ref)
+and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString`
+`Tuple` and `NamedTuple` objects, and are linear indices starting at `1`
+for other iterables.
 
 # Examples
 ```jldoctest
@@ -1875,9 +1938,6 @@ julia> findall(x -> x >= 0, d)
 """
 findall(testf::Function, A) = collect(first(p) for p in _pairs(A) if testf(last(p)))
 
-_pairs(A::Union{AbstractArray, AbstractDict, AbstractString, Tuple, NamedTuple}) = pairs(A)
-_pairs(iter) = zip(OneTo(typemax(Int)), iter)  # safe for objects that don't implement length
-
 """
     findall(A)
 

test/arrayops.jl

@@ -460,18 +460,49 @@ end
     @test findnext(equalto(0x00), [0x00, 0x01, 0x00], 2) == 3
     @test findprev(equalto(0x00), [0x00, 0x01, 0x00], 2) == 1
 end
-@testset "findall with Matrix" begin
+@testset "find with Matrix" begin
     A = [1 2 0; 3 4 0]
     @test findall(isodd, A) == [CartesianIndex(1, 1), CartesianIndex(2, 1)]
     @test findall(!iszero, A) == [CartesianIndex(1, 1), CartesianIndex(2, 1),
                                CartesianIndex(1, 2), CartesianIndex(2, 2)]
-end
-@testset "findall with general iterables" begin
+    @test findfirst(isodd, A) == CartesianIndex(1, 1)
+    @test findlast(isodd, A) == CartesianIndex(2, 1)
+    @test findnext(isodd, A, CartesianIndex(1, 1)) == CartesianIndex(1, 1)
+    @test findprev(isodd, A, CartesianIndex(2, 1)) == CartesianIndex(2, 1)
+    @test findnext(isodd, A, CartesianIndex(1, 2)) === nothing
+    @test findprev(iseven, A, CartesianIndex(2, 1)) === nothing
+end
+@testset "find with general iterables" begin
     s = "julia"
     @test findall(c -> c == 'l', s) == [3]
+    @test findfirst(c -> c == 'l', s) == 3
+    @test findlast(c -> c == 'l', s) == 3
+    @test findnext(c -> c == 'l', s, 1) == 3
+    @test findprev(c -> c == 'l', s, 5) == 3
+    @test findnext(c -> c == 'l', s, 4) === nothing
+    @test findprev(c -> c == 'l', s, 2) === nothing
+
     g = Base.Unicode.graphemes("日本語")
-    @test findall(isascii, g) == Int[]
-    @test findall(!iszero, (i % 2 for i in 1:10)) == 1:2:9
+    @test findall(!isempty, g) == 1:3
+    @test isempty(findall(isascii, g))
+    @test findfirst(!isempty, g) == 1
+    @test findfirst(isascii, g) === nothing
+    # Check that the last index isn't assumed to be typemax(Int)
+    @test_throws MethodError findlast(!iszero, g)
+
+    g2 = (i % 2 for i in 1:10)
+    @test findall(!iszero, g2) == 1:2:9
+    @test findfirst(!iszero, g2) == 1
+    @test findlast(!iszero, g2) == 9
+    @test findfirst(equalto(2), g2) === nothing
+    @test findlast(equalto(2), g2) === nothing
+
+    g3 = (i % 2 for i in 1:10, j in 1:2)
+    @test findall(!iszero, g3) == 1:2:19
+    @test findfirst(!iszero, g3) == 1
+    @test findlast(!iszero, g3) == 19
+    @test findfirst(equalto(2), g3) === nothing
+    @test findlast(equalto(2), g3) === nothing
 end
 
 @testset "findmin findmax indmin indmax" begin

test/dict.jl

@@ -760,9 +760,14 @@ end
     @test map(string, keys(d)) == Set(["1","3"])
 end
 
-@testset "findall" begin
-    @test @inferred findall(equalto(1), Dict(:a=>1, :b=>2)) == [:a]
-    @test @inferred sort(findall(equalto(1), Dict(:a=>1, :b=>1))) == [:a, :b]
-    @test @inferred isempty(findall(equalto(1), Dict()))
-    @test @inferred isempty(findall(equalto(1), Dict(:a=>2, :b=>3)))
+@testset "find" begin
+    @test findall(equalto(1), Dict(:a=>1, :b=>2)) == [:a]
+    @test sort(findall(equalto(1), Dict(:a=>1, :b=>1))) == [:a, :b]
+    @test isempty(findall(equalto(1), Dict()))
+    @test isempty(findall(equalto(1), Dict(:a=>2, :b=>3)))
+
+    @test findfirst(equalto(1), Dict(:a=>1, :b=>2)) == :a
+    @test findfirst(equalto(1), Dict(:a=>1, :b=>1, :c=>3)) in (:a, :b)
+    @test findfirst(equalto(1), Dict()) === nothing
+    @test findfirst(equalto(1), Dict(:a=>2, :b=>3)) === nothing
 end

test/namedtuple.jl

@@ -209,7 +209,15 @@ abstr_nt_22194_3()
 @test typeof(Base.structdiff(NamedTuple{(:a, :b), Tuple{Int32, Union{Int32, Nothing}}}((1, Int32(2))),
                              (a=0,))) === NamedTuple{(:b,), Tuple{Union{Int32, Nothing}}}
 
-@test @inferred findall(equalto(1), (a=1, b=2)) == [:a]
-@test @inferred findall(equalto(1), (a=1, b=1)) == [:a, :b]
-@test @inferred isempty(findall(equalto(1), NamedTuple()))
-@test @inferred isempty(findall(equalto(1), (a=2, b=3)))
+@test findall(equalto(1), (a=1, b=2)) == [:a]
+@test findall(equalto(1), (a=1, b=1)) == [:a, :b]
+@test isempty(findall(equalto(1), NamedTuple()))
+@test isempty(findall(equalto(1), (a=2, b=3)))
+@test findfirst(equalto(1), (a=1, b=2)) == :a
+@test findlast(equalto(1), (a=1, b=2)) == :a
+@test findfirst(equalto(1), (a=1, b=1)) == :a
+@test findlast(equalto(1), (a=1, b=1)) == :b
+@test findfirst(equalto(1), ()) === nothing
+@test findlast(equalto(1), ()) === nothing
+@test findfirst(equalto(1), (a=2, b=3)) === nothing
+@test findlast(equalto(1), (a=2, b=3)) === nothing

test/tuple.jl

@@ -365,9 +365,25 @@ end
     @test eltype(Tuple{Vararg{T}} where T<:Integer) >: Integer
 end
 
-@testset "findall" begin
-    @test @inferred findall(equalto(1), (1, 2)) == [1]
-    @test @inferred findall(equalto(1), (1, 1)) == [1, 2]
-    @test @inferred isempty(findall(equalto(1), ()))
-    @test @inferred isempty(findall(equalto(1), (2, 3)))
+@testset "find" begin
+    @test findall(equalto(1), (1, 2)) == [1]
+    @test findall(equalto(1), (1, 1)) == [1, 2]
+    @test isempty(findall(equalto(1), ()))
+    @test isempty(findall(equalto(1), (2, 3)))
+
+    @test findfirst(equalto(1), (1, 2)) == 1
+    @test findlast(equalto(1), (1, 2)) == 1
+    @test findfirst(equalto(1), (1, 1)) == 1
+    @test findlast(equalto(1), (1, 1)) == 2
+    @test findfirst(equalto(1), ()) === nothing
+    @test findlast(equalto(1), ()) === nothing
+    @test findfirst(equalto(1), (2, 3)) === nothing
+    @test findlast(equalto(1), (2, 3)) === nothing
+
+    @test findnext(equalto(1), (1, 2), 1) == 1
+    @test findprev(equalto(1), (1, 2), 2) == 1
+    @test findnext(equalto(1), (1, 1), 2) == 2
+    @test findprev(equalto(1), (1, 1), 1) == 1
+    @test findnext(equalto(1), (2, 3), 1) === nothing
+    @test findprev(equalto(1), (2, 3), 2) === nothing
 end