Rework names scope

bff-cli/src/info.rs

@@ -24,7 +24,7 @@ pub fn info(
         let mut writer = BufWriter::new(f);
         let graph = bigfile.reference_graph();
         let dot = Dot::with_config(&graph, &[Config::EdgeNoLabel]);
-        write!(&mut writer, "{:?}", dot)?;
+        name_context.scope(|| write!(&mut writer, "{:?}", dot))?;
     }
 
     Ok(())

bff/src/names/context.rs

@@ -1,65 +1,16 @@
-use std::cell::RefCell;
 use std::collections::HashMap;
 use std::fmt::Write as _;
 use std::io::{BufRead, Write};
 
 use encoding_rs::WINDOWS_1252;
 
-use super::{ALL_NAME_STYLES, Name, NameType, apply_name_style, hash_string_for_type};
+use super::{ALL_NAME_STYLES, Name, NameType, apply_name_style, hash_string_for_type, scope};
 use crate::BffResult;
 use crate::class::class_base_names;
 use crate::error::{InvalidNameDecodingError, InvalidNameEncodingError};
 
 const DEFAULT_NAME_STRING: &str = "";
 
-thread_local! {
-    static ACTIVE_NAME_CONTEXT_STACK: RefCell<Vec<*const NameContext>> = const { RefCell::new(Vec::new()) };
-    static ACTIVE_MUT_NAME_CONTEXT_STACK: RefCell<Vec<*mut NameContext>> = const { RefCell::new(Vec::new()) };
-}
-
-struct NameContextScopeGuard {
-    is_mut: bool,
-}
-
-impl Drop for NameContextScopeGuard {
-    fn drop(&mut self) {
-        if self.is_mut {
-            ACTIVE_MUT_NAME_CONTEXT_STACK.with(|stack| {
-                stack.borrow_mut().pop();
-            });
-        }
-        ACTIVE_NAME_CONTEXT_STACK.with(|stack| {
-            stack.borrow_mut().pop();
-        });
-    }
-}
-
-pub(super) fn with_name_context<R>(f: impl FnOnce(Option<&NameContext>) -> R) -> R {
-    ACTIVE_NAME_CONTEXT_STACK.with(|stack| {
-        let context = stack.borrow().last().copied().map(|ptr| {
-            // SAFETY: Pointers are pushed only from `NameContext::scope` and popped by
-            // `NameContextScopeGuard`, so they are valid for the duration of the scope.
-            unsafe { &*ptr }
-        });
-        f(context)
-    })
-}
-
-pub(super) fn with_name_context_mut<R>(f: impl FnOnce(Option<&mut NameContext>) -> R) -> R {
-    ACTIVE_MUT_NAME_CONTEXT_STACK.with(|stack| {
-        let context = stack.borrow().last().copied().map(|ptr| {
-            // SAFETY: Pointers are pushed only from `NameContext::scope_mut` and popped by
-            // `NameContextScopeGuard`, so they are valid for the duration of the scope.
-            unsafe { &mut *ptr }
-        });
-        f(context)
-    })
-}
-
-pub(crate) fn current_name_type() -> Option<NameType> {
-    with_name_context(|name_context| name_context.map(NameContext::name_type))
-}
-
 pub(super) type NameMap = HashMap<Name, String>;
 
 fn insert_name(names: &mut NameMap, name_type: NameType, string: &str) -> Name {
@@ -201,22 +152,11 @@ impl NameContext {
     }
 
     pub fn scope<R>(&self, f: impl FnOnce() -> R) -> R {
-        ACTIVE_NAME_CONTEXT_STACK.with(|stack| {
-            stack.borrow_mut().push(self as *const Self);
-        });
-        let _guard = NameContextScopeGuard { is_mut: false };
-        f()
+        scope::scope(self, f)
     }
 
     pub fn scope_mut<R>(&mut self, f: impl FnOnce() -> R) -> R {
-        ACTIVE_NAME_CONTEXT_STACK.with(|stack| {
-            stack.borrow_mut().push(self as *const Self);
-        });
-        ACTIVE_MUT_NAME_CONTEXT_STACK.with(|stack| {
-            stack.borrow_mut().push(self as *mut Self);
-        });
-        let _guard = NameContextScopeGuard { is_mut: true };
-        f()
+        scope::scope_mut(self, f)
     }
 
     pub fn name_type(&self) -> NameType {

bff/src/names/mod.rs

@@ -1,5 +1,6 @@
 pub mod context;
 pub mod json;
+mod scope;
 pub mod serde_schema;
 pub mod value;
 pub mod wordlist;

bff/src/names/scope.rs

@@ -0,0 +1,302 @@
+use std::cell::{Cell, RefCell};
+use std::ptr::NonNull;
+
+use super::{NameContext, NameType};
+
+#[derive(Copy, Clone, PartialEq, Eq)]
+enum BorrowState {
+    Idle,
+    Shared,
+    Exclusive,
+}
+
+enum Slot {
+    Shared(NonNull<NameContext>),
+    Exclusive {
+        ptr: NonNull<NameContext>,
+        borrow: Cell<BorrowState>,
+    },
+}
+
+thread_local! {
+    static STACK: RefCell<Vec<Slot>> = const { RefCell::new(Vec::new()) };
+}
+
+struct Guard;
+
+impl Drop for Guard {
+    fn drop(&mut self) {
+        STACK.with(|s| {
+            s.borrow_mut().pop();
+        });
+    }
+}
+
+pub(super) fn scope<R>(ctx: &NameContext, f: impl FnOnce() -> R) -> R {
+    STACK.with(|s| {
+        s.borrow_mut().push(Slot::Shared(NonNull::from(ctx)));
+    });
+    let _guard = Guard;
+    f()
+}
+
+pub(super) fn scope_mut<R>(ctx: &mut NameContext, f: impl FnOnce() -> R) -> R {
+    STACK.with(|s| {
+        s.borrow_mut().push(Slot::Exclusive {
+            ptr: NonNull::from(ctx),
+            borrow: Cell::new(BorrowState::Idle),
+        });
+    });
+    let _guard = Guard;
+    f()
+}
+
+// Inspect the topmost slot, optionally claim a borrow on it, and return what
+// we need to dereference the pointer outside the STACK borrow. The slot index
+// is returned for Exclusive slots so the caller can clear the flag later via
+// a fresh STACK borrow — this avoids holding a reference into the Vec across
+// the user closure (during which nested scopes may reallocate the Vec).
+fn claim_top_for_shared() -> Option<(NonNull<NameContext>, Option<usize>)> {
+    STACK.with(|s| {
+        let stack = s.borrow();
+        let idx = stack.len().checked_sub(1)?;
+        match &stack[idx] {
+            Slot::Shared(p) => Some((*p, None)),
+            Slot::Exclusive { ptr, borrow } => {
+                if borrow.get() == BorrowState::Exclusive {
+                    panic!("with_name_context: NameContext is already mutably borrowed");
+                }
+                borrow.set(BorrowState::Shared);
+                Some((*ptr, Some(idx)))
+            }
+        }
+    })
+}
+
+fn claim_top_for_exclusive() -> Option<(NonNull<NameContext>, usize)> {
+    STACK.with(|s| {
+        let stack = s.borrow();
+        let idx = stack.len().checked_sub(1)?;
+        match &stack[idx] {
+            Slot::Shared(_) => None,
+            Slot::Exclusive { ptr, borrow } => {
+                if borrow.get() != BorrowState::Idle {
+                    panic!("with_name_context_mut: NameContext is already borrowed");
+                }
+                borrow.set(BorrowState::Exclusive);
+                Some((*ptr, idx))
+            }
+        }
+    })
+}
+
+fn release_borrow(idx: usize) {
+    STACK.with(|s| {
+        if let Some(Slot::Exclusive { borrow, .. }) = s.borrow().get(idx) {
+            borrow.set(BorrowState::Idle);
+        }
+    });
+}
+
+pub(super) fn with_name_context<R>(f: impl FnOnce(Option<&NameContext>) -> R) -> R {
+    let Some((ptr, claimed_idx)) = claim_top_for_shared() else {
+        return f(None);
+    };
+    // SAFETY: `ptr` was registered by an enclosing `scope`/`scope_mut` whose
+    // Guard has not yet dropped, so the pointee is alive. For Exclusive slots
+    // we have just set the borrow flag to Shared, so any nested
+    // `with_name_context_mut` on this slot will panic before producing an
+    // aliasing `&mut`. For Shared slots the original `&NameContext` is
+    // upheld by Rust's borrow checker around the enclosing `scope` call.
+    let r = unsafe { ptr.as_ref() };
+    let out = f(Some(r));
+    if let Some(idx) = claimed_idx {
+        release_borrow(idx);
+    }
+    out
+}
+
+pub(super) fn with_name_context_mut<R>(f: impl FnOnce(Option<&mut NameContext>) -> R) -> R {
+    let Some((mut ptr, idx)) = claim_top_for_exclusive() else {
+        return f(None);
+    };
+    // SAFETY: `ptr` originated from `scope_mut(&mut self, ...)` whose Guard
+    // has not yet dropped, so the pointee is alive and uniquely owned by the
+    // active `scope_mut`. We just set the borrow flag to Exclusive, so any
+    // nested `with_name_context` or `with_name_context_mut` on this slot will
+    // panic before aliasing.
+    let r = unsafe { ptr.as_mut() };
+    let out = f(Some(r));
+    release_borrow(idx);
+    out
+}
+
+pub(crate) fn current_name_type() -> Option<NameType> {
+    with_name_context(|c| c.map(NameContext::name_type))
+}
+
+#[cfg(test)]
+mod tests {
+    use std::panic::{AssertUnwindSafe, catch_unwind};
+
+    use super::super::{NameContext, NameType};
+    use super::*;
+
+    fn make(name_type: NameType) -> NameContext {
+        NameContext::new(name_type)
+    }
+
+    fn stack_len() -> usize {
+        STACK.with(|s| s.borrow().len())
+    }
+
+    #[test]
+    fn no_scope_returns_none() {
+        assert!(with_name_context(|c| c.is_none()));
+        assert!(with_name_context_mut(|c| c.is_none()));
+        assert_eq!(current_name_type(), None);
+    }
+
+    #[test]
+    fn scope_makes_context_visible_then_clears() {
+        let ctx = make(NameType::Asobo32);
+        ctx.scope(|| {
+            assert!(with_name_context(|c| c.is_some()));
+            assert_eq!(current_name_type(), Some(NameType::Asobo32));
+        });
+        assert!(with_name_context(|c| c.is_none()));
+        assert_eq!(stack_len(), 0);
+    }
+
+    #[test]
+    fn scope_mut_offers_both_shared_and_exclusive() {
+        let mut ctx = make(NameType::Asobo32);
+        ctx.scope_mut(|| {
+            assert!(with_name_context(|c| c.is_some()));
+            assert!(with_name_context_mut(|c| c.is_some()));
+        });
+        assert!(with_name_context(|c| c.is_none()));
+        assert!(with_name_context_mut(|c| c.is_none()));
+    }
+
+    #[test]
+    fn scope_returns_none_for_mut() {
+        let ctx = make(NameType::Asobo32);
+        ctx.scope(|| {
+            assert!(with_name_context_mut(|c| c.is_none()));
+        });
+    }
+
+    #[test]
+    fn nested_scopes_lifo_with_distinct_types() {
+        let outer = make(NameType::Asobo32);
+        let inner = make(NameType::Asobo64);
+        outer.scope(|| {
+            assert_eq!(current_name_type(), Some(NameType::Asobo32));
+            inner.scope(|| {
+                assert_eq!(current_name_type(), Some(NameType::Asobo64));
+            });
+            assert_eq!(current_name_type(), Some(NameType::Asobo32));
+        });
+        assert_eq!(current_name_type(), None);
+    }
+
+    #[test]
+    fn nested_scope_mut_inside_scope_works() {
+        let outer = make(NameType::Asobo32);
+        let mut inner = make(NameType::Asobo64);
+        outer.scope(|| {
+            inner.scope_mut(|| {
+                assert!(with_name_context_mut(|c| c.is_some()));
+                assert_eq!(current_name_type(), Some(NameType::Asobo64));
+            });
+            // Outer is shared, mut not available.
+            assert!(with_name_context_mut(|c| c.is_none()));
+            assert_eq!(current_name_type(), Some(NameType::Asobo32));
+        });
+    }
+
+    #[test]
+    fn panic_in_scope_unwinds_and_pops_stack() {
+        let ctx = make(NameType::Asobo32);
+        let result = catch_unwind(AssertUnwindSafe(|| {
+            ctx.scope(|| {
+                panic!("boom");
+            });
+        }));
+        assert!(result.is_err());
+        assert_eq!(stack_len(), 0);
+        assert!(with_name_context(|c| c.is_none()));
+    }
+
+    #[test]
+    #[should_panic(expected = "already mutably borrowed")]
+    fn shared_inside_mut_panics() {
+        let mut ctx = make(NameType::Asobo32);
+        ctx.scope_mut(|| {
+            with_name_context_mut(|outer| {
+                let _outer = outer.expect("mut should be available");
+                with_name_context(|_| {});
+            });
+        });
+    }
+
+    #[test]
+    #[should_panic(expected = "already borrowed")]
+    fn mut_inside_shared_panics() {
+        let mut ctx = make(NameType::Asobo32);
+        ctx.scope_mut(|| {
+            with_name_context(|outer| {
+                let _outer = outer.expect("shared should be available");
+                with_name_context_mut(|_| {});
+            });
+        });
+    }
+
+    #[test]
+    fn distinct_slots_do_not_conflict() {
+        let a = make(NameType::Asobo32);
+        let mut b = make(NameType::Asobo64);
+        a.scope(|| {
+            b.scope_mut(|| {
+                with_name_context_mut(|m| {
+                    let m = m.expect("top is exclusive");
+                    assert_eq!(m.name_type(), NameType::Asobo64);
+                });
+                with_name_context(|s| {
+                    let s = s.expect("top still resolvable");
+                    assert_eq!(s.name_type(), NameType::Asobo64);
+                });
+            });
+        });
+    }
+
+    #[test]
+    fn current_name_type_tracks_topmost() {
+        let outer = make(NameType::Asobo32);
+        let inner = outer.into_retyped(NameType::Asobo64);
+        let outer = make(NameType::Asobo32);
+        outer.scope(|| {
+            assert_eq!(current_name_type(), Some(NameType::Asobo32));
+            inner.scope(|| {
+                assert_eq!(current_name_type(), Some(NameType::Asobo64));
+            });
+            assert_eq!(current_name_type(), Some(NameType::Asobo32));
+        });
+    }
+
+    #[test]
+    fn insert_inside_scope_mut_is_resolvable() {
+        let mut ctx = make(NameType::Asobo32);
+        let inserted = ctx.scope_mut(|| {
+            with_name_context_mut(|c| {
+                let c = c.expect("mut available");
+                c.insert("hello_inserted_name")
+            })
+        });
+        assert_eq!(
+            ctx.resolve(inserted).as_deref(),
+            Some("hello_inserted_name")
+        );
+    }
+}

bff/src/names/serde_schema.rs

@@ -4,7 +4,7 @@ use schemars::schema::{InstanceType, Schema, SchemaObject, SingleOrVec};
 use schemars::{JsonSchema, SchemaGenerator};
 use serde::{Deserialize, Deserializer, Serialize};
 
-use super::context::{with_name_context, with_name_context_mut};
+use super::scope::{with_name_context, with_name_context_mut};
 use super::{Name, NameType, hash_string_for_type};
 use crate::traits::NameHashFunction;
 

bff/src/names/value.rs

@@ -6,7 +6,7 @@ use binrw::{BinRead, BinResult, BinWrite, Endian};
 use const_power_of_two::PowerOfTwoUsize;
 use num_traits::AsPrimitive;
 
-use super::context::{current_name_type, with_name_context};
+use super::scope::{current_name_type, with_name_context};
 use super::{NameContext, NameType};
 use crate::traits::{NameHashFunction, NameTarget};