diff --git a/bff-cli/src/info.rs b/bff-cli/src/info.rs index a1b66fb..1413bf7 100644 --- a/bff-cli/src/info.rs +++ b/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(()) diff --git a/bff/src/names/context.rs b/bff/src/names/context.rs index 5e15b33..6da2618 100644 --- a/bff/src/names/context.rs +++ b/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> = const { RefCell::new(Vec::new()) }; - static ACTIVE_MUT_NAME_CONTEXT_STACK: RefCell> = 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(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(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 { - with_name_context(|name_context| name_context.map(NameContext::name_type)) -} - pub(super) type NameMap = HashMap; fn insert_name(names: &mut NameMap, name_type: NameType, string: &str) -> Name { @@ -201,22 +152,11 @@ impl NameContext { } pub fn scope(&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(&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 { diff --git a/bff/src/names/mod.rs b/bff/src/names/mod.rs index 09e9d1d..f475b19 100644 --- a/bff/src/names/mod.rs +++ b/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; diff --git a/bff/src/names/scope.rs b/bff/src/names/scope.rs new file mode 100644 index 0000000..f20c574 --- /dev/null +++ b/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), + Exclusive { + ptr: NonNull, + borrow: Cell, + }, +} + +thread_local! { + static STACK: RefCell> = 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(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(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, Option)> { + 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, 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(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(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 { + 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") + ); + } +} diff --git a/bff/src/names/serde_schema.rs b/bff/src/names/serde_schema.rs index ae23d00..1ab8be0 100644 --- a/bff/src/names/serde_schema.rs +++ b/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; diff --git a/bff/src/names/value.rs b/bff/src/names/value.rs index 7dde213..cb737e8 100644 --- a/bff/src/names/value.rs +++ b/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};