Recover from attached_packages cycles in R/ collation

crates/oak_db/src/file.rs

@@ -160,8 +160,11 @@ impl File {
     /// The two handlers behave differently:
     ///
     /// - `semantic_index` (this query, custom rebuild): the file is rebuilt
-    ///   with `NoopImportsResolver`. Cross-file injection drops, but local
-    ///   analysis (scopes, use-def maps, function bodies) is preserved.
+    ///   with `NoopImportsResolver`. Scopes, use-def maps and function bodies
+    ///   survive, but everything that needs the resolver drops. That includes
+    ///   effect detection: the Noop resolver never resolves the `library()` or
+    ///   `source()` callee, so the rebuilt index records no attaches and no
+    ///   source sites at all.
     ///
     /// - `exports` (FallbackImmediate, empty): the file contributes no names
     ///   for the revision.
@@ -188,7 +191,15 @@ impl File {
     ///
     /// A `library()` in a function body does not count here; for every attach
     /// regardless of context see [`Self::attached_packages_anywhere`].
-    #[salsa::tracked(returns(ref))]
+    ///
+    /// `cycle_result` is required. In an `R/` directory,
+    /// [`File::cross_file_layers`] reads the `attached_packages` of each
+    /// collation predecessor, and building a predecessor's index resolves that
+    /// file's own `source()` sites, which reaches back into
+    /// `cross_file_layers` and asks for this same file again. Salsa re-enters
+    /// here rather than at `semantic_index`, so this query needs its own
+    /// recovery (#15631).
+    #[salsa::tracked(returns(ref), cycle_result = attached_packages_cycle_result)]
     pub fn attached_packages(self, db: &dyn Db) -> Vec<Name<'_>> {
         self.semantic_index(db)
             .attached_packages()
@@ -204,7 +215,13 @@ impl File {
     /// Over-approximates the load-time search path. Used for workspace
     /// dependency discovery, where a package attached only inside a function
     /// still counts as a dependency.
-    #[salsa::tracked(returns(ref))]
+    ///
+    /// `cycle_result` is defensive here, and unreachable today. Nothing inside
+    /// `semantic_index` or `cross_file_layers` reads this query, so it can only
+    /// sit above a cycle head, never between the head and the re-entry. It
+    /// shares [`attached_packages_cycle_result`] so that a future edge into it
+    /// degrades like [`Self::attached_packages`] instead of panicking.
+    #[salsa::tracked(returns(ref), cycle_result = attached_packages_cycle_result)]
     pub fn attached_packages_anywhere(self, db: &dyn Db) -> Vec<Name<'_>> {
         self.semantic_index(db)
             .attached_packages_anywhere()
@@ -346,6 +363,20 @@ fn build_semantic_index_inner(file: File, db: &dyn Db) -> SemanticIndex {
     oak_semantic::build_index(&parsed.tree(), resolver)
 }
 
+/// A file caught in an attach cycle contributes no attaches for the revision.
+///
+/// This only restates what the file reports anyway. The cycle always also runs
+/// through `semantic_index`, and its Noop rebuild already records no attaches
+/// (see [`File::semantic_index`]). That recovery raises
+/// [`SemanticDiagnostic::SourceCycle`], so nothing is reported here.
+fn attached_packages_cycle_result<'db>(
+    _db: &'db dyn Db,
+    _id: salsa::Id,
+    _file: File,
+) -> Vec<Name<'db>> {
+    Vec::new()
+}
+
 fn semantic_index_cycle_result(db: &dyn Db, _id: salsa::Id, file: File) -> SemanticIndex {
     log::warn!(
         "Cyclic `source()` detected at {}. Rebuilding without cross-file resolution.",

crates/oak_db/src/tests/workspace.rs

@@ -55,6 +55,12 @@ fn workspace_with_script(db: &mut TestDb, contents: &str) {
 /// Create a `proj` workspace root with editor overrides for `scripts`.
 /// Paths are relative to `proj`.
 fn workspace_with_scripts(db: &mut TestDb, scripts: &[(&str, &str)]) {
+    workspace_with_scripts_files(db, scripts);
+}
+
+/// Like [`workspace_with_scripts`] but hands back the files so a test can query
+/// a specific one first.
+fn workspace_with_scripts_files(db: &mut TestDb, scripts: &[(&str, &str)]) -> Vec<File> {
     let root = workspace_root(&*db, "proj");
     let files: Vec<File> = scripts
         .iter()
@@ -68,8 +74,9 @@ fn workspace_with_scripts(db: &mut TestDb, scripts: &[(&str, &str)]) {
             )
         })
         .collect();
-    root.set_scripts(db).to(files);
+    root.set_scripts(db).to(files.clone());
     db.workspace_roots().set_roots(db).to(vec![root]);
+    files
 }
 
 fn all_package_dependencies_names(db: &TestDb) -> Vec<String> {
@@ -324,3 +331,50 @@ fn test_testthat_file_depends_on_testthat() {
 
     assert_eq!(all_package_dependencies_names(&db), vec!["testthat"]);
 }
+
+#[test]
+fn test_r_directory_collation_with_a_source_call_does_not_panic() {
+    // Three loose scripts in an `R/` directory, collated alphabetically, where
+    // `a.R` sources `b.R`. Touching `c.R` first is what makes salsa re-enter
+    // `attached_packages` instead of `semantic_index` (#15631).
+    //
+    // The cycling path resolves effects with `CollationView::Eager`, so each
+    // `cross_file_layers` sees only that file's collation predecessors:
+    //
+    //   semantic_index(c)
+    //     -> cross_file_layers(c)             predecessors a, b
+    //       -> attached_packages(a)           1st entry
+    //         -> semantic_index(a)
+    //           -> resolves `source("R/b.R")`
+    //             -> semantic_index(b)
+    //               -> cross_file_layers(b)   predecessor a
+    //                 -> attached_packages(a) 2nd entry, cycle
+    //
+    // `semantic_index` and `exports` carry `cycle_result` recovery, so a cycle
+    // re-entered at either of them degrades gracefully. Without recovery on
+    // `attached_packages` salsa panics, which killed the LSP main loop.
+    let mut db = TestDb::new();
+    register_library(&mut db, &["pkga", "pkgb", "pkgc"]);
+    let files = workspace_with_scripts_files(&mut db, &[
+        ("R/a.R", "library(pkga)\nsource(\"R/b.R\")\n"),
+        ("R/b.R", "library(pkgb)\n"),
+        ("R/c.R", "library(pkgc)\n"),
+    ]);
+
+    // `c.R` first.
+    let _ = files[2].used_packages(&db);
+
+    // `a.R` and `b.R` drop out of the dependency set, but the empty attach
+    // fallback is not what loses them. `semantic_index_cycle_result` rebuilds
+    // both files with `NoopImportsResolver`, which resolves no callee and so
+    // records no `library()` call at all. The same loss shows up in the
+    // orderings that never panicked.
+    //
+    // This assertion therefore pins down current behaviour, not intended
+    // behaviour. The cycle is a false one: `a.R` sources `b.R` and nothing
+    // sources back, and only `R/` collation puts `attached_packages(a)` on the
+    // stack twice. Breaking the cycle, by giving `predecessor_attach_layers` an
+    // attach query that does not read a sibling's semantic index, would keep
+    // all three packages and drop the spurious `SourceCycle` diagnostic.
+    assert_eq!(all_package_dependencies_names(&db), vec!["pkgc"]);
+}