diff --git a/src/lib.rs b/src/lib.rs index 911be173..b3d226cc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -368,7 +368,7 @@ macro_rules! query_definition { ( @storage_ty[$QC:ident, $Self:ident, volatile] ) => { - $crate::volatile::VolatileStorage + $crate::volatile::VolatileStorage<$QC, $Self> }; // Accept a "field-like" query definition (input) diff --git a/src/volatile.rs b/src/volatile.rs index 911da65a..5312ffd4 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -5,8 +5,8 @@ use crate::QueryContext; use crate::QueryStorageOps; use crate::QueryTable; use log::debug; -use parking_lot::{RwLock, RwLockUpgradableReadGuard}; -use rustc_hash::FxHashMap; +use parking_lot::Mutex; +use rustc_hash::FxHashSet; use std::any::Any; use std::cell::RefCell; use std::collections::hash_map::Entry; @@ -17,10 +17,29 @@ use std::hash::Hash; /// Volatile Storage is just **always** considered dirty. Any time you /// ask for the result of such a query, it is recomputed. -#[derive(Default)] -pub struct VolatileStorage; +pub struct VolatileStorage +where + Q: Query, + QC: QueryContext, +{ + /// We don't store the results of volatile queries, + /// but we track in-progress set to detect cycles. + in_progress: Mutex>, +} -impl QueryStorageOps for VolatileStorage +impl Default for VolatileStorage +where + Q: Query, + QC: QueryContext, +{ + fn default() -> Self { + VolatileStorage { + in_progress: Mutex::new(FxHashSet::default()), + } + } +} + +impl QueryStorageOps for VolatileStorage where Q: Query, QC: QueryContext, @@ -31,12 +50,18 @@ where key: &Q::Key, descriptor: &QC::QueryDescriptor, ) -> Result { + if !self.in_progress.lock().insert(key.clone()) { + return Err(CycleDetected); + } + // FIXME: Should we even call `execute_query_implementation` // here? Or should we just call `Q::execute`, and maybe // separate out the `push`/`pop` operations. let (value, _inputs) = query .salsa_runtime() .execute_query_implementation::(query, descriptor, key); + let was_in_progress = self.in_progress.lock().remove(key); + assert!(was_in_progress); query.salsa_runtime().report_query_read(descriptor); diff --git a/tests/cycles.rs b/tests/cycles.rs new file mode 100644 index 00000000..6ca25320 --- /dev/null +++ b/tests/cycles.rs @@ -0,0 +1,73 @@ +#![feature(crate_visibility_modifier)] + +#[derive(Default)] +pub struct QueryContextImpl { + runtime: salsa::runtime::Runtime, +} + +impl salsa::QueryContext for QueryContextImpl { + fn salsa_runtime(&self) -> &salsa::runtime::Runtime { + &self.runtime + } +} + +salsa::query_context_storage! { + pub struct QueryContextImplStorage for QueryContextImpl { + impl QueryContext { + fn memoized_a() for MemoizedA; + fn memoized_b() for MemoizedB; + fn volatile_a() for VolatileA; + fn volatile_b() for VolatileB; + } + } +} + +trait QueryContext: salsa::QueryContext { + salsa::query_prototype! { + // `a` and `b` depend on each other and form a cycle + fn memoized_a() for MemoizedA; + fn memoized_b() for MemoizedB; + fn volatile_a() for VolatileA; + fn volatile_b() for VolatileB; + } +} + +salsa::query_definition! { + crate MemoizedA(query: &impl QueryContext, (): ()) -> () { + query.memoized_b().get(()) + } +} + +salsa::query_definition! { + crate MemoizedB(query: &impl QueryContext, (): ()) -> () { + query.memoized_a().get(()) + } +} + +salsa::query_definition! { + #[storage(volatile)] + crate VolatileA(query: &impl QueryContext, (): ()) -> () { + query.volatile_b().get(()) + } +} + +salsa::query_definition! { + #[storage(volatile)] + crate VolatileB(query: &impl QueryContext, (): ()) -> () { + query.volatile_a().get(()) + } +} + +#[test] +#[should_panic(expected = "cycle detected")] +fn cycle_memoized() { + let query = QueryContextImpl::default(); + query.memoized_a().get(()); +} + +#[test] +#[should_panic(expected = "cycle detected")] +fn cycle_volatile() { + let query = QueryContextImpl::default(); + query.volatile_a().get(()); +}