From c040b0c6737006deeae82979e95512be6bfb7cee Mon Sep 17 00:00:00 2001
From: Niko Matsakis <niko@alum.mit.edu>
Date: Fri, 22 Mar 2019 16:24:37 -0400
Subject: [PATCH] fix gc and volatile tests

---
 src/derived.rs             | 34 +++++++++++++-----
 tests/gc/db.rs             |  3 +-
 tests/gc/main.rs           |  1 +
 tests/gc/volatile_tests.rs | 72 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+), 10 deletions(-)
 create mode 100644 tests/gc/volatile_tests.rs

diff --git a/src/derived.rs b/src/derived.rs
index a18235fb..3419d3e1 100644
--- a/src/derived.rs
+++ b/src/derived.rs
@@ -942,15 +942,6 @@ where
     fn sweep(&self, db: &DB, strategy: SweepStrategy) {
         let mut map_write = self.map.write();
         let revision_now = db.salsa_runtime().current_revision();
-        match (strategy.discard_if, strategy.discard_what) {
-            (DiscardIf::Always, DiscardWhat::Everything) => {
-                debug!("sweep({:?}): clearing the table", Q::default());
-                map_write.clear();
-                return;
-            }
-            (DiscardIf::Never, _) | (_, DiscardWhat::Nothing) => return,
-            _ => {}
-        }
         map_write.retain(|key, query_state| {
             match query_state {
                 // Leave stuff that is currently being computed -- the
@@ -972,6 +963,19 @@ where
                         revision_now
                     );
 
+                    // Check if this memo read something "untracked"
+                    // -- meaning non-deterministic.  In this case, we
+                    // can only collect "outdated" data that wasn't
+                    // used in the current revision. This is because
+                    // if we collected something from the current
+                    // revision, we might wind up re-executing the
+                    // query later in the revision and getting a
+                    // distinct result.
+                    let is_volatile = match memo.inputs {
+                        MemoInputs::Untracked => true,
+                        _ => false,
+                    };
+
                     // Since we don't acquire a query lock in this
                     // method, it *is* possible for the revision to
                     // change while we are executing. However, it is
@@ -982,7 +986,19 @@ where
                     assert!(memo.verified_at <= revision_now);
                     match strategy.discard_if {
                         DiscardIf::Never => unreachable!(),
+
+                        // If we are only discarding outdated things,
+                        // and this is not outdated, keep it.
                         DiscardIf::Outdated if memo.verified_at == revision_now => true,
+
+                        // As explained on the `is_volatile` variable
+                        // definition, if this is a volatile entry, we
+                        // can't discard it unless it is outdated.
+                        DiscardIf::Always if is_volatile && memo.verified_at == revision_now => {
+                            true
+                        }
+
+                        // Otherwise, we can discard -- discard whatever the user requested.
                         DiscardIf::Outdated | DiscardIf::Always => match strategy.discard_what {
                             DiscardWhat::Nothing => unreachable!(),
                             DiscardWhat::Values => {
diff --git a/tests/gc/db.rs b/tests/gc/db.rs
index b81dae8c..b6aaba57 100644
--- a/tests/gc/db.rs
+++ b/tests/gc/db.rs
@@ -1,8 +1,9 @@
 use crate::group;
 use crate::interned;
 use crate::log::{HasLog, Log};
+use crate::volatile_tests;
 
-#[salsa::database(group::Gc, interned::Intern)]
+#[salsa::database(group::Gc, interned::Intern, volatile_tests::Volatile)]
 #[derive(Default)]
 pub(crate) struct DatabaseImpl {
     runtime: salsa::Runtime<DatabaseImpl>,
diff --git a/tests/gc/main.rs b/tests/gc/main.rs
index ff9dedfe..9bfa6b30 100644
--- a/tests/gc/main.rs
+++ b/tests/gc/main.rs
@@ -16,3 +16,4 @@ mod group;
 mod interned;
 mod log;
 mod shallow_constant_tests;
+mod volatile_tests;
diff --git a/tests/gc/volatile_tests.rs b/tests/gc/volatile_tests.rs
new file mode 100644
index 00000000..9953fdbf
--- /dev/null
+++ b/tests/gc/volatile_tests.rs
@@ -0,0 +1,72 @@
+use crate::db;
+use salsa::{Database, SweepStrategy};
+use std::sync::atomic::{AtomicU32, Ordering};
+use std::sync::Arc;
+
+/// Query group for tests for how interned keys interact with GC.
+#[salsa::query_group(Volatile)]
+pub(crate) trait VolatileDatabase {
+    #[salsa::input]
+    fn atomic_cell(&self) -> Arc<AtomicU32>;
+
+    /// Underlying volatile query.
+    #[salsa::volatile]
+    fn volatile(&self) -> u32;
+
+    /// This just executes the intern query and returns the result.
+    fn repeat1(&self) -> u32;
+
+    /// Same as `repeat_intern1`. =)
+    fn repeat2(&self) -> u32;
+}
+
+fn volatile(db: &impl VolatileDatabase) -> u32 {
+    db.atomic_cell().load(Ordering::SeqCst)
+}
+
+fn repeat1(db: &impl VolatileDatabase) -> u32 {
+    db.volatile()
+}
+
+fn repeat2(db: &impl VolatileDatabase) -> u32 {
+    db.volatile()
+}
+
+#[test]
+fn consistency_no_gc() {
+    let mut db = db::DatabaseImpl::default();
+
+    let cell = Arc::new(AtomicU32::new(22));
+
+    db.set_atomic_cell(cell.clone());
+
+    let v1 = db.repeat1();
+
+    cell.store(23, Ordering::SeqCst);
+
+    let v2 = db.repeat2();
+
+    assert_eq!(v1, v2);
+}
+
+#[test]
+fn consistency_with_gc() {
+    let mut db = db::DatabaseImpl::default();
+
+    let cell = Arc::new(AtomicU32::new(22));
+
+    db.set_atomic_cell(cell.clone());
+
+    let v1 = db.repeat1();
+
+    cell.store(23, Ordering::SeqCst);
+    db.query(VolatileQuery).sweep(
+        SweepStrategy::default()
+            .discard_everything()
+            .sweep_all_revisions(),
+    );
+
+    let v2 = db.repeat2();
+
+    assert_eq!(v1, v2);
+}