fix: an @inline marked kernel registered a probe nothing could ever set - #28
Merged
Merged
Conversation
Asked whether `@experimental` itself has settled, the churn says yes — `src/mark.jl` has not been touched since #13, across the fourteen commits since. So the question was put to a sweep of 25 definition forms instead, and the sweep found one hole, in the class this package exists to close. @experimental "kernel unverified" @inline f(x) = x # mark outside @inline @experimental "kernel unverified" g(x) = x # mark inside entered(M) → [:g] Both are marked. Both **register a probe**, so both count as observable marked definitions. Only one can ever be entered. With the mark outside, `entered` said "not entered" no matter what ran, `@entered` and `record` reported a clean run through unvalidated code, and `unverified` reported the definition forever. `_subject` was right: `@inline` and its five neighbours are in `_ANNOTATING_MACROS`, the wrapper is rebuilt around the definition, and `instrumentable` is passed through — with a comment saying "an `@inline` marked kernel is exactly the kind that has to be observable". `_instrument`, in the other file, began `(def.head === :function || def.head === :(=)) || return nothing` and handed back `nothing` for the `:macrocall` it was given. Two halves of one feature written to different assumptions. It now recurses through the wrapper and rebuilds it around the instrumented body. **The docstring documented the defect.** Its table read "`Base.@kwdef`, `@inline`, `@noinline` and the other pass-through macros | no", which is what the code did, not what the code was for. The row is now split: the six annotating macros are observed; `@generated` (its body returns an expression) and `Base.@kwdef` (it wraps a struct) are not, and each says why. **The test was named for the claim and asserted something weaker.** `@testset "@inline and the mark compose in both orders"` asserted `Set([:f, :g]) == names marked` — satisfied by a mark that can never fire. It now asserts the observation, and a second testset pins the split that a careless fix would break: instrumenting every macrocall would put a probe inside `@generated`'s returned expression. Three refusals had no test at all — measured by grepping `test/` for their message text and finding zero hits: a wrapping macro this cannot read, a `begin` block with two definitions in it, and a bare qualified name. Each is now pinned on the part that makes it actionable — the name of the macro it could not read, the word `block`, `Sub.g` and the form that would work — rather than on the fact that something was thrown. One incidental find while pinning them: a macro that throws while expanding a `module` body run through `include_string` comes back wrapped in `LoadError` **twice**, so the usual single `.error` leaves a `LoadError` that reads exactly like the failure it hides. 186 behaviours, 1210 assertions, green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
📚 Docs preview: https://codes.sota-shimozono.com/ExperimentalAPI.jl/previews/PR28/ (updates on each push to this PR) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The question was whether
@experimentalitself has settled. The churn says yes —src/mark.jlhasnot been touched since #13, across the fourteen commits since, while everything around it moved. So
I put the question to a sweep of 25 definition forms instead of to the git log, and the sweep found
one hole, in the class this package exists to close.
The finding
Both are marked. Both register a probe, so both count toward "observable marked definitions".
Only one can ever be entered. With the mark outside:
entered@entered/recordunverifiedSix macros affected:
@inline,@noinline,@propagate_inbounds,@assume_effects,@constprop,@nospecializeinfer. An@inlinekernel is exactly the small numeric body thispackage is aimed at.
Cause: two halves of one feature, written to different assumptions
_subject(src/mark.jl) was right — those six are in_ANNOTATING_MACROS, the wrapper is rebuiltaround the definition,
instrumentableis passed through, and the comment says "an@inlinemarked kernel is exactly the kind that has to be observable".
_instrument(src/detect.jl) began(def.head === :function || def.head === :(=)) || return nothingand handed backnothingfor the:macrocallit was given. The probe was dropped; theflag was registered anyway.
It now recurses through the wrapper and rebuilds it around the instrumented body.
Two documents that described the defect rather than the design
The docstring table read
`Base.@kwdef`, `@inline`, `@noinline` and the other pass-through macros | no— which is what the code did, not what the code was for. Split into two rows, eachsaying why: the annotating six are observed;
@generated(its body returns an expression) andBase.@kwdef(it wraps a struct) are not.The test was named
"@inline and the mark compose in both orders"and assertedSet([:f, :g]) == names marked— satisfied by a mark that can never fire. The two orders did notcompose the same way and the assertion was structurally unable to see it. It now asserts the
observation, and a second testset pins the split a careless fix would break: instrumenting every
macrocall would put a probe inside
@generated's returned expression.Three refusals that no test touched
Measured by grepping
test/for their message text — zero hits each:@experimental "why" the_namebegin f(x)=x; g(x)=x endblockand the alternative — half-marking two definitions is the silence this package removesSub.gSub.g,WHICH method, andSub.g(::— the form that works, spelled with their own nameIncidental: a macro that throws while expanding a
modulebody run throughinclude_stringcomesback wrapped in
LoadErrortwice, so the usual single.errorleaves aLoadErrorthat readsexactly like the failure it hides.
186 behaviours, 1210 assertions, green.
🤖 Generated with Claude Code