fix: reach threw a MethodError on sum(f(x) for x in xs) instead of returning a verdict - #29
Merged
Merged
Conversation
…returning a verdict
Found while writing a worked example that runs: `reach` is specified to be three-valued —
`:depends`, `:clean`, `:unknown`, where the third is the honest non-answer for a call site it
could not pin down. It had a fourth outcome.
reach(f, Tuple{Vector{Float64}})
# ERROR: MethodError: no method matching nameof(::Base.MappingRF{…})
`sum(f(x) for x in xs)` lowers to a `Base.MappingRF` holding the generator closure and a reducing
function. Both fields are singletons, which makes the struct itself a singleton — so
`isdefined(w, :instance)` is true for a callable that is neither a `Function` nor a `Type`, and
`_callee_name` handed it to `nameof`, which has no method for that. The value is now checked
against the three things `nameof` accepts before being asked for a name, and falls back to the
`:?` the function already used for "cannot name this callee".
This is the idiom the package's own `@entered` docstring uses as its worked example
(`driver(x, n) = sum(inner(x) for _ in 1:n)`), so the documented example function could not be
analysed by the documented analysis.
The throw was also masking correct answers, not only crashing. Measured across the higher-order
shapes, before and after:
| caller | before | after |
|---|---|---|
| `sum(unstable(x) for x in xs)` | **threw** | `:depends` |
| `sum(solid(x) for x in xs)` | **threw** | `:clean` |
| `[unstable(x) for x in xs]` | `:unknown` | `:depends` |
| `sum(map(unstable, xs))` | `:unknown` | `:depends` |
| a plain `for` loop | `:clean` | `:clean` |
`[solid(x) for x in xs]` stays `:unknown`, and that asymmetry is the design rather than a
remaining gap: `:depends` needs one witness, `:clean` needs the whole graph resolved.
The new spec testset was checked against the reverted fix — 4 failures with it out, 0 with it in.
187 behaviours, 1215 assertions, green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
📚 Docs preview: https://codes.sota-shimozono.com/ExperimentalAPI.jl/previews/PR29/ (updates on each push to this PR) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
sotashimozono
added a commit
that referenced
this pull request
Sep 9, 2026
…ing to bound it (#30) * fix: reach did not terminate on nightly, and depth alone was never going to bound it My own regression, and the way it reached main is worth writing down. #29 removed a `MethodError` that `reach` threw on `sum(f(x) for x in xs)`. That throw was also, accidentally, a terminator. Without it, on 1.14.0-DEV: reach(f, Tuple{Vector{Float64}}) # [f(x) for x in xs] — did not return reach(g, Tuple{Vector{Float64}}) # sum(map(f, xs)) — did not return Both answer in milliseconds on 1.12.2. The nightly leg is `continue-on-error`, so it could not block the merge; I then cancelled the run that was sitting in `runtest` and a background job merged #29 the moment `gh pr checks` reported nothing pending. The hang was on main for about forty minutes. `maxdepth` bounds how FAR the walk goes, not how much of it there is. Thirty-two levels branching by sixteen candidates is not a finite amount of work in any useful sense, and `visited` only prunes signatures that repeat — a higher-order call generates new ones. So the walk now also carries a `maxwork` budget, shared with every subwalk, and spends `:unknown` with `why = :budget` when it runs out. That is what `:unknown` is for; the alternative was a call that never comes back. Shared, not per-branch, and the distinction is load-bearing: `visited` is deliberately reset in `_subwalk` so a candidate reached under another branch is still walked here, which means `visited` cannot also be the thing that bounds the total. `maxwork` is a keyword on `reach`, `reach(::Module)` and `reach_script`, with the measurement in the docstring — a caller whose entry point comes back `:unknown` with a `:budget` is in a different situation from one that is genuinely dynamic, and only they can decide to pay for more. The spec now pins the property that does not move between versions, because the verdict does: `[unstable(x) for x in xs]` is `:depends` on 1.12.2 and `:unknown` on 1.14.0-DEV. What must hold everywhere is that a caller which can reach a mark is never reported `:clean` — with a control that the same shapes with nothing marked behind them still are, so the assertion is not satisfied by an analysis that never says `:clean` at all. Measured on both: 1237 assertions on 1.12.2, 1236 on 1.14.0-DEV, 190 behaviours, green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: truncated makes reached a lower bound, and the docstring now says so Measured while answering "what happens with several marks behind one loop": budgets between "too small to reach any" and the default report `:depends` with one, two, … of twelve found and the rest never walked to. The verdict is right either way; the LIST is not complete, and `truncated = true` is the only thing that says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Found while writing a worked example that actually runs — not by reading the code.
reachis specified to be three-valued::depends,:clean, and:unknownfor a call site itcould not pin to a method. It had a fourth outcome.
Cause
sum(f(x) for x in xs)lowers to aBase.MappingRFholding the generator closure and a reducingfunction. Both fields are singletons, so the struct itself is a singleton —
isdefined(w, :instance)is true for a callable that is neither aFunctionnor aType, and_callee_namehanded it straight to
nameof, which has no method for that.The value is now checked against the three things
nameofaccepts, falling back to the:?thefunction already used for "cannot name this callee".
This is the idiom the package's own
@entereddocstring uses as its worked example(
driver(x, n) = sum(inner(x) for _ in 1:n)) — so the documented example function could not beanalysed by the documented analysis.
It was masking correct answers, not only crashing
sum(unstable(x) for x in xs):dependssum(solid(x) for x in xs):clean[unstable(x) for x in xs]:unknown:dependssum(map(unstable, xs)):unknown:dependsforloop:clean:clean[solid(x) for x in xs]stays:unknown. That asymmetry is the design, not a remaining gap::dependsneeds one witness,:cleanneeds the whole graph resolved.Verification
The new spec testset was run against the reverted fix: 4 failures with it out, 0 with it in.
187 behaviours, 1215 assertions, green.
🤖 Generated with Claude Code