From 58aa8e437a9630b8a1dd3f625dfbc715e69a08a3 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Thu, 13 Aug 2026 18:11:13 -0700 Subject: [PATCH 1/5] clang-tidy: add regression fixtures for jsg-visit-for-gc 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, 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. --- tools/clang-tidy/BUILD.bazel | 16 +++ .../clang-tidy/visit-for-gc-negative-test.c++ | 108 ++++++++++++++++++ .../clang-tidy/visit-for-gc-positive-test.c++ | 95 +++++++++++++++ tools/clang-tidy/visit-for-gc-test.sh | 63 ++++++++++ 4 files changed, 282 insertions(+) create mode 100644 tools/clang-tidy/visit-for-gc-negative-test.c++ create mode 100644 tools/clang-tidy/visit-for-gc-positive-test.c++ create mode 100755 tools/clang-tidy/visit-for-gc-test.sh diff --git a/tools/clang-tidy/BUILD.bazel b/tools/clang-tidy/BUILD.bazel index 231e99a222b..066ff61de64 100644 --- a/tools/clang-tidy/BUILD.bazel +++ b/tools/clang-tidy/BUILD.bazel @@ -113,6 +113,22 @@ sh_test( ], ) +sh_test( + name = "visit-for-gc-test", + srcs = ["visit-for-gc-test.sh"], + data = [ + ":visit-for-gc-negative-test.c++", + ":visit-for-gc-positive-test.c++", + ":workerd-lint", + "//tools:clang-tidy", + ], + tags = ["no-asan"], + target_compatible_with = [ + "@platforms//os:linux", + "@platforms//cpu:x86_64", + ], +) + # Tests the `custom-iocontext-run-manual-capture` query-based check, which is # defined in the workerd `.clang-tidy` config rather than in the plugin. The # test drives clang-tidy against the real merged config, so it does not need the diff --git a/tools/clang-tidy/visit-for-gc-negative-test.c++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ new file mode 100644 index 00000000000..a276ffb5e2c --- /dev/null +++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ @@ -0,0 +1,108 @@ +// Copyright (c) 2017-2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +// Negative fixtures for the jsg-visit-for-gc check: nothing below may produce +// a diagnostic. Self-contained stubs mirror the qualified names the check +// keys on. + +namespace workerd::jsg { + +class GcVisitor; + +template +class Ref { + public: + void visitForGc(GcVisitor& visitor) {} +}; + +class Name { + public: + void visitForGc(GcVisitor& visitor) {} +}; + +class GcVisitor { + public: + template + void visit(Args&&... args) {} + template + void visitAll(T& collection) {} +}; + +class Object { + public: + void visitForGc(GcVisitor& visitor) {} +}; + +// Records inside namespace jsg are framework internals and are skipped. +class FrameworkInternal { + public: + Ref unvisited; +}; + +} // namespace workerd::jsg + +namespace kj { + +template +class Maybe { + public: + bool operator==(decltype(nullptr)) const { + return true; + } +}; + +template +class OneOf {}; + +} // namespace kj + +namespace jsg = workerd::jsg; + +struct Widget: public jsg::Object {}; + +// Case N1: every visitable field is visited. +struct AllVisited: public jsg::Object { + jsg::Ref ref; + kj::Maybe> maybeRef; + kj::OneOf> stateful; + jsg::Name name; + + void visitForGc(jsg::GcVisitor& visitor) { + visitor.visit(ref, maybeRef, stateful, name); + } +}; + +struct NestedState { + jsg::Ref func; +}; + +// Case N2: a parent's visitForGc reaches into a directly-held nested struct +// member; the nested struct needs no visitForGc of its own. +struct ParentReachesNested: public jsg::Object { + NestedState state; + + void visitForGc(jsg::GcVisitor& visitor) { + visitor.visit(state.func); + } +}; + +// Case N3: a plain standalone holder (no jsg::Object base, no visitForGc, not +// used as a field of any record in this TU) is not demanded against. +struct StandalonePlainHolder { + jsg::Ref strongRoot; +}; + +// Case N4: KNOWN BLIND SPOT, locked as current behavior. Any MemberExpr +// naming the field inside the visitForGc body counts as a "visit" — including +// a mere comparison. The check does not verify the field is an argument of +// GcVisitor::visit. +struct MentionOnlyCountsAsVisit: public jsg::Object { + kj::Maybe> mentioned; + + void visitForGc(jsg::GcVisitor& visitor) { + if (mentioned == nullptr) { + return; + } + } +}; diff --git a/tools/clang-tidy/visit-for-gc-positive-test.c++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ new file mode 100644 index 00000000000..4107a00a25f --- /dev/null +++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ @@ -0,0 +1,95 @@ +// Copyright (c) 2017-2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +// Positive fixtures for the jsg-visit-for-gc check: every case below must +// produce exactly one diagnostic, and visit-for-gc-test.sh asserts the exact +// total. Self-contained stubs mirror the qualified names the check keys on. + +namespace workerd::jsg { + +class GcVisitor; + +template +class Ref { + public: + void visitForGc(GcVisitor& visitor) {} +}; + +class Name { + public: + void visitForGc(GcVisitor& visitor) {} +}; + +class GcVisitor { + public: + template + void visit(Args&&... args) {} + template + void visitAll(T& collection) {} +}; + +class Object { + public: + void visitForGc(GcVisitor& visitor) {} +}; + +} // namespace workerd::jsg + +namespace kj { + +template +class Maybe { + public: + bool operator==(decltype(nullptr)) const { + return true; + } +}; + +template +class OneOf {}; + +} // namespace kj + +namespace jsg = workerd::jsg; + +struct Widget: public jsg::Object {}; + +// Case P1: visitForGc exists but misses a jsg::Ref field. +struct MissedRefField: public jsg::Object { + jsg::Ref visited; + jsg::Ref missed; + + void visitForGc(jsg::GcVisitor& visitor) { + visitor.visit(visited); + } +}; + +// Case P2: resource type with no visitForGc of its own; the framework +// dispatches to jsg::Object's empty default and misses the field. +struct NoVisitMethod: public jsg::Object { + jsg::Ref orphaned; +}; + +// Case P3: unvisited jsg::Name field. This locks CURRENT behavior: the check +// lists jsg::Name as a visitable leaf type. +struct MissedNameField: public jsg::Object { + jsg::Name name; + + void visitForGc(jsg::GcVisitor& visitor) {} +}; + +// Case P4: unvisited kj::Maybe> field (FirstArg container). +struct MissedMaybeRef: public jsg::Object { + kj::Maybe> maybeRef; + + void visitForGc(jsg::GcVisitor& visitor) {} +}; + +// Case P5: unvisited kj::OneOf with a visitable alternative (AnyArg +// container). +struct MissedOneOf: public jsg::Object { + kj::OneOf> stateful; + + void visitForGc(jsg::GcVisitor& visitor) {} +}; diff --git a/tools/clang-tidy/visit-for-gc-test.sh b/tools/clang-tidy/visit-for-gc-test.sh new file mode 100755 index 00000000000..d47d9e3080d --- /dev/null +++ b/tools/clang-tidy/visit-for-gc-test.sh @@ -0,0 +1,63 @@ +#!/usr/bin/env bash + +# Copyright (c) 2017-2026 Cloudflare, Inc. +# Licensed under the Apache 2.0 license found in the LICENSE file or at: +# https://opensource.org/licenses/Apache-2.0 + +set -euo pipefail + +readonly ROOT="${TEST_SRCDIR}/${TEST_WORKSPACE}" +readonly CLANG_TIDY="${ROOT}/tools/clang_tidy" +readonly PLUGIN="${ROOT}/tools/clang-tidy/libworkerd-lint.so" +readonly POSITIVE="${ROOT}/tools/clang-tidy/visit-for-gc-positive-test.c++" +readonly NEGATIVE="${ROOT}/tools/clang-tidy/visit-for-gc-negative-test.c++" +readonly CHECKS="-*,jsg-visit-for-gc" + +set +e +positive_output=$("${CLANG_TIDY}" "--load=${PLUGIN}" --checks="${CHECKS}" \ + --warnings-as-errors='*' "${POSITIVE}" -- -std=c++23 2>&1) +positive_status=$? +set -e + +if [[ ${positive_status} -eq 0 ]]; then + printf '%s\n' "Expected jsg-visit-for-gc to reject the positive fixtures." >&2 + printf '%s\n' "${positive_output}" >&2 + exit 1 +fi + +expect_diag() { + local needle="$1" + local description="$2" + if [[ "${positive_output}" != *"${needle}"* ]]; then + printf 'Missing expected diagnostic (%s): %s\n' "${description}" "${needle}" >&2 + printf '%s\n' "${positive_output}" >&2 + exit 1 + fi +} + +expect_diag "field 'missed' of visitable type" "P1: unvisited jsg::Ref field" +expect_diag \ + "field 'orphaned' of visitable type 'jsg::Ref' is not visited in visitForGc (class has no visitForGc method)" \ + "P2: resource with no visitForGc" +expect_diag "field 'name' of visitable type" "P3: unvisited jsg::Name field" +expect_diag "field 'maybeRef' of visitable type" "P4: unvisited kj::Maybe" +expect_diag "field 'stateful' of visitable type" "P5: unvisited kj::OneOf alternative" + +# Exact-count lock: one diagnostic per positive case, no more, no fewer. +expected_count=5 +actual_count=$(grep -c "\[jsg-visit-for-gc" <<<"${positive_output}" || true) +if [[ "${actual_count}" -ne "${expected_count}" ]]; then + printf 'Expected exactly %s jsg-visit-for-gc diagnostics, got %s\n' \ + "${expected_count}" "${actual_count}" >&2 + printf '%s\n' "${positive_output}" >&2 + exit 1 +fi + +negative_output=$("${CLANG_TIDY}" "--load=${PLUGIN}" --checks="${CHECKS}" \ + --warnings-as-errors='*' "${NEGATIVE}" -- -std=c++23 2>&1) + +if [[ "${negative_output}" == *"jsg-visit-for-gc"* ]]; then + printf '%s\n' "Expected the negative fixtures to be accepted." >&2 + printf '%s\n' "${negative_output}" >&2 + exit 1 +fi From 84c18fd6f1b9e8fea719307fac379ef9340df76f Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Thu, 13 Aug 2026 18:16:06 -0700 Subject: [PATCH 2/5] clang-tidy: remove jsg::Name from jsg-visit-for-gc visitable leaf types 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. --- build/AGENTS.md | 3 +-- src/workerd/api/node/diagnostics-channel.h | 8 ++++---- src/workerd/jsg/AGENTS.md | 4 ++-- src/workerd/jsg/README.md | 6 +++++- .../clang-tidy/visit-for-gc-negative-test.c++ | 19 +++++++++++++++---- .../clang-tidy/visit-for-gc-positive-test.c++ | 13 ------------- tools/clang-tidy/visit-for-gc-test.sh | 3 +-- tools/clang-tidy/visit-for-gc.c++ | 8 +++++++- 8 files changed, 35 insertions(+), 29 deletions(-) diff --git a/build/AGENTS.md b/build/AGENTS.md index 459b9aed628..afb00c298cd 100644 --- a/build/AGENTS.md +++ b/build/AGENTS.md @@ -68,8 +68,7 @@ Usage: plugin via `--load=`. - Suppress an intentional non-visit with `// NOLINT(jsg-visit-for-gc)` plus a comment explaining why the field is safe to skip (see `src/workerd/api/streams/queue.h` - for `ByteQueue::Entry::store` and `src/workerd/api/node/diagnostics-channel.h` - for `Channel::name`). + for `ByteQueue::Entry::store`). ### Incremental check rollout diff --git a/src/workerd/api/node/diagnostics-channel.h b/src/workerd/api/node/diagnostics-channel.h index ffb6cfc457f..77e6bd22284 100644 --- a/src/workerd/api/node/diagnostics-channel.h +++ b/src/workerd/api/node/diagnostics-channel.h @@ -73,10 +73,10 @@ class Channel: public jsg::Object { } }; - // jsg::Name has a private visitForGc and is visited through NameWrapper - // rather than through the GcVisitor::visit() overload set, so we cannot - // and do not visit it from Channel::visitForGc. - jsg::Name name; // NOLINT(jsg-visit-for-gc) + // Not GC-visited: jsg::Name's visitForGc is private, so holders cannot + // visit it. The symbol handle (if any) is held as a strong root for the + // Channel's lifetime, which is bounded by the module's channel map. + jsg::Name name; kj::HashMap, MessageCallback> subscribers; kj::Table> stores; diff --git a/src/workerd/jsg/AGENTS.md b/src/workerd/jsg/AGENTS.md index 496a7a27244..21e96964c43 100644 --- a/src/workerd/jsg/AGENTS.md +++ b/src/workerd/jsg/AGENTS.md @@ -78,8 +78,8 @@ class MyType: public jsg::Object { These rules MUST be followed when writing or modifying JSG code: 1. **MUST implement `visitForGc()`** on any Resource Type holding `Ref`, `V8Ref`, - `JsRef`, `Function`, `Promise`, `Promise::Resolver`, or - `Name` — see `README.md` §GC-Visitable Types for the complete list + `JsRef`, `Function`, `Promise`, or `Promise::Resolver` — see + `README.md` §GC-Visitable Types for the complete list 2. **MUST visit ALL GC-visitable fields** — missing one causes GC corruption 3. **MUST NOT store `v8::Local` or `JsValue` types as class members** — use `V8Ref` or `JsRef` for persistence diff --git a/src/workerd/jsg/README.md b/src/workerd/jsg/README.md index be8a83f73ba..0d01864d97c 100644 --- a/src/workerd/jsg/README.md +++ b/src/workerd/jsg/README.md @@ -235,7 +235,6 @@ All types that must be visited in `visitForGc()` if held as Resource Type member | `jsg::JsRef` | Persistent ref to JsValue type | | `jsg::Optional` | When `T` is GC-visitable | | `jsg::LenientOptional` | When `T` is GC-visitable | -| `jsg::Name` | Property name (string or symbol) | | `jsg::Function` | Wrapped JS/C++ function | | `jsg::Promise` | JS promise wrapper | | `jsg::Promise::Resolver` | Promise resolver | @@ -250,6 +249,11 @@ This is intentionally weak and does NOT keep its target alive during GC. Attempting to `visitor.visit()` a weak ref field is a compile error — the correct signal that weak references should not be traced. Do not include them in `visitForGc()`. +`jsg::Name` is also not visitable: its `visitForGc` is private (friend-only), so +holders cannot visit it. A `Name` field's symbol handle is held as a strong root +for the holder's lifetime; a `v8::Symbol` cannot participate in a JS↔C++ +reference cycle, so this cannot leak cycles — only pin the symbol wrapper. + ## Weak References `jsg::WeakRef` provides a non-owning, automatically-invalidated reference diff --git a/tools/clang-tidy/visit-for-gc-negative-test.c++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ index a276ffb5e2c..c078ef4349a 100644 --- a/tools/clang-tidy/visit-for-gc-negative-test.c++ +++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ @@ -16,8 +16,10 @@ class Ref { void visitForGc(GcVisitor& visitor) {} }; +// Mirrors the real jsg::Name: visitForGc is private (friend-only), so no +// holder can visit a Name. class Name { - public: + private: void visitForGc(GcVisitor& visitor) {} }; @@ -66,10 +68,9 @@ struct AllVisited: public jsg::Object { jsg::Ref ref; kj::Maybe> maybeRef; kj::OneOf> stateful; - jsg::Name name; void visitForGc(jsg::GcVisitor& visitor) { - visitor.visit(ref, maybeRef, stateful, name); + visitor.visit(ref, maybeRef, stateful); } }; @@ -93,7 +94,17 @@ struct StandalonePlainHolder { jsg::Ref strongRoot; }; -// Case N4: KNOWN BLIND SPOT, locked as current behavior. Any MemberExpr +// Case N4: jsg::Name is not a demanded type. Its visitForGc is private +// (friend-only), so no holder can visit it; the symbol handle is held as a +// strong root for the holder's lifetime, and a v8::Symbol cannot form a +// JS<->C++ cycle, so visitation is never required. +struct UnvisitedNameField: public jsg::Object { + jsg::Name name; + + void visitForGc(jsg::GcVisitor& visitor) {} +}; + +// Case N5: KNOWN BLIND SPOT, locked as current behavior. Any MemberExpr // naming the field inside the visitForGc body counts as a "visit" — including // a mere comparison. The check does not verify the field is an argument of // GcVisitor::visit. diff --git a/tools/clang-tidy/visit-for-gc-positive-test.c++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ index 4107a00a25f..85d7d1f34db 100644 --- a/tools/clang-tidy/visit-for-gc-positive-test.c++ +++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ @@ -16,11 +16,6 @@ class Ref { void visitForGc(GcVisitor& visitor) {} }; -class Name { - public: - void visitForGc(GcVisitor& visitor) {} -}; - class GcVisitor { public: template @@ -71,14 +66,6 @@ struct NoVisitMethod: public jsg::Object { jsg::Ref orphaned; }; -// Case P3: unvisited jsg::Name field. This locks CURRENT behavior: the check -// lists jsg::Name as a visitable leaf type. -struct MissedNameField: public jsg::Object { - jsg::Name name; - - void visitForGc(jsg::GcVisitor& visitor) {} -}; - // Case P4: unvisited kj::Maybe> field (FirstArg container). struct MissedMaybeRef: public jsg::Object { kj::Maybe> maybeRef; diff --git a/tools/clang-tidy/visit-for-gc-test.sh b/tools/clang-tidy/visit-for-gc-test.sh index d47d9e3080d..3f8d8a74975 100755 --- a/tools/clang-tidy/visit-for-gc-test.sh +++ b/tools/clang-tidy/visit-for-gc-test.sh @@ -39,12 +39,11 @@ expect_diag "field 'missed' of visitable type" "P1: unvisited jsg::Ref field" expect_diag \ "field 'orphaned' of visitable type 'jsg::Ref' is not visited in visitForGc (class has no visitForGc method)" \ "P2: resource with no visitForGc" -expect_diag "field 'name' of visitable type" "P3: unvisited jsg::Name field" expect_diag "field 'maybeRef' of visitable type" "P4: unvisited kj::Maybe" expect_diag "field 'stateful' of visitable type" "P5: unvisited kj::OneOf alternative" # Exact-count lock: one diagnostic per positive case, no more, no fewer. -expected_count=5 +expected_count=4 actual_count=$(grep -c "\[jsg-visit-for-gc" <<<"${positive_output}" || true) if [[ "${actual_count}" -ne "${expected_count}" ]]; then printf 'Expected exactly %s jsg-visit-for-gc diagnostics, got %s\n' \ diff --git a/tools/clang-tidy/visit-for-gc.c++ b/tools/clang-tidy/visit-for-gc.c++ index 80524793014..9f5d630772b 100644 --- a/tools/clang-tidy/visit-for-gc.c++ +++ b/tools/clang-tidy/visit-for-gc.c++ @@ -43,8 +43,14 @@ const llvm::StringRef kVisitableLeafTemplates[] = { }; // Non-template visitable leaf types. +// +// jsg::Name is deliberately absent: its visitForGc is private (friend-only), +// so GcVisitor::visit(name) does not compile and no holder can satisfy a +// demand to visit it. A Name field's symbol handle is simply held as a strong +// root for the holder's lifetime; a v8::Symbol holds only its description and +// cannot participate in a JS<->C++ reference cycle, so visitation is never +// required for collectability either. const llvm::StringRef kVisitableLeafTypes[] = { - "jsg::Name", "jsg::Value", "jsg::Data", }; From e467808b4c09541978e3470eef0ca5009dbab731 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Thu, 13 Aug 2026 18:20:38 -0700 Subject: [PATCH 3/5] clang-tidy: match jsg::Promise::Resolver in jsg-visit-for-gc Promise::Resolver has a public visitForGc tracing the underlying V8Ref, 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. --- .../clang-tidy/visit-for-gc-negative-test.c++ | 21 +++++++++++++++ .../clang-tidy/visit-for-gc-positive-test.c++ | 27 +++++++++++++++++++ tools/clang-tidy/visit-for-gc-test.sh | 4 ++- tools/clang-tidy/visit-for-gc.c++ | 22 +++++++++++++++ 4 files changed, 73 insertions(+), 1 deletion(-) diff --git a/tools/clang-tidy/visit-for-gc-negative-test.c++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ index c078ef4349a..cb9edf61458 100644 --- a/tools/clang-tidy/visit-for-gc-negative-test.c++ +++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ @@ -23,6 +23,17 @@ class Name { void visitForGc(GcVisitor& visitor) {} }; +template +class Promise { + public: + class Resolver { + public: + void visitForGc(GcVisitor& visitor) {} + }; + + void visitForGc(GcVisitor& visitor) {} +}; + class GcVisitor { public: template @@ -117,3 +128,13 @@ struct MentionOnlyCountsAsVisit: public jsg::Object { } } }; + +// Case N6: visited resolver fields are accepted. +struct VisitedResolver: public jsg::Object { + jsg::Promise::Resolver resolver; + kj::Maybe::Resolver> maybeResolver; + + void visitForGc(jsg::GcVisitor& visitor) { + visitor.visit(resolver, maybeResolver); + } +}; diff --git a/tools/clang-tidy/visit-for-gc-positive-test.c++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ index 85d7d1f34db..73ef5482fe9 100644 --- a/tools/clang-tidy/visit-for-gc-positive-test.c++ +++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ @@ -16,6 +16,17 @@ class Ref { void visitForGc(GcVisitor& visitor) {} }; +template +class Promise { + public: + class Resolver { + public: + void visitForGc(GcVisitor& visitor) {} + }; + + void visitForGc(GcVisitor& visitor) {} +}; + class GcVisitor { public: template @@ -80,3 +91,19 @@ struct MissedOneOf: public jsg::Object { void visitForGc(jsg::GcVisitor& visitor) {} }; + +// Case P6: unvisited jsg::Promise::Resolver field. Resolver has a public +// visitForGc tracing the underlying V8Ref; leaving it unvisited pins the +// promise and its reaction closures. +struct MissedResolver: public jsg::Object { + jsg::Promise::Resolver resolver; + + void visitForGc(jsg::GcVisitor& visitor) {} +}; + +// Case P7: unvisited kj::Maybe::Resolver> field. +struct MissedMaybeResolver: public jsg::Object { + kj::Maybe::Resolver> maybeResolver; + + void visitForGc(jsg::GcVisitor& visitor) {} +}; diff --git a/tools/clang-tidy/visit-for-gc-test.sh b/tools/clang-tidy/visit-for-gc-test.sh index 3f8d8a74975..73cc0a802a4 100755 --- a/tools/clang-tidy/visit-for-gc-test.sh +++ b/tools/clang-tidy/visit-for-gc-test.sh @@ -41,9 +41,11 @@ expect_diag \ "P2: resource with no visitForGc" expect_diag "field 'maybeRef' of visitable type" "P4: unvisited kj::Maybe" expect_diag "field 'stateful' of visitable type" "P5: unvisited kj::OneOf alternative" +expect_diag "field 'resolver' of visitable type" "P6: unvisited Promise::Resolver" +expect_diag "field 'maybeResolver' of visitable type" "P7: unvisited Maybe" # Exact-count lock: one diagnostic per positive case, no more, no fewer. -expected_count=4 +expected_count=6 actual_count=$(grep -c "\[jsg-visit-for-gc" <<<"${positive_output}" || true) if [[ "${actual_count}" -ne "${expected_count}" ]]; then printf 'Expected exactly %s jsg-visit-for-gc diagnostics, got %s\n' \ diff --git a/tools/clang-tidy/visit-for-gc.c++ b/tools/clang-tidy/visit-for-gc.c++ index 9f5d630772b..5cfbc797750 100644 --- a/tools/clang-tidy/visit-for-gc.c++ +++ b/tools/clang-tidy/visit-for-gc.c++ @@ -55,6 +55,25 @@ const llvm::StringRef kVisitableLeafTypes[] = { "jsg::Data", }; +// jsg::Promise::Resolver has a public visitForGc (it traces the underlying +// V8Ref). It is a non-template class nested inside the +// Promise template, so its printed qualified name embeds the specialization +// arguments ("jsg::Promise::Resolver"); match the parent's template +// name instead of suffix-matching the full string. +bool isPromiseResolver(const clang::CXXRecordDecl *rd) { + if (rd->getName() != "Resolver") return false; + const auto *parent = llvm::dyn_cast(rd->getDeclContext()); + if (parent == nullptr) return false; + std::string fqn; + if (const auto *spec = + llvm::dyn_cast(parent)) { + fqn = spec->getSpecializedTemplate()->getQualifiedNameAsString(); + } else { + fqn = parent->getQualifiedNameAsString(); + } + return endsWithQualified(fqn, "jsg::Promise"); +} + // Container templates whose visitability is determined by their type // arguments. `FirstArg` containers visit one element; `AnyArg` containers // (variants) are visitable if any element type is. @@ -99,6 +118,9 @@ bool isVisitableType(clang::QualType qt) { for (auto suffix : kVisitableLeafTypes) { if (endsWithQualified(fqn, suffix)) return true; } + if (const auto *rd = llvm::dyn_cast(rt->getDecl())) { + if (isPromiseResolver(rd)) return true; + } } // Template specialization: dispatch on outer template name. From ebb903075b9e86feedb02e9a2b863760f3670273 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Thu, 13 Aug 2026 18:24:01 -0700 Subject: [PATCH 4/5] clang-tidy: match jsg::Generator and jsg::Sequence in jsg-visit-for-gc 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 has a public visitForGc: add as a visitable leaf. - jsg::Sequence is a kj::Array 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 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. --- src/workerd/jsg/README.md | 6 ++-- .../clang-tidy/visit-for-gc-negative-test.c++ | 28 +++++++++++++++++++ .../clang-tidy/visit-for-gc-positive-test.c++ | 25 +++++++++++++++++ tools/clang-tidy/visit-for-gc-test.sh | 4 ++- tools/clang-tidy/visit-for-gc.c++ | 14 ++++++++-- 5 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/workerd/jsg/README.md b/src/workerd/jsg/README.md index 0d01864d97c..beacd80732d 100644 --- a/src/workerd/jsg/README.md +++ b/src/workerd/jsg/README.md @@ -238,9 +238,8 @@ All types that must be visited in `visitForGc()` if held as Resource Type member | `jsg::Function` | Wrapped JS/C++ function | | `jsg::Promise` | JS promise wrapper | | `jsg::Promise::Resolver` | Promise resolver | -| `jsg::Sequence` | Iterable sequence | +| `jsg::Sequence` | When `T` is GC-visitable (use `visitor.visitAll`) | | `jsg::Generator` | Sync generator | -| `jsg::AsyncGenerator` | Async generator | | `kj::Maybe` | When `T` is GC-visitable | **Not GC-visitable** (compile error if visited): @@ -254,6 +253,9 @@ holders cannot visit it. A `Name` field's symbol handle is held as a strong root for the holder's lifetime; a `v8::Symbol` cannot participate in a JS↔C++ reference cycle, so this cannot leak cycles — only pin the symbol wrapper. +`jsg::AsyncGenerator` is likewise not visitable (it has no `visitForGc`, +unlike `jsg::Generator`); a holder keeps its handles as strong roots. + ## Weak References `jsg::WeakRef` provides a non-owning, automatically-invalidated reference diff --git a/tools/clang-tidy/visit-for-gc-negative-test.c++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ index cb9edf61458..55bf50830f6 100644 --- a/tools/clang-tidy/visit-for-gc-negative-test.c++ +++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ @@ -34,6 +34,20 @@ class Promise { void visitForGc(GcVisitor& visitor) {} }; +template +class Generator { + public: + void visitForGc(GcVisitor& visitor) {} +}; + +// Mirrors the real jsg::AsyncGenerator: no visitForGc, so holders cannot +// visit one. +template +class AsyncGenerator {}; + +template +class Sequence {}; + class GcVisitor { public: template @@ -138,3 +152,17 @@ struct VisitedResolver: public jsg::Object { visitor.visit(resolver, maybeResolver); } }; + +// Case N7: visited Generator, Sequence of non-visitable elements, Sequence +// visited via visitAll, and AsyncGenerator (not visitable, held strong). +struct GeneratorAndSequence: public jsg::Object { + jsg::Generator gen; + jsg::Sequence plainSeq; + jsg::Sequence> refSeq; + jsg::AsyncGenerator asyncGen; + + void visitForGc(jsg::GcVisitor& visitor) { + visitor.visit(gen); + visitor.visitAll(refSeq); + } +}; diff --git a/tools/clang-tidy/visit-for-gc-positive-test.c++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ index 73ef5482fe9..c96ddd76fdd 100644 --- a/tools/clang-tidy/visit-for-gc-positive-test.c++ +++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ @@ -27,6 +27,15 @@ class Promise { void visitForGc(GcVisitor& visitor) {} }; +template +class Generator { + public: + void visitForGc(GcVisitor& visitor) {} +}; + +template +class Sequence {}; + class GcVisitor { public: template @@ -107,3 +116,19 @@ struct MissedMaybeResolver: public jsg::Object { void visitForGc(jsg::GcVisitor& visitor) {} }; + +// Case P8: unvisited jsg::Generator field (public visitForGc, traces the +// generator's underlying object handle). +struct MissedGenerator: public jsg::Object { + jsg::Generator gen; + + void visitForGc(jsg::GcVisitor& visitor) {} +}; + +// Case P9: unvisited jsg::Sequence with a visitable element type (visited via +// GcVisitor::visitAll in real code). +struct MissedSequence: public jsg::Object { + jsg::Sequence> seq; + + void visitForGc(jsg::GcVisitor& visitor) {} +}; diff --git a/tools/clang-tidy/visit-for-gc-test.sh b/tools/clang-tidy/visit-for-gc-test.sh index 73cc0a802a4..fc8e13b0384 100755 --- a/tools/clang-tidy/visit-for-gc-test.sh +++ b/tools/clang-tidy/visit-for-gc-test.sh @@ -43,9 +43,11 @@ expect_diag "field 'maybeRef' of visitable type" "P4: unvisited kj::Maybe::Resolver" expect_diag "field 'maybeResolver' of visitable type" "P7: unvisited Maybe" +expect_diag "field 'gen' of visitable type" "P8: unvisited jsg::Generator" +expect_diag "field 'seq' of visitable type" "P9: unvisited jsg::Sequence of visitable elements" # Exact-count lock: one diagnostic per positive case, no more, no fewer. -expected_count=6 +expected_count=8 actual_count=$(grep -c "\[jsg-visit-for-gc" <<<"${positive_output}" || true) if [[ "${actual_count}" -ne "${expected_count}" ]]; then printf 'Expected exactly %s jsg-visit-for-gc diagnostics, got %s\n' \ diff --git a/tools/clang-tidy/visit-for-gc.c++ b/tools/clang-tidy/visit-for-gc.c++ index 5cfbc797750..0b145e22717 100644 --- a/tools/clang-tidy/visit-for-gc.c++ +++ b/tools/clang-tidy/visit-for-gc.c++ @@ -35,11 +35,16 @@ bool endsWithQualified(llvm::StringRef qualifiedName, llvm::StringRef suffix) { return qualifiedName[sep - 1] == ':' && qualifiedName[sep - 2] == ':'; } -// Visitable leaf templates: each holds a GC root and must be visited. +// Visitable leaf templates: each holds a GC root, has a public visitForGc, +// and must be visited. +// +// jsg::AsyncGenerator is deliberately absent: unlike jsg::Generator it has no +// visitForGc at all, so a holder cannot visit one — its handles are strong +// roots for the holder's lifetime. const llvm::StringRef kVisitableLeafTemplates[] = { "jsg::Ref", "jsg::V8Ref", "jsg::JsRef", "jsg::Function", "jsg::Promise", "jsg::HashableV8Ref", - "jsg::MemoizedIdentity", + "jsg::MemoizedIdentity", "jsg::Generator", }; // Non-template visitable leaf types. @@ -79,9 +84,12 @@ bool isPromiseResolver(const clang::CXXRecordDecl *rd) { // (variants) are visitable if any element type is. enum class ContainerKind { None, FirstArg, AnyArg }; +// jsg::Sequence is a kj::Array subclass with no visitForGc of its own; +// like the other containers it is visitable iff its element type is (via +// GcVisitor::visitAll). const llvm::StringRef kFirstArgContainers[] = { "kj::Maybe", "kj::Array", "kj::Vector", - "jsg::Optional", "jsg::LenientOptional", + "jsg::Optional", "jsg::LenientOptional", "jsg::Sequence", }; const llvm::StringRef kAnyArgContainers[] = { From 44c571cd975be93035f5ad9ca20b84a49c780d19 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Thu, 13 Aug 2026 18:35:07 -0700 Subject: [PATCH 5/5] clang-tidy: document the jsg-visit-for-gc model and its deliberate limits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/workerd/api/node/diagnostics-channel.h | 5 ++- src/workerd/jsg/README.md | 10 +++--- .../clang-tidy/visit-for-gc-negative-test.c++ | 34 +++++++------------ .../clang-tidy/visit-for-gc-positive-test.c++ | 19 ++++------- tools/clang-tidy/visit-for-gc.c++ | 33 ++++++------------ tools/clang-tidy/visit-for-gc.h | 21 +++++++++--- 6 files changed, 53 insertions(+), 69 deletions(-) diff --git a/src/workerd/api/node/diagnostics-channel.h b/src/workerd/api/node/diagnostics-channel.h index 77e6bd22284..04a6350b3f2 100644 --- a/src/workerd/api/node/diagnostics-channel.h +++ b/src/workerd/api/node/diagnostics-channel.h @@ -73,9 +73,8 @@ class Channel: public jsg::Object { } }; - // Not GC-visited: jsg::Name's visitForGc is private, so holders cannot - // visit it. The symbol handle (if any) is held as a strong root for the - // Channel's lifetime, which is bounded by the module's channel map. + // Not GC-visited: jsg::Name's visitForGc is private. The symbol handle (if + // any) is a strong root for the Channel's lifetime. jsg::Name name; kj::HashMap, MessageCallback> subscribers; kj::Table> stores; diff --git a/src/workerd/jsg/README.md b/src/workerd/jsg/README.md index beacd80732d..347c4ab82cf 100644 --- a/src/workerd/jsg/README.md +++ b/src/workerd/jsg/README.md @@ -248,13 +248,11 @@ This is intentionally weak and does NOT keep its target alive during GC. Attempting to `visitor.visit()` a weak ref field is a compile error — the correct signal that weak references should not be traced. Do not include them in `visitForGc()`. -`jsg::Name` is also not visitable: its `visitForGc` is private (friend-only), so -holders cannot visit it. A `Name` field's symbol handle is held as a strong root -for the holder's lifetime; a `v8::Symbol` cannot participate in a JS↔C++ -reference cycle, so this cannot leak cycles — only pin the symbol wrapper. +`jsg::Name` is also not visitable (private `visitForGc`): its symbol handle is +a strong root, and a `v8::Symbol` cannot form a JS↔C++ cycle. -`jsg::AsyncGenerator` is likewise not visitable (it has no `visitForGc`, -unlike `jsg::Generator`); a holder keeps its handles as strong roots. +`jsg::AsyncGenerator` is likewise not visitable (no `visitForGc`); holders +keep its handles as strong roots. ## Weak References diff --git a/tools/clang-tidy/visit-for-gc-negative-test.c++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ index 55bf50830f6..0c7f7096d01 100644 --- a/tools/clang-tidy/visit-for-gc-negative-test.c++ +++ b/tools/clang-tidy/visit-for-gc-negative-test.c++ @@ -2,9 +2,8 @@ // Licensed under the Apache 2.0 license found in the LICENSE file or at: // https://opensource.org/licenses/Apache-2.0 -// Negative fixtures for the jsg-visit-for-gc check: nothing below may produce -// a diagnostic. Self-contained stubs mirror the qualified names the check -// keys on. +// Negative fixtures for jsg-visit-for-gc: nothing below may produce a +// diagnostic. namespace workerd::jsg { @@ -16,8 +15,7 @@ class Ref { void visitForGc(GcVisitor& visitor) {} }; -// Mirrors the real jsg::Name: visitForGc is private (friend-only), so no -// holder can visit a Name. +// Mirrors the real jsg::Name: visitForGc is private. class Name { private: void visitForGc(GcVisitor& visitor) {} @@ -40,8 +38,7 @@ class Generator { void visitForGc(GcVisitor& visitor) {} }; -// Mirrors the real jsg::AsyncGenerator: no visitForGc, so holders cannot -// visit one. +// Mirrors the real jsg::AsyncGenerator: no visitForGc. template class AsyncGenerator {}; @@ -103,8 +100,8 @@ struct NestedState { jsg::Ref func; }; -// Case N2: a parent's visitForGc reaches into a directly-held nested struct -// member; the nested struct needs no visitForGc of its own. +// Case N2: a parent's visitForGc may reach into a directly-held nested +// struct member. struct ParentReachesNested: public jsg::Object { NestedState state; @@ -113,26 +110,21 @@ struct ParentReachesNested: public jsg::Object { } }; -// Case N3: a plain standalone holder (no jsg::Object base, no visitForGc, not -// used as a field of any record in this TU) is not demanded against. +// Case N3: a plain standalone holder is not demanded against. struct StandalonePlainHolder { jsg::Ref strongRoot; }; -// Case N4: jsg::Name is not a demanded type. Its visitForGc is private -// (friend-only), so no holder can visit it; the symbol handle is held as a -// strong root for the holder's lifetime, and a v8::Symbol cannot form a -// JS<->C++ cycle, so visitation is never required. +// Case N4: jsg::Name is not demanded; its private visitForGc makes visiting +// impossible, and the symbol handle is a strong root. struct UnvisitedNameField: public jsg::Object { jsg::Name name; void visitForGc(jsg::GcVisitor& visitor) {} }; -// Case N5: KNOWN BLIND SPOT, locked as current behavior. Any MemberExpr -// naming the field inside the visitForGc body counts as a "visit" — including -// a mere comparison. The check does not verify the field is an argument of -// GcVisitor::visit. +// Case N5: KNOWN BLIND SPOT, locked as current behavior: any mention of the +// field inside the body counts as a visit, even a comparison. struct MentionOnlyCountsAsVisit: public jsg::Object { kj::Maybe> mentioned; @@ -153,8 +145,8 @@ struct VisitedResolver: public jsg::Object { } }; -// Case N7: visited Generator, Sequence of non-visitable elements, Sequence -// visited via visitAll, and AsyncGenerator (not visitable, held strong). +// Case N7: visited Generator; Sequence via visitAll; non-visitable element +// Sequence and AsyncGenerator held strong. struct GeneratorAndSequence: public jsg::Object { jsg::Generator gen; jsg::Sequence plainSeq; diff --git a/tools/clang-tidy/visit-for-gc-positive-test.c++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ index c96ddd76fdd..8a5b08c2ec7 100644 --- a/tools/clang-tidy/visit-for-gc-positive-test.c++ +++ b/tools/clang-tidy/visit-for-gc-positive-test.c++ @@ -2,9 +2,8 @@ // Licensed under the Apache 2.0 license found in the LICENSE file or at: // https://opensource.org/licenses/Apache-2.0 -// Positive fixtures for the jsg-visit-for-gc check: every case below must -// produce exactly one diagnostic, and visit-for-gc-test.sh asserts the exact -// total. Self-contained stubs mirror the qualified names the check keys on. +// Positive fixtures for jsg-visit-for-gc: each case must produce exactly one +// diagnostic; visit-for-gc-test.sh asserts the exact total. namespace workerd::jsg { @@ -80,8 +79,8 @@ struct MissedRefField: public jsg::Object { } }; -// Case P2: resource type with no visitForGc of its own; the framework -// dispatches to jsg::Object's empty default and misses the field. +// Case P2: no visitForGc of its own; jsg::Object's empty default misses the +// field. struct NoVisitMethod: public jsg::Object { jsg::Ref orphaned; }; @@ -101,9 +100,7 @@ struct MissedOneOf: public jsg::Object { void visitForGc(jsg::GcVisitor& visitor) {} }; -// Case P6: unvisited jsg::Promise::Resolver field. Resolver has a public -// visitForGc tracing the underlying V8Ref; leaving it unvisited pins the -// promise and its reaction closures. +// Case P6: unvisited jsg::Promise::Resolver field. struct MissedResolver: public jsg::Object { jsg::Promise::Resolver resolver; @@ -117,16 +114,14 @@ struct MissedMaybeResolver: public jsg::Object { void visitForGc(jsg::GcVisitor& visitor) {} }; -// Case P8: unvisited jsg::Generator field (public visitForGc, traces the -// generator's underlying object handle). +// Case P8: unvisited jsg::Generator field. struct MissedGenerator: public jsg::Object { jsg::Generator gen; void visitForGc(jsg::GcVisitor& visitor) {} }; -// Case P9: unvisited jsg::Sequence with a visitable element type (visited via -// GcVisitor::visitAll in real code). +// Case P9: unvisited jsg::Sequence with a visitable element type. struct MissedSequence: public jsg::Object { jsg::Sequence> seq; diff --git a/tools/clang-tidy/visit-for-gc.c++ b/tools/clang-tidy/visit-for-gc.c++ index 0b145e22717..e752eb07225 100644 --- a/tools/clang-tidy/visit-for-gc.c++ +++ b/tools/clang-tidy/visit-for-gc.c++ @@ -35,36 +35,26 @@ bool endsWithQualified(llvm::StringRef qualifiedName, llvm::StringRef suffix) { return qualifiedName[sep - 1] == ':' && qualifiedName[sep - 2] == ':'; } -// Visitable leaf templates: each holds a GC root, has a public visitForGc, -// and must be visited. -// -// jsg::AsyncGenerator is deliberately absent: unlike jsg::Generator it has no -// visitForGc at all, so a holder cannot visit one — its handles are strong -// roots for the holder's lifetime. +// Visitable leaf templates: each holds a GC root and has a public visitForGc. +// jsg::AsyncGenerator is absent: it has no visitForGc, so holders cannot +// visit one; its handles are strong roots. const llvm::StringRef kVisitableLeafTemplates[] = { "jsg::Ref", "jsg::V8Ref", "jsg::JsRef", "jsg::Function", "jsg::Promise", "jsg::HashableV8Ref", "jsg::MemoizedIdentity", "jsg::Generator", }; -// Non-template visitable leaf types. -// -// jsg::Name is deliberately absent: its visitForGc is private (friend-only), -// so GcVisitor::visit(name) does not compile and no holder can satisfy a -// demand to visit it. A Name field's symbol handle is simply held as a strong -// root for the holder's lifetime; a v8::Symbol holds only its description and -// cannot participate in a JS<->C++ reference cycle, so visitation is never -// required for collectability either. +// Non-template visitable leaf types. jsg::Name is deliberately absent: its +// visitForGc is private (friend-only), so no holder can visit one; the symbol +// handle is a strong root, and a v8::Symbol cannot form a JS<->C++ cycle, so +// visitation is never required. const llvm::StringRef kVisitableLeafTypes[] = { "jsg::Value", "jsg::Data", }; -// jsg::Promise::Resolver has a public visitForGc (it traces the underlying -// V8Ref). It is a non-template class nested inside the -// Promise template, so its printed qualified name embeds the specialization -// arguments ("jsg::Promise::Resolver"); match the parent's template -// name instead of suffix-matching the full string. +// jsg::Promise::Resolver's printed qualified name embeds the +// specialization arguments, so match the parent record's template instead. bool isPromiseResolver(const clang::CXXRecordDecl *rd) { if (rd->getName() != "Resolver") return false; const auto *parent = llvm::dyn_cast(rd->getDeclContext()); @@ -84,9 +74,8 @@ bool isPromiseResolver(const clang::CXXRecordDecl *rd) { // (variants) are visitable if any element type is. enum class ContainerKind { None, FirstArg, AnyArg }; -// jsg::Sequence is a kj::Array subclass with no visitForGc of its own; -// like the other containers it is visitable iff its element type is (via -// GcVisitor::visitAll). +// jsg::Sequence is a kj::Array subclass without its own visitForGc: visitable +// iff its element type is (via visitAll). const llvm::StringRef kFirstArgContainers[] = { "kj::Maybe", "kj::Array", "kj::Vector", "jsg::Optional", "jsg::LenientOptional", "jsg::Sequence", diff --git a/tools/clang-tidy/visit-for-gc.h b/tools/clang-tidy/visit-for-gc.h index 5b27dce5ec7..65897921db0 100644 --- a/tools/clang-tidy/visit-for-gc.h +++ b/tools/clang-tidy/visit-for-gc.h @@ -14,12 +14,23 @@ namespace clang_tidy { // Clang-tidy check that validates JSG resource types correctly visit their // GC roots. Flags fields of visitable types (jsg::Ref, jsg::V8Ref, jsg::JsRef, -// jsg::Function, jsg::Promise, jsg::Value, etc., plus -// kj::Maybe/Array/Vector/OneOf and jsg::Optional wrappers thereof) that are -// not visited in the class's visitForGc() method. +// jsg::Function, jsg::Promise, jsg::Promise::Resolver, jsg::Generator, +// jsg::Value, etc., plus kj::Maybe/Array/Vector/OneOf, jsg::Optional, and +// jsg::Sequence wrappers thereof) that are not visited in the class's +// visitForGc() method. // -// This check helps prevent GC-related bugs where JavaScript objects are -// prematurely collected because the C++ side failed to mark them as reachable. +// Model: an unvisited handle is a strong root — bounded retention, never +// use-after-free. Visiting enables JS<->C++ cycle collection but is only safe +// when the holder is re-traversed every GC cycle from a live wrapper, a +// traversal property this per-record check cannot decide. It therefore only +// demands visits where the framework guarantees traversal, and never forbids +// one. Never fix a diagnostic by blindly adding a visit; deliberate strong +// roots get NOLINT(jsg-visit-for-gc) plus a reason. +// +// Deliberate limitations: any mention of a field in the body counts as a +// visit (accepts the KJ_IF_SOME/KJ_SWITCH_ONEOF binding idiom); templated +// bodies are only checked where instantiated; plain holders without +// visitForGc are only diagnosed when a visible body reaches into them. class VisitForGcCheck : public clang::tidy::ClangTidyCheck { public: VisitForGcCheck(clang::StringRef Name, clang::tidy::ClangTidyContext *Context)