Make cycle recovery observable in tests

crates/oak_db/src/file.rs

@@ -12,6 +12,8 @@ use crate::file_imports::CollationView;
 use crate::file_revision::report_untracked_if_zero;
 use crate::imports::SalsaImportsResolver;
 use crate::parse::OakParse;
+use crate::recovery::record;
+use crate::recovery::Recovery;
 use crate::Db;
 use crate::FileRevision;
 use crate::Name;
@@ -211,10 +213,12 @@ impl File {
     /// dependency discovery, where a package attached only inside a function
     /// still counts as a dependency.
     ///
-    /// This query is not currently below a cycle path because neither
-    /// `semantic_index()` nor `cross_file_layers()` reads it. We recover from
-    /// cycles defensively.
-    #[salsa::tracked(returns(ref), cycle_result = attached_packages_cycle_result)]
+    /// Defensive fallback. [`Self::semantic_index`] and [`Self::cross_file_layers`]
+    /// do not read this query, so it cannot currently be Salsa's repeated key.
+    /// If a future dependency re-enters it,
+    /// [`attached_packages_anywhere_cycle_result`] returns no packages rather
+    /// than panicking.
+    #[salsa::tracked(returns(ref), cycle_result = attached_packages_anywhere_cycle_result)]
     pub fn attached_packages_anywhere(self, db: &dyn Db) -> Vec<Name<'_>> {
         self.semantic_index(db)
             .attached_packages_anywhere()
@@ -372,9 +376,26 @@ fn build_semantic_index_inner(file: File, db: &dyn Db) -> SemanticIndex {
 
 fn attached_packages_cycle_result<'db>(
     db: &'db dyn Db,
-    _id: salsa::Id,
+    id: salsa::Id,
+    file: File,
+) -> Vec<Name<'db>> {
+    record(db, Recovery::AttachedPackages(file));
+    attached_packages_fallback(db, id, file)
+}
+
+fn attached_packages_anywhere_cycle_result<'db>(
+    db: &'db dyn Db,
+    id: salsa::Id,
     file: File,
 ) -> Vec<Name<'db>> {
+    record(db, Recovery::AttachedPackagesAnywhere(file));
+    attached_packages_fallback(db, id, file)
+}
+
+/// Return no attaches. [`File::semantic_index`] recovery rebuilds with
+/// `NoopImportsResolver`, which emits [`SemanticDiagnostic::SourceCycle`] and
+/// also reports no attaches.
+fn attached_packages_fallback<'db>(db: &'db dyn Db, _id: salsa::Id, file: File) -> Vec<Name<'db>> {
     log::warn!(
         "Cyclic attaches detected at {}. Reporting no attached packages.",
         file.path(db),
@@ -383,6 +404,7 @@ fn attached_packages_cycle_result<'db>(
 }
 
 fn semantic_index_cycle_result(db: &dyn Db, _id: salsa::Id, file: File) -> SemanticIndex {
+    record(db, Recovery::SemanticIndex(file));
     log::warn!(
         "Cyclic `source()` detected at {}. Rebuilding without cross-file resolution.",
         file.path(db),

crates/oak_db/src/file_exports.rs

@@ -2,6 +2,8 @@ use aether_path::FilePath;
 use oak_semantic::semantic_index::DefinitionKind;
 use rustc_hash::FxHashMap;
 
+use crate::recovery::record;
+use crate::recovery::Recovery;
 use crate::Db;
 use crate::File;
 
@@ -101,6 +103,7 @@ impl File {
     }
 }
 
-fn exports_cycle_result(_db: &dyn Db, _id: salsa::Id, _file: File) -> FileExports {
+fn exports_cycle_result(db: &dyn Db, _id: salsa::Id, file: File) -> FileExports {
+    record(db, Recovery::Exports(file));
     FileExports::default()
 }

crates/oak_db/src/file_imports.rs

@@ -16,6 +16,8 @@ use crate::load_context::load_context;
 use crate::load_context::LoadContext;
 use crate::load_context::LoadKind;
 use crate::load_context::SearchPathTail;
+use crate::recovery::record;
+use crate::recovery::Recovery;
 use crate::Db;
 use crate::File;
 use crate::Package;
@@ -419,9 +421,11 @@ impl File {
     ///
     /// Empty for a file with an explicit load order.
     ///
-    /// `cycle_result` is defensive. Resolving a source site reads the target's
-    /// `exports`, meaning that a source cycle is always also a `semantic_index`
-    /// cycle which has its own recovery.
+    /// Defensive fallback. A `sourced_by()` edge is created only after the
+    /// resolver reads `exports(target)`, so a `source()` cycle re-enters
+    /// `exports()` or `semantic_index()` before `inherited_layers()`. The
+    /// `NoopImportsResolver` rebuild removes the source sites that could
+    /// otherwise re-enter this query.
     #[salsa::tracked(returns(ref), cycle_result =
     inherited_layers_cycle_result)]
     pub(crate) fn inherited_layers(self, db: &dyn Db, view: CollationView) -> Vec<InheritedLayers> {
@@ -512,15 +516,19 @@ fn cross_file_layers_cycle_result(
     file: File,
     view: CollationView,
 ) -> CrossFileLayers {
+    record(db, Recovery::CrossFileLayers(file, view));
     lower_load_context(db, load_context(db, file, view), PredecessorAttaches::Skip)
 }
 
+/// Return no inherited layers when this query is Salsa's repeated key in a
+/// `source()` cycle.
 fn inherited_layers_cycle_result(
-    _db: &dyn Db,
+    db: &dyn Db,
     _id: salsa::Id,
-    _file: File,
-    _view: CollationView,
+    file: File,
+    view: CollationView,
 ) -> Vec<InheritedLayers> {
+    record(db, Recovery::InheritedLayers(file, view));
     Vec::new()
 }
 

crates/oak_db/src/lib.rs

@@ -17,6 +17,7 @@ mod name;
 mod package;
 mod package_resolve;
 mod parse;
+mod recovery;
 mod search;
 mod storage;
 mod workspace;

crates/oak_db/src/package_resolve.rs

@@ -1,3 +1,5 @@
+use crate::recovery::record;
+use crate::recovery::Recovery;
 use crate::Db;
 use crate::Definition;
 use crate::Name;
@@ -86,8 +88,9 @@ fn resolve_cycle_result<'db>(
     _id: salsa::Id,
     package: Package,
     name: Name<'db>,
-    _visibility: NamespaceVisibility,
+    visibility: NamespaceVisibility,
 ) -> Vec<Definition<'db>> {
+    record(db, Recovery::PackageResolve(package, name, visibility));
     log::warn!(
         "Cyclic NAMESPACE re-export of `{}` detected at `{}`. Resolving to no candidates.",
         name.text(db).as_str(),

crates/oak_db/src/recovery.rs

@@ -0,0 +1,84 @@
+//! Records every Salsa cycle-recovery handler invoked during a test.
+//!
+//! Recovery handlers return ordinary fallback values, often an empty `Vec` that
+//! a non-cycling query can also return. Tests therefore cannot tell from a
+//! query result whether recovery happened. [`record()`] writes the handler and
+//! its query key here, allowing tests to assert the handler Salsa chose.
+//!
+//! The log is process-global so calls from Salsa worker threads are recorded.
+//! Nextest runs each test in its own process, so tests do not share this log.
+
+#[cfg(test)]
+use std::sync::Mutex;
+
+use crate::file_imports::CollationView;
+#[cfg(test)]
+use crate::tests::test_db::path_name;
+use crate::Db;
+use crate::File;
+use crate::Name;
+use crate::NamespaceVisibility;
+use crate::Package;
+
+/// One variant per query that declares a `cycle_result` handler, carrying
+/// that query's salsa key.
+#[cfg_attr(not(test), allow(dead_code))]
+pub(crate) enum Recovery<'db> {
+    SemanticIndex(File),
+    Exports(File),
+    AttachedPackages(File),
+    AttachedPackagesAnywhere(File),
+    InheritedLayers(File, CollationView),
+    CrossFileLayers(File, CollationView),
+    PackageResolve(Package, Name<'db>, NamespaceVisibility),
+}
+
+#[cfg(test)]
+static FIRED: Mutex<Vec<String>> = Mutex::new(Vec::new());
+
+#[cfg(test)]
+pub(crate) fn record(db: &dyn Db, recovery: Recovery<'_>) {
+    FIRED.lock().unwrap().push(render(db, recovery));
+}
+
+#[cfg(not(test))]
+pub(crate) fn record(_db: &dyn Db, _recovery: Recovery<'_>) {}
+
+/// Clear before each probe. Salsa memoizes a cycle result, so a warm database
+/// does not invoke its handler again. Without a reset, an earlier probe's entry
+/// would look like recovery from this probe.
+#[cfg(test)]
+pub(crate) fn reset() {
+    FIRED.lock().unwrap().clear();
+}
+
+/// Recorded firings, in the order salsa consulted them.
+#[cfg(test)]
+pub(crate) fn fired() -> Vec<String> {
+    FIRED.lock().unwrap().clone()
+}
+
+#[cfg(test)]
+fn render(db: &dyn Db, recovery: Recovery<'_>) -> String {
+    match recovery {
+        Recovery::SemanticIndex(file) => format!("semantic_index({})", path_name(file.path(db))),
+        Recovery::Exports(file) => format!("exports({})", path_name(file.path(db))),
+        Recovery::AttachedPackages(file) => {
+            format!("attached_packages({})", path_name(file.path(db)))
+        },
+        Recovery::AttachedPackagesAnywhere(file) => {
+            format!("attached_packages_anywhere({})", path_name(file.path(db)))
+        },
+        Recovery::InheritedLayers(file, view) => {
+            format!("inherited_layers({}, {view:?})", path_name(file.path(db)))
+        },
+        Recovery::CrossFileLayers(file, view) => {
+            format!("cross_file_layers({}, {view:?})", path_name(file.path(db)))
+        },
+        Recovery::PackageResolve(package, name, visibility) => format!(
+            "Package::resolve({}, {}, {visibility:?})",
+            package.name(db),
+            name.text(db).as_str(),
+        ),
+    }
+}

crates/oak_db/src/tests.rs

@@ -1,4 +1,5 @@
 mod contrib;
+mod cycle_results;
 mod db;
 mod diagnostic_render;
 mod file;
@@ -13,7 +14,8 @@ mod file_source_site;
 mod identifier;
 mod inputs;
 mod package_resolve;
+mod recovery;
 mod resolver;
-mod test_db;
+pub(crate) mod test_db;
 mod tidy;
 mod workspace;