Realias result on the DiffResults paths (fixes #269 and #251) - #295
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #295 +/- ##
==========================================
+ Coverage 85.53% 86.08% +0.54%
==========================================
Files 19 19
Lines 1964 1947 -17
==========================================
- Hits 1680 1676 -4
+ Misses 284 271 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
result on the gradient path (fixes #269)result on the DiffResults paths (fixes #269 and #251)
This was referenced Sep 16, 2026
`DiffResults` requires the object returned by a mutating call to be used, and ReverseDiff returned the argument binding instead. Besides violating that contract (#251), it miscompiles on Julia 1.8 - 1.11.4: the optimizer forwards a stale pre-call read of `result`'s fields across the call that writes them, so `DiffResults.value(result)` still holds the value the result was constructed with. The underlying bug is JuliaLang/julia#62812, fixed by JuliaLang/julia#57201 in Julia 1.11.5 but never backported to the 1.10 LTS. Rebinding has to happen at every level of the chain - `gradients.jl` alone and `tape.jl` alone both still return the stale value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`DiffResults` requires the object returned by a mutating call to be used.
For an `ImmutableDiffResult`, which is what `DiffResults.GradientResult(x)`
returns for a `StaticArray`, `value!` cannot mutate and returns a new struct,
so discarding it drops the primal value. Only the wrapper is immutable, so the
derivatives still come out correct and only `DiffResults.value` goes stale.
The rebind has to be unbroken. `utils.jl` is the load-bearing layer: its leaf
`extract_result!`/`extract_result_value!` methods already realias, but every
caller discarded the result.
Two changes beyond the mechanical rebind:
The `Tuple` methods looped over `result[i]`, which is not assignable, so a
returned struct had nowhere to go. They now `map` over the tuple, matching
`construct_result(input::Tuple)`. Since `map` over tuples truncates to the
shorter argument, the two methods taking both a result and an input tuple
constrain them to a common `NTuple{N,Any}`, so a mismatched arity is a
dispatch error rather than a silent truncation.
`hessian!(result::DiffResult, tape, input)` builds an inner `DiffResult`
holding the gradient in its value slot. `MutableDiffResult` copies into the
shared buffer so it propagates, `ImmutableDiffResult` rebuilds the struct
instead, so the gradient was dropped. It now reads the gradient back off the
inner result.
Allocations are byte-identical across all 26 gradient/Jacobian/Hessian entry
points measured A/B, array and tuple, and inference stays concrete.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
devmotion
force-pushed
the
dmw/fix-269-realias-diffresult
branch
from
September 17, 2026 13:21
ac2f9c6 to
55d69bf
Compare
Codecov flagged the two tuple `map` statements in `utils.jl` as uncovered.
They were uncovered for different reasons.
`extract_result!(result::Tuple, output)` was reachable: it runs when the
target function's output does not depend on the input, so the recorded
output is an untracked `Number` and every derivative is zero. No test ever
differentiated such a function, so the whole family was uncovered, not just
the tuple method. `seeded_reverse_pass!` now passes `input` on to the 3-arg
`extract_result!`, which puts the untracked path on the same methods as the
tracked one. The tuple method is then redundant, and a `result` that does
not match the input is rejected by dispatch on both paths alike -- before,
an untracked output silently filled a mismatched `result` with zeros.
The `Number` methods constrain `input::TrackedArray` even though they
ignore it. Leaving it unconstrained makes them match a tracked output too,
since `TrackedReal <: Number`, which turns those dispatch errors back into
silently-zero gradients.
`extract_result_value!(result::Tuple, output)` was unreachable. Its only
non-recursive call sites pass the `f!` output buffer, which `JacobianConfig`
and `track!` constrain to an `AbstractArray`; a tuple of `DiffResult`s is
handled one level up by `seeded_reverse_pass!`, which recurses per element.
`extract_result_value!(::AbstractArray, ::TrackedArray)` was unreachable for
the same reason: `output_hook` is an `Array{TrackedReal}` on those paths,
never a `TrackedArray`. Both date back to the 2016 pre-release overhaul and
are dropped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `gradient!`/`jacobian!`/`hessian!` docstrings opened with "Returns `result`" and promised to store the derivatives in `result` rather than allocating. That is no longer the whole story: an `ImmutableDiffResult` cannot be mutated, so it is rebuilt and the caller must use the returned object. Since this is the release that makes the rebinding work, the contract needs to be stated where users read it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #269. Fixes #251.
DiffResultsrequires the object returned by a mutating call to be used; we returned the argument binding instead. Two bugs follow from that:result's fields across the call that writes them (1.10 LTS (and 1.11.0–1.11.4): SROA deletes field writes to a mutable that escapes to a non-inlined callee (silent wrong results; fixed by #57201 in 1.11.5) JuliaLang/julia#62812, fixed in 1.11.5, never backported to the 1.10 LTS).ImmutableDiffResult— whatDiffResults.GradientResult(x)returns for aStaticArray—value!cannot mutate and returns a new struct, so the primal value is dropped on every Julia version. Only the wrapper is immutable, so the derivatives still come out correct and onlyDiffResults.valuegoes stale.The rebind has to be unbroken:
gradients.jl,tape.jlandutils.jleach still fail on their own.utils.jlis the load-bearing one — its leafextract_result!/extract_result_value!methods already realias, but every caller discarded the result.Two changes beyond the mechanical rebind:
Tuplemethods inutils.jllooped overresult[i], which is not assignable, so a returned struct had nowhere to go. They nowmapover the tuple, matchingconstruct_result(input::Tuple)just below them. This also removes the pattern that allowed the bug:maphas no place to drop a return value. Sincemapover tuples silently truncates to the shorter argument, the two methods taking both a result and an input tuple now constrain them to a commonNTuple{N,Any}, so a mismatched arity is rejected by dispatch rather than silently ignored.hessian!(result::DiffResult, tape, input)builds an innerDiffResultholding the gradient in its value slot.MutableDiffResultcopies into the shared buffer so it propagates,ImmutableDiffResultrebuilds the struct instead, so the gradient was dropped. That path now reads the gradient back off the inner result.It costs nothing: allocations are byte-identical across all 26
gradient/jacobian/hessianentry points measured A/B, array and tuple, and inference stays concrete throughout. The compiled-tape tuple paths still allocate 0 — the rebuilt tuple is concretely typed and doesn't escape, so it is elided.New tests cover
gradient!,jacobian!andhessian!withMVectorinputs via the config path, a tape and a compiled tape, plus a tuple of immutable results. The #269 test was verified to fail on Julia 1.10 with either half of the fix reverted, under the same--check-bounds=yes --code-coverage=userflags CI uses. Full suite green on 1.10, 1.11 and 1.12.Rebased onto master, which now carries #292 and the
StaticArrays = "1.6.4"bound this needs. The#251Hessian test goes throughHessianConfig(result, input), which builds a tracked buffer withsimilar(output, TrackedReal{D,D,Nothing});TrackedRealis notisbitstype, and StaticArrays only returns a writableSizedArrayfor non-isbits eltypes from 1.6.4 on. That is what theJulia minjob hit on Julia 1.0, where the resolver picked StaticArrays 1.3.6; that job no longer exists.The redundant
v1.17.3bump is dropped, since master released it, and the version is set tov1.18.0for tagging.🤖 Generated with Claude Code