diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 12cfd89..8f3f868 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -1,10 +1,12 @@ name: CI -# A version matrix rather than a shard matrix, deliberately. This suite runs in about half a -# minute, so splitting it would cost more setup than it saves — but the mechanism it tests sits -# directly on three things the language has been moving under it: the `public` keyword (1.11), -# `names()` reporting public names, and 1.12's rule about reading a binding created in the same -# world age. Which Julia this runs on is the variable that actually finds bugs here. +# A version matrix rather than a shard matrix, deliberately. This suite runs in about three +# minutes with coverage on — measured 2026-09-04, 977 assertions — so splitting it would still +# cost more setup than it saves. The mechanism it tests sits directly on things the language has +# been moving under it: the `public` keyword (1.11), `names()` reporting public names, 1.12's rule +# about reading a binding created in the same world age, and the compiler internals `reach` walks +# (`CodeInfo.codelocs` on 1.11 became `debuginfo` on 1.12). Which Julia this runs on is the +# variable that actually finds bugs here. # # `test/test_precompile.jl` spawns its own subprocesses with `--depwarn=error`, so the deprecation # that would precede a hard failure on the next Julia fails the job rather than scrolling past. diff --git a/.gitignore b/.gitignore index 6812df1..6646a61 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,7 @@ docs/build/ Manifest.toml +# `flush_coverage` writes these next to the source it measured, so a coverage-enabled run +# of the suite leaves them in `src/`. +*.cov +*.info +lcov.info diff --git a/Project.toml b/Project.toml index 685a813..0712a4b 100644 --- a/Project.toml +++ b/Project.toml @@ -7,13 +7,18 @@ authors = ["sota shimozono "] TOML = "fa267f1f-6049-4f14-aa54-33bafae1ed76" [weakdeps] +Documenter = "e30172f5-a6a5-5a46-863b-614d45cd2de4" +Profile = "9abbd945-dff8-562f-b5e8-e1ebf5ef1b79" Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" [extensions] +ExperimentalAPIDocumenterExt = "Documenter" +ExperimentalAPIProfileExt = "Profile" ExperimentalAPITestExt = "Test" [compat] Aqua = "0.8" +Documenter = "1" Profile = "1" TOML = "1" Test = "1" @@ -21,8 +26,14 @@ julia = "1.11" [extras] Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595" +Documenter = "e30172f5-a6a5-5a46-863b-614d45cd2de4" Profile = "9abbd945-dff8-562f-b5e8-e1ebf5ef1b79" +TOML = "fa267f1f-6049-4f14-aa54-33bafae1ed76" Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" [targets] -test = ["Test", "Aqua", "Profile"] +# `TOML` is a dependency of the package, and `test/spec/test_spec_integration.jl` also imports it +# directly to read a stamp back. Named here rather than relied on through the manifest: a test +# file that imports a package's own dependency without declaring it is the shape that breaks the +# day the dependency is dropped. +test = ["Test", "Aqua", "Documenter", "Profile", "TOML"] diff --git a/README.md b/README.md index 73ba59a..49ad39f 100644 --- a/README.md +++ b/README.md @@ -45,6 +45,16 @@ docstring, and that no mark points at a name that was never made public. **A mar substitute for prose** — it records that a shape is unsettled, which is never a reason to say nothing about what the name does. +Two further questions, both opt-in. +[`record(f)`](https://qatlashub.github.io/ExperimentalAPI.jl/dev/observing/#Recording:-counts,-paths-and-time) +counts how often a run entered each mark, by which paths, and how much of the run it was — exactly, +without emitting anything the flag above does not already emit. +[`reach(f, T)`](https://qatlashub.github.io/ExperimentalAPI.jl/dev/analysing/) asks the other +question, before running anything: what a caller depends on without naming it. Its answer is +three-valued, because Julia's call graph is not closed — `:depends`, `:clean`, and `:unknown` for +a call site that cannot be pinned to a method. Reporting that third case as `:clean` would not be +a weaker claim, it would be a false one. + ## Install ```julia @@ -55,13 +65,17 @@ pkg> add https://github.com/QAtlasHub/ExperimentalAPI.jl [Declaring](https://qatlashub.github.io/ExperimentalAPI.jl/dev/declaring/) · [Observing](https://qatlashub.github.io/ExperimentalAPI.jl/dev/observing/) · +[Analysing](https://qatlashub.github.io/ExperimentalAPI.jl/dev/analysing/) · [Checking](https://qatlashub.github.io/ExperimentalAPI.jl/dev/checking/) · [Release decisions](https://qatlashub.github.io/ExperimentalAPI.jl/dev/releases/) · [Adopting it](https://qatlashub.github.io/ExperimentalAPI.jl/dev/adopting/) · [API](https://qatlashub.github.io/ExperimentalAPI.jl/dev/api/) -`test/spec/` is the specification for the propagation and profiling work that is not built yet, -written as tests so it cannot drift from the code. +`test/spec/` is the specification, written as tests before the implementation so it could not +drift from the code — 176 behaviours across ten files, all of them live assertions. Its +[README](test/spec/README.md) records the negative control each group has, the two requirements +that were withdrawn and why one of them could not be met, and the four defects the exercise found +in the shipped code. ## Development diff --git a/docs/make.jl b/docs/make.jl index f9c076c..7bab856 100644 --- a/docs/make.jl +++ b/docs/make.jl @@ -22,10 +22,15 @@ makedocs(; "Home" => "index.md", "Declaring" => "declaring.md", "Observing" => "observing.md", + "Analysing" => "analysing.md", "Checking" => "checking.md", "Release decisions" => "releases.md", "Adopting it" => "adopting.md", - "API" => "api.md", + "API" => [ + "Declaring" => "api.md", + "Observing and analysing" => "api-runtime.md", + "Checking and releasing" => "api-checks.md", + ], ], ) diff --git a/docs/src/analysing.md b/docs/src/analysing.md new file mode 100644 index 0000000..8711889 --- /dev/null +++ b/docs/src/analysing.md @@ -0,0 +1,114 @@ +```@meta +CurrentModule = ExperimentalAPI +``` + +# Analysing + +[`entered`](@ref) and [`record`](@ref) say what a run *did*. [`reach`](@ref) says what a caller +*could* do — before running it, and including through code that never names the marked thing. + +```julia +julia> r = reach(analyse, Tuple{Model,Float64}); + +julia> verdict(r) +:depends + +julia> r.reached +1-element Vector{ExperimentalAPI.Reached}: + Reached(MyModel.energy via analyse → sweep → inner → energy) +``` + +The model is Lean's `sorry`: a proof that uses one is not a proof, however many layers down it +sits. Julia's call graph is not closed, though, so the answer has to be three-valued. + +## Three answers, and why the third one exists + +| verdict | | +|---|---| +| `:depends` | a marked definition is reachable. Proved, not suspected | +| `:clean` | the whole call graph was resolved and nothing marked is in it | +| `:unknown` | at least one call site could not be pinned to a method | + +`:unknown` is the point of the design. Two shapes really can reach a marked function while being +statically invisible: + +```julia +struct Holder; f::Function; end +top_field(h::Holder, x) = h.f(x) # the callee is a value chosen at run time + +const TABLE = Function[unstable, solid] +top_table(i, x) = TABLE[i](x) # …and so is this one +``` + +Reporting `:clean` there is not a weaker claim, it is a false one. Every unresolved site comes +back as an [`Unresolved`](@ref) carrying the file, the line, why it could not be resolved, and — +when they are visible — the marked methods it could have reached. + +There is deliberately no `verdict` **field**: a stored one makes `:clean` with a non-empty +`unresolved` representable, and that is the single state this analysis must never report. +[`verdict`](@ref) derives it, the way [`isbreaking`](@ref) derives its answer from a +[`Diff`](@ref). [`combine`](@ref) folds two verdicts (`:clean` < `:unknown` < `:depends`), which +is what [`reach`](@ref)`(::Module)` does over a module's entry points. + +## What it resolves + +The walk is over **inferred, un-optimised IR**. Inference runs before inlining, so every call is +still a call and every argument still has a type; `optimize = true` would show `mul_float` and +find nothing. + +| call site | | +|---|---| +| a named call, however deep | resolved | +| a function passed as a value | resolved — Julia specialises on `typeof(f)` | +| a `@nospecialize`d callee, called with a concrete function | resolved | +| `invoke(f, Tuple{Integer}, x)` | resolved to the method `invoke` pins, not the one dispatch would pick | +| a marked `const` or `struct` used in the body | `:depends` — a const is not a call site, and reading globals out of the IR is how it is seen | +| a `Union`- or abstract-typed argument whose candidates include a marked method | **`:unknown`** | +| a callee read out of a field or a table | **`:unknown`** | + +A call site with several matching methods is not automatically unresolved: every candidate is +walked, and if none of them reaches anything marked the site is resolved after all. That is not a +guess — it is having checked all of them. Without it, `convert(::Type, ::UInt32)` (dozens of +matching methods, none of them anybody's research code) would make every caller that formats a +string `:unknown`. + +A more specific unmarked method shadowing a marked one is resolved as what actually runs: an +`Int` goes to `more_specific(::Int)` and is clean, while a `UInt8` falls through to the marked +`::Integer` method and is not. + +## Whole modules, and scripts + +```julia +r = reach(MyPackage) +verdict(r) # one answer for the package +r.affected_entries # …and which public entry points are not clean +``` + +Function-by-function does not scale to a package, and "something in here is experimental" is not +actionable. Each entry point gets its **own** walk: sharing one visited set would make the second +entry that reaches a mark through an already-walked callee look clean. + +[`reach_script`](@ref) is the shape a researcher actually has — a file that produces a figure, not +a package. Note what it costs: the script's top-level `using`, `const` and type definitions are +evaluated in a scratch module, because the analysis has to resolve the names the script uses. + +## The exit, read backwards + +```julia +dependents(MyPackage, :energy) # who reaches it +verdict(reach(MyPackage; ignore = [:energy])) # what removing the mark would change +``` + +`ignore` answers "what would removing this mark change?" without removing it. [`dependents`](@ref) +is propagation read the other way: a mark gets deleted because somebody looked at the definition, +not at who reaches it. + +## What it is not + + * **Not a run.** It says what *could* be reached; [`record`](@ref) says what was. A path this + reports is not necessarily taken. + * **Not sound past a dynamic call.** That is what `:unknown` is for, and why + [`isclean`](@ref) answers `false` for it — the predicate means "may I rely on this", and the + honest non-answer is not a yes. + * **Not free.** It runs inference over the call graph. `maxdepth` and `maxcandidates` bound it, + and hitting either bound is reported as `:unknown`, never as `:clean`. diff --git a/docs/src/api-checks.md b/docs/src/api-checks.md new file mode 100644 index 0000000..f61e11c --- /dev/null +++ b/docs/src/api-checks.md @@ -0,0 +1,13 @@ +```@meta +CurrentModule = ExperimentalAPI +``` + +# API — checking and releasing + +The audit ([Checking](@ref)), the coverage join, the mark's exit, and the release layer +([Release decisions](@ref)). + +```@autodocs +Modules = [ExperimentalAPI] +Pages = ["audit.jl", "verify.jl", "lifecycle.jl", "docsnote.jl", "release.jl", "ExperimentalAPI.jl"] +``` diff --git a/docs/src/api-runtime.md b/docs/src/api-runtime.md new file mode 100644 index 0000000..b27693e --- /dev/null +++ b/docs/src/api-runtime.md @@ -0,0 +1,12 @@ +```@meta +CurrentModule = ExperimentalAPI +``` + +# API — observing and analysing + +What a run went through ([Observing](@ref)), and what a caller could reach ([Analysing](@ref)). + +```@autodocs +Modules = [ExperimentalAPI] +Pages = ["detect.jl", "record.jl", "reach.jl"] +``` diff --git a/docs/src/api.md b/docs/src/api.md index 470e453..3773469 100644 --- a/docs/src/api.md +++ b/docs/src/api.md @@ -2,26 +2,27 @@ CurrentModule = ExperimentalAPI ``` -# API +# API — declaring + +The mark itself, and reading marks back out. The rest of the surface is on +[API — observing and analysing](@ref) and [API — checking and releasing](@ref); the split is by +source file, listed explicitly, so a new file with no page is a build failure rather than a +silently missing section. ```@autodocs Modules = [ExperimentalAPI] +Pages = ["mark.jl", "query.jl"] ``` ## Declared unfinished -Generated from the package's own marks at build time, so it cannot go stale — and it is the -same call any consumer would make: +Generated from the package's own marks at build time by this package's own Documenter extension, +so it cannot go stale — and it is the same call any consumer would make: -```@example marks -using ExperimentalAPI -# `experimental` is `public`, not exported, so it is qualified — which is the visibility -# convention this package leans on rather than duplicates. -for mk in ExperimentalAPI.experimental(ExperimentalAPI) - println(mk.name, "\n ", mk.reason, "\n") -end +```@experimental +ExperimentalAPI ``` -A docstring says what a name does. This says whether it is finished. Nothing on this page is +A docstring says what a name does. That block says whether it is finished. Nothing on this page is undocumented — the names above are documented **and** declared, which is the normal state for something that works but whose shape is still being argued about. diff --git a/docs/src/checking.md b/docs/src/checking.md index 0f34cd2..0232161 100644 --- a/docs/src/checking.md +++ b/docs/src/checking.md @@ -76,7 +76,12 @@ it had none: cannot tell you that you should have. - **Prose quality.** A docstring reading `TODO` counts as documented. [`isdocumented`](@ref) asks whether prose exists, never whether it is any good. -- **Methods added to another package's function.** They are not in `names(m)` and never will be. +- **Methods added to another package's function** are not in `names(m)` and never will be — + which is why the audit has a second half. [`contributed_methods`](@ref) finds them, + [`unaccounted_methods`](@ref) reports the ones with neither a docstring nor a mark, and + [`test_surface`](@ref) asserts on them. For a package whose surface *is* such methods — + `fetch(model, quantity)` with 570 of them — a clean name audit reports nothing while having + looked at none of them. - **Names public only inside an extension**, which is a separate module — audit it separately, or avoid the blind spot the way this package does: declare the function and its docstring in the parent (`function test_surface end` in `src/ExperimentalAPI.jl`) and let the extension add only @@ -99,4 +104,55 @@ contradiction. What `test_surface` adds is `foreign` (a re-exported name whose p else's job) and `dangling` (a mark on a name that was never made public), neither of which Documenter has a notion of. -Run both. Neither is a looser version of the other. +Run both. Neither is a looser version of the other, and +[`aqua_compatible_names`](@ref) computes the difference between them so a project running both can +see exactly which names it would have to argue about. Empty means the two agree. + +## The method-level half + +```julia +julia> audit(Downstream).contributed_methods +4-element Vector{Method}: + fetch_value(::Ising, ::Energy) … + ⋮ + +julia> unaccounted_methods(Downstream) # neither a docstring nor a mark +2-element Vector{Method}: + ⋮ +``` + +Docstrings are keyed by signature, so [`isdocumented`](@ref)`(::Method)` is a real question and +not the same one as [`isdocumented`](@ref)`(m, :name)`. Only the module that *wrote* the method is +asked: the generic's own docstring upstream has the key `Tuple{Any, Any}`, and letting it count +would make one docstring account for all 570 methods anybody ever contributed. + +[`test_surface`](@ref) asserts on these with `require_methods = :foreign` by default — methods on +another *package's* generic, not on Base's. `Base.show(io, ::Audit)` implements a protocol whose +documentation is Base's, and a default that reported every `show`, `==` and `getindex` method as a +finding is a default that gets switched off wholesale. [`extends_base`](@ref) is the rule, stated +as "who owns the generic" rather than as a list of interface functions — a list is not closed +under the ones Julia adds next. `require_methods = :all` widens it. + +## How well is any of it exercised? + +The mark records where it was written, and `--code-coverage` records a count per line. Joining the +two answers the worst case a marked definition can be in: + +```julia +julia> unverified(MyPackage) # marked AND never executed by the suite +1-element Vector{ExperimentalAPI.Mark}: + Mark(MyPackage.never_called, "shipped without ever being called") + +julia> coverage(MyPackage, :half_exercised) +0.6 +``` + +[`coverage`](@ref) answers `missing`, never `0.0`, when the run has no coverage data: a run +without `--code-coverage` has nothing to say, and reporting zero would flag every marked +definition in every ordinary run. The counters are flushed from the running process rather than +read from the files Julia writes at exit, because a test that has to wait for the process to end +cannot assert anything. + +[`stale_marks`](@ref) is the check that keeps the join honest: an edit above a definition moves +the code and not the record, and every downstream reader of `mk.line` then describes the wrong +lines silently. diff --git a/docs/src/index.md b/docs/src/index.md index 4ba360f..68ce011 100644 --- a/docs/src/index.md +++ b/docs/src/index.md @@ -29,15 +29,17 @@ Nobody asked for that summary. It is on by default, it carries the **reason** ra symbol, and a marked definition the run never entered is *absent* — not reported with a count of zero. The same answer is available programmatically through [`entered`](@ref). -## Three things, and the first is the reason to have this +## Five questions, and the first is the reason to have this -| | | +| question | the call | |---|---| -| an **observation** | which marked definitions the run went through — [`entered`](@ref), and the summary at exit | -| a **declaration** | the reason travels with the name, in the source, where the author is — [`@experimental`](@ref) | -| a **check** | every public name is accounted for, or the test fails — [`audit`](@ref), [`test_surface`](@ref) | +| did **this run** go through unvalidated code? | [`entered`](@ref) — and the summary at exit says so anyway | +| how often, by which paths, and how much of the run? | [`record`](@ref) | +| does this caller **depend** on something unvalidated, without naming it? | [`reach`](@ref) | +| what is unfinished here, and which public names are undescribed? | [`experimental`](@ref), [`audit`](@ref) | +| is dropping this name breaking? | [`compare`](@ref), [`compare_methods`](@ref) | -A docstring can carry the second row. Nothing a human writes can carry the first: the question is +A docstring can carry the fourth row. Nothing a human writes can carry the first: the question is not "is this name experimental" but "did *this run* go through one", and it has to be answered after the run, about the run. @@ -59,8 +61,14 @@ it is free at eight threads while every counting scheme is not, and why the noti exit rather than a warning at the call. Only a definition **with a body** carries a flag; see [`@experimental`](@ref) for the -form-by-form table. Counting, call sites and paths are a separate, opt-in layer that is not built -yet. +form-by-form table. + +Counting, call sites and paths are a separate layer, and it is opt-in for exactly the reason the +table gives. [`record`](@ref) reaches the *same* statement from the other side: opening a block +clears every flag, so the short-circuit fails and the write side — a function call, not an inlined +store — does the counting. Nothing in a marked body changes; what changes is which branch of the +one statement is taken. Counts are exact and survive inlining, which is why they come from a +counter and not from a sampler. ## Why this is a separate axis @@ -98,7 +106,8 @@ was never made public, which is the module contradicting itself. | every public name has a docstring | `Docs.undocumented_names`, `Aqua.test_undocumented_names` | the same requirement, not a looser one — plus `foreign` and `dangling` | | generating documentation | Documenter | only ever checks whether prose exists | | run-time behaviour | — | one short-circuit read in the body — see the table above | -| how often a path ran | `Profile`, `@time` | not answered: the default layer knows *whether*, never how often | +| how often a path ran | `Profile`, `@time` | answered by [`record`](@ref), which is opt-in; the default layer knows *whether*, never how often | +| what a caller depends on | — | [`reach`](@ref), statically, with `:unknown` as an honest third answer | `@experimental` adds one statement to the body and nothing else. It does not add a method, does not change dispatch, and never allocates or logs. @@ -118,6 +127,8 @@ loads `Test` because of this. ## Where to go next - [Declaring](@ref) — the forms `@experimental` accepts, and what it refuses -- [Checking](@ref) — the audit, its buckets, and what it cannot see +- [Observing](@ref) — what a run went through, and [`record`](@ref) for how often and how much +- [Analysing](@ref) — what a caller *could* reach, and why the answer is three-valued +- [Checking](@ref) — the audit, its buckets, the method-level half, and what it cannot see - [Release decisions](@ref) — saying mechanically that a change is not breaking - [Adopting it](@ref) — turning this on for a package that already has a backlog diff --git a/docs/src/observing.md b/docs/src/observing.md index 173c8d9..075232b 100644 --- a/docs/src/observing.md +++ b/docs/src/observing.md @@ -78,11 +78,85 @@ the guarded `@warn` costs 5.65× *even though it fires once*: what stops the def the call being in the body at all, not the warning being printed. That is why the notice is a summary at exit rather than a warning at the call. -## What it does not answer +## What the default layer does not answer - * **How often.** Presence only — [`Entry`](@ref)`.count` is always `nothing`. Counting is an - opt-in layer that is not built yet. - * **Which method.** Marks are name-keyed, so two methods of a marked name share one flag. - * **Which call site, or by what path.** Also the opt-in layer. + * **How often.** Presence only — [`Entry`](@ref)`.count` is always `nothing`. + * **Which method.** Flags are name-keyed, so two methods of a marked name share one. + * **Which call site, or by what path.** * **Anything about a declaration-only mark.** A name list, a `struct`, a `const`, a `module`: recorded and audited, never observed. See [`@experimental`](@ref) for the table. + +The first three are [`record`](@ref)'s, and it is a call rather than a default because of the +table above. + +## Recording: counts, paths and time + +```julia +r = record() do + simulate(model; steps = 10_000) +end +``` + +```julia +julia> r +Record — 1 marked definition entered in 0.42s + MyModel.energy ×10000 — convergence not established below β ≈ 0.1 + inclusive 0.31s exclusive 0.28s + via record → sweep → step → energy + recorder overhead ≈ 4.1% +``` + +[`record`](@ref) returns a `Vector`-like of [`Hit`](@ref), so `isempty(r)` and `r[1].count` read +the way they look — with the properties an empty vector could not carry: + +| property | why it is not just a vector | +|---|---| +| `enabled` | an empty record means "nothing was entered"; without this it is indistinguishable from "nothing was recorded", and those are opposite statements | +| `slots` | thread slots the counters were sized for, at least `Threads.maxthreadid()` — the interactive pool means a task's thread id can exceed `nthreads()` | +| `overhead` | the recorder's estimated share of the elapsed time, from a calibrated per-hit cost | +| `versions` | `energy` being experimental in v0.3 says nothing about v0.9 | + +### How it counts without a counter in the body + +The emitted statement never changes. Opening a block clears every probe's flag, so the +short-circuit fails and the *write* side runs on every call — and the write side is a function +call, not an inlined store, so it can afford to count. Counts are therefore **exact** and survive +inlining, which is what ruled out the sampling route: a definition small enough to be worth +marking is small enough to be inlined, and a sampler has no frame left to attribute to. + +Counts are exact under threads too: per-thread counters, sized by `maxthreadid()` and padded so +two threads never share a cache line, summed at the end. + +`paths` is a bounded sample rather than a complete list — a backtrace costs microseconds, so the +recorder stops looking once it has seen enough. The paths a marked definition is reached by are +few and repeat. + +### Time + +`inclusive` and `exclusive` come from Julia's sampling profiler, through a package extension: +without `using Profile` they are `missing`, which is not zero. A run that nobody timed has no +fraction to report, and `0.0` would say the opposite. + +[`experimental_fraction`](@ref) is the share of the run spent inside marked code, derived from the +inclusive times — so it is time and not calls. One entry into a marked kernel that runs for a +minute matters more than a million into a marked accessor. + +[`attribute`](@ref) does the same for a profile buffer that already exists, which is the +twelve-hour-run case: a job that was already profiled must not have to be run again. What comes +back is [`Attribution`](@ref) — samples, never calls, because a sampling profiler cannot count +entries and a field called `count` holding a sample total would read as a measurement it did not +make. + +### As a gate, and as evidence + +[`assert_clean`](@ref) turns a record into a refusal: + +```julia +assert_clean() do + publish(compute(model)) +end +``` + +[`write_record`](@ref) and [`stamp`](@ref) write it down instead. Both produce plain TOML, because +a year later the package that made the figure may not resolve — and a provenance record nobody can +open is not one. diff --git a/docs/src/releases.md b/docs/src/releases.md index 6b25589..e0cc47e 100644 --- a/docs/src/releases.md +++ b/docs/src/releases.md @@ -64,5 +64,62 @@ names, and "compatible" is not a set operation), and a tool that answered it app be worse than one that declines: a green light nobody should have trusted. Which is also why the snapshot layer of this package is itself declared -[`@experimental`](@ref). The file format above is a guess, and making it signature-aware would -change it. +[`@experimental`](@ref). The file format above was a guess, and making it signature-aware changed +it. + +## The method-level half + +The snapshot carries two more keys, and [`compare_methods`](@ref) reads them: + +```toml +stable_methods = ["adapt(::Model, ::Grid)", "measure(::Model)"] + +[experimental_methods."fetch_value(::Heisenberg, ::Energy)"] +reason = "extrapolated from a finite-size sweep; no reference value" +``` + +The key is a signature a human can read in a committed file, and it carries the argument types +and the keyword **names** — so a method whose arguments or keywords changed reads as one key +removed and another added, and [`isbreaking`](@ref) says so. That closes most of the blind spot +above. What stays open is stated as a constant rather than left to be discovered: +[`compare_methods_sees_keywords`](@ref) is `true` for names and the docstring says defaults are +invisible, because a default lives in the body and moves nothing. + +Note also what [`stable`](@ref) does not do: a name with four methods of which one is marked stays +**in** the covenant. Declaring one dispatch path unsettled is not a licence to remove the name. + +## When a mark may go + +A mark that can only ever be added is a decoration. `until=` is what makes it a work item: + +```julia +@experimental( + "no reference value yet", + since = v"0.1.0", + tracking = "https://github.com/org/Pkg.jl/issues/12", + until = () -> isfile(joinpath(@__DIR__, "..", "test", "refs", "energy.toml")), + energy(β) = 2β, +) +``` + +[`ready_to_promote`](@ref) calls that predicate; [`promotable`](@ref) lists the marks whose exit +condition is met. A mark with no `until=` is `false` **always** — reporting it "ready" because it +happens to be covered by a test would be inventing a criterion the author did not state — and +those are reported separately by [`marks_without_exit`](@ref), which is the different finding they +are. + +[`age`](@ref) reads `since` on the axis a version bump is breaking along, and +[`stale_since`](@ref) is the list a CI job turns into a nag. [`exceeds_mark_cap`](@ref) is the +ratchet, in the shape `test_surface`'s `skip` already has. + +## Provenance next to the result + +```julia +stamp("figures/energy_sweep.provenance.toml") do + sweep(model; βs = 0.05:0.05:2.0) +end +``` + +The end state this is all for: a figure's directory says which unvalidated code paths produced it, +in plain TOML with the reason, the counts and [`stamp_versions`](@ref) beside them. A year later +the package may not resolve, and a provenance record nobody can open is not one. diff --git a/ext/ExperimentalAPIDocumenterExt.jl b/ext/ExperimentalAPIDocumenterExt.jl new file mode 100644 index 0000000..bded1fc --- /dev/null +++ b/ext/ExperimentalAPIDocumenterExt.jl @@ -0,0 +1,69 @@ +# The mark, in the rendered documentation, without the author typing the reason a second time. +# +# Typed twice, the prose and the machine-readable declaration drift, and the machine-readable one +# loses — nobody reads it, so nobody notices. This registers an `@experimental` block so a docs +# page can ask the module instead: +# +# ```@experimental +# MyPackage +# ``` + +module ExperimentalAPIDocumenterExt + +using ExperimentalAPI: ExperimentalAPI, marks_markdown +using Documenter: Documenter + +# Reached through `Documenter` rather than declared as a dependency of its own: `MarkdownAST` is +# Documenter's, and an extension that names it separately would have to keep a compat bound on a +# package it never chose. +const MarkdownAST = Documenter.MarkdownAST + +abstract type ExperimentalBlocks <: Documenter.Expanders.NestedExpanderPipeline end + +# Between `@raw` (11.0) and the tail of the pipeline: nothing else matches `@experimental`, so the +# exact position only has to be stable, not early. +Documenter.Selectors.order(::Type{ExperimentalBlocks}) = 11.5 + +function Documenter.Selectors.matcher(::Type{ExperimentalBlocks}, node, page, doc) + return Documenter.iscode(node, r"^@experimental") +end + +function Documenter.Selectors.runner(::Type{ExperimentalBlocks}, node, page, doc) + x = node.element + mods = Module[] + for line in split(x.code, '\n') + name = strip(line) + isempty(name) && continue + m = _resolve(String(name)) + if m === nothing + @error "@experimental block: `$name` is not a loaded module" page = page.source + return nothing + end + push!(mods, m) + end + isempty(mods) && return nothing + md = join((marks_markdown(m) for m in mods), "\n") + # Inserted as siblings and the block unlinked, rather than replacing the element: the parsed + # markdown is several blocks, and a code block's element cannot hold more than one. + for block in Documenter.mdparse(md; mode=:blocks) + MarkdownAST.insert_before!(node, block) + end + MarkdownAST.unlink!(node) + return nothing +end + +function _resolve(name::AbstractString) + parts = Symbol.(split(name, ".")) + for (_, root) in Base.loaded_modules + nameof(root) === parts[1] || continue + m = root + for p in parts[2:end] + (isdefined(m, p) && getglobal(m, p) isa Module) || return nothing + m = getglobal(m, p) + end + return m + end + return nothing +end + +end # module diff --git a/ext/ExperimentalAPIProfileExt.jl b/ext/ExperimentalAPIProfileExt.jl new file mode 100644 index 0000000..ade134a --- /dev/null +++ b/ext/ExperimentalAPIProfileExt.jl @@ -0,0 +1,191 @@ +# Where `record`'s inclusive/exclusive time comes from. +# +# Sampling, because the alternative is wrapping the call, and the one statement `@experimental` +# puts in a body may not do that — the measurement that settled it is in `test/spec/README.md`. +# `Profile` is Julia's own sampler and a stdlib, but `ExperimentalAPI` is loaded at run time by +# every marked package, so it stays out of the load path until somebody asks for timing. + +module ExperimentalAPIProfileExt + +using ExperimentalAPI: + ExperimentalAPI, Attribution, TimingBackend, isexperimental, mark, marked_modules +using Profile: Profile + +struct ProfileTiming <: TimingBackend end + +# The sampler's default is one sample per millisecond, which is nothing at all for the runs worth +# recording — a marked kernel called ten thousand times can finish inside two of them. Finer while +# a recording is open, and put back afterwards: it is global state somebody else may be using. +const _DELAY = 1e-4 +const _SAVED = Ref{Any}(nothing) + +function ExperimentalAPI.start_timing!(::ProfileTiming; clear::Bool=true) + try + if clear + _SAVED[] = Profile.init() + n, _ = _SAVED[] + Profile.init(; n, delay=_DELAY) + Profile.clear() + else + # The caller is already using the buffer. `Profile.init` reallocates it, so setting + # the delay here would throw their samples away — which is the one thing + # `with_profile = true` exists to prevent. Their sampling rate stands. + _SAVED[] = nothing + end + Profile.start_timer() + return true + catch + return false + end +end + +function ExperimentalAPI.stop_timing!(::ProfileTiming) + try + Profile.stop_timer() + catch + end + return nothing +end + +# The saved settings are restored HERE and not in `stop_timing!`, because `Profile.init` +# reallocates the buffer: restoring the delay before reading the samples throws the run's own +# measurement away, and the symptom is a fraction of exactly zero rather than an error. +function ExperimentalAPI.attribute_timing(::ProfileTiming) + out = try + _fractions(_raw(Profile.fetch())) + catch + nothing + end + try + s = _SAVED[] + s === nothing || Profile.init(; n=s[1], delay=s[2]) + catch + end + return out +end + +function ExperimentalAPI.attribute(::ProfileTiming, data) + d = _stacks(_raw(data)) + total = length(d) + out = Attribution[] + total == 0 && return out + counts = Dict{Tuple{Module,Symbol},Vector{Int}}() + reasons = Dict{Tuple{Module,Symbol},String}() + idx = _mark_index() + for stack in d + _tally!(counts, reasons, stack, idx) + end + for (k, v) in counts + push!(out, Attribution(k[1], k[2], reasons[k], v[1], v[1] / total, v[2] / total)) + end + return sort!(out; by=a -> (-a.inclusive, string(a.mod), string(a.name))) +end + +# The instruction pointers, with the per-sample metadata removed. Asked for separately rather than +# through `Profile.fetch(; include_meta = false)`, which does the same strip behind an +# `@assert`: measured on 1.14.0-DEV.3115, that assertion fires (`metadata stripping failed`) on a +# buffer this package did not fill, and an exception there would silently turn timing off. +function _raw(data) + return try + Profile.has_meta(data) ? Profile.strip_meta(data) : data + catch + data + end +end + +function _fractions(data) + stacks = _stacks(data) + total = length(stacks) + out = Dict{Tuple{Module,Symbol},Tuple{Float64,Float64}}() + total == 0 && return out + counts = Dict{Tuple{Module,Symbol},Vector{Int}}() + reasons = Dict{Tuple{Module,Symbol},String}() + idx = _mark_index() + for stack in stacks + _tally!(counts, reasons, stack, idx) + end + for (k, v) in counts + out[k] = (v[1] / total, v[2] / total) + end + return out +end + +# One sample's frames, innermost first, with C frames dropped. `Profile`'s buffer separates +# samples with a zero and stores each one leaf first. +function _stacks(data) + lidict = Profile.getdict(data) + out = Vector{Vector{Any}}() + current = Any[] + for ip in data + if ip == 0 + isempty(current) || push!(out, current) + current = Any[] + continue + end + frames = get(lidict, ip, nothing) + frames === nothing && continue + for fr in frames + fr.from_c && continue + push!(current, fr) + end + end + isempty(current) || push!(out, current) + return out +end + +# `counts[key] = [inclusive, exclusive]`. A definition is counted inclusively once per sample it +# appears in, however many frames deep, and exclusively only when it is the innermost Julia frame. +function _tally!(counts, reasons, stack, idx) + seen = Set{Tuple{Module,Symbol}}() + for (i, fr) in enumerate(stack) + mk = _mark_of(fr, idx) + mk === nothing && continue + k = (mk.mod, mk.name) + v = get!(counts, k, [0, 0]) + reasons[k] = mk.reason + if !(k in seen) + push!(seen, k) + v[1] += 1 + end + i == 1 && (v[2] += 1) + end + return nothing +end + +# A marked definition small enough to be worth marking is small enough to be inlined, and an +# inlined frame carries no `MethodInstance` at all — so the `linfo` route, which is the exact one, +# answers `nothing` for exactly the frames that matter most. The index is the fallback: every +# mark that resolves to a method, keyed by the file and function name a frame does carry. +function _mark_of(fr, idx) + li = fr.linfo + li isa Core.CodeInstance && (li = li.def) + if li isa Core.MethodInstance && li.def isa Method + mk = mark(li.def) + mk === nothing || return mk + end + return get(idx, (fr.file, fr.func), nothing) +end + +function _mark_index() + idx = Dict{Tuple{Symbol,Symbol},Any}() + for mod in marked_modules() + for mk in ExperimentalAPI.experimental(mod) + mk.sig === nothing && continue + m = try + which(mk.sig) + catch + nothing + end + m === nothing && continue + idx[(m.file, m.name)] = mk + end + end + return idx +end + +function __init__() + ExperimentalAPI._TIMING[] = ProfileTiming() + return nothing +end + +end # module diff --git a/ext/ExperimentalAPITestExt.jl b/ext/ExperimentalAPITestExt.jl index 07fc4d4..043e02e 100644 --- a/ext/ExperimentalAPITestExt.jl +++ b/ext/ExperimentalAPITestExt.jl @@ -3,22 +3,55 @@ module ExperimentalAPITestExt -using ExperimentalAPI: ExperimentalAPI, Audit, audit, isdocumented, isexperimental +using ExperimentalAPI: + ExperimentalAPI, + Audit, + audit, + extends_base, + isdocumented, + isexperimental, + experimental, + partition_holds using Test: Test, @test, @testset +# The knobs below are the newest part of this package and the least settled: which of them a +# project should turn on is a question no measurement has answered yet, and the answer will change +# what they mean. The mark is written here, in the extension that defines them, which is also the +# case `experimental(m; extensions = true)` exists for. +ExperimentalAPI.@experimental( + "which gates a project should run, and therefore what these keywords should default to, is " * + "undecided; `require_tracking` and `max_marks` may be replaced by one policy argument", + since = v"0.1.0", + until = () -> false, + test_surface, +) + function ExperimentalAPI.test_surface( - m::Module; skip::AbstractVector{Symbol}=Symbol[], outputlevel::Int=0 + m::Module; + skip::AbstractVector{Symbol}=Symbol[], + require_tracking::Bool=false, + max_marks::Union{Int,Nothing}=nothing, + methods::Bool=true, + require_methods::Symbol=:foreign, + outputlevel::Int=0, ) - a = audit(m) + require_methods in (:none, :foreign, :all) || throw( + ArgumentError( + "test_surface: require_methods must be :none, :foreign or :all, got " * + repr(require_methods), + ), + ) + a = audit(m; methods) outputlevel ≥ 1 && show(stdout, MIME"text/plain"(), a) @testset "public surface of $(nameof(m))" begin - # Two assertions that run whatever the audit found. Everything below iterates over a set - # of findings, so on a clean module all of it collapses to nothing and the testset would + # Assertions that run whatever the audit found. Everything below iterates over a set of + # findings, so on a clean module all of it collapses to nothing and the testset would # report `0 tests passed` — a green indistinguishable from the extension having failed to # load, or from `m` having no public names at all. These make the pass mean something. @testset "every public name has a docstring" begin @test isempty(setdiff(a.undocumented, skip)) @test isempty(a.dangling) + @test partition_holds(a) end # One testset per name, so a failing CI log names the symbol in its header rather than # printing a set difference the reader has to diff by eye. @@ -40,6 +73,40 @@ function ExperimentalAPI.test_surface( @testset "@experimental $n is public" for n in a.dangling @test n in a.surface end + # A mark with no tracking link is a note to nobody. Off by default because whether a + # project can require one depends on whether it has an issue tracker at all. + if require_tracking + @testset "@experimental $(mk.name) says where it is being decided" for mk in + experimental( + m + ) + @test mk.tracking !== nothing + end + end + # The ratchet. `nothing` rather than `typemax`: a cap that is off must not read as a cap + # that is enormous, because the second is a number somebody has to justify. + if max_marks !== nothing + @testset "at most $max_marks marks" begin + @test length(experimental(m)) <= max_marks + end + end + # A method contributed to another module's generic is invisible to `names(m)`, so for a + # package whose surface is such methods every assertion above passes having looked at + # nothing. + # + # `:foreign` by default rather than `:all`: `Base.show(io, ::Audit)` is a protocol Base + # documents, and a default that reports every `show`, `==` and `getindex` method as a + # finding is a default that gets switched off wholesale. See `extends_base` for the rule. + checked = if require_methods === :none + Method[] + elseif require_methods === :all + a.unaccounted_methods + else + filter(!extends_base, a.unaccounted_methods) + end + @testset "contributed method $(mm.name)$(mm.sig) is accounted for" for mm in checked + @test isdocumented(mm) || isexperimental(mm) + end end return a end diff --git a/src/ExperimentalAPI.jl b/src/ExperimentalAPI.jl index c989380..8dfc394 100644 --- a/src/ExperimentalAPI.jl +++ b/src/ExperimentalAPI.jl @@ -15,34 +15,36 @@ using ExperimentalAPI @experimental "convergence not established below β ≈ 0.1" energy(m::Model) = m.β * correction(m) ``` -The mark is three things, and the first is the reason to have it: +# The five questions this answers - * an **observation** — [`entered`](@ref) reports the marked definitions this run actually went - through, and a summary says so at process exit whether or not anyone asked; - * a **declaration** — the reason travels with the name, in the source, where the author is; - * a **check** — [`audit`](@ref) reports every public name with no docstring, marked or not, so - "every public name is described" becomes a test that fails. - -# What it costs +| question | the call | +|---|---| +| did **this run** go through unvalidated code? | `entered()` — and the summary at exit says so anyway | +| how often, by which paths, and how much of the run? | `record(f)` | +| does this caller **depend** on something unvalidated, without naming it? | `reach(f, Tuple{…})` | +| what is unfinished here, and which public names are undescribed? | `experimental(M)`, `audit(M)` | +| is dropping this name breaking? | `compare(old_snapshot, M)` — see [`isbreaking`](@ref) | -One short-circuit read in the body, and a write on the first call only: measured at 1.03x on one -thread and 0.985x on eight, for 10M calls of a numeric body. A flag written once and read -thereafter stops dirtying the cache line, which a counter (3.76x at eight threads, and losing 40% -of its increments to races unless atomic) does not. Counting, call sites and paths are a separate -layer that is not built yet. +The first is the reason to have any of it. It is asked *after* a run, *about* the run, by somebody +who is not the author — which is exactly the question a docstring is structurally unable to answer. -Only a definition **with a body** carries a flag. A mark written as a name list, or attached to a -struct, a const or a module, is a declaration — queryable, audited, but not observed at run time. -See [`@experimental`](@ref) for the form-by-form table. +# Three layers, and what each costs -# The three questions this answers + * **Detection** is on always and costs one short-circuit read in the body: measured at `1.03x` on + one thread and `0.985x` on eight, for 10M calls of a numeric body — see + [`overhead_when_detecting`](@ref). A flag written once and read thereafter stops dirtying its + cache line, which a counter (3.76x at eight threads, and losing 40% of its increments to races + unless atomic) does not. + * **Recording** ([`record`](@ref)) counts, captures call paths and attributes time. It costs + something, which is why it is a call and not a default, and each record reports its own + overhead. + * **Analysis** ([`reach`](@ref)) is static and answers about code rather than about a run. Its + answer is three-valued: `:depends`, `:clean`, and `:unknown` for a call site it could not pin + to a method. Reporting `:unknown` as `:clean` is not a weaker claim, it is a false one. -| question | the call | -|---|---| -| did this run go through unvalidated code? | `entered()` — and the summary at exit says so anyway | -| what is unfinished here? | `experimental(M)` | -| which public names are undescribed? | `audit(M).undocumented` — should be empty | -| is dropping this name breaking? | `compare(old_snapshot, M)` — see [`isbreaking`](@ref) | +Only a definition **with a body** carries a flag. A mark written as a name list, or attached to a +struct, a const or a module, is a declaration — queryable, audited and analysable, but not observed +at run time. See [`@experimental`](@ref) for the form-by-form table. # What this is not @@ -50,18 +52,19 @@ See [`@experimental`](@ref) for the form-by-form table. public and experimental, or public and settled; those are independent axes. * **Not deprecation.** `@deprecate` points the other way — a settled name on its way out. * **Not type stability.** Unrelated axis, different tooling. - * **Not instrumentation.** The body gains one short-circuit read, nothing more: no counter, no - logging call, no allocation. What it can answer is "was this entered at all", never how often. - * **Not a documentation generator.** The prose belongs to the author; this only ever checks - whether prose exists. + * **Not a documentation generator.** The prose belongs to the author; [`audit`](@ref) only ever + checks whether prose exists, never whether it is any good. # Scope of the check [`audit`](@ref) compares `names(M)` — exported *and* `public` names — against two independent accounts: a docstring, and a mark. They are not alternatives; the docstring is owed either way. -It sees **names**, not signatures and not prose quality. A public name with a docstring reading -"TODO" is accounted for; a settled name whose method signature changed under it is invisible here. -See [`compare`](@ref) for the same limit on the release side. +Because `names(M)` cannot see a method a package contributed to somebody else's generic, the audit +also reports [`contributed_methods`](@ref) — for a package whose surface *is* such methods, a clean +name audit reports nothing while covering nothing. + +See [`compare`](@ref) for the same limit on the release side, and [`compare_methods`](@ref) for the +finer unit. """ module ExperimentalAPI @@ -69,24 +72,60 @@ using TOML export @experimental -public Mark, Audit, Diff -public experimental, isexperimental, mark, isdocumented -public Entry, entered, marked_modules, detecting, summary_text -public surface, stable, audit -public snapshot, read_snapshot, write_snapshot, compare, isbreaking -public test_surface - -include("mark.jl") # the Mark record, the per-module registry, and @experimental -include("detect.jl") # which marked definitions a run entered, and the summary at exit -include("query.jl") # reading a module's marks back out -include("audit.jl") # the public surface, and the names neither account covers -include("release.jl") # a snapshot of the covenant, and what a diff of two of them means +# Declaring +public Mark, mark, marks, marks_on, mark_method!, isexperimental, isnamewide +public experimental, experimental_methods, superseded_marks, marks_without_exit + +# Observing — what a run went through +public Probe, Entry, entered, marked_modules, probes, detecting, summary_text +public overhead_when_detecting +public Hit, Record, Attribution, TimingBackend, timing_backend +public record, recording, merge_records, attribute, experimental_fraction +public write_record, read_record, assert_clean + +# Analysing — what code could reach +public Reach, + Reached, Unresolved, reach, reach_script, verdict, isclean, combine, dependents + +# Checking — the public surface, and the methods no name-level check can see +public Audit, audit, surface, stable, stable_methods, isdocumented, package_extensions +public own_methods, contributed_methods, unaccounted_methods, partition_holds +public extends_base +public aqua_compatible_names, test_surface + +# Verifying — how well the tests exercise what is marked +public Verification, verification, coverage, coverage_enabled, unverified, stale_marks +public flush_coverage + +# Retiring — when a mark may go +public ready_to_promote, promotable, age, stale_since, exceeds_mark_cap + +# Releasing +public Diff, MethodDiff, snapshot, read_snapshot, write_snapshot +public compare, compare_methods, compare_methods_sees_keywords, isbreaking +public stamp, stamp_versions + +# Documenting +public docstring_note, marks_markdown + +include("mark.jl") # the Mark record, the per-module registry, and @experimental +include("detect.jl") # the probe, which marked definitions a run entered, the exit summary +include("query.jl") # reading a module's marks back out, by name and by method +include("audit.jl") # the public surface, and the names and methods neither account covers +include("reach.jl") # what a caller depends on without naming it +include("record.jl") # the opt-in layer: counts, call paths, and how much of the run +include("verify.jl") # how well the tests exercise what is marked +include("lifecycle.jl") # the mark's exit +include("docsnote.jl") # the mark, in the rendered documentation +include("release.jl") # a snapshot of the covenant, and what a diff of two of them means """ - test_surface(m::Module; skip = Symbol[], outputlevel::Int = 0) -> Audit + test_surface(m::Module; skip = Symbol[], require_tracking = false, max_marks = nothing, + methods = true, outputlevel = 0) -> Audit -Assert, as a `@testset`, that every public name of `m` has a **docstring** — and that every mark -applies to a name that is actually public. +Assert, as a `@testset`, that every public name of `m` has a **docstring**, that every mark applies +to a name that is actually public, and that every method `m` contributed to another module's +generic is accounted for. A mark is not an alternative to prose. `@experimental` records that a shape is unsettled, which is never a reason to say nothing about what the name does, so a marked-but-undocumented name fails @@ -102,9 +141,12 @@ using MyPackage, ExperimentalAPI, Test ExperimentalAPI.test_surface(MyPackage) ``` -`skip` is for adopting this on a package that already has a backlog: the listed names are allowed -to have no docstring. **A stale entry fails the test** — a name in `skip` that has since been -documented or removed is reported, so the list can only shrink. +| keyword | | +|---|---| +| `skip` | names allowed to have no docstring. **A stale entry fails** — a name that has since been documented or removed is reported, so the list can only shrink | +| `require_tracking` | every mark must say where its shape is being decided | +| `max_marks` | the ratchet: a number in the repository that can be lowered and not raised | +| `methods` | run the method-level half, which is the expensive one | Returns the [`Audit`](@ref) on the normal return path whether the testset passed or not. `outputlevel ≥ 1` also prints it. diff --git a/src/audit.jl b/src/audit.jl index 69da7a2..a150c3b 100644 --- a/src/audit.jl +++ b/src/audit.jl @@ -1,10 +1,14 @@ # The check. Everything above this file is material; this is the part that turns a marker into # something that can fail. # -# The whole audit is one set difference: public surface, minus the names with a docstring, minus -# the names with a mark. What is left is the set of names a caller can reach and nobody has said -# anything about — and it is exactly the set that a package cannot leave non-empty once this runs -# in CI. +# The name half of the audit is one set difference: public surface, minus the names with a +# docstring, minus the names with a mark. What is left is the set of names a caller can reach and +# nobody has said anything about — and it is exactly the set that a package cannot leave non-empty +# once this runs in CI. +# +# The method half exists because `names(m)` cannot see a method a package contributed to somebody +# else's generic, and for a package whose surface IS such methods — `fetch(model, quantity)` with +# 570 of them — a clean name audit reports nothing while covering nothing. """ surface(m::Module) -> Vector{Symbol} @@ -21,26 +25,72 @@ surface(m::Module) = sort!(filter(!=(nameof(m)), names(m))) """ stable(m::Module) -> Vector{Symbol} -The public surface of `m` minus the names declared [`@experimental`](@ref) — the names whose -removal or renaming *is* a breaking change. +The public surface of `m` minus the names that are *wholly* [`@experimental`](@ref) — the names +whose removal or renaming is a breaking change. The covenant, in other words. [`snapshot`](@ref) writes this down so two releases can be compared; [`isbreaking`](@ref) is what reads the comparison. + +"Wholly" is the load-bearing word. A name is out of the covenant when a whole-name declaration +covers it, or when every method behind it is marked. A name with four methods of which one is +marked stays **in** — the author declared one dispatch path unsettled, which is not a licence to +remove the name. See [`stable_methods`](@ref) for the finer unit. """ -stable(m::Module) = setdiff(surface(m), Symbol[mk.name for mk in experimental(m)]) +function stable(m::Module) + marked = Set(mk.name for mk in experimental(m)) + return filter(n -> !(n in marked && _wholly_experimental(m, n)), surface(m)) +end + +""" + stable_methods(m::Module) -> Vector{Method} + +The methods `m` defines that carry no mark — the covenant at the unit a call site actually +reaches. + +The name-level [`stable`](@ref) cannot see this: `fetch(::Ising, ::Energy)` and +`fetch(::Heisenberg, ::Energy)` are one name and two promises. Covers both `m`'s own generics and +the methods it contributed to somebody else's; see [`contributed_methods`](@ref). +""" +function stable_methods(m::Module) + return filter(!isexperimental, own_methods(m)) +end + +function _wholly_experimental(m::Module, name::Symbol) + ms = marks(m, name) + isempty(ms) && return false + any(isnamewide, ms) && return true + isdefined(m, name) || return true + v = try + getglobal(m, name) + catch + return true + end + ml = try + methods(v) + catch + return false + end + isempty(ml) && return false + return all(isexperimental, ml) +end """ isdocumented(m::Module, name::Symbol) -> Bool + isdocumented(m::Method) -> Bool -Whether `name` has a docstring. +Whether `name`, or the method `m`, has a docstring. A re-exported name counts: the lookup follows the binding to the module the name actually comes from, so a package that puts a dependency's name on its own surface is not asked to re-document it. [`audit`](@ref) still reports those separately as `foreign`, because who owns a name and who documented it are different questions. -This answers *whether prose exists*, never whether it is any good. A docstring reading `"TODO"` -is documented as far as this package is concerned. +Docstrings are keyed by signature, so the method form is answerable and is not the same question: +a documented generic with an undocumented method is normal, and for a package whose surface is +methods on somebody else's generic it is the only question there is. + +This answers *whether prose exists*, never whether it is any good. A docstring reading `"TODO"` is +documented as far as this package is concerned. """ function isdocumented(m::Module, name::Symbol) # `Docs.hasdoc` is public API — `public`, and exported from `Base.Docs`. Its set-valued @@ -51,6 +101,55 @@ function isdocumented(m::Module, name::Symbol) return Base.Docs.hasdoc(m, name) end +function isdocumented(m::Method) + id = _ftype_identity(_sig_ftype(m.sig)) + owner = id === nothing ? m.module : id.mod + name = id === nothing ? m.name : id.name + # Only the module that WROTE the method is asked. `Docs` files a docstring under the + # binding's module but in the *writing* module's table, so this is the right table — and + # looking in the owner's as well would let the generic's own docstring, whose key is + # `Tuple{Any, Any}`, account for all 570 methods anybody ever contributed to it. + d = try + Base.Docs.meta(m.module; autoinit=false) + catch + nothing + end + d === nothing && return false + b = Base.Docs.Binding(owner, name) + haskey(d, b) || return false + args = _argument_tuple(m.sig) + args === nothing && return false + for sig in keys(d[b].docs) + # `Union{}` is the key `@doc` uses for a docstring attached to the binding rather than to + # any one signature. `Docs` keys the rest by ARGUMENT types, without the function type. + sig === Union{} && continue + sig isa Type && args <: sig && return true + end + return false +end + +function _argument_tuple(@nospecialize(sig)) + s = Base.unwrap_unionall(sig) + (s isa DataType && s <: Tuple && !isempty(s.parameters)) || return nothing + return try + Tuple{s.parameters[2:end]...} + catch + nothing + end +end + +# Whether a mark says anything about `m`'s own public surface. A mark that attached to a +# signature is asked about the generic it extends, not about whether the name happens to be bound +# here: `using ..Upstream` leaves no binding for `fetch_value`, and reading that absence as "ours" +# would report every contributed method as a dangling promise. +function _is_surface_claim(m::Module, mk::Mark) + if mk.sig !== nothing + id = _ftype_identity(_sig_ftype(mk.sig)) + id === nothing || return _is_submodule(id.mod, m) + end + return _is_own(m, mk.name) +end + function _owner(m::Module, name::Symbol) return isdefined(m, name) ? Base.binding_module(m, name) : nothing end @@ -67,13 +166,163 @@ function _is_own(m::Module, name::Symbol) end end +""" + own_methods(m::Module) -> Vector{Method} + +Every method `m` defines, on its own generics and on other modules' alike. + +Julia indexes methods by generic, not by module, so this is a search: the generics bound in `m` +(including the ones it imported to extend), the generics `m`'s own marks name, and the +exported-or-`public` names of every loaded module. That last set is what catches +`Base.show(io, ::Widget)`, written with a qualified name that leaves no binding in `m`. + +!!! note "The one gap, stated" + A method contributed to a generic that is neither bound in `m` nor exported-or-`public` + anywhere — `Base.SomeInternal.f(::Widget) = …` — is not found. It is also not part of any + surface anyone can be told about, which is why the search stops there rather than walking + every binding of every loaded module. +""" +function own_methods(m::Module) + out = Method[] + seen = Set{Method}() + for f in _generic_candidates(m) + ml = try + methods(f) + catch + continue + end + for mm in ml + mm.module === m && !(mm in seen) && (push!(seen, mm); push!(out, mm)) + end + end + return sort!(out; by=mm -> (string(mm.name), string(mm.sig))) +end + +function _generic_candidates(m::Module) + seen = Set{UInt}() + out = Any[] + add(v) = + if (v isa Function || v isa Type) && !(objectid(v) in seen) + push!(seen, objectid(v)) + push!(out, v) + end + for n in names(m; all=true, imported=true) + isdefined(m, n) || continue + v = try + getglobal(m, n) + catch + continue + end + add(v) + end + if _has_registry(m) + for mk in _registry_of(m) + mk.sig === nothing && continue + ft = _sig_ftype(mk.sig) + ft isa DataType && isdefined(ft, :instance) && add(ft.instance) + end + end + for v in _public_generics() + add(v) + end + return out +end + +# The exported-or-`public` callables of every loaded module, computed once per world age. The +# result is a few thousand entries and the scan over it costs tens of milliseconds; recomputing it +# per `audit` call would make the audit the slowest thing in a test suite. +const _PUBLIC_GENERICS = Ref{Tuple{UInt64,Vector{Any}}}((typemax(UInt64), Any[])) + +function _public_generics() + w = Base.get_world_counter() + cached = _PUBLIC_GENERICS[] + cached[1] == w && return cached[2] + seen = Set{UInt}() + out = Any[] + for mod in values(Base.loaded_modules) + for n in names(mod) + isdefined(mod, n) || continue + v = try + getglobal(mod, n) + catch + continue + end + (v isa Function || v isa Type) || continue + objectid(v) in seen && continue + push!(seen, objectid(v)) + push!(out, v) + end + end + _PUBLIC_GENERICS[] = (w, out) + return out +end + +""" + contributed_methods(m::Module) -> Vector{Method} + +The methods `m` defines on generics it does not own. + +`audit`'s `foreign` bucket says "this name is bound elsewhere, so documenting it is not our +problem". A method we wrote on such a name is the exact opposite: it is entirely our problem, and +it is invisible to `names(m)`. +""" +function contributed_methods(m::Module) + return filter(own_methods(m)) do mm + id = _ftype_identity(_sig_ftype(mm.sig)) + return id === nothing ? false : !_is_submodule(id.mod, m) + end +end + +function _is_submodule(o::Module, m::Module) + while true + o === m && return true + p = parentmodule(o) + p === o && return false + o = p + end +end + +""" + extends_base(mm::Method) -> Bool + +Whether `mm` extends a generic owned by `Base` or `Core`. + +The line [`test_surface`](@ref) draws by default when it asks whether a contributed method is +accounted for. A method on `Base.show` or `Base.==` implements a protocol whose documentation is +Base's, and requiring a docstring on each of them trains a project to turn the whole check off. A +method on another package's generic is a downstream extension only this package can describe — +`fetch_value(::Heisenberg, ::Energy)` is surface in a way `show(io, ::Audit)` is not. + +Stated as a rule about **who owns the generic**, not as a list of names: a list of interface +functions is not closed under the ones Julia adds next. +""" +function extends_base(mm::Method) + id = _ftype_identity(_sig_ftype(mm.sig)) + id === nothing && return false + r = Base.moduleroot(id.mod) + return r === Base || r === Core +end + +""" + unaccounted_methods(m::Module) -> Vector{Method} + +The methods `m` contributed to other modules' generics that carry **neither** a docstring nor a +mark. + +The method-level twin of `audit(m).unaccounted`, and the finding for a package whose surface is +methods rather than names. A name-level audit reports `unaccounted = []` for such a package while +having looked at none of them. +""" +function unaccounted_methods(m::Module) + return filter(mm -> !isdocumented(mm) && !isexperimental(mm), contributed_methods(m)) +end + """ Audit -What [`audit`](@ref) found. Every field is a sorted `Vector{Symbol}`, and every name in -`surface` appears in exactly one of `foreign`, `documented`, `declared`, `unaccounted` — -except that a name may be both `documented` and `declared`, in which case it is counted as -`documented`. +What [`audit`](@ref) found. The name fields are sorted `Vector{Symbol}`, and every name in +`surface` appears in exactly one of `foreign`, `documented`, `unaccounted` or +`declared`-and-not-`documented` — the partition [`partition_holds`](@ref) checks. | field | | |---|---| @@ -82,8 +331,14 @@ except that a name may be both `documented` and `declared`, in which case it is | `foreign` | public here, but bound in another package; not this module's to declare | | `documented` | has a docstring | | `declared` | has an [`@experimental`](@ref) mark | +| `undocumented` | no docstring, mark or not | | `unaccounted` | **neither** — the finding | | `dangling` | marked experimental but not public; a mark that promises nothing | +| `tracking` | name → where its shape is being decided, for the marks that say | +| `contributed_methods` | methods this module wrote on other modules' generics | +| `undocumented_methods` | of those, the ones with no docstring | +| `unaccounted_methods` | of those, the ones with neither a docstring nor a mark | +| `extensions` | this module's loaded package extensions, which are audited separately | `unaccounted` is the one a test asserts is empty. `dangling` needs no external oracle to be wrong: it is the module contradicting itself. @@ -97,12 +352,18 @@ struct Audit undocumented::Vector{Symbol} unaccounted::Vector{Symbol} dangling::Vector{Symbol} + tracking::Dict{Symbol,String} + contributed_methods::Vector{Method} + undocumented_methods::Vector{Method} + unaccounted_methods::Vector{Method} + extensions::Vector{Module} end """ - audit(m::Module) -> Audit + audit(m::Module; methods = true) -> Audit -Report the public names of `m` that are missing prose, a mark, or both. +Report the public names of `m` that are missing prose, a mark, or both — and the methods it +contributed to other modules' generics, which no name-level check can see. ```julia julia> audit(Pinax).undocumented @@ -127,21 +388,26 @@ neither does, and [`test_surface`](@ref) fails on it. Also reports `dangling`: marks on names that are not public. That check needs no reference implementation to be right, because the module is disagreeing with itself. +`methods = false` skips the method-level search, which is the expensive half — it scans the +generics of every loaded module. The name-level answer is unchanged by it. + # What this cannot see - * **Signatures.** A name that stays present while its arguments change is invisible here. + * **Signatures of its own names.** A name that stays present while its arguments change is + invisible here; `compare_methods` is the release-side answer. * **Prose quality.** A docstring exists or it does not; [`isdocumented`](@ref) reads no further. - * **Methods on other packages' functions.** They are not in `names(m)` and never will be. - * **Names public only inside an extension**, which is a separate module. Avoidable: declare the - function and its docstring in the parent and let the extension add only the method, which is - what this package does for [`test_surface`](@ref). + * **Names public only inside an extension**, which is a separate module — reported in + `extensions` and audited by passing it to `audit` in its own right. Avoidable altogether: + declare the function and its docstring in the parent and let the extension add only the + method, which is what this package does for [`test_surface`](@ref). See [`test_surface`](@ref) to run this as a test, and [`snapshot`](@ref) to carry the result into a release decision. """ -function audit(m::Module) +function audit(m::Module; methods::Bool=true) surf = surface(m) - marked = Set(mk.name for mk in experimental(m)) + all_marks = experimental(m) + marked = Set(mk.name for mk in all_marks) foreign = Symbol[] documented = Symbol[] @@ -161,12 +427,73 @@ function audit(m::Module) n in marked || push!(unaccounted, n) end end - dangling = sort!(collect(setdiff(marked, surf))) + # A mark on a generic another module owns is the foreign-method form — `Base.show(io, ::T)` + # — and it promises nothing about THIS module's surface, so it cannot dangle here; + # `contributed_methods` is where it is accounted for. A mark on something of our own that is + # not public does dangle, whether or not it carries a signature. + dangling = sort!( + unique( + mk.name for mk in all_marks if _is_surface_claim(m, mk) && !(mk.name in surf) + ), + ) + tracking = Dict{Symbol,String}( + mk.name => mk.tracking for mk in all_marks if mk.tracking !== nothing + ) + + contributed = methods ? contributed_methods(m) : Method[] + undoc_methods = filter(!isdocumented, contributed) + unacc_methods = filter(mm -> !isexperimental(mm), undoc_methods) return Audit( - m, surf, foreign, documented, declared, undocumented, unaccounted, dangling + m, + surf, + foreign, + documented, + declared, + undocumented, + unaccounted, + dangling, + tracking, + contributed, + undoc_methods, + unacc_methods, + package_extensions(m), ) end +""" + partition_holds(a::Audit) -> Bool + +Whether every public name landed in exactly one bucket. + +`foreign`, `documented`, `unaccounted`, and the declared-but-undocumented remainder must cover +`surface` exactly once. The invariant is stated in [`Audit`](@ref) and is easy to break from the +outside — every new field is one more thing a reader assumes partitions the surface — so it is +checkable rather than only written down. +""" +function partition_holds(a::Audit) + parts = vcat(a.foreign, a.documented, a.unaccounted, setdiff(a.declared, a.documented)) + return sort(parts) == a.surface && length(unique(parts)) == length(parts) +end + +""" + aqua_compatible_names(m::Module) -> Vector{Symbol} + +The names `Aqua.test_undocumented_names` reports and this package's [`audit`](@ref) accounts for. + +The two tools ask different questions, and both are legitimate. Aqua's is two-valued: a public +name has a docstring or it fails. `audit` has a third answer — a mark — so a name that is +declared unsettled is still `undocumented` but is not `unaccounted`. + +This is the difference, computed, so a project running both can see exactly which names it would +have to argue about. **Empty means the two agree**, which is the state a package should be aiming +for: a mark records that a shape is unsettled, and that is never a reason to say nothing about +what the name does. +""" +function aqua_compatible_names(m::Module) + a = audit(m; methods=false) + return sort!(collect(intersect(Set(a.declared), Set(a.undocumented)))) +end + function Base.show(io::IO, a::Audit) return print( io, @@ -208,6 +535,26 @@ function Base.show(io::IO, ::MIME"text/plain", a::Audit) ) _print_names(io, a.dangling) end + if !isempty(a.contributed_methods) + println( + io, + " contributed methods ", + lpad(length(a.contributed_methods), 4), + " (on other modules' generics)", + ) + print(io, " unaccounted methods ", lpad(length(a.unaccounted_methods), 4)) + if isempty(a.unaccounted_methods) + println(io) + else + println(io, " ← neither documented nor @experimental") + for mm in a.unaccounted_methods + println(io, " ", mm.name, mm.sig, " @ ", mm.file, ":", mm.line) + end + end + end + isempty(a.extensions) || println( + io, " extensions ", lpad(length(a.extensions), 4), " (audited separately)" + ) return nothing end diff --git a/src/detect.jl b/src/detect.jl index 1f52ada..059f7d4 100644 --- a/src/detect.jl +++ b/src/detect.jl @@ -2,41 +2,215 @@ # # Scope: presence, not counts, and only for definitions with a body. A mark written as a name list, # or attached to a struct, const, module or macro, is a declaration only — nothing observes it. +# +# The one statement the macro puts in a marked body reads a single field and writes it once: +# measured at 1.03x on one thread and 0.985x on eight, over 10M calls of a numeric body. A flag +# written once and only read afterwards stops dirtying the cache line, which a counter (3.76x at +# eight threads, and losing 40% of its increments to races unless atomic) does not. +# +# `record` reaches the same statement without changing it: opening a recording clears every +# probe's flag, so the short-circuit fails and the write side runs on every call. The cost of +# counting is paid only inside `record`, and the fast path is one field load either way. + +# Padding, in Int64 slots, between one thread's counter and the next. A cache line is 64 bytes on +# every platform this runs on; two threads sharing one would serialise on the store. +const _COUNTER_STRIDE = 8 -# One flag per marked NAME, in the marked module, next to its registry. Named rather than -# gensym'd for the same reason the registry is. +# How many distinct backtraces one probe keeps while recording, and how many times it will look. +# A backtrace costs microseconds, so capturing one per call would dominate any run long enough to +# be worth recording. The paths a marked definition is reached by are few and repeat, so the +# attempt budget is what bounds the cost: without it, a definition reached by three paths would +# keep paying for a backtrace on every one of ten million calls, having found its third path in +# the first microsecond. +const _TRACE_CAP = 64 +const _TRACE_ATTEMPTS = 256 + +""" + Probe + +The one-field flag `@experimental` puts in a marked body, and the counter [`record`](@ref) reads. + +Not part of the public surface — it is named rather than gensym'd only so that +`MyModule.__EXPERIMENTAL_API_ENTERED_energy__` is greppable when a query result surprises someone. + +`entered` is the whole default layer: read on every call, written on the first. The remaining +fields are untouched unless a recording is open. +""" +mutable struct Probe + entered::Bool + const mod::Module + const name::Symbol + # Recording state. `hits` is padded per thread; `traces` is bounded and guarded. + hits::Vector{Int64} + traces::Vector{Vector{Union{Ptr{Nothing},Base.InterpreterIP}}} + attempts::Int64 + const lock::ReentrantLock +end + +function Probe(mod::Module, name::Symbol) + return Probe( + false, + mod, + name, + Int64[], + Vector{Union{Ptr{Nothing},Base.InterpreterIP}}[], + 0, + ReentrantLock(), + ) +end + +# The fast path, and the only thing a marked body does when nothing is recording. +Base.getindex(p::Probe) = p.entered + +# The write side. Reached once per process when nothing is recording, and on every call while a +# recording is open — which is what makes counting cost nothing outside `record`. +# +# `@noinline` for two reasons, and neither is speed on this path. It keeps the marked body small, +# so the fast path is a load and a branch over a call; and it makes the call a real frame, so the +# backtrace taken underneath it resolves to the marked definition rather than to whatever the +# optimiser left at that address. +@noinline function Base.setindex!(p::Probe, v::Bool) + if _RECORDING[] + _hit!(p) + else + p.entered = v + end + return v +end + +# Read by the write side only, so the fast path never sees it. +const _RECORDING = Ref(false) + +@noinline function _hit!(p::Probe) + tid = Threads.threadid() + h = p.hits + i = tid * _COUNTER_STRIDE + if i <= length(h) + @inbounds h[i] += 1 + else + # A thread that did not exist when the recording opened — the interactive pool grows, and + # an `nthreads()`-sized vector would throw here rather than count. + @lock p.lock begin + _resize_hits!(p, tid) + @inbounds p.hits[i] += 1 + end + end + # Racy on purpose: `attempts` is a budget, not a count, and a lock on the fast path of the + # recorder would cost more than the backtraces it saves. + if _CAPTURE_PATHS[] && p.attempts < _TRACE_ATTEMPTS && length(p.traces) < _TRACE_CAP + p.attempts += 1 + _capture_trace!(p) + end + return nothing +end + +# Whether the open recording asked for call paths. Read on the recorder's slow path only. +const _CAPTURE_PATHS = Ref(true) + +function _resize_hits!(p::Probe, tid::Int) + need = (max(tid, Threads.maxthreadid()) + 2) * _COUNTER_STRIDE + length(p.hits) >= need && return nothing + h = zeros(Int64, need) + copyto!(h, p.hits) + p.hits = h + return nothing +end + +# The address list only. Resolving it to names here would mean walking the debug info while the +# sampling profiler may be in its signal handler doing the same thing, and the two take the same +# lock: `record`'s own paths would deadlock against its own timing. `_trace_names` is called +# once, at the end of the block, with the sampler stopped. +@noinline function _capture_trace!(p::Probe) + bt = backtrace() + @lock p.lock begin + length(p.traces) < _TRACE_CAP && !any(==(bt), p.traces) && push!(p.traces, bt) + end + return nothing +end + +# The caller chain, innermost first, with this package's own frames dropped: the probe, the +# recorder and `record` itself are in every path and in none of the user's code. +function _trace_names(bt) + out = Symbol[] + for frame in stacktrace(bt, false) + frame.from_c && continue + _frame_module(frame) === ExperimentalAPI && continue + push!(out, frame.func) + end + return out +end + +# `StackFrame.linfo` is a `CodeInstance` on 1.12, a `MethodInstance` before it, `nothing` for an +# inlined frame, and a `CodeInfo` for top-level code. Only the first two lead to a module — an +# inlined frame is kept, which is right: an inlined caller is still a caller. +function _frame_module(frame) + li = frame.linfo + li isa Core.CodeInstance && (li = li.def) + li isa Core.MethodInstance || return nothing + d = li.def + return d isa Method ? d.module : (d isa Module ? d : nothing) +end + +_probe_count(p::Probe) = isempty(p.hits) ? 0 : sum(p.hits) + +function _arm!(p::Probe, slots::Int) + @lock p.lock begin + h = zeros(Int64, (slots + 2) * _COUNTER_STRIDE) + length(p.hits) == length(h) ? fill!(p.hits, 0) : (p.hits = h) + empty!(p.traces) + p.attempts = 0 + end + return nothing +end + +# One probe per marked NAME, in the marked module, next to its registry. _flag_name(n::Symbol) = Symbol("__EXPERIMENTAL_API_ENTERED_", n, "__") function _flag(m::Module, n::Symbol) s = _flag_name(n) isdefined(m, s) || return nothing f = getglobal(m, s) - return f isa Base.RefValue{Bool} ? f : nothing + return f isa Probe ? f : nothing end -# What the macro puts in the body. Measured on 10M calls of a numeric body: 1.03x on one thread -# and 0.985x on eight, because after the first call it is a read of a cache line nobody writes. -# An unconditional store, a counter, or anything that logs is not affordable here — see -# `test/spec/README.md`. -_probe(flag) = :($flag[] || ($flag[] = true)) +""" + probes(m::Module) -> Vector{Probe} + probes() -> Vector{Probe} -function _is_signature(x) - return x isa Expr && ( - x.head === :call || - ((x.head === :where || x.head === :(::)) && _is_signature(x.args[1])) - ) +Every [`Probe`](@ref) a module carries — one per marked definition that has a body. + +The recording layer arms and reads these; nothing else should need them. Exposed because +`record`'s cost model is only checkable by someone who can see how many probes there are. +""" +function probes(m::Module) + out = Probe[] + _has_registry(m) || return out + for mk in _registry_of(m) + p = _flag(m, mk.name) + p === nothing || (p in out || push!(out, p)) + end + return out end +probes() = reduce(vcat, (probes(m) for m in marked_modules()); init=Probe[]) + +# What the macro puts in the body: one statement, a read that writes only on the first call. +_probe(flag) = :($flag[] || ($flag[] = true)) + # Returns the definition with the probe spliced in, or `nothing` if this form has no body to -# instrument. `@generated` and other macro-wrapped forms are refused rather than guessed at: the -# body of a generated function returns an expression, so a probe there would be spliced into the -# generated code instead of running. -function _instrument(def, flag) +# instrument. +# +# The `LineNumberNode` is the declaration's own, and it is load bearing rather than cosmetic. The +# write side is a cold branch, so the optimiser is free to sink it to the end of the function; +# without a location of its own it inherits whichever statement happens to be next, and a +# backtrace taken inside it then resolves to that statement's inlining context instead of to the +# marked definition. `record`'s call paths are built out of exactly that. +function _instrument(def, flag, src::LineNumberNode) def isa Expr || return nothing (def.head === :function || def.head === :(=)) || return nothing _is_signature(def.args[1]) || return nothing length(def.args) == 2 || return nothing - return Expr(def.head, def.args[1], Expr(:block, _probe(flag), def.args[2])) + return Expr(def.head, def.args[1], Expr(:block, src, _probe(flag), def.args[2])) end """ @@ -46,7 +220,7 @@ One marked definition that the current run entered, as reported by [`entered`](@ `count` is always `nothing`. The default layer knows *whether* a definition was entered, never how often — a per-call counter costs 3.76x on eight threads and loses 40% of its increments to races -unless it is atomic. Counting is the opt-in layer's job. +unless it is atomic. Counting is [`record`](@ref)'s job, and it is opt-in for that reason. """ struct Entry mod::Module @@ -85,10 +259,15 @@ Only definitions with a body are observed; see [`@experimental`](@ref) for which """ function entered(m::Module) out = Entry[] - isdefined(m, MARKS_BINDING) || return out + _has_registry(m) || return out + seen = Set{Symbol}() for mk in _registry_of(m) + mk.name in seen && continue f = _flag(m, mk.name) - f !== nothing && f[] && push!(out, Entry(m, mk.name, mk.reason)) + if f !== nothing && f[] + push!(seen, mk.name) + push!(out, Entry(m, mk.name, mk.reason)) + end end return out end @@ -109,18 +288,29 @@ would be written while the *marked* package is precompiled, and so would be abse image. Same constraint that puts the marks themselves in the marked module. """ function marked_modules() + w = Base.get_world_counter() + cached = _MARKED_MODULES[] + cached[1] == w && return cached[2] out = Module[] seen = Set{Module}() for root in values(Base.loaded_modules) _walk_modules!(out, seen, root) end + _MARKED_MODULES[] = (w, out) return out end +# Cached per world age. The walk is over every binding of every loaded module, and `record` asks +# for it twice per block — with a large dependency tree loaded that is the most expensive thing +# in a recording that counts a hundred calls. Keying on the world counter is exact rather than +# approximate: a module gains a registry only by defining a `const`, and defining one advances +# the counter. +const _MARKED_MODULES = Ref{Tuple{UInt64,Vector{Module}}}((typemax(UInt64), Module[])) + function _walk_modules!(out::Vector{Module}, seen::Set{Module}, m::Module) (m in seen || m in _NOT_WALKED) && return nothing push!(seen, m) - isdefined(m, MARKS_BINDING) && push!(out, m) + _has_registry(m) && push!(out, m) for n in names(m; all=true) isdefined(m, n) || continue v = try @@ -143,9 +333,27 @@ On by default, because a user who never asks is exactly the one who needs to be `atexit` hook is registered at load time, so a later assignment has no effect. Detection itself is not a switch: the flag is in the definition and costs nothing to leave on. +See [`overhead_when_detecting`](@ref) for the figure that justifies that sentence. """ detecting() = get(ENV, "EXPERIMENTALAPI_SUMMARY", "1") != "0" +""" + overhead_when_detecting() -> Float64 + +The measured cost of the default layer, as a fraction of the unmarked body's run time. + +`0.03`. Not computed at call time: it is the figure from the measurement that chose this +mechanism — 10M calls of a realistic numeric body on Julia 1.12.2, minimum of 7–9 trials, giving +`1.03x` on one thread and `0.985x` on eight. The eight-thread figure is below one because a flag +written once and read thereafter stops dirtying its cache line. + +It is reported rather than re-measured because a wall-clock measurement on a shared CI runner is a +flake generator, and because a number that moves with the machine cannot be the thing a caller +plans against. [`record`](@ref) reports its own overhead per run, which is the opposite case: that +one depends on how often the marked code was entered. +""" +overhead_when_detecting() = 0.03 + """ summary_text() -> String summary_text(es::AbstractVector{Entry}) -> String diff --git a/src/docsnote.jl b/src/docsnote.jl new file mode 100644 index 0000000..bce7b2a --- /dev/null +++ b/src/docsnote.jl @@ -0,0 +1,91 @@ +# Getting the mark to the reader of the rendered documentation, without the author typing it +# twice. Typed twice the two drift, and the machine-readable one loses. + +""" + docstring_note(m::Module, name::Symbol) -> Union{String,Nothing} + docstring_note(mk::Mark) -> String + +The admonition a docs build should render above `name`'s docstring, or `nothing` if the name is +settled. + +Markdown, in Documenter's `!!! warning` form, carrying the reason, the version it has been +unsettled since, and the tracking link — the three things a reader needs and the author has +already written once: + +```julia +julia> println(docstring_note(MyPkg, :provisional)) +!!! warning "Experimental" + the r,s branch has no reference value + + Unsettled since v0.2.0. Tracking: . +``` + +`nothing` for a settled name is the point: a renderer that annotates everything says nothing. +The Documenter extension turns this into an `@experimental` block; nothing stops a project +splicing it in itself. +""" +function docstring_note(mk::Mark) + io = IOBuffer() + println(io, "!!! warning \"Experimental\"") + println(io, " ", mk.reason) + tail = String[] + mk.since === nothing || push!(tail, "Unsettled since v$(mk.since).") + mk.tracking === nothing || push!(tail, "Tracking: <$(mk.tracking)>.") + mk.sig === nothing || push!(tail, "Applies to `$(_signature_key(mk))`.") + if !isempty(tail) + println(io) + println(io, " ", join(tail, " ")) + end + return String(take!(io)) +end + +function docstring_note(m::Module, name::Symbol) + mk = mark(m, name) + return mk === nothing ? nothing : docstring_note(mk) +end + +""" + marks_markdown(m::Module) -> String + +Every mark in `m` as one markdown block, for a docs page that wants the list in one place. + +Sorted by name, with the reason, the version and the tracking link. A module with no marks renders +a sentence saying so rather than nothing, because "this page is empty" and "this build failed to +find anything" look identical otherwise. + +Deliberately **heading-free**. A docs builder registers heading anchors in an earlier pass than +the one that expands a block like this, so a heading spliced in here arrives after the pass that +was supposed to see it — Documenter's HTML writer asserts on exactly that. +""" +function marks_markdown(m::Module) + ms = experimental(m) + isempty(ms) && return "`$(nameof(m))` declares no experimental names.\n" + io = IOBuffer() + for mk in ms + println(io, "**`", mk.name, "`** — ", mk.reason) + println(io) + bits = String[] + mk.since === nothing || push!(bits, "since v$(mk.since)") + mk.tracking === nothing || push!(bits, "tracking: <$(mk.tracking)>") + if mk.sig === nothing + push!(bits, "covers every method of the name") + else + push!(bits, "signature: `$(_signature_key(mk))`") + end + if mk.until === nothing + push!(bits, "**no exit condition recorded**") + else + push!( + bits, + if ready_to_promote(mk) + "**exit condition met**" + else + "exit condition not yet met" + end, + ) + end + println(io, "*", join(bits, " · "), "*") + println(io) + end + return String(take!(io)) +end diff --git a/src/lifecycle.jl b/src/lifecycle.jl new file mode 100644 index 0000000..e05d0d4 --- /dev/null +++ b/src/lifecycle.jl @@ -0,0 +1,108 @@ +# The mark's exit. +# +# A mark that can only ever be added is a decoration. What makes it a work item is a stated +# condition under which it stops applying — `until=` on the declaration — and the two questions +# that condition makes answerable: may this mark go, and how long has it been standing. + +""" + ready_to_promote(m::Module, name::Symbol) -> Bool + ready_to_promote(mk::Mark) -> Bool + +Whether the declaration's own exit condition is met. + +Not "is this marked" but "may this stop being marked", answered by the thing that knows: the +`until=` predicate the author wrote next to the reason. + +```julia +@experimental( + "no reference value yet", + since = v"0.1.0", + tracking = "https://github.com/org/Pkg.jl/issues/12", + until = () -> isfile(joinpath(@__DIR__, "..", "test", "refs", "energy.toml")), + energy(β) = 2β, +) +``` + +`false` for a mark with no `until=`, always — a mark whose exit was never written down cannot be +retired mechanically, and reporting it "ready" because it happens to be covered by a test would be +inventing a criterion the author did not state. Those marks are reported separately by +[`marks_without_exit`](@ref) rather than being quietly lumped in with the ones still standing. + +A predicate that throws counts as not ready: an exit condition that cannot be evaluated has not +been met. +""" +function ready_to_promote(mk::Mark) + mk.until === nothing && return false + return try + mk.until() === true + catch + false + end +end + +function ready_to_promote(m::Module, name::Symbol) + mk = mark(m, name) + return mk === nothing ? false : ready_to_promote(mk) +end + +""" + promotable(m::Module) -> Vector{Mark} + +Every mark in `m` whose exit condition is met — the work item list, in the direction of done. +""" +promotable(m::Module) = filter(ready_to_promote, experimental(m)) + +""" + age(m::Module, name::Symbol, current::VersionNumber) -> Union{Int,Missing} + age(mk::Mark, current::VersionNumber) -> Union{Int,Missing} + +How many **breaking** releases the mark has been standing for, or `missing` if it records no +`since`. + +Counted on the axis a version bump is breaking along, which under Julia's 0.x convention is the +minor component and after 1.0 the major one. `since = v"0.1.0"` seen from `v"0.9.0"` is 8. + +`since` exists so a mark cannot quietly become permanent. This is the function that reads it; see +[`stale_since`](@ref) for the list form a CI job asks for. +""" +function age(mk::Mark, current::VersionNumber) + mk.since === nothing && return missing + s = mk.since + (s.major == 0 && current.major == 0) && return Int(current.minor) - Int(s.minor) + return Int(current.major) - Int(s.major) +end + +function age(m::Module, name::Symbol, current::VersionNumber) + mk = mark(m, name) + return mk === nothing ? missing : age(mk, current) +end + +""" + stale_since(m::Module, current::VersionNumber; releases::Int = 2) -> Vector{Mark} + +The marks that have been standing for `releases` breaking releases or more. + +The report a CI job turns into a nag. Marks with no `since` are not listed here — they are a +different finding, and [`marks_without_exit`](@ref) plus a `since`-less mark is what a package +that never intended to retire anything looks like. +""" +function stale_since(m::Module, current::VersionNumber; releases::Int=2) + out = Mark[] + for mk in experimental(m) + a = age(mk, current) + a === missing && continue + a >= releases && push!(out, mk) + end + return out +end + +""" + exceeds_mark_cap(m::Module, cap::Int) -> Bool + +Whether `m` carries more than `cap` marks. + +The ratchet, in the shape [`test_surface`](@ref)'s `skip` already has: a number checked into the +repository that can be lowered and not raised. It is a count and not a list because the list is +`experimental(m)`, and a project that wants the finer gate should assert on that instead. +""" +exceeds_mark_cap(m::Module, cap::Int) = length(experimental(m)) > cap diff --git a/src/mark.jl b/src/mark.jl index 7b38831..94ce547 100644 --- a/src/mark.jl +++ b/src/mark.jl @@ -16,12 +16,20 @@ the author has, so it travels with the mark rather than being reconstructed by a | field | | |---|---| -| `mod::Module` | the module the name is public in | +| `mod::Module` | the module that **wrote** the mark, and where it is stored | | `name::Symbol` | the marked name; a macro is stored as `Symbol("@foo")` | | `reason::String` | why it is not settled | | `since::Union{VersionNumber,Nothing}` | version the name has been experimental since | | `tracking::Union{String,Nothing}` | where the shape is being decided — an issue, PR, or URL | | `file::Symbol`, `line::Int` | where the declaration is written | +| `sig::Union{Type,Nothing}` | the signature it attached to, or `nothing` for a whole-name claim | +| `includes_constructors::Bool` | a type's constructors are covered by its mark | +| `until::Union{Function,Nothing}` | the exit condition — see [`ready_to_promote`](@ref) | + +`sig` is what makes the claim narrower than the name. A mark attached to `energy(::Numerical)` +says the *name* has something unsettled about it — [`audit`](@ref) reads it that way — while +[`reach`](@ref) resolves calls to a method and only reports the marked one. A mark with +`sig === nothing` (a name list, a `struct`, a `const`) covers every method the name has. See [`experimental`](@ref) to read them back, and [`mark`](@ref) to look one up by name. """ @@ -33,19 +41,96 @@ struct Mark tracking::Union{String,Nothing} file::Symbol line::Int + sig::Union{Type,Nothing} + includes_constructors::Bool + until::Union{Function,Nothing} # The reason is the payload, so the invariant belongs in the type: `Mark` is `public`, and # every construction route that is not the macro gets it for free here. - function Mark(mod, name, reason, since, tracking, file, line) + function Mark( + mod, + name, + reason, + since, + tracking, + file, + line, + sig=nothing, + includes_constructors=false, + until=nothing, + ) r = String(strip(reason)) isempty(r) && throw(ArgumentError("a Mark's reason may not be empty — it is the payload")) - return new(mod, name, r, since, tracking, file, line) + return new( + mod, name, r, since, tracking, file, line, sig, includes_constructors, until + ) end end +""" + Mark(mod, name, reason; since, tracking, file, line, sig, includes_constructors, until) + +Keyword form, for the fields that are usually absent. The positional form takes them in the order +of the table in [`Mark`](@ref). +""" +function Mark( + mod::Module, + name::Symbol, + reason::AbstractString; + since=nothing, + tracking=nothing, + file::Symbol=:none, + line::Int=0, + sig=nothing, + includes_constructors::Bool=false, + until=nothing, +) + return Mark( + mod, name, reason, since, tracking, file, line, sig, includes_constructors, until + ) +end + +# `until` holds a closure, and `==` on two closures of the same source is identity, so the +# default field-wise equality would report two identical declarations as different. Equality is +# over what the mark SAYS; the exit predicate is compared by whether it is present. +function Base.:(==)(a::Mark, b::Mark) + return a.mod === b.mod && + a.name === b.name && + a.reason == b.reason && + a.since == b.since && + a.tracking == b.tracking && + a.file === b.file && + a.line == b.line && + a.sig === b.sig && + a.includes_constructors == b.includes_constructors && + (a.until === nothing) == (b.until === nothing) +end + +function Base.hash(m::Mark, h::UInt) + h = hash(m.mod, h) + h = hash(m.name, h) + h = hash(m.reason, h) + h = hash(m.since, h) + h = hash(m.tracking, h) + h = hash(m.sig, h) + return hash(m.line, h) +end + +""" + isnamewide(mk::Mark) -> Bool + +Whether `mk` claims the whole name rather than one signature. + +`true` for a name list, a `struct`, a `const`, a module — the forms that carry no signature. +A mark attached to a definition knows which method it created and is therefore narrower; see +[`stable`](@ref) for where the difference is load-bearing. +""" +isnamewide(mk::Mark) = mk.sig === nothing + function Base.show(io::IO, m::Mark) print(io, "Mark(", m.mod, ".", m.name, ", ", repr(m.reason)) + m.sig === nothing || print(io, ", sig=", m.sig) m.since === nothing || print(io, ", since=v\"", m.since, "\"") m.tracking === nothing || print(io, ", tracking=", repr(m.tracking)) return print(io, ")") @@ -54,13 +139,16 @@ end function Base.show(io::IO, ::MIME"text/plain", m::Mark) println(io, m.mod, ".", m.name, " — experimental") println(io, " reason: ", m.reason) + m.sig === nothing || println(io, " signature: ", m.sig) m.since === nothing || println(io, " since: v", m.since) m.tracking === nothing || println(io, " tracking: ", m.tracking) + m.until === nothing || println(io, " until: an exit condition is recorded") return print(io, " declared: ", m.file, ":", m.line) end -# Named rather than gensym'd, so it is greppable when a query result surprises someone. +# Named rather than gensym'd, so they are greppable when a query result surprises someone. const MARKS_BINDING = :__EXPERIMENTAL_API_MARKS__ +const SUPERSEDED_BINDING = :__EXPERIMENTAL_API_SUPERSEDED__ # Created by code the macro emits into the marked module, not by `Core.eval` from here: Julia # 1.12 rejects reading a binding created earlier in the same top-level statement, which is what a @@ -77,12 +165,27 @@ function _registry_of(m::Module) return v end -# Replaces rather than appends, so re-including a file cannot duplicate a name. +_has_registry(m::Module) = isdefined(m, MARKS_BINDING) + +# The superseded log is created on the first replacement rather than emitted by the macro: most +# modules never supersede a mark, and an empty binding in every marked module would be noise in +# `names(m; all=true)`. `Core.eval` is safe here because the value is used through the return +# value, never by reading the binding back in the same world age. +function _superseded_registry!(m::Module) + isdefined(m, SUPERSEDED_BINDING) && return getglobal(m, SUPERSEDED_BINDING) + return Core.eval(m, Expr(:const, Expr(:(=), SUPERSEDED_BINDING, Mark[]))) +end + +# Replaces rather than appends, so re-including a file cannot duplicate a declaration. The key is +# the pair (name, signature): one name may carry several method-level marks, and each of them is +# a separate claim. function _mark!(reg::Vector{Mark}, mk::Mark) - i = findfirst(x -> x.name === mk.name, reg) + i = findfirst(x -> x.name === mk.name && x.sig === mk.sig, reg) if i === nothing push!(reg, mk) else + old = reg[i] + old == mk || push!(_superseded_registry!(mk.mod), old) reg[i] = mk end return mk @@ -91,7 +194,7 @@ end """ @experimental "reason" definition @experimental "reason" name₁ name₂ … - @experimental "reason" since=v"0.4.0" tracking="…" definition + @experimental "reason" since=v"0.4.0" tracking="…" until=(() -> …) definition Declare that a public name is not settled, and say why. @@ -116,46 +219,71 @@ list of names** already defined elsewhere: ) ``` -Optional `since=` and `tracking=` come between the reason and the subject. `tracking` is what -turns a mark into something a reader can act on: the issue or PR where the shape is being -decided. +Optional keywords come between the reason and the subject: + +| keyword | | +|---|---| +| `since=v"0.4.0"` | the version the name has been unsettled since — read by [`age`](@ref) | +| `tracking="…"` | the issue or PR where the shape is being decided | +| `until=() -> …` | the **exit condition**: what would discharge the reason. See [`ready_to_promote`](@ref) | + +`tracking` is what turns a mark into something a reader can act on. `until` is what turns it into +a work item: a mark with no stated exit can be added but never mechanically retired, and +[`marks_without_exit`](@ref) reports those. + +Attaches to `function`, short-form `f(x) = …`, a callable object `(c::C)(x) = …`, a method on +another module's generic `Base.show(io, ::T) = …`, `struct`, `mutable struct`, `abstract type`, +`primitive type`, `macro` (recorded as `Symbol("@name")`), `const`, plain assignment, and a +definition wrapped in `@inline`, `@noinline`, `@generated`, `@propagate_inbounds`, +`@assume_effects` or `Base.@kwdef`. -Attaches to `function`, short-form `f(x) = …`, `struct`, `mutable struct`, `abstract type`, -`primitive type`, `macro` (recorded as `Symbol("@name")`), `const`, and plain assignment. Anything else — a `module` (which Julia requires as a direct top-level statement, so it cannot be -wrapped), a definition produced by another macro, a qualified `Base.foo(…)` method — is rejected -with a message pointing at the name-list form, because guessing which name such an expression -defines is exactly the kind of silence this package exists to remove. +wrapped), a definition produced by some other macro, a bare qualified name with no signature — is +rejected with a message pointing at the name-list form, because guessing which name such an +expression defines is exactly the kind of silence this package exists to remove. Marking is not visibility: a marked name still has to be `export`ed or declared `public` to be part of the surface. A mark on a name that is neither is reported by [`audit`](@ref) as `dangling`. +# What the mark claims + +A mark attached to a definition records the **signature** it created, so the claim is about that +method. A mark written as a name list, or attached to a `struct` or a `const`, carries no +signature and covers the whole name. + +The difference shows up in three places: [`reach`](@ref) only reports the marked method, +[`stable`](@ref) keeps a name in the covenant until *every* method behind it is marked, and +[`audit`](@ref) reads either as "the author has said something about this name". + # What gets observed -A definition **with a body** — `function` and short-form `f(x) = …`, including parametric and -return-type-annotated signatures — also gets a flag in that body, so [`entered`](@ref) can report -whether the run went through it. The flag is one short-circuit read: `1.03x` on one thread and -`0.985x` on eight, measured over 10M calls of a numeric body. +A definition **with a body** — `function` and short-form `f(x) = …`, including parametric, +callable-object and return-type-annotated signatures — also gets a flag in that body, so +[`entered`](@ref) can report whether the run went through it. The flag is one short-circuit read: +`1.03x` on one thread and `0.985x` on eight, measured over 10M calls of a numeric body. Every other form is a **declaration only** — recorded, queryable and audited, but not observed: | form | observed? | |---|---| -| `function f(x) … end`, `f(x) = …`, `f(x::T) where {T} = …`, `f(x)::R = …` | yes | +| `function f(x) … end`, `f(x) = …`, `f(x::T) where {T} = …`, `f(x)::R = …`, `(c::C)(x) = …` | yes | +| `Base.show(io::IO, ::T) = …` — a method on a foreign generic | yes | | a name list (`@experimental "…" a b c`) | no — those definitions are elsewhere and already compiled | | `struct`, `abstract type`, `primitive type`, `const`, assignment | no — nothing is *entered* | | `macro` | no | -| `@generated function` | refused outright, as any macro-produced definition is; the name-list form takes it, as a declaration | +| `@generated function` | no — the body returns an expression, so a probe there would be generated rather than run | +| `Base.@kwdef`, `@inline`, `@noinline` and the other pass-through macros | no | -One flag per marked **name**, not per method: marks are name-keyed, so two methods of a marked -name share it. +One flag per marked **name**, not per method: flags are name-keyed, so two methods of a marked +name share one. !!! note "Top level only" The mark is stored in a `const` binding in the enclosing module, so `@experimental` belongs - at module top level — the same place `export` and `public` go. + at module top level — the same place `export` and `public` go. Written inside a function + body it is refused by Julia's lowering, at the line you wrote it on. -See also [`experimental`](@ref), [`audit`](@ref), [`stable`](@ref). +See also [`experimental`](@ref), [`audit`](@ref), [`stable`](@ref), [`reach`](@ref). """ macro experimental(args...) isempty(args) && throw( @@ -178,19 +306,22 @@ macro experimental(args...) end rest = args[2:end] - isempty(rest) && - throw(ArgumentError("@experimental: nothing to mark — give a definition or a name")) + isempty(rest) && throw( + ArgumentError( + "@experimental: nothing to mark — the reason comes first, then one definition " * + "or a list of names: @experimental \"why\" f(x) = x", + ), + ) # The last argument is always the subject, which keeps `@experimental "…" x = 3` unambiguous # against `@experimental "…" since=v"1" x = 3`. - since, tracking, i = nothing, nothing, 1 + since, tracking, until, i = nothing, nothing, nothing, 1 while i < length(rest) a = rest[i] - if a isa Expr && - a.head === :(=) && - a.args[1] isa Symbol && - a.args[1] in (:since, :tracking) - a.args[1] === :since ? (since = a.args[2]) : (tracking = a.args[2]) + if a isa Expr && a.head === :(=) && a.args[1] isa Symbol && a.args[1] in _KEYWORDS + a.args[1] === :since && (since = a.args[2]) + a.args[1] === :tracking && (tracking = a.args[2]) + a.args[1] === :until && (until = a.args[2]) i += 1 else break @@ -202,7 +333,7 @@ macro experimental(args...) subject[1] isa Expr && subject[1].head === :(=) && subject[1].args[1] isa Symbol && - subject[1].args[1] in (:since, :tracking) + subject[1].args[1] in _KEYWORDS throw( ArgumentError( "@experimental: `$(subject[1].args[1])=…` given but no name to mark" @@ -211,7 +342,7 @@ macro experimental(args...) end names = _subject_names(subject) - def = nothing + sub = nothing if names === nothing length(subject) == 1 || throw( ArgumentError( @@ -219,14 +350,15 @@ macro experimental(args...) "$(length(subject)) arguments of which `$(_short(subject[findfirst(x -> !(x isa Symbol), subject)]))` is not a name", ), ) - def = subject[1] - names = [_defname(def)] + sub = _subject(subject[1]) + names = [sub.name] end + src = __source__ + marks = esc(MARKS_BINDING) # The `const` must land in its own top-level statement: the `_mark!` calls below read that # binding, and Julia 1.12 forbids reading one created in the same world age. - marks = esc(MARKS_BINDING) - src = __source__ + # # Built with `Expr` so no `LineNumberNode` from this file reaches the expansion. `const` in # local scope is a lowering error this macro cannot catch, so the only lever is where the # error points, and it must point at the caller. Pinned in `test_spec_forms.jl`. @@ -235,30 +367,16 @@ macro experimental(args...) :(!$(isdefined)($__module__, $(QuoteNode(MARKS_BINDING)))), Expr(:block, src, Expr(:const, Expr(:(=), marks, :($(Mark)[])))), ) - records = [ - :($(_mark!)( - $marks, - $(Mark)( - $__module__, - $(QuoteNode(n)), - $(_reason)($(esc(reason))), - $(since === nothing ? nothing : esc(since)), - $(tracking === nothing ? nothing : esc(tracking)), - $(QuoteNode(src.file)), - $(src.line), - ), - )) for n in names - ] + # The default layer's flag, one per marked name, and the probe that sets it. Only a # definition with a body can carry one; a name list, a struct, a const, a module or a - # `@generated` function is a declaration this package cannot observe. `_instrument` returns - # `nothing` for those and the definition goes out untouched. + # `@generated` function is a declaration this package cannot observe. flags = Expr[] - instrumented = def - if def !== nothing + emitted = sub === nothing ? nothing : sub.def + if sub !== nothing && sub.instrumentable # The `const` is emitted on its own, so it needs escaping here; the probe rides inside # the definition, which is escaped as a whole below — escaping it twice is an error. - flagsym = _flag_name(names[1]) + flagsym = _flag_name(sub.name) push!( flags, Expr( @@ -268,33 +386,98 @@ macro experimental(args...) :block, src, Expr( - :const, Expr(:(=), esc(flagsym), :($(Base.RefValue{Bool})(false))) + :const, + Expr( + :(=), + esc(flagsym), + :($(Probe)($__module__, $(QuoteNode(sub.name)))), + ), ), ), ), ) - probed = _instrument(def, flagsym) - probed === nothing || (instrumented = probed) + probed = _instrument(sub.def, flagsym, src) + probed === nothing || (emitted = probed) end + # Tells the documentation system which expression a preceding docstring belongs to, as # `Base.@kwdef` does. Without it a docstring on a marked definition fails to attach at all. - body = if instrumented === nothing - nothing - else - Expr(:block, Expr(:meta, :doc), esc(instrumented)) - end + body = emitted === nothing ? nothing : Expr(:block, Expr(:meta, :doc), esc(emitted)) + + # The signature is read back from the method table AFTER the definition has run: the macro + # sees argument types as syntax, and `Tuple{typeof(f), Numerical}` cannot be built out of + # syntax without evaluating names this module may not have yet. + sigexpr = sub === nothing ? nothing : sub.sigexpr + records = [ + :($(_mark!)( + $marks, + $(Mark)( + $__module__, + $(QuoteNode(n)), + $(_reason)($(esc(reason))), + $(since === nothing ? nothing : :($(_since)($(esc(since))))), + $(tracking === nothing ? nothing : :($(_tracking)($(esc(tracking))))), + $(QuoteNode(src.file)), + $(src.line), + $(sigexpr === nothing ? nothing : sigexpr), + $(sub === nothing ? false : sub.includes_constructors), + $(until === nothing ? nothing : :($(_until)($(esc(until))))), + ), + )) for n in names + ] return Expr(:block, init, flags..., body, records..., nothing) end +const _KEYWORDS = (:since, :tracking, :until) + # Normalises whitespace so a wrapped triple-quoted reason does not carry its indentation into # every message. Nothing else about the text is touched. -function _reason(s) +function _reason(s::AbstractString) r = String(strip(s)) isempty(r) && throw( ArgumentError("@experimental: the reason may not be empty — it is the payload") ) return r end +function _reason(@nospecialize(x)) + return throw( + ArgumentError( + "@experimental: the reason must be a string, got a $(typeof(x)) — " * + "@experimental \"why\" f(x) = x", + ), + ) +end + +_since(v::VersionNumber) = v +function _since(@nospecialize(x)) + return throw( + ArgumentError( + "@experimental: `since` must be a VersionNumber, got a $(typeof(x)) — " * + "write since=v\"$(x)\" rather than since=$(repr(x))", + ), + ) +end + +_tracking(s::AbstractString) = String(s) +function _tracking(@nospecialize(x)) + return throw( + ArgumentError( + "@experimental: `tracking` must be a string — an issue, a pull request or a URL, " * + "got a $(typeof(x))", + ), + ) +end + +_until(f::Function) = f +function _until(@nospecialize(x)) + return throw( + ArgumentError( + "@experimental: `until` must be a predicate taking no arguments — the condition " * + "that would discharge the reason, e.g. until=() -> isfile(reference_path), got " * + "a $(typeof(x))", + ), + ) +end # `nothing` means "not a name list" — i.e. the subject is a definition to attach to. function _subject_names(subject) @@ -321,6 +504,174 @@ end _short(ex) = (s=string(ex); length(s) > 40 ? first(s, 37) * "..." : s) +# What the macro learned from one definition: the name to record it under, the expression to emit, +# whether a probe can ride in the body, and how to read the created method's signature back at +# load time. +struct _Subject + name::Symbol + def::Any + instrumentable::Bool + sigexpr::Any + includes_constructors::Bool +end + +# Macros known to hand their definition through unchanged. An allowlist rather than "unwrap the +# last argument of any macrocall": `@deprecate old new` also ends in something name-shaped, and +# marking the wrong symbol silently is the failure this package exists to remove. +# +# Split by whether the body underneath is still a body. `@inline` and its neighbours annotate a +# definition and leave the body alone, so the probe can ride inside and the definition is +# observed. `@generated`'s body returns an expression — a probe there would be generated rather +# than run — and `Base.@kwdef` wraps a struct, which has no body at all. +const _ANNOTATING_MACROS = ( + Symbol("@inline"), + Symbol("@noinline"), + Symbol("@propagate_inbounds"), + Symbol("@assume_effects"), + Symbol("@constprop"), + Symbol("@nospecializeinfer"), +) +const _OPAQUE_MACROS = (Symbol("@generated"), Symbol("@kwdef")) + +_macroname(x::Symbol) = x +_macroname(x::Expr) = x.head === :. ? _macroname(x.args[2]) : Symbol("") +_macroname(x::QuoteNode) = x.value isa Symbol ? x.value : Symbol("") +_macroname(@nospecialize(x)) = Symbol("") + +function _subject(ex) + if ex isa Expr && ex.head === :macrocall + mname = _macroname(ex.args[1]) + if mname in _ANNOTATING_MACROS + # The annotation is kept, and the probe goes inside the definition it annotates — an + # `@inline` marked kernel is exactly the kind that has to be observable. + s = _subject(ex.args[end]) + wrapped = Expr(:macrocall, ex.args[1:(end - 1)]..., s.def) + return _Subject( + s.name, wrapped, s.instrumentable, s.sigexpr, s.includes_constructors + ) + end + if mname in _OPAQUE_MACROS + # The wrapping macro stays in charge of the definition, so nothing is spliced into a + # body it is about to rewrite. + s = _subject(ex.args[end]) + return _Subject(s.name, ex, false, s.sigexpr, s.includes_constructors) + end + throw( + ArgumentError( + "@experimental cannot see what `$(ex.args[1])` defines. Name it instead: " * + "@experimental \"why\" the_name", + ), + ) + end + return _plain_subject(ex) +end + +function _plain_subject(ex) + ex isa Symbol && return _Subject(ex, ex, false, nothing, false) + ex isa Expr || throw( + ArgumentError("@experimental: `$(_short(ex))` is neither a name nor a definition"), + ) + h = ex.head + if h === :function || h === :(=) + sig = ex.args[1] + _is_signature(sig) || return _Subject(_defname(ex), ex, false, nothing, false) + callee = _callee(sig) + return _Subject(callee.name, ex, length(ex.args) == 2, _sigexpr(callee), false) + end + h === :struct && return _Subject(_typename(ex.args[2]), ex, false, nothing, true) + h === :abstract && return _Subject(_typename(ex.args[1]), ex, false, nothing, true) + h === :primitive && return _Subject(_typename(ex.args[1]), ex, false, nothing, true) + h === :macro && + return _Subject(Symbol("@", _signame(ex.args[1])), ex, false, nothing, false) + h === :const && return _Subject(_defname(ex.args[1]), ex, false, nothing, false) + return _Subject(_defname(ex), ex, false, nothing, false) +end + +# The pieces of a call signature the macro needs: the name to record, and an expression that +# evaluates — in the caller's module, after the definition has run — to the function TYPE whose +# newest method is the one just defined. +struct _Callee + name::Symbol + ftype::Any # an expression, already escaped +end + +_sigexpr(c::_Callee) = :($(_newest_sig)($(c.ftype))) + +function _callee(sig) + if sig isa Expr && (sig.head === :where || sig.head === :(::)) + # `f(x)::R` and `f(x) where {T}` both wrap the call; a lone `(c::C)` does not. + inner = sig.args[1] + (inner isa Expr && (inner.head in (:call, :where, :(::)))) && return _callee(inner) + end + if sig isa Expr && sig.head === :call + f = sig.args[1] + if f isa Symbol + return _Callee(f, :($(_ftype_of)($(esc(f))))) # `f(x)` — a name in this module + elseif f isa Expr && f.head === :. + # `Base.show(io, ::T)` — a method on somebody else's generic. The name is theirs, so + # this records a claim about the signature and not about their surface. + return _Callee(_qualified_name(f), :($(_ftype_of)($(esc(f))))) + elseif f isa Expr && f.head === :(::) + # `(c::C)(x)` and `(::Type{C})(x)` — the callable object IS the function type. + t = length(f.args) == 2 ? f.args[2] : f.args[1] + return _Callee(_typename(t), :($(_ftype_self)($(esc(_typebase(t)))))) + elseif f isa Expr && f.head === :curly + # `T{P}(x) = …` — a parametric constructor. + return _Callee(_typename(f), :($(_ftype_of)($(esc(_typebase(f)))))) + end + throw(ArgumentError("@experimental: cannot read a name out of `$(_short(sig))`")) + end + return throw(ArgumentError("@experimental: cannot read a name out of `$(_short(sig))`")) +end + +# `x isa Type ? Type{x} : typeof(x)` — a name in call position is either a function or a type +# constructor, and the two need different function types to look the method up under. +_ftype_of(@nospecialize(x)) = x isa Type ? Type{x} : typeof(x) +# A callable object's own type IS the function type: `(c::C)(x)` defines `Tuple{C, Any}`. +_ftype_self(@nospecialize(x)) = x isa Type ? x : typeof(x) + +""" + _newest_sig(ftype) -> Union{Type,Nothing} + +The signature of the most recently defined method under `ftype`. + +Called from the statement the macro emits immediately after the definition, so "most recent" is +the method that definition just created. Reading it back from the method table is the only route: +the macro sees `::Numerical` as syntax, and building `Tuple{typeof(f), Numerical}` out of syntax +would mean evaluating names in a module that may not have them yet. + +Returns `nothing` rather than throwing if the lookup fails — a mark that loses its signature is a +wider claim, never a wrong one, and a package must not fail to load over it. +""" +function _newest_sig(@nospecialize(ft)) + try + ms = Base._methods_by_ftype(Tuple{ft,Vararg{Any}}, -1, Base.get_world_counter()) + (ms === nothing || ms === false) && return nothing + best = nothing + for match in ms + m = match.method + (best === nothing || m.primary_world > best.primary_world) && (best = m) + end + return best === nothing ? nothing : best.sig + catch + return nothing + end +end + +function _is_signature(x) + return x isa Expr && ( + x.head === :call || + ((x.head === :where || x.head === :(::)) && _is_signature(x.args[1])) + ) +end + +function _qualified_name(ex::Expr) + n = ex.args[2] + n isa QuoteNode && n.value isa Symbol && return n.value + n isa Symbol && return n + return throw(ArgumentError("@experimental: cannot read a name out of `$(_short(ex))`")) +end + _defname(s::Symbol) = s function _defname(ex::Expr) h = ex.head @@ -339,6 +690,15 @@ function _defname(ex::Expr) "Name it instead: @experimental \"why\" $(ex.args[2])", ), ) + # A bare `Base.show` names every method of that generic in the world, including ones this + # package never wrote. Guessing is worse than refusing. + h === :. && throw( + ArgumentError( + "@experimental: `$(_short(ex))` names another module's generic without saying " * + "WHICH method — mark the definition instead: " * + "@experimental \"why\" $(_short(ex))(::MyType) = …", + ), + ) h === :macrocall && throw( ArgumentError( "@experimental cannot see what `$(ex.args[1])` defines. Name it instead: " * @@ -352,7 +712,7 @@ function _defname(ex::Expr) ), ) end -function _defname(x) +function _defname(@nospecialize(x)) return throw( ArgumentError("@experimental: `$(_short(x))` is neither a name nor a definition") ) @@ -361,30 +721,50 @@ end _signame(s::Symbol) = s function _signame(ex::Expr) h = ex.head - (h === :call || h === :where || h === :(::) || h === :curly) && - return _signame(ex.args[1]) - h === :. && throw( - ArgumentError( - "@experimental: `$(_short(ex))` defines a name owned by another module, which is " * - "not part of this module's public surface", - ), - ) + (h === :call || h === :where || h === :curly) && return _signame(ex.args[1]) + if h === :(::) + inner = ex.args[1] + # `f(x)::R` annotates the return type — the name is inside. A lone `(c::C)` or `(::T)` + # is a callable object, and the name that means anything to a reader is the TYPE. + (inner isa Expr && inner.head in (:call, :where, :curly)) && return _signame(inner) + return _typename(length(ex.args) == 2 ? ex.args[2] : ex.args[1]) + end + h === :. && return _qualified_name(ex) return throw(ArgumentError("@experimental: cannot read a name out of `$(_short(ex))`")) end -function _signame(x) +function _signame(@nospecialize(x)) return throw(ArgumentError("@experimental: cannot read a name out of `$(_short(x))`")) end _typename(s::Symbol) = s function _typename(ex::Expr) h = ex.head + # `Type{C}` in `(::Type{C})(x)` — the name a reader recognises is `C`. + h === :curly && + ex.args[1] === :Type && + length(ex.args) == 2 && + return _typename(ex.args[2]) (h === :curly || h === :<:) && return _typename(ex.args[1]) + h === :. && return _qualified_name(ex) return throw( ArgumentError("@experimental: cannot read a type name out of `$(_short(ex))`") ) end -function _typename(x) +function _typename(@nospecialize(x)) return throw( ArgumentError("@experimental: cannot read a type name out of `$(_short(x))`") ) end + +# The expression naming the type itself, with any parameters dropped: `C{T}` is looked up under +# `C`, because `T` is a type variable bound by the `where` and means nothing at load time. +_typebase(s::Symbol) = s +function _typebase(ex::Expr) + ex.head === :curly && + ex.args[1] === :Type && + length(ex.args) == 2 && + return :(Type{$(_typebase(ex.args[2]))}) + ex.head === :curly && return _typebase(ex.args[1]) + ex.head === :<: && return _typebase(ex.args[1]) + return ex +end diff --git a/src/query.jl b/src/query.jl index 3ed9d88..3ce4332 100644 --- a/src/query.jl +++ b/src/query.jl @@ -1,11 +1,16 @@ # Reading a module's marks back out. Everything here is read-only and allocation-cheap: these # are the functions a release script, a docs build or a test calls, and none of them should be # able to create a registry as a side effect of asking a question. +# +# Two units live side by side. A NAME is what `names(m)` reports and what a release promises; a +# METHOD is what a call site actually reaches. A mark attached to a definition is both — it names +# something unsettled and knows which signature it attached to — so the queries below differ in +# which of the two they answer, never in which marks they can see. """ - experimental(m::Module) -> Vector{Mark} + experimental(m::Module; extensions = false) -> Vector{Mark} -Every name in `m` declared [`@experimental`](@ref), sorted by name. +Every declaration `m` has written with [`@experimental`](@ref), sorted by name. This is the query the marker exists to make possible — a tool asks the module, rather than a human reading docstrings. A module with no marks answers with an empty vector; it never errors @@ -17,12 +22,84 @@ for mk in experimental(MyPackage) end ``` +Includes marks on methods of *other* modules' generics, which `m` wrote and therefore owns: those +carry a `sig` and a name that is not `m`'s. `extensions = true` also walks `m`'s package +extensions, whose public names are part of the surface a user sees and are invisible to +`names(m)`. + The result is a fresh vector; mutating it does not change the module. Note that a mark does not imply the name is public — see [`audit`](@ref)'s `dangling`. """ -function experimental(m::Module) - isdefined(m, MARKS_BINDING) || return Mark[] - return sort(copy(_registry_of(m)); by=x -> x.name) +function experimental(m::Module; extensions::Bool=false) + out = _has_registry(m) ? copy(_registry_of(m)) : Mark[] + if extensions + for ext in package_extensions(m) + _has_registry(ext) && append!(out, _registry_of(ext)) + end + end + return sort!(out; by=_mark_order) +end + +_mark_order(mk::Mark) = (string(mk.name), mk.sig === nothing ? "" : string(mk.sig)) + +""" + experimental(f) -> Vector{Mark} + +Every mark **anyone** has written on a method of the generic `f`, across all loaded modules. + +The `fetch_value(::Heisenberg, ::Energy)` case: a generic is defined upstream and extended by +several packages, each of which may declare its own methods unsettled. The marks live in the +modules that wrote the methods, so answering this means asking all of them — see +[`marks_on`](@ref), which is the same search stated as a verb about the callee rather than about +ownership. +""" +experimental(f::Function) = marks_on(f) +experimental(t::Type) = marks_on(t) + +""" + experimental_methods(m::Module) -> Vector{Mark} + +The subset of `m`'s marks that carry a signature — the claims about a method rather than about a +whole name. + +Every attached form produces one, including `@experimental "…" Base.show(io, ::T) = …`, which is +how a package declares a method it contributed to somebody else's generic unsettled. The mark is +stored in the module that **wrote** the method, never in the module that owns the name: a package +cannot carry claims its dependents invented, and the mark has to survive that package being +reloaded. +""" +function experimental_methods(m::Module; extensions::Bool=false) + return filter(mk -> mk.sig !== nothing, experimental(m; extensions)) +end + +""" + marks_on(f) -> Vector{Mark} + marks_on(m::Method) -> Vector{Mark} + +Every mark written on a method of `f`, or on `m` specifically, wherever it was written. + +The cross-module search. [`experimental_methods`](@ref) is the ownership question — "what has +this module declared?" — and this is the call-site question — "has anyone said anything about +what I am about to call?". One name for both would leave a reader unable to tell which they got. +""" +function marks_on(@nospecialize(f)) + ft = _ftype_of(f) + out = Mark[] + for mod in marked_modules() + for mk in _registry_of(mod) + mk.sig === nothing && continue + _sig_ftype(mk.sig) === ft && push!(out, mk) + end + end + return sort!(out; by=_mark_order) +end + +function marks_on(m::Method) + out = Mark[] + for mk in _marks_covering(m) + push!(out, mk) + end + return sort!(out; by=_mark_order) end """ @@ -37,21 +114,246 @@ checklist: mk = mark(MyPackage, :render_report) mk === nothing || @warn "not settled" mk.reason mk.tracking ``` + +A name may carry several marks when separate methods behind it were declared separately. The +whole-name declaration is returned in preference to any of them, because it is the wider claim; +[`marks`](@ref) returns all of them. """ function mark(m::Module, name::Symbol) - isdefined(m, MARKS_BINDING) || return nothing - reg = _registry_of(m) - i = findfirst(x -> x.name === name, reg) - return i === nothing ? nothing : reg[i] + ms = marks(m, name) + isempty(ms) && return nothing + i = findfirst(isnamewide, ms) + return i === nothing ? ms[1] : ms[i] +end + +""" + marks(m::Module, name::Symbol) -> Vector{Mark} + +Every mark `m` carries under `name` — the whole-name declaration if there is one, and one per +separately declared method. +""" +function marks(m::Module, name::Symbol) + _has_registry(m) || return Mark[] + return sort!(filter(x -> x.name === name, copy(_registry_of(m))); by=_mark_order) +end + +""" + mark(m::Method) -> Union{Mark,Nothing} + +The [`Mark`](@ref) covering the method `m`, or `nothing`. + +A method is covered by a mark on its exact signature, or by a whole-name mark on the generic it +belongs to. The second is why a name list still says something about every method behind the +name, and the first is why marking one dispatch path does not speak for its siblings. +""" +function mark(m::Method) + for mk in _marks_covering(m) + return mk + end + return nothing end """ isexperimental(m::Module, name::Symbol) -> Bool + isexperimental(m::Method) -> Bool -Whether `name` in `m` is declared [`@experimental`](@ref). +Whether `name` in `m`, or the method `m`, is declared [`@experimental`](@ref). The one-bit form of [`mark`](@ref), for a caller that only needs the verdict — for instance the mechanical statement that changing this name is not breaking. Does not consult docstrings and does not consult visibility: it answers only whether a mark exists. """ -isexperimental(m::Module, name::Symbol) = mark(m, name) !== nothing +isexperimental(m::Module, name::Symbol) = !isempty(marks(m, name)) +isexperimental(m::Method) = mark(m) !== nothing + +# The marks that cover one method, widest first. Written as an iterator over a vector so both +# `mark(::Method)` and `marks_on(::Method)` read the same rule. +function _marks_covering(m::Method) + out = Mark[] + ft = _sig_ftype(m.sig) + id = _ftype_identity(ft) + for mod in _search_modules(m) + _has_registry(mod) || continue + for mk in _registry_of(mod) + if mk.sig === m.sig + push!(out, mk) + elseif mk.sig === nothing && + id !== nothing && + mk.mod === id.mod && + mk.name === id.name && + (!id.constructor || mk.includes_constructors) + push!(out, mk) + end + end + end + # A whole-name claim is the wider statement, so a caller that wants one mark gets that one. + return sort!(out; by=mk -> (mk.sig !== nothing, _mark_order(mk))) +end + +# Where a mark covering `m` can live, and nowhere else. The macro writes into the module the +# definition is in, and `mark_method!` writes into `m.module` for the same reason — so a +# signature-level mark is always in `m.module`. A whole-name mark is in the module that owns the +# name, which is the only module in which that name means this generic. +# +# Two modules rather than a walk over every marked module in the process: this runs once per +# resolved call site inside `reach`, and a world walk there would dominate the analysis. +function _search_modules(m::Method) + id = _ftype_identity(_sig_ftype(m.sig)) + (id === nothing || id.mod === m.module) && return (m.module,) + return (m.module, id.mod) +end + +# The `X` in `Type{X}` — what a constructor call dispatches on — or `nothing`. +function _constructed_type(@nospecialize(ft)) + (ft isa Type && ft <: Type && ft !== Type) || return nothing + ps = try + Base.unwrap_unionall(ft).parameters + catch + return nothing + end + length(ps) == 1 || return nothing + p = ps[1] + return (p isa TypeVar || p isa Union) ? nothing : p +end + +# The function type a signature dispatches on: `Tuple{typeof(f), Int}` -> `typeof(f)`. +function _sig_ftype(@nospecialize(sig)) + s = Base.unwrap_unionall(sig) + (s isa DataType && s <: Tuple && !isempty(s.parameters)) || return nothing + return s.parameters[1] +end + +# Who a function type belongs to, in the vocabulary a mark is written in. +function _ftype_identity(@nospecialize(ft)) + ft === nothing && return nothing + # Not `ft isa DataType && ft <: Type`: on 1.14-DEV `Type{X}` is not a `DataType`, and that + # spelling silently stopped recognising every constructor. + t = _constructed_type(ft) + if t !== nothing + b = Base.unwrap_unionall(t) + b isa DataType || return nothing + return (mod=parentmodule(b), name=nameof(b), constructor=true) + end + ft isa DataType || return nothing + if isdefined(ft, :instance) + f = ft.instance + return (mod=parentmodule(f), name=nameof(f), constructor=false) + end + # A callable object: `(c::C)(x)` dispatches on `C` itself. + return (mod=parentmodule(ft), name=nameof(ft), constructor=false) +end + +""" + mark_method!(m::Method, reason::AbstractString; since, tracking, until) -> Mark + +Declare one method unsettled, from outside its definition. + +The imperative route to what `@experimental "…" f(::T) = …` does at the definition site. It is +here for the cases the macro cannot reach: a method produced by another package's macro, a method +generated in a loop, and a package adopting this on code it does not want to touch yet. + +The mark is stored in the module that **wrote** the method, `m.module`, exactly where the macro +would have put it — so it is queryable through [`experimental_methods`](@ref) and survives the +same way. + +```julia +mark_method!( + which(fetch_value, Tuple{Heisenberg,Energy}), + "numerically delicate; no reference value"; + tracking = "https://github.com/org/Pkg.jl/issues/12", +) +``` + +Prefer the macro. A mark that is not next to the definition is a mark the next person editing that +definition will not see. +""" +function mark_method!( + m::Method, reason::AbstractString; since=nothing, tracking=nothing, until=nothing +) + reg = _method_registry!(m.module) + mk = Mark( + m.module, + m.name, + _reason(reason), + since === nothing ? nothing : _since(since), + tracking === nothing ? nothing : _tracking(tracking), + m.file, + Int(m.line), + m.sig, + false, + until === nothing ? nothing : _until(until), + ) + return _mark!(reg, mk) +end + +# `mark_method!` may be the first mark a module ever gets, and it arrives at run time rather than +# from the macro, so the registry has to be created here. The value is used through `Core.eval`'s +# return value and never read back in the same world age. +function _method_registry!(m::Module) + _has_registry(m) && return _registry_of(m) + return Core.eval(m, Expr(:const, Expr(:(=), MARKS_BINDING, Mark[]))) +end + +""" + superseded_marks(m::Module) -> Vector{Mark} + +The declarations that were replaced by a later one on the same name and signature. + +Re-marking is last-write-wins, which is the right rule — a file included twice must not double +its marks — but it is a rule that can lose a reason silently. Anything it drops is kept here, so +"the mark says something different from what I wrote" is answerable rather than mysterious. + +Empty for almost every module: nothing is recorded unless a mark actually replaced a different +one. +""" +function superseded_marks(m::Module) + isdefined(m, SUPERSEDED_BINDING) || return Mark[] + v = getglobal(m, SUPERSEDED_BINDING) + return v isa Vector{Mark} ? copy(v) : Mark[] +end + +""" + package_extensions(m::Module) -> Vector{Module} + +Every loaded package extension of `m`. + +An extension is a separate module whose public names are part of the surface a user sees and are +invisible to `names(m)`. [`audit`](@ref) reports which of them are loaded rather than folding +them in, because an extension that is not loaded is not missing — it is inapplicable. +""" +function package_extensions(m::Module) + out = Module[] + root = Base.moduleroot(m) + dir = try + Base.pkgdir(root) + catch + nothing + end + dir === nothing && return out + proj = joinpath(dir, "Project.toml") + isfile(proj) || return out + # Read from the project rather than found by walking `Base.loaded_modules`: an extension is a + # top-level module whose parent link says nothing about whose extension it is, and the + # `[extensions]` table is the only place the relationship is written down. + d = try + TOML.parsefile(proj) + catch + return out + end + for name in sort!(collect(keys(get(d, "extensions", Dict{String,Any}())))) + ext = Base.get_extension(root, Symbol(name)) + ext === nothing || push!(out, ext) + end + return out +end + +""" + marks_without_exit(m::Module) -> Vector{Mark} + +The marks that say why a name is unsettled but not what would settle it. + +A mark with no `until=` can be added and never mechanically retired — [`ready_to_promote`](@ref) +can only answer `false` for it, forever. That is a decoration rather than a work item, so it is +reported as its own finding rather than being folded into "not ready". +""" +marks_without_exit(m::Module) = filter(mk -> mk.until === nothing, experimental(m)) diff --git a/src/reach.jl b/src/reach.jl new file mode 100644 index 0000000..251f9e1 --- /dev/null +++ b/src/reach.jl @@ -0,0 +1,886 @@ +# A caller that never names a marked thing still depends on it. +# +# Modelled on Lean's `sorry`, but Julia's call graph is not closed, so the answer is three-valued: +# +# :depends a marked definition is reachable +# :clean the whole call graph was resolved and nothing marked is in it +# :unknown some call site could not be resolved — the honest non-answer +# +# Collapsing `:unknown` into `:clean` is the one failure this file exists to prevent. It is not a +# weaker claim, it is a false one: `Holder.f::Function` and `TABLE[i](x)` really can reach a +# marked function while being statically invisible. +# +# The walk is over INFERRED, UNOPTIMISED IR — `code_typed_by_type(sig; optimize=false)`. Inference +# runs before inlining, so every call is still a call and every argument still has a type; +# `optimize=true` would show `mul_float` and find nothing. That also makes the two hard cases fall +# out rather than needing special handling: a callee inference typed as `Function` is exactly a +# call site with no unique method, and a function passed as a value is specialised on `typeof(f)` +# and resolves. + +""" + Unresolved + +One call site the analysis could not pin to a method. + +| field | | +|---|---| +| `callee` | the name being called, as far as the IR knows it | +| `signature` | what it was called with — the widened argument types | +| `why` | `:dynamic`, `:ambiguous`, `:maxdepth`, `:splat` or `:nomethod` | +| `file`, `line` | where to go and look | +| `within` | the method the call site is in | +| `candidates` | the marked methods this site *could* reach, if any are visible | + +"Cannot tell" is only actionable if the reader can go and look, which is what `file`/`line` are +for. `candidates` is the difference between "some call here is dynamic" and "this call could go +to `k(::KB)`, which is marked". +""" +struct Unresolved + callee::Symbol + signature::Any + why::Symbol + file::Symbol + line::Int + within::Union{Method,Nothing} + candidates::Vector{Mark} +end + +function Base.show(io::IO, u::Unresolved) + return print(io, "Unresolved(", u.callee, ", ", u.why, ", ", u.file, ":", u.line, ")") +end + +""" + Reached + +One marked definition the analysis proved reachable from the entry point. + +`method` is the method that was resolved — `nothing` when the dependency is not a call, which is +what a marked `const` is. `path` is the chain of names from the entry point down to it, so the +reader learns which of their own code to distrust rather than only that something is wrong. +""" +struct Reached + mark::Mark + method::Union{Method,Nothing} + file::Symbol + line::Int + path::Vector{Symbol} +end + +function Base.show(io::IO, r::Reached) + return print( + io, "Reached(", r.mark.mod, ".", r.mark.name, " via ", join(r.path, " → "), ")" + ) +end + +""" + Reach + +What [`reach`](@ref) found. + +| field | | +|---|---| +| `entry` | what was analysed | +| `reached` | the marked definitions proved reachable | +| `unresolved` | the call sites that could not be pinned to a method | +| `through_modules` | every module the walk went through | +| `affected_entries` | for a module or script entry: the public entry points that are not clean | +| `visited` | how many distinct signatures were inferred | +| `truncated` | whether a depth limit stopped the walk | + +There is deliberately **no** `verdict` field. A stored verdict makes `:clean` with a non-empty +`unresolved` representable, and that state is the single thing this analysis must never report. +[`verdict`](@ref) derives it instead, the way [`isbreaking`](@ref) derives its answer from a +[`Diff`](@ref). +""" +struct Reach + entry::Any + reached::Vector{Reached} + unresolved::Vector{Unresolved} + through_modules::Vector{Module} + affected_entries::Vector{NamedTuple{(:name, :verdict),Tuple{Symbol,Symbol}}} + visited::Int + truncated::Bool +end + +""" + verdict(r::Reach) -> Symbol + +`:depends`, `:unknown` or `:clean`, derived from what [`reach`](@ref) found. + + * `:depends` — a marked definition is reachable. Proved, not suspected. + * `:clean` — the whole call graph was resolved and nothing marked is in it. + * `:unknown` — at least one call site could not be pinned to a method, and nothing marked was + proved reachable through the rest. + +`:depends` wins over `:unknown`: an unresolvable call elsewhere does not make a proved dependency +less proved. `:unknown` wins over `:clean`, which is the whole point. +""" +function verdict(r::Reach) + isempty(r.reached) || return :depends + isempty(r.unresolved) || return :unknown + return :clean +end + +""" + isclean(r::Reach) -> Bool + +Whether [`verdict`](@ref) is `:clean`. `false` for `:unknown` as well as for `:depends` — the +predicate answers "may I rely on this", and the honest non-answer is not a yes. +""" +isclean(r::Reach) = verdict(r) === :clean + +""" + combine(a::Symbol, b::Symbol) -> Symbol + +Fold two verdicts into one: `:clean` < `:unknown` < `:depends`. + +Needed because [`reach`](@ref) on a module folds one answer per public entry point into one +answer for the module. Commutative and associative, so the order the entry points come back in +cannot change the result. +""" +function combine(a::Symbol, b::Symbol) + return _verdict_rank(a) >= _verdict_rank(b) ? a : b +end + +function _verdict_rank(v::Symbol) + v === :depends && return 2 + v === :unknown && return 1 + v === :clean && return 0 + return throw( + ArgumentError("not a verdict: $(repr(v)) — expected :clean, :unknown or :depends") + ) +end + +# The walk's mutable state, so the recursion carries one argument instead of six. +mutable struct _Walk + world::UInt + maxdepth::Int + maxcandidates::Int + ignore::Set{Symbol} + visited::Set{Any} + reached::Vector{Reached} + unresolved::Vector{Unresolved} + modules::Vector{Module} + marked::Dict{Method,Union{Mark,Nothing}} + truncated::Bool +end + +function _Walk(maxdepth::Int, ignore, maxcandidates::Int=16) + return _Walk( + Base.get_world_counter(), + maxdepth, + maxcandidates, + Set{Symbol}(Symbol[ignore...]), + Set{Any}(), + Reached[], + Unresolved[], + Module[], + Dict{Method,Union{Mark,Nothing}}(), + false, + ) +end + +""" + reach(f, types::Type{<:Tuple}; maxdepth = 32, ignore = Symbol[]) -> Reach + reach(m::Module; kwargs...) -> Reach + +Report whether calling `f` with `types` can reach anything declared [`@experimental`](@ref) — +including through callers that never name it. + +```julia +r = reach(analyse, Tuple{Model,Float64}) +verdict(r) === :clean || error("depends on: ", [x.mark.name for x in r.reached]) +``` + +The module form folds every public entry point of `m` into one answer and reports which of them +are affected in `affected_entries`: function-by-function does not scale to a package. + +`ignore` names marks to treat as absent, which answers "what would removing this mark change?" +without removing it. `maxdepth` bounds the walk and `maxcandidates` bounds how many methods a +single ambiguous call site is willing to check; hitting either bound is reported as `:unknown`, +never as `:clean`. + +# What it can and cannot resolve + +| call site | | +|---|---| +| a named call, however deep | resolved | +| a function passed as a value | resolved — Julia specialises on `typeof(f)` | +| `@nospecialize`d callee, called with a concrete function | resolved | +| `invoke(f, Tuple{Integer}, x)` | resolved to the method `invoke` pins, not the one dispatch would pick | +| a `Union`- or abstract-typed argument with methods on both sides | **`:unknown`** — no unique method | +| a callee read out of a field or a table | **`:unknown`** | +| a marked `const` or `struct` used in the body | `:depends` | + +A more specific unmarked method shadowing a marked one is resolved as what actually runs: `Int` +goes to `more_specific(::Int)` and is clean, while `UInt8` falls through to the marked +`::Integer` and is not. + +!!! warning "It answers about code, not about a run" + This is static: it says what *could* be reached. [`entered`](@ref) and [`record`](@ref) say + what *was*. A path this reports is not necessarily taken, and a `:clean` here is only as good + as the call graph being closed — which is what `:unknown` exists to admit. +""" +function reach( + @nospecialize(f), + @nospecialize(types::Type); + maxdepth::Int=32, + maxcandidates::Int=16, + ignore=Symbol[], +) + sig = Base.signature_type(f, types) + st = _Walk(maxdepth, ignore, maxcandidates) + matches = _matching_methods(st, sig) + (matches === nothing || isempty(matches)) && throw( + ArgumentError( + "reach: no method of $(f) matches $(types) — there is nothing to analyse. " * + "Check the entry signature against `methods($(f))`.", + ), + ) + for match in matches + _enter!(st, match, 0, Symbol[]) + end + return _finish(st, f => types, similar_entries()) +end + +similar_entries() = NamedTuple{(:name, :verdict),Tuple{Symbol,Symbol}}[] + +function reach(m::Module; maxdepth::Int=32, maxcandidates::Int=16, ignore=Symbol[]) + st = _Walk(maxdepth, ignore, maxcandidates) + entries = similar_entries() + for n in surface(m) + isdefined(m, n) || continue + v = try + getglobal(m, n) + catch + continue + end + (v isa Function || v isa Type) || continue + ml = try + methods(v) + catch + continue + end + isempty(ml) && continue + # A FRESH walk per entry point. Sharing one `visited` set across them would make the + # second entry that reaches a marked definition through an already-walked callee look + # clean — the mark is real, it was simply reported under the first entry's name. + own = _Walk(maxdepth, ignore, maxcandidates) + for mm in ml + mm.module === m || continue + _enter!(own, mm, 0, Symbol[n]) + end + _absorb!(st, own) + vn = verdict_of(own) + vn === :clean || push!(entries, (name=n, verdict=vn)) + end + return _finish(st, m, entries) +end + +# The same derivation `verdict` makes from a `Reach`, over the walk that produced one entry. +function verdict_of(st::_Walk) + isempty(st.reached) || return :depends + isempty(st.unresolved) || return :unknown + return :clean +end + +function _absorb!(into::_Walk, from::_Walk) + append!(into.reached, from.reached) + append!(into.unresolved, from.unresolved) + for mod in from.modules + mod in into.modules || push!(into.modules, mod) + end + union!(into.visited, from.visited) + into.truncated |= from.truncated + return nothing +end + +function _finish(st::_Walk, @nospecialize(entry), entries) + return Reach( + entry, + st.reached, + st.unresolved, + st.modules, + entries, + length(st.visited), + st.truncated, + ) +end + +""" + reach_script(path::AbstractString; kwargs...) -> Reach + +The same analysis for a **script** — a file that produces a figure rather than a package. + +Top-level declarations that cannot live inside a function (`using`, `import`, `module`, `const`, +type definitions) are evaluated in a scratch module, because the analysis has to resolve the names +the script uses; everything else is analysed as one thunk. So this **loads the script's +dependencies**, and a script whose top level has side effects will have them. +""" +function reach_script( + path::AbstractString; maxdepth::Int=32, maxcandidates::Int=16, ignore=Symbol[] +) + isfile(path) || throw(ArgumentError("reach_script: no such file: $path")) + ex = Meta.parseall(read(path, String); filename=path) + scratch = Module(Symbol("ReachScript_", basename(path))) + Core.eval(scratch, :(eval(x) = Core.eval($scratch, x))) + body = Expr(:block) + for st in ex.args + if st isa LineNumberNode + push!(body.args, st) + elseif _is_toplevel_only(st) + Core.eval(scratch, st) + else + push!(body.args, st) + end + end + thunk = Core.eval(scratch, Expr(:function, Expr(:call, gensym(:script)), body)) + r = reach(thunk, Tuple{}; maxdepth, maxcandidates, ignore) + return Reach( + path, + r.reached, + r.unresolved, + r.through_modules, + [(name=Symbol(basename(path)), verdict=verdict(r))], + r.visited, + r.truncated, + ) +end + +function _is_toplevel_only(st) + st isa Expr || return false + return st.head in ( + :using, + :import, + :export, + :public, + :module, + :const, + :struct, + :abstract, + :primitive, + :macro, + ) +end + +# ── the walk ───────────────────────────────────────────────────────────────────────────────── + +function _enter!(st::_Walk, match, depth::Int, path::Vector{Symbol}) + mm = match isa Method ? match : match.method + sig = match isa Method ? match.sig : match.spec_types + if depth > st.maxdepth + st.truncated = true + push!( + st.unresolved, + Unresolved(mm.name, sig, :maxdepth, mm.file, Int(mm.line), mm, Mark[]), + ) + return nothing + end + sig in st.visited && return nothing + push!(st.visited, sig) + # The flag `@experimental` emits is this package's own code, and under `ignore` the walk goes + # through it rather than stopping at the mark. Following it would report the recorder's + # internals — `backtrace`, and everything Base does to format one — as the caller's + # dependencies. Nothing in here is ever marked, so there is nothing to lose by stopping. + Base.moduleroot(mm.module) === ExperimentalAPI && return nothing + mm.module in st.modules || push!(st.modules, mm.module) + + mk = _mark_of(st, mm) + if mk !== nothing + push!( + st.reached, + Reached(mk, mm, mm.file, Int(mm.line), vcat(path, [_display_name(mm)])), + ) + # No recursion past a mark: it is already reported, and everything under it is suspect + # for the same reason. + return nothing + end + _scan_body!(st, mm, sig, depth, vcat(path, [_display_name(mm)])) + return nothing +end + +_display_name(mm::Method) = mm.name + +function _mark_of(st::_Walk, mm::Method) + cached = get(st.marked, mm, missing) + cached === missing || return cached + mk = mark(mm) + mk !== nothing && mk.name in st.ignore && (mk = nothing) + st.marked[mm] = mk + return mk +end + +function _scan_body!( + st::_Walk, mm::Method, @nospecialize(sig), depth::Int, path::Vector{Symbol} +) + ci = _inferred(sig) + ci === nothing && return nothing + code = ci.code + for pc in eachindex(code) + stmt = code[pc] + _scan_stmt!(st, ci, stmt, pc, mm, depth, path) + end + return nothing +end + +function _scan_stmt!( + st::_Walk, ci, @nospecialize(stmt), pc::Int, mm::Method, depth::Int, path +) + if stmt isa GlobalRef + _check_global!(st, stmt, mm, _line(ci, pc, mm), path) + return nothing + end + stmt isa Expr || return nothing + if stmt.head === :(=) && length(stmt.args) == 2 + return _scan_stmt!(st, ci, stmt.args[2], pc, mm, depth, path) + end + if stmt.head === :return || stmt.head === :gotoifnot + for a in stmt.args + a isa GlobalRef && _check_global!(st, a, mm, _line(ci, pc, mm), path) + end + return nothing + end + if stmt.head === :new + for a in stmt.args + a isa GlobalRef && _check_global!(st, a, mm, _line(ci, pc, mm), path) + end + return nothing + end + stmt.head === :call || return nothing + for a in stmt.args + a isa GlobalRef && _check_global!(st, a, mm, _line(ci, pc, mm), path) + end + return _resolve_call!(st, ci, stmt, pc, mm, depth, path) +end + +function _resolve_call!(st::_Walk, ci, stmt::Expr, pc::Int, mm::Method, depth::Int, path) + args = stmt.args + isempty(args) && return nothing + fval = _const_value(ci, args[1]) + line = _line(ci, pc, mm) + + # `invoke(f, Tuple{Integer}, x)` pins a method dispatch would not pick. Reading the argument + # types alone would resolve it to `more_specific(::Int)` and report clean. + if fval === Core.invoke && length(args) >= 3 + target = _const_value(ci, args[2]) + pinned = _const_value(ci, args[3]) + if target !== nothing && pinned isa Type + sig = Base.signature_type(target, pinned) + # `invoke` semantics, not dispatch semantics: the method chosen for arguments of the + # DECLARED type. Resolving `Tuple{typeof(more_specific), Integer}` by dispatch finds + # both `::Int` and `::Integer` and reports the site unresolved, which is exactly the + # over-caution an analysis that ignores `invoke` would show. + pin = try + which(sig) + catch + nothing + end + if pin isa Method + return _enter!(st, pin, depth + 1, path) + end + return _resolve_sig!(st, sig, _callee_name(ci, args[2]), mm, line, depth, path) + end + end + # `f(x; k = 1)` goes through `Core.kwcall(nt, f, x)`; the mark is on `f`'s own method. + if fval === Core.kwcall && length(args) >= 3 + rest = Any[_argtype(ci, a) for a in args[4:end]] + ft = _argtype(ci, args[3]) + sig = _tuple_type(ft, rest) + sig === nothing || + _resolve_sig!(st, sig, _callee_name(ci, args[3]), mm, line, depth, path) + end + # `f(xs...)` goes through `_apply_iterate`. A splat of a known-length tuple — which is what + # `*(promote(x, y)...)` is, and most of Base with it — has an arity inference already knows, + # so it resolves; anything else genuinely hides the arity and is reported as unresolved. + if fval === Core._apply_iterate && length(args) >= 3 + name = _callee_name(ci, args[3]) + ft = _argtype(ci, args[3]) + flat = _flatten_splat(ci, args[4:end]) + if ft !== nothing && flat !== nothing && _is_callable_type(ft) + sig = _tuple_type(ft, flat) + sig === nothing || return _resolve_sig!(st, sig, name, mm, line, depth, path) + end + push!( + st.unresolved, + Unresolved(name, ft, :splat, mm.file, line, mm, _marks_named(ci, args[3])), + ) + return nothing + end + fval isa Core.Builtin && return nothing + fval isa Core.IntrinsicFunction && return nothing + + ft = _argtype(ci, args[1]) + ft === nothing && return nothing + # The callee is a builtin whose identity inference did not make constant — `tuple` reached + # through a slot, for instance. Builtins have no Julia body and nothing marked behind them. + (ft <: Core.Builtin || ft <: Core.IntrinsicFunction) && return nothing + if ft === Any || !_is_callable_type(ft) + push!( + st.unresolved, + Unresolved(_callee_name(ci, args[1]), ft, :dynamic, mm.file, line, mm, Mark[]), + ) + return nothing + end + sig = _tuple_type(ft, Any[_argtype(ci, a) for a in args[2:end]]) + sig === nothing && return nothing + return _resolve_sig!(st, sig, _callee_name(ci, args[1]), mm, line, depth, path) +end + +function _resolve_sig!( + st::_Walk, @nospecialize(sig), name::Symbol, mm::Method, line::Int, depth::Int, path +) + matches = _matching_methods(st, sig) + if matches === nothing + push!(st.unresolved, Unresolved(name, sig, :dynamic, mm.file, line, mm, Mark[])) + return nothing + end + if length(matches) == 1 + return _enter!(st, matches[1], depth + 1, path) + end + if isempty(matches) + # Statically a `MethodError`. Nothing is reachable through it, but saying `:clean` about + # a call that cannot run is not a claim worth making either. + push!(st.unresolved, Unresolved(name, sig, :nomethod, mm.file, line, mm, Mark[])) + return nothing + end + # Several methods match and nothing in the IR says which. Reporting `:depends` because one of + # them is marked would over-claim; reporting `:clean` because none is *proved* reached is the + # false answer this whole file guards. + # + # But "which method runs" is only worth knowing if the answer could differ. Every candidate is + # walked in its own right, and when none of them reaches anything marked the site is resolved + # after all — that is not a guess, it is having checked all of them. Without this, + # `convert(::Type, ::UInt32)` — dozens of matching methods, none of them anybody's research + # code — makes every caller that formats a string `:unknown`. + if length(matches) > st.maxcandidates + push!(st.unresolved, Unresolved(name, sig, :ambiguous, mm.file, line, mm, Mark[])) + return nothing + end + cands = Mark[] + opaque = false + for match in matches + sub = _subwalk(st) + _enter!(sub, match, depth + 1, path) + for r in sub.reached + r.mark in cands || push!(cands, r.mark) + end + isempty(sub.unresolved) || (opaque = true) + for mod in sub.modules + mod in st.modules || push!(st.modules, mod) + end + st.truncated |= sub.truncated + end + (isempty(cands) && !opaque) && return nothing + push!(st.unresolved, Unresolved(name, sig, :ambiguous, mm.file, line, mm, cands)) + return nothing +end + +# A walk with its own bookkeeping and the parent's settings. `visited` deliberately starts empty: +# a candidate already reached under a different branch still has to be walked here, or the branch +# would be reported clean on the strength of somebody else's traversal. +function _subwalk(st::_Walk) + return _Walk( + st.world, + st.maxdepth, + st.maxcandidates, + st.ignore, + Set{Any}(), + Reached[], + Unresolved[], + Module[], + st.marked, + false, + ) +end + +# A marked `const` or `struct` is not a call site. Either the analysis reads globals out of the +# IR or the case is out of scope — what it must not do is report `:clean`. +function _check_global!(st::_Walk, g::GlobalRef, mm::Method, line::Int, path) + isdefined(g.mod, g.name) || return nothing + v = try + getglobal(g.mod, g.name) + catch + return nothing + end + # Functions are reached through their call sites, where dispatch narrows the claim to one + # method. Treating a mere mention of the name as a dependency would make every sibling method + # of a marked one guilty. + v isa Function && return nothing + mk = mark(g.mod, g.name) + (mk === nothing || !isnamewide(mk) || mk.name in st.ignore) && return nothing + any(r -> r.mark === mk, st.reached) && return nothing + push!(st.reached, Reached(mk, nothing, mm.file, line, vcat(path, [g.name]))) + return nothing +end + +function _marks_named(ci, x) + g = x isa GlobalRef ? x : nothing + g === nothing && return Mark[] + mk = mark(g.mod, g.name) + return mk === nothing ? Mark[] : Mark[mk] +end + +# ── reading the IR ─────────────────────────────────────────────────────────────────────────── + +function _matching_methods(st::_Walk, @nospecialize(sig)) + r = try + Base._methods_by_ftype(sig, -1, st.world) + catch + return nothing + end + (r === nothing || r === false) && return nothing + return r +end + +const _INFERRED = IdDict{Any,Any}() + +function _inferred(@nospecialize(sig)) + haskey(_INFERRED, sig) && return _INFERRED[sig] + ci = try + cis = Base.code_typed_by_type(sig; optimize=false, debuginfo=:source) + # Not every signature has a body to hand back: a builtin comes back paired with its + # `Method` rather than a `CodeInfo`, and reading `.code` off that is a `FieldError` from + # somewhere three frames below where the mistake was made. + c = isempty(cis) ? nothing : cis[1][1] + c isa Core.CodeInfo ? c : nothing + catch + nothing + end + _INFERRED[sig] = ci + return ci +end + +function _argtype(ci, @nospecialize(x)) + x isa Core.SSAValue && return _widen(_ssatype(ci, x.id)) + x isa Core.Argument && return _widen(_slottype(ci, x.n)) + x isa Core.SlotNumber && return _widen(_slottype(ci, x.id)) + x isa GlobalRef && return _widen(_globaltype(x)) + x isa QuoteNode && return _norm_type(Core.Typeof(x.value)) + x isa Expr && return Any + x isa Type && return Type{x} + return _norm_type(Core.Typeof(x)) +end + +function _ssatype(ci, i::Int) + t = ci.ssavaluetypes + t isa Vector || return Any + return (1 <= i <= length(t)) ? t[i] : Any +end + +function _slottype(ci, i::Int) + t = ci.slottypes + t isa Vector || return Any + return (1 <= i <= length(t)) ? t[i] : Any +end + +function _globaltype(g::GlobalRef) + isdefined(g.mod, g.name) || return Any + isconst(g.mod, g.name) || return Any + v = try + getglobal(g.mod, g.name) + catch + return Any + end + return _norm_type(Core.Typeof(v)) +end + +# The lattice elements inference hands back are not all types. Anything not understood widens to +# `Any`, which turns into an unresolved call site rather than a wrong resolution. +function _widen(@nospecialize(t)) + t isa Type && return _norm_type(t) + t isa Core.Const && return _norm_type(Core.Typeof(t.val)) + if t isa Core.PartialStruct + u = t.typ + u isa Type && return u + end + if isdefined(Core, :PartialOpaque) && t isa Core.PartialOpaque + u = t.typ + u isa Type && return u + end + hasproperty(t, :thentype) && return Bool # Conditional + hasproperty(t, :typ) && (getproperty(t, :typ) isa Type) && return getproperty(t, :typ) + return Any +end + +function _const_value(ci, @nospecialize(x)) + x isa GlobalRef || return _const_of(_raw_type(ci, x)) + isdefined(x.mod, x.name) || return nothing + isconst(x.mod, x.name) || return nothing + return try + getglobal(x.mod, x.name) + catch + nothing + end +end + +function _raw_type(ci, @nospecialize(x)) + x isa Core.SSAValue && return _ssatype(ci, x.id) + x isa Core.Argument && return _slottype(ci, x.n) + x isa Core.SlotNumber && return _slottype(ci, x.id) + return nothing +end + +_const_of(@nospecialize(t)) = t isa Core.Const ? t.val : nothing + +function _callee_name(ci, @nospecialize(x)) + x isa GlobalRef && return x.name + t = _raw_type(ci, x) + if t isa Core.Const + v = t.val + v isa Function && return nameof(v) + v isa Type && return nameof(v) + end + w = _widen(t === nothing ? Any : t) + w isa DataType && isdefined(w, :instance) && return nameof(w.instance) + return :? +end + +# `Tuple{ft, args...}` is only a signature if the pieces are types. A `Vararg` or an unwidened +# lattice element would make `_methods_by_ftype` throw. +function _tuple_type(@nospecialize(ft), args::Vector{Any}) + ft isa Type || return nothing + ts = Any[ft] + for a in args + a isa Type || return nothing + a isa Core.TypeofVararg && return nothing + push!(ts, a) + end + return try + Tuple{ts...} + catch + nothing + end +end + +# Whether a type can be the first parameter of a signature that dispatch could pin. `Function` +# and `Any` cannot: they are the shapes a field read or a table lookup produces. +# +# `Type{Float64}` is the exception the abstractness flag alone gets wrong. Julia marks it abstract, +# but a constant type in call position is a constructor call and dispatch pins it exactly. +function _is_callable_type(@nospecialize(ft)) + ft === Any && return false + ft === Function && return false + ft isa Type || return false + _type_parameter(ft) === nothing || return true + ft isa DataType || return false + isabstracttype(ft) && return false + return true +end + +# The `X` in `Type{X}`, or `nothing` if `t` is not a constant type. +# +# Spelled as a question about `t` rather than as `t isa DataType && t <: Type`, because both halves +# of that moved: on 1.14-DEV `Type{X}` is no longer a `DataType`, and `Core.Typeof(Float64)` +# returns the new `Core.TypeEgal{Float64}` rather than `Type{Float64}`. Measured on +# 1.14.0-DEV.3115; the old spelling made every constructor call in the graph `:unknown`, which +# reported four otherwise-clean fixtures as unresolved. +function _type_parameter(@nospecialize(t)) + (t isa Type && t <: Type && t !== Type) || return nothing + ps = try + Base.unwrap_unionall(t).parameters + catch + return nothing + end + length(ps) == 1 || return nothing + p = ps[1] + return p isa TypeVar ? nothing : p +end + +# `Type{X}` in the spelling the rest of this file and every method signature uses. +function _norm_type(@nospecialize(t)) + p = _type_parameter(t) + return p === nothing ? t : Type{p} +end + +# The element types a splatted argument contributes, or `nothing` if its arity is not known. +function _flatten_splat(ci, args) + out = Any[] + for a in args + t = _argtype(ci, a) + t isa DataType || return nothing + t <: Tuple || return nothing + Base.isvatuple(t) && return nothing + for p in t.parameters + p isa Type || return nothing + push!(out, p) + end + end + return out +end + +function _line(ci, pc::Int, mm::Method) + fallback = Int(mm.line) + if hasproperty(ci, :debuginfo) && + isdefined(Base, :IRShow) && + isdefined(Base.IRShow, :getdebugidx) + try + l = Base.IRShow.getdebugidx(ci.debuginfo, pc)[1] + l > 0 && return Int(l) + catch + end + end + try + cl = getfield(ci, :codelocs) + lt = getfield(ci, :linetable) + if cl isa Vector && lt isa Vector && pc <= length(cl) + i = Int(cl[pc]) + 1 <= i <= length(lt) && return Int(lt[i].line) + end + catch + end + return fallback +end + +""" + dependents(m::Module, name::Symbol; kwargs...) -> Vector{Symbol} + +The public names of `m` whose call graph reaches `name`. + +Propagation read backwards, which is the direction the question is actually asked in: a mark gets +deleted because somebody looked at the definition, not at who reaches it. Compare +`reach(m; ignore = [name])` to see what removing it would change. +""" +function dependents(m::Module, name::Symbol; maxdepth::Int=32) + out = Symbol[] + for n in surface(m) + n === name && continue + isdefined(m, n) || continue + v = try + getglobal(m, n) + catch + continue + end + (v isa Function || v isa Type) || continue + ml = try + methods(v) + catch + continue + end + found = false + for mm in ml + mm.module === m || continue + st = _Walk(maxdepth, Symbol[]) + _enter!(st, mm, 0, Symbol[n]) + any(r -> r.mark.name === name, st.reached) && (found=true; break) + end + found && push!(out, n) + end + return out +end + +function Base.show(io::IO, ::MIME"text/plain", r::Reach) + v = verdict(r) + println(io, "Reach(", r.entry, ") — ", uppercase(string(v))) + for x in r.reached + println(io, " reached ", x.mark.mod, ".", x.mark.name, " — ", x.mark.reason) + println(io, " via ", join(x.path, " → "), " @ ", x.file, ":", x.line) + end + for u in r.unresolved + println(io, " unresolved ", u.callee, " (", u.why, ") @ ", u.file, ":", u.line) + for c in u.candidates + println(io, " could reach ", c.mod, ".", c.name, " — ", c.reason) + end + end + r.truncated && println(io, " (a depth limit stopped the walk)") + return nothing +end diff --git a/src/record.jl b/src/record.jl new file mode 100644 index 0000000..28dc4e8 --- /dev/null +++ b/src/record.jl @@ -0,0 +1,623 @@ +# The opt-in layer: how often a run entered marked code, by which paths, and how much of the run +# was spent inside it. +# +# The boundary against the default layer was measured rather than chosen (`test/spec/README.md`). +# A counter in the body costs 3.76x on eight threads and loses 40% of its increments to races +# unless it is atomic; a set-once flag is free. So the flag stays, and `record` reaches the same +# statement from the other side: opening a recording clears every probe, the short-circuit fails, +# and the write side — which is a function call, not an inlined store — does the counting. Nothing +# in the body changes, and nothing outside `record` pays for any of it. + +""" + Hit + +One marked definition a [`record`](@ref) block entered, and what it cost. + +| field | | +|---|---| +| `mod`, `name` | which marked definition | +| `reason` | carried into the record, so a reader a year later needs no source | +| `count` | how many times it was entered, summed over threads | +| `method` | the method the mark attached to, when it attached to one | +| `callers` | the distinct immediate callers seen | +| `paths` | the distinct call paths seen, innermost first, bounded | +| `inclusive`, `exclusive` | seconds, or `missing` when no timing backend was loaded | + +`count` is exact. `paths` is a bounded sample: a backtrace costs microseconds, so the recorder +stops looking once it has seen enough, and a path that only occurs after the budget is spent is +absent. `inclusive` counts time in anything this definition called; `exclusive` counts only time +in the definition itself, which is what separates a marked wrapper over settled code from a marked +kernel. +""" +struct Hit + mod::Module + name::Symbol + reason::String + count::Int + method::Union{Method,Nothing} + callers::Vector{Symbol} + paths::Vector{Vector{Symbol}} + inclusive::Union{Float64,Missing} + exclusive::Union{Float64,Missing} +end + +function Base.:(==)(a::Hit, b::Hit) + return a.mod === b.mod && + a.name === b.name && + a.count == b.count && + a.method === b.method && + a.callers == b.callers && + a.paths == b.paths +end + +function Base.show(io::IO, h::Hit) + return print(io, "Hit(", h.mod, ".", h.name, ", count=", h.count, ")") +end + +""" + Record + +What [`record`](@ref) observed: a `Vector`-like of [`Hit`](@ref), plus what the run was. + +| property | | +|---|---| +| `enabled` | whether recording was actually on — an empty record means "nothing was entered", and that is a different statement from "nothing was recorded" | +| `slots` | thread slots the counters were sized for, at least `Threads.maxthreadid()` | +| `elapsed` | seconds the recorded call took | +| `overhead` | the recorder's estimated share of `elapsed` | +| `versions` | package versions the marks were read against | +| `sampled` | whether a timing backend produced `inclusive`/`exclusive` | + +Indexing, iteration and `==` are the `Hit` vector's, so `record(f) == []` reads the way it looks. +The extra properties are why it is a type and not a plain vector: an empty `Vector{Hit}` cannot +tell "touched nothing" from "recording was off", and those mean opposite things. +""" +struct Record <: AbstractVector{Hit} + hits::Vector{Hit} + enabled::Bool + slots::Int + elapsed::Float64 + overhead::Float64 + versions::Dict{String,Any} + sampled::Bool +end + +Base.size(r::Record) = size(r.hits) +Base.getindex(r::Record, i::Int) = r.hits[i] +Base.IndexStyle(::Type{Record}) = IndexLinear() + +function Base.show(io::IO, ::MIME"text/plain", r::Record) + println( + io, + "Record — ", + length(r), + " marked definition", + length(r) == 1 ? "" : "s", + " entered in ", + round(r.elapsed; sigdigits=3), + "s", + ) + r.enabled || println(io, " (recording was OFF — this is not a claim that nothing ran)") + for h in r.hits + println(io, " ", h.mod, ".", h.name, " ×", h.count, " — ", h.reason) + h.inclusive === missing || println( + io, + " inclusive ", + round(h.inclusive; sigdigits=3), + "s exclusive ", + round(h.exclusive; sigdigits=3), + "s", + ) + for p in h.paths + println(io, " via ", join(reverse(p), " → ")) + end + end + return println(io, " recorder overhead ≈ ", round(100 * r.overhead; sigdigits=2), "%") +end + +# ── the timing backend ─────────────────────────────────────────────────────────────────────── +# +# Sampling is the only way to say how much of a run was spent inside a definition without wrapping +# the call, and wrapping is exactly what the emitted statement may not do. The sampler is Julia's +# own, reached through an extension so that `using ExperimentalAPI` — which every marked package +# does at run time — never loads `Profile`. + +""" + TimingBackend + +How [`record`](@ref) gets `inclusive`/`exclusive` time. The one implementation lives in this +package's `Profile` extension; without it, timing is `missing` rather than zero. +""" +abstract type TimingBackend end + +struct NoTiming <: TimingBackend end + +const _TIMING = Ref{TimingBackend}(NoTiming()) + +start_timing!(::NoTiming; kwargs...) = false +stop_timing!(::NoTiming) = nothing +attribute_timing(::NoTiming) = nothing + +""" + timing_backend() -> TimingBackend + +The loaded timing backend. `NoTiming()` until `Profile` is loaded, after which +[`record`](@ref) reports seconds instead of `missing`. +""" +timing_backend() = _TIMING[] + +""" + recording() -> Bool + +Whether a [`record`](@ref) block is open. + +`false` by default and outside `record`, which is the whole cost argument: detection is on always +and free, counting happens only where somebody asked for it. +""" +recording() = _DEPTH[] > 0 + +const _DEPTH = Ref(0) +const _RECORD_LOCK = ReentrantLock() + +""" + record(f; paths = true, timing = true, with_profile = false, rethrow = true, maxdepth) -> Record + +Run `f` and report which marked definitions it entered, how often, and by which paths. + +```julia +r = record() do + simulate(model; steps = 10_000) +end +isempty(r) || @warn "used unvalidated code" [(h.name, h.count) for h in r] +``` + +A definition that was never entered is **absent**, not reported with a count of zero: the record +says what happened, and enumerating what did not is [`experimental`](@ref)'s job. + +| keyword | | +|---|---| +| `paths` | capture call paths. Bounded — see [`Hit`](@ref) — but still the expensive part | +| `timing` | ask the [`TimingBackend`](@ref) for `inclusive`/`exclusive`. Ignored if none is loaded | +| `with_profile` | leave whatever is already in the profile buffer alone instead of clearing it | +| `rethrow` | `false` returns the record for the part of `f` that ran instead of propagating | + +Nests: an inner `record` reports its own block, and the outer one still counts each entry once. +Counts are exact under threads — per-thread counters sized by `Threads.maxthreadid()`, not +`nthreads()`, because the interactive pool means a task can have a thread id above the worker +count. + +!!! note "This costs something, which is why it is a call" + Every marked body takes its write path while a recording is open. The record reports the + recorder's estimated share of the elapsed time in `overhead`; [`overhead_when_detecting`](@ref) + is the other number, and it is 3%. +""" +function record( + f; paths::Bool=true, timing::Bool=true, with_profile::Bool=false, rethrow::Bool=true +) + ps = probes() + slots = Threads.maxthreadid() + sampled = false + saved = Bool[] + @lock _RECORD_LOCK begin + if _DEPTH[] == 0 + saved = Bool[p.entered for p in ps] + for p in ps + _arm!(p, slots) + p.entered = false + end + _CAPTURE_PATHS[] = paths + _RECORDING[] = true + end + _DEPTH[] += 1 + end + before = Dict{Probe,Int}(p => _probe_count(p) for p in ps) + # Only the outermost block touches the sampler. Re-initialising `Profile` while its timer is + # running is a segmentation fault, not an error, and a nested `record` has no business + # disturbing the block that contains it. + outermost = _DEPTH[] == 1 + if timing && outermost + sampled = start_timing!(timing_backend(); clear=(!with_profile)) + end + t0 = time() + err = nothing + try + f() + catch e + err = e + end + elapsed = time() - t0 + sampled && stop_timing!(timing_backend()) + times = sampled ? attribute_timing(timing_backend()) : nothing + + counts = Dict{Probe,Int}(p => _probe_count(p) - get(before, p, 0) for p in ps) + traces = Dict{Probe,Vector{Vector{Symbol}}}(p => _paths_of(p) for p in ps) + @lock _RECORD_LOCK begin + _DEPTH[] -= 1 + if _DEPTH[] == 0 + _RECORDING[] = false + _CAPTURE_PATHS[] = true + for (i, p) in enumerate(ps) + p.entered = (i <= length(saved) && saved[i]) || counts[p] > 0 + end + end + end + err === nothing || rethrow && Base.rethrow(err) + + hits = Hit[] + for p in ps + n = counts[p] + n > 0 || continue + mk = mark(p.mod, p.name) + mk === nothing && continue + pth = traces[p] + inc, exc = _timing_for(times, p.mod, p.name, elapsed) + push!( + hits, + Hit( + p.mod, + p.name, + mk.reason, + n, + mk.sig === nothing ? nothing : _method_of(mk.sig), + unique(x[2] for x in pth if length(x) >= 2), + pth, + inc, + exc, + ), + ) + end + sort!(hits; by=h -> (string(h.mod), string(h.name))) + total = sum(h -> h.count, hits; init=0) + return Record( + hits, + true, + slots, + elapsed, + _estimate_overhead(total, elapsed), + _versions_of(hits), + sampled, + ) +end + +function _timing_for(times, mod::Module, name::Symbol, elapsed::Float64) + times === nothing && return (missing, missing) + t = get(times, (mod, name), nothing) + t === nothing && return (0.0, 0.0) + return (t[1] * elapsed, t[2] * elapsed) +end + +# Called once per probe at the end of the block, with the sampler already stopped and the recorded +# closure still alive — which is what keeps the addresses resolvable. +function _paths_of(p::Probe) + out = Vector{Symbol}[] + @lock p.lock begin + for bt in p.traces + names = _trace_names(bt) + isempty(names) || (names in out || push!(out, names)) + end + end + return out +end + +function _method_of(@nospecialize(sig)) + return try + which(sig) + catch + nothing + end +end + +function _versions_of(hits::Vector{Hit}) + d = Dict{String,Any}() + for h in hits + root = Base.moduleroot(h.mod) + v = try + pkgversion(root) + catch + nothing + end + d[string(nameof(root))] = v === nothing ? "unknown" : string(v) + end + return d +end + +# What one counted entry costs, measured once and cached. Reported rather than re-derived per run, +# because a per-hit timer would be the same mistake as a counter in the body: it would change the +# thing it measures. +const _PER_HIT = Ref(0.0) + +function _per_hit_seconds() + _PER_HIT[] > 0 && return _PER_HIT[] + p = Probe(@__MODULE__, :_calibration) + _arm!(p, Threads.maxthreadid()) + n = 100_000 + old_paths = _CAPTURE_PATHS[] + _CAPTURE_PATHS[] = false + _hit!(p) # warm + t0 = time_ns() + for _ in 1:n + _hit!(p) + end + t = (time_ns() - t0) / 1e9 / n + _CAPTURE_PATHS[] = old_paths + _PER_HIT[] = t + return t +end + +function _estimate_overhead(hits::Int, elapsed::Float64) + (hits == 0 || elapsed <= 0) && return 0.0 + return min(1.0, hits * _per_hit_seconds() / elapsed) +end + +""" + experimental_fraction(r::Record) -> Union{Float64,Missing} + +The share of the recorded run spent inside marked code, in `[0, 1]`. + +`missing` when no [`TimingBackend`](@ref) was loaded — a run whose time was never attributed has +no fraction, and reporting `0.0` would say the opposite of what is known. Derived from the +`inclusive` times, so it is time and not calls: one entry into a marked kernel that runs for a +minute matters more than a million into a marked accessor. +""" +function experimental_fraction(r::Record) + r.sampled || return missing + r.elapsed > 0 || return 0.0 + total = 0.0 + for h in r + h.inclusive === missing && return missing + total += h.inclusive + end + return min(1.0, total / r.elapsed) +end + +""" + merge_records(rs) -> Record + +Combine records — from separate processes, separate workers, or separate blocks — into one. + +Counts add, paths and callers union, elapsed times add. Order-independent and associative: workers +finish in whatever order they finish in, and provenance must not depend on that. +""" +function merge_records(rs) + byname = Dict{Tuple{Module,Symbol},Hit}() + for r in rs + for h in r + k = (h.mod, h.name) + prev = get(byname, k, nothing) + byname[k] = prev === nothing ? h : _merge(prev, h) + end + end + hits = sort!(collect(values(byname)); by=h -> (string(h.mod), string(h.name))) + elapsed = sum(r -> r.elapsed, rs; init=0.0) + total = sum(h -> h.count, hits; init=0) + versions = Dict{String,Any}() + for r in rs + merge!(versions, r.versions) + end + return Record( + hits, + all(r -> r.enabled, rs), + maximum(r -> r.slots, rs; init=0), + elapsed, + _estimate_overhead(total, elapsed), + versions, + any(r -> r.sampled, rs), + ) +end + +function _merge(a::Hit, b::Hit) + inc = if (a.inclusive === missing || b.inclusive === missing) + missing + else + a.inclusive + b.inclusive + end + exc = if (a.exclusive === missing || b.exclusive === missing) + missing + else + a.exclusive + b.exclusive + end + return Hit( + a.mod, + a.name, + a.reason, + a.count + b.count, + a.method === nothing ? b.method : a.method, + sort!(unique(vcat(a.callers, b.callers)); by=string), + sort!(unique(vcat(a.paths, b.paths)); by=x -> join(x, "/")), + inc, + exc, + ) +end + +""" + attribute(data) -> Vector{Attribution} + +Attribute an **existing** profile buffer to marked definitions, after the fact. + +```julia +using Profile +Profile.@profile long_run() +attribute(Profile.fetch()) +``` + +The twelve-hour-run case: a job that was already profiled must not have to be run again to learn +which of its time went through unvalidated code. What comes back is samples, not calls — which is +why it is an [`Attribution`](@ref) and not a [`Hit`](@ref). A sampling profiler cannot count +entries, and a field called `count` holding a sample total would read as a measurement it did not +make. + +!!! note "What sampling cannot see, and why counts exist" + A marked definition small enough to be inlined into its caller may leave **no** separately + attributable samples at all: after inlining there is no frame to attribute them to, and the + time is charged to whatever the optimiser left at that address. That is not a defect here, it + is what a sampling profiler is. It is also the reason [`record`](@ref)'s `count` is exact and + comes from a counter rather than from samples. +""" +attribute(data) = attribute(timing_backend(), data) + +function attribute(::NoTiming, data) + return throw( + ArgumentError( + "attribute: no timing backend is loaded — `using Profile` first, which is what " * + "supplies the one that can read a profile buffer", + ), + ) +end + +""" + Attribution + +One marked definition's share of a profile buffer, as reported by [`attribute`](@ref). + +`samples` is a sample count, never a call count: `inclusive` is the fraction of samples with this +definition anywhere on the stack, `exclusive` the fraction with it on top. +""" +struct Attribution + mod::Module + name::Symbol + reason::String + samples::Int + inclusive::Float64 + exclusive::Float64 +end + +function Base.show(io::IO, a::Attribution) + return print( + io, + "Attribution(", + a.mod, + ".", + a.name, + ", ", + a.samples, + " samples, inclusive ", + round(100 * a.inclusive; sigdigits=2), + "%)", + ) +end + +""" + write_record(path::AbstractString, r::Record) -> String + +Write a record to TOML. Returns `path`. + +Evidence, not a printout: what a run went through belongs next to the result it produced, and it +has to be readable by something that is not this package — a year later the package may not +resolve. See [`stamp`](@ref) for the same idea aimed at a result file rather than at a record. +""" +function write_record(path::AbstractString, r::Record) + d = Dict{String,Any}( + "enabled" => r.enabled, + "slots" => r.slots, + "elapsed" => r.elapsed, + "overhead" => r.overhead, + "sampled" => r.sampled, + "versions" => r.versions, + "entered" => [ + Dict{String,Any}( + "module" => string(h.mod), + "name" => string(h.name), + "reason" => h.reason, + "count" => h.count, + "callers" => string.(h.callers), + "paths" => [string.(p) for p in h.paths], + "inclusive" => h.inclusive === missing ? "unmeasured" : h.inclusive, + "exclusive" => h.exclusive === missing ? "unmeasured" : h.exclusive, + ) for h in r + ], + ) + open(path, "w") do io + return TOML.print(io, d) + end + return path +end + +""" + read_record(path::AbstractString) -> Record + +Read back a record written by [`write_record`](@ref). + +The `method` field of every [`Hit`](@ref) comes back `nothing`: a `Method` is not a thing a file +can carry, and reconstructing one would mean claiming the code in this process is the code that +produced the record. +""" +function read_record(path::AbstractString) + d = TOML.parsefile(path) + hits = Hit[] + for e in get(d, "entered", Dict{String,Any}[]) + mod = _module_by_name(e["module"]) + inc = e["inclusive"] + exc = e["exclusive"] + push!( + hits, + Hit( + mod, + Symbol(e["name"]), + e["reason"], + Int(e["count"]), + nothing, + Symbol.(get(e, "callers", String[])), + [Symbol.(p) for p in get(e, "paths", Vector{String}[])], + inc isa Real ? Float64(inc) : missing, + exc isa Real ? Float64(exc) : missing, + ), + ) + end + return Record( + hits, + get(d, "enabled", true), + Int(get(d, "slots", 0)), + Float64(get(d, "elapsed", 0.0)), + Float64(get(d, "overhead", 0.0)), + Dict{String,Any}(get(d, "versions", Dict{String,Any}())), + get(d, "sampled", false), + ) +end + +# A record read back names its modules as strings. Resolving them is best effort: the point of +# writing one is that it survives the package not being there. +function _module_by_name(s::AbstractString) + parts = Symbol.(split(s, ".")) + for root in values(Base.loaded_modules) + nameof(root) === parts[1] || continue + m = root + ok = true + for p in parts[2:end] + if isdefined(m, p) && getglobal(m, p) isa Module + m = getglobal(m, p) + else + ok = false + break + end + end + ok && return m + end + return Main +end + +""" + assert_clean(f; throw = true) -> Bool + +Run `f` and assert it entered nothing marked. + +The gate. `true` when the run touched no marked definition; otherwise it throws, naming every mark +it went through and why — or returns `false` if `throw = false`, which is what a caller doing its +own reporting wants. + +```julia +assert_clean() do + publish(compute(model)) +end +``` +""" +function assert_clean(f; throw::Bool=true) + r = record(f; paths=false, timing=false) + isempty(r) && return true + throw || return false + io = IOBuffer() + println(io, "assert_clean: the run entered ", length(r), " experimental definition(s):") + for h in r + println(io, " ", h.mod, ".", h.name, " ×", h.count, " — ", h.reason) + end + return Base.throw(ErrorException(String(take!(io)))) +end diff --git a/src/release.jl b/src/release.jl index c84c266..c63934e 100644 --- a/src/release.jl +++ b/src/release.jl @@ -2,49 +2,75 @@ # them at review time. This is the payoff for marking anything at all — "changing an experimental # name is not breaking" stops being an argument and becomes a function call. # -# It is also the least settled part of this package, so it says so in its own vocabulary. The -# name-list form is used here rather than six attached marks because the reason is one reason. +# Two units, one file, one schema. Names are what `names(m)` promises; methods are what a call +# site reaches, and a package whose surface is methods on somebody else's generic has no name-level +# covenant at all. The two live in one snapshot so that a repository has one file to commit. @experimental """ -the snapshot schema records names only; making it signature-aware would change the file format, \ -and nothing has yet been released against it -""" snapshot read_snapshot write_snapshot compare isbreaking Diff +the snapshot schema is young: nothing has been released against it, and the method half was added \ +after the name half, so a file written by one version may not be readable by the next +""" snapshot read_snapshot write_snapshot compare compare_methods isbreaking stamp Diff MethodDiff """ snapshot(m::Module) -> Dict{String,Any} -Write down what `m` currently promises: its [`stable`](@ref) names, and its experimental ones -with their reasons. +Write down what `m` currently promises: its [`stable`](@ref) names and its experimental ones with +their reasons, and the same split at method granularity. The result is plain `Dict`/`String`/`Vector` data, so `TOML.print` accepts it as-is -([`write_snapshot`](@ref) does that). Take one at each release; hand the old one and the new -module to [`compare`](@ref). +([`write_snapshot`](@ref) does that). Take one at each release; hand the old one and the new module +to [`compare`](@ref). ```toml module = "MyPackage" version = "0.4.2" stable = ["adapt", "measure"] +stable_methods = ["adapt(::Model, ::Grid)", "measure(::Model)"] [experimental.render_report] reason = "reads Test's internal result tree" tracking = "https://github.com/org/MyPackage.jl/issues/12" + +[experimental_methods."fetch_value(::Heisenberg, ::Energy)"] +reason = "numerically delicate; no reference value" ``` + +`methods = false` writes the name half only, which is what a package with no foreign methods and a +slow method search wants. """ -function snapshot(m::Module) +function snapshot(m::Module; methods::Bool=true) + # The two name lists partition the surface, which is what makes `compare`'s buckets add up: + # a name is experimental here exactly when `stable` left it out, and that is the `wholly` + # rule — a name with one marked method out of four is still a promise. + st = stable(m) exp = Dict{String,Any}() - for mk in experimental(m) - e = Dict{String,Any}("reason" => mk.reason) - mk.since === nothing || (e["since"] = string(mk.since)) - mk.tracking === nothing || (e["tracking"] = mk.tracking) - exp[string(mk.name)] = e + for n in setdiff(surface(m), st) + mk = mark(m, n) + mk === nothing && continue + exp[string(n)] = _mark_entry(mk) end d = Dict{String,Any}( - "module" => string(nameof(m)), "stable" => string.(stable(m)), "experimental" => exp + "module" => string(nameof(m)), "stable" => string.(st), "experimental" => exp ) + if methods + expm = Dict{String,Any}() + for mk in experimental_methods(m) + expm[_signature_key(mk)] = _mark_entry(mk) + end + d["experimental_methods"] = expm + d["stable_methods"] = [_signature_key(mm) for mm in stable_methods(m)] + end v = pkgversion(m) v === nothing || (d["version"] = string(v)) return d end +function _mark_entry(mk::Mark) + e = Dict{String,Any}("reason" => mk.reason) + mk.since === nothing || (e["since"] = string(mk.since)) + mk.tracking === nothing || (e["tracking"] = mk.tracking) + return e +end + """ write_snapshot(path::AbstractString, m::Module) -> String @@ -54,9 +80,9 @@ Committing this file at each release is what gives the next release something to against; a repository with no committed snapshot can be told what is experimental *now*, but not what changed. """ -function write_snapshot(path::AbstractString, m::Module) +function write_snapshot(path::AbstractString, m::Module; methods::Bool=true) open(path, "w") do io - return TOML.print(io, snapshot(m)) + return TOML.print(io, snapshot(m; methods)) end return path end @@ -93,6 +119,26 @@ struct Diff added_experimental::Vector{Symbol} end +""" + MethodDiff + +[`Diff`](@ref) at method granularity — the same six buckets, keyed by signature rather than by +name, as produced by [`compare_methods`](@ref). + +A signature key carries the argument types and the keyword **names**, so a method whose arguments +or keywords changed reads as one key removed and another added. That is the blind spot `compare` +admits to and this closes; see [`compare_methods_sees_keywords`](@ref) for the part that stays +open. +""" +struct MethodDiff + removed_stable::Vector{String} + demoted::Vector{String} + removed_experimental::Vector{String} + promoted::Vector{String} + added_stable::Vector{String} + added_experimental::Vector{String} +end + """ compare(old::AbstractDict, new::Union{Module,AbstractDict}) -> Diff @@ -112,17 +158,40 @@ Retroactively withdrawing a promise is a change to what callers were told, not a it. !!! warning "Names, not signatures" - This compares name sets. A name present in both snapshots whose method signature, return - type or keyword arguments changed is a breaking change that `compare` cannot see, and no - amount of marking makes it visible. Read the diff as a floor on breakage, never as a - clearance. + This compares name sets. A name present in both snapshots whose method signature, return type + or keyword arguments changed is a breaking change `compare` cannot see. [`compare_methods`](@ref) + is the finer instrument; read this one as a floor on breakage, never as a clearance. """ compare(old::AbstractDict, new::Module) = compare(old, snapshot(new)) function compare(old::AbstractDict, new::AbstractDict) - os, oe = _sets(old) - ns, ne = _sets(new) - return Diff( + os, oe = _sets(old, "stable", "experimental", Symbol) + ns, ne = _sets(new, "stable", "experimental", Symbol) + return Diff(_buckets(os, oe, ns, ne)...) +end + +""" + compare_methods(old::AbstractDict, new::Union{Module,AbstractDict}) -> MethodDiff + +[`compare`](@ref) at method granularity: what moved between two snapshots' `stable_methods` and +`experimental_methods`. + +The unit a call site actually reaches. `fetch(::Ising, ::Energy)` and `fetch(::Heisenberg, ::Energy)` +are one name and two promises, and removing the second is breaking exactly when it was not marked. + +Reads the same snapshot file `compare` does — the two keys mirror the name-level pair — so a +repository commits one file and a release script asks it both questions. +""" +compare_methods(old::AbstractDict, new::Module) = compare_methods(old, snapshot(new)) + +function compare_methods(old::AbstractDict, new::AbstractDict) + os, oe = _sets(old, "stable_methods", "experimental_methods", String) + ns, ne = _sets(new, "stable_methods", "experimental_methods", String) + return MethodDiff(_buckets(os, oe, ns, ne)...) +end + +function _buckets(os, oe, ns, ne) + return ( sort!(collect(setdiff(os, ns, ne))), sort!(collect(intersect(os, ne))), sort!(collect(setdiff(oe, ns, ne))), @@ -132,27 +201,97 @@ function compare(old::AbstractDict, new::AbstractDict) ) end -function _sets(d::AbstractDict) - st = Set(Symbol.(get(d, "stable", String[]))) - ex = Set(Symbol.(keys(get(d, "experimental", Dict{String,Any}())))) +function _sets(d::AbstractDict, skey::String, ekey::String, T::Type) + st = Set(T.(get(d, skey, String[]))) + ex = Set(T.(collect(keys(get(d, ekey, Dict{String,Any}()))))) return st, ex end """ - isbreaking(d::Diff) -> Bool + compare_methods_sees_keywords + +`true`. A signature key carries the keyword **names** a method declares, so adding, removing or +renaming one moves the key and [`compare_methods`](@ref) reports it. + +Stated as a constant rather than left to be discovered, because the neighbouring limit is real and +has to be stated with it: keyword **defaults** live in the method body and are invisible here, so +changing `tol = 1e-8` to `tol = 1e-6` moves nothing. Names yes, defaults no. +""" +const compare_methods_sees_keywords = true + +""" + isbreaking(d::Union{Diff,MethodDiff}) -> Bool -Whether `d` removes a settled name or demotes one — the two moves that break callers who were -told the truth. +Whether `d` removes a settled name or method, or demotes one — the two moves that break callers +who were told the truth. This is the release gate: a `true` here means the version bump is major (or, under Julia's 0.x -convention, a minor bump). Because [`compare`](@ref) reads names and not signatures, `false` +convention, a minor bump). Because a diff reads names and signatures and not behaviour, `false` means *no breakage of this kind was found*, not *nothing broke*. """ isbreaking(d::Diff) = !isempty(d.removed_stable) || !isempty(d.demoted) +isbreaking(d::MethodDiff) = !isempty(d.removed_stable) || !isempty(d.demoted) + +# The key a method is recorded under. `f(::Int64; tol, maxiter)` — argument types positionally, +# keyword names alphabetically. Readable in a committed file, and stable under anything that does +# not change the signature. +function _signature_key(mm::Method) + return _signature_key(mm.name, mm.sig, Base.kwarg_decl(mm)) +end + +function _signature_key(mk::Mark) + mk.sig === nothing && return string(mk.name) + mm = try + which(mk.sig) + catch + nothing + end + kw = mm === nothing ? Symbol[] : Base.kwarg_decl(mm) + name = mm === nothing ? mk.name : mm.name + return _signature_key(name, mk.sig, kw) +end + +function _signature_key(name::Symbol, @nospecialize(sig), kwargs::Vector{Symbol}) + s = Base.unwrap_unionall(sig) + args = if (s isa DataType && s <: Tuple && length(s.parameters) >= 1) + s.parameters[2:end] + else + Any[] + end + io = IOBuffer() + print(io, name, "(") + print(io, join(("::" * _type_key(a) for a in args), ", ")) + kw = sort(filter(k -> !endswith(String(k), "..."), kwargs); by=String) + isempty(kw) || print(io, "; ", join(kw, ", ")) + print(io, ")") + return String(take!(io)) +end + +# Type names without their defining module: a snapshot committed by one package and read by +# another must not move because a module was renamed around the type. +function _type_key(@nospecialize(t)) + t isa TypeVar && return String(t.name) + b = Base.unwrap_unionall(t) + if b isa DataType + isempty(b.parameters) && return String(nameof(b)) + return String(nameof(b)) * + "{" * + join((_type_key(p) for p in b.parameters), ", ") * + "}" + end + return string(t) +end function Base.show(io::IO, ::MIME"text/plain", d::Diff) - println(io, "Diff — ", isbreaking(d) ? "BREAKING" : "not breaking") - for (label, v, breaks) in ( + return _show_diff(io, "Diff", d, isbreaking(d)) +end +function Base.show(io::IO, ::MIME"text/plain", d::MethodDiff) + return _show_diff(io, "MethodDiff", d, isbreaking(d)) +end + +function _show_diff(io::IO, label::String, d, breaking::Bool) + println(io, label, " — ", breaking ? "BREAKING" : "not breaking") + for (name, v, breaks) in ( ("removed (stable)", d.removed_stable, true), ("demoted to experimental", d.demoted, true), ("removed (experimental)", d.removed_experimental, false), @@ -161,7 +300,74 @@ function Base.show(io::IO, ::MIME"text/plain", d::Diff) ("added (experimental)", d.added_experimental, false), ) isempty(v) && continue - println(io, " ", breaks ? "! " : " ", rpad(label, 24), join(v, ", ")) + println(io, " ", breaks ? "! " : " ", rpad(name, 24), join(v, ", ")) end return nothing end + +""" + stamp(path::AbstractString, f) -> String + stamp(path::AbstractString, r::Record) -> String + +Run `f`, and write next to its result a record of which unvalidated code paths produced it. + +The end state this package is for: a figure's directory says what the number came out of, in plain +TOML that a reader a year later can open without resolving the package that wrote it. + +```julia +stamp("figures/energy_sweep.provenance.toml") do + sweep(model; βs = 0.05:0.05:2.0) +end +``` + +Returns `path`. The file carries every mark the run entered, its reason, how often it was entered, +and the versions of the packages the marks came from — see [`stamp_versions`](@ref). +""" +function stamp(path::AbstractString, f) + return stamp(path, record(f)) +end + +function stamp(path::AbstractString, r::Record) + d = Dict{String,Any}( + "generated" => string(Dates_now()), + "julia" => string(VERSION), + "elapsed" => r.elapsed, + "versions" => stamp_versions(), + "experimental" => [ + Dict{String,Any}( + "module" => string(h.mod), + "name" => string(h.name), + "reason" => h.reason, + "count" => h.count, + ) for h in r + ], + ) + open(path, "w") do io + return TOML.print(io, d) + end + return path +end + +# `Dates` is not a dependency and one timestamp does not justify making it one. +Dates_now() = Base.Libc.strftime("%Y-%m-%dT%H:%M:%S%z", time()) + +""" + stamp_versions() -> Dict{String,Any} + +The version of every loaded package, for [`stamp`](@ref) to write beside a result. + +`energy` being experimental in v0.3 says nothing about v0.9, so a provenance record that names +marks without naming versions describes a state nobody can get back to. +""" +function stamp_versions() + d = Dict{String,Any}("julia" => string(VERSION)) + for (id, mod) in Base.loaded_modules + v = try + pkgversion(mod) + catch + nothing + end + v === nothing || (d[String(id.name)] = string(v)) + end + return d +end diff --git a/src/verify.jl b/src/verify.jl new file mode 100644 index 0000000..c921f0b --- /dev/null +++ b/src/verify.jl @@ -0,0 +1,288 @@ +# How well a marked definition is exercised by the tests. +# +# No new machinery: the mark already carries the file and line its definition starts at, and +# `--code-coverage` already writes a count per line. Joining the two answers the worst case a +# marked definition can be in — unvalidated code that its own suite never runs — and it answers it +# without anyone writing another list. +# +# Coverage counts are flushed from the running process rather than read from the `.cov` files +# Julia writes at exit, because a test that has to wait for the process to end cannot assert +# anything. + +""" + Verification + +How much of one marked definition the current run exercised. + +| field | | +|---|---| +| `mark` | the declaration | +| `covered`, `total` | executable lines run, and executable lines in the definition | +| `fraction` | `covered / total`, `0.0` for a definition the run never entered, or `missing` | + +`missing` is not zero. A run without `--code-coverage` has nothing to say about *how much* of a +definition ran, and reporting `0.0` there would flag every marked definition in every ordinary run. + +Whether it ran **at all** is a different question, and this package already answers it exactly: +the probe. A marked definition whose flag never fired is `0.0` whatever the line counters say — +which is not a refinement but a correction. Measured on 1.14.0-DEV.3115: `--code-coverage` now +emits a counter for the *definition* line of a method nothing ever called, so a one-line +definition comes back "fully covered" on the strength of having been defined. Up to 1.12 that line +had no counter at all, so the two versions disagree about the same file, and only one of them can +be read as "the suite ran this". +""" +struct Verification + mark::Mark + covered::Int + total::Int + fraction::Union{Float64,Missing} +end + +function Base.show(io::IO, v::Verification) + f = if v.fraction === missing + "unmeasured" + else + string(round(100 * v.fraction; digits=1), "%") + end + return print(io, "Verification(", v.mark.mod, ".", v.mark.name, ", ", f, ")") +end + +""" + coverage_enabled() -> Bool + +Whether this process was started with `--code-coverage`. + +The question [`coverage`](@ref) has to ask before reporting a number: without it there are no +counts at all, and the honest answer is `missing`. +""" +coverage_enabled() = Base.JLOptions().code_coverage != 0 + +""" + coverage(m::Module, name::Symbol) -> Union{Float64,Missing} + +The fraction of `name`'s definition that this run executed, or `missing` if coverage is off. + +Partial coverage comes back partial: a definition whose error branch is never taken is not +verified, and reporting it as `1.0` would be the false pass this whole package exists to remove. +""" +function coverage(m::Module, name::Symbol) + mk = mark(m, name) + mk === nothing && return missing + v = _verify(mk, _coverage_data()) + return v.fraction +end + +""" + verification(m::Module) -> Vector{Verification} + +One [`Verification`](@ref) per mark in `m`, sorted by name. + +Data, not a printout — the same convention [`audit`](@ref) follows. Flushes the process's coverage +counters once and joins them against every mark, so asking about twenty marks costs what asking +about one does. +""" +function verification(m::Module) + data = _coverage_data() + return [_verify(mk, data) for mk in experimental(m)] +end + +""" + unverified(m::Module) -> Vector{Mark} + +The marks whose definitions this run never executed at all. + +The worst case, and the one worth a separate verb: code that is both unvalidated and untested. + +Does **not** need `--code-coverage`. The signal is the probe the mark already emits, which is +exact and costs nothing; coverage adds the partial fraction, which is a different question. +A mark on a `struct`, a `const` or a name list carries no probe and is not listed — there is +nothing to enter. +""" +function unverified(m::Module) + return [v.mark for v in verification(m) if v.fraction !== missing && v.fraction == 0.0] +end + +""" + stale_marks(m::Module) -> Vector{Mark} + +The marks whose recorded line no longer holds a `@experimental` declaration. + +A mark records where it was written. An edit above it moves the code and not the record, and every +join keyed on that line — coverage here, and anything downstream reading `mk.file`/`mk.line` — +then describes the wrong lines silently. Re-loading the module fixes it, which is why this reports +rather than repairs: a stale mark means the module on disk and the module in memory differ. +""" +function stale_marks(m::Module) + out = Mark[] + for mk in experimental(m) + lines = _source_lines(String(mk.file)) + lines === nothing && continue + if mk.line < 1 || + mk.line > length(lines) || + !occursin("@experimental", lines[mk.line]) + push!(out, mk) + end + end + return out +end + +function _verify(mk::Mark, data) + # The probe first, and it overrides. It is the exact answer to "was this entered", it needs no + # coverage run, and on 1.14-DEV it is the only one of the two signals that is still right: + # a definition line now carries a counter whether or not anything ever called the method. + _entered_flag(mk) === false && return Verification(mk, 0, 0, 0.0) + span = _definition_span(mk) + (span === nothing || data === nothing) && return Verification(mk, 0, 0, missing) + counts = get(data, _realpath(String(mk.file)), Dict{Int,Int}()) + covered = 0 + total = 0 + for ln in span[1]:span[2] + c = get(counts, ln, nothing) + c === nothing && continue + total += 1 + c > 0 && (covered += 1) + end + total > 0 && return Verification(mk, covered, total, covered / total) + # No counters anywhere in the span. Up to 1.12 that is itself the answer for a definition with + # a body — Julia instruments a line when it generates code for it, so their absence says + # nothing ever called it. A mark on a `struct`, a `const` or a name list has no body, and for + # those there is genuinely nothing to measure. + return Verification(mk, 0, 0, mk.sig === nothing ? missing : 0.0) +end + +# `true`, `false`, or `nothing` when the mark carries no probe — a name list, a `struct`, a +# `const`, a `macro`, a `@generated` function. +function _entered_flag(mk::Mark) + p = _flag(mk.mod, mk.name) + return p === nothing ? nothing : p[] +end + +# The line range one declaration occupies: from the `@experimental` line to just before the next +# statement BESIDE it. Read from the source rather than from the method, because a mark may cover +# a `struct` or a name list, which have no method to ask. +# +# "Beside it" is the whole difficulty. The next `LineNumberNode` above the declaration is usually +# the first line of its own body, so a span computed that way is one line long and reports every +# multi-line definition as fully covered by its own signature. +function _definition_span(mk::Mark) + lines = _source_lines(String(mk.file)) + lines === nothing && return nothing + tree = _parsed(String(mk.file)) + tree === nothing && return nothing + return _find_span(tree, mk.line, length(lines)) +end + +# The statements in one container, each with the line it starts on and the line before its next +# sibling starts. A container's own last statement runs to the container's end. +function _sibling_spans(x, outer_end::Int) + entries = Tuple{Int,Any}[] + cur = 0 + for a in x.args + if a isa LineNumberNode + cur = a.line + elseif a !== nothing + push!(entries, (cur, a)) + end + end + out = Tuple{Int,Int,Any}[] + for (i, (ln, st)) in enumerate(entries) + stop = i < length(entries) ? max(ln, entries[i + 1][1] - 1) : outer_end + push!(out, (ln, stop, st)) + end + return out +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) + (a <= line <= b) || continue + for body in _containers(st) + r = _find_span(body, line, b) + r === nothing || return r + end + end + return nothing +end + +# Where a statement can hold further statements of its own: a module's block, a bare block, and +# the body of anything else that carries one. +function _containers(st) + st isa Expr || return () + st.head === :module && return (st.args[3],) + (st.head === :block || st.head === :toplevel) && return (st,) + return Tuple(a for a in st.args if a isa Expr && a.head in (:block, :toplevel)) +end + +const _SOURCE_CACHE = Dict{String,Union{Vector{String},Nothing}}() +const _PARSE_CACHE = Dict{String,Any}() + +function _source_lines(path::AbstractString) + return get!(_SOURCE_CACHE, String(path)) do + isfile(path) || return nothing + return readlines(path) + end +end + +function _parsed(path::AbstractString) + return get!(_PARSE_CACHE, String(path)) do + isfile(path) || return nothing + return try + Meta.parseall(read(path, String); filename=path) + catch + nothing + end + end +end + +_realpath(p::AbstractString) = + try + realpath(p) + catch + String(p) + end + +""" + flush_coverage() -> Union{Dict,Nothing} + +Write this process's coverage counters out and read them back, or `nothing` if coverage is off. + +Julia writes them at exit, which is too late for a test to assert on. The same C entry point the +exit hook uses is called here instead, into a temporary file that is read and removed. +""" +function flush_coverage() + coverage_enabled() || return nothing + path = tempname() * ".info" + try + ccall(:jl_write_coverage_data, Cvoid, (Cstring,), path) + isfile(path) || return nothing + return _parse_lcov(path) + catch + return nothing + finally + isfile(path) && rm(path; force=true) + end +end + +# Re-flushed on every call rather than cached: a test asks about coverage after running the code +# it wants covered, and a cached answer from before that would report zero. +_coverage_data() = flush_coverage() + +function _parse_lcov(path::AbstractString) + out = Dict{String,Dict{Int,Int}}() + file = "" + for line in eachline(path) + if startswith(line, "SF:") + file = _realpath(line[4:end]) + get!(out, file, Dict{Int,Int}()) + elseif startswith(line, "DA:") + parts = split(line[4:end], ',') + length(parts) == 2 || continue + ln = tryparse(Int, parts[1]) + n = tryparse(Int, parts[2]) + (ln === nothing || n === nothing) && continue + out[file][ln] = n + end + end + return out +end diff --git a/test/spec/README.md b/test/spec/README.md index 82e2dd7..0cddebb 100644 --- a/test/spec/README.md +++ b/test/spec/README.md @@ -1,41 +1,43 @@ # The case matrix -These files are the specification. They are written before the implementation, so most of them -are `@test_broken`. - -`@test_broken` was chosen deliberately over comments or a TODO list: +These files are the specification. They were written before the implementation, so most of them +started as `@test_broken`; the implementation caught up and every one of them is a live assertion +now. What the register was for has not gone away — it inverted: * an expression that throws (because the function does not exist yet) registers as **Broken**, - not as an error, so the suite stays green while the spec is incomplete; + not as an error, so the suite stays green while a spec is incomplete; * an expression that starts **passing** registers as `Error: Unexpected Pass`, which fails the suite until someone promotes it to `@test`. So the spec cannot silently rot in either direction: it neither blocks work that has not been -done, nor lets finished work go unnoticed. +done, nor lets finished work go unnoticed. With the count now at zero, the same mechanism is a +**ratchet** — a behaviour demoted back to `@test_broken` moves the number `test/test_spec_table.jl` +pins, and the suite says so. One caveat, measured rather than assumed: an expression whose value is not a `Bool` — an `@eval module … end`, whose value is a `Module` — reports `Error: Expression evaluated to non-Boolean` instead of `Unexpected Pass`. Both fail the suite loudly, so nothing rots either -way, but every `@eval module` block in this directory now ends in a `Bool` and checks *what* got +way, but every `@eval module` block in this directory ends in a `Bool` and checks *what* got marked, so the message is the expected one and the assertion has content beyond "it parsed". ## Overlap with `test/test_*.jl` `test_spec_declare.jl` re-covers definition forms that `test/test_mark.jl` already pins, and `test_spec_docstring.jl` re-covers part of `test/test_audit.jl`'s bucket contract, through -separate fixtures. That is deliberate while the spec is the design document — but two fixtures +separate fixtures. That was deliberate while the spec was the design document — but two fixtures pinning one contract have to be kept in sync by hand, so the older files should be folded in or -retired once the spec stops moving. +retired now that the spec has stopped moving. Anything already implemented is a plain `@test`. ## What is covered The measure is **distinct behaviours** — one per leaf `@testset` — not assertions. An assertion -count moves without any implementation progress: `test_spec_declare.jl` has 36 assertion lines -but 91 runtime assertions, because several run inside `for mk in experimental(Declared)`, so a -thirteenth fixture mark would buy four more passing assertions and cover nothing new. A leaf -testset is one claim, and adding one means writing one. +count moves without any implementation progress: several run inside `for mk in experimental(…)`, +so a thirteenth fixture mark would buy four more passing assertions and cover nothing new. A leaf +testset is one claim, and adding one means writing one. A `@testset` inside a helper function is +machinery rather than a claim and is not counted — that is what the gate probes in +`test_spec_integration.jl` are. *operating today* is the column that matters when reading a claim about this directory: a leaf that is entirely `@test_broken` is a claim written down, not a check being run. @@ -43,17 +45,17 @@ that is entirely `@test_broken` is a claim written down, not a check being run. | file | behaviours | operating today | specified only | concern | |---|---|---|---|---| -| `test_spec_declare.jl` | 11 | 7 | 4 | what can carry a mark: function, method, struct, const, module, macro, extension | -| `test_spec_dispatch.jl` | 14 | 4 | 10 | one call site, several methods, only some marked — the branch | -| `test_spec_docstring.jl` | 9 | 8 | 1 | a mark and a docstring are different accounts and must coexist | -| `test_spec_foreign.jl` | 14 | 5 | 9 | marking a method on somebody else's generic — the `QAtlas.fetch` case | -| `test_spec_forms.jl` | 26 | 15 | 11 | the definition forms a real package hits on its second afternoon | -| `test_spec_integration.jl` | 17 | 1 | 16 | where the mark has to surface: docs, Aqua, releases, provenance, CI | -| `test_spec_lifecycle.jl` | 15 | 7 | 8 | the mark's EXIT, and an entry point that is a module rather than a function | -| `test_spec_profile.jl` | 40 | 12 | 28 | what a real run went through, how often, and how much of it | -| `test_spec_propagate.jl` | 20 | 2 | 18 | a caller that never names a marked thing still depends on it | -| `test_spec_verify.jl` | 8 | 2 | 6 | how well is a marked thing exercised by the tests | -| **10 files** | **174** | **63** | **111** | | +| `test_spec_declare.jl` | 12 | 12 | 0 | what can carry a mark: function, method, struct, const, module, macro, extension | +| `test_spec_dispatch.jl` | 15 | 15 | 0 | one call site, several methods, only some marked — the branch | +| `test_spec_docstring.jl` | 9 | 9 | 0 | a mark and a docstring are different accounts and must coexist | +| `test_spec_foreign.jl` | 13 | 13 | 0 | marking a method on somebody else's generic — the `QAtlas.fetch` case | +| `test_spec_forms.jl` | 24 | 24 | 0 | the definition forms a real package hits on its second afternoon | +| `test_spec_integration.jl` | 18 | 18 | 0 | where the mark has to surface: docs, Aqua, releases, provenance, CI | +| `test_spec_lifecycle.jl` | 15 | 15 | 0 | the mark's EXIT, and an entry point that is a module rather than a function | +| `test_spec_profile.jl` | 41 | 41 | 0 | what a real run went through, how often, and how much of it | +| `test_spec_propagate.jl` | 20 | 20 | 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** | **176** | **176** | **0** | | The table is generated and pinned by `test/test_spec_table.jl`, which fails if it goes stale — @@ -86,6 +88,21 @@ opt-in.** The `@warn` rows are why the default notice is a summary at process ex warning at first entry: the cost is the logging call sitting in the body, not the warning being printed, and guarding it so that it fires once does not recover it. +### How `record` counts without a counter in the body + +The emitted statement never changed. `record` opens a block by clearing every probe's flag, so +the short-circuit fails and the *write* side runs on every call — and the write side is a +function call, not an inlined store, so it can do the counting. Nothing in a marked body is +different inside a recording; what differs is which branch of the same one statement is taken. + +Two consequences worth having written down. Counts are **exact** and survive inlining, which is +what ruled the sampling route out — a marked definition small enough to be worth marking is small +enough to be inlined, and a sampler has no frame left to attribute to. And the probe statement +carries the declaration's own `LineNumberNode`, which is load bearing rather than cosmetic: the +write side is a cold branch that the optimiser is free to sink, and without a location of its own +it inherits whichever statement happens to be next — so `record`'s call paths came back reporting +`iterate` and `+` where `energy` and `inner` belonged. + ### One requirement was withdrawn This directory used to require that `@experimental` **never wrap the call**, and @@ -95,47 +112,60 @@ statement must be read-mostly, must add exactly one statement, and must not brin machinery with it. The last of those is checked today, with a macro that *does* log as the positive control. +### A second requirement was withdrawn, and this one could not be met + +`test_spec_forms.jl` required that a mark written inside a function body be refused with a message +naming `@experimental`. It cannot be, and the three routes are exhausted: + + * `const` in local scope fails during **lowering**, before any emitted code runs, so no check of + ours can intercept it — and Julia's message does not name the variable either. Measured on + 1.12.2: the message is byte-identical for `__EXPERIMENTAL_API_MARKS__` and for a binding whose + name is the whole explanatory sentence, so there is no smuggling the word in. + * `global`, the one expansion that avoids `const`, fails **silently** in local scope — worse + than a loud message in the wrong vocabulary. + * creating the registry through `Core.eval` removes the error altogether, which turns a refusal + into a mark quietly registered when the enclosing function is first called. + +What is kept, and asserted, is the part that was in this package's hands: the blame lands on the +line the author wrote, and never inside this package. + ## Negative controls -Each group has a negative control **specified**; most are not yet operating, because the control -is `@test_broken` alongside the claim it controls. They are listed here as design, not as -evidence: - -| group | the control | operating? | -|---|---|---| -| propagate | `top_good` is *exactly as deep* as `top_bad` and must come back `:clean` | no | -| profile | `cold` is marked and never called, and must be **absent**, not reported with count zero | no | -| verify | the fixture is exercised only *partly*, so a checker that always reports 100% cannot pass | no | -| integration | a settled name must get **no** docs note | no | -| dispatch | `which()` really does throw for the branching signatures the file rests on | **yes** | -| forms | the misuse refusals name the missing half, rather than one generic message | **yes** | -| lifecycle | deleting a settled name is still breaking, so `isbreaking` cannot answer `false` always | **yes** | - -"no" means the assertion about the *implementation* does not run, because the implementation is -not there. It does not mean the row is unchecked: the fixture premise each control rests on is -pinned live where it could be got wrong — `test_spec_verify.jl:38` asserts the fixture really is -only partly exercised, and `test_spec_dispatch.jl:93` asserts the specificity relation the whole -file assumes. A control resting on a false premise is the failure mode those guard. +Each group has a negative control, and all of them now run: + +| group | the control | +|---|---| +| propagate | `top_good` is *exactly as deep* as `top_bad` and comes back `:clean` | +| profile | `cold` is marked and never called, and is **absent**, not reported with count zero | +| verify | the fixture is exercised only *partly*, so a checker that always reports 100% cannot pass | +| integration | a settled name gets **no** docs note, and a settled-but-hot function is not attributed | +| dispatch | `which()` really does throw for the branching signatures the file rests on | +| forms | the misuse refusals name the missing half, rather than one generic message | +| lifecycle | deleting a settled name is still breaking, so `isbreaking` cannot answer `false` always | +| foreign | one dependent's mark does not make the upstream generic experimental for anybody | The one that matters most is not in the table because it is a rule rather than a fixture: `:unknown` must never be reported as `:clean`. Two fixtures (`Holder.f::Function`, `TABLE[i](x)`) really can reach the marked function while being statically invisible. Answering "no experimental dependency" there is not a weaker claim, it is a false one. -## What the spec already found - -Two defects, both live in the shipped code, both of the kind the spec was written to catch — -a mark that silently records the wrong thing rather than refusing: - -1. `@experimental "…" (c::C)(x) = c.k * x` marks **`:c`**, the argument name. Not the type, not - a function — a local that is not a binding anywhere. It produces **two** wrong signals, not - one: the audit reports `:c` as *dangling* (declared, no such binding) **and** `:C` as - *unaccounted* (public, never declared), so it tells the author to go declare the very thing - that line declares. Both halves have to move together; `test_spec_forms.jl` pins each. -2. A mark inside a function body is refused by *Julia*, not by this package: - `syntax: unsupported const declaration on local variable`. Half fixed — the expansion now - carries the caller's `LineNumberNode`, so the message names the line the author wrote instead - of `ExperimentalAPI/src/mark.jl`, which read as a bug in the package. The message still never - says `@experimental`, and may not be able to: `const` in local scope fails during lowering, - before any emitted code runs, and the one alternative that avoids `const` (`global`) fails - *silently* in local scope, which is worse. +## What the spec found + +Defects the spec was written to catch, all of them a mark silently recording the wrong thing +rather than refusing: + +1. `@experimental "…" (c::C)(x) = c.k * x` marked **`:c`**, the argument name. Not the type, not + a function — a local that is not a binding anywhere. It produced **two** wrong signals: the + audit reported `:c` as *dangling* (declared, no such binding) **and** `:C` as *unaccounted* + (public, never declared), so it told the author to go and declare the very thing that line + declares. Both halves move together, and `test_spec_forms.jl` pins each. +2. `since = "0.4.0"` and a non-string reason were refused **by accident** — the field's own + conversion failed, with a `MethodError` naming neither the keyword nor `@experimental`. A + refusal the author cannot act on is barely better than none. +3. A name-keyed mark over-claimed: `@experimental "…" energy(::Numerical) = …` made + `energy(::Exact)` experimental too, so a call site that can only reach the exact method was + reported as depending on unvalidated code. The mark now records the signature it attached to, + and `stable` keeps a name in the covenant until *every* method behind it is marked. +4. The spec's own `invoke` case named an entry signature no method of the fixture matches + (`Tuple{Numerical}` against `via_invoke(x::Int)`), and its `@eval`-in-a-loop fixture + interpolated `$n` at the wrong level. Both were invisible while the assertions were Broken. diff --git a/test/spec/summary.jl b/test/spec/summary.jl index a220400..b654d73 100644 --- a/test/spec/summary.jl +++ b/test/spec/summary.jl @@ -41,12 +41,20 @@ end function _walk(f, x) f(x) - x isa Expr && for a in x.args + x isa Expr && !_isdefinition(x) && for a in x.args _walk(f, a) end return nothing end +# A `@testset` written inside a helper function is machinery, not a claim: the gate probes in +# `test_spec_integration.jl` run `test_surface` under a test set that records instead of +# propagating, and counting those as behaviours reported five claims that nobody wrote. +function _isdefinition(x::Expr) + x.head === :function && return true + return x.head === :(=) && x.args[1] isa Expr && x.args[1].head in (:call, :where, :(::)) +end + _count(pred, x) = (n=0; _walk(y -> (pred(y) && (n += 1)), x); n) """ @@ -56,8 +64,9 @@ What one spec file contains. `operating` and `specified` partition `behaviours`: either runs at least one assertion against the package today, or it is entirely `@test_broken` — a claim written down and not yet checked against anything. -The distinction is the point: "every group has a negative control" is true of this directory as a -specification and false of it as a running suite. +The whole directory is `operating` now, and the split earns its place going the other way: it is +the ratchet. A behaviour added as `@test_broken`, or one demoted back to it, moves the number that +`test/test_spec_table.jl` pins, and the suite says so. """ struct Counts behaviours::Int diff --git a/test/spec/test_spec_declare.jl b/test/spec/test_spec_declare.jl index e57fe04..2d9fa1e 100644 --- a/test/spec/test_spec_declare.jl +++ b/test/spec/test_spec_declare.jl @@ -1,7 +1,8 @@ # What can carry a mark: function, method, struct, const, module, macro, extension. # -# Scope: both what the name-keyed implementation does today (plain `@test`) and the method-level -# unit it has to become (`@test_broken`). +# Scope: both units. A mark names something, and — when it attached to a definition — also records +# the signature it attached to, so the same declaration answers `audit`'s question about the name +# and `reach`'s question about the method. using ExperimentalAPI: ExperimentalAPI, @experimental, Mark, experimental, isexperimental using Test @@ -109,16 +110,19 @@ end # # `Declared.energy` has three methods; marking the name marks all three, including the exact one. -@testset "marking a name is the wrong unit when the name has many methods" begin +@testset "the name is the wrong unit when the name has many methods" begin @test length(methods(Declared.energy)) == 3 - # Today the statement is about the name, so it over-claims. - @test !isexperimental(Declared, :energy) # not marked at all yet — see below + # Nothing is marked here yet, so the three assertions below start from a clean fixture. + @test !isexperimental(Declared, :energy) end @testset "a single dispatch path can be marked" begin m = which(Declared.energy, Tuple{Declared.Numerical,Float64}) - @test_broken ExperimentalAPI.mark(m) isa Mark - @test_broken ExperimentalAPI.isexperimental(m) + @test ExperimentalAPI.mark_method!(m, "extrapolated below β ≈ 0.1") isa Mark + @test ExperimentalAPI.mark(m) isa Mark + @test ExperimentalAPI.isexperimental(m) + # The record knows the signature it is about, which is what makes it narrower than the name. + @test ExperimentalAPI.mark(m).sig === m.sig end @testset "marking one method leaves its siblings alone" begin @@ -126,23 +130,35 @@ end exact = which(Declared.energy, Tuple{Declared.Exact,Float64}) rational = which(Declared.energy, Tuple{Declared.Numerical,Rational}) @test numerical !== exact !== rational - @test_broken ExperimentalAPI.isexperimental(numerical) - @test_broken !ExperimentalAPI.isexperimental(exact) - @test_broken !ExperimentalAPI.isexperimental(rational) + @test ExperimentalAPI.isexperimental(numerical) + @test !ExperimentalAPI.isexperimental(exact) + @test !ExperimentalAPI.isexperimental(rational) end -@testset "a method mark survives precompilation" begin - # Scope: the name-keyed registry already survives (`test/test_precompile.jl`); whether - # `Method` objects do is a separate claim needing its own fixture package. - @test_broken ExperimentalAPI.experimental_methods isa Function +@testset "a method mark is stored where a name mark is, and comes back with one" begin + # Scope: the name-keyed registry already survives precompilation (`test/test_precompile.jl`), + # and a method mark is a record in the same vector — a signature is serialisable where a + # `Method` object would not have been. + @test ExperimentalAPI.experimental_methods isa Function + ms = ExperimentalAPI.experimental_methods(Declared) + @test :energy in [mk.name for mk in ms] + @test all(mk -> mk.mod === Declared, ms) + @test ExperimentalAPI.mark(Declared, :energy) !== nothing end @testset "a method mark is queryable from a call site" begin - @test_broken ExperimentalAPI.isexperimental( + @test ExperimentalAPI.isexperimental( which(Declared.energy, Tuple{Declared.Numerical,Float64}) ) end +@testset "…but the marked NAME is still a promise, because a sibling is unmarked" begin + # The direction a method-level mark must not leak: one unvalidated dispatch path is not a + # licence to remove `energy`. `stable` keeps a name until every method behind it is marked. + @test :energy in ExperimentalAPI.stable(Declared) + @test :short_fn ∉ ExperimentalAPI.stable(Declared) # one method, and it is marked +end + # ── extensions ─────────────────────────────────────────────────────────────────────────────── # # An extension is a separate module: its public names are part of the surface a user sees, and @@ -151,12 +167,14 @@ end @testset "a mark inside a package extension is reachable from the parent" begin ext = Base.get_extension(ExperimentalAPI, :ExperimentalAPITestExt) @test ext !== nothing - # Not `!isempty(...)`: this package marks six of its own names, so an ignored keyword would - # satisfy that. The claim is that a mark whose home is the extension comes back. - @test isempty(ExperimentalAPI.experimental(ext)) # today the extension declares none - @test_broken any( + # Not `!isempty(...)`: this package marks several of its own names, so an ignored keyword + # would satisfy that. The claim is that a mark whose home is the EXTENSION comes back. + @test !isempty(ExperimentalAPI.experimental(ext)) + @test any( mk -> mk.mod === ext, ExperimentalAPI.experimental(ExperimentalAPI; extensions=true) ) + # Control: without the keyword it does not, so the keyword is doing the work. + @test !any(mk -> mk.mod === ext, ExperimentalAPI.experimental(ExperimentalAPI)) end @testset "every new Audit field keeps the partition invariant" begin @@ -166,11 +184,17 @@ end @test sort( vcat(a.foreign, a.documented, a.unaccounted, setdiff(a.declared, a.documented)) ) == a.surface - @test_broken ExperimentalAPI.partition_holds(a) === true + @test ExperimentalAPI.partition_holds(a) === true end @testset "auditing a package does not silently ignore its extensions" begin a = ExperimentalAPI.audit(ExperimentalAPI) - # Scope: `audit` looks at one module today; whether extensions are included must be stated. - @test_broken hasproperty(a, :extensions) + # Stated rather than silent: an extension is a separate module, so it is REPORTED and audited + # in its own right rather than folded in — an extension that is not loaded is not missing, it + # is inapplicable. + @test hasproperty(a, :extensions) + @test Base.get_extension(ExperimentalAPI, :ExperimentalAPITestExt) in a.extensions + @test ExperimentalAPI.audit( + only(filter(x -> nameof(x) === :ExperimentalAPITestExt, a.extensions)) + ) isa ExperimentalAPI.Audit end diff --git a/test/spec/test_spec_dispatch.jl b/test/spec/test_spec_dispatch.jl index fe7f1b8..e670800 100644 --- a/test/spec/test_spec_dispatch.jl +++ b/test/spec/test_spec_dispatch.jl @@ -24,6 +24,7 @@ public Exact, via_abstract, via_invoke, more_specific, + imperative, pair, via_pair_clean, via_pair_marked @@ -67,6 +68,12 @@ via_pair_marked(a::Numerical, b::Numerical) = pair(a, b) more_specific(::Int) = 0 @experimental "the fallback is a placeholder" more_specific(::Integer) = 1 +# Definitions the macro cannot attach to: `@eval` in a loop is the shape a table-driven package +# has, and it is why `mark_method!` exists at all. +for T in (:Exact, :Numerical) + @eval imperative(::$T) = 1.0 +end + end # module Dispatch # ── the fixture really has the shape the file claims ───────────────────────────────────────── @@ -82,7 +89,7 @@ end # module Dispatch end @testset "the specificity premise the file rests on is true" begin - # The premise every `@test_broken` below rests on, and which nothing else would notice. + # The premise every verdict below rests on, and which nothing else would notice. @test which(Dispatch.more_specific, Tuple{Int}).sig === Tuple{typeof(Dispatch.more_specific),Int} @test Dispatch.more_specific(5) == 0 # the UNMARKED, more specific method @@ -111,26 +118,45 @@ end end end -@testset "attaching the mark at one method's definition marks the NAME today" begin - # The attached form reads as if it scoped the claim to one method; `_signame` throws the - # argument types away. So method-level marking needs a separate imperative route. +@testset "the attached mark names the name AND the signature it attached to" begin + # Both statements are true of one record, and they answer different questions. `audit` asks + # about the name — has the author said anything about `energy`? `reach` asks about the + # method — is THIS dispatch path the unvalidated one? @test isexperimental(Dispatch, :energy) - @test mark(Dispatch, :energy).name === :energy # not a signature - @test_broken ExperimentalAPI.mark_method! isa Function + @test mark(Dispatch, :energy).name === :energy + @test mark(Dispatch, :energy).sig === Tuple{typeof(Dispatch.energy),Dispatch.Numerical} + @test ExperimentalAPI.isexperimental(which(Dispatch.energy, Tuple{Dispatch.Numerical})) + # Control: the sibling method is not marked, so the name-level answer is the wider one. + @test !ExperimentalAPI.isexperimental(which(Dispatch.energy, Tuple{Dispatch.Exact})) +end + +@testset "a method the macro could not reach can be marked imperatively" begin + # The route for a definition produced by another package's macro, or generated in a loop. + # The mark lands in the module that WROTE the method, exactly where the macro would put it. + @test ExperimentalAPI.mark_method! isa Function + m = which(Dispatch.imperative, Tuple{Dispatch.Numerical}) + @test !ExperimentalAPI.isexperimental(m) # …before + mk = ExperimentalAPI.mark_method!(m, "generated in a loop; shape not settled") + @test mk isa ExperimentalAPI.Mark + @test mk.mod === Dispatch + @test mk.sig === m.sig + @test ExperimentalAPI.isexperimental(m) # …and after + # Control: the sibling produced by the same loop is untouched. + @test !ExperimentalAPI.isexperimental(which(Dispatch.imperative, Tuple{Dispatch.Exact})) end # ── what the analysis has to say about each shape ──────────────────────────────────────────── @testset "a call site that can only reach settled methods is clean" begin # Control: rejects a tool answering `:depends` for every multi-candidate call site. - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Dispatch.only_settled, Tuple{Dispatch.Exact}) ) === :clean end @testset "a Union-typed call site that could reach a mark is not clean" begin # Half the run-time values take the marked branch, so `:clean` is false, not conservative. - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach( Dispatch.either_way, Tuple{Union{Dispatch.Exact,Dispatch.Numerical}} ), @@ -138,18 +164,18 @@ end end @testset "an abstract-typed call site that could reach a mark is not clean" begin - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Dispatch.via_abstract, Tuple{Dispatch.Kind}) ) !== :clean end @testset "the unresolvable call site is named, not just counted" begin # Structured fields, not `occursin`: a short needle matches any boilerplate diagnostic. - @test_broken all( + @test all( u -> hasproperty(u, :file) && hasproperty(u, :line) && hasproperty(u, :callee), ExperimentalAPI.reach(Dispatch.via_abstract, Tuple{Dispatch.Kind}).unresolved, ) - @test_broken any( + @test any( u -> u.callee === :k, ExperimentalAPI.reach(Dispatch.via_abstract, Tuple{Dispatch.Kind}).unresolved, ) @@ -158,7 +184,7 @@ end @testset "which() throwing must not be swallowed into :clean" begin # The mistake this file exists to prevent: catching `which`, skipping the site, reporting # the rest as clean. - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Dispatch.via_abstract, Tuple{Dispatch.Kind}) ) === :unknown end @@ -166,27 +192,35 @@ end # ── dispatch subtleties ────────────────────────────────────────────────────────────────────── @testset "invoke pins the method it names" begin - # Argument types alone cannot see `invoke` pinning a method dispatch would not pick. - @test_broken ExperimentalAPI.verdict( - ExperimentalAPI.reach(Dispatch.via_invoke, Tuple{Dispatch.Numerical}) + # Argument types alone cannot see `invoke` pinning a method dispatch would not pick: an `Int` + # goes to the unmarked `::Int` method, and only `invoke` reaches the marked `::Integer` one. + # The entry signature is `Tuple{Int}` because that is the only method `via_invoke` has — + # the spec was written with `Tuple{Numerical}`, which no method of it matches. + @test ExperimentalAPI.verdict( + ExperimentalAPI.reach(Dispatch.via_invoke, Tuple{Int}) ) === :depends + # Control: the same argument reaching the same generic WITHOUT `invoke` is clean, so the + # verdict above is `invoke` being followed and not the argument type being widened. + @test ExperimentalAPI.verdict( + ExperimentalAPI.reach(Dispatch.more_specific, Tuple{Int}) + ) === :clean end @testset "a more specific unmarked method shadows a marked one" begin # An `Int` never reaches the mark. `:depends` here is the name-level over-claim one level # down. - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Dispatch.more_specific, Tuple{Int}) ) === :clean # …and the call that does fall through is reported. - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Dispatch.more_specific, Tuple{UInt8}) ) === :depends end @testset "a mark on one method does not leak to its siblings at a call site" begin # Both call the same name and must get different verdicts. - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Dispatch.only_settled, Tuple{Dispatch.Exact}) ) !== ExperimentalAPI.verdict( ExperimentalAPI.reach( @@ -197,10 +231,10 @@ end @testset "a marked combination is not reachable from either argument alone" begin # Widening each argument independently would call both call sites `:depends`. - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Dispatch.via_pair_clean, Tuple{Dispatch.Exact,Dispatch.Exact}) ) === :clean - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach( Dispatch.via_pair_marked, Tuple{Dispatch.Numerical,Dispatch.Numerical} ), @@ -208,7 +242,7 @@ end end @testset "the report says WHICH method was reached, not which name" begin - @test_broken first( + @test first( ExperimentalAPI.reach(Dispatch.either_way, Tuple{Dispatch.Numerical}).reached ).method === which(Dispatch.energy, Tuple{Dispatch.Numerical}) end diff --git a/test/spec/test_spec_docstring.jl b/test/spec/test_spec_docstring.jl index aae9f93..db3c02e 100644 --- a/test/spec/test_spec_docstring.jl +++ b/test/spec/test_spec_docstring.jl @@ -100,8 +100,12 @@ end @testset "the reason is reachable from the rendered documentation" begin # Scope: the reason must reach the docs site without the author typing it twice. - @test_broken ExperimentalAPI.docstring_note(Both, :documented_and_marked) isa - AbstractString + note = ExperimentalAPI.docstring_note(Both, :documented_and_marked) + @test note isa AbstractString + @test occursin("convergence not established at low β", note) + # Control: a settled name gets no note, so a renderer built on this cannot annotate + # everything. + @test ExperimentalAPI.docstring_note(Both, :documented_only) === nothing end @testset "what Documenter's checkdocs sees is not what audit sees" begin diff --git a/test/spec/test_spec_foreign.jl b/test/spec/test_spec_foreign.jl index 974dfed..36dc568 100644 --- a/test/spec/test_spec_foreign.jl +++ b/test/spec/test_spec_foreign.jl @@ -25,13 +25,23 @@ struct Heisenberg end struct Energy end struct Susceptibility end -# exact — trustworthy +# exact — trustworthy, and documented at the signature it applies to +""" + fetch_value(::Ising, ::Energy) + +Closed form. Exact for every lattice size. +""" UpstreamGeneric.fetch_value(::Ising, ::Energy) = -2.0 -# numerically delicate — this is the one that should carry a mark -UpstreamGeneric.fetch_value(::Heisenberg, ::Energy) = -1.7724538509055159 +# numerically delicate — this is the one that carries a mark +@experimental( + "extrapolated from a finite-size sweep; no reference value", + since = v"0.1.0", + tracking = "https://example.invalid/issues/7", + UpstreamGeneric.fetch_value(::Heisenberg, ::Energy) = -1.7724538509055159, +) -# a whole family that is provisional +# a whole family that is provisional, and says nothing at all — the finding UpstreamGeneric.fetch_value(::Ising, ::Susceptibility) = 0.0 UpstreamGeneric.fetch_value(::Heisenberg, ::Susceptibility) = 0.0 @@ -44,18 +54,11 @@ end # module Downstream @test :fetch_value ∉ ExperimentalAPI.surface(Downstream) # invisible to names() end -@testset "the macro currently refuses a qualified definition" begin - # Pinned so the change is visible when it happens. - @test_throws LoadError @eval module RefusedForeign - using ExperimentalAPI - using ..UpstreamGeneric - @experimental "why" UpstreamGeneric.fetch_value(::Int, ::Int) = 0 - end -end - -@testset "a method on a foreign generic can be marked" begin - # Ends in a Bool and checks what was marked — see `README.md` on `@eval module`. - @test_broken begin +@testset "a qualified definition is accepted, and names a method rather than a surface" begin + # This used to be a refusal, pinned so the change would be visible when it happened. It has: + # extending another package's generic is the normal Julia idiom, and a mark that cannot + # attach there cannot describe the surface that matters. + @test begin @eval module MarkedForeign using ExperimentalAPI using ..UpstreamGeneric @@ -64,6 +67,11 @@ end end !isempty(ExperimentalAPI.experimental_methods(Main.MarkedForeign)) end + # It carries a signature, which is what makes it a claim about one method… + mk = only(ExperimentalAPI.experimental_methods(Main.MarkedForeign)) + @test mk.sig !== nothing + # …and it is not a promise about `MarkedForeign`'s own surface, so it cannot dangle. + @test isempty(ExperimentalAPI.audit(Main.MarkedForeign; methods=false).dangling) end @testset "marking one foreign method leaves the siblings alone" begin @@ -72,50 +80,68 @@ end UpstreamGeneric.fetch_value, Tuple{Downstream.Heisenberg,Downstream.Energy} ) @test exact !== delicate - # The fixture cannot carry `@experimental` here — a qualified definition is refused today - # and the module would not load — so the test marks it through the future API. Otherwise this - # stays Broken even once method-level marking works. - @test_broken begin - ExperimentalAPI.mark_method!(delicate, "numerically delicate") - ExperimentalAPI.isexperimental(delicate) && !ExperimentalAPI.isexperimental(exact) - end + @test ExperimentalAPI.isexperimental(delicate) + @test !ExperimentalAPI.isexperimental(exact) + # The reason travels with the method, which is the whole point of putting it there. + @test occursin("extrapolated", ExperimentalAPI.mark(delicate).reason) + @test ExperimentalAPI.mark(delicate).tracking == "https://example.invalid/issues/7" end @testset "the mark is stored in the module that WROTE the method" begin # Not in `UpstreamGeneric`: a package cannot carry claims its dependents invented, and the # mark must survive it being reloaded. `all(pred, [])` is `true`, so non-emptiness is part of # the claim. - @test_broken !isempty(ExperimentalAPI.experimental_methods(Downstream)) && all( - mk -> mk.mod === Downstream, ExperimentalAPI.experimental_methods(Downstream) - ) + @test !isempty(ExperimentalAPI.experimental_methods(Downstream)) && + all(mk -> mk.mod === Downstream, ExperimentalAPI.experimental_methods(Downstream)) end @testset "the ownership query and the cross-module search are different verbs" begin # "what this module owns" and "what anyone has marked on this generic" are both wanted, and # one name for both means the reader cannot tell which they got. - @test_broken ExperimentalAPI.marks_on isa Function + @test ExperimentalAPI.marks_on isa Function + # The two really do answer differently: `Downstream` owns one mark on `fetch_value`, and the + # search over the generic finds that one plus whatever `MarkedForeign` contributed. + @test length(ExperimentalAPI.experimental_methods(Downstream)) == 1 + @test length(ExperimentalAPI.marks_on(UpstreamGeneric.fetch_value)) >= 2 + @test all(mk -> mk.mod === Downstream, ExperimentalAPI.experimental_methods(Downstream)) end @testset "asking the generic finds marks contributed by every package" begin - @test_broken !isempty(ExperimentalAPI.experimental(UpstreamGeneric.fetch_value)) + @test !isempty(ExperimentalAPI.experimental(UpstreamGeneric.fetch_value)) end @testset "audit reports foreign methods this module owns" begin # `foreign` means "bound elsewhere, not our problem". A method we wrote is the opposite. a = audit(Downstream) - @test_broken hasproperty(a, :contributed_methods) - @test_broken length(a.contributed_methods) == 4 + @test hasproperty(a, :contributed_methods) + @test length(a.contributed_methods) == 4 + # Control: the name-level half cannot see any of this, which is why the method half exists — + # `fetch_value` is not in `names(Downstream)` and never will be. + @test :fetch_value ∉ a.surface + @test isempty(a.dangling) end @testset "a contributed method with neither docstring nor mark is a finding" begin - @test_broken !isempty(ExperimentalAPI.unaccounted_methods(Downstream)) + unacc = ExperimentalAPI.unaccounted_methods(Downstream) + @test !isempty(unacc) + # Exactly the two that say nothing: the documented one and the marked one are accounted for. + @test length(unacc) == 2 + @test all(mm -> mm.sig.parameters[3] === Downstream.Susceptibility, unacc) end @testset "a docstring on a specific signature counts" begin # Docstrings are keyed by signature, so "documented" is answerable per method. - @test_broken ExperimentalAPI.isdocumented( - which(UpstreamGeneric.fetch_value, Tuple{Downstream.Ising,Downstream.Energy}) - ) isa Bool + documented = which( + UpstreamGeneric.fetch_value, Tuple{Downstream.Ising,Downstream.Energy} + ) + silent = which( + UpstreamGeneric.fetch_value, Tuple{Downstream.Ising,Downstream.Susceptibility} + ) + @test ExperimentalAPI.isdocumented(documented) isa Bool + @test ExperimentalAPI.isdocumented(documented) + # Control: the generic itself is documented upstream, and that must not count for every + # method behind it — otherwise one docstring in `UpstreamGeneric` accounts for all 570. + @test !ExperimentalAPI.isdocumented(silent) end @testset "marking a method does not make the foreign NAME experimental" begin @@ -128,13 +154,17 @@ end # upstream generic. caller(x) = UpstreamGeneric.fetch_value(Downstream.Heisenberg(), Downstream.Energy()) + x - @test_broken ExperimentalAPI.verdict(ExperimentalAPI.reach(caller, Tuple{Float64})) === + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(caller, Tuple{Float64})) === :depends + # Control: the same call through the exact method is clean, so the verdict is the MARK being + # found and not the generic being foreign. + clean(x) = UpstreamGeneric.fetch_value(Downstream.Ising(), Downstream.Energy()) + x + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(clean, Tuple{Float64})) === :clean end @testset "a mark on a method of a function defined in Base is possible" begin # "Base is off limits" would exclude a large part of every package's surface. - @test_broken begin + @test begin @eval module MarkedBase using ExperimentalAPI struct Widget end @@ -143,6 +173,9 @@ end end !isempty(ExperimentalAPI.experimental_methods(Main.MarkedBase)) end + # Control: `Base.show` is not made experimental for anybody else. + @test !ExperimentalAPI.isexperimental(Base, :show) + @test !ExperimentalAPI.isexperimental(which(show, Tuple{IO,Int})) end @testset "it stays refused when the mark cannot say WHICH method" begin diff --git a/test/spec/test_spec_forms.jl b/test/spec/test_spec_forms.jl index 475d1a5..8fc408f 100644 --- a/test/spec/test_spec_forms.jl +++ b/test/spec/test_spec_forms.jl @@ -18,9 +18,12 @@ public kw_fn, where_fn, vararg_fn, ret_typed, Callable, Ctor, INTERP @experimental "varargs" vararg_fn(x, rest...) = x @experimental "return type annotation" ret_typed(x)::Float64 = x -struct Callable +# A marked type covers the constructors it implies: `MarkedStruct(x)` is a call, and an analysis +# that only looked at named functions would report constructing one as clean. +@experimental "the call operator's scaling rule is provisional" struct Callable k::Float64 end +(c::Callable)(x) = c.k * x struct Ctor v::Int end @@ -49,9 +52,10 @@ end # ── forms that are not covered ─────────────────────────────────────────────────────────────── -@testset "a callable struct is marked on the WRONG symbol today" begin - # `(c::C)(x)` has no function name; `_signame` walks the `::` and returns the argument name. - # The mark lands on `:c`, which is not a binding anywhere. +@testset "a callable struct is marked on the type" begin + # `(c::C)(x)` has no function name. Reading the argument name out of the `::` is what the + # first implementation did, and it produced a mark on `:c` — a local that is not a binding + # anywhere. The name a reader recognises is the TYPE. @eval module CallableMarked using ExperimentalAPI struct C @@ -59,14 +63,20 @@ end end @experimental "scaling rule provisional" (c::C)(x) = c.k * x end - @test :c in [mk.name for mk in experimental(CallableMarked)] # today, and wrong - @test !isdefined(CallableMarked, :c) # marks a non-binding + got = [mk.name for mk in experimental(Main.CallableMarked)] + @test :C in got + @test :c ∉ got # the argument name, and it was the bug + @test !isdefined(Main.CallableMarked, :c) + # …and the mark is about the call operator, not about the constructor. + @test ExperimentalAPI.mark(Main.CallableMarked, :C).sig === + Tuple{Main.CallableMarked.C,Any} end -@testset "and the audit compounds it: C is reported as UNDECLARED" begin - # The second wrong signal: the audit reports a declaration for a name that does not exist AND - # a public name with no declaration, so it tells the author to declare what that line already - # declares. Both halves have to move together. +@testset "…and the audit reports neither a dangling mark nor an undeclared name" begin + # The compounding failure the fix has to clear: a mark on `:c` made the audit report a + # declaration for a name that does not exist AND a public name with no declaration, so it + # told the author to go and declare the very thing that line declares. Both halves move + # together, so both are checked together. @eval module CallablePublic using ExperimentalAPI public C @@ -75,22 +85,11 @@ end end @experimental "scaling rule provisional" (c::C)(x) = c.k * x end - a = ExperimentalAPI.audit(Main.CallablePublic) - @test :c in a.dangling # declared, but no such binding - @test :C in a.unaccounted # public, and — per the audit — never declared - @test Main.CallablePublic.C(2.0)(3.0) == 6.0 # the definition itself is fine; the mark is not -end - -@testset "a callable struct marks the type, or refuses" begin - # Either answer is defensible; the argument name is not. - @test_broken :C in [mk.name for mk in experimental(Main.CallableMarked)] -end - -@testset "…and fixing that clears BOTH signals" begin - # `_signame` returning `:C` is necessary but not sufficient — the audit is what the author - # reads. - a = ExperimentalAPI.audit(Main.CallablePublic) - @test_broken isempty(a.dangling) && :C ∉ a.unaccounted + a = ExperimentalAPI.audit(Main.CallablePublic; methods=false) + @test isempty(a.dangling) + @test :C ∉ a.unaccounted + @test :C in a.declared + @test Main.CallablePublic.C(2.0)(3.0) == 6.0 end @testset "a constructor method is marked on the type" begin @@ -108,16 +107,22 @@ end @test :s ∉ got end -@testset "an inner constructor inside a marked struct is not separately marked" begin - # Included or excluded is a decision; silence is not. - @test_broken hasproperty(mark(FormsSpec, :Callable), :includes_constructors) +@testset "a marked type covers its constructors, and says so" begin + # Included or excluded is a decision; silence is not. Included: `MarkedStruct(x)` is a call, + # and an analysis that treated construction as unmarked would report building one as clean. + mk = mark(FormsSpec, :Callable) + @test hasproperty(mk, :includes_constructors) + @test mk.includes_constructors === true + @test ExperimentalAPI.isexperimental(which(FormsSpec.Callable, Tuple{Float64})) + # Control: a mark attached to a function covers no constructor, because there is none. + @test mark(FormsSpec, :kw_fn).includes_constructors === false end @testset "an operator method can be marked" begin # Each ends in a Bool and checks what was marked: `@eval module` returns a Module, which # reports "non-Boolean" instead of the Unexpected Pass this directory relies on, and accepting # the syntax while recording the wrong symbol is the defect above. - @test_broken begin + @test begin @eval module OpMarked using ExperimentalAPI struct V @@ -130,7 +135,7 @@ end end @testset "a generated function can be marked" begin - @test_broken begin + @test begin @eval module GenMarked using ExperimentalAPI @experimental "generator is a prototype" @generated g(x) = :(x) @@ -142,7 +147,7 @@ end @testset "Base.@kwdef stacks with the mark" begin # Two macros that both wrap a definition must compose in at least one order, and which one # must be documented. - @test_broken begin + @test begin @eval module KwdefMarked using ExperimentalAPI @experimental "defaults are guesses" Base.@kwdef struct S @@ -154,7 +159,7 @@ end end @testset "@inline and the mark compose in both orders" begin - @test_broken begin + @test begin @eval module InlineMarked using ExperimentalAPI @experimental "kernel unverified" @inline f(x) = x @@ -166,11 +171,13 @@ end @testset "a definition produced by @eval can be marked by name" begin # Metaprogrammed definitions cannot be attached to, so the name-list form must reach them. - @test_broken begin + @test begin @eval module EvalMarked using ExperimentalAPI + # Built with `Expr` rather than `@eval $n(...)`: inside an `@eval module` the outer + # macro interpolates `$n` first, where `n` does not exist yet. for n in (:a, :b) - @eval $n(x) = x + Core.eval(@__MODULE__, Expr(:(=), Expr(:call, n, :x), :x)) end @experimental "generated in a loop" a b end @@ -219,9 +226,22 @@ end @test !occursin("mark.jl", msg) end -@testset "the refusal names @experimental rather than leaking the emitted const" begin - # The message still names a `const` the author never wrote. Whether this is reachable is - # open: the only expansion avoiding `const` is `global`, which fails silently in local scope. +@testset "the refusal cannot name @experimental, and that is now a decision" begin + # WITHDRAWN, with the measurement that withdrew it. The requirement was that the message name + # `@experimental`. It cannot, and the three routes are exhausted: + # + # * `const` in local scope fails during LOWERING, before any emitted code runs, so no check + # of ours can intercept it — and Julia's message does not name the variable either, so + # naming the binding `var"@experimental ..."` does not smuggle the word in. Measured on + # 1.12.2: the message is byte-identical for `:__EXPERIMENTAL_API_MARKS__` and for a + # binding whose name is the whole sentence. + # * `global`, the one expansion that avoids `const`, fails SILENTLY in local scope — a + # worse outcome than a loud message pointing at the wrong vocabulary. + # * creating the registry through `Core.eval` removes the error altogether, which turns a + # refusal into a mark registered when the enclosing function is first called. + # + # What is kept is the part that is in this package's hands and is asserted above: the blame + # lands on the line the author wrote, and never inside this package. e = try @eval module ClosureMarked2 using ExperimentalAPI @@ -234,14 +254,17 @@ end catch err err end - @test_broken occursin("@experimental", sprint(showerror, e)) + msg = sprint(showerror, e isa LoadError ? e.error : e) + @test occursin("unsupported `const` declaration", msg) + @test !occursin("mark.jl", msg) end # ── metadata ───────────────────────────────────────────────────────────────────────────────── @testset "since must be a version, and the refusal must say so" begin - # Refused by accident: the field's conversion fails, with a message naming neither `since` - # nor `@experimental`. A deliberate check would also throw, so assert the diagnostic. + # Refused by a check rather than by accident. The first implementation let the field's own + # conversion fail, which threw a `MethodError` naming neither `since` nor `@experimental` — + # a refusal the author cannot act on is barely better than none. e = try @eval module BadSince using ExperimentalAPI @@ -251,8 +274,12 @@ end catch err err isa LoadError ? err.error : err end - @test e isa MethodError # today, and accidental - @test_broken occursin("since", sprint(showerror, e)) + @test e isa ArgumentError + msg = sprint(showerror, e) + @test occursin("since", msg) + @test occursin("VersionNumber", msg) + # …and it says what to write instead, which is the whole difference from the MethodError. + @test occursin("v\"0.4.0\"", msg) end @testset "an unknown keyword is refused rather than ignored" begin @@ -264,8 +291,24 @@ end end @testset "tracking is carried through to every report" begin - # Stored today; it has to survive into the audit and the record as well. - @test_broken ExperimentalAPI.audit(FormsSpec).tracking isa AbstractDict + # Stored is not enough: it has to survive into the audit, and into the note the docs render. + @test ExperimentalAPI.audit(FormsSpec; methods=false).tracking isa AbstractDict + @eval module Tracked + using ExperimentalAPI + public f + "Documented." + @experimental( + "shape undecided", tracking = "https://example.invalid/issues/3", f(x) = x + ) + end + a = ExperimentalAPI.audit(Main.Tracked; methods=false) + @test a.tracking[:f] == "https://example.invalid/issues/3" + @test occursin( + "example.invalid/issues/3", ExperimentalAPI.docstring_note(Main.Tracked, :f) + ) + # Control: a mark with no tracking link contributes no entry, so the table is not a list of + # every mark with a blank beside most of them. + @test :kw_fn ∉ keys(ExperimentalAPI.audit(FormsSpec; methods=false).tracking) end # Evaluate an expression in a fresh module and hand back the exception it raised, unwrapped. @@ -325,14 +368,14 @@ end end end -@testset "a non-string reason is refused by accident, not by a check" begin - # Refused by `strip` failing inside `_reason`, with a message naming neither `@experimental` - # nor `reason`. Same shape as the `since` case above. +@testset "a non-string reason is refused by a check, and the check says why" begin + # Same shape as the `since` case: letting `strip` fail inside `_reason` also refused it, with + # a `MethodError` naming neither `@experimental` nor `reason`. for r in (:(:sym), 42) @testset "reason=$(repr(r))" begin e = probe(:(@experimental $r f(x) = x)) - @test e isa MethodError # today, and accidental - @test_broken occursin("reason", sprint(showerror, e)) + @test e isa ArgumentError + @test occursin("reason", sprint(showerror, e)) end end end @@ -343,7 +386,7 @@ end e = probe(:(@experimental f(x) = x)) @test e isa ArgumentError @test occursin("nothing to mark", sprint(showerror, e)) # today, and misleading - @test_broken occursin("reason", sprint(showerror, e)) + @test occursin("reason", sprint(showerror, e)) end @testset "nothing is marked when the macro refuses" begin @@ -388,6 +431,13 @@ end end @testset "the replacement is reported rather than silent" begin - # Last-write-wins is a decision, and it should be visible. - @test_broken ExperimentalAPI.superseded_marks isa Function + # Last-write-wins is a decision, and it should be visible: a reason that was overwritten is + # exactly the thing a reader cannot reconstruct from the source they are looking at. + @test ExperimentalAPI.superseded_marks isa Function + sup = ExperimentalAPI.superseded_marks(A3) + @test length(sup) == 1 + @test sup[1].reason == "first" + # Control: a module whose marks were never replaced records nothing, so the log is not just + # a copy of the registry. + @test isempty(ExperimentalAPI.superseded_marks(FormsSpec)) end diff --git a/test/spec/test_spec_integration.jl b/test/spec/test_spec_integration.jl index 6bca95f..dbd95ba 100644 --- a/test/spec/test_spec_integration.jl +++ b/test/spec/test_spec_integration.jl @@ -5,6 +5,9 @@ # package does not have — so nothing in this group is blocked on infrastructure. using ExperimentalAPI: ExperimentalAPI, @experimental, experimental +using Documenter: Documenter +using Documenter: MarkdownAST +using TOML: TOML using Test module Shown @@ -32,30 +35,79 @@ end # module Shown @testset "the docs can render the mark without the author repeating it" begin # Typed twice, the two drift and the machine-readable one loses. - @test_broken ExperimentalAPI.docstring_note(Shown, :provisional) isa AbstractString + @test ExperimentalAPI.docstring_note(Shown, :provisional) isa AbstractString end @testset "the rendered note carries the reason, the version and the tracking link" begin for needle in ("reference value", "0.2.0", "example.invalid") - @test_broken occursin(needle, ExperimentalAPI.docstring_note(Shown, :provisional)) + @testset "$needle" begin + @test occursin(needle, ExperimentalAPI.docstring_note(Shown, :provisional)) + end end end @testset "a Documenter block can list a module's marks" begin - # `@autodocs`-style. The only assertion here needing a new test dependency. - @test_broken ExperimentalAPI.DocumenterExt isa Module + # `@autodocs`-style: a docs page writes ```` ```@experimental ```` and names a module. Reached + # through `Base.get_extension` rather than as a field of the package — an extension is a + # separate module and is not a binding in its parent. + ext = Base.get_extension(ExperimentalAPI, :ExperimentalAPIDocumenterExt) + @test ext isa Module + @test ext in ExperimentalAPI.package_extensions(ExperimentalAPI) + # The expander is registered against Documenter's pipeline, not merely defined. + @test isdefined(ext, :ExperimentalBlocks) + @test Documenter.Selectors.order(ext.ExperimentalBlocks) isa Real + @test Documenter.Selectors.matcher( + ext.ExperimentalBlocks, + MarkdownAST.Node(MarkdownAST.CodeBlock("@experimental", "Shown")), + nothing, + nothing, + ) + # Control: an ordinary code block is not claimed by it. + @test !Documenter.Selectors.matcher( + ext.ExperimentalBlocks, + MarkdownAST.Node(MarkdownAST.CodeBlock("julia", "1 + 1")), + nothing, + nothing, + ) +end + +@testset "the rendered list is markdown, and says what it found" begin + md = ExperimentalAPI.marks_markdown(Shown) + @test occursin("provisional", md) + @test occursin("reference value", md) + # Control: a module with nothing marked renders a sentence rather than an empty heading, so + # "this page is empty" and "the build found nothing" do not look identical. + @eval module NothingMarked + "Settled." + f(x) = x + public f + end + @test occursin( + "no experimental names", ExperimentalAPI.marks_markdown(Main.NothingMarked) + ) end @testset "a settled name gets no note" begin # Control: rejects a renderer that annotates everything. - @test_broken ExperimentalAPI.docstring_note(Shown, :settled) === nothing + @test ExperimentalAPI.docstring_note(Shown, :settled) === nothing end # ── Aqua ───────────────────────────────────────────────────────────────────────────────────── @testset "the audit composes with Aqua rather than competing" begin - # Aqua has no third answer; a package must be able to run both. - @test_broken ExperimentalAPI.aqua_compatible_names(Shown) isa AbstractVector + # Aqua has no third answer; a package must be able to run both. What this reports is the + # DIFFERENCE — the names Aqua flags and `audit` accounts for — so a project can see exactly + # what it would have to argue about. + @test ExperimentalAPI.aqua_compatible_names(Shown) isa AbstractVector + # `Shown` documents everything it marks, so the two tools agree and the difference is empty. + @test isempty(ExperimentalAPI.aqua_compatible_names(Shown)) + # Control: a marked-but-undocumented name is exactly where they part company. + @eval module Disagreeing + using ExperimentalAPI + public marked_only + @experimental "no prose yet" marked_only(x) = x + end + @test ExperimentalAPI.aqua_compatible_names(Main.Disagreeing) == [:marked_only] end # ── release ────────────────────────────────────────────────────────────────────────────────── @@ -83,70 +135,159 @@ end @testset "a snapshot records marks at method granularity" begin # The schema change method-level marks force — which is why the release layer is itself # declared experimental. - @test_broken haskey(ExperimentalAPI.snapshot(Shown), "experimental_methods") + snap = ExperimentalAPI.snapshot(Shown) + @test haskey(snap, "experimental_methods") + @test haskey(snap, "stable_methods") + # The key is a signature a human can read in a committed file, and it carries the argument + # types — not `Tuple{typeof(provisional), Any}`, which moves when a module is renamed. + @test collect(keys(snap["experimental_methods"])) == ["provisional(::Any)"] + @test "settled(::Any)" in snap["stable_methods"] end @testset "removing a marked METHOD is not breaking" begin - @test_broken !ExperimentalAPI.isbreaking( + @test !ExperimentalAPI.isbreaking( ExperimentalAPI.compare_methods(MARKED_METHOD, PROMOTED_METHOD) ) end @testset "removing a SETTLED method is breaking" begin # Control: same schema, differing only in whether the method was marked. - @test_broken ExperimentalAPI.isbreaking( + @test ExperimentalAPI.isbreaking( ExperimentalAPI.compare_methods(SETTLED_METHOD, GONE_METHOD) ) end @testset "a signature change to a settled method is reported as breaking" begin # The blind spot `compare` admits to in its own docstring. - @test_broken ExperimentalAPI.isbreaking( + @test ExperimentalAPI.isbreaking( ExperimentalAPI.compare_methods(SETTLED_METHOD, RESIGNED_METHOD) ) end -@testset "a keyword-only change is a blind spot here too" begin - # Keyword arguments live in a separate `kwcall` method, so a signature string cannot see a - # changed default any more than a name set can. Stated rather than discovered later. - @test_broken ExperimentalAPI.compare_methods_sees_keywords === true +@testset "keyword NAMES are seen, and keyword DEFAULTS are not" begin + # Stated rather than discovered later, and stated in both directions: the signature key + # carries the keyword names a method declares, so adding or renaming one moves the key — + # while a changed default lives in the body and moves nothing. + @test ExperimentalAPI.compare_methods_sees_keywords === true + @eval module Kw + using ExperimentalAPI + public f + "Documented." + f(x; tol=1e-8) = x * tol + end + key = only( + filter( + k -> startswith(k, "f("), ExperimentalAPI.snapshot(Main.Kw)["stable_methods"] + ), + ) + @test occursin("tol", key) + # …and the blind spot, named: the default is not in the key, so changing it moves nothing. + @test !occursin("1e-8", key) + @test !occursin("1.0e-8", key) end # ── the provenance record next to a result ─────────────────────────────────────────────────── @testset "a result file can carry the experimental dependencies of the run that made it" begin # The end state: a figure's directory says which unvalidated code paths produced it. - @test_broken ExperimentalAPI.stamp(tempname(), () -> Shown.provisional(1)) isa - AbstractString + @test ExperimentalAPI.stamp(tempname(), () -> Shown.provisional(1)) isa AbstractString end @testset "the stamp is readable without loading the package that made it" begin - # Plain TOML or JSON: a year later the package may not resolve. The path goes through - # `stamp` first, since a bare `tempname()` throws for an unrelated reason. - @test_broken occursin( - "reference value", - read(ExperimentalAPI.stamp(tempname(), () -> Shown.provisional(1)), String), - ) + # Plain TOML: a year later the package may not resolve. The path goes through `stamp` first, + # since a bare `tempname()` throws for an unrelated reason. + text = read(ExperimentalAPI.stamp(tempname(), () -> Shown.provisional(1)), String) + @test occursin("reference value", text) + @test occursin("provisional", text) + # Parseable by something that is not this package. + @test TOML.parse(text) isa AbstractDict + # Control: a run that touched nothing marked writes a stamp that says so, rather than none. + clean = read(ExperimentalAPI.stamp(tempname(), () -> Shown.settled(1)), String) + @test !occursin("reference value", clean) + @test isempty(TOML.parse(clean)["experimental"]) end @testset "a stamped result names the package versions involved" begin - @test_broken ExperimentalAPI.stamp_versions isa Function + @test ExperimentalAPI.stamp_versions isa Function + v = ExperimentalAPI.stamp_versions() + @test v isa AbstractDict + @test haskey(v, "julia") + # `energy` being experimental in v0.3 says nothing about v0.9, so the stamp carries versions. + text = read(ExperimentalAPI.stamp(tempname(), () -> Shown.provisional(1)), String) + @test occursin("[versions]", text) end # ── CI gates ───────────────────────────────────────────────────────────────────────────────── +# A gate is only a gate if it can be shown to fire, and `test_surface` reports through a +# `@testset` — so the failing direction has to be run under a test set that records instead of +# propagating. Two lines of `AbstractTestSet` is the whole cost of checking that. +struct Collect <: Test.AbstractTestSet + results::Vector{Any} + Collect(::AbstractString) = new(Any[]) +end +Test.record(ts::Collect, res) = (push!(ts.results, res); res) +# A nested `@testset` that does not name a type inherits the enclosing one, so every set inside +# `test_surface` is a `Collect` too — and one that did not hand itself up would drop everything +# it recorded on the floor. Measured by an empty `results` where three passes and a failure had +# just gone by. +function Test.finish(ts::Collect) + Test.get_testset_depth() > 0 && Test.record(Test.get_testset(), ts) + return ts +end +""" + gate_failed(f) -> Bool + +Run `f` under a test set that records instead of propagating, and say whether anything in it +failed. This is how a gate is shown to fire without the failure it is supposed to produce +reaching the suite that is checking for it. +""" +function gate_failed(f) + ts = @testset Collect "probe" begin + f() + end + return any(r -> r isa Test.Fail || r isa Test.Error, _flatten(ts)) +end + +_flatten(ts::Collect) = reduce(vcat, (_flatten(r) for r in ts.results); init=Any[]) +function _flatten(ts::Test.DefaultTestSet) + return reduce(vcat, (_flatten(r) for r in ts.results); init=Any[]) +end +_flatten(r) = Any[r] + @testset "CI can fail a PR that adds a mark without a tracking link" begin # Whether `tracking` is required is a per-project decision, and must be expressible. - @test_broken ExperimentalAPI.test_surface(Shown; require_tracking=true) isa + @test ExperimentalAPI.test_surface(Shown; require_tracking=true) isa ExperimentalAPI.Audit + # `Shown`'s one mark has a link, so the gate passes… + @test !gate_failed(() -> ExperimentalAPI.test_surface(Shown; require_tracking=true)) + # …and it fires on a mark that has none, which is what makes it a gate. + @eval module Untracked + using ExperimentalAPI + public f + "Documented." + @experimental "shape undecided" f(x) = x + end + @test gate_failed( + () -> ExperimentalAPI.test_surface(Main.Untracked; require_tracking=true) + ) + # Control: without the keyword the same module passes, so the failure is the gate and not + # something else about the fixture. + @test !gate_failed(() -> ExperimentalAPI.test_surface(Main.Untracked)) end @testset "CI can fail a PR that increases the number of marks" begin # The ratchet, in the shape `skip` already has. - @test_broken ExperimentalAPI.test_surface(Shown; max_marks=0) isa ExperimentalAPI.Audit + @test ExperimentalAPI.test_surface(Shown; max_marks=1) isa ExperimentalAPI.Audit + @test !gate_failed(() -> ExperimentalAPI.test_surface(Shown; max_marks=1)) + @test gate_failed(() -> ExperimentalAPI.test_surface(Shown; max_marks=0)) end @testset "a mark older than N releases is reported" begin - # `since` exists so a mark cannot quietly become permanent. Nothing reads it yet. - @test_broken ExperimentalAPI.stale_since(Shown, v"0.9.0") isa AbstractVector + # `since` exists so a mark cannot quietly become permanent. + @test ExperimentalAPI.stale_since(Shown, v"0.9.0") isa AbstractVector + @test :provisional in [mk.name for mk in ExperimentalAPI.stale_since(Shown, v"0.9.0")] + # Control: seen from the release it was made in, nothing is stale. + @test isempty(ExperimentalAPI.stale_since(Shown, v"0.2.0")) + @test ExperimentalAPI.age(Shown, :provisional, v"0.9.0") == 7 end diff --git a/test/spec/test_spec_lifecycle.jl b/test/spec/test_spec_lifecycle.jl index cee3b9d..51890e3 100644 --- a/test/spec/test_spec_lifecycle.jl +++ b/test/spec/test_spec_lifecycle.jl @@ -13,15 +13,21 @@ using ExperimentalAPI public verified_now, still_unverified, tracked_but_unresolved, settled, consumer, entry -# Reason discharged: reference value exists, suite exercises it. This one is removable. +# The exit condition is written down where the reason is, and it is a predicate rather than a +# sentence: "what would discharge this" is knowledge the author has and nobody else can recover. +const REFERENCE = Ref(false) + +# Reason discharged: the reference value now exists, so this one is removable. @experimental( "no reference value yet", since = v"0.1.0", tracking = "https://example.invalid/issues/1", + until = () -> REFERENCE[], verified_now(β::Float64) = 2 * β ) -# A mark whose reason still stands. +# A mark whose reason still stands, and which never said what would settle it. `ready_to_promote` +# can only ever answer `false` for this one, which is itself a finding. @experimental( "convergence not established below β ≈ 0.1", since = v"0.1.0", @@ -29,11 +35,13 @@ public verified_now, still_unverified, tracked_but_unresolved, settled, consumer ) # Without a third mark, the two above differ only in whether `tracking` is set, and a rule keyed -# on that alone would satisfy every assertion below. This one has a link and is still not ready. +# on that alone would satisfy every assertion below. This one has a link, has a stated exit, and +# the exit is not met. @experimental( "reference value exists but disagrees with the literature at the third digit", since = v"0.1.0", tracking = "https://example.invalid/issues/2", + until = () -> false, tracked_but_unresolved(β::Float64) = β + 1e-9 ) @@ -63,16 +71,14 @@ end @testset "a whole module can be the entry point" begin # Function-by-function does not scale to a package. - @test_broken ExperimentalAPI.verdict(ExperimentalAPI.reach(Lifecycle)) === :depends + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(Lifecycle)) === :depends end @testset "the module-level answer names which public entry points are affected" begin # "Something in here is experimental" is not actionable at package scale. - @test_broken :entry in - [e.name for e in ExperimentalAPI.reach(Lifecycle).affected_entries] + @test :entry in [e.name for e in ExperimentalAPI.reach(Lifecycle).affected_entries] # Control: `settled` reaches nothing marked and must not be listed. - @test_broken :settled ∉ - [e.name for e in ExperimentalAPI.reach(Lifecycle).affected_entries] + @test :settled ∉ [e.name for e in ExperimentalAPI.reach(Lifecycle).affected_entries] end @testset "a module with nothing marked comes back clean" begin @@ -82,7 +88,7 @@ end f(x) = x public f end - @test_broken ExperimentalAPI.verdict(ExperimentalAPI.reach(Main.CleanModule)) === :clean + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(Main.CleanModule)) === :clean end @testset "a script can be the entry point" begin @@ -91,20 +97,32 @@ end path = tempname() write(path, "1 + 1\n") @test isfile(path) - @test_broken hasproperty(ExperimentalAPI.reach_script(path), :reached) + @test hasproperty(ExperimentalAPI.reach_script(path), :reached) end # ── the exit: what licenses removing a mark ────────────────────────────────────────────────── @testset "the tool says a mark is ready to be removed, and why" begin - # Not "is this marked" but "may this stop being marked", with nameable evidence. - @test_broken ExperimentalAPI.ready_to_promote(Lifecycle, :verified_now) === true + # Not "is this marked" but "may this stop being marked", answered by the thing that knows: + # the condition the author wrote next to the reason. + Lifecycle.REFERENCE[] = false + @test ExperimentalAPI.ready_to_promote(Lifecycle, :verified_now) === false + Lifecycle.REFERENCE[] = true + @test ExperimentalAPI.ready_to_promote(Lifecycle, :verified_now) === true + # …and the verdict follows the world, not the source: nothing about the declaration changed + # between those two lines. + @test :verified_now in [mk.name for mk in ExperimentalAPI.promotable(Lifecycle)] end @testset "a mark whose reason still stands is NOT reported ready" begin # Control: rejects a checker that says "ready" for everything. All three are exercised by # this suite, so coverage cannot be the whole criterion. - @test_broken ExperimentalAPI.ready_to_promote(Lifecycle, :still_unverified) === false + @test ExperimentalAPI.ready_to_promote(Lifecycle, :still_unverified) === false + # …and it is reported as the different finding it is: a mark that never said what would + # settle it can be added and never mechanically retired. + @test :still_unverified in + [mk.name for mk in ExperimentalAPI.marks_without_exit(Lifecycle)] + @test :verified_now ∉ [mk.name for mk in ExperimentalAPI.marks_without_exit(Lifecycle)] end @testset "having a tracking link is not the same as being ready" begin @@ -112,8 +130,9 @@ end @test mark(Lifecycle, :tracked_but_unresolved).tracking !== nothing @test mark(Lifecycle, :verified_now).tracking !== nothing @test mark(Lifecycle, :still_unverified).tracking === nothing - @test_broken ExperimentalAPI.ready_to_promote(Lifecycle, :tracked_but_unresolved) === - false + @test ExperimentalAPI.ready_to_promote(Lifecycle, :tracked_but_unresolved) === false + # …and neither is having an exit condition: this one has one, and it is not met. + @test mark(Lifecycle, :tracked_but_unresolved).until !== nothing end @testset "removing a mark is reported as not breaking" begin @@ -143,39 +162,68 @@ end @testset "removing the mark flips its callers, and only its callers" begin # `consumer` becomes clean; `entry` does not, since it still reaches `still_unverified`. - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Lifecycle.consumer, Tuple{Float64}; ignore=[:verified_now]) ) === :clean - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Lifecycle.entry, Tuple{Float64}; ignore=[:verified_now]) ) === :depends end @testset "how long a mark has been standing is answerable" begin - # `since` is recorded and nothing reads it; a mark that never expires is just a label. + # `since` was recorded and nothing read it; a mark that never expires is just a label. @test mark(Lifecycle, :still_unverified).since == v"0.1.0" # The number the caller needs, not `isa Any` — which is true of `nothing` from a stub. - @test_broken ExperimentalAPI.age(Lifecycle, :still_unverified, v"0.9.0") == 8 + @test ExperimentalAPI.age(Lifecycle, :still_unverified, v"0.9.0") == 8 + # Counted on the axis a bump is breaking along: under 0.x that is the minor component. + @test ExperimentalAPI.age(Lifecycle, :still_unverified, v"0.1.9") == 0 + @test ExperimentalAPI.age(Lifecycle, :still_unverified, v"3.0.0") == 3 + # A mark with no `since` has no age, and `missing` is the honest answer rather than zero. + @eval module NoSince + using ExperimentalAPI + public f + @experimental "undated" f(x) = x + end + @test ExperimentalAPI.age(Main.NoSince, :f, v"9.9.9") === missing + # The list form a CI job asks for, and a control that it is not everything. + @test length(ExperimentalAPI.stale_since(Lifecycle, v"0.9.0")) == 3 + @test isempty(ExperimentalAPI.stale_since(Lifecycle, v"0.1.0")) end @testset "the number of marks can only go down under a ratchet" begin # Same shape as `test_surface`'s skip list. Not `isa Audit`: that comes back whether the cap # was honoured or ignored, so assert what the cap does. @test length(experimental(Lifecycle)) == 3 - @test_broken ExperimentalAPI.exceeds_mark_cap(Lifecycle, 1) === true - @test_broken ExperimentalAPI.exceeds_mark_cap(Lifecycle, 5) === false + @test ExperimentalAPI.exceeds_mark_cap(Lifecycle, 1) === true + @test ExperimentalAPI.exceeds_mark_cap(Lifecycle, 5) === false + @test ExperimentalAPI.exceeds_mark_cap(Lifecycle, 3) === false # the cap is inclusive end @testset "a mark removed while callers still depend on it is caught" begin # Propagation read backwards: the line gets deleted because the author looked at the # definition, not at who reaches it. - @test_broken :consumer in ExperimentalAPI.dependents(Lifecycle, :verified_now) + @test :consumer in ExperimentalAPI.dependents(Lifecycle, :verified_now) # Control: rejects a `dependents` that returns every public name. - @test_broken :settled ∉ ExperimentalAPI.dependents(Lifecycle, :verified_now) + @test :settled ∉ ExperimentalAPI.dependents(Lifecycle, :verified_now) end @testset "the exit works at method granularity too" begin # The intersection neither this file nor `test_spec_dispatch.jl` covers: promoting one # method must not promote its siblings. - @test_broken ExperimentalAPI.ready_to_promote isa Function + @test ExperimentalAPI.ready_to_promote isa Function + @eval module MethodExit + using ExperimentalAPI + public g + struct A end + struct B end + g(::A) = 1 + @experimental("the B branch is a placeholder", until = () -> true, g(::B) = 2) + end + a = which(Main.MethodExit.g, Tuple{Main.MethodExit.A}) + b = which(Main.MethodExit.g, Tuple{Main.MethodExit.B}) + @test ExperimentalAPI.ready_to_promote(ExperimentalAPI.mark(b)) === true + # Control: the sibling carries no mark at all, so there is nothing to promote… + @test ExperimentalAPI.mark(a) === nothing + # …and promoting one method's mark is not a statement about the name. + @test ExperimentalAPI.isexperimental(Main.MethodExit, :g) end diff --git a/test/spec/test_spec_profile.jl b/test/spec/test_spec_profile.jl index 6fb6858..fc8cb95 100644 --- a/test/spec/test_spec_profile.jl +++ b/test/spec/test_spec_profile.jl @@ -139,64 +139,88 @@ end # ── the opt-in layer: the basic question ───────────────────────────────────────────────────── @testset "a run reports which marked definitions it entered" begin - @test_broken :energy in - [h.name for h in ExperimentalAPI.record(() -> Sim.driver(M, 100))] + @test :energy in [h.name for h in ExperimentalAPI.record(() -> Sim.driver(M, 100))] end @testset "a run reports how many times each was entered" begin - @test_broken ExperimentalAPI.record(() -> Sim.driver(M, 100))[1].count == 100 + @test ExperimentalAPI.record(() -> Sim.driver(M, 100))[1].count == 100 + # …and the count is of THIS block, not of the process: the same call again reports 100 and + # not 200, which is what makes a record a measurement of one run. + @test ExperimentalAPI.record(() -> Sim.driver(M, 100))[1].count == 100 end @testset "a marked definition the run never entered is absent, not zero" begin # Control: separates observed from enumerated. - @test_broken :cold ∉ [h.name for h in ExperimentalAPI.record(() -> Sim.driver(M, 10))] + @test :cold ∉ [h.name for h in ExperimentalAPI.record(() -> Sim.driver(M, 10))] end @testset "a run that touches nothing marked reports an empty record, not an error" begin - @test_broken ExperimentalAPI.record(() -> sum(1:10)) == [] + @test ExperimentalAPI.record(() -> sum(1:10)) == [] end @testset "a record distinguishes 'touched nothing' from 'recording was off'" begin # Both are an empty vector otherwise, and they mean opposite things. - @test_broken ExperimentalAPI.record(() -> sum(1:10)).enabled === true + @test ExperimentalAPI.record(() -> sum(1:10)).enabled === true end # ── granularity ────────────────────────────────────────────────────────────────────────────── @testset "attribution is to a method, not to a name" begin - @test_broken first(ExperimentalAPI.record(() -> Sim.driver(M, 10))).method isa Method + h = first(ExperimentalAPI.record(() -> Sim.driver(M, 10))) + @test h.method isa Method + @test h.method === which(Sim.energy, Tuple{Sim.Model}) end @testset "two marked definitions in one run are reported separately" begin - @test_broken length( - ExperimentalAPI.record(() -> (Sim.driver(M, 10); Sim.sweep(M, 10))) - ) == 2 + r = ExperimentalAPI.record(() -> (Sim.driver(M, 10); Sim.sweep(M, 10))) + @test length(r) == 2 + @test Set(h.name for h in r) == Set([:energy, :correlator]) end @testset "the call site that reached the mark is recorded" begin # Scope: which part of the caller's own code to distrust, not just that a mark was hit. - @test_broken !isempty(first(ExperimentalAPI.record(() -> Sim.driver(M, 10))).callers) + callers = first(ExperimentalAPI.record(() -> Sim.driver(M, 10))).callers + @test !isempty(callers) + # The immediate caller, not the marked definition itself — a `callers` list whose only entry + # is `energy` is a list of the wrong thing, and it reads exactly the same. + @test :inner in callers + @test :energy ∉ callers end @testset "the whole path from the entry point is available" begin - @test_broken :driver in first(ExperimentalAPI.record(() -> Sim.driver(M, 10))).paths[1] + paths = first(ExperimentalAPI.record(() -> Sim.driver(M, 10))).paths + @test :driver in paths[1] + # Innermost first, starting at the marked definition, so a reader can follow it outwards. + @test paths[1][1] === :energy end # ── proportion, not just presence ──────────────────────────────────────────────────────────── @testset "the record says what fraction of the run was inside experimental code" begin - @test_broken 0.0 < - ExperimentalAPI.experimental_fraction( - ExperimentalAPI.record(() -> Sim.driver(M, 100_000)) - ) <= - 1.0 + # Long enough to be sampled: the timing backend is Julia's sampling profiler, and a run that + # finishes inside one sampling interval has no fraction to report. Two million iterations of + # a recorded body is tens of milliseconds — hundreds of samples, not a handful. + r = ExperimentalAPI.record(() -> Sim.driver(M, 2_000_000)) + @test r.sampled # …the backend really was loaded + f = ExperimentalAPI.experimental_fraction(r) + @test 0.0 < f <= 1.0 end @testset "inclusive and exclusive time are distinguished" begin # Scope: a marked wrapper over settled code is not a marked kernel. - @test_broken let h = first(ExperimentalAPI.record(() -> Sim.driver(M, 1000))) - h.inclusive >= h.exclusive - end + h = first(ExperimentalAPI.record(() -> Sim.driver(M, 1000))) + @test h.inclusive >= h.exclusive + @test h.inclusive isa Float64 +end + +@testset "without a timing backend the fraction is missing, not zero" begin + # Control for the two above: `Profile` is loaded in this file, so the only way to see the + # other branch is to ask a record that was not sampled. Zero would say the run spent no time + # in marked code, which is the opposite of "nobody measured". + r = ExperimentalAPI.record(() -> Sim.driver(M, 100); timing=false) + @test r.sampled === false + @test ExperimentalAPI.experimental_fraction(r) === missing + @test first(r).inclusive === missing end # ── the floor: what may be emitted into the body ───────────────────────────────────────────── @@ -282,33 +306,50 @@ end @testset "recording survives inlining" begin # Scope: marked definitions are usually small, so a mechanism needing `@noinline` is no - # mechanism. This is why the sampling route was rejected. - @test_broken ExperimentalAPI.record(() -> Sim.driver(M, 100))[1].count == 100 + # mechanism. `Sim.energy` is one multiplication and is inlined into `inner`, which is inlined + # into `driver`; the count still has to be exact. This is why the sampling route was rejected. + @test ExperimentalAPI.record(() -> Sim.driver(M, 100))[1].count == 100 end @testset "detection is on by default; counting is not" begin @test ExperimentalAPI.detecting() === true - @test_broken ExperimentalAPI.recording() === false + @test ExperimentalAPI.recording() === false + # …and it is on exactly inside the block, which is the whole cost argument. + @test ExperimentalAPI.record(() -> ExperimentalAPI.recording()) isa AbstractVector + inside = Ref(false) + ExperimentalAPI.record(() -> (inside[] = ExperimentalAPI.recording())) + @test inside[] === true + @test ExperimentalAPI.recording() === false end @testset "the default layer's cost is stated, and it is the flag's cost" begin # Not `>= 0`, which every number satisfies. Above a few percent it has become a counter. - @test_broken ExperimentalAPI.overhead_when_detecting() < 0.10 + @test ExperimentalAPI.overhead_when_detecting() < 0.10 + # Stated rather than re-measured: a wall-clock figure taken on a shared runner is a flake + # generator, and a number that moves with the machine is not one a caller can plan against. + @test ExperimentalAPI.overhead_when_detecting() === + ExperimentalAPI.overhead_when_detecting() end @testset "the opt-in layer's overhead is measured and reported, not discovered" begin - @test_broken ExperimentalAPI.record(() -> Sim.driver(M, 1000)).overhead isa Real + r = ExperimentalAPI.record(() -> Sim.driver(M, 1000)) + @test r.overhead isa Real + @test 0.0 <= r.overhead <= 1.0 end @testset "recording nests without double counting" begin - @test_broken ExperimentalAPI.record( - () -> ExperimentalAPI.record(() -> Sim.driver(M, 10)) - )[1].count == 10 + @test ExperimentalAPI.record(() -> ExperimentalAPI.record(() -> Sim.driver(M, 10)))[1].count == + 10 end @testset "an exception inside the recorded block still yields a record" begin - @test_broken ExperimentalAPI.record(() -> error("boom"); rethrow=false) isa - AbstractVector + r = ExperimentalAPI.record(() -> (Sim.driver(M, 7); error("boom")); rethrow=false) + @test r isa AbstractVector + # The part that ran is in it: a record that swallowed the exception AND the counts would be + # indistinguishable from a block that did nothing. + @test r[1].count == 7 + # …and by default the exception is not swallowed. + @test_throws ErrorException ExperimentalAPI.record(() -> error("boom")) end # ── concurrency and distribution ───────────────────────────────────────────────────────────── @@ -324,76 +365,149 @@ end threaded() = Threads.@threads for _ in 1:8 Sim.driver(M, 100) end - @test_broken ExperimentalAPI.record(threaded)[1].count == 800 + @test ExperimentalAPI.record(threaded)[1].count == 800 end @testset "per-thread storage is sized by maxthreadid, not nthreads" begin # The interactive pool is counted separately, so `threadid()` exceeds `nthreads()` — an # `nthreads()`-sized vector throws on the first hit from a REPL task. @test Threads.maxthreadid() >= Threads.nthreads() - @test_broken ExperimentalAPI.record(() -> Sim.driver(M, 10)).slots >= - Threads.maxthreadid() + @test ExperimentalAPI.record(() -> Sim.driver(M, 10)).slots >= Threads.maxthreadid() end @testset "a merged record is still a record" begin # `isa AbstractVector` would let the merge return a plain `Vector` with no `.enabled`. - @test_broken hasproperty( + @test hasproperty( ExperimentalAPI.merge_records([ExperimentalAPI.record(() -> Sim.driver(M, 10))]), :enabled, ) end @testset "records from separate processes merge into one" begin - @test_broken ExperimentalAPI.merge_records([ + m = ExperimentalAPI.merge_records([ ExperimentalAPI.record(() -> Sim.driver(M, 10)) for _ in 1:2 - ]) isa AbstractVector + ]) + @test m isa AbstractVector + # Counts ADD. A merge that took the maximum, or the last one, would also be a record. + @test m[1].count == 20 end @testset "merging is associative and order-independent" begin # Workers finish in arbitrary order; provenance must not depend on that. - @test_broken let a = ExperimentalAPI.record(() -> Sim.driver(M, 10)), - b = ExperimentalAPI.record(() -> Sim.sweep(M, 10)) + a = ExperimentalAPI.record(() -> Sim.driver(M, 10)) + b = ExperimentalAPI.record(() -> Sim.sweep(M, 10)) + @test ExperimentalAPI.merge_records([a, b]) == ExperimentalAPI.merge_records([b, a]) + # …and the fixture can disagree: the two records are not equal to each other. + @test a != b +end + +# ── coexistence with the profiler people already use ───────────────────────────────────────── + +# `Sim.energy` is one multiplication, and after inlining there is no frame for a sampler to +# attribute anything to — see `attribute`'s docstring, and note that this is exactly why +# `record`'s counts come from a counter and not from samples. The fixture for the sampling +# question therefore has to be a marked definition that is worth a sample. +module Hot + +using ExperimentalAPI + +public grind, settled_grind + +@experimental "the summation order is provisional" function grind(n::Int) + s = 0.0 + for i in 1:n + s += sqrt(abs(sin(i * 1.0))) + end + return s +end - ExperimentalAPI.merge_records([a, b]) == ExperimentalAPI.merge_records([b, a]) +"Settled, and just as hot." +function settled_grind(n::Int) + s = 0.0 + for i in 1:n + s += sqrt(abs(cos(i * 1.0))) end + return s end -# ── coexistence with the profiler people already use ───────────────────────────────────────── +end # module Hot @testset "recording does not disturb Profile" begin - @test_broken ExperimentalAPI.record(() -> Sim.driver(M, 10); with_profile=true) isa - AbstractVector + # `with_profile = true` means the caller is already using the buffer: whatever is in it stays. + # + # Two things this measures rather than assumes. The buffer has to be filled by a run that is + # long compared with the sampling interval — `Sim.driver(M, 200_000)` is about one interval at + # the default rate, and came back with **zero** samples on macOS, which made the whole + # assertion a coin flip. And the size is read with `Profile.len_data`, not by fetching: + # `fetch(; include_meta = false)` strips metadata behind an `@assert` that fires on + # 1.14.0-DEV.3115 for a buffer this test did not fill. + Hot.grind(10) + Profile.clear() + Profile.init(; delay=1e-5) + Profile.@profile Hot.grind(2_000_000) + before = Profile.len_data() + @test before > 0 + r = ExperimentalAPI.record(() -> Sim.driver(M, 10); with_profile=true) + @test r isa AbstractVector + @test Profile.len_data() >= before + # Control: without the keyword the buffer is cleared, so the keyword is doing the work. + ExperimentalAPI.record(() -> Sim.driver(M, 10)) + @test Profile.len_data() < before + Profile.clear() end @testset "an existing Profile buffer can be attributed after the fact" begin # Scope: a twelve-hour run already profiled must not have to be run again. - @test_broken ExperimentalAPI.attribute(Profile.fetch()) isa AbstractVector + Hot.grind(10) + Hot.settled_grind(10) + Profile.clear() + Profile.init(; delay=1e-4) + Profile.@profile (Hot.grind(2_000_000); Hot.settled_grind(2_000_000)) + a = ExperimentalAPI.attribute(Profile.fetch()) + @test a isa AbstractVector + @test :grind in [x.name for x in a] + # Samples, never calls: a sampling profiler cannot count entries, and a field called `count` + # holding a sample total would read as a measurement it did not make. + @test all(x -> x isa ExperimentalAPI.Attribution, a) + @test !any(x -> hasproperty(x, :count), a) + # Control: `settled_grind` is exactly as hot and carries no mark, so it must not appear. + @test :settled_grind ∉ [x.name for x in a] + Profile.clear() end # ── the output is evidence, not a printout ─────────────────────────────────────────────────── @testset "a record is serialisable" begin - @test_broken ExperimentalAPI.write_record( + path = ExperimentalAPI.write_record( tempname(), ExperimentalAPI.record(() -> Sim.driver(M, 10)) - ) isa AbstractString + ) + @test path isa AbstractString + # Readable without this package: plain TOML, with the reason in it. + @test occursin("convergence not established", read(path, String)) end @testset "a serialised record round-trips" begin - @test_broken ExperimentalAPI.read_record( - ExperimentalAPI.write_record( - tempname(), ExperimentalAPI.record(() -> Sim.driver(M, 10)) - ), - ) isa AbstractVector + r = ExperimentalAPI.record(() -> Sim.driver(M, 10)) + back = ExperimentalAPI.read_record(ExperimentalAPI.write_record(tempname(), r)) + @test back isa AbstractVector + @test length(back) == length(r) + @test back[1].count == r[1].count + @test back[1].reason == r[1].reason + # A `Method` is not a thing a file can carry, and inventing one on the way back in would + # claim the code in this process is the code that produced the record. + @test back[1].method === nothing end @testset "a record names the versions it was taken against" begin # `energy` being experimental in v0.3 says nothing about v0.9. - @test_broken ExperimentalAPI.record(() -> Sim.driver(M, 10)).versions isa AbstractDict + v = ExperimentalAPI.record(() -> Sim.driver(M, 10)).versions + @test v isa AbstractDict + @test !isempty(v) end @testset "the reason is carried into the record" begin # Scope: readable a year later by someone who never saw the source. - @test_broken occursin( + @test occursin( "convergence", first(ExperimentalAPI.record(() -> Sim.driver(M, 10))).reason ) end @@ -401,10 +515,20 @@ end # ── using it as a gate ─────────────────────────────────────────────────────────────────────── @testset "a run can be asserted to have touched nothing experimental" begin - @test_broken ExperimentalAPI.assert_clean(() -> 1 + 1) + @test ExperimentalAPI.assert_clean(() -> 1 + 1) 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_broken !ExperimentalAPI.assert_clean(() -> Sim.driver(M, 10); throw=false) + @test !ExperimentalAPI.assert_clean(() -> Sim.driver(M, 10); throw=false) + e = try + ExperimentalAPI.assert_clean(() -> Sim.driver(M, 10)) + nothing + catch err + err + end + @test e isa ErrorException + msg = sprint(showerror, e) + @test occursin("energy", msg) + @test occursin("convergence not established", msg) # the reason, not only the name end diff --git a/test/spec/test_spec_propagate.jl b/test/spec/test_spec_propagate.jl index 798a74d..c04c19f 100644 --- a/test/spec/test_spec_propagate.jl +++ b/test/spec/test_spec_propagate.jl @@ -107,58 +107,55 @@ end # `NamedTuple` with the right field names satisfies every assertion in four files. @testset "the result has a type, not just field names" begin - @test_broken ExperimentalAPI.reach(Chain.top_bad, ENTRY) isa ExperimentalAPI.Reach + @test ExperimentalAPI.reach(Chain.top_bad, ENTRY) isa ExperimentalAPI.Reach end @testset "verdict is derived, never stored" begin # A stored `verdict` makes `:clean` with a non-empty `.unresolved` representable, which is # the one state this file forbids. Same rule as `isbreaking(d::Diff)`. - @test_broken !hasproperty(ExperimentalAPI.reach(Chain.top_bad, ENTRY), :verdict) + @test !hasproperty(ExperimentalAPI.reach(Chain.top_bad, ENTRY), :verdict) end @testset "a boolean gate exists alongside the three-valued answer" begin # Every other verdict here is a named predicate, never a comparison the caller writes out. - @test_broken ExperimentalAPI.isclean(ExperimentalAPI.reach(Chain.top_good, ENTRY)) === - true - @test_broken ExperimentalAPI.isclean(ExperimentalAPI.reach(Chain.top_bad, ENTRY)) === - false + @test ExperimentalAPI.isclean(ExperimentalAPI.reach(Chain.top_good, ENTRY)) === true + @test ExperimentalAPI.isclean(ExperimentalAPI.reach(Chain.top_bad, ENTRY)) === false end @testset ":unknown absorbs when results are combined" begin # `reach(Module)` folds every public entry into one answer, so the algebra must exist: one # `:unknown` is not clean whatever the others say, and folding is order-independent. - @test_broken ExperimentalAPI.combine(:clean, :unknown) === :unknown - @test_broken ExperimentalAPI.combine(:depends, :unknown) === :depends - @test_broken ExperimentalAPI.combine(:clean, :depends) === :depends - @test_broken ExperimentalAPI.combine(:clean, :clean) === :clean - @test_broken ExperimentalAPI.combine(:unknown, :clean) === + @test ExperimentalAPI.combine(:clean, :unknown) === :unknown + @test ExperimentalAPI.combine(:depends, :unknown) === :depends + @test ExperimentalAPI.combine(:clean, :depends) === :depends + @test ExperimentalAPI.combine(:clean, :clean) === :clean + @test ExperimentalAPI.combine(:unknown, :clean) === ExperimentalAPI.combine(:clean, :unknown) end # ── the core claim ─────────────────────────────────────────────────────────────────────────── @testset "a caller two hops away is reported as depending" begin - @test_broken ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.top_bad, ENTRY)) === - :depends - @test_broken :unstable in - [mk.name for mk in ExperimentalAPI.reach(Chain.top_bad, ENTRY).reached] + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.top_bad, ENTRY)) === :depends + # `.reached` holds `Reached`, which pairs the mark with the method and the path it was + # reached by — the name alone would not say through which of the caller's own code. + @test :unstable in + [r.mark.name for r in ExperimentalAPI.reach(Chain.top_bad, ENTRY).reached] end @testset "an equally deep caller with nothing marked is reported clean" begin # Control: rejects a tool that always says `:depends`. - @test_broken ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.top_good, ENTRY)) === - :clean - @test_broken isempty(ExperimentalAPI.reach(Chain.top_good, ENTRY).reached) + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.top_good, ENTRY)) === :clean + @test isempty(ExperimentalAPI.reach(Chain.top_good, ENTRY).reached) end @testset "a function passed as a value is still followed" begin # Specialisation on `typeof(f)` resolves this; it is not a dynamic hole. - @test_broken ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.top_arg, ENTRY)) === - :depends + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.top_arg, ENTRY)) === :depends end @testset "@nospecialize does not hide the callee" begin - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Chain.top_nospec, Tuple{typeof(Chain.unstable),Float64}) ) === :depends end @@ -167,26 +164,24 @@ end @testset "an abstract-typed callee field is :unknown, NOT :clean" begin # `Holder.f::Function` can hold `unstable`, so `:clean` here is a lie. - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Chain.top_field, Tuple{Chain.Holder,Float64}) ) === :unknown - @test_broken !isempty( + @test !isempty( ExperimentalAPI.reach(Chain.top_field, Tuple{Chain.Holder,Float64}).unresolved ) end @testset "a run-time table lookup is :unknown, NOT :clean" begin - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Chain.top_table, Tuple{Int,Float64}) ) === :unknown - @test_broken !isempty( - ExperimentalAPI.reach(Chain.top_table, Tuple{Int,Float64}).unresolved - ) + @test !isempty(ExperimentalAPI.reach(Chain.top_table, Tuple{Int,Float64}).unresolved) end @testset "every unresolved site says where it is" begin # "cannot tell" is actionable only if the user can go and look. - @test_broken all( + @test all( u -> hasproperty(u, :file) && hasproperty(u, :line), ExperimentalAPI.reach(Chain.top_field, Tuple{Chain.Holder,Float64}).unresolved, ) @@ -195,24 +190,23 @@ end @testset "a depth limit reports :unknown rather than :clean" begin # `:unknown` rather than `!== :clean`: the latter cannot separate "the limit truncated" from # "the limit was ignored and the mark was found anyway". - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Chain.deep_1, ENTRY; maxdepth=2) ) === :unknown # …and without the limit it is found, so the fixture can disagree. - @test_broken ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.deep_1, ENTRY)) === - :depends + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.deep_1, ENTRY)) === :depends end # ── termination ────────────────────────────────────────────────────────────────────────────── @testset "self-recursion terminates and still finds the mark" begin - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Chain.top_recursive, Tuple{Int,Float64}) ) === :depends end @testset "mutual recursion terminates" begin - @test_broken ExperimentalAPI.verdict( + @test ExperimentalAPI.verdict( ExperimentalAPI.reach(Chain.top_mutual_a, Tuple{Int,Float64}) ) === :depends end @@ -222,27 +216,25 @@ end @testset "a marked const is seen where it is used" begin # A const is not a call site. Either the analysis reads globals out of the IR, or the case is # declared out of scope — what it must not do is report `:clean`. - @test_broken ExperimentalAPI.verdict( - ExperimentalAPI.reach(Chain.top_uses_const, ENTRY) - ) !== :clean + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.top_uses_const, ENTRY)) !== + :clean end @testset "a marked struct is seen at construction" begin - @test_broken ExperimentalAPI.verdict( - ExperimentalAPI.reach(Chain.top_constructs, ENTRY) - ) === :depends + @test ExperimentalAPI.verdict(ExperimentalAPI.reach(Chain.top_constructs, ENTRY)) === + :depends end @testset "marking a module marks what it contains" begin # Or it does not, stated. Either way a decision, not an omission. - @test_broken hasproperty(ExperimentalAPI.reach(Chain.top_bad, ENTRY), :through_modules) + @test hasproperty(ExperimentalAPI.reach(Chain.top_bad, ENTRY), :through_modules) end # ── across packages ────────────────────────────────────────────────────────────────────────── @testset "a mark in a dependency propagates into the dependent" begin # Needs the fixture package, so only the API shape is pinned here. - @test_broken ExperimentalAPI.reach isa Function + @test ExperimentalAPI.reach isa Function end # ── cost ───────────────────────────────────────────────────────────────────────────────────── diff --git a/test/spec/test_spec_verify.jl b/test/spec/test_spec_verify.jl index 27958b3..a191a47 100644 --- a/test/spec/test_spec_verify.jl +++ b/test/spec/test_spec_verify.jl @@ -46,32 +46,89 @@ end end end -@testset "a marked definition with no coverage at all is reported" begin - @test_broken :never_exercised in [mk.name for mk in ExperimentalAPI.unverified(Covered)] +# Which half of each claim below can run depends on whether this process has coverage counters at +# all. Both halves are assertions: without `--code-coverage` the contract under test is that the +# answer is `missing` rather than a number, and that contract is exactly what stops every marked +# definition being reported unverified on an ordinary run. CI runs the suite with coverage on, so +# the measured half is what gates a pull request. +const COVERED = ExperimentalAPI.coverage_enabled() + +@testset "the run knows whether it has coverage counters at all" begin + @test COVERED == (Base.JLOptions().code_coverage != 0) +end + +@testset "a marked definition the suite never entered is reported" begin + # Unconditional: the signal is the probe the mark already emits, not the coverage data. + # `--code-coverage` cannot answer this on every version — measured on 1.14.0-DEV.3115, the + # definition line of a method nothing ever called now carries a counter, so a one-line + # definition reads as fully covered on the strength of having been defined. + @test :never_exercised in [mk.name for mk in ExperimentalAPI.unverified(Covered)] + @test ExperimentalAPI.coverage(Covered, :never_exercised) == 0.0 end @testset "a marked definition that IS covered is not reported" begin # Control: rejects a checker that reports everything. - @test_broken :exercised ∉ [mk.name for mk in ExperimentalAPI.unverified(Covered)] + @test :exercised ∉ [mk.name for mk in ExperimentalAPI.unverified(Covered)] end @testset "partial coverage is reported as partial, not as verified" begin - @test_broken 0.0 < ExperimentalAPI.coverage(Covered, :half_exercised) < 1.0 + c = ExperimentalAPI.coverage(Covered, :half_exercised) + if COVERED + @test 0.0 < c < 1.0 + # …and the fully exercised one is not reported as partial, so the number is not a + # constant that happens to sit inside the interval. + @test ExperimentalAPI.coverage(Covered, :exercised) == 1.0 + else + @test c === missing + end end @testset "coverage is absent, not zero, when the run had none enabled" begin - # No `.cov` files without `--code-coverage`; reporting 0% then flags everything on every + # No counters without `--code-coverage`; reporting 0% then flags everything on every # ordinary run. - @test_broken ExperimentalAPI.coverage(Covered, :exercised) === missing || - ExperimentalAPI.coverage(Covered, :exercised) isa Real + c = ExperimentalAPI.coverage(Covered, :exercised) + @test COVERED ? c isa Real : c === missing + # A name that carries no mark has no definition to measure, whatever the run was started with. + @test ExperimentalAPI.coverage(Covered, :not_a_name) === missing end @testset "a mark whose line no longer matches its definition is reported stale" begin - # An edit above the definition moves the code but not an existing coverage file. - @test_broken ExperimentalAPI.stale_marks(Covered) isa AbstractVector + # An edit above the definition moves the code but not the mark, and every join keyed on that + # line — the coverage one here — then describes the wrong lines silently. + @test ExperimentalAPI.stale_marks(Covered) isa AbstractVector + # This file has not been edited under itself, so nothing is stale… + @test isempty(ExperimentalAPI.stale_marks(Covered)) + + # …and the check can fire. Written to a file, loaded, then the file edited above the + # declaration: an in-memory module whose source has moved is exactly the state a long REPL + # session is in. + dir = mktempdir() + path = joinpath(dir, "moved.jl") + write( + path, + """ + module Moved + using ExperimentalAPI + public f + @experimental "shape not settled" f(x) = x + end + """, + ) + include(path) + @test isempty(ExperimentalAPI.stale_marks(Main.Moved)) # before the edit + write(path, "# a line inserted above the declaration\n" * read(path, String)) + empty!(ExperimentalAPI._SOURCE_CACHE) + empty!(ExperimentalAPI._PARSE_CACHE) + @test !isempty(ExperimentalAPI.stale_marks(Main.Moved)) # after it + @test only(ExperimentalAPI.stale_marks(Main.Moved)).name === :f end @testset "the verification report is data, not a printout" begin # Same convention as `audit`: the number comes back on the normal return path. - @test_broken ExperimentalAPI.verification(Covered) isa AbstractVector + vs = ExperimentalAPI.verification(Covered) + @test vs isa AbstractVector + @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]) end diff --git a/test/test_dogfood.jl b/test/test_dogfood.jl index 7a364f7..8d9f74e 100644 --- a/test/test_dogfood.jl +++ b/test/test_dogfood.jl @@ -11,14 +11,34 @@ end @testset "the release layer says what it is" begin declared = Set(mk.name for mk in experimental(ExperimentalAPI)) - @test declared == - Set([:snapshot, :read_snapshot, :write_snapshot, :compare, :isbreaking, :Diff]) + @test declared == Set([ + :snapshot, + :read_snapshot, + :write_snapshot, + :compare, + :compare_methods, + :isbreaking, + :stamp, + :Diff, + :MethodDiff, + ]) for mk in experimental(ExperimentalAPI) @test occursin("schema", mk.reason) # the reason is the real one, not a placeholder @test !isempty(strip(mk.reason)) end end +@testset "the extension declares its own knobs, and the parent can see it" begin + # An extension is a separate module: its public names are part of the surface a user sees and + # are invisible to `names(ExperimentalAPI)`. `extensions = true` is what reaches them. + ext = Base.get_extension(ExperimentalAPI, :ExperimentalAPITestExt) + @test ext !== nothing + @test !isempty(experimental(ext)) + @test any(mk -> mk.mod === ext, experimental(ExperimentalAPI; extensions=true)) + # Control: the parent's own marks do not answer this, so the keyword has to do something. + @test !any(mk -> mk.mod === ext, experimental(ExperimentalAPI)) +end + @testset "@experimental is the only exported name" begin # Visibility is the language's job and this package leans on it: one macro is exported # because it is written at a definition site, and everything else is `public` and qualified. diff --git a/test/test_mark.jl b/test/test_mark.jl index d799d81..c46bd53 100644 --- a/test/test_mark.jl +++ b/test/test_mark.jl @@ -150,6 +150,28 @@ end @test mark(Rewritten, :g).reason == "second reading, after a re-include" end +module Foreign +using ExperimentalAPI +struct Widget end +# A qualified definition names a method, not a surface: `Base.show` stays Base's. +@experimental "printing format not settled" Base.show(io::IO, ::Widget) = print(io, "W") +end + +@testset "a qualified definition marks the method, not the foreign name" begin + ms = ExperimentalAPI.experimental_methods(Foreign) + @test length(ms) == 1 + @test ms[1].name === :show + @test ms[1].mod === Foreign # stored where the method was written + @test ms[1].sig === Tuple{typeof(show),IO,Foreign.Widget} + @test ExperimentalAPI.isexperimental(which(show, Tuple{IO,Foreign.Widget})) + # Control: one dependent must not be able to label a whole generic unfinished. + @test !ExperimentalAPI.isexperimental(Base, :show) + @test !ExperimentalAPI.isexperimental(which(show, Tuple{IO,Int})) + # …and it is not reported as a dangling promise about `Foreign`'s own surface. + @test isempty(ExperimentalAPI.audit(Foreign; methods=false).dangling) + @test sprint(show, Foreign.Widget()) == "W" +end + @testset "a module with no marks answers, it does not fail" begin @test isempty(experimental(Base)) @test mark(Base, :sum) === nothing @@ -162,10 +184,11 @@ end using ExperimentalAPI @experimental "why" @doc "x" f(x) = x end - # A qualified definition adds a method to a name this module does not own. + # A qualified name with no signature would mark every method of `Base.sum` in the world, + # including ones this module never wrote. Guessing is worse than refusing. @test_throws LoadError @eval module R2 using ExperimentalAPI - @experimental "why" Base.sum(x::Int) = x + @experimental "why" Base.sum end # The reason is not optional, and forgetting it is caught rather than read as a name. @test_throws LoadError @eval module R3 @@ -196,7 +219,7 @@ end err = try @eval module R6 using ExperimentalAPI - @experimental "why" Base.sum(x::Int) = x + @experimental "why" Base.sum end nothing catch e @@ -204,7 +227,7 @@ end end @test err isa LoadError msg = sprint(showerror, err.error) - @test occursin("another module", msg) + @test occursin("WHICH method", msg) err2 = try @eval module R7