RFC: Deprecate partial linear indexing (JuliaLang#20079)

* Allow multiple lookup sites in depwarn

* Deprecate partial linear indexing

* Add NEWS.md

* Add comments on the disabled PLI tests

* Ensure linearindices completely inlines (through _length)

Author: mbauman

NEWS.md

@@ -224,6 +224,13 @@ Compiler/Runtime improvements
 Deprecated or removed
 ---------------------
 
+  * Linear indexing is now only supported when there is exactly one
+    non-cartesian index provided. Allowing a trailing index at dimension `d` to
+    linearly access the higher dimensions from array `A` (beyond `size(A, d)`)
+    has been deprecated as a stricter constraint during bounds checking.
+    Instead, `reshape` the array such that its dimensionality matches the
+    number of indices ([#20079]).
+
   * `isdefined(a::Array, i::Int)` has been deprecated in favor of `isassigned` ([#18346]).
 
   * `is` has been deprecated in favor of `===` (which used to be an alias for `is`) ([#17758]).

base/abstractarray.jl

@@ -120,10 +120,10 @@ julia> length(A)
 60
 ```
 """
-length(t::AbstractArray) = prod(size(t))
-_length(A::AbstractArray) = prod(map(unsafe_length, indices(A))) # circumvent missing size
-_length(A) = length(A)
-endof(a::AbstractArray) = length(a)
+length(t::AbstractArray) = (@_inline_meta; prod(size(t)))
+_length(A::AbstractArray) = (@_inline_meta; prod(map(unsafe_length, indices(A)))) # circumvent missing size
+_length(A) = (@_inline_meta; length(A))
+endof(a::AbstractArray) = (@_inline_meta; length(a))
 first(a::AbstractArray) = a[first(eachindex(a))]
 
 """
@@ -306,6 +306,11 @@ function checkbounds(::Type{Bool}, A::AbstractArray, I...)
     @_inline_meta
     checkbounds_indices(Bool, indices(A), I)
 end
+# Linear indexing is explicitly allowed when there is only one (non-cartesian) index
+function checkbounds(::Type{Bool}, A::AbstractArray, i)
+    @_inline_meta
+    checkindex(Bool, linearindices(A), i)
+end
 # As a special extension, allow using logical arrays that match the source array exactly
 function checkbounds{_,N}(::Type{Bool}, A::AbstractArray{_,N}, I::AbstractArray{Bool,N})
     @_inline_meta
@@ -358,7 +363,21 @@ function checkbounds_indices(::Type{Bool}, IA::Tuple{Any}, I::Tuple{Any})
 end
 function checkbounds_indices(::Type{Bool}, IA::Tuple, I::Tuple{Any})
     @_inline_meta
-    checkindex(Bool, OneTo(trailingsize(IA)), I[1])  # linear indexing
+    checkbounds_linear_indices(Bool, IA, I[1])
+end
+function checkbounds_linear_indices(::Type{Bool}, IA::Tuple, i)
+    @_inline_meta
+    if checkindex(Bool, IA[1], i)
+        return true
+    elseif checkindex(Bool, OneTo(trailingsize(IA)), i)  # partial linear indexing
+        partial_linear_indexing_warning_lookup(length(IA))
+        return true # TODO: Return false after the above function is removed in deprecated.jl
+    end
+    return false
+end
+function checkbounds_linear_indices(::Type{Bool}, IA::Tuple, i::Union{Slice,Colon})
+    partial_linear_indexing_warning_lookup(length(IA))
+    true
 end
 checkbounds_indices(::Type{Bool}, ::Tuple, ::Tuple{}) = true
 

base/deprecated.jl

@@ -59,40 +59,40 @@ end
 function depwarn(msg, funcsym)
     opts = JLOptions()
     if opts.depwarn > 0
-        ln = Int(unsafe_load(cglobal(:jl_lineno, Cint)))
-        fn = unsafe_string(unsafe_load(cglobal(:jl_filename, Ptr{Cchar})))
         bt = backtrace()
-        caller = firstcaller(bt, funcsym)
-        if opts.depwarn == 1 # raise a warning
-            warn(msg, once=(caller != C_NULL), key=caller, bt=bt,
-                 filename=fn, lineno=ln)
-        elseif opts.depwarn == 2 # raise an error
-            throw(ErrorException(msg))
-        end
+        _depwarn(msg, opts, bt, firstcaller(bt, funcsym))
     end
     nothing
 end
+function _depwarn(msg, opts, bt, caller)
+    ln = Int(unsafe_load(cglobal(:jl_lineno, Cint)))
+    fn = unsafe_string(unsafe_load(cglobal(:jl_filename, Ptr{Cchar})))
+    if opts.depwarn == 1 # raise a warning
+        warn(msg, once=(caller != StackTraces.UNKNOWN), key=(caller,fn,ln), bt=bt,
+             filename=fn, lineno=ln)
+    elseif opts.depwarn == 2 # raise an error
+        throw(ErrorException(msg))
+    end
+end
 
-function firstcaller(bt::Array{Ptr{Void},1}, funcsym::Symbol)
+firstcaller(bt::Array{Ptr{Void},1}, funcsym::Symbol) = firstcaller(bt, (funcsym,))
+function firstcaller(bt::Array{Ptr{Void},1}, funcsyms)
     # Identify the calling line
-    i = 1
-    while i <= length(bt)
-        lkups = StackTraces.lookup(bt[i])
-        i += 1
+    found = false
+    lkup = StackTraces.UNKNOWN
+    for frame in bt
+        lkups = StackTraces.lookup(frame)
         for lkup in lkups
             if lkup === StackTraces.UNKNOWN
                 continue
             end
-            if lkup.func == funcsym
-                @goto found
-            end
+            found && @goto found
+            found = lkup.func in funcsyms
         end
     end
+    return StackTraces.UNKNOWN
     @label found
-    if i <= length(bt)
-        return bt[i]
-    end
-    return C_NULL
+    return lkup
 end
 
 deprecate(s::Symbol) = deprecate(current_module(), s)
@@ -1739,6 +1739,46 @@ eval(Base.Test, quote
     export @test_approx_eq
 end)
 
+# Deprecate partial linear indexing
+function partial_linear_indexing_warning_lookup(nidxs_remaining)
+    # We need to figure out how many indices were passed for a sensible deprecation warning
+    opts = JLOptions()
+    if opts.depwarn > 0
+        # Find the caller -- this is very expensive so we don't want to do it twice
+        bt = backtrace()
+        found = false
+        call = StackTraces.UNKNOWN
+        caller = StackTraces.UNKNOWN
+        for frame in bt
+            lkups = StackTraces.lookup(frame)
+            for caller in lkups
+                if caller === StackTraces.UNKNOWN
+                    continue
+                end
+                found && @goto found
+                if caller.func in (:getindex, :setindex!, :view)
+                    found = true
+                    call = caller
+                end
+            end
+        end
+        @label found
+        fn = "`reshape`"
+        if call != StackTraces.UNKNOWN && !isnull(call.linfo)
+            # Try to grab the number of dimensions in the parent array
+            mi = get(call.linfo)
+            args = mi.specTypes.parameters
+            if length(args) >= 2 && args[2] <: AbstractArray
+                fn = "`reshape(A, Val{$(ndims(args[2]) - nidxs_remaining + 1)})`"
+            end
+        end
+        _depwarn("Partial linear indexing is deprecated. Use $fn to make the dimensionality of the array match the number of indices.", opts, bt, caller)
+    end
+end
+function partial_linear_indexing_warning(n)
+    depwarn("Partial linear indexing is deprecated. Use `reshape(A, Val{$n})` to make the dimensionality of the array match the number of indices.", (:getindex, :setindex!, :view))
+end
+
 # Deprecate Array(T, dims...) in favor of proper type constructors
 @deprecate Array{T,N}(::Type{T}, d::NTuple{N,Int})               Array{T,N}(d)
 @deprecate Array{T}(::Type{T}, d::Int...)                        Array{T,length(d)}(d...)

base/multidimensional.jl

@@ -181,6 +181,12 @@ end  # IteratorsMD
 using .IteratorsMD
 
 ## Bounds-checking with CartesianIndex
+# Disallow linear indexing with CartesianIndex
+function checkbounds(::Type{Bool}, A::AbstractArray, i::Union{CartesianIndex, AbstractArray{C} where C <: CartesianIndex})
+    @_inline_meta
+    checkbounds_indices(Bool, indices(A), (i,))
+end
+
 @inline checkbounds_indices(::Type{Bool}, ::Tuple{}, I::Tuple{CartesianIndex,Vararg{Any}}) =
     checkbounds_indices(Bool, (), (I[1].I..., tail(I)...))
 @inline checkbounds_indices(::Type{Bool}, IA::Tuple{Any}, I::Tuple{CartesianIndex,Vararg{Any}}) =

src/array.c

@@ -530,7 +530,7 @@ int jl_array_isdefined(jl_value_t **args0, int nargs)
 {
     assert(jl_is_array(args0[0]));
     jl_depwarn("`isdefined(a::Array, i::Int)` is deprecated, "
-               "use `isassigned(a, i)` instead", jl_symbol("isdefined"));
+               "use `isassigned(a, i)` instead", (jl_value_t*)jl_symbol("isdefined"));
 
     jl_array_t *a = (jl_array_t*)args0[0];
     jl_value_t **args = &args0[1];

src/builtins.c

@@ -1045,7 +1045,7 @@ JL_CALLABLE(jl_f_invoke)
     if (jl_is_tuple(args[1])) {
         jl_depwarn("`invoke(f, (types...), ...)` is deprecated, "
                    "use `invoke(f, Tuple{types...}, ...)` instead",
-                   jl_symbol("invoke"));
+                   (jl_value_t*)jl_symbol("invoke"));
         argtypes = (jl_value_t*)jl_apply_tuple_type_v((jl_value_t**)jl_data_ptr(argtypes),
                                                       jl_nfields(argtypes));
     }
@@ -1735,7 +1735,7 @@ JL_DLLEXPORT void jl_breakpoint(jl_value_t *v)
     // put a breakpoint in your debugger here
 }
 
-void jl_depwarn(const char *msg, jl_sym_t *sym)
+void jl_depwarn(const char *msg, jl_value_t *sym)
 {
     static jl_value_t *depwarn_func = NULL;
     if (!depwarn_func && jl_base_module) {
@@ -1749,11 +1749,31 @@ void jl_depwarn(const char *msg, jl_sym_t *sym)
     JL_GC_PUSHARGS(depwarn_args, 3);
     depwarn_args[0] = depwarn_func;
     depwarn_args[1] = jl_cstr_to_string(msg);
-    depwarn_args[2] = (jl_value_t*)sym;
+    depwarn_args[2] = sym;
     jl_apply(depwarn_args, 3);
     JL_GC_POP();
 }
 
+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("partial_linear_indexing_warning"));
+    }
+    if (!depwarn_func) {
+        jl_safe_printf("WARNING: Partial linear indexing is deprecated. Use "
+            "`reshape(A, Val{%zd})` 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

src/cgutils.cpp

@@ -1396,8 +1396,30 @@ static Value *emit_array_nd_index(const jl_cgval_t &ainfo, jl_value_t *ex, ssize
         if (linear_indexing) {
             // Compare the linearized index `i` against the linearized size of
             // the accessed array, i.e. `if !(i < alen) goto error`.
-            Value *alen = emit_arraylen(ainfo, ex, ctx);
-            builder.CreateCondBr(builder.CreateICmpULT(i, alen), endBB, failBB);
+            if (nidxs > 1) {
+                // TODO: REMOVE DEPWARN AND RETURN FALSE AFTER 0.6.
+                // We need to check if this is inside the non-linearized size
+                BasicBlock *partidx = BasicBlock::Create(jl_LLVMContext, "partlinidx");
+                BasicBlock *partidxwarn = BasicBlock::Create(jl_LLVMContext, "partlinidxwarn");
+                Value *d = emit_arraysize_for_unsafe_dim(ainfo, ex, nidxs, nd, ctx);
+                builder.CreateCondBr(builder.CreateICmpULT(ii, d), endBB, partidx);
+
+                // We failed the normal bounds check; check to see if we're
+                // inside the linearized size (partial linear indexing):
+                ctx->f->getBasicBlockList().push_back(partidx);
+                builder.SetInsertPoint(partidx);
+                Value *alen = emit_arraylen(ainfo, ex, ctx);
+                builder.CreateCondBr(builder.CreateICmpULT(i, alen), partidxwarn, failBB);
+
+                // We passed the linearized bounds check; now throw the depwarn:
+                ctx->f->getBasicBlockList().push_back(partidxwarn);
+                builder.SetInsertPoint(partidxwarn);
+                builder.CreateCall(prepare_call(jldepwarnpi_func), ConstantInt::get(T_size, nidxs));
+                builder.CreateBr(endBB);
+            } else {
+                Value *alen = emit_arraylen(ainfo, ex, ctx);
+                builder.CreateCondBr(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`.

src/codegen.cpp

@@ -394,6 +394,7 @@ static Function *expect_func;
 static Function *jldlsym_func;
 static Function *jlnewbits_func;
 static Function *jltypeassert_func;
+static Function *jldepwarnpi_func;
 #if JL_LLVM_VERSION < 30600
 static Function *jlpow_func;
 static Function *jlpowf_func;
@@ -5777,6 +5778,13 @@ static void init_julia_llvm_env(Module *m)
                                         "jl_typeassert", m);
     add_named_global(jltypeassert_func, &jl_typeassert);
 
+    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);
+
     queuerootfun = Function::Create(FunctionType::get(T_void, args_1ptr, false),
                                     Function::ExternalLinkage,
                                     "jl_gc_queue_root", m);

src/julia_internal.h

@@ -817,7 +817,8 @@ STATIC_INLINE void *jl_get_frame_addr(void)
 }
 
 JL_DLLEXPORT jl_array_t *jl_array_cconvert_cstring(jl_array_t *a);
-void jl_depwarn(const char *msg, jl_sym_t *sym);
+void jl_depwarn(const char *msg, jl_value_t *sym);
+void jl_depwarn_partial_indexing(size_t n);
 
 int isabspath(const char *in);