Follow-up for attached packages fix

crates/oak_db/src/diagnostic.rs

@@ -3,6 +3,11 @@ use biome_rowan::TextSize;
 use oak_semantic::semantic_index::AmbiguityReason;
 use oak_semantic::semantic_index::SemanticDiagnostic;
 
+use crate::load_context::loader;
+use crate::load_context::LoaderInfo;
+use crate::Db;
+use crate::File;
+
 /// A diagnostic derived from a file's semantic analysis.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Diagnostic {
@@ -104,7 +109,11 @@ pub enum Severity {
 }
 
 /// Lower one of `oak_semantic`'s raw diagnostic records into a `Diagnostic`.
-pub(crate) fn lower_semantic_diagnostic(diagnostic: &SemanticDiagnostic) -> Diagnostic {
+pub(crate) fn lower_semantic_diagnostic(
+    db: &dyn Db,
+    file: File,
+    diagnostic: &SemanticDiagnostic,
+) -> Diagnostic {
     match diagnostic {
         SemanticDiagnostic::AmbiguousEffect {
             name,
@@ -117,7 +126,7 @@ pub(crate) fn lower_semantic_diagnostic(diagnostic: &SemanticDiagnostic) -> Diag
         SemanticDiagnostic::UninstalledPackage { package, range } => {
             lower_uninstalled_package(package, *range)
         },
-        SemanticDiagnostic::SourceCycle => lower_source_cycle(),
+        SemanticDiagnostic::SourceCycle => lower_source_cycle(loader(db, file)),
     }
 }
 
@@ -199,14 +208,30 @@ fn lower_uninstalled_package(package: &str, range: TextRange) -> Diagnostic {
     )
 }
 
-/// Anchored at the start of the file because the record carries no range.
-/// Every file in the cycle gets its own diagnostic.
-fn lower_source_cycle() -> Diagnostic {
+/// Anchor at the file start because cycle records carry no source range.
+///
+/// Report every participant. Recovery rebuilds without cross-file resolution,
+/// so we can't identify the exact source call or effect that formed the cycle.
+fn lower_source_cycle(loader: Option<LoaderInfo>) -> Diagnostic {
+    let cause = match loader {
+        Some(LoaderInfo { name, loads }) => {
+            format!(
+                "{name} already loads {loads}.\n\
+                 A `source()` call into a file that the loader also loads creates this cycle."
+            )
+        },
+        None => "These files may `source()` each other, or `source()` a file that a loader such \
+                 as a Shiny app or testthat suite already loads for them."
+            .to_string(),
+    };
+
     Diagnostic::new(
         DiagnosticKind::SourceCycle,
-        "This file takes part in a cycle of mutual `source()` calls.\n\
-         Language analysis will be incomplete until the cycle is resolved."
-            .to_string(),
+        format!(
+            "This file is part of a cycle in how the project's files load each other.\n\
+             {cause}\n\
+             Language analysis will be incomplete until the cycle is resolved."
+        ),
         TextRange::empty(TextSize::from(0)),
         Vec::new(),
     )

crates/oak_db/src/file.rs

@@ -159,12 +159,10 @@ impl File {
     ///
     /// The two handlers behave differently:
     ///
-    /// - `semantic_index` (this query, custom rebuild): the file is rebuilt
-    ///   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.
+    /// - `semantic_index` (this query, custom rebuild): rebuilds the file with
+    ///   `NoopImportsResolver`. Scopes, use-def maps, and function bodies remain
+    ///   available. Resolver-dependent data, including `library()` attachments
+    ///   and `source()` sites, is omitted.
     ///
     /// - `exports` (FallbackImmediate, empty): the file contributes no names
     ///   for the revision.
@@ -192,13 +190,9 @@ impl File {
     /// A `library()` in a function body does not count here; for every attach
     /// regardless of context see [`Self::attached_packages_anywhere`].
     ///
-    /// `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).
+    /// In `R/` collation, `cross_file_layers()` queries each predecessor's
+    /// `attached_packages()`. Resolving a predecessor's `source()` can re-enter
+    /// this query through `cross_file_layers()`, so recovery belongs here.
     #[salsa::tracked(returns(ref), cycle_result = attached_packages_cycle_result)]
     pub fn attached_packages(self, db: &dyn Db) -> Vec<Name<'_>> {
         self.semantic_index(db)
@@ -216,11 +210,9 @@ impl File {
     /// dependency discovery, where a package attached only inside a function
     /// still counts as a dependency.
     ///
-    /// `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.
+    /// 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)]
     pub fn attached_packages_anywhere(self, db: &dyn Db) -> Vec<Name<'_>> {
         self.semantic_index(db)
@@ -283,7 +275,7 @@ impl File {
             .semantic_index(db)
             .diagnostics()
             .iter()
-            .map(lower_semantic_diagnostic)
+            .map(|diagnostic| lower_semantic_diagnostic(db, self, diagnostic))
             .collect();
 
         diagnostics.extend(inherited_shadow_diagnostics(db, self));
@@ -363,17 +355,15 @@ 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,
+    db: &'db dyn Db,
     _id: salsa::Id,
-    _file: File,
+    file: File,
 ) -> Vec<Name<'db>> {
+    log::warn!(
+        "Cyclic attaches detected at {}. Reporting no attached packages.",
+        file.path(db),
+    );
     Vec::new()
 }
 

crates/oak_db/src/file_imports.rs

@@ -397,8 +397,7 @@ impl File {
 
     /// The file's own layers, plus one alternative per file that sources it.
     fn layers_by_sourcing_file(self, db: &dyn Db, view: CollationView) -> Vec<&CrossFileLayers> {
-        let mut alternatives: Vec<&CrossFileLayers> =
-            self.own_layers(db, view).into_iter().collect();
+        let mut alternatives = vec![self.cross_file_layers(db, view)];
         alternatives.extend(
             self.inherited_layers(db, view)
                 .iter()
@@ -407,19 +406,6 @@ impl File {
         alternatives
     }
 
-    /// [`File::cross_file_layers`], unless an inherited source site replaces a
-    /// [`LoadKind::Fallback`] context.
-    ///
-    /// The fallback assumes a non-package `R/` directory collates. An explicit
-    /// source site supplies the actual context and overrides the fallback.
-    /// Retain the fallback when cycle recovery leaves no inherited source sites.
-    fn own_layers(self, db: &dyn Db, view: CollationView) -> Option<&CrossFileLayers> {
-        if self.has_fallback_context(db, view) && !self.inherited_layers(db, view).is_empty() {
-            return None;
-        }
-        Some(self.cross_file_layers(db, view))
-    }
-
     /// The layers `self` inherits from each file that sources it, one entry per
     /// file in `self.sourced_by(db)`, each recursively including what that file
     /// itself inherits. That recursion is what makes inheritance transitive
@@ -487,15 +473,6 @@ impl File {
         lower_load_context(db, load_context(db, self, view))
     }
 
-    /// Whether [`File::cross_file_layers`] uses the `R/`-directory fallback.
-    ///
-    /// Tracking avoids loader detection for every [`File::imports_at`] cursor
-    /// position.
-    #[salsa::tracked(returns(copy))]
-    pub(crate) fn has_fallback_context(self, db: &dyn Db, view: CollationView) -> bool {
-        load_context(db, self, view).kind.is_fallback()
-    }
-
     /// The collation members of `self`'s own `R/` directory, in load order.
     ///
     /// Path-based only. The scan-time resolver
@@ -536,7 +513,7 @@ fn build_inherited_layers(
         CollationView::Eager => source_offsets(db, source_site, file),
     };
 
-    let own_cross = source_site.own_layers(db, view);
+    let own_cross = source_site.cross_file_layers(db, view);
     let grandparents = source_site.inherited_layers(db, view);
 
     let (own_attach, exports_so_far) = match offsets.as_deref() {
@@ -579,9 +556,7 @@ fn build_inherited_layers(
     };
 
     enclosing.push(source_layer);
-    if let Some(own_cross) = own_cross {
-        enclosing.extend(own_cross.enclosing.iter().cloned());
-    }
+    enclosing.extend(own_cross.enclosing.iter().cloned());
     enclosing.extend(
         grandparents
             .iter()
@@ -594,16 +569,14 @@ fn build_inherited_layers(
             .iter()
             .flat_map(|site| site.layers.attaches.iter().cloned()),
     );
-    if let Some(own_cross) = own_cross {
-        attaches.extend(own_cross.attaches.iter().cloned());
-    }
+    attaches.extend(own_cross.attaches.iter().cloned());
 
     InheritedLayers {
         file: source_site,
         layers: CrossFileLayers {
             enclosing,
             attaches,
-            tail: own_cross.map_or(SearchPathTail::Default, |layers| layers.tail),
+            tail: own_cross.tail,
         },
     }
 }
@@ -672,6 +645,7 @@ pub(crate) fn lower_load_context(db: &dyn Db, context: LoadContext) -> CrossFile
         kind,
         visible_files,
         implicit_attaches,
+        loader: _,
     } = context;
 
     let mut enclosing: Vec<ImportLayer> = visible_files

crates/oak_db/src/load_context.rs

@@ -28,6 +28,32 @@ pub(crate) struct LoadContext {
     /// Packages attached by the loader, omitting packages unavailable in every
     /// root during lowering.
     pub implicit_attaches: Vec<&'static str>,
+
+    /// Which loader produced this context. Resolution ignores it; diagnostics
+    /// use it to name what already loads the file.
+    pub loader: Option<LoaderInfo>,
+}
+
+/// How a loader names itself in user reports. Whichever module recognises the
+/// loader supplies it, so a new one doesn't touch this file.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) struct LoaderInfo {
+    /// Sentence subject, e.g. `"testthat"`.
+    pub name: &'static str,
+
+    /// Completes "<name> already loads <loads>".
+    pub loads: &'static str,
+}
+
+const PACKAGE_LOADER: LoaderInfo = LoaderInfo {
+    name: "The package",
+    loads: "its `R/` files in collation order",
+};
+
+/// The loader that owns `file`, if one does. Reads only paths and source text,
+/// so it is safe to call while a semantic index is being built.
+pub(crate) fn loader(db: &dyn Db, file: File) -> Option<LoaderInfo> {
+    load_context(db, file, CollationView::Deferred).loader
 }
 
 /// Resolver context selected by the loader.
@@ -41,10 +67,6 @@ pub(crate) enum LoadKind {
 
     /// Use the default session search path and allow source-site inheritance.
     Session,
-
-    /// Session-like context inferred from a non-package `R/` layout. An explicit
-    /// source site supplies the actual context, so it replaces this fallback.
-    Fallback,
 }
 
 impl LoadKind {
@@ -54,15 +76,10 @@ impl LoadKind {
         matches!(self, LoadKind::Namespace(_))
     }
 
-    /// Whether source-site inheritance replaces this context instead of joining it.
-    pub fn is_fallback(self) -> bool {
-        matches!(self, LoadKind::Fallback)
-    }
-
     pub fn search_path_tail(self) -> SearchPathTail {
         match self {
             LoadKind::Namespace(_) => SearchPathTail::Base,
-            LoadKind::Session | LoadKind::Fallback => SearchPathTail::Default,
+            LoadKind::Session => SearchPathTail::Default,
         }
     }
 }
@@ -92,14 +109,6 @@ pub(crate) fn load_context(db: &dyn Db, file: File, view: CollationView) -> Load
         return context;
     }
 
-    // Only unowned `R/` files use directory collation. A package file excluded
-    // from `Collate:` has no loader and remains standalone.
-    if file.package(db).is_none() {
-        if let Some(context) = script_load_context(db, file, view) {
-            return context;
-        }
-    }
-
     standalone_load_context()
 }
 
@@ -118,19 +127,7 @@ fn package_load_context(db: &dyn Db, file: File, view: CollationView) -> Option<
         kind: LoadKind::Namespace(package),
         visible_files: visible_siblings(file, files, view, prefix_len),
         implicit_attaches: Vec::new(),
-    })
-}
-
-/// A non-package script in an `R/` directory, collated alphabetically, like a
-/// package `R/` directory without `Collate:`.
-fn script_load_context(db: &dyn Db, file: File, view: CollationView) -> Option<LoadContext> {
-    if !in_r_directory(file, db) {
-        return None;
-    }
-    Some(LoadContext {
-        kind: LoadKind::Fallback,
-        visible_files: collation_visible_files(db, file, view),
-        implicit_attaches: Vec::new(),
+        loader: Some(PACKAGE_LOADER),
     })
 }
 
@@ -140,6 +137,7 @@ fn standalone_load_context() -> LoadContext {
         kind: LoadKind::Session,
         visible_files: Vec::new(),
         implicit_attaches: Vec::new(),
+        loader: None,
     }
 }
 
@@ -176,8 +174,8 @@ pub(crate) fn visible_siblings(
     }
 }
 
-/// Whether `file` sits directly in an `R/` directory, which triggers collation
-/// for non-package scripts. The directory name is case-sensitive to match
+/// Whether `file` sits directly in an `R/` directory, which Shiny autoloads
+/// alongside its app. The directory name is case-sensitive to match
 /// [`load_context()`] and the package scanner.
 pub(crate) fn in_r_directory(file: File, db: &dyn Db) -> bool {
     let Some(path) = file.path(db).as_path() else {