Throw an error when collecting from iterators with inconsistent length #29458

nalimilan · 2018-10-01T19:54:20Z

Check that HasLength and HasShape iterators return the same number of elements as indicated by length(itr). An error was already thrown in some situations when the number of elements was higher than the declared length, but not all.

Fixes JuliaData/DelimitedFiles.jl#3.

I'd appreciate if somebody with Scheme skills could indicate how to change the lowering of T[x for x in itr] here to throw an error if idx != length(itr) when !szunk in construct-loops. That would fix the two @test_broken cases.

Check that HasLength and HasShape iterators return the same number of elements as indicated by length(itr). An error was already thrown in some situations when the number of elements was higher than the declared length, but not all.

JeffBezanson · 2018-10-01T20:03:47Z

base/array.jl

+    len = length(dest)
+    i = 0
+    for x in src
+        i == len &&


It looks like copyto! already does this check.

Yes but it doesn't do the one below, and it cannot be done with access to i.

Ok, then we should add a _copyto_impl! that takes a strict::Bool argument, using the current code from copyto!. Then it can be shared by both this code (passing true for strict) and copyto! (passing false).

I can do that. Though note that the error message will be less specific (like the current one in copyto!), since copyto! cannot assume that the problem comes from an invalid declared length. So I'm not sure it's really better.

Ok, either way. This does need to use eachindex though.

Not if we keep this method, as it only applies to Array.

Actually it can also be called on OffsetArray and possibly on other arrays in corner cases. So I've taken the _copy_impl! approach.

JeffBezanson · 2018-10-01T20:08:35Z

base/array.jl

@@ -667,6 +687,10 @@ function collect_to!(dest::AbstractArray{T}, itr, offs, st) where T
            return collect_to!(new, itr, i+1, st)
        end
    end
+    i-1 < length(dest) &&


This should use lastindex.

JeffBezanson · 2018-10-01T20:12:09Z

test/arrayops.jl

+    @test_throws ErrorException collect(InvalidIter2())
+    @test_throws ErrorException collect(Any, InvalidIter2())
+    @test_throws ErrorException collect(Int, InvalidIter2())
+    @test_throws ErrorException [x for x in InvalidIter2()]


I don't think we can test this, since it will do an invalid memory write.

Mmm, yeah, so the only way to catch this would be to drop the @inbounds in collect_to!, or put a i == len check as in copyto!/copyto_check_length!. Not sure that's worth it, but it sounds inconsistent to have it in one place and not the other.

Better to add checks for some cases (when it can be efficient) than to have no checks at all, I'd say.

But are there reasons to care less about performance in copyto! than in collect_to!?

collect_to! is used by comprehensions.

Hmm, does that mean it's OK for copyto! to be slow? :-/

No, it doesn't mean it's OK for copyto! to be slow, it just means we don't want regressions.

That was actually an honest question: maybe we should remove the check from copyto! to make it faster.

nalimilan · 2018-10-02T19:54:43Z

@JeffBezanson Could you suggest how to apply the same change to the parser (cf. issue description)?

martinholters · 2018-10-05T11:44:46Z

base/array.jl

@@ -667,6 +687,10 @@ function collect_to!(dest::AbstractArray{T}, itr, offs, st) where T
            return collect_to!(new, itr, i+1, st)
        end
    end
+    i-1 < length(dest) &&
+        throw(ErrorException("iterator returned fewer elements than its declared length"))
+    i-1 > length(dest) &&


In this case, the @inbounds dest[i] above has likely corrupted something or segfaulted. Should probably replace the while true with while i <= lastindex(dest) and verify here that iterate(itr, st) === nothing (or Base.isdone(itr, st) === true || iterate(itr, st) === nothing ?). Should then fix #29532.

In theory I agree, I don't like this @inbounds call which isn't associated with an actual check of the index against the array. But it comes with an additional cost with hurts performance at lot since it disables SIMD (see discussion above. I've just tried it:

# After change julia> x = Vector{Int}(undef, 1000); julia> itr = 1:1000; julia> @btime Base.collect_to!($x, $itr, 1, 0); 676.981 ns (0 allocations: 0 bytes) julia> y = copy(x); julia> @btime Base.collect_to!($x, $y, 1, 1); 847.618 ns (0 allocations: 0 bytes) # Master julia> x = Vector{Int}(undef, 1000); julia> itr = 1:1000; julia> @btime Base.collect_to!($x, $itr, 1, 0); 86.233 ns (0 allocations: 0 bytes) julia> y = copy(x); julia> @btime Base.collect_to!($x, $y, 1, 1); 90.980 ns (0 allocations: 0 bytes)

Maybe the compiler could be improved, but for now I guess we can't do this.

Ouch, that's bad.

nalimilan · 2018-10-05T13:40:28Z

I've pushed a commit applying @JeffBezanson's suggestions. I think this could be merged as-is, and improved later to cover more cases.

nalimilan · 2018-10-06T20:17:45Z

base/array.jl

@@ -667,6 +677,11 @@ function collect_to!(dest::AbstractArray{T}, itr, offs, st) where T
            return collect_to!(new, itr, i+1, st)
        end
    end
+    lastidx = lastindex(dest)


This should actually be last(LinearIndices(dest)). Will fix in the next round of reviews.

nalimilan · 2018-10-06T20:30:53Z

base/abstractarray.jl

@@ -720,8 +725,12 @@ end
 ## copy between abstract arrays - generally more efficient
 ## since a single index variable can be used.

-copyto!(dest::AbstractArray, src::AbstractArray) =
+function _copyto_impl!(dest::AbstractArray, src::AbstractArray, allowshorter::Bool)
+    if !allowshorter && length(src) < length(dest)


This check and the one below is actually not needed for this PR since we already allocate an array of the correct length, and the AbstractArray interface relies on the fact that the length is correctly declared. But I added these for consistency, so that the allowshorter argument (which needs to be accepted) isn't a no-op. I can drop it and add a comment explaining that if you want.

nalimilan · 2018-11-28T08:34:43Z

@JeffBezanson Are you happy with what the PR implements now? It doesn't fix all cases, but at least it catches more errors than before.

nalimilan · 2022-03-02T08:18:02Z

@JeffBezanson Do you think this is worth reviving?

nalimilan mentioned this pull request Oct 1, 2018

Custom iterator breaks on last row of a readdlm-loaded array JuliaData/DelimitedFiles.jl#3

Open

JeffBezanson reviewed Oct 1, 2018

View reviewed changes

martinholters reviewed Oct 5, 2018

View reviewed changes

martinholters mentioned this pull request Oct 5, 2018

Segmentation fault with typo in iterate(x, n) #29532

Closed

Review fixes

e811fa6

Use _copy_impl! instead of custom function

c7b6896

nalimilan force-pushed the nl/length branch from fd91297 to c7b6896 Compare October 6, 2018 10:54

nalimilan commented Oct 6, 2018

View reviewed changes

nalimilan mentioned this pull request Oct 9, 2018

SIMD not enabled when iterating over two Arrays with equal sizes #29571

Open

KristofferC mentioned this pull request May 13, 2019

Segmentation fault when iterating over markdown #32013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw an error when collecting from iterators with inconsistent length #29458

Throw an error when collecting from iterators with inconsistent length #29458

nalimilan commented Oct 1, 2018 •

edited

Loading

JeffBezanson Oct 1, 2018

nalimilan Oct 1, 2018

JeffBezanson Oct 1, 2018

nalimilan Oct 1, 2018

JeffBezanson Oct 1, 2018

nalimilan Oct 2, 2018

nalimilan Oct 5, 2018

JeffBezanson Oct 1, 2018

JeffBezanson Oct 1, 2018

nalimilan Oct 1, 2018

JeffBezanson Oct 1, 2018

nalimilan Oct 1, 2018 •

edited

Loading

JeffBezanson Oct 2, 2018

nalimilan Oct 2, 2018

JeffBezanson Oct 2, 2018

nalimilan Oct 2, 2018

nalimilan commented Oct 2, 2018

martinholters Oct 5, 2018

nalimilan Oct 5, 2018 •

edited

Loading

martinholters Oct 5, 2018

nalimilan commented Oct 5, 2018

nalimilan Oct 6, 2018

nalimilan Oct 6, 2018

nalimilan commented Nov 28, 2018

nalimilan commented Mar 2, 2022

Throw an error when collecting from iterators with inconsistent length #29458

Are you sure you want to change the base?

Throw an error when collecting from iterators with inconsistent length #29458

Conversation

nalimilan commented Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Oct 2, 2018

Choose a reason for hiding this comment

nalimilan Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Nov 28, 2018

nalimilan commented Mar 2, 2022

nalimilan commented Oct 1, 2018 •

edited

Loading

nalimilan Oct 1, 2018 •

edited

Loading

nalimilan Oct 5, 2018 •

edited

Loading