From 19b7a9a56f7a192ef8e236954269dbf4012828fe Mon Sep 17 00:00:00 2001 From: sotashimozono Date: Wed, 9 Sep 2026 07:32:30 +0000 Subject: [PATCH 1/2] fix: a docstring above a mark hid its coverage, and attribute failed inside somebody else's package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eleven usage patterns, tried against the package rather than read out of it — the record file round-trip, counting across threads, a run that threw, the mark's exit, `reach_script` on a real script, a method on a foreign generic, the verify layer, the timing half, the documentation helpers, records merged across processes, and a consumer of a marked package. Nine came back correct. Two did not. ## `verification` reported `missing` for the shape this package recommends bare_mark covered=1 total=1 fraction=1.0 documented_mark covered=0 total=0 fraction=missing Same file, same process, each entered once. The only difference is a docstring above the mark. `"docstring"` above a definition parses into ONE statement — `Core.@doc "…" ` — whose start line is the DOCSTRING's. `_find_span` matched a sibling starting exactly on `mk.line`, nothing started there, and the fraction came back `missing`: "no information", for a definition that had just run forty times with a `41` sitting next to it in the `.cov` file. It matters because it is the *recommended* shape. `audit` asks every public name for a docstring and every unfinished one for a mark — "they are not alternatives; the docstring is owed either way" — so a package that follows the advice got the broken half. `unverified` was unaffected: it reads the probe first, which is exact. The spec now pins it with the control in the same run — bare and documented, both entered, same fraction — and was checked against the reverted fix: 2 failures with it out. ## `attribute(record)` failed inside `Profile` MethodError: no method matching getdict(::ExperimentalAPI.Record) `record` hands back a `Record`, `attribute` is the neighbouring verb, and `attribute` takes a profile *buffer*. The mistake is the one this API sets its user up for, and the message named neither the function they wrote nor the argument they passed. It now says what it got, what a `Record` already carries, and what to pass instead. ## And one thing stated rather than fixed `read_record` resolves `Hit.mod` by name: present in this process, it comes back pointing at the module; absent — the ordinary case when shards are merged on a machine that never loaded the package — it falls back to `Main`, and the report reads `Main.energy` for a name that does not exist in `Main`. `Hit.mod` is a `Module` and a file cannot carry one. Documented next to the `method`/`value` note that was already there, and pinned in both directions. 193 behaviours, 1253 assertions, green. Co-Authored-By: Claude Opus 5 --- src/record.jl | 25 +++++++++++++++ src/verify.jl | 14 +++++++++ test/spec/README.md | 6 ++-- test/spec/test_spec_profile.jl | 56 ++++++++++++++++++++++++++++++++++ test/spec/test_spec_verify.jl | 34 ++++++++++++++++++++- 5 files changed, 131 insertions(+), 4 deletions(-) diff --git a/src/record.jl b/src/record.jl index 49bd5d1..db87926 100644 --- a/src/record.jl +++ b/src/record.jl @@ -523,6 +523,22 @@ make. """ attribute(data) = attribute(timing_backend(), data) +# The mistake this package sets its users up for: `record` hands back a `Record`, `attribute` is +# the neighbouring verb, and calling `attribute(rec)` failed inside `Profile` with +# `MethodError: no method matching getdict(::Record)` — a message naming neither argument nor +# function the caller wrote. A `Record` already carries per-hit `inclusive`/`exclusive`; the raw +# buffer is the input here. +function attribute(rec::Record) + return throw( + ArgumentError( + "attribute: expected a profile buffer, got a `Record`. A `Record` already carries " * + "the attribution — read `inclusive`/`exclusive` off its hits, or call " * + "`record(f; timing = true)` to collect them. `attribute` is for a buffer somebody " * + "else profiled: `attribute(Profile.fetch())`.", + ), + ) +end + function attribute(::NoTiming, data) return throw( ArgumentError( @@ -608,6 +624,15 @@ Read back a record written by [`write_record`](@ref). The `method` field of every [`Hit`](@ref) comes back `nothing`, and so does the record's `value`: neither a `Method` nor a run's result is a thing a file can carry, and reconstructing one would mean claiming the code in this process is the code that produced the record. + +!!! warning "`mod` falls back to `Main` when the module is not loaded here" + The file carries a module's **name**. If this process has that module the field comes back + pointing at it; if it does not — which is the ordinary case when shards are merged on a + machine that never loaded the package — `mod` is `Main`, and a report then reads + `Main.energy` for a name that does not exist in `Main`. Read `mod` from a merged record as + "where it was, if that is here", never as an address. `Hit.mod` is a `Module` and a file + cannot carry one, which is why this is stated rather than fixed; [`record`](@ref) and its + file format are declared `@experimental` for this kind of reason. """ function read_record(path::AbstractString) d = TOML.parsefile(path) diff --git a/src/verify.jl b/src/verify.jl index 09242dd..6e7d216 100644 --- a/src/verify.jl +++ b/src/verify.jl @@ -198,10 +198,24 @@ function _sibling_spans(x, outer_end::Int) return out end +# `"docstring"` above a definition parses into `Core.@doc "…" `, one statement whose +# start line is the DOCSTRING's. Nothing then starts on the definition's own line, which is what +# the mark recorded. +function _is_doc_call(st) + return st isa Expr && + st.head === :macrocall && + (st.args[1] === GlobalRef(Core, Symbol("@doc")) || st.args[1] === Symbol("@doc")) +end + function _find_span(x, line::Int, outer_end::Int) (x isa Expr) || return nothing for (a, b, st) in _sibling_spans(x, outer_end) a == line && return (a, b) + # The documented-and-marked definition is the shape this package asks for — `audit` wants + # a docstring on every public name AND a mark on the unfinished ones — and it was the one + # shape whose coverage came back `missing`, because no sibling started on `mk.line`. The + # span is still this sibling's; only where it starts moved. + (_is_doc_call(st) && a < line <= b) && return (line, b) (a <= line <= b) || continue for body in _containers(st) r = _find_span(body, line, b) diff --git a/test/spec/README.md b/test/spec/README.md index 34786e5..26f427d 100644 --- a/test/spec/README.md +++ b/test/spec/README.md @@ -52,10 +52,10 @@ that is entirely `@test_broken` is a claim written down, not a check being run. | `test_spec_forms.jl` | 28 | 28 | 0 | the definition forms a real package hits on its second afternoon | | `test_spec_integration.jl` | 19 | 19 | 0 | where the mark has to surface: docs, Aqua, releases, provenance, CI | | `test_spec_lifecycle.jl` | 16 | 16 | 0 | the mark's EXIT, and an entry point that is a module rather than a function | -| `test_spec_profile.jl` | 45 | 45 | 0 | what a real run went through, how often, and how much of it | +| `test_spec_profile.jl` | 47 | 47 | 0 | what a real run went through, how often, and how much of it | | `test_spec_propagate.jl` | 24 | 24 | 0 | a caller that never names a marked thing still depends on it | -| `test_spec_verify.jl` | 9 | 9 | 0 | how well is a marked thing exercised by the tests | -| **10 files** | **190** | **190** | **0** | | +| `test_spec_verify.jl` | 10 | 10 | 0 | how well is a marked thing exercised by the tests | +| **10 files** | **193** | **193** | **0** | | The table is generated and pinned by `test/test_spec_table.jl`, which fails if it goes stale — diff --git a/test/spec/test_spec_profile.jl b/test/spec/test_spec_profile.jl index 6cfd773..1cedeb3 100644 --- a/test/spec/test_spec_profile.jl +++ b/test/spec/test_spec_profile.jl @@ -632,6 +632,62 @@ end @test ExperimentalAPI.assert_clean(() -> 1 + 1) end +@testset "attribute refuses a Record by name instead of failing inside Profile" begin + # The mistake the API sets a user up for: `record` hands back a `Record`, `attribute` is the + # neighbouring verb, and `attribute(rec)` failed with + # `MethodError: no method matching getdict(::Record)` — naming neither the function the caller + # wrote nor the argument they passed. `attribute` takes a profile BUFFER. + rec = ExperimentalAPI.record(() -> Sim.driver(M, 3); paths=false, timing=false) + e = try + ExperimentalAPI.attribute(rec) + nothing + catch err + err + end + @test e isa ArgumentError + msg = sprint(showerror, e) + @test occursin("attribute", msg) # the verb they wrote + @test occursin("Record", msg) # what they passed + @test occursin("Profile.fetch()", msg) # …and what to pass instead + @test !occursin("getdict", msg) # not an internal of somebody else's package +end + +@testset "a record read where its module is not loaded says Main, and that is stated" begin + # The cross-process case the file format exists for: record on the machine that ran the job, + # merge somewhere else. A file carries a module's NAME, and `Hit.mod` is a `Module`, so a + # reader that never loaded the package has nothing to resolve it to. + # + # Measured: resolving works when the module is present, and falls back to `Main` when it is + # not — so a merged report reads `Main.energy` for a name that does not exist in `Main`. The + # docstring says so; this is what keeps it saying so. + dir = mktempdir() + p = joinpath(dir, "rec.toml") + rec = ExperimentalAPI.record(() -> Sim.driver(M, 3); paths=false, timing=false) + ExperimentalAPI.write_record(p, rec) + + here = ExperimentalAPI.read_record(p) + @test only(here).mod === Sim # present: resolved + + # Absent: read it in a process that never defined `Sim`. + probe = joinpath(dir, "probe.jl") + write( + probe, + """ + using ExperimentalAPI + r = ExperimentalAPI.read_record(ARGS[1]) + println(only(r).mod, " ", only(r).name, " ", only(r).count) + """, + ) + out = read( + `$(Base.julia_cmd()) --startup-file=no --project=$(Base.active_project()) $probe $p`, + String, + ) + parts = split(strip(out)) + @test parts[1] == "Main" # absent: the documented fallback + @test parts[2] == String(only(rec).name) # …and the name and count still cross over + @test parts[3] == string(only(rec).count) +end + @testset "the assertion fails, naming the mark, when the run is not clean" begin # Control: a gate that cannot be shown to fire is not a gate. @test !ExperimentalAPI.assert_clean(() -> Sim.driver(M, 10); throw=false) diff --git a/test/spec/test_spec_verify.jl b/test/spec/test_spec_verify.jl index a191a47..75b6ebe 100644 --- a/test/spec/test_spec_verify.jl +++ b/test/spec/test_spec_verify.jl @@ -26,6 +26,14 @@ end @experimental "shipped without ever being called" never_exercised(x) = x * 0 +""" + documented_and_marked(x) + +Documented AND marked, which is the shape `audit` asks for: a docstring is owed either way, and +the mark is the second account. It is also the shape whose coverage came back `missing`. +""" +@experimental "documented, and still unfinished" documented_and_marked(x) = x + 2 + end # module Covered # Deliberately partial: a fully exercised fixture cannot tell a working join from one that @@ -33,10 +41,34 @@ end # module Covered @testset "the fixture is exercised only partly, on purpose" begin @test Covered.exercised(1) == 2 @test Covered.half_exercised(1) == 1 + @test Covered.documented_and_marked(1) == 3 # `half_exercised(-1)` is NOT called # `never_exercised` is NOT called end +@testset "a docstring above the mark does not hide the coverage" begin + # `"docstring"` above a definition parses into ONE statement, `Core.@doc "…" `, + # whose start line is the docstring's. The span search matched a sibling starting exactly on + # `mk.line`, nothing started there, and the fraction came back `missing` — "no information" — + # for a definition that had just run. Measured: entered once each in the same process, the + # bare mark reported 1/1 and the documented one 0/0 `missing`. + # + # It matters because this is the shape the package RECOMMENDS. `audit` asks every public name + # for a docstring and the unfinished ones for a mark, so a package following the advice got + # the broken half. + vs = Dict(v.mark.name => v for v in ExperimentalAPI.verification(Covered)) + doc = vs[:documented_and_marked] + bare = vs[:exercised] + if bare.fraction === missing + # No coverage in this run at all — then BOTH must say so, which is the claim either way. + @test doc.fraction === missing + else + @test doc.fraction !== missing + @test doc.fraction == bare.fraction == 1.0 + @test doc.total > 0 + end +end + @testset "marks carry the location a coverage file is keyed by" begin for mk in experimental(Covered) @test isfile(String(mk.file)) @@ -130,5 +162,5 @@ end @test length(vs) == length(ExperimentalAPI.experimental(Covered)) @test all(v -> v isa ExperimentalAPI.Verification, vs) @test Set(v.mark.name for v in vs) == - Set([:exercised, :half_exercised, :never_exercised]) + Set([:exercised, :half_exercised, :never_exercised, :documented_and_marked]) end From db5fbdf67d49b23a31ca52ed8fc26b088bdd05b4 Mon Sep 17 00:00:00 2001 From: sotashimozono Date: Wed, 9 Sep 2026 07:35:50 +0000 Subject: [PATCH 2/2] style: cut the comments back to the facts 34 comment lines added by the previous commit, 13 now. What is left is the measurement and the parse fact that is not visible from the code; what went is the narration around them. --- src/record.jl | 16 ++++------------ src/verify.jl | 4 ---- test/spec/test_spec_profile.jl | 19 ++++++------------- test/spec/test_spec_verify.jl | 19 ++++--------------- 4 files changed, 14 insertions(+), 44 deletions(-) diff --git a/src/record.jl b/src/record.jl index db87926..29b95f7 100644 --- a/src/record.jl +++ b/src/record.jl @@ -523,11 +523,7 @@ make. """ attribute(data) = attribute(timing_backend(), data) -# The mistake this package sets its users up for: `record` hands back a `Record`, `attribute` is -# the neighbouring verb, and calling `attribute(rec)` failed inside `Profile` with -# `MethodError: no method matching getdict(::Record)` — a message naming neither argument nor -# function the caller wrote. A `Record` already carries per-hit `inclusive`/`exclusive`; the raw -# buffer is the input here. +# Without this the call failed inside `Profile` with `no method matching getdict(::Record)`. function attribute(rec::Record) return throw( ArgumentError( @@ -626,13 +622,9 @@ neither a `Method` nor a run's result is a thing a file can carry, and reconstru mean claiming the code in this process is the code that produced the record. !!! warning "`mod` falls back to `Main` when the module is not loaded here" - The file carries a module's **name**. If this process has that module the field comes back - pointing at it; if it does not — which is the ordinary case when shards are merged on a - machine that never loaded the package — `mod` is `Main`, and a report then reads - `Main.energy` for a name that does not exist in `Main`. Read `mod` from a merged record as - "where it was, if that is here", never as an address. `Hit.mod` is a `Module` and a file - cannot carry one, which is why this is stated rather than fixed; [`record`](@ref) and its - file format are declared `@experimental` for this kind of reason. + The file carries a module's **name** and `Hit.mod` is a `Module`. Absent from this process — + the ordinary case when shards are merged elsewhere — `mod` is `Main`, and the report reads + `Main.energy` for a name that is not in `Main`. """ function read_record(path::AbstractString) d = TOML.parsefile(path) diff --git a/src/verify.jl b/src/verify.jl index 6e7d216..49e68f1 100644 --- a/src/verify.jl +++ b/src/verify.jl @@ -211,10 +211,6 @@ function _find_span(x, line::Int, outer_end::Int) (x isa Expr) || return nothing for (a, b, st) in _sibling_spans(x, outer_end) a == line && return (a, b) - # The documented-and-marked definition is the shape this package asks for — `audit` wants - # a docstring on every public name AND a mark on the unfinished ones — and it was the one - # shape whose coverage came back `missing`, because no sibling started on `mk.line`. The - # span is still this sibling's; only where it starts moved. (_is_doc_call(st) && a < line <= b) && return (line, b) (a <= line <= b) || continue for body in _containers(st) diff --git a/test/spec/test_spec_profile.jl b/test/spec/test_spec_profile.jl index 1cedeb3..a49f3dd 100644 --- a/test/spec/test_spec_profile.jl +++ b/test/spec/test_spec_profile.jl @@ -633,10 +633,8 @@ end end @testset "attribute refuses a Record by name instead of failing inside Profile" begin - # The mistake the API sets a user up for: `record` hands back a `Record`, `attribute` is the - # neighbouring verb, and `attribute(rec)` failed with - # `MethodError: no method matching getdict(::Record)` — naming neither the function the caller - # wrote nor the argument they passed. `attribute` takes a profile BUFFER. + # `attribute` takes a profile buffer. Given the `Record` next door it failed inside `Profile` + # with `no method matching getdict(::Record)`. rec = ExperimentalAPI.record(() -> Sim.driver(M, 3); paths=false, timing=false) e = try ExperimentalAPI.attribute(rec) @@ -653,13 +651,8 @@ end end @testset "a record read where its module is not loaded says Main, and that is stated" begin - # The cross-process case the file format exists for: record on the machine that ran the job, - # merge somewhere else. A file carries a module's NAME, and `Hit.mod` is a `Module`, so a - # reader that never loaded the package has nothing to resolve it to. - # - # Measured: resolving works when the module is present, and falls back to `Main` when it is - # not — so a merged report reads `Main.energy` for a name that does not exist in `Main`. The - # docstring says so; this is what keeps it saying so. + # A file carries a module's name; `Hit.mod` is a `Module`. Present here it resolves, absent it + # falls back to `Main` — so a merged report reads `Main.energy` for a name not in `Main`. dir = mktempdir() p = joinpath(dir, "rec.toml") rec = ExperimentalAPI.record(() -> Sim.driver(M, 3); paths=false, timing=false) @@ -683,8 +676,8 @@ end String, ) parts = split(strip(out)) - @test parts[1] == "Main" # absent: the documented fallback - @test parts[2] == String(only(rec).name) # …and the name and count still cross over + @test parts[1] == "Main" + @test parts[2] == String(only(rec).name) @test parts[3] == string(only(rec).count) end diff --git a/test/spec/test_spec_verify.jl b/test/spec/test_spec_verify.jl index 75b6ebe..3258c12 100644 --- a/test/spec/test_spec_verify.jl +++ b/test/spec/test_spec_verify.jl @@ -26,12 +26,7 @@ end @experimental "shipped without ever being called" never_exercised(x) = x * 0 -""" - documented_and_marked(x) - -Documented AND marked, which is the shape `audit` asks for: a docstring is owed either way, and -the mark is the second account. It is also the shape whose coverage came back `missing`. -""" +"Documented and marked — the shape `audit` asks for, and the one whose coverage came back `missing`." @experimental "documented, and still unfinished" documented_and_marked(x) = x + 2 end # module Covered @@ -47,15 +42,9 @@ end # module Covered end @testset "a docstring above the mark does not hide the coverage" begin - # `"docstring"` above a definition parses into ONE statement, `Core.@doc "…" `, - # whose start line is the docstring's. The span search matched a sibling starting exactly on - # `mk.line`, nothing started there, and the fraction came back `missing` — "no information" — - # for a definition that had just run. Measured: entered once each in the same process, the - # bare mark reported 1/1 and the documented one 0/0 `missing`. - # - # It matters because this is the shape the package RECOMMENDS. `audit` asks every public name - # for a docstring and the unfinished ones for a mark, so a package following the advice got - # the broken half. + # Measured: entered once each in one process, bare reported 1/1 and documented 0/0 `missing`, + # because the docstring makes the pair one statement starting on the docstring's line. The + # documented-and-marked shape is the one `audit` asks for. vs = Dict(v.mark.name => v for v in ExperimentalAPI.verification(Covered)) doc = vs[:documented_and_marked] bare = vs[:exercised]