fix(gc): a Symbol-typed local is a GC-heap reference, and gets a shadow slot (#7236)

.github/workflows/gc-root-dominance.yml

@@ -28,9 +28,17 @@ name: GC Root Dominance
 #      corollary in CLAUDE.md warns about.
 #
 #      **ACTION REQUIRED, and it is not something this workflow can do to
-#      itself**: with the allowlist below the job is green on `main`, so a
-#      repo admin must add `gc-root-dominance` to branch protection's required
-#      contexts. Until that happens this file is documentation, not a gate.
+#      itself**: a repo admin must add `gc-root-dominance` to branch
+#      protection's required contexts. Until that happens this file is
+#      documentation, not a gate.
+#
+#      Both conditions #7198 named are met as of #7236: the dominance check is
+#      green with an EMPTY allowlist, and `--unrooted-allocas --moving-only` --
+#      which was 98 before #7235 and 2 after -- now reads 0 and is a step
+#      below. Promote after this job's first green run on `main` WITH that
+#      step, not before: a gate that has never been green in its current shape
+#      blocks every open PR the day it becomes required, which is the corollary
+#      that produced this whole paragraph.
 #      See docs/src/internals/gc-rooting-invariant.md, "Promoting this gate".
 #   3. `concurrency` cancels pull-request runs only, never `main` runs;
 #   4. the subject is ASSERTED live, not assumed, at three levels.
@@ -181,6 +189,38 @@ jobs:
             --seeded-violations 40 \
             -v
 
+      # ★ The OTHER mode, and until #7236 it had never been at zero.
+      #
+      # `--unrooted-allocas` asks a different question from the step above: not
+      # "is the root store late" but "is there a root store AT ALL". #7235 split
+      # its heap-source predicate by movability and got the count from 98 to 2,
+      # and both residuals were one bug -- `Type::Symbol` classified as an
+      # immediate, so a `Symbol` local got no shadow slot. That is now fixed and
+      # the corpus reads 0, which is the condition #7198 named for promoting
+      # this job to a required context.
+      #
+      # A number that is the acceptance criterion for a promotion and is checked
+      # by nothing will regress silently -- CLAUDE.md hazard 4, applied to the
+      # measurement rather than to the job. So it is a step. It shares the
+      # corpus, the exemptions and the allowlist file with the step above, and
+      # carries the same liveness floors: "0 violations" over a corpus that did
+      # not compile means nothing.
+      #
+      # If this arm ever goes red for a hazard that is real but deliberately
+      # deferred, the answer is an entry in gc_root_dominance_allowlist.json
+      # with an issue number, NOT deleting the step -- rule 1 in that file
+      # (an entry matching nothing fails the build) is what makes the tolerance
+      # temporary.
+      - name: Check that every GC value in an alloca has a root store
+        run: |
+          set -euo pipefail
+          python3 scripts/gc_root_dominance_check.py ir-corpus \
+            --unrooted-allocas \
+            --moving-only \
+            --min-files 90 --min-binds 1500 --min-funcs 1200 \
+            --allowlist scripts/gc_root_dominance_allowlist.json \
+            -v
+
       - name: Upload the IR corpus on failure
         if: failure()
         uses: actions/upload-artifact@v7

changelog.d/7243-symbol-local-shadow-slot.md

@@ -0,0 +1,59 @@
+Fixed `collectors/pointer_locals.rs` classifying **`Type::Symbol` as a
+non-pointer**, so a `Symbol`-typed local got no shadow-stack slot and lived in a
+plain `alloca` — invisible to the precise root walk (#7236). `alloc_symbol` is
+`gc_malloc(size_of::<SymbolHeader>(), GC_TYPE_STRING)` and `js_symbol_new`
+returns it `POINTER_TAG`-boxed; nothing else holds a fresh symbol, so the malloc
+sweep inside the copying minor frees one that is still live.
+
+**The drift had already happened, one variant wide, inside one crate.**
+`typed_shape::type_is_pointer_bearing` — an exhaustive match, and the function
+that lays out the GC's own inline-field pointer masks — already answered `true`
+for `Symbol` while `is_definitely_non_pointer_type` answered "non-pointer", so a
+`Symbol` local was simultaneously not-a-pointer (no slot) and pointer-bearing
+(scanned as one). That is verbatim what the doc comment over
+`is_definitely_non_pointer_type` predicted would cost a use-after-move. There
+was a third copy in `expr/shadow_slot.rs` carrying the same entry — a separate
+hazard, since a `true` there suppresses `temp_root`'s protection of a symbol
+operand held across an allocating call — and a fourth in
+`lower_call/closure_analysis.rs` that correctly did not. All three now route to
+one exhaustive definition, so a new `Type` variant is a compile error rather
+than a silent "not a pointer". The full 21-variant audit found `Symbol` to be
+the only misclassification; the other six non-pointers are NaN-boxed immediates
+with no allocator.
+
+**The failure is a premature FREE, not a stale address**, and the correction
+matters because it dictates the witness. `gc_malloc` is the SYSTEM allocator
+with a `GcHeader` in front, not an arena allocation, so the copying minor cannot
+relocate a symbol — under #7235's taxonomy a `Symbol` local is RECLAIMABLE and
+not MOVABLE (#7230's class, not #7019's). `SYMBOL_POINTERS` does not save it
+either: `scan_symbol_pointer_metadata_roots_mut` uses
+`visit_metadata_usize_slot`, which rewrites without marking, exactly as
+`alloc_symbol`'s own comment says ("kept alive … or **not at all**"). Two
+consequences are baked into the new witness: the pressure must be **symbols**,
+because `copied_minor_malloc_sweep_due` gates the malloc sweep on a
+`MallocCount` trigger while object churn reaches the *arena* trigger (object
+churn reproduced 1 failure in 80 probes, and 0 on four of five repeats of
+another shape); and identity probes report nothing, because `js_symbol_equals`
+falls back to comparing `id` off the freed header and `js_is_symbol` to reading
+`magic`.
+
+New witness `test-files/test_gap_gc_symbol_local_rooting.ts`, registered in
+`test-parity/gc_repsel_corpus.txt`: `A 30 B 20` at `f8f1e7188` on the
+`loop_polls` arm (3/3 identical) **and on the shipped default** (2/2), `A 0 B 0`
+after, byte-exact against node 26.5.1.
+
+`gc_root_dominance_check.py --unrooted-allocas --moving-only` over a freshly
+generated 118-source / 136-module corpus: **4 → 0** (the two #7236 hits in
+`test_gap_class_forward_capture_6523` plus the new witness's two). That was 98
+before #7235 and 2 after, and 0 is the condition #7198 named for promoting
+`gc-root-dominance` to a required context. **The mode is now a step in that
+job** rather than a number living in issue bodies — verified able to fail (exit
+1 at `f8f1e7188`, exit 0 after) and sharing the corpus, exemptions, allowlist
+and liveness floors with the existing dominance step. The promotion itself
+remains a repo-admin action; `docs/src/internals/gc-rooting-invariant.md` now
+carries the exact steps.
+
+The representation-selection promotion census
+(`compiler_output_regression.py census --gate`) is **byte-identical** across all
+18 workloads before and after, so no promotion floor moved and no ratchet was
+taken.

crates/perry-codegen/src/collectors/pointer_locals.rs

@@ -184,26 +184,27 @@ impl HirTypeFacts for PointerAnalysisFacts<'_> {
 /// Types that can NEVER hold a heap pointer, and therefore cost a local its
 /// shadow-stack slot in [`collect_pointer_typed_locals`].
 ///
-/// **This is the single definition.** It used to be a nested `fn` inside
-/// `collect_pointer_typed_locals`; it is module-level and `pub(crate)` because
-/// anything that decides a value may be treated as a rooted pointer has to
-/// agree with the pass that actually assigns the root slot. A second copy
-/// drifting by one `Type` variant would mean a value this collector left
-/// unrooted while another pass treated it as a live, movable pointer — a
-/// use-after-move under the evacuating minor (#7019), not a cosmetic
-/// inconsistency. `collectors/ptr_shape_returns.rs` (#7034 §4) is the current
-/// second caller.
+/// **Exactly the negation of [`crate::typed_shape::type_is_pointer_bearing`],
+/// by construction rather than by review.** This used to be a second hand-
+/// maintained `matches!` list, and it had already drifted from that one by a
+/// single variant: `Type::Symbol`. `typed_shape` said pointer (correctly —
+/// `js_symbol_new` NaN-boxes a `gc_malloc`'d `SymbolHeader` that a malloc
+/// sweep inside the copying minor frees when nothing marks it), this said
+/// non-pointer, so a `Symbol`-typed local got no shadow-stack slot and sat in a
+/// plain `alloca` across every collection point in its scope (#7236). That is
+/// precisely the failure the previous comment here predicted: "a second copy
+/// drifting by one `Type` variant … a use-after-move under the evacuating
+/// minor (#7019), not a cosmetic inconsistency." The realised failure turned
+/// out to be the sibling one — a premature free rather than a stale address,
+/// because `gc_malloc` is outside the arena — which is the distinction #7235
+/// drew between RECLAIMABLE and MOVABLE and the reason it drew it.
+///
+/// So there is now one copy, it is exhaustive, and a new `Type` variant is a
+/// compile error over there instead of a silent "not a pointer" here.
+/// `collectors/ptr_shape_returns.rs` (#7034 §4) is the second caller of this
+/// negation and inherits the answer.
 pub(crate) fn is_definitely_non_pointer_type(ty: &Type) -> bool {
-    matches!(
-        ty,
-        Type::Number
-            | Type::Int32
-            | Type::Boolean
-            | Type::Null
-            | Type::Void
-            | Type::Never
-            | Type::Symbol
-    ) || matches!(ty, Type::Union(variants) if variants.iter().all(is_definitely_non_pointer_type))
+    !crate::typed_shape::type_is_pointer_bearing(ty)
 }
 
 pub fn collect_pointer_typed_locals(
@@ -212,39 +213,18 @@ pub fn collect_pointer_typed_locals(
     flat_const_ids: &HashSet<u32>,
 ) -> std::collections::HashMap<u32, u32> {
     use perry_hir::Stmt;
+    /// Does this local need a shadow-stack slot?
+    ///
+    /// The third copy of the pointer question, now routed to the one
+    /// definition (#7236). It was the exact complement of
+    /// [`is_definitely_non_pointer_type`] over every `Type` variant *except*
+    /// `Symbol`, which neither of them claimed — so a `Symbol` local was
+    /// simultaneously "not a pointer" (no slot here) and "pointer-bearing"
+    /// (`typed_shape`, which lays out the GC's own field masks). The
+    /// per-variant rationale that used to live here moved to
+    /// `type_is_pointer_bearing`'s doc comment with it.
     fn is_ptr_typed(ty: &Type) -> bool {
-        matches!(
-            ty,
-            Type::String
-                // A string-LITERAL type (`"foo"`, or a `"a" | "b"` discriminant
-                // union member) is a heap String at runtime — it needs a root
-                // slot exactly like `Type::String`, or the moving-GC precise scan
-                // reaps a live string → silent corruption.
-                | Type::StringLiteral(_)
-                | Type::Array(_)
-                | Type::Tuple(_)
-                | Type::Object(_)
-                | Type::Named(_)
-                // An unresolved generic type parameter (`T`) can bind to any
-                // heap type; treat it as a pointer (fail-safe).
-                | Type::TypeVar(_)
-                | Type::Promise(_)
-                | Type::Function(_)
-                // A generic instantiation (`Map<K,V>`, `Set<T>`, `WeakMap`,
-                // `Box<T>`, `Array<T>`, a user generic class, …) is always a
-                // heap-reference type. Without this, a `Map`/`Set`-typed local
-                // got NO shadow-stack slot, so the PRECISE moving-GC root scan
-                // never saw it — the object was reaped as dead while still live
-                // (crash: "grown Map must retain its side-allocation owner
-                // record"). The non-moving default GC hid this via its
-                // conservative C-stack scan. Treating a rare non-pointer generic
-                // value as a root is harmless: the GC decode rejects any slot
-                // value that isn't a live heap pointer.
-                | Type::Generic { .. }
-                | Type::BigInt
-                | Type::Any
-                | Type::Unknown
-        ) || matches!(ty, Type::Union(variants) if variants.iter().any(is_ptr_typed))
+        crate::typed_shape::type_is_pointer_bearing(ty)
     }
 
     fn expr_value_type(
@@ -1165,4 +1145,124 @@ mod tests {
         assert!(!slots.contains_key(&1));
         assert!(slots.contains_key(&2));
     }
+
+    /// Every `Type` variant, with the answer pinned.
+    ///
+    /// #7236: `is_definitely_non_pointer_type` and `typed_shape`'s
+    /// `type_is_pointer_bearing` are two names for one question, and they had
+    /// drifted by exactly one variant — `Symbol` — which is how a
+    /// `POINTER_TAG`-boxed, `gc_malloc`'d, collector-freed object came to be
+    /// classified as an immediate. They are now the same function, so this test
+    /// is about
+    /// the ANSWERS: it enumerates the enum and fails if any classification
+    /// flips. `type_is_pointer_bearing`'s exhaustive `match` covers the other
+    /// half (a new variant is a compile error, not a silent "non-pointer").
+    #[test]
+    fn every_type_variant_has_its_pointer_classification_pinned() {
+        // The six immediates: a raw f64, INT32_TAG, TAG_TRUE/TAG_FALSE,
+        // TAG_NULL, TAG_UNDEFINED, and the uninhabited type. No allocator
+        // produces any of them, so there is nothing for the collector to see.
+        let non_pointers = [
+            Type::Number,
+            Type::Int32,
+            Type::Boolean,
+            Type::Null,
+            Type::Void,
+            Type::Never,
+        ];
+        // Everything else is, or can be, a heap reference. `Symbol` is in this
+        // list and not the one above: that IS #7236.
+        let pointers = [
+            Type::Symbol,
+            Type::String,
+            Type::StringLiteral("foo".to_string()),
+            Type::BigInt,
+            Type::Array(Box::new(Type::Number)),
+            Type::Tuple(vec![Type::Number]),
+            Type::Object(Default::default()),
+            Type::Function(FunctionType {
+                params: Vec::new(),
+                return_type: Box::new(Type::Any),
+                is_async: false,
+                is_generator: false,
+            }),
+            Type::Promise(Box::new(Type::Number)),
+            Type::Named("C".to_string()),
+            Type::Generic {
+                base: "Map".to_string(),
+                type_args: vec![Type::String, Type::Number],
+            },
+            Type::TypeVar("T".to_string()),
+            Type::Any,
+            Type::Unknown,
+        ];
+        for ty in &non_pointers {
+            assert!(
+                is_definitely_non_pointer_type(ty),
+                "{ty:?} must be a non-pointer"
+            );
+            assert!(!crate::typed_shape::type_is_pointer_bearing(ty));
+        }
+        for ty in &pointers {
+            assert!(
+                !is_definitely_non_pointer_type(ty),
+                "{ty:?} must be pointer-possible — a local of this type needs a \
+                 shadow-stack slot or the precise moving-GC root scan cannot see it"
+            );
+            assert!(crate::typed_shape::type_is_pointer_bearing(ty));
+        }
+        // A union is a non-pointer only when EVERY member is.
+        assert!(is_definitely_non_pointer_type(&Type::Union(vec![
+            Type::Number,
+            Type::Boolean
+        ])));
+        assert!(!is_definitely_non_pointer_type(&Type::Union(vec![
+            Type::Number,
+            Type::Symbol
+        ])));
+    }
+
+    /// #7236's reproducer, at the collector: `const s = Symbol("x")` must get a
+    /// slot. `alloc_symbol` is `gc_malloc(_, GC_TYPE_STRING)`, and a fresh
+    /// symbol is reachable from nothing else (`SYMBOL_POINTERS` is visited
+    /// metadata-only), so without a slot the local sits in a plain `alloca`
+    /// across every collection point in its scope and the malloc sweep inside
+    /// the copying minor frees it while it is live — exactly what
+    /// `gc_root_dominance_check.py --unrooted-allocas --moving-only` reported
+    /// twice on `test_gap_class_forward_capture_6523`.
+    #[test]
+    fn a_symbol_typed_local_gets_a_shadow_slot() {
+        let stmts = vec![Stmt::Let {
+            id: 1,
+            name: "s".to_string(),
+            ty: Type::Symbol,
+            mutable: false,
+            init: Some(Expr::SymbolNew(Some(Box::new(Expr::String(
+                "x".to_string(),
+            ))))),
+        }];
+        let slots = collect_pointer_typed_locals(&[], &stmts, &HashSet::new());
+        assert!(slots.contains_key(&1), "Symbol local must be shadow-rooted");
+    }
+
+    /// The other half, and the sharper one: the declared type is `any`, so the
+    /// slot survives only if the write-refinement fixpoint ALSO agrees that
+    /// `Symbol` is a pointer. `is_definitely_non_pointer_type` is what that
+    /// loop consults (`all_non_pointer`), so a fix applied to `is_ptr_typed`
+    /// alone would hand out the slot and then take it away again.
+    #[test]
+    fn an_inferred_symbol_local_keeps_its_shadow_slot() {
+        let stmts = vec![Stmt::Let {
+            id: 1,
+            name: "s".to_string(),
+            ty: Type::Any,
+            mutable: false,
+            init: Some(Expr::SymbolNew(None)),
+        }];
+        let slots = collect_pointer_typed_locals(&[], &stmts, &HashSet::new());
+        assert!(
+            slots.contains_key(&1),
+            "a local refined to Symbol must keep its shadow slot"
+        );
+    }
 }

crates/perry-codegen/src/expr/shadow_slot.rs

@@ -44,19 +44,28 @@ pub(crate) fn expr_is_known_non_pointer_shadow_value(ctx: &FnCtx<'_>, expr: &Exp
         Expr::LocalGet(id) => {
             // A reserved shadow slot means the local is pointer-possible even
             // if its initializer refined `local_types` to a scalar.
+            //
+            // #7236: this list also carried `HirType::Symbol`, and it is a
+            // SEPARATE hazard from the missing shadow slot — a `true` here
+            // suppresses `temp_root`'s argument/operand rooting, so
+            // `obj[s] = alloc()` with `s: symbol` left the movable symbol
+            // unrooted across the allocating call even once the local itself
+            // had a slot. `lower_call/closure_analysis.rs`'s
+            // `local_is_inert_primitive` never listed `Symbol`; this copy and
+            // `collectors/pointer_locals.rs` were the two that did.
+            //
+            // Derived from the one definition rather than restated, but kept
+            // NON-`Union` on purpose: `type_is_pointer_bearing` answers `false`
+            // for an all-scalar union (`number | undefined`), which would
+            // WIDEN this suppression to locals it never covered. That is a
+            // plausible optimisation and an unmeasured one; it is not this
+            // change. The guard makes the arm exactly the old list minus
+            // `Symbol`.
             !ctx.shadow_slot_map.contains_key(id)
-                && matches!(
-                    ctx.local_types.get(id),
-                    Some(
-                        HirType::Number
-                            | HirType::Int32
-                            | HirType::Boolean
-                            | HirType::Null
-                            | HirType::Void
-                            | HirType::Never
-                            | HirType::Symbol
-                    )
-                )
+                && ctx.local_types.get(id).is_some_and(|ty| {
+                    !matches!(ty, HirType::Union(_))
+                        && !crate::typed_shape::type_is_pointer_bearing(ty)
+                })
         }
         Expr::Compare { .. } | Expr::Void(_) => true,
         Expr::Unary { .. } => true,