feat: the value comes back from record, and @entered says where it is not @time - #26
Merged
Merged
Conversation
…hat threw Two defects in `record`, both found by reviewing #24 and both older than it. The first breaks the one thing the default layer promises. **A mark that came into existence WHILE the block ran was lost — from the always-on layer too.** `record` snapshotted the probe set before calling `f` and never looked again, so a probe born during the call was entered by code that ran, counted by nobody, and left with its flag `false` for the rest of the process. `entered()` and the exit summary never learned about it either, because while a recording is open the write side counts into the probe instead of setting the flag, and only `record`'s epilogue sets it — over the stale snapshot. record saw: [:tracked] entered(D) = [:tracked] …after: [:newborn, :tracked] entered(D) = [:newborn, :tracked] A package extension loaded inside the block is the ordinary way this happens, and this package ships three of them. The probe set is now re-derived after the call, `saved` is keyed by probe rather than by position, and the reconciliation runs over the union. Re-deriving needs `invokelatest`: the new probes' bindings are younger than the frame reading them, and 1.12 warns that will become an error. **The exception's backtrace pointed at `record`, not at the caller.** `throw(err)` after the `catch` block manufactures a fresh backtrace, so a user debugging a failed run saw `record.jl` and macro expansion where `outer → mid → deep → energy` should be, with nothing to say frames had been dropped — in exactly the case `record(f; rethrow = false)`'s own docstring names as the reason to use it. Closing now happens inside the `catch` and the exception is re-raised with `rethrow()`, which keeps the backtrace it arrived with. The old comment claiming this was impossible was wrong: it is impossible *after* the catch, which is where the call had drifted to. Both are pinned by tests that fail against the unfixed file, each with a control — a mark defined and never called is still absent, and the direct call is shown to carry the frames the recorded one must also carry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… not @time The review of #24 found that `@entered` breaks the `@time` parity its own docstring claims, in two shapes with one cause: `record` takes a function, so the expression runs inside a closure. * `@entered begin x > 5 && return :early; … end` returned from the CLOSURE. `record` discarded what the closure returned, and the macro then read an unassigned `Ref` — so the computed value was silently dropped and the caller got `UndefRefError`, an error naming nothing to do with the cause. * `@entered y = f(x)` binds `y` inside the closure, so at global scope no `y` appears. The first is now fixed rather than documented: `Record` carries `value`, `record` captures what `f` returned, and the macro reads it from there. No box, nothing to leave undefined, and `record(f)` itself stops costing the caller their result — which was the only reason to hand-roll the box pattern the macro used internally. The second is inherent to a closure and is now stated next to the `@time` comparison it contradicts, with the form that does work (`y = @entered f(x)`). Also from the review, all measured against the shipped renderer rather than read: * **The docstring's sample output could not be produced by running the macro.** Hits are sorted by name, so `correlator` comes before `energy`, and the padding was one space wide on every row. Both copies — docstring and `docs/src/observing.md` — are corrected, and the sort is now stated with its reason: an order that moves with the measurement cannot be diffed. * **"seven names they did not write" was wrong.** The real captured path for `sum(inner(x) for _ in 1:n)` has thirteen, including keyword-dispatch wrappers and a generator closure. Corrected in both copies. That number was written from a simplified trace and never checked. * "1 observable marked definition **were** loaded" — the verb agreed with a different count than the noun, and `total == 1` is every package on the day it adopts this. * `@entered @somemacro …` leaked a raw `#= file:line =#` into the header, because `remove_linenums!` leaves the `LineNumberNode` that is a `:macrocall`'s second argument — and the 64-character cut then spent its budget on the file path. * `_short_expr` no longer swallows `InterruptException`. * `_report_entered` prints its header once instead of once per branch, and builds each label and count string once instead of twice. Four test gaps closed, each verified by the mutation that used to survive: deleting the whole footer, `for h in rec[1:1]`, disabling the truncation, and a `\d+` loaded-count that a hardcoded number satisfied. The footer — the line the feature exists for — had no assertion at all. 1079 assertions, green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
📚 Docs preview: https://codes.sota-shimozono.com/ExperimentalAPI.jl/previews/PR26/ (updates on each push to this PR) |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
sotashimozono
added a commit
that referenced
this pull request
Sep 8, 2026
…e told where to write (#27) Before this the package had 82 public names and declared 9 of them — the release layer, because its file format was a guess. The four layers that moved *this week* were not declared, which is the state this package exists to make visible in somebody else's code. * **`record`** — `Record` gained a `value` field in #26, after `write_record` already had a file format, so a file written by an earlier version reads back with that field empty. And `paths` and `timing` are refused together because the pair segfaulted 2 runs in 4 where each alone crashed 0 in 4. * **`@entered`** — its report is text with no schema and it changed twice in its first week: the hit lines gained a sort order and column alignment (#26), the footer a singular verb. * **`reach`** — where `:clean` stops and `:unknown` starts is drawn by Julia's own IR accessors, which are internal and differ by minor version: `Base.IRShow.getdebugidx` on 1.12, `codelocs`/`linetable` on 1.11, and `Core.TypeEgal{T}` where earlier versions say `Type{T}`. * **`verify`** — the counts come from `ccall(:jl_write_coverage_data, …)` and the `.cov` line format, neither of which Julia documents. 39 of 82 are now declared, 43 are not. The point of doing this before a release is what the package's own `isbreaking` then says: a declared name can change without the change being breaking, by this package's own definition. Undeclared, it cannot — and `Record` gaining a field one commit ago is not the behaviour of a settled struct. `test/test_dogfood.jl` used to pin the declared set to exactly the release layer's nine names and assert every reason contained "schema". It now carries a per-layer table, and each layer's anchor is a measured detail — "segfault", "getdebugidx", "jl_write_coverage_data" — so a reason that could have been written without doing the measurement fails. The equality against a hand-written set has a failure mode: marking every name and updating the set to match satisfies it. So a second testset asserts the 24 names of the settled core — the three questions the front page promises — are on the surface and are NOT marked. A package that declared everything would be telling you nothing. Also: `@entered io expr`. The report was hardwired to `stdout`, so a caller who wanted it in a log had to redirect the whole process to get at one line. `io` is looked up when the block runs rather than when the macro expands, so the default still follows `redirect_stdout` — and the test's control asserts that naming an `io` MOVES the report rather than copying it. 1192 assertions, green. 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.
Follow-up to the review of #24. Everything below was measured against the shipped code, not read
off it.
The
@timeparity claim was false, in two shapes with one causerecordtakes a function, so@entered's expression runs inside a closure.The first shape is fixed, not documented:
Recordnow carriesvalue,recordcaptures whatfreturned, and the macro reads it from there. No box, nothing to leave undefined — andrecord(f)itself stops costing the caller their result, which was the only reason to hand-rollthe box pattern the macro was using internally.
The second (
@entered y = f(x)bindsyinside the closure) is inherent to a closure and is nowstated, with the form that does work:
y = @entered f(x).The documentation described output the macro cannot produce
energy, thencorrelatorcorrelatorfirst×All three were in two places (docstring and
docs/src/observing.md). The frame count was writtenfrom a hand-simplified trace and never checked against a real one.
Also
1 observable marked definition **were** loaded— the verb agreed with a different count thanthe noun, and
total == 1is every package on the day it adopts this.@entered @somemacro …leaked#= file:line =#into the header (remove_linenums!leaves a:macrocall's second argument) and the 64-character cut then spent its budget on the file path._short_exprno longer swallowsInterruptException._report_enteredprints its header once instead of once per branch.Four test gaps, each closed against the mutation that used to survive
for h in rec[1:1]\d+vs hardcodedThe footer — the line the feature exists for — had no assertion at all.
1079 assertions, green.
🤖 Generated with Claude Code