Base.getindex(x::AbstractArray{T,N}, mask::Trues{N, NTuple{N,Base.OneTo{Int}}}) is downstream chaos

[ChrisRackauckas]

Found in the ambiguity detection in SciMLBase after a FillArrays.jl dependency was added:

https://github.com/SciML/RecursiveArrayTools.jl/runs/3752855248#step:6:202

The issue stems from Base.getindex(x::AbstractArray{T,N}, mask::Trues{N, NTuple{N,Base.OneTo{Int}}}) types of definitions. The abstract array interface is defined as https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-array . This has definitions getindex(A, I...) and setindex!(A, X, I...) specific to the array A, but possibly non-specific to the type I. To handle some ambiguities we distinguish over a decently sized set:

https://github.com/SciML/SciMLBase.jl/blob/master/src/solutions/solution_interface.jl#L31-L32

but adding FillArrays to the set still fails:

SciML/SciMLBase.jl#105

I think downstream packages defining any overloads of the form Base.getindex(x::AbstractArray{T,N}, I::T) where T is the new package thing will cause ambiguity issues and require all downstream array types to define the intersection method Base.getindex(x::A, I::T), which means the only way it can be handled correctly is for type package T to be a dependency of all downstream array packages A (😱). To me that doesn't sound like a tenable solution.

On top of that, I would presume that this definition, since it introduces ambiguities into all of these downstream "open indexing" definitions, it's also invalidating a ton of downstream getindex and setindex! operations as well. So it's likely not just an issue causing ambiguity errors but also hitting compile times.

Maybe @timholy or @mbauman have an opinion on whether these overloads are okay. I would think the right thing would be to remove them.

[dlfivefifty]

This is a common case of a bad overload that causes problems. Perhaps for now we change it to only N = 1, N = 2, and N = 3?

[ChrisRackauckas]

I think specializing on a few might work.

[timholy]

When specializing on the indices rather than the array, it's better to specialize to_index or to_indices. I would try deleting all of these methods:

FillArrays.jl/src/trues.jl

Lines 26 to 54 in eb8df92

	function Base.setindex!(y::AbstractArray{T,N}, x, mask::Trues{N, NTuple{N,Base.OneTo{Int}}}) where {T,N}
	if axes(x) isa NTuple{N,Base.OneTo{Int}} && axes(y) isa NTuple{N,Base.OneTo{Int}}
	@boundscheck size(y) == size(mask) || throw(BoundsError(y, mask))
	@boundscheck size(x) == size(mask) || throw(DimensionMismatch(
	"tried to assign $(length(x)) elements to $(length(y)) destinations"))
	@boundscheck checkbounds(y, mask)
	return copyto!(y, x)
	end
	return setindex!(y, x, trues(size(mask))) # fall back on usual setindex!
	end
	# x[mask] when mask isa Trues (cf x[trues(size(x))] or x[:])
	# Supported here only for arrays with standard OneTo axes.
	function Base.getindex(x::AbstractArray{T,N}, mask::Trues{N, NTuple{N,Base.OneTo{Int}}}) where {T,N}
	if axes(x) isa NTuple{N,Base.OneTo{Int}}
	@boundscheck size(x) == size(mask) || throw(BoundsError(x, mask))
	return vec(x)
	end
	return x[trues(size(x))] # else revert to usual getindex method
	end
	# https://github.com/JuliaArrays/FillArrays.jl/issues/148 and 150
	function Base.getindex(
	a::AbstractFill{T, N, Tuple{Vararg{Base.OneTo{Int}, N}}},
	b::Trues{N, Tuple{Vararg{Base.OneTo{Int}, N}}},
	) where {T, N}
	@boundscheck size(a) == size(b) || throw(BoundsError(a, b))
	return Fill(getindex_value(a), length(a))
	end

and replacing them with to_indices(A, I) = LinearIndices(A) (with appropriate types on I).

[dlfivefifty]

@timholy I don't see how that could work in this case as we need to do something special with y and x for a special type of indices. Doesn't setindex! have to be overloaded somewhere?

[timholy]

Tell that to CI 😄 (see #163)