pretty sure build failures are unrelated

This looks like a simple typo:

That's not how @boundscheck works. That won't throw even if the indices don't line up. Would it work to simply fix that typo or would that be too strict because the typo has been present for six years?

typo aside, it's checking the wrong thing. that's checking that literally all the indices are equal.

map is supposed to stop when any of the constituent arguments are exhausted --- this should be honored even in the presence of @inbounds ! I only made it skip this check in the presence of @inbounds in the PR because I figured there would be complaints if the added indices intersection impacted performance

but for a correct implementation to match the docstring, I think intersect(map(eachindex ∘ LinearIndices, As)...) --- or something else to that effect must be computed, @inbounds or no

this is one of those cases where Julia skipped directly to "make it fast" before "make it right" ☹️

• added a bunch more tests (many failing or segfaulting on main)
• removed all @inbounds until it's clear that these are needed for performance, we can re-add
• I would like to abdicate responsibility for fixing OffsetArrays interactions :)

• I would like to abdicate responsibility for fixing OffsetArrays interactions :)

Unfortunately we have to deal with them before merging because otherwise existing, valid, code that uses OffsetArrays will break. For example,

julia> R = fill(0, -4:-1);

julia> B = OffsetArray(reshape(1:12, 4, 3), -5, 6);

julia> minimum!(R, B)
4-element OffsetArray(::Vector{Int64}, -4:-1) with eltype Int64 with indices -4:-1:
 1
 2
 3
 4

This is a test failure from CI which fails because the implementation of minimum! (which lives in Base) for arrays with unconventional indexing relies on a bug that this PR fixes:

julia> x = fill(0, 1:3);

julia> y = fill(1, 2:4);

julia> map!(identity, x, y)
3-element OffsetArray(::Vector{Int64}, 1:3) with eltype Int64 with indices 1:3:
 1
 1
 1

@nanosoldier runtests()

I guess we'll have to iterate through destination idxs separately to fix the OffsetArrays issue? then dest will be treated just as a fully generic iterable / assignable collection.

function map!(f::F, dest::AbstractArray, A::AbstractArray, B::AbstractArray) where F
    inds = intersect(eachindex(LinearIndices(A)), eachindex(LinearIndices(B)))
    @boundscheck length(dest) >= length(inds)
    dest_idx = firstindex(dest)
    for i = inds
        dest[dest_idx] = f(A[i], B[i])
        dest_idx = nextind(dest, dest_idx)
    end
    return dest
end

I could be wrong but this seems more likely to hurt performance.

Would there be a meaningful difference here between iterating through firstindex, nextindex of dest directly vs through LinearIndices(dest) ?

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Oh, wow! Nanosoldier looks clean. Rerunning failures to be sure:

@nanosoldier runtests(["ApproxLogFunction", "TestSetExtensions", "FunctionOperators", "RemoteFiles", "ChebyshevFiltering", "EinExprs", "Andes", "OpenTelemetryExporterPrometheus", "RegressionTables", "GLPK", "CDDLib", "OrdinaryDiffEqSSPRK", "MPIMeasurements", "ManifoldDiffEq", "OceanRobots", "DynamicMovementPrimitives", "ActiveInference", "Yunir", "ImmersedLayers", "FrequencySweep", "LowRankIntegrators", "NetworkJumpProcesses", "BoxCox"])

I guess we'll have to iterate through destination idxs separately to fix the OffsetArrays issue?

Given how nice PkgEval is looking, another option would be to fix the minimum! implementation to not rely on the map! bug.

Would there be a meaningful difference here between iterating through firstindex, nextindex of dest directly vs through LinearIndices(dest) ?

I doubt it, but hopefully that won't be necessary. If we can implement it in a way that isn't practically breaking, I'd love to make map! take offset indices into account (like map) rather than iterating separately.

I'm finding all the indirection in reducedim.jl pretty inscrutable 😓

but I guess the "real" issue is in mapfirst! ?

function mapfirst!(f::F, R::AbstractArray, A::AbstractArray{<:Any,N}) where {N, F}
    lsiz = check_reducedims(R, A)
    t = _firstreducedslice(axes(R), axes(A))
    map!(f, R, view(A, t...))
end

in your example, we have

julia> t
(IdOffsetRange(values=-4:-1, indices=-4:-1), 7:7)

so then

julia> LinearIndices(view(B, t...)) |> collect
4×1 Matrix{Int64}:
 1
 2
 3
 4

on main, that final map! call will happily map indices [1 2 3 4] into [-4, -3, -2, -1] whereas this PR makes it (correctly?) throw a BoundsError

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

actually, @mbauman 's tour de force #55318 with this PR rebased on top seems to solve the offset arrays case. so maybe the solution is rebase & wait to get more help on that one?

at the very least, this PR would swap "bad" bugs (segfaults, undefined behavior) for "regular" bugs (something throwing a BoundsError when it should not)

updates #30389. also breadcrumb to #46352

#old
map!(f, R, view(A, t...))

#new
map!(f, view(R, firstindex(R):lastindex(R)), view(A, t...))

in mapfirst! is the main update. it looks (and feels) a bit odd to assign into a view like this, but it seems to pass all the test cases now, existing, newly added, and offset-arrays alike.

since a more comprehensive refactor to dimensional reductions appears to be still a ways away, I'd suggest leniency for "ugly" bugfixes like this.

@nanosoldier runtests()

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

ok, I narrowed the scope. only map_n! is changed now with the implementation you suggested.

as you note, this would leave open the question of input validation when the docstring is violated, but at least it fixes the segfaults and uninitialized value exposure (which was my main goal)