comprehensions segfault if length and done are inconsistent

[simonster]

Clearly this is a pathological case, but I'm not sure it's acceptable to segfault, since the @inbounds is implicit in the comprehension and not part of the code I wrote.

type A; end
Base.length(::A) = 1
Base.start(::A) = 1
Base.next(::A, i) = (1, 1)
Base.done(::A, i) = false
[x for x in A()]

I discovered this because of #7426, but it's a different code path.

[stevengj]

Can't you segfault basically any function in Base that uses @inbounds if you create a type with an incorrect length?

There is a general danger here that arises from using @inbounds in functions whose arguments are abstract array or iterator types.

[simonster]

I don't think this issue should be all that common outside of functions that use comprehensions. Most of the functions that operate on AbstractArrays using @inbounds that I see do indexing and not iteration. If getindex and length are inconsistent, this could result in incorrect output or cause getindex to throw an error, but I don't think it should cause a segfault, except that I think @inbounds can presently affect inlined functions. This problem specifically relates to the code pattern:

y = Array(T, length(a))
i = 1
for x in a
    @inbounds y[i] = f(x)
    i += 1
end

This is used because indexing via a[i] may incur an unnecessary bounds check, and this bounds check can be costly if it prevents inlining. However, we are now in danger of segfaulting when assigning to y.

[stevengj]

It's pretty easy to make any @inbounds function crash with a buggy AbstractArray subtype. e.g.

import Base: size, getindex
type CrashVec{T} <: AbstractVector{T}
    a::Vector{T}
end
size(a::CrashVec) = size(a.a)
getindex(a::CrashVec, i::Integer) = a.a[i+10^23]
count(iseven, CrashVec([1]))

[simonster]

I think that crash is due to the aforementioned issue of @inbounds affecting inlined functions, which seems different. It would also be good to fix, although I'm not sure if it's currently possible to put @inbounds within a function in a way that still allows it to be inlined. With getindex(a::CrashVec, i::Integer) = true ? a.a[i+10^23] : a.a[i+10^23], which isn't inlined, I get a BoundsError instead.

[JeffBezanson]

Comprehensions can be fixed to apply @inbounds more narrowly. Really inbounds should only be used for monomorphic code, where you know there really is no possibility of a bounds error.

[JeffBezanson]

Rather, I should say comprehensions could iterate based on the length instead of the range's done method. This might actually be faster since we can avoid calling done altogether. Of course this just moves the problem elsewhere, since it still assumes length and done are consistent. But at least it avoids writing outside the Array; problems will be confined to the inconsistently-defined type.