Deprecate the omission of trailing indices over non-singleton dimensions

NEWS.md

@@ -282,6 +282,11 @@ Deprecated or removed
   * The keyword `immutable` is fully deprecated to `struct`, and
     `type` is fully deprecated to `mutable struct` ([#19157], [#20418]).
 
+  * Indexing into multidimensional arrays with more than one index but fewer indices than there are
+    dimensions is no longer permitted when those trailing dimensions have lengths greater than 1.
+    Instead, reshape the array or add trailing indices so the dimensionality and number of indices
+    match ([#14770], [#23628]).
+
   * `writecsv(io, a; opts...)` has been deprecated in favor of
     `writedlm(io, a, ','; opts...)` ([#23529]).
 

base/abstractarray.jl

@@ -429,7 +429,7 @@ function checkbounds_indices(::Type{Bool}, ::Tuple{}, I::Tuple)
     @_inline_meta
     checkindex(Bool, OneTo(1), I[1]) & checkbounds_indices(Bool, (), tail(I))
 end
-checkbounds_indices(::Type{Bool}, ::Tuple,   ::Tuple{}) = true
+checkbounds_indices(::Type{Bool}, IA::Tuple, ::Tuple{}) = (@_inline_meta; all(x->unsafe_length(x)==1, IA))
 checkbounds_indices(::Type{Bool}, ::Tuple{}, ::Tuple{}) = true
 
 throw_boundserror(A, I) = (@_noinline_meta; throw(BoundsError(A, I)))

base/deprecated.jl

@@ -1748,6 +1748,24 @@ function countnz(x)
     return count(t -> t != 0, x)
 end
 
+# issue #14470
+# TODO: More deprecations must be removed in src/cgutils.cpp:emit_array_nd_index()
+# TODO: Re-enable the disabled tests marked PLI
+# On the Julia side, this definition will gracefully supercede the new behavior (already coded)
+@inline function checkbounds_indices(::Type{Bool}, IA::Tuple{Any,Vararg{Any}}, ::Tuple{})
+    any(x->unsafe_length(x)==0, IA) && return false
+    any(x->unsafe_length(x)!=1, IA) && return _depwarn_for_trailing_indices(IA)
+    return true
+end
+function _depwarn_for_trailing_indices(n::Integer) # Called by the C boundscheck
+    depwarn("omitting indices for non-singleton trailing dimensions is deprecated. Add `1`s as trailing indices or use `reshape(A, Val($n))` to make the dimensionality of the array match the number of indices.", (:getindex, :setindex!, :view))
+    true
+end
+function _depwarn_for_trailing_indices(t::Tuple)
+    depwarn("omitting indices for non-singleton trailing dimensions is deprecated. Add `$(join(map(first, t),','))` as trailing indices or use `reshape` to make the dimensionality of the array match the number of indices.", (:getindex, :setindex!, :view))
+    true
+end
+
 # issue #22791
 @deprecate select partialsort
 @deprecate select! partialsort!

src/cgutils.cpp

@@ -1693,18 +1693,49 @@ static Value *emit_array_nd_index(
     if (bc) {
         // We have already emitted a bounds check for each index except for
         // the last one which we therefore have to do here.
-        bool linear_indexing = nd == -1 || nidxs < (size_t)nd;
-        if (linear_indexing && nidxs == 1) {
-            // Check against the entire linear span of the array
+        if (nidxs == 1) {
+            // Linear indexing: Check against the entire linear span of the array
             Value *alen = emit_arraylen(ctx, ainfo, ex);
             ctx.builder.CreateCondBr(ctx.builder.CreateICmpULT(i, alen), endBB, failBB);
-        } else {
-            // Compare the last index of the access against the last dimension of
-            // the accessed array, i.e. `if !(last_index < last_dimension) goto error`.
+        } else if (nidxs >= (size_t)nd){
+            // No dimensions were omitted; just check the last remaining index
             assert(nd >= 0);
             Value *last_index = ii;
             Value *last_dimension = emit_arraysize_for_unsafe_dim(ctx, ainfo, ex, nidxs, nd);
             ctx.builder.CreateCondBr(ctx.builder.CreateICmpULT(last_index, last_dimension), endBB, failBB);
+        } else {
+            // There were fewer indices than dimensions; check the last remaining index
+            BasicBlock *depfailBB = BasicBlock::Create(jl_LLVMContext, "dimsdepfail"); // REMOVE AFTER 0.7
+            BasicBlock *depwarnBB = BasicBlock::Create(jl_LLVMContext, "dimsdepwarn"); // REMOVE AFTER 0.7
+            BasicBlock *checktrailingdimsBB = BasicBlock::Create(jl_LLVMContext, "dimsib");
+            assert(nd >= 0);
+            Value *last_index = ii;
+            Value *last_dimension = emit_arraysize_for_unsafe_dim(ctx, ainfo, ex, nidxs, nd);
+            ctx.builder.CreateCondBr(ctx.builder.CreateICmpULT(last_index, last_dimension), checktrailingdimsBB, failBB);
+            ctx.f->getBasicBlockList().push_back(checktrailingdimsBB);
+            ctx.builder.SetInsertPoint(checktrailingdimsBB);
+            // And then also make sure that all dimensions that weren't explicitly
+            // indexed into have size 1
+            for (size_t k = nidxs+1; k < (size_t)nd; k++) {
+                BasicBlock *dimsokBB = BasicBlock::Create(jl_LLVMContext, "dimsok");
+                Value *dim = emit_arraysize_for_unsafe_dim(ctx, ainfo, ex, k, nd);
+                ctx.builder.CreateCondBr(ctx.builder.CreateICmpEQ(dim, ConstantInt::get(T_size, 1)), dimsokBB, depfailBB); // s/depfailBB/failBB/ AFTER 0.7
+                ctx.f->getBasicBlockList().push_back(dimsokBB);
+                ctx.builder.SetInsertPoint(dimsokBB);
+            }
+            Value *dim = emit_arraysize_for_unsafe_dim(ctx, ainfo, ex, nd, nd);
+            ctx.builder.CreateCondBr(ctx.builder.CreateICmpEQ(dim, ConstantInt::get(T_size, 1)), endBB, depfailBB);   // s/depfailBB/failBB/ AFTER 0.7
+
+            // Remove after 0.7: Ensure no dimensions were 0 and depwarn
+            ctx.f->getBasicBlockList().push_back(depfailBB);
+            ctx.builder.SetInsertPoint(depfailBB);
+            Value *total_length = emit_arraylen(ctx, ainfo, ex);
+            ctx.builder.CreateCondBr(ctx.builder.CreateICmpULT(i, total_length), depwarnBB, failBB);
+
+            ctx.f->getBasicBlockList().push_back(depwarnBB);
+            ctx.builder.SetInsertPoint(depwarnBB);
+            ctx.builder.CreateCall(prepare_call(jldepwarnpi_func), ConstantInt::get(T_size, nidxs));
+            ctx.builder.CreateBr(endBB);
         }
 
         ctx.f->getBasicBlockList().push_back(failBB);

src/codegen.cpp

@@ -341,6 +341,7 @@ static Function *expect_func;
 static Function *jldlsym_func;
 static Function *jlnewbits_func;
 static Function *jltypeassert_func;
+static Function *jldepwarnpi_func;
 //static Function *jlgetnthfield_func;
 static Function *jlgetnthfieldchecked_func;
 //static Function *jlsetnthfield_func;
@@ -6233,6 +6234,14 @@ static void init_julia_llvm_env(Module *m)
 
     jlapply2va_func = jlcall_func_to_llvm("jl_apply_2va", &jl_apply_2va, m);
 
+    std::vector<Type*> argsdepwarnpi(0);
+    argsdepwarnpi.push_back(T_size);
+    jldepwarnpi_func = Function::Create(FunctionType::get(T_void, argsdepwarnpi, false),
+                                        Function::ExternalLinkage,
+                                        "jl_depwarn_partial_indexing", m);
+    add_named_global(jldepwarnpi_func, &jl_depwarn_partial_indexing);
+
+
     std::vector<Type *> args_1ptr(0);
     args_1ptr.push_back(T_prjlvalue);
     queuerootfun = Function::Create(FunctionType::get(T_void, args_1ptr, false),

src/rtutils.c

@@ -1023,6 +1023,26 @@ void jl_depwarn(const char *msg, jl_value_t *sym)
     JL_GC_POP();
 }
 
+JL_DLLEXPORT void jl_depwarn_partial_indexing(size_t n)
+{
+    static jl_value_t *depwarn_func = NULL;
+    if (!depwarn_func && jl_base_module) {
+        depwarn_func = jl_get_global(jl_base_module, jl_symbol("_depwarn_for_trailing_indices"));
+    }
+    if (!depwarn_func) {
+        jl_safe_printf("WARNING: omitting indices for non-singleton trailing dimensions is deprecated. Use "
+            "`reshape(A, Val(%zd))` or add trailing `1` indices to make the dimensionality of the array match "
+            "the number of indices\n", n);
+        return;
+    }
+    jl_value_t **depwarn_args;
+    JL_GC_PUSHARGS(depwarn_args, 2);
+    depwarn_args[0] = depwarn_func;
+    depwarn_args[1] = jl_box_long(n);
+    jl_apply(depwarn_args, 2);
+    JL_GC_POP();
+}
+
 #ifdef __cplusplus
 }
 #endif

test/abstractarray.jl

@@ -15,11 +15,11 @@ A = rand(5,4,3)
     @test checkbounds(Bool, A, 61) == false
     @test checkbounds(Bool, A, 2, 2, 2, 1) == true  # extra indices
     @test checkbounds(Bool, A, 2, 2, 2, 2) == false
-    @test checkbounds(Bool, A, 1, 1)  == true       # partial linear indexing (PLI)
-    @test checkbounds(Bool, A, 1, 12) == false
-    @test checkbounds(Bool, A, 5, 12) == false
-    @test checkbounds(Bool, A, 1, 13) == false
-    @test checkbounds(Bool, A, 6, 12) == false
+    # @test checkbounds(Bool, A, 1, 1)  == false       # TODO: partial linear indexing (PLI)
+    # @test checkbounds(Bool, A, 1, 12) == false
+    # @test checkbounds(Bool, A, 5, 12) == false
+    # @test checkbounds(Bool, A, 1, 13) == false
+    # @test checkbounds(Bool, A, 6, 12) == false
 end
 
 @testset "single CartesianIndex" begin
@@ -31,16 +31,16 @@ end
     @test checkbounds(Bool, A, CartesianIndex((6, 4, 3))) == false
     @test checkbounds(Bool, A, CartesianIndex((5, 5, 3))) == false
     @test checkbounds(Bool, A, CartesianIndex((5, 4, 4))) == false
-    @test checkbounds(Bool, A, CartesianIndex((1,))) == true
-    @test checkbounds(Bool, A, CartesianIndex((60,))) == false
-    @test checkbounds(Bool, A, CartesianIndex((61,))) == false
+    # @test checkbounds(Bool, A, CartesianIndex((1,))) == false # TODO: PLI
+    # @test checkbounds(Bool, A, CartesianIndex((60,))) == false
+    # @test checkbounds(Bool, A, CartesianIndex((61,))) == false
     @test checkbounds(Bool, A, CartesianIndex((2, 2, 2, 1,))) == true
     @test checkbounds(Bool, A, CartesianIndex((2, 2, 2, 2,))) == false
-    @test checkbounds(Bool, A, CartesianIndex((1, 1,)))  == true
-    @test checkbounds(Bool, A, CartesianIndex((1, 12,))) == false
-    @test checkbounds(Bool, A, CartesianIndex((5, 12,))) == false
-    @test checkbounds(Bool, A, CartesianIndex((1, 13,))) == false
-    @test checkbounds(Bool, A, CartesianIndex((6, 12,))) == false
+    # @test checkbounds(Bool, A, CartesianIndex((1, 1,)))  == false # TODO: PLI
+    # @test checkbounds(Bool, A, CartesianIndex((1, 12,))) == false
+    # @test checkbounds(Bool, A, CartesianIndex((5, 12,))) == false
+    # @test checkbounds(Bool, A, CartesianIndex((1, 13,))) == false
+    # @test checkbounds(Bool, A, CartesianIndex((6, 12,))) == false
 end
 
 @testset "mix of CartesianIndex and Int" begin
@@ -66,10 +66,10 @@ end
     @test checkbounds(Bool, A, 1:61) == false
     @test checkbounds(Bool, A, 2, 2, 2, 1:1) == true  # extra indices
     @test checkbounds(Bool, A, 2, 2, 2, 1:2) == false
-    @test checkbounds(Bool, A, 1:5, 1:4) == true
-    @test checkbounds(Bool, A, 1:5, 1:12) == false
-    @test checkbounds(Bool, A, 1:5, 1:13) == false
-    @test checkbounds(Bool, A, 1:6, 1:12) == false
+    # @test checkbounds(Bool, A, 1:5, 1:4) == false # TODO: PLI
+    # @test checkbounds(Bool, A, 1:5, 1:12) == false
+    # @test checkbounds(Bool, A, 1:5, 1:13) == false
+    # @test checkbounds(Bool, A, 1:6, 1:12) == false
 end
 
 @testset "logical" begin
@@ -81,9 +81,9 @@ end
     @test checkbounds(Bool, A, trues(61)) == false
     @test checkbounds(Bool, A, 2, 2, 2, trues(1)) == true  # extra indices
     @test checkbounds(Bool, A, 2, 2, 2, trues(2)) == false
-    @test checkbounds(Bool, A, trues(5), trues(12)) == false
-    @test checkbounds(Bool, A, trues(5), trues(13)) == false
-    @test checkbounds(Bool, A, trues(6), trues(12)) == false
+    # @test checkbounds(Bool, A, trues(5), trues(12)) == false # TODO: PLI
+    # @test checkbounds(Bool, A, trues(5), trues(13)) == false
+    # @test checkbounds(Bool, A, trues(6), trues(12)) == false
     @test checkbounds(Bool, A, trues(5, 4, 3)) == true
     @test checkbounds(Bool, A, trues(5, 4, 2)) == false
     @test checkbounds(Bool, A, trues(5, 12)) == false
@@ -258,16 +258,20 @@ function test_scalar_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where
     B = T(A)
     @test A == B
     # Test indexing up to 5 dimensions
+    trailing5 = CartesianIndex(ntuple(x->1, max(ndims(B)-5, 0)))
+    trailing4 = CartesianIndex(ntuple(x->1, max(ndims(B)-4, 0)))
+    trailing3 = CartesianIndex(ntuple(x->1, max(ndims(B)-3, 0)))
+    trailing2 = CartesianIndex(ntuple(x->1, max(ndims(B)-2, 0)))
     i=0
     for i5 = 1:size(B, 5)
         for i4 = 1:size(B, 4)
             for i3 = 1:size(B, 3)
                 for i2 = 1:size(B, 2)
                     for i1 = 1:size(B, 1)
                         i += 1
-                        @test A[i1,i2,i3,i4,i5] == B[i1,i2,i3,i4,i5] == i
-                        @test A[i1,i2,i3,i4,i5] ==
-                              Base.unsafe_getindex(B, i1, i2, i3, i4, i5) == i
+                        @test A[i1,i2,i3,i4,i5,trailing5] == B[i1,i2,i3,i4,i5,trailing5] == i
+                        @test A[i1,i2,i3,i4,i5,trailing5] ==
+                              Base.unsafe_getindex(B, i1, i2, i3, i4, i5, trailing5) == i
                     end
                 end
             end
@@ -283,7 +287,7 @@ function test_scalar_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where
     for i2 = 1:size(B, 2)
         for i1 = 1:size(B, 1)
             i += 1
-            @test A[i1,i2] == B[i1,i2] == i
+            @test A[i1,i2,trailing2] == B[i1,i2,trailing2] == i
         end
     end
     @test A == B
@@ -292,7 +296,7 @@ function test_scalar_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where
         for i2 = 1:size(B, 2)
             for i1 = 1:size(B, 1)
                 i += 1
-                @test A[i1,i2,i3] == B[i1,i2,i3] == i
+                @test A[i1,i2,i3,trailing3] == B[i1,i2,i3,trailing3] == i
             end
         end
     end
@@ -310,20 +314,20 @@ function test_scalar_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where
                 for i2 = 1:size(B, 2)
                     for i1 = 1:size(B, 1)
                         i += 1
-                        C[i1,i2,i3,i4,i5] = i
+                        C[i1,i2,i3,i4,i5,trailing5] = i
                         # test general unsafe_setindex!
-                        Base.unsafe_setindex!(D1, i, i1,i2,i3,i4,i5)
+                        Base.unsafe_setindex!(D1, i, i1,i2,i3,i4,i5,trailing5)
                         # test for dropping trailing dims
-                        Base.unsafe_setindex!(D2, i, i1,i2,i3,i4,i5, 1, 1, 1)
+                        Base.unsafe_setindex!(D2, i, i1,i2,i3,i4,i5,trailing5, 1, 1, 1)
                         # test for expanding index argument to appropriate dims
-                        Base.unsafe_setindex!(D3, i, i1,i2,i3,i4)
+                        Base.unsafe_setindex!(D3, i, i1,i2,i3,i4,trailing4)
                     end
                 end
             end
         end
     end
     @test D1 == D2 == C == B == A
-    @test D3[:, :, :, :, 1] == D2[:, :, :, :, 1]
+    @test D3[:, :, :, :, 1, trailing5] == D2[:, :, :, :, 1, trailing5]
     # Test linear indexing and partial linear indexing
     C = T(Int, shape)
     fill!(C, 0)
@@ -340,7 +344,7 @@ function test_scalar_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where
     for i2 = 1:size(C2, 2)
         for i1 = 1:size(C2, 1)
             i += 1
-            C2[i1,i2] = i
+            C2[i1,i2,trailing2] = i
         end
     end
     @test C == B == A
@@ -351,7 +355,7 @@ function test_scalar_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where
         for i2 = 1:size(C3, 2)
             for i1 = 1:size(C3, 1)
                 i += 1
-                C3[i1,i2,i3] = i
+                C3[i1,i2,i3,trailing3] = i
             end
         end
     end
@@ -367,13 +371,17 @@ function test_vector_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where
         N = prod(shape)
         A = reshape(collect(1:N), shape)
         B = T(A)
+        trailing5 = CartesianIndex(ntuple(x->1, max(ndims(B)-5, 0)))
+        trailing4 = CartesianIndex(ntuple(x->1, max(ndims(B)-4, 0)))
+        trailing3 = CartesianIndex(ntuple(x->1, max(ndims(B)-3, 0)))
+        trailing2 = CartesianIndex(ntuple(x->1, max(ndims(B)-2, 0)))
         idxs = rand(1:N, 3, 3, 3)
         @test B[idxs] == A[idxs] == idxs
         @test B[vec(idxs)] == A[vec(idxs)] == vec(idxs)
         @test B[:] == A[:] == collect(1:N)
         @test B[1:end] == A[1:end] == collect(1:N)
-        @test B[:,:] == A[:,:] == B[:,:,1] == A[:,:,1]
-            B[1:end,1:end] == A[1:end,1:end] == B[1:end,1:end,1] == A[1:end,1:end,1]
+        @test B[:,:,trailing2] == A[:,:,trailing2] == B[:,:,1,trailing3] == A[:,:,1,trailing3]
+            B[1:end,1:end,trailing2] == A[1:end,1:end,trailing2] == B[1:end,1:end,1,trailing3] == A[1:end,1:end,1,trailing3]
 
         @testset "Test with containers that aren't Int[]" begin
             @test B[[]] == A[[]] == []
@@ -383,14 +391,14 @@ function test_vector_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where
         idx1 = rand(1:size(A, 1), 3)
         idx2 = rand(1:size(A, 2), 4, 5)
         @testset "Test adding dimensions with matrices" begin
-            @test B[idx1, idx2] == A[idx1, idx2] == reshape(A[idx1, vec(idx2)], 3, 4, 5) == reshape(B[idx1, vec(idx2)], 3, 4, 5)
-            @test B[1, idx2] == A[1, idx2] == reshape(A[1, vec(idx2)], 4, 5) == reshape(B[1, vec(idx2)], 4, 5)
+            @test B[idx1, idx2, trailing2] == A[idx1, idx2, trailing2] == reshape(A[idx1, vec(idx2), trailing2], 3, 4, 5) == reshape(B[idx1, vec(idx2), trailing2], 3, 4, 5)
+            @test B[1, idx2, trailing2] == A[1, idx2, trailing2] == reshape(A[1, vec(idx2), trailing2], 4, 5) == reshape(B[1, vec(idx2), trailing2], 4, 5)
         end
             # test removing dimensions with 0-d arrays
         @testset "test removing dimensions with 0-d arrays" begin
             idx0 = reshape([rand(1:size(A, 1))])
-            @test B[idx0, idx2] == A[idx0, idx2] == reshape(A[idx0[], vec(idx2)], 4, 5) == reshape(B[idx0[], vec(idx2)], 4, 5)
-            @test B[reshape([end]), reshape([end])] == A[reshape([end]), reshape([end])] == reshape([A[end,end]]) == reshape([B[end,end]])
+            @test B[idx0, idx2, trailing2] == A[idx0, idx2, trailing2] == reshape(A[idx0[], vec(idx2), trailing2], 4, 5) == reshape(B[idx0[], vec(idx2), trailing2], 4, 5)
+            @test B[reshape([end]), reshape([end]), trailing2] == A[reshape([end]), reshape([end]), trailing2] == reshape([A[end,end,trailing2]]) == reshape([B[end,end,trailing2]])
         end
 
         mask = bitrand(shape)
@@ -399,8 +407,8 @@ function test_vector_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where
             @test B[vec(mask)] == A[vec(mask)] == find(mask)
             mask1 = bitrand(size(A, 1))
             mask2 = bitrand(size(A, 2))
-            @test B[mask1, mask2] == A[mask1, mask2] == B[find(mask1), find(mask2)]
-            @test B[mask1, 1] == A[mask1, 1] == find(mask1)
+            @test B[mask1, mask2, trailing2] == A[mask1, mask2, trailing2] == B[find(mask1), find(mask2), trailing2]
+            @test B[mask1, 1, trailing2] == A[mask1, 1, trailing2] == find(mask1)
         end
     end
 end

test/arrayops.jl

@@ -2229,3 +2229,9 @@ let a = Vector{Int}[[1]],
     @test eltype([a;b]) == Vector{Float64}
     @test eltype([a;c]) == Vector
 end
+
+# Issue #23629
+@testset "issue 23629" begin
+    @test_throws BoundsError zeros(2,3,0)[2,3]
+    @test_throws BoundsError checkbounds(zeros(2,3,0), 2, 3)
+end

test/offsetarray.jl

@@ -31,11 +31,17 @@ S = OffsetArray(view(A0, 1:2, 1:2), (-1,2))   # IndexCartesian
 @test_throws BoundsError A[0,3,2]
 @test_throws BoundsError S[0,3,2]
 # partial indexing
-S3 = OffsetArray(view(reshape(collect(1:4*3*2), 4, 3, 2), 1:3, 1:2, :), (-1,-2,1))
+S3 = OffsetArray(view(reshape(collect(1:4*3*1), 4, 3, 1), 1:3, 1:2, :), (-1,-2,1))
 @test S3[1,-1] == 2
 @test S3[1,0] == 6
 @test_throws BoundsError S3[1,1]
 @test_throws BoundsError S3[1,-2]
+S4 = OffsetArray(view(reshape(collect(1:4*3*2), 4, 3, 2), 1:3, 1:2, :), (-1,-2,1))
+@test S4[1,-1,2] == 2
+@test S4[1,0,2] == 6
+@test_throws BoundsError S4[1,1,2]
+@test_throws BoundsError S4[1,-2,2]
+
 
 # Vector indexing
 @test A[:, 3] == S[:, 3] == OffsetArray([1,2], (A.offsets[1],))