Fix OffsetArray-correct indexing in ArrayPartition

[ChrisRackauckas-Claude]

What and why

Three related ArrayPartition indexing paths assumed every partition (and
every copyto! destination) is 1-based, and silently compute the wrong
linear offset for a partition with non-1-based indices (e.g. an
OffsetArray):

1. getindex/setindex!(::ArrayPartition, i::Int) (src/array_partition.jl:308,337)
   compute the within-partition position as length(x) + i, which is only
   correct when firstindex(x) == 1. Fixed to
   firstindex(x) + length(x) + i - 1.
2. copyto!(dest::AbstractArray, A::ArrayPartition) (:238) always starts
   writing dest at linear index 1. Fixed to start at firstindex(dest).
3. The same method's inner per-partition assignment,
   dest[cur:(cur+length(x)-1)] .= vec(x), additionally breaks for a
   non-1-based partition x: vec on an OffsetArray preserves its offset
   axes, so the dot-assignment's axis check fails against the (always
   1-based) destination range. Switched to the 3-arg
   copyto!(dest, cur, x), which copies by position regardless of x's own
   axes.

@inbounds is kept in both getindex/setindex!; the fix stays inside the
existing bounds-checking structure (@boundscheck checkbounds(A, i) still
runs first).

Correctness: failing-before / passing-after

Reused the audit's probes (scratch/probes/p_ap_offset_getindex.jl,
p_ap_offset_setindex.jl, p_ap_copyto_offsetdest.jl).

getindex, ArrayPartition([1.0, 2.0], OffsetArray([10.0,20.0,30.0], 0:2)),
reading A[3], A[4], A[5] (expected 10.0, 20.0, 30.0):

• Before: (20.0, 30.0, 6.9412553390231e-310) (garbage from reading one
  element past the OffsetArray's parent, i.e. silently wrong, plus
  undefined memory on the last read).
• After: (10.0, 20.0, 30.0).

setindex!, A[5] = 99.0 into
ArrayPartition([1.0, 2.0], OffsetArray(zeros(3), 0:2)):

• Before: parent(o) = [0.0, 0.0, 0.0] under default --check-bounds=auto
  (write landed one slot past the end of o's backing Vector, so it's
  invisible via parent, but is an out-of-bounds write — confirmed it
  throws BoundsError under --check-bounds=yes).
• After: parent(o) = [0.0, 0.0, 99.0], and --check-bounds=yes passes.

copyto!, copyto!(OffsetArray(zeros(4), 0:3), ArrayPartition([1.0,2.0],[3.0,4.0])):

• Before: parent(dest) = [0.0, 1.0, 2.0, 3.0] (everything shifted one slot;
  4.0 dropped). Throws BoundsError under --check-bounds=yes.
• After: parent(dest) = [1.0, 2.0, 3.0, 4.0], and --check-bounds=yes passes.

All three re-confirmed on Julia 1.12.4 and 1.10.12, with and without
--check-bounds=yes.

Also checked collect(A) for an ArrayPartition with an OffsetArray
partition (exercises the copyto!(dest::AbstractArray, A::ArrayPartition)
path via Base.collect/_collect_indices), which the first version of this
fix (changing only the starting index, not the inner assignment) still broke
with DimensionMismatch: arrays could not be broadcast to a common size
under the dest[range] .= vec(x) form; collect(A) == [1.0, 2.0, 10.0, 20.0, 30.0]
now.

Normal (non-offset, same-size, mismatched-size, scalar-partition) indexing
and copyto! behavior is unchanged — re-ran scratch/probes/p_ap_mismatch.jl
and got byte-identical output before/after.

Performance

Candidate 13 in the audit noted a possible +2.5%, within noise cost for the
getindex/setindex! change. Measured directly (Julia 1.12.4 and 1.10.12,
@benchmark 5000 samples, ArrayPartition(rand(1000), rand(1000)),
reading/writing index 1500):

	before	after
1.12.4 getindex	5.64 ns	5.64 ns
1.12.4 setindex!	4.43 ns	4.43 ns
1.10.12 getindex	7.63 ns	7.64 ns
1.10.12 setindex!	4.83 ns	4.83 ns

No measurable difference (firstindex on a Vector is a compile-time
constant 1, so the extra term constant-folds away).

Test results

Added test/Core/partitions_test.jl::"OffsetArray partitions index correctly",
using OffsetArrays.jl (newly added to [extras]/[targets]/[compat] in
Project.toml, as this package had no existing OffsetArrays test
dependency).

GROUP=Core: all 12 Core testsets pass (Partitions Tests: 135 Pass, 0 Fail).
GROUP=QA: QA: 20 Pass, 0 Fail.

Both re-run on Julia 1.12.4 and 1.10.12.

What was not verified

• Downstream, GPU, AD, and NoPre (JET) test groups were not run.
• OffsetArray support elsewhere in the package (vector_of_array.jl) is a
  separate, already-known issue (candidate 16/18 in the audit, ragged
  VectorOfArray + OffsetArray inner arrays) and is out of scope here.
• Did not check PermutedDimsArray destinations specifically (the same
  for type in [AbstractArray, PermutedDimsArray] loop defines both methods,
  so the fix applies to both, but the regression test only exercises
  OffsetArray/Vector).

What a reviewer should push back on

Whether copyto!(dest, cur, x) (3-arg) is the right general replacement for
dest[range] .= vec(x) for every x type this loop can see (plain Array,
OffsetArray, and whatever other AbstractArray partitions appear in
practice) — it relies on copyto!'s own linear-index contract for src
rather than broadcasting, which should be strictly more permissive, but is
worth double-checking against any partition type with unusual iteration
order.

Please ignore until reviewed by @ChrisRackauckas.

Risk assessment

• Risk: low
• Blast radius: src/array_partition.jl, three methods
  (getindex/setindex!(::ArrayPartition, ::Int),
  copyto!(dest::AbstractArray/PermutedDimsArray, ::ArrayPartition)); no
  signature or public API change. Only changes behavior for non-1-based
  partitions/destinations, which previously silently corrupted data or threw
  under --check-bounds=yes; 1-based (the common case) is unaffected.
• Evidence: failing-before/passing-after on three probes plus a collect
  check, both with and without --check-bounds=yes, on Julia 1.12.4 and
  1.10.12; no performance change; Core and QA test groups pass.
• Independent review: pending
• Merge: auto-merge candidate, pending independent review

🤖 Generated with Claude Code

https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv

Independent review: Devin Fusion (fusion-claude-opus-5-5-high-sidekick-swe-2-medium) rated it low, verdict MERGE: #672 (comment)

[ChrisRackauckas]

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): MERGE, risk low.

Full review

VERDICT: MERGE
RISK: low

Reviewed head ef22751f7fb4cde51f5d1043422b50f7a080eb53 (matches gh pr view 672 --json headRefOid). The PR links no issue. The closest are #28 (OffsetArrays support, from 2018) and #675 (VectorOfArray with OffsetArray inner arrays, which the PR leaves out of scope). There is no root-level AGENTS.md or CLAUDE.md in the repo, so I applied the user-level CLAUDE.md rules.

Blocking findings

None.

All three changes are correct for every case I tested:

• getindex and setindex! now compute firstindex(x) + length(x) + i - 1.
• copyto! now starts at cur = firstindex(dest).
• The inner per-partition copy is now copyto!(dest, cur, x).

firstindex is the right choice even for N-d OffsetArray partitions, whose linear indices are 1-based. I confirmed this by running the probe: the 2-d offset partition gives the same result before and after the change. The new test's expected values are literals worked out by hand (10.0, 20.0, 30.0, [1,2,3,4]), not copied from the code's output. Without the fix the test fails, and with it the test passes; details below.

Non-blocking findings

1. The test is narrower than the fix (confirmed by running). test/Core/partitions_test.jl:420-441 does not cover:

   • an offset destination together with an offset partition;
   • a Number partition with an offset destination (on master this misplaces data, and throws BoundsError under --check-bounds=yes);
   • an empty partition followed by an offset partition (on master [1.0, 0.0] instead of [1.0, 2.0]);
   • an N-d OffsetArray partition;
   • a PermutedDimsArray destination.

   My probe shows the PR handles all of these correctly. Adding them in a loop would lock that in.

2. The comment in the test is written about the old behaviour (read, rule violation). test/Core/partitions_test.jl:421-423 says "...assumed every partition ... were 1-based, silently reading/writing the wrong element". That describes the bug the PR replaces, which the user-level CLAUDE.md "describe what exists" rule forbids. The PR adds 6 comment lines out of 38 added lines (about 16%, against a ~10% guideline). The 3-line comment in src/array_partition.jl:246-248 is acceptable because it explains why 3-arg copyto! is used.

3. Merge conflict with Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check #670 (confirmed with gh pr diff 670). Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check #670 also appends a testset at test/Core/partitions_test.jl hunk @@ -416,3 +416,67 @@. Whichever lands second will hit a textual conflict. It is trivial to resolve.

4. Inconsistent with sibling PRs on the new test dependency. This PR adds OffsetArrays (MIT, test-only, [extras]/[targets]/[compat] = "1.17"). Siblings Fix two silent memory-safety bugs in recursivecopy!/copyat_or_push! #667 and Add O(1) fast path to AbstractVectorOfArray getindex #668 deliberately hand-roll a minimal OffsetVec "avoiding an OffsetArrays dependency". Either approach is acceptable, but the repo should settle on one. The PR body does not state the license of the new dependency, which CLAUDE.md asks for.

5. The PR body understates the master bug (confirmed by running). The body says the out-of-bounds setindex! write is "invisible via parent". In fact, running the PR's own new testset against master with the default bounds checking crashed Julia 1.12 with signal 11 (Segmentation fault) during GC (scratch/base_newtest.txt), which points to heap corruption. This makes the fix more important than the body suggests.

6. PR body problems (read).

   • It cites "the audit's probes (scratch/probes/*.jl)" and "candidate 13/16/18", which are not in the repo and not linked, so readers cannot check them.
   • It does not open with the UNREVIEWED banner CLAUDE.md requires as its first line (there is only a "Please ignore until reviewed" line partway down), and it ends with a "Generated with Claude Code" line plus a session link instead of the footer format.
   • The commit trailers repeat Co-Authored-By: Claude and Claude Sonnet 5.

   Process only; none of this affects the code.

7. Possible downstream risk (suspected from reading, not observed). The inner copy changed from a broadcast (dest[r] .= vec(x)) to 3-arg copyto!, which goes through Base's generic linear-index 5-arg copyto! for anything that is not an Array or a GPUArray. A partition type that supports broadcasting but has slow or unsupported scalar getindex would regress. I found no such case:

   • JLArray (with scalar indexing disallowed), views, reshapes, sparse, SArray/SMatrix, Adjoint, ranges and nested ArrayPartition all give the same results.
   • One case improved: copyto!(::Vector, ArrayPartition(JLArray...)) errored with "Scalar indexing is disallowed" on master and works now.
   • CUDA was not tested.

What I ran

Setup: TMPDIR was set to the review tmp/, and all scratch work went in scratch/. The checkout was never modified; git status is clean. Before-fix runs used a copy, scratch/base, with src/ taken from origin/master. Test groups ran in a copy, scratch/pr.

• Probe script scratch/probe.jl (47 cases). I ran it on master and on the PR, each under the default bounds checking and under --check-bounds=yes, on Julia 1.12; output is in scratch/probe_out.txt and part{1..4}.txt.

  • Before → after: offset1d getindex goes from [1,2,20,30,7.1e-322] to [1,2,10,20,30].
  • offset1d collect goes from DimensionMismatch to correct.
  • setindex! goes from [0,7,0] (the write to index 5 went out of bounds) to [7,0,9].
  • Offset-destination copyto! goes from [0,1,2,3] to [1,2,3,4].
  • Offset destination with an offset partition, a Number partition with an offset destination, and an empty partition followed by an offset partition are now all correct.
  • Every other case is byte-identical before and after: 1-based baseline, N-d offset, eltype conversion, InexactError still raised, AssertionError on length mismatch, BoundsError at index 0 and length+1, and PermutedDimsArray, view and strided-view destinations.
  • PR output is identical with and without --check-bounds=yes.

• Same probe on Julia 1.10.12 under --check-bounds=yes against the PR checkout: identical to 1.12.

• The PR's new testset, extracted (scratch/newtest.jl):

  • master, default bounds checking: segfault;
  • master, --check-bounds=yes: Fail 2, Error 3;
  • PR on 1.12: Pass 7;
  • PR on 1.10: Pass 7.

• GROUP=Core Pkg.test() on Julia 1.12 (PR copy): all 12 testsets pass, including Partitions Tests 135/135. VecOfArr Indexing shows 2 Broken; those @test_broken markers are in a file the PR does not touch.

• GROUP=QA on Julia 1.12: QA 20/20.

• Runic --check on the two changed .jl files: clean. typos on the diff: clean.

• Micro-benchmark (scratch/bench.jl, best of 200, two rounds each):

  • copyto! 2x1000: 294 → 291 ns
  • copyto! 4x3: 61 → 54 ns
  • mixed eltype: 286 → 278 ns
  • getindex: unchanged

  No regression.

• Sibling-PR overlap: gh pr diff for Fix two silent memory-safety bugs in recursivecopy!/copyat_or_push! #667–Fix method ambiguity in AbstractVectorOfArray similar() breaking QA on Julia 1.10 #678.

What I did not verify

• CUDA, AD and Downstream test groups: no GPU here, and I limited runs to the changed code. GPUArrays behaviour was checked only through JLArrays.
• GROUP=Core on Julia 1.10 (only the probes and the new testset were run on 1.10), and the Downgrade lane (OffsetArrays 1.17.0 was what resolved locally, so the compat floor was exercised).
• The PR body's @benchmark numbers. My own timing shows no regression, but it was not a reproduction of their setup.

---

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (review), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium
Conversation: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master)