Skip to content

Realias result on the DiffResults paths (fixes #269 and #251) - #295

Merged
devmotion merged 5 commits into
masterfrom
dmw/fix-269-realias-diffresult
Sep 17, 2026
Merged

devmotion merged 5 commits into
masterfrom
dmw/fix-269-realias-diffresult

Conversation

@devmotion

@devmotion devmotion commented Sep 16, 2026 •

Copy link
Copy Markdown
Member

Fixes #269. Fixes #251.

DiffResults requires the object returned by a mutating call to be used; we returned the argument binding instead. Two bugs follow from that:

The rebind has to be unbroken: gradients.jl, tape.jl and utils.jl each still fail on their own. utils.jl is the load-bearing one — 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 in utils.jl 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) just below them. This also removes the pattern that allowed the bug: map has no place to drop a return value. Since map over tuples silently truncates to the shorter argument, the two methods taking both a result and an input tuple now constrain them to a common NTuple{N,Any}, so a mismatched arity is rejected by dispatch rather than silently ignored.
  • 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. That path now reads the gradient back off the inner result.

It costs nothing: allocations are byte-identical across all 26 gradient/jacobian/hessian entry 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! and hessian! with MVector inputs 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=user flags 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 #251 Hessian test goes through HessianConfig(result, input), which builds a tracked buffer with similar(output, TrackedReal{D,D,Nothing}); TrackedReal is not isbitstype, and StaticArrays only returns a writable SizedArray for non-isbits eltypes from 1.6.4 on. That is what the Julia min job hit on Julia 1.0, where the resolver picked StaticArrays 1.3.6; that job no longer exists.

The redundant v1.17.3 bump is dropped, since master released it, and the version is set to v1.18.0 for tagging.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.08%. Comparing base (2b1bbb5) to head (b69f5d7).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@devmotion devmotion changed the title Realias result on the gradient path (fixes #269) Realias result on the DiffResults paths (fixes #269 and #251) Sep 16, 2026
devmotion and others added 3 commits September 17, 2026 15:12
`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
devmotion force-pushed the dmw/fix-269-realias-diffresult branch from ac2f9c6 to 55d69bf Compare September 17, 2026 13:21
devmotion and others added 2 commits September 17, 2026 23:45
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>
@devmotion
devmotion merged commit 588d0ef into master Sep 17, 2026
8 checks passed
@devmotion
devmotion deleted the dmw/fix-269-realias-diffresult branch September 17, 2026 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Value is sometimes not set when using DiffResults DiffResults objects are not re-aliased properly

1 participant