Correct the jsg-visit-for-gc check's type lists and document its model - #7015
Open
guybedford wants to merge 6 commits into
Open
Correct the jsg-visit-for-gc check's type lists and document its model#7015guybedford wants to merge 6 commits into
guybedford wants to merge 6 commits into
Conversation
The check had no test coverage. Adds positive/negative fixture TUs and a sh_test (consume-test pattern) that lock current behavior exactly: - P1-P5: unvisited jsg::Ref (with and without visitForGc), jsg::Name, kj::Maybe<Ref>, and kj::OneOf alternatives are diagnosed, with an exact-count assertion so lost or extra diagnostics both fail. - N1-N4: fully-visited resources, nested-struct reach-through (visitor.visit(state.func)), standalone plain holders, and jsg-namespace internals are accepted. - The mention-only blind spot (a field named in any expression counts as visited) is locked and documented as current behavior rather than silently relied upon. No change to the check itself.
jsg::Name's visitForGc is private with only NameWrapper and MemoryTracker as friends, so GcVisitor::visit(name) does not compile: no holder can satisfy a demand to visit a Name field, and none does (NameWrapper only converts values, it does not trace). The demand was unsatisfiable and could only ever be answered with a NOLINT. Not visiting a Name is also correct: the symbol handle is held as a strong root for the holder's lifetime, and a v8::Symbol holds only its description, so it cannot participate in a JS<->C++ reference cycle. The single Name field in the tree (Channel::name, retention bounded by the module's channel map) drops its NOLINT, whose comment wrongly claimed the field was "visited through NameWrapper". Full-tree clang-tidy audit remains clean with the NOLINT removed, confirming the delisting (not the NOLINT) carries the suppression.
Promise<T>::Resolver has a public visitForGc tracing the underlying V8Ref<v8::Promise::Resolver>, and the JSG docs have always required holders to visit it, but the check never matched it: Resolver is a non-template class nested in the Promise template, so its printed qualified name embeds specialization arguments and defeated the suffix match. Match the parent record's template name instead. An unvisited resolver pins the promise and its reaction closures, the same leak class as an unvisited jsg::Function. Audited every Resolver field in the tree (18 production sites) before enabling: all are already visited except the two intentionally-strong queue ReadRequest resolvers, which provably cannot be demanded against (plain structs, no visitForGc anywhere reaches their fields). Full-tree clang-tidy (all 509 ClangTidy actions re-executed against the rebuilt plugin) produces zero diagnostics.
The JSG docs list Sequence, Generator, and AsyncGenerator as GC-visitable, but the check matched none of them, and scrutiny shows the docs were only one-third right: - jsg::Generator<T> has a public visitForGc: add as a visitable leaf. - jsg::Sequence<T> is a kj::Array<T> subclass with no visitForGc of its own; it is visitable iff its element type is, via visitAll. Add as a first-arg container and correct the README row accordingly. - jsg::AsyncGenerator<T> has no visitForGc at all, so holders cannot visit one (the same impossible-demand class as jsg::Name). Do not match it; correct the README, which wrongly required visiting it. No fields of any of these types exist in the tree today (verified by inventory), and the full-tree clang-tidy run (all 509 ClangTidy actions re-executed) produces zero diagnostics; this is future-proofing plus doc truth.
Contributor
|
APIError: Invalid Anthropic API Key |
2 similar comments
Contributor
|
APIError: Invalid Anthropic API Key |
Contributor
|
APIError: Invalid Anthropic API Key |
Contributor
|
@guybedford Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
Merging this PR will improve performance by 10.12%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | bm_Promise_Fib10 |
20.3 µs | 18.5 µs | +10.12% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing gbedford/jsg-visit-for-gc-lint-correctness (328922c) with main (899b30a)
Footnotes
-
129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
…mits States the correctness model in the check header: an unvisited handle is a strong root (bounded retention, never use-after-free); visiting is what enables cycle collection and is only safe when the holder is re-traversed every GC cycle from a live wrapper. That is a traversal property the per-record structural check cannot decide, so the check never forbids a visit, and a diagnostic must never be answered by blindly adding one. Documents the three verified blind spots: mention-counts-as-visit (the cost of accepting the KJ_IF_SOME/KJ_SWITCH_ONEOF binding idiom, which an instrumented tree audit shows is the only reason all 10 mention-only sites in the tree pass — all correct), template bodies only being checked where instantiated (probe-verified: called bodies are checked, uncalled ones never run anyway), and plain holders without visitForGc only being demanded against when a visible body reaches into them.
guybedford
force-pushed
the
gbedford/jsg-visit-for-gc-lint-correctness
branch
from
August 14, 2026 18:54
7544973 to
44c571c
Compare
jasnell
approved these changes
Aug 15, 2026
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
2 participants
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.
This makes the jsg-visit-for-gc clang-tidy check's claims true, without changing any runtime behavior. The check's type lists had drifted from the framework in both directions: it demanded visits that are impossible to satisfy, and silently failed to match types the JSG docs have always required holders to visit.
GcVisitor::visit(name)does not compile and no holder can satisfy the demand. The sole Name field in the tree (Channel::name) carried a NOLINT whose comment wrongly claimed NameWrapper traces it. A Name's symbol handle is a strong root, and a v8::Symbol cannot form a JS<->C++ cycle, so visitation is never required either.Every list change was gated on a full-tree clang-tidy run (all 509 ClangTidy actions re-executed against the rebuilt plugin) producing zero diagnostics, plus an instrumented tree audit: all 10 mention-only-satisfied fields were individually verified as the correct binding idiom, and the never-checked plain-holder landscape was inventoried (138 fields: 77 JSG_STRUCT value types, 28 transitively covered in their authoritative TU, 33 deliberate strong roots kept as a follow-up review list).
The only non-tooling change is the Channel::name comment, corrected to the truth.