Correct the jsg-visit-for-gc check's type lists and document its model

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
 

src/workerd/api/node/diagnostics-channel.h

@@ -73,10 +73,9 @@ 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. The symbol handle (if
+  // any) is a strong root for the Channel's lifetime.
+  jsg::Name name;
   kj::HashMap<jsg::HashableV8Ref<v8::Object>, MessageCallback> subscribers;
   kj::Table<StoreEntry, kj::HashIndex<StoreCallbacks>> stores;
 

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<T>`, `V8Ref<T>`,
-   `JsRef<T>`, `Function<T>`, `Promise<T>`, `Promise<T>::Resolver`, or
-   `Name` — see `README.md` §GC-Visitable Types for the complete list
+   `JsRef<T>`, `Function<T>`, `Promise<T>`, or `Promise<T>::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<T>` or `JsValue` types as class members** — use `V8Ref<T>`
    or `JsRef<T>` for persistence

src/workerd/jsg/README.md

@@ -235,13 +235,11 @@ All types that must be visited in `visitForGc()` if held as Resource Type member
 | `jsg::JsRef<T>`             | Persistent ref to JsValue type   |
 | `jsg::Optional<T>`          | When `T` is GC-visitable         |
 | `jsg::LenientOptional<T>`   | When `T` is GC-visitable         |
-| `jsg::Name`                 | Property name (string or symbol) |
 | `jsg::Function<Sig>`        | Wrapped JS/C++ function          |
 | `jsg::Promise<T>`           | JS promise wrapper               |
 | `jsg::Promise<T>::Resolver` | Promise resolver                 |
-| `jsg::Sequence<T>`          | Iterable sequence                |
+| `jsg::Sequence<T>`          | When `T` is GC-visitable (use `visitor.visitAll`) |
 | `jsg::Generator<T>`         | Sync generator                   |
-| `jsg::AsyncGenerator<T>`    | Async generator                  |
 | `kj::Maybe<T>`              | When `T` is GC-visitable         |
 
 **Not GC-visitable** (compile error if visited):
@@ -250,6 +248,12 @@ 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 (private `visitForGc`): its symbol handle is
+a strong root, and a `v8::Symbol` cannot form a JS↔C++ cycle.
+
+`jsg::AsyncGenerator<T>` is likewise not visitable (no `visitForGc`); holders
+keep its handles as strong roots.
+
 ## Weak References
 
 `jsg::WeakRef<T>` provides a non-owning, automatically-invalidated reference

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

tools/clang-tidy/visit-for-gc-negative-test.c++

@@ -0,0 +1,160 @@
+// 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 jsg-visit-for-gc: nothing below may produce a
+// diagnostic.
+
+namespace workerd::jsg {
+
+class GcVisitor;
+
+template <typename T>
+class Ref {
+ public:
+  void visitForGc(GcVisitor& visitor) {}
+};
+
+// Mirrors the real jsg::Name: visitForGc is private.
+class Name {
+ private:
+  void visitForGc(GcVisitor& visitor) {}
+};
+
+template <typename T>
+class Promise {
+ public:
+  class Resolver {
+   public:
+    void visitForGc(GcVisitor& visitor) {}
+  };
+
+  void visitForGc(GcVisitor& visitor) {}
+};
+
+template <typename T>
+class Generator {
+ public:
+  void visitForGc(GcVisitor& visitor) {}
+};
+
+// Mirrors the real jsg::AsyncGenerator: no visitForGc.
+template <typename T>
+class AsyncGenerator {};
+
+template <typename T>
+class Sequence {};
+
+class GcVisitor {
+ public:
+  template <typename... Args>
+  void visit(Args&&... args) {}
+  template <typename T>
+  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<FrameworkInternal> unvisited;
+};
+
+}  // namespace workerd::jsg
+
+namespace kj {
+
+template <typename T>
+class Maybe {
+ public:
+  bool operator==(decltype(nullptr)) const {
+    return true;
+  }
+};
+
+template <typename... T>
+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<Widget> ref;
+  kj::Maybe<jsg::Ref<Widget>> maybeRef;
+  kj::OneOf<int, jsg::Ref<Widget>> stateful;
+
+  void visitForGc(jsg::GcVisitor& visitor) {
+    visitor.visit(ref, maybeRef, stateful);
+  }
+};
+
+struct NestedState {
+  jsg::Ref<Widget> func;
+};
+
+// Case N2: a parent's visitForGc may reach into a directly-held nested
+// struct member.
+struct ParentReachesNested: public jsg::Object {
+  NestedState state;
+
+  void visitForGc(jsg::GcVisitor& visitor) {
+    visitor.visit(state.func);
+  }
+};
+
+// Case N3: a plain standalone holder is not demanded against.
+struct StandalonePlainHolder {
+  jsg::Ref<Widget> strongRoot;
+};
+
+// 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 mention of the
+// field inside the body counts as a visit, even a comparison.
+struct MentionOnlyCountsAsVisit: public jsg::Object {
+  kj::Maybe<jsg::Ref<Widget>> mentioned;
+
+  void visitForGc(jsg::GcVisitor& visitor) {
+    if (mentioned == nullptr) {
+      return;
+    }
+  }
+};
+
+// Case N6: visited resolver fields are accepted.
+struct VisitedResolver: public jsg::Object {
+  jsg::Promise<int>::Resolver resolver;
+  kj::Maybe<jsg::Promise<int>::Resolver> maybeResolver;
+
+  void visitForGc(jsg::GcVisitor& visitor) {
+    visitor.visit(resolver, maybeResolver);
+  }
+};
+
+// Case N7: visited Generator; Sequence via visitAll; non-visitable element
+// Sequence and AsyncGenerator held strong.
+struct GeneratorAndSequence: public jsg::Object {
+  jsg::Generator<int> gen;
+  jsg::Sequence<int> plainSeq;
+  jsg::Sequence<jsg::Ref<Widget>> refSeq;
+  jsg::AsyncGenerator<int> asyncGen;
+
+  void visitForGc(jsg::GcVisitor& visitor) {
+    visitor.visit(gen);
+    visitor.visitAll(refSeq);
+  }
+};

tools/clang-tidy/visit-for-gc-positive-test.c++

@@ -0,0 +1,129 @@
+// 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 jsg-visit-for-gc: each case must produce exactly one
+// diagnostic; visit-for-gc-test.sh asserts the exact total.
+
+namespace workerd::jsg {
+
+class GcVisitor;
+
+template <typename T>
+class Ref {
+ public:
+  void visitForGc(GcVisitor& visitor) {}
+};
+
+template <typename T>
+class Promise {
+ public:
+  class Resolver {
+   public:
+    void visitForGc(GcVisitor& visitor) {}
+  };
+
+  void visitForGc(GcVisitor& visitor) {}
+};
+
+template <typename T>
+class Generator {
+ public:
+  void visitForGc(GcVisitor& visitor) {}
+};
+
+template <typename T>
+class Sequence {};
+
+class GcVisitor {
+ public:
+  template <typename... Args>
+  void visit(Args&&... args) {}
+  template <typename T>
+  void visitAll(T& collection) {}
+};
+
+class Object {
+ public:
+  void visitForGc(GcVisitor& visitor) {}
+};
+
+}  // namespace workerd::jsg
+
+namespace kj {
+
+template <typename T>
+class Maybe {
+ public:
+  bool operator==(decltype(nullptr)) const {
+    return true;
+  }
+};
+
+template <typename... T>
+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<Widget> visited;
+  jsg::Ref<Widget> missed;
+
+  void visitForGc(jsg::GcVisitor& visitor) {
+    visitor.visit(visited);
+  }
+};
+
+// Case P2: no visitForGc of its own; jsg::Object's empty default misses the
+// field.
+struct NoVisitMethod: public jsg::Object {
+  jsg::Ref<Widget> orphaned;
+};
+
+// Case P4: unvisited kj::Maybe<jsg::Ref<T>> field (FirstArg container).
+struct MissedMaybeRef: public jsg::Object {
+  kj::Maybe<jsg::Ref<Widget>> maybeRef;
+
+  void visitForGc(jsg::GcVisitor& visitor) {}
+};
+
+// Case P5: unvisited kj::OneOf with a visitable alternative (AnyArg
+// container).
+struct MissedOneOf: public jsg::Object {
+  kj::OneOf<int, jsg::Ref<Widget>> stateful;
+
+  void visitForGc(jsg::GcVisitor& visitor) {}
+};
+
+// Case P6: unvisited jsg::Promise<T>::Resolver field.
+struct MissedResolver: public jsg::Object {
+  jsg::Promise<int>::Resolver resolver;
+
+  void visitForGc(jsg::GcVisitor& visitor) {}
+};
+
+// Case P7: unvisited kj::Maybe<jsg::Promise<T>::Resolver> field.
+struct MissedMaybeResolver: public jsg::Object {
+  kj::Maybe<jsg::Promise<int>::Resolver> maybeResolver;
+
+  void visitForGc(jsg::GcVisitor& visitor) {}
+};
+
+// Case P8: unvisited jsg::Generator<T> field.
+struct MissedGenerator: public jsg::Object {
+  jsg::Generator<int> gen;
+
+  void visitForGc(jsg::GcVisitor& visitor) {}
+};
+
+// Case P9: unvisited jsg::Sequence with a visitable element type.
+struct MissedSequence: public jsg::Object {
+  jsg::Sequence<jsg::Ref<Widget>> seq;
+
+  void visitForGc(jsg::GcVisitor& visitor) {}
+};