diff --git a/CHANGELOG.md b/CHANGELOG.md index c3990f11..6b918ea7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,11 @@ opaque function calls, closures, conditional tails). - `escape_lint` preference (`"error"` default | `"warn"` | `"off"`), loaded once at package load as the compile-time constant `ESCAPE_LINT`, controlling the severity of the three new incidental-tail patterns above. +- The runtime `PoolEscapeError` (`RUNTIME_CHECK >= 1`) fix hint now teaches the + `nothing` ending (the most common accidental-escape fix) alongside `collect()` + and returning a scalar, matching the compile-time lint wording. Expanded the + runtime-safety docs with capacity-retention, zero-allocation, and + what-it-cannot-catch guarantees. ### Performance diff --git a/docs/src/safety/runtime.md b/docs/src/safety/runtime.md index b596dd23..36769148 100644 --- a/docs/src/safety/runtime.md +++ b/docs/src/safety/runtime.md @@ -71,9 +71,11 @@ PoolEscapeError (runtime, RUNTIME_CHECK >= 1) ← acquired at src/solver.jl:42 v = acquire!(pool, Float64, n) - Fix: Wrap with collect() to return an owned copy, or compute a scalar result. + Fix: end the block with nothing if the value is discarded, collect() to return an owned copy, or compute a scalar. ``` +The `nothing` ending is the most common fix: a scope's **last expression is its return value**, so a block that ends on a pool-backed array leaks it by accident. End with `nothing` (or a scalar) when the array is only used for side effects. + ### 5. Mutation Detection Detects structural mutations that escaped compile-time analysis by comparing wrapper state against backing storage at rewind: @@ -86,6 +88,32 @@ Detects structural mutations that escaped compile-time analysis by comparing wra Emits a one-time advisory `@warn` (`maxlog=1`). The pool **self-heals** on next `acquire!` — no data corruption, only pooling benefits are temporarily lost. +## Guarantees & Limitations + +**Capacity is retained.** Poisoning and structural invalidation mark a slot's +*current* contents as stale — they do **not** shrink the pool. The slot keeps its +allocated capacity and is reused on the next `acquire!`; `compact!` skips +still-poisoned slots. So `RUNTIME_CHECK=1` never trades away pooling's memory +reuse — it only makes stale references fail loudly. + +**Allocation.** `S=0` (the production default) is zero-allocation — the checks +are dead-code-eliminated. `S=1` is a development tool and *does* allocate: borrow +tracking lazily builds an `IdDict` and records a callsite string per `acquire!` +so escape/mutation errors can name the source. The poison fills and rewind +comparisons overwrite existing buffers without allocating, but don't measure a +zero-GC hot path under `S=1` — use `S=0`. + +**What it cannot catch.** Escape detection inspects the *return value* at scope +exit — it cannot prove ownership through arrays captured by closures, stored in +globals that outlive the scope, or aliased behind opaque calls that never surface +in the return value. These are undecidable statically; `S=1` catches the +return-position cases, and the [compile-time lint](compile-time.md) catches the +common syntactic ones. + +**Test hygiene.** For tests that run under both `S=0` and `S=1`: keep asserts +S-adaptive (don't assert poison values unless `S≥1`), and end `@with_pool` blocks +with `nothing` unless you deliberately return an owned value. + ## Recommended Workflow ```toml diff --git a/src/debug.jl b/src/debug.jl index 2b893ece..636441de 100644 --- a/src/debug.jl +++ b/src/debug.jl @@ -287,9 +287,11 @@ function Base.showerror(io::IO, e::PoolRuntimeEscapeError) println(io) printstyled(io, " Fix: "; bold = true) - printstyled(io, "Wrap with "; color = :light_black) + printstyled(io, "end the block with "; color = :light_black) + printstyled(io, "nothing"; bold = true) + printstyled(io, " if the value is discarded, "; color = :light_black) printstyled(io, "collect()"; bold = true) - printstyled(io, " to return an owned copy, or compute a scalar result.\n"; color = :light_black) + printstyled(io, " to return an owned copy, or compute a scalar.\n"; color = :light_black) return nothing end diff --git a/src/macros.jl b/src/macros.jl index 9592bbc5..5fbc6d13 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -786,7 +786,12 @@ function _generate_pool_code(pool_name, expr, force_enable; safe::Bool = false, # Compile-time structural mutation detection (zero runtime cost) _check_structural_mutation(expr, pool_name, source) - # Block logic — shared with backend-specific code generation + # Block logic — shared with backend-specific code generation. + # NOTE: `@with_pool` (force_enable) and `@maybe_with_pool` (!force_enable) reuse + # this single `inner`; the only difference below is the runtime MAYBE_POOLING[] + # gate. So they never need manual syncing — any change to _generate_block_inner + # (or the escape/mutation checks above, which run before this branch) applies to + # both. The axis that DOES diverge is safe ↔ non-safe (see _generate_block_inner). inner = _generate_block_inner(pool_name, expr, safe, source) if force_enable @@ -854,6 +859,21 @@ function _generate_block_inner(pool_name, expr, safe::Bool, source) end transformed_expr = _inject_pending_callsite(transformed_expr, pool_name, expr) + # ── safe ↔ non-safe divergence (the axis that MUST be kept in sync) ────────── + # These two branches are fundamentally different control flow, NOT a shared body + # with a flag: `safe` wraps the work in try/finally (rewind guaranteed even when + # an exception escapes the outermost scope); non-safe uses a direct rewind + + # entry-depth guard + leaked-scope cleanup + break/continue injection. + # + # The split is deliberate and measurement-justified: try/finally costs a fixed + # ~4–11 ns/scope on Julia 1.12+ (≈20% on a hot typed scope, persists with real + # work) — unacceptable for the zero-overhead default, so `safe` stays opt-in. + # + # ⚠️ Any change to the checkpoint/rewind/tp-hoisting contract above MUST be + # verified on BOTH branches — the parametrized divergence matrix in + # test/test_macros.jl ("divergence matrix") is the mechanical guard. In + # particular, tp bindings must stay inside the try (see below): emitting them + # before it would leak the checkpoint if get_typed_pool! throws. if safe transformed_expr = _transform_return_stmts(transformed_expr, pool_name) return quote diff --git a/test/test_borrow_registry.jl b/test/test_borrow_registry.jl index ad4e73e1..75361fdd 100644 --- a/test/test_borrow_registry.jl +++ b/test/test_borrow_registry.jl @@ -255,6 +255,21 @@ _test_leak(x) = x @test contains(msg, "RUNTIME_CHECK >= 1") end + @testset "showerror: Fix line teaches nothing/collect/scalar" begin + # The runtime Fix suggestion should teach the `nothing` ending (the most + # common accidental-escape fix — the same ending the compile-time + # `_lint_message` recommends), alongside the collect() and scalar options. + err = PoolRuntimeEscapeError("Vector{Float64}", "Float64", "test.jl:42", nothing) + io = IOBuffer() + showerror(io, err) + msg = String(take!(io)) + + @test contains(msg, "Fix:") + @test contains(msg, "nothing") # discard-value ending + @test contains(msg, "collect()") # owned copy + @test contains(msg, "scalar") # scalar result + end + # ============================================================================== # Multiple types: each gets correct callsite # ============================================================================== diff --git a/test/test_macros.jl b/test/test_macros.jl index b2cf0a20..4d507ccf 100644 --- a/test/test_macros.jl +++ b/test/test_macros.jl @@ -2,6 +2,11 @@ import AdaptiveArrayPools: checkpoint!, rewind! @testset "Macro System" begin + # File-wide guard: several testsets below toggle the process-global + # MAYBE_POOLING flag (and some legacy ones hard-reset to `true` rather than + # preserving the entry value). Capture it here and restore it at the end so a + # run of this file cannot contaminate later test files. + _macro_system_entry_maybe = MAYBE_POOLING[] @testset "Explicit pool with checkpoint!/rewind!" begin pool = AdaptiveArrayPool() @@ -624,4 +629,214 @@ import AdaptiveArrayPools: checkpoint!, rewind! end end + # Gated to Julia >= 1.12. The metaprogramming below (@eval-generating four + # macro runners × four scenarios, with a nested macrocall spliced via `$(...)`) + # crashes Julia 1.11's type inference with a segfault on some CI platforms — a + # 1.11 compiler bug that does not reproduce on 1.10 or 1.12. The matrix targets + # the >= 1.12 tp-hoisting divergence anyway, and legacy macro behavior is + # covered by the testsets above; @static keeps this uncompiled on 1.11. + @static if VERSION >= v"1.12-" + + # ========================================================================== + # safe/non-safe divergence guard + # ========================================================================== + # `@with_pool`/`@maybe_with_pool` (non-safe) and `@safe_*` (try/finally) are + # two fundamentally different generator branches. This matrix runs identical + # scenarios through ALL FOUR macros and asserts they agree on result AND leave + # no depth/active leak — the mechanical guard that a change to one branch + # (checkpoint/rewind/tp-hoisting) must not silently break the other. A leaked + # checkpoint from tp bindings emitted before the try (instead of inside it) is + # exactly what the safe rows catch. + + # Build one runner per macro at module scope (avoids 4× body duplication). + # A macro name interpolated as `$sym pool begin … end` is not valid syntax, so + # each scope is assembled as an explicit `Expr(:macrocall, …)`. Each runner runs + # 4 scenarios exercising the divergent paths: typed fixed-slot (tp hoisting), + # fallback/lazy (local-var demotion), nested, mixed (2 fixed slots). + _divmat_mc(m, body) = Expr(:macrocall, m, LineNumberNode(0, :divmat), :pool, body) + for macname in (:with_pool, :maybe_with_pool, :safe_with_pool, :safe_maybe_with_pool) + fname = Symbol("_divmat_", macname) + m = Symbol("@", macname) + typed = _divmat_mc( + m, quote + a = acquire!(pool, Float64, 8) + @inbounds for i in 1:8 + a[i] = Float64(i) + end + sum(a) + end + ) + fallback = _divmat_mc( + m, quote + T = Vector{Float64} # local var → macro demotes to lazy path + a = acquire!(pool, T, 4) + a[1] = Float64[1.0] + length(a) + end + ) + nested = _divmat_mc( + m, quote + a = acquire!(pool, Float64, 4) + a[1] = 10.0 + inner = $( + _divmat_mc( + m, quote + b = acquire!(pool, Float64, 4) + b[1] = 20.0 + b[1] + end + ) + ) + a[1] + inner + end + ) + mixed = _divmat_mc( + m, quote + a = acquire!(pool, Float64, 4) + a[1] = 1.0 + c = acquire!(pool, Int32, 2) + c[1] = Int32(2) + Float64(a[1]) + c[1] + end + ) + @eval $fname() = ($typed, $fallback, $nested, $mixed) + end + + # These runners throw after a hoisted (fallback-type) acquire, so the safe rows + # verify that a scope rewinds on exception. Fixed slots aren't hoisted, so a + # fallback type (Vector{Float64}) is needed to actually exercise the hoisted + # binding. Note this alone does NOT pin the binding's position — the structural + # expansion test below is what guards "binding stays inside the try". + @eval _divmat_throw_safe() = @safe_with_pool pool begin + a = acquire!(pool, Vector{Float64}, 8) + a[1] = Float64[] + error("boom") + end + @eval _divmat_throw_safe_maybe() = @safe_maybe_with_pool pool begin + a = acquire!(pool, Vector{Float64}, 8) + a[1] = Float64[] + error("boom") + end + + # Every pool the matrix touches (fixed float64/int32 + fallback others) must be + # fully rewound; checking only float64 would miss a lazy/fallback rewind bug. + _divmat_clean(pool) = pool.float64.n_active == 0 && + pool.int32.n_active == 0 && + all(tp -> tp.n_active == 0, pool._others_values) + + @testset "divergence matrix: all 4 macros agree (result + no leak)" begin + # Only meaningful when pooling is compile-time enabled. With + # STATIC_POOLING = false every macro takes the DisabledPool path, so the + # guard would pass vacuously (no typed pool is ever touched). + if AdaptiveArrayPools.STATIC_POOLING + old_maybe = MAYBE_POOLING[] + MAYBE_POOLING[] = true # @maybe_* take the pooled branch + try + reset!(get_task_local_pool()) + + # Known-good results. Asserting these exact values (not just mutual + # agreement) means a shared regression that makes all four return + # the same WRONG tuple still fails: typed sum(1..8)=36.0, + # fallback length=4, nested 10+20=30.0, mixed 1+2=3.0. + expected = (36.0, 4, 30.0, 3.0) + runners = ( + ("@with_pool", _divmat_with_pool), + ("@maybe_with_pool", _divmat_maybe_with_pool), + ("@safe_with_pool", _divmat_safe_with_pool), + ("@safe_maybe_with_pool", _divmat_safe_maybe_with_pool), + ) + for (label, f) in runners + r = f() + pool = get_task_local_pool() + @test pool._current_depth == 1 # no depth leak ($label) + @test _divmat_clean(pool) # all touched pools reclaimed ($label) + @test r == expected # exact known result ($label) + end + finally + MAYBE_POOLING[] = old_maybe + end + end + end + + @testset "safe macros: exception rewinds despite throw" begin + old_maybe = MAYBE_POOLING[] + MAYBE_POOLING[] = true + try + reset!(get_task_local_pool()) + for f in (_divmat_throw_safe, _divmat_throw_safe_maybe) + pool = get_task_local_pool() + d0 = pool._current_depth + @test_throws ErrorException f() + @test pool._current_depth == d0 # finally rewound + @test _divmat_clean(pool) # no leaked capacity in use + end + finally + MAYBE_POOLING[] = old_maybe + end + end + + @testset "safe+typed expansion keeps the tp binding inside the try" begin + # Direct structural guard for the invariant the exception test cannot see: + # the hoisted get_typed_pool! binding must live INSIDE the try, so a throw + # from it still hits the finally rewind. If the binding moved before the + # try, the try body would no longer reference get_typed_pool! and this + # fails. Only meaningful where curly types are hoisted (Julia >= 1.12). + if AdaptiveArrayPools._MACRO_TYPED_UPGRADES + # The macro splices the get_typed_pool! *function value* (via `$`), so + # match that as well as a GlobalRef/Symbol spelling. + gtp = AdaptiveArrayPools.get_typed_pool! + refs_gtp(e) = + e === gtp || + (e isa GlobalRef && e.name === :get_typed_pool!) || + (e isa Symbol && e === :get_typed_pool!) || + (e isa Expr && any(refs_gtp, e.args)) + function find_try(e) + e isa Expr || return nothing + e.head === :try && return e + for a in e.args + t = find_try(a) + t === nothing || return t + end + return nothing + end + ex = @macroexpand @safe_with_pool pool begin + a = acquire!(pool, Vector{Float64}, 8) + a[1] = Float64[] + nothing + end + tnode = find_try(ex) + @test tnode !== nothing + @test refs_gtp(tnode.args[1]) # binding is inside the try body + end + end + + @testset "non-safe: parent normalizes a leaked inner scope on throw" begin + reset!(get_task_local_pool()) + # Inner @with_pool throws without rewinding; the outer scope's leaked-scope + # cleanup loop (the non-safe counterpart of the safe finally) restores depth. + f() = @with_pool pool begin + a = acquire!(pool, Float64, 4) + a[1] = 1.0 + try + @with_pool pool begin + b = acquire!(pool, Float64, 4) + error("inner boom") + end + catch + end + a[1] + end + r = f() + pool = get_task_local_pool() + @test r == 1.0 + @test pool._current_depth == 1 # outer cleanup normalized the leak + @test _divmat_clean(pool) + end + + end # @static if VERSION >= v"1.12-" (divergence matrix) + + # Restore the file-entry MAYBE_POOLING value (see capture at top). @testset + # continues past inner failures, so this runs even if a testset above failed. + MAYBE_POOLING[] = _macro_system_entry_maybe + end # Macro System