From 1f8d2c8ce21db91decf530ba1d7743c4988e4d72 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 7 Sep 2026 15:49:14 +0200 Subject: [PATCH 1/8] Simplify comments --- crates/oak_db/src/file.rs | 34 ++++++++----------------- crates/oak_db/src/tests/workspace.rs | 38 +++------------------------- 2 files changed, 14 insertions(+), 58 deletions(-) diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index 81f14c836..710ff615d 100644 --- a/crates/oak_db/src/file.rs +++ b/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> { 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> { self.semantic_index(db) @@ -363,12 +355,6 @@ 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, diff --git a/crates/oak_db/src/tests/workspace.rs b/crates/oak_db/src/tests/workspace.rs index efbce6707..05f57f32c 100644 --- a/crates/oak_db/src/tests/workspace.rs +++ b/crates/oak_db/src/tests/workspace.rs @@ -58,8 +58,6 @@ 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 { let root = workspace_root(&*db, "proj"); let files: Vec = scripts @@ -334,25 +332,8 @@ fn test_testthat_file_depends_on_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. + // Query `c.R` first to re-enter `attached_packages()` through `R/` collation + // when `a.R` sources `b.R`. let mut db = TestDb::new(); register_library(&mut db, &["pkga", "pkgb", "pkgc"]); let files = workspace_with_scripts_files(&mut db, &[ @@ -361,20 +342,9 @@ fn test_r_directory_collation_with_a_source_call_does_not_panic() { ("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. + // The cycle rebuilds `a.R` and `b.R` with `NoopImportsResolver`, omitting + // their `library()` attachments. Only `c.R` remains a dependency. assert_eq!(all_package_dependencies_names(&db), vec!["pkgc"]); } From e7146beebed7b5e9cfbf12a8908f6d60594e65a0 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 7 Sep 2026 15:52:04 +0200 Subject: [PATCH 2/8] Warn in case of attach cycle --- crates/oak_db/src/file.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index 710ff615d..1c7bc639c 100644 --- a/crates/oak_db/src/file.rs +++ b/crates/oak_db/src/file.rs @@ -356,10 +356,14 @@ fn build_semantic_index_inner(file: File, db: &dyn Db) -> SemanticIndex { } fn attached_packages_cycle_result<'db>( - _db: &'db dyn Db, + db: &'db dyn Db, _id: salsa::Id, - _file: File, + file: File, ) -> Vec> { + log::warn!( + "Cyclic attaches detected at {}. Reporting no attached packages.", + file.path(db), + ); Vec::new() } From 25abd86c116a4ce8e374b20dce810f85b37fac2e Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 7 Sep 2026 16:33:52 +0200 Subject: [PATCH 3/8] Add missing tests --- crates/oak_db/src/tests/workspace.rs | 30 ++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/crates/oak_db/src/tests/workspace.rs b/crates/oak_db/src/tests/workspace.rs index 05f57f32c..84724e653 100644 --- a/crates/oak_db/src/tests/workspace.rs +++ b/crates/oak_db/src/tests/workspace.rs @@ -348,3 +348,33 @@ fn test_r_directory_collation_with_a_source_call_does_not_panic() { // their `library()` attachments. Only `c.R` remains a dependency. assert_eq!(all_package_dependencies_names(&db), vec!["pkgc"]); } + +#[test] +fn test_r_directory_collation_cycle_without_attaches_does_not_panic() { + // No `library()` call and no installed package anywhere: the cycle only + // needs an NSE-annotated call (`local()`) in the sourced predecessor to + // reach `cross_file_layers()` and re-enter `attached_packages()`. + let mut db = TestDb::new(); + let files = workspace_with_scripts_files(&mut db, &[ + ("R/a.R", "source(\"R/b.R\")\n"), + ("R/b.R", "local({ 1 })\n"), + ("R/c.R", "local({ 2 })\n"), + ]); + + let _ = files[2].used_packages(&db); + + assert_eq!(all_package_dependencies_names(&db), Vec::::new()); +} + +#[test] +fn test_single_r_file_does_not_panic() { + // Smoke test for https://github.com/posit-dev/positron/issues/15631#issuecomment-5437414045 + // We couldn't reproduce the reported panic but we keep that test as baseline. + let mut db = TestDb::new(); + let files = workspace_with_scripts_files(&mut db, &[("R/test.R", "x <- 1\n")]); + + let _ = all_package_dependencies_names(&db); + let _ = files[0].used_packages(&db); + let _ = files[0].imports(&db); + let _ = files[0].diagnostics(&db); +} From 7a65d1f04795e8b19825e58b5aec6ac608d295e6 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 7 Sep 2026 16:56:48 +0200 Subject: [PATCH 4/8] Remove redundant helper --- crates/oak_db/src/tests/workspace.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/crates/oak_db/src/tests/workspace.rs b/crates/oak_db/src/tests/workspace.rs index 84724e653..89cf10a62 100644 --- a/crates/oak_db/src/tests/workspace.rs +++ b/crates/oak_db/src/tests/workspace.rs @@ -54,11 +54,7 @@ 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); -} - -fn workspace_with_scripts_files(db: &mut TestDb, scripts: &[(&str, &str)]) -> Vec { +fn workspace_with_scripts(db: &mut TestDb, scripts: &[(&str, &str)]) -> Vec { let root = workspace_root(&*db, "proj"); let files: Vec = scripts .iter() @@ -336,7 +332,7 @@ fn test_r_directory_collation_with_a_source_call_does_not_panic() { // when `a.R` sources `b.R`. let mut db = TestDb::new(); register_library(&mut db, &["pkga", "pkgb", "pkgc"]); - let files = workspace_with_scripts_files(&mut db, &[ + let files = workspace_with_scripts(&mut db, &[ ("R/a.R", "library(pkga)\nsource(\"R/b.R\")\n"), ("R/b.R", "library(pkgb)\n"), ("R/c.R", "library(pkgc)\n"), @@ -355,7 +351,7 @@ fn test_r_directory_collation_cycle_without_attaches_does_not_panic() { // needs an NSE-annotated call (`local()`) in the sourced predecessor to // reach `cross_file_layers()` and re-enter `attached_packages()`. let mut db = TestDb::new(); - let files = workspace_with_scripts_files(&mut db, &[ + let files = workspace_with_scripts(&mut db, &[ ("R/a.R", "source(\"R/b.R\")\n"), ("R/b.R", "local({ 1 })\n"), ("R/c.R", "local({ 2 })\n"), @@ -371,7 +367,7 @@ fn test_single_r_file_does_not_panic() { // Smoke test for https://github.com/posit-dev/positron/issues/15631#issuecomment-5437414045 // We couldn't reproduce the reported panic but we keep that test as baseline. let mut db = TestDb::new(); - let files = workspace_with_scripts_files(&mut db, &[("R/test.R", "x <- 1\n")]); + let files = workspace_with_scripts(&mut db, &[("R/test.R", "x <- 1\n")]); let _ = all_package_dependencies_names(&db); let _ = files[0].used_packages(&db); From 33b8e91d64f8ca68640add3d544733c5b2f646d3 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 8 Sep 2026 09:56:48 +0200 Subject: [PATCH 5/8] Remove `R/` directory fallback auto-loading It can introduce unexpected cycles that trigger unactionable user lints. Instead we'll strive to model explicit loading mechanisms like we did for targets and shiny. --- crates/oak_db/src/file_imports.rs | 37 +-- crates/oak_db/src/load_context.rs | 36 +-- crates/oak_db/src/tests/contrib/shiny.rs | 28 +-- crates/oak_db/src/tests/file_diagnostics.rs | 19 +- crates/oak_db/src/tests/file_imports.rs | 239 +++----------------- crates/oak_db/src/tests/file_imports_at.rs | 23 +- crates/oak_db/src/tests/file_resolve.rs | 18 -- crates/oak_db/src/tests/workspace.rs | 28 +-- 8 files changed, 84 insertions(+), 344 deletions(-) diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 52950d1cd..6dd352afd 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/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, }, } } diff --git a/crates/oak_db/src/load_context.rs b/crates/oak_db/src/load_context.rs index 151e4f076..9c45b95b6 100644 --- a/crates/oak_db/src/load_context.rs +++ b/crates/oak_db/src/load_context.rs @@ -41,10 +41,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 +50,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 +83,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() } @@ -121,19 +104,6 @@ fn package_load_context(db: &dyn Db, file: File, view: CollationView) -> Option< }) } -/// 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 { - 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(), - }) -} - /// A file nothing else loads. It sees only its own attaches and the search path. fn standalone_load_context() -> LoadContext { LoadContext { @@ -176,8 +146,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 { diff --git a/crates/oak_db/src/tests/contrib/shiny.rs b/crates/oak_db/src/tests/contrib/shiny.rs index 834fb9493..692cb7d20 100644 --- a/crates/oak_db/src/tests/contrib/shiny.rs +++ b/crates/oak_db/src/tests/contrib/shiny.rs @@ -102,12 +102,12 @@ fn test_shiny_disable_autoload_drops_the_directory_but_keeps_global() { "Package(base)".to_string(), ]); - // Disabled autoload leaves `a.R` on plain `R/` collation, so it inherits - // neither `global.R` nor `shiny`. - assert_eq!(shape(&db, a.imports(&db)), vec![ - "File(_disable_autoload.R)".to_string(), - "Package(base)".to_string(), - ]); + // Disabled autoload drops `a.R` from Shiny's loader, so it becomes a + // standalone script that inherits neither `global.R` nor `shiny`. + assert_eq!( + shape(&db, a.imports(&db)), + vec!["Package(base)".to_string()] + ); } #[test] @@ -159,22 +159,6 @@ fn test_autoloaded_file_sees_global_and_the_implicit_shiny_attach() { ]); } -#[test] -fn test_r_directory_without_an_entry_point_ignores_global() { - let mut db = TestDb::new(); - install_packages(&mut db, &["base", "shiny"]); - let (_, files) = script_workspace(&mut db, &[ - ("ws/global.R", "cfg <- 1\n"), - ("ws/R/a.R", "a_val <- 1\n"), - ("ws/R/b.R", "b_val <- 2\n"), - ]); - - assert_eq!(shape(&db, files[1].imports(&db)), vec![ - "File(b.R)".to_string(), - "Package(base)".to_string(), - ]); -} - #[test] fn test_app_r_without_the_shiny_app_call_is_not_detected() { let mut db = TestDb::new(); diff --git a/crates/oak_db/src/tests/file_diagnostics.rs b/crates/oak_db/src/tests/file_diagnostics.rs index 3bd9fca3e..fecdd6c82 100644 --- a/crates/oak_db/src/tests/file_diagnostics.rs +++ b/crates/oak_db/src/tests/file_diagnostics.rs @@ -453,8 +453,9 @@ fn test_diagnostic_inherited_attach_shadows_a_callee() { #[test] fn test_diagnostic_no_inherited_shadow_when_neither_binding_is_effectful() { - // `zzz-shadow.R` and `main.R` both bind `library` to a plain function, so - // either winner leaves the call equally non-NSE. + // `zzz-shadow.R` (autoloaded alongside `helpers.R` by the Shiny app) and + // `main.R` both bind `library` to a plain function, so either winner leaves + // the call equally non-NSE. // // The shadow must be a successor. A predecessor prevents `library(dplyr)` // from reaching `semantic_calls()` during the eager scan. @@ -462,6 +463,7 @@ fn test_diagnostic_no_inherited_shadow_when_neither_binding_is_effectful() { install_package_binding(&mut db, "base", &["source", "library"]); install_package_binding(&mut db, "dplyr", &[]); let root = workspace_root(&db, "w"); + let app = new_file(&db, "w/app.R", "shinyApp(ui, server)\n"); let main = new_file( &db, "w/main.R", @@ -470,7 +472,8 @@ fn test_diagnostic_no_inherited_shadow_when_neither_binding_is_effectful() { let helpers_source = "library(dplyr)\n"; let helpers = new_file(&db, "w/R/helpers.R", helpers_source); let shadow = new_file(&db, "w/R/zzz-shadow.R", "library <- function(...) NULL\n"); - root.set_scripts(&mut db).to(vec![main, helpers, shadow]); + root.set_scripts(&mut db) + .to(vec![app, main, helpers, shadow]); db.workspace_roots().set_roots(&mut db).to(vec![root]); insta::assert_snapshot!(render( @@ -648,17 +651,19 @@ fn test_diagnostic_inherited_shadow_names_every_differing_context() { #[test] fn test_diagnostic_inherited_shadow_when_only_the_standalone_view_binds() { - // Standalone `helpers.R` resolves `library` through the later `R/` sibling. - // The inherited context excludes that collation fallback and reaches base's - // builtin without a scanned base root. + // Standalone `helpers.R` resolves `library` through its Shiny-autoloaded + // `R/` sibling. The `main.R` source-site context excludes that sibling and + // reaches base's builtin without a scanned base root. let mut db = TestDb::new(); install_package_binding(&mut db, "dplyr", &[]); let root = workspace_root(&db, "w"); + let app = new_file(&db, "w/app.R", "shinyApp(ui, server)\n"); let main = new_file(&db, "w/main.R", "source(\"R/helpers.R\")\n"); let helpers_source = "library(dplyr)\n"; let helpers = new_file(&db, "w/R/helpers.R", helpers_source); let shadow = new_file(&db, "w/R/zzz-shadow.R", "library <- function(...) NULL\n"); - root.set_scripts(&mut db).to(vec![main, helpers, shadow]); + root.set_scripts(&mut db) + .to(vec![app, main, helpers, shadow]); db.workspace_roots().set_roots(&mut db).to(vec![root]); insta::assert_snapshot!(render( diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index 75593d5a7..56ecc09da 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -398,33 +398,6 @@ fn test_cross_file_layers_memoized_across_effect_calls() { assert_eq!(db.executions("cross_file_layers"), 1); } -#[test] -fn test_script_r_directory_siblings_see_each_other() { - // Non-package scripts in an `R/` directory are collated alphabetically, - // exactly like a package `R/` with no `Collate:` (#15144, #14790). - let mut db = TestDb::new(); - let root = workspace_root(&db, "ws"); - let a = File::new( - &db, - file_path("ws/R/a.R"), - FileRevision::zero(), - Some("a_val <- 1\n".to_string()), - None, - ); - let b = File::new( - &db, - file_path("ws/R/b.R"), - FileRevision::zero(), - Some("b_val <- 2\n".to_string()), - None, - ); - root.set_scripts(&mut db).to(vec![a, b]); - db.workspace_roots().set_roots(&mut db).to(vec![root]); - - assert_eq!(shape(&db, a.imports(&db)), vec!["File(b.R)".to_string()]); - assert_eq!(shape(&db, b.imports(&db)), vec!["File(a.R)".to_string()]); -} - #[test] fn test_script_outside_r_directory_stays_standalone() { let mut db = TestDb::new(); @@ -492,107 +465,24 @@ fn test_package_owned_r_file_excluded_from_collate_stays_standalone() { assert_eq!(shape(&db, extra.imports(&db)), Vec::::new()); } -#[test] -fn test_script_r_directory_predecessor_attach_reaches_sibling() { - let mut db = TestDb::new(); - install_packages(&mut db, &["dplyr"]); - - let root = workspace_root(&db, "ws"); - let a = File::new( - &db, - file_path("ws/R/a.R"), - FileRevision::zero(), - Some("library(dplyr)\n".to_string()), - None, - ); - let b = File::new( - &db, - file_path("ws/R/b.R"), - FileRevision::zero(), - Some("x <- 1\n".to_string()), - None, - ); - root.set_scripts(&mut db).to(vec![a, b]); - db.workspace_roots().set_roots(&mut db).to(vec![root]); - - assert_eq!(shape(&db, b.imports(&db)), vec![ - "File(a.R)".to_string(), - "Package(dplyr)".to_string(), - ]); -} - -#[test] -fn test_script_r_directory_below_uses_full_default_search_path() { - // `SearchPathTail::Base` is for package code whose dependencies come from - // the NAMESPACE. A script needs the complete default search path. - let mut db = TestDb::new(); - install_packages(&mut db, &[ - "stats", - "graphics", - "grDevices", - "utils", - "datasets", - "methods", - "base", - ]); - - let root = workspace_root(&db, "ws"); - let a = File::new( - &db, - file_path("ws/R/a.R"), - FileRevision::zero(), - Some("x <- 1\n".to_string()), - None, - ); - root.set_scripts(&mut db).to(vec![a]); - db.workspace_roots().set_roots(&mut db).to(vec![root]); - - assert_eq!(shape(&db, a.imports(&db)), vec![ - "Package(stats)".to_string(), - "Package(graphics)".to_string(), - "Package(grDevices)".to_string(), - "Package(utils)".to_string(), - "Package(datasets)".to_string(), - "Package(methods)".to_string(), - "Package(base)".to_string(), - ]); -} - -#[test] -fn test_separate_r_directories_do_not_cross_collate() { - // Each `R/` directory collates independently, keyed on its parent path, - // so a monorepo with several `R/` folders doesn't cross-collate. - let mut db = TestDb::new(); - let root = workspace_root(&db, "ws"); - let a = File::new( - &db, - file_path("ws/one/R/a.R"), - FileRevision::zero(), - Some("a_val <- 1\n".to_string()), - None, - ); - let b = File::new( - &db, - file_path("ws/two/R/b.R"), - FileRevision::zero(), - Some("b_val <- 2\n".to_string()), - None, - ); - root.set_scripts(&mut db).to(vec![a, b]); - db.workspace_roots().set_roots(&mut db).to(vec![root]); - - assert_eq!(shape(&db, a.imports(&db)), Vec::::new()); - assert_eq!(shape(&db, b.imports(&db)), Vec::::new()); -} - #[test] fn test_cross_file_layers_backdates_on_unrelated_script_change() { // `collation_siblings` reads every workspace root's `scripts`, so a // script added anywhere forces it to re-execute. But the result filtered // to `a`/`b`'s own `R/` directory is unchanged, so salsa backdates it and // `cross_file_layers` never re-executes. + // + // `a`/`b` need a real loader to read `collation_siblings` at all, so the + // app's `R/` directory autoloads through Shiny. let mut db = TestDb::new(); let root = workspace_root(&db, "ws"); + let app = File::new( + &db, + file_path("ws/app.R"), + FileRevision::zero(), + Some("shinyApp(ui, server)\n".to_string()), + None, + ); let a = File::new( &db, file_path("ws/R/a.R"), @@ -607,7 +497,7 @@ fn test_cross_file_layers_backdates_on_unrelated_script_change() { Some("b_val <- 2\n".to_string()), None, ); - root.set_scripts(&mut db).to(vec![a, b]); + root.set_scripts(&mut db).to(vec![app, a, b]); db.workspace_roots().set_roots(&mut db).to(vec![root]); let _ = a.imports(&db); @@ -620,7 +510,7 @@ fn test_cross_file_layers_backdates_on_unrelated_script_change() { Some("z_val <- 1\n".to_string()), None, ); - root.set_scripts(&mut db).to(vec![a, b, elsewhere]); + root.set_scripts(&mut db).to(vec![app, a, b, elsewhere]); let _ = a.imports(&db); assert_eq!(db.executions("cross_file_layers"), 1); @@ -759,44 +649,19 @@ fn test_multiple_sourcing_files_appear_ordered_by_path() { } #[test] -fn test_inheritance_replaces_the_r_directory_fallback() { - // A non-package `R/` directory only implies collation. Because `main.R` - // explicitly sources `a.R` but not `b.R`, the inferred context is replaced - // and `b.R` drops out. +fn test_file_nobody_sources_keeps_its_own_cross_file_layers() { + // A Shiny app's `R/` directory really does autoload, so `b.R` (which + // nothing sources) still resolves through its own collation context. let mut db = TestDb::new(); + install_packages(&mut db, &["dplyr", "shiny", "base"]); let root = workspace_root(&db, "ws"); - let a = File::new( - &db, - file_path("ws/R/a.R"), - FileRevision::zero(), - Some("a_val <- 1\n".to_string()), - None, - ); - let b = File::new( - &db, - file_path("ws/R/b.R"), - FileRevision::zero(), - Some("b_val <- 2\n".to_string()), - None, - ); - let main = File::new( + let app = File::new( &db, - file_path("ws/main.R"), + file_path("ws/app.R"), FileRevision::zero(), - Some("source(\"R/a.R\")\n".to_string()), + Some("shinyApp(ui, server)\n".to_string()), None, ); - root.set_scripts(&mut db).to(vec![a, b, main]); - db.workspace_roots().set_roots(&mut db).to(vec![root]); - - assert_eq!(shape(&db, a.imports(&db)), vec!["File(main.R)".to_string()]); -} - -#[test] -fn test_file_nobody_sources_keeps_its_own_cross_file_layers() { - let mut db = TestDb::new(); - install_packages(&mut db, &["dplyr", "base"]); - let root = workspace_root(&db, "ws"); let a = File::new( &db, file_path("ws/R/a.R"), @@ -811,14 +676,15 @@ fn test_file_nobody_sources_keeps_its_own_cross_file_layers() { Some("x <- 1\n".to_string()), None, ); - root.set_scripts(&mut db).to(vec![a, b]); + root.set_scripts(&mut db).to(vec![app, a, b]); db.workspace_roots().set_roots(&mut db).to(vec![root]); - // Nobody sources `b.R`, so it keeps exactly the collation view - // `cross_file_layers` alone would give it. + // Nobody sources `b.R`, so it keeps exactly the collation view its Shiny + // autoload context gives it. assert_eq!(shape(&db, b.imports(&db)), vec![ "File(a.R)".to_string(), "Package(dplyr)".to_string(), + "Package(shiny)".to_string(), "Package(base)".to_string(), ]); } @@ -836,6 +702,16 @@ fn test_cross_file_layers_never_carries_inherited_layers() { install_packages(&mut db, &["base"]); let root = workspace_root(&db, "w"); + // A Shiny app makes `w/R/` a real loader, so `sibling` collates before + // `helpers.R` and gives the scan side a real `File` layer to tell apart + // from a `SourcingFile` one. + let app = File::new( + &db, + file_path("w/app.R"), + FileRevision::zero(), + Some("shinyApp(ui, server)\n".to_string()), + None, + ); let main = File::new( &db, file_path("w/main.R"), @@ -843,8 +719,6 @@ fn test_cross_file_layers_never_carries_inherited_layers() { Some("cfg <- 1\nsource(\"R/helpers.R\")\n".to_string()), None, ); - // Collates before `helpers.R`, so the scan side has a real `File` layer to - // tell apart from a `SourcingFile` one. let sibling = File::new( &db, file_path("w/R/a_sib.R"), @@ -860,7 +734,8 @@ fn test_cross_file_layers_never_carries_inherited_layers() { Some(helpers_source.to_string()), None, ); - root.set_scripts(&mut db).to(vec![main, sibling, helpers]); + root.set_scripts(&mut db) + .to(vec![app, main, sibling, helpers]); db.workspace_roots().set_roots(&mut db).to(vec![root]); // The read side narrows `main.R` to what had run by its `source()` call. @@ -1114,50 +989,6 @@ fn test_shiny_autoload_survives_an_explicit_source() { ]); } -#[test] -fn test_source_cycle_keeps_the_r_directory_fallback() { - // Cycle recovery leaves no inherited source sites, so `a.R` keeps its - // fallback collation context and still sees `b.R`. - let mut db = TestDb::new(); - install_packages(&mut db, &["base"]); - let root = workspace_root(&db, "ws"); - let a = File::new( - &db, - file_path("ws/R/a.R"), - FileRevision::zero(), - Some("source(\"R/c.R\")\n".to_string()), - None, - ); - let b = File::new( - &db, - file_path("ws/R/b.R"), - FileRevision::zero(), - Some("b_val <- 2\n".to_string()), - None, - ); - let c = File::new( - &db, - file_path("ws/R/c.R"), - FileRevision::zero(), - Some("source(\"R/a.R\")\n".to_string()), - None, - ); - root.set_scripts(&mut db).to(vec![a, b, c]); - db.workspace_roots().set_roots(&mut db).to(vec![root]); - - assert_eq!(a.sourced_by(&db), &Vec::::new()); - - // Per-sourcing-file resolution retains one fallback context when recovery - // removes every inherited source site. - let contexts = a.imports_by_sourcing_file(&db); - assert_eq!(contexts.len(), 1); - assert_eq!(shape(&db, &contexts[0]), vec![ - "File(c.R)".to_string(), - "File(b.R)".to_string(), - "Package(base)".to_string(), - ]); -} - #[test] fn test_mutual_sourcing_devolves_to_standalone_scripts() { // A mutual pair cycles `semantic_index` through `exports`, and the cycling diff --git a/crates/oak_db/src/tests/file_imports_at.rs b/crates/oak_db/src/tests/file_imports_at.rs index 7253e5d94..c2703c2ec 100644 --- a/crates/oak_db/src/tests/file_imports_at.rs +++ b/crates/oak_db/src/tests/file_imports_at.rs @@ -838,13 +838,16 @@ fn test_attach_in_a_function_body_is_visible_later_in_that_body() { } #[test] -fn test_script_r_directory_top_level_sees_only_alphabetic_predecessor() { +fn test_autoloaded_r_directory_top_level_sees_only_alphabetic_predecessor() { + // `app.R` turns `ws/R/` into a Shiny autoload directory, which is what + // collates these files. let mut db = TestDb::new(); let root = workspace_root(&db, "ws"); + let app = make_file(&mut db, "ws/app.R", "shinyApp(ui, server)\n"); let a = make_file(&mut db, "ws/R/a.R", "a_val <- 1\n"); let b_source = "x <- 1\n"; let b = make_file(&mut db, "ws/R/b.R", b_source); - root.set_scripts(&mut db).to(vec![a, b]); + root.set_scripts(&mut db).to(vec![app, a, b]); db.workspace_roots().set_roots(&mut db).to(vec![root]); // `b.R` is alphabetically after `a.R`, so `a.R` is its collation @@ -861,16 +864,20 @@ fn test_script_r_directory_top_level_sees_only_alphabetic_predecessor() { } #[test] -fn test_script_r_directory_collation_is_case_insensitive() { +fn test_autoloaded_r_directory_collation_is_case_insensitive() { // Non-package `R/` files use `list.files()` collation, so `a.R` precedes // `Z.R` in a UTF-8 session locale. Package installation's `LC_COLLATE=C` // reverses them. + // + // `app.R` turns `ws/R/` into a Shiny autoload directory, which is what + // collates these files. let mut db = TestDb::new(); let root = workspace_root(&db, "ws"); + let app = make_file(&mut db, "ws/app.R", "shinyApp(ui, server)\n"); let z_source = "z_val <- 1\n"; let z_file = make_file(&mut db, "ws/R/Z.R", z_source); let a_file = make_file(&mut db, "ws/R/a.R", "a_val <- 1\n"); - root.set_scripts(&mut db).to(vec![z_file, a_file]); + root.set_scripts(&mut db).to(vec![app, z_file, a_file]); db.workspace_roots().set_roots(&mut db).to(vec![root]); let offset = TextSize::from(z_source.len() as u32); @@ -878,20 +885,24 @@ fn test_script_r_directory_collation_is_case_insensitive() { } #[test] -fn test_script_r_directory_unplaced_file_still_sees_only_predecessors() { +fn test_autoloaded_r_directory_unplaced_file_still_sees_only_predecessors() { // A file the editor opened before the scanner placed it sits in // `OrphanRoot`, so it's missing from its own `collation_siblings`. Its // collation position comes from its basename anyway, so the top-level view // stays the strict predecessor prefix instead of widening to every sibling. + // + // `app.R` turns `ws/R/` into a Shiny autoload directory, which is what + // collates these files. let mut db = TestDb::new(); let root = workspace_root(&db, "ws"); + let app = make_file(&mut db, "ws/app.R", "shinyApp(ui, server)\n"); let a = make_file(&mut db, "ws/R/a.R", "a_val <- 1\n"); let b_source = "x <- 1\n"; let b = make_file(&mut db, "ws/R/b.R", b_source); let c = make_file(&mut db, "ws/R/c.R", "c_val <- 3\n"); // `b.R` is left out: unscanned, so it isn't a collation sibling of anyone. - root.set_scripts(&mut db).to(vec![a, c]); + root.set_scripts(&mut db).to(vec![app, a, c]); db.workspace_roots().set_roots(&mut db).to(vec![root]); let offset = TextSize::from(b_source.find('x').unwrap() as u32); diff --git a/crates/oak_db/src/tests/file_resolve.rs b/crates/oak_db/src/tests/file_resolve.rs index 368598d82..8b4b69b57 100644 --- a/crates/oak_db/src/tests/file_resolve.rs +++ b/crates/oak_db/src/tests/file_resolve.rs @@ -978,24 +978,6 @@ fn test_resolve_namespace_import_beats_attached_package() { assert_eq!(def.file(&db), dep_a_files[0]); } -#[test] -fn test_sourcing_a_script_replaces_fallback_collation_siblings() { - // `main.R` explicitly loads `R/a.R` but not `b.R`, replacing the inferred - // `R/`-directory collation context. `helper()` has no definition to resolve. - let mut db = TestDb::new(); - let files = setup_workspace(&mut db, &[ - ("w/main.R", "source(\"R/a.R\")\n"), - ("w/R/a.R", "f <- function() helper()\n"), - ("w/R/b.R", "helper <- function() 1\n"), - ]); - - assert!(files[1].resolve(&db, name(&db, "helper")).is_empty()); - - // Nothing sources `b.R`, so it retains the inferred collation context. - let def = resolve_one(&db, files[2], "f"); - assert_eq!(def.file(&db), files[1]); -} - #[test] fn test_shiny_global_resolves_reactive_but_not_r_directory_definitions() { // `loadSupport()` calls `require(shiny)` before sourcing anything, so an diff --git a/crates/oak_db/src/tests/workspace.rs b/crates/oak_db/src/tests/workspace.rs index 89cf10a62..c6a7fbc44 100644 --- a/crates/oak_db/src/tests/workspace.rs +++ b/crates/oak_db/src/tests/workspace.rs @@ -327,9 +327,10 @@ fn test_testthat_file_depends_on_testthat() { } #[test] -fn test_r_directory_collation_with_a_source_call_does_not_panic() { - // Query `c.R` first to re-enter `attached_packages()` through `R/` collation - // when `a.R` sources `b.R`. +fn test_source_call_between_loose_r_scripts_collects_every_attach() { + // Layout from https://github.com/posit-dev/positron/issues/15631. A loose + // `R/` script is standalone, so building `c.R`'s index never reaches `a.R` + // or `b.R` and no attach is lost to cycle recovery. let mut db = TestDb::new(); register_library(&mut db, &["pkga", "pkgb", "pkgc"]); let files = workspace_with_scripts(&mut db, &[ @@ -340,26 +341,9 @@ fn test_r_directory_collation_with_a_source_call_does_not_panic() { let _ = files[2].used_packages(&db); - // The cycle rebuilds `a.R` and `b.R` with `NoopImportsResolver`, omitting - // their `library()` attachments. Only `c.R` remains a dependency. - assert_eq!(all_package_dependencies_names(&db), vec!["pkgc"]); -} - -#[test] -fn test_r_directory_collation_cycle_without_attaches_does_not_panic() { - // No `library()` call and no installed package anywhere: the cycle only - // needs an NSE-annotated call (`local()`) in the sourced predecessor to - // reach `cross_file_layers()` and re-enter `attached_packages()`. - let mut db = TestDb::new(); - let files = workspace_with_scripts(&mut db, &[ - ("R/a.R", "source(\"R/b.R\")\n"), - ("R/b.R", "local({ 1 })\n"), - ("R/c.R", "local({ 2 })\n"), + assert_eq!(all_package_dependencies_names(&db), vec![ + "pkga", "pkgb", "pkgc" ]); - - let _ = files[2].used_packages(&db); - - assert_eq!(all_package_dependencies_names(&db), Vec::::new()); } #[test] From 1d808802ee268f60326196170c7a7208911c9ee1 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 8 Sep 2026 13:39:22 +0200 Subject: [PATCH 6/8] Mention file loaders in cycle diagnostics --- crates/oak_db/src/diagnostic.rs | 38 +++- crates/oak_db/src/file.rs | 2 +- crates/oak_db/src/file_imports.rs | 1 + crates/oak_db/src/load_context.rs | 28 +++ .../oak_db/src/load_context/contrib/shiny.rs | 9 + .../src/load_context/contrib/testthat.rs | 7 + crates/oak_db/src/tests/contrib/shiny.rs | 94 +++++++++ crates/oak_db/src/tests/contrib/testthat.rs | 64 ++++++ crates/oak_db/src/tests/file_diagnostics.rs | 195 ++++++++++++++++++ crates/oak_db/src/tests/file_imports.rs | 61 ++++++ ..._diagnostics__diagnostic_source_cycle.snap | 3 +- ...cycle_package_collation_sourcing_file.snap | 11 + ...rce_cycle_package_collation_successor.snap | 11 + ...c_source_cycle_reported_on_both_files.snap | 3 +- ...ce_cycle_shiny_autoload_sourcing_file.snap | 11 + ...source_cycle_shiny_autoload_successor.snap | 11 + ..._cycle_testthat_support_sourcing_file.snap | 11 + ...urce_cycle_testthat_support_successor.snap | 11 + ..._source_cycle_with_three_participants.snap | 11 + 19 files changed, 571 insertions(+), 11 deletions(-) create mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_sourcing_file.snap create mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_successor.snap create mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_sourcing_file.snap create mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_successor.snap create mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_sourcing_file.snap create mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_successor.snap create mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_with_three_participants.snap diff --git a/crates/oak_db/src/diagnostic.rs b/crates/oak_db/src/diagnostic.rs index 005fd71ab..3abc830b6 100644 --- a/crates/oak_db/src/diagnostic.rs +++ b/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,27 @@ 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) -> Diagnostic { + let cause = match loader { + Some(LoaderInfo { name, loads }) => { + format!("{name} already loads {loads}, so a `source()` call between them is redundant.") + }, + 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(), ) diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index 1c7bc639c..68ba96624 100644 --- a/crates/oak_db/src/file.rs +++ b/crates/oak_db/src/file.rs @@ -275,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)); diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 6dd352afd..643760308 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -645,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 = visible_files diff --git a/crates/oak_db/src/load_context.rs b/crates/oak_db/src/load_context.rs index 9c45b95b6..3fd240008 100644 --- a/crates/oak_db/src/load_context.rs +++ b/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, +} + +/// 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 " already 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 { + load_context(db, file, CollationView::Deferred).loader } /// Resolver context selected by the loader. @@ -101,6 +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(), + loader: Some(PACKAGE_LOADER), }) } @@ -110,6 +137,7 @@ fn standalone_load_context() -> LoadContext { kind: LoadKind::Session, visible_files: Vec::new(), implicit_attaches: Vec::new(), + loader: None, } } diff --git a/crates/oak_db/src/load_context/contrib/shiny.rs b/crates/oak_db/src/load_context/contrib/shiny.rs index ac6ddc536..bb314cf17 100644 --- a/crates/oak_db/src/load_context/contrib/shiny.rs +++ b/crates/oak_db/src/load_context/contrib/shiny.rs @@ -12,9 +12,15 @@ use crate::load_context::collation_visible_files; use crate::load_context::in_r_directory; use crate::load_context::LoadContext; use crate::load_context::LoadKind; +use crate::load_context::LoaderInfo; use crate::Db; use crate::File; +const LOADER: LoaderInfo = LoaderInfo { + name: "This Shiny app", + loads: "its `global.R` and `R/` files through `shiny::loadSupport()`", +}; + /// A file `shiny::runApp()` loads through `loadSupport()` or as an entry point. /// `None` means Shiny does not load it, including `R/` files disabled by /// `_disable_autoload.R`. Checking `R/` membership first keeps `R/app.R` in its @@ -49,6 +55,7 @@ fn autoload_context( kind: LoadKind::Session, visible_files, implicit_attaches: vec!["shiny"], + loader: Some(LOADER), } } @@ -66,6 +73,7 @@ fn entry_context(file: File, autoload: &[File]) -> LoadContext { .filter(|support| *support != file) .collect(), implicit_attaches: vec!["shiny"], + loader: Some(LOADER), } } @@ -74,6 +82,7 @@ fn global_context() -> LoadContext { kind: LoadKind::Session, visible_files: Vec::new(), implicit_attaches: vec!["shiny"], + loader: Some(LOADER), } } diff --git a/crates/oak_db/src/load_context/contrib/testthat.rs b/crates/oak_db/src/load_context/contrib/testthat.rs index 9a36f3a63..8b1ea8e1b 100644 --- a/crates/oak_db/src/load_context/contrib/testthat.rs +++ b/crates/oak_db/src/load_context/contrib/testthat.rs @@ -6,9 +6,15 @@ use crate::file_imports::CollationView; use crate::load_context::visible_siblings; use crate::load_context::LoadContext; use crate::load_context::LoadKind; +use crate::load_context::LoaderInfo; use crate::Db; use crate::File; +const LOADER: LoaderInfo = LoaderInfo { + name: "testthat", + loads: "`helper*.R` and `setup*.R` before your tests", +}; + /// A `tests/testthat/` file. It runs with the package loaded and `testthat` /// attached, after testthat has sourced the package's `helper*.R` and /// `setup*.R` files into the test environment. @@ -45,6 +51,7 @@ pub(crate) fn load_context(db: &dyn Db, file: File, view: CollationView) -> Opti kind: LoadKind::Namespace(package), visible_files, implicit_attaches: vec!["testthat"], + loader: Some(LOADER), }) } diff --git a/crates/oak_db/src/tests/contrib/shiny.rs b/crates/oak_db/src/tests/contrib/shiny.rs index 692cb7d20..3a830ba5f 100644 --- a/crates/oak_db/src/tests/contrib/shiny.rs +++ b/crates/oak_db/src/tests/contrib/shiny.rs @@ -419,3 +419,97 @@ fn test_app_rooted_at_an_r_directory_is_still_an_entry_point() { "Package(base)".to_string(), ]); } + +#[test] +fn test_backward_source_into_collation_successor_cycles() { + // `a.R` precedes `b.R` in the autoload collation but also sources it. + // Resolving `b.R`'s own `library(pkgb)` reads `a.R`'s + // `attached_packages` (a collation predecessor's search-path + // contribution), which rebuilds `a.R`'s index, which resolves its + // `source("R/b.R")` by reading `b.R`'s `exports`, which needs `b.R`'s + // index again: a real salsa cycle, through `cross_file_layers` and + // `attached_packages` rather than through two `source()` calls facing + // each other. + // + // `semantic_index`'s `FallbackImmediate` recovery degrades every + // participant, not just the file salsa re-entered, so both files rebuild + // under `NoopImportsResolver`: neither `library()` call is recognized as + // an attach, and `a.R`'s `source()` call is not recognized as effectful + // either, so the edge itself disappears. + let mut db = TestDb::new(); + install_packages(&mut db, &["base", "shiny", "pkga", "pkgb"]); + let (_, files) = script_workspace(&mut db, &[ + ("ws/app.R", "shinyApp(ui, server)\n"), + ("ws/R/a.R", "library(pkga)\nsource(\"R/b.R\")\n"), + ("ws/R/b.R", "library(pkgb)\n"), + ]); + let (a, b) = (files[1], files[2]); + + assert_eq!(shape(&db, a.imports(&db)), vec![ + "File(b.R)".to_string(), + "Package(shiny)".to_string(), + "Package(base)".to_string(), + ]); + assert_eq!(shape(&db, b.imports(&db)), vec![ + "File(a.R)".to_string(), + "Package(shiny)".to_string(), + "Package(base)".to_string(), + ]); + assert!(a.sourced_by(&db).is_empty()); + assert!(b.sourced_by(&db).is_empty()); + + // Both files carry a `SourceCycle` diagnostic with the same generic + // message, even though `b.R` has no `source()` call of its own: it is + // degraded collaterally by the recovery, not because it takes part in + // mutual sourcing. The message's premise ("mutual `source()` calls") + // doesn't actually hold for this shape. + assert_eq!(a.diagnostics(&db).len(), 1); + assert_eq!(b.diagnostics(&db).len(), 1); + assert_eq!( + a.diagnostics(&db)[0].message(), + "This file is part of a cycle in how the project's files load each other.\n\ + This Shiny app already loads its `global.R` and `R/` files through \ + `shiny::loadSupport()`, so a `source()` call between them is redundant.\n\ + Language analysis will be incomplete until the cycle is resolved." + ); + assert_eq!( + a.diagnostics(&db)[0].message(), + b.diagnostics(&db)[0].message() + ); +} + +#[test] +fn test_forward_source_into_collation_predecessor_does_not_cycle() { + // `b.R` sources its own predecessor `a.R`, which has already loaded by + // the time `b.R` runs, so this is not a back edge and must not cycle. + let mut db = TestDb::new(); + install_packages(&mut db, &["base", "shiny"]); + let (_, files) = script_workspace(&mut db, &[ + ("ws/app.R", "shinyApp(ui, server)\n"), + ("ws/R/a.R", "a_val <- 1\n"), + ("ws/R/b.R", "b_val <- 2\nsource(\"R/a.R\")\n"), + ]); + let (a, b) = (files[1], files[2]); + + assert!(a.diagnostics(&db).is_empty()); + assert!(b.diagnostics(&db).is_empty()); + assert_eq!(a.sourced_by(&db), &vec![b]); + + // Known gap. `a.R`'s own collation view contributes `File(b.R)`, and the + // context inherited from `b.R` contributes `File(b.R)` again plus + // `File(a.R)`, since `a.R` is `b.R`'s only sibling. Harmless today because + // own bindings resolve before the import list is consulted. + assert_eq!(shape(&db, a.imports(&db)), vec![ + "File(b.R)".to_string(), + "File(b.R)".to_string(), + "File(a.R)".to_string(), + "Package(shiny)".to_string(), + "Package(shiny)".to_string(), + "Package(base)".to_string(), + ]); + assert_eq!(shape(&db, b.imports(&db)), vec![ + "File(a.R)".to_string(), + "Package(shiny)".to_string(), + "Package(base)".to_string(), + ]); +} diff --git a/crates/oak_db/src/tests/contrib/testthat.rs b/crates/oak_db/src/tests/contrib/testthat.rs index 892d066a6..e2108db4c 100644 --- a/crates/oak_db/src/tests/contrib/testthat.rs +++ b/crates/oak_db/src/tests/contrib/testthat.rs @@ -284,3 +284,67 @@ fn test_testthat_file_ignores_source_sites() { "Package(base)".to_string(), ]); } + +#[test] +fn test_helper_backward_source_into_setup_cycles() { + // `helper*.R` sorts before `setup*.R`, so `setup.R` is `helper.R`'s + // collation successor, but `helper.R` also sources it explicitly. Same + // shape and same outcome as the Shiny and package `R/` cases: resolving + // `setup.R`'s own `library(pkgb)` reads `helper.R`'s `attached_packages` + // as a support-file predecessor, which cycles back through + // `helper.R`'s own `source()` resolution. Both files degrade, both + // attaches are lost, and both carry the same `SourceCycle` diagnostic + // even though `setup.R` has no `source()` call of its own. + let mut db = TestDb::new(); + install_packages(&mut db, &["testthat", "base", "pkga", "pkgb"]); + + let workspace = workspace_root(&db, "w"); + let pkg = Package::new( + &db, + file_path("w/pkg/DESCRIPTION"), + "pkg".to_string(), + FileRevision::zero(), + FileRevision::zero(), + None, + None, + Vec::new(), + Vec::new(), + ); + let helper = File::new( + &db, + file_path("w/pkg/tests/testthat/helper.R"), + FileRevision::zero(), + Some("library(pkga)\nsource(\"pkg/tests/testthat/setup.R\")\n".to_string()), + Some(pkg), + ); + let setup = File::new( + &db, + file_path("w/pkg/tests/testthat/setup.R"), + FileRevision::zero(), + Some("library(pkgb)\n".to_string()), + Some(pkg), + ); + pkg.set_scripts(&mut db).to(vec![helper, setup]); + workspace.set_packages(&mut db).to(vec![pkg]); + db.workspace_roots().set_roots(&mut db).to(vec![workspace]); + + assert_eq!(shape(&db, helper.imports(&db)), vec![ + "File(setup.R)".to_string(), + "Package(testthat)".to_string(), + "Package(base)".to_string(), + ]); + assert_eq!(shape(&db, setup.imports(&db)), vec![ + "File(helper.R)".to_string(), + "Package(testthat)".to_string(), + "Package(base)".to_string(), + ]); + assert!(helper.sourced_by(&db).is_empty()); + assert!(setup.sourced_by(&db).is_empty()); + + assert_eq!(helper.diagnostics(&db).len(), 1); + assert_eq!(setup.diagnostics(&db).len(), 1); + assert_eq!( + helper.diagnostics(&db)[0].message(), + setup.diagnostics(&db)[0].message() + ); +} diff --git a/crates/oak_db/src/tests/file_diagnostics.rs b/crates/oak_db/src/tests/file_diagnostics.rs index fecdd6c82..2c9a2a312 100644 --- a/crates/oak_db/src/tests/file_diagnostics.rs +++ b/crates/oak_db/src/tests/file_diagnostics.rs @@ -748,3 +748,198 @@ fn cyclic_pair(db: &mut TestDb, a_source: &str, b_source: &str) -> (File, File) db.workspace_roots().set_roots(db).to(vec![root]); (a, b) } + +#[test] +fn test_diagnostic_source_cycle_with_three_participants() { + // `a.R` -> `b.R` -> `c.R` -> `a.R`. Recovery isn't special-cased to + // pairs: every file on the ring gets its own `SourceCycle` diagnostic. + let mut db = TestDb::new(); + let root = workspace_root(&db, "w"); + let a_source = "source(\"b.R\")\n"; + let b_source = "source(\"c.R\")\n"; + let c_source = "source(\"a.R\")\n"; + let a = new_file(&db, "w/a.R", a_source); + let b = new_file(&db, "w/b.R", b_source); + let c = new_file(&db, "w/c.R", c_source); + root.set_scripts(&mut db).to(vec![a, b, c]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + assert_eq!(a.diagnostics(&db).len(), 1); + assert_eq!(b.diagnostics(&db).len(), 1); + assert_eq!(c.diagnostics(&db).len(), 1); + insta::assert_snapshot!(render("w/a.R", a_source, a.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_source_cycle_not_reported_on_a_caller_outside_the_cycle() { + // `main.R` sources into the cyclic `a.R` / `b.R` pair but isn't itself a + // cycle participant. `FallbackImmediate` fans out to the files salsa + // actually re-entered, not to every caller reachable from them, so + // `main.R` stays clean while `a.R` and `b.R` each get one diagnostic. + let mut db = TestDb::new(); + let root = workspace_root(&db, "w"); + let main_source = "source(\"a.R\")\n"; + let a_source = "source(\"b.R\")\n"; + let b_source = "source(\"a.R\")\n"; + let main = new_file(&db, "w/main.R", main_source); + let a = new_file(&db, "w/a.R", a_source); + let b = new_file(&db, "w/b.R", b_source); + root.set_scripts(&mut db).to(vec![main, a, b]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + assert!(main.diagnostics(&db).is_empty()); + assert_eq!(a.diagnostics(&db).len(), 1); + assert_eq!(b.diagnostics(&db).len(), 1); +} + +const BACK_EDGE_SOURCING: &str = "library(pkga)\nsource(\"R/b.R\")\n"; +const BACK_EDGE_SUCCESSOR: &str = "library(pkgb)\n"; +const PKG_BACK_EDGE_SOURCING: &str = "library(pkga)\nsource(\"pkg/R/b.R\")\n"; +const TESTTHAT_BACK_EDGE_SOURCING: &str = "library(pkga)\nsource(\"pkg/tests/testthat/setup.R\")\n"; + +#[test] +fn test_diagnostic_source_cycle_shiny_autoload_sourcing_file() { + // `R/a.R` is the file that actually writes the `source()` call, so the + // "mutual `source()` calls" wording is at least half true here. + let mut db = TestDb::new(); + let (a, _b) = shiny_back_edge(&mut db); + + insta::assert_snapshot!(render("w/R/a.R", BACK_EDGE_SOURCING, a.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_source_cycle_shiny_autoload_successor() { + // `R/b.R` contains no `source()` call at all. It is degraded because + // `FallbackImmediate` hands every cycle participant its fallback, so the + // message's premise is simply false for this file. + let mut db = TestDb::new(); + let (_a, b) = shiny_back_edge(&mut db); + + insta::assert_snapshot!(render("w/R/b.R", BACK_EDGE_SUCCESSOR, b.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_source_cycle_package_collation_sourcing_file() { + let mut db = TestDb::new(); + let (a, _b) = package_back_edge(&mut db); + + insta::assert_snapshot!(render( + "w/pkg/R/a.R", + PKG_BACK_EDGE_SOURCING, + a.diagnostics(&db) + )); +} + +#[test] +fn test_diagnostic_source_cycle_package_collation_successor() { + // `source()` inside a package's `R/` is already wrong on its own, and + // `b.R` carries the same generic message without having written one. + let mut db = TestDb::new(); + let (_a, b) = package_back_edge(&mut db); + + insta::assert_snapshot!(render( + "w/pkg/R/b.R", + BACK_EDGE_SUCCESSOR, + b.diagnostics(&db) + )); +} + +#[test] +fn test_diagnostic_source_cycle_testthat_support_sourcing_file() { + let mut db = TestDb::new(); + let (helper, _setup) = testthat_back_edge(&mut db); + + insta::assert_snapshot!(render( + "w/pkg/tests/testthat/helper.R", + TESTTHAT_BACK_EDGE_SOURCING, + helper.diagnostics(&db) + )); +} + +#[test] +fn test_diagnostic_source_cycle_testthat_support_successor() { + // testthat already sources `setup.R` after `helper.R`, so the explicit + // call is a double load rather than a cycle the user wrote. + let mut db = TestDb::new(); + let (_helper, setup) = testthat_back_edge(&mut db); + + insta::assert_snapshot!(render( + "w/pkg/tests/testthat/setup.R", + BACK_EDGE_SUCCESSOR, + setup.diagnostics(&db) + )); +} + +/// A Shiny app whose autoloaded `R/a.R` sources its collation successor +/// `R/b.R`. Returns the two `R/` files, sourcing file first. +fn shiny_back_edge(db: &mut TestDb) -> (File, File) { + install_packages(db, &["base", "shiny", "pkga", "pkgb"]); + let root = workspace_root(&*db, "w"); + let app = new_file(&*db, "w/app.R", "shinyApp(ui, server)\n"); + let a = new_file(&*db, "w/R/a.R", BACK_EDGE_SOURCING); + let b = new_file(&*db, "w/R/b.R", BACK_EDGE_SUCCESSOR); + root.set_scripts(db).to(vec![app, a, b]); + db.workspace_roots().set_roots(db).to(vec![root]); + (a, b) +} + +/// The same back edge inside a package's `R/` collation. +fn package_back_edge(db: &mut TestDb) -> (File, File) { + install_packages(db, &["base", "pkga", "pkgb"]); + let workspace = workspace_root(&*db, "w"); + let pkg = back_edge_package(&*db); + let a = package_file(&*db, "w/pkg/R/a.R", PKG_BACK_EDGE_SOURCING, pkg); + let b = package_file(&*db, "w/pkg/R/b.R", BACK_EDGE_SUCCESSOR, pkg); + pkg.set_files(db).to(vec![a, b]); + workspace.set_packages(db).to(vec![pkg]); + db.workspace_roots().set_roots(db).to(vec![workspace]); + (a, b) +} + +/// The same back edge across testthat support files, which sort `helper.R` +/// before `setup.R`. +fn testthat_back_edge(db: &mut TestDb) -> (File, File) { + install_packages(db, &["testthat", "base", "pkga", "pkgb"]); + let workspace = workspace_root(&*db, "w"); + let pkg = back_edge_package(&*db); + let helper = package_file( + &*db, + "w/pkg/tests/testthat/helper.R", + TESTTHAT_BACK_EDGE_SOURCING, + pkg, + ); + let setup = package_file( + &*db, + "w/pkg/tests/testthat/setup.R", + BACK_EDGE_SUCCESSOR, + pkg, + ); + pkg.set_scripts(db).to(vec![helper, setup]); + workspace.set_packages(db).to(vec![pkg]); + db.workspace_roots().set_roots(db).to(vec![workspace]); + (helper, setup) +} + +fn back_edge_package(db: &TestDb) -> Package { + Package::new( + db, + file_path("w/pkg/DESCRIPTION"), + "pkg".to_string(), + FileRevision::zero(), + FileRevision::zero(), + None, + Some(Namespace::default()), + Vec::new(), + Vec::new(), + ) +} + +fn package_file(db: &TestDb, path: &str, contents: &str, pkg: Package) -> File { + File::new( + db, + file_path(path), + FileRevision::zero(), + Some(contents.to_string()), + Some(pkg), + ) +} diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index 56ecc09da..813329157 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -1082,6 +1082,67 @@ fn test_qualified_mutual_sourcing_records_sites_but_no_edges() { ); } +#[test] +fn test_package_backward_source_into_collation_successor_cycles() { + // Same shape as Shiny's `R/` autoload, but through package collation: + // `a.R` precedes `b.R` and also sources it. `LoadKind::Namespace` fixes + // load order, so unlike the Shiny case there's no source-site + // inheritance to muddy the shape, but the cycle through + // `cross_file_layers` -> `attached_packages` -> `semantic_index` is the + // same, and recovery degrades both files identically: both attaches are + // lost, the source edge itself disappears, and both carry the same + // `SourceCycle` diagnostic even though only `a.R` calls `source()`. + let mut db = TestDb::new(); + install_packages(&mut db, &["base", "pkga", "pkgb"]); + let workspace = workspace_root(&db, "w"); + let pkg = Package::new( + &db, + file_path("w/pkg/DESCRIPTION"), + "pkg".to_string(), + FileRevision::zero(), + FileRevision::zero(), + None, + Some(Namespace::default()), + Vec::new(), + Vec::new(), + ); + let a = File::new( + &db, + file_path("w/pkg/R/a.R"), + FileRevision::zero(), + Some("library(pkga)\nsource(\"pkg/R/b.R\")\n".to_string()), + Some(pkg), + ); + let b = File::new( + &db, + file_path("w/pkg/R/b.R"), + FileRevision::zero(), + Some("library(pkgb)\n".to_string()), + Some(pkg), + ); + pkg.set_files(&mut db).to(vec![a, b]); + workspace.set_packages(&mut db).to(vec![pkg]); + db.workspace_roots().set_roots(&mut db).to(vec![workspace]); + + assert_eq!(shape(&db, a.imports(&db)), vec![ + "File(b.R)".to_string(), + "Package(base)".to_string(), + ]); + assert_eq!(shape(&db, b.imports(&db)), vec![ + "File(a.R)".to_string(), + "Package(base)".to_string(), + ]); + assert!(a.sourced_by(&db).is_empty()); + assert!(b.sourced_by(&db).is_empty()); + + assert_eq!(a.diagnostics(&db).len(), 1); + assert_eq!(b.diagnostics(&db).len(), 1); + assert_eq!( + a.diagnostics(&db)[0].message(), + b.diagnostics(&db)[0].message() + ); +} + #[test] fn test_package_r_file_ignores_source_sites() { let mut db = TestDb::new(); diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle.snap index 706530c48..d89ab8d45 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle.snap @@ -2,7 +2,8 @@ source: crates/oak_db/src/tests/file_diagnostics.rs expression: "render(\"w/a.R\", a_source, a.diagnostics(&db))" --- -warning[source-cycle]: This file takes part in a cycle of mutual `source()` calls. +warning[source-cycle]: This file is part of a cycle in how the project's files load each other. + 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. Language analysis will be incomplete until the cycle is resolved. --> w/a.R:1:1 | diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_sourcing_file.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_sourcing_file.snap new file mode 100644 index 000000000..216011706 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_sourcing_file.snap @@ -0,0 +1,11 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"w/pkg/R/a.R\", PKG_BACK_EDGE_SOURCING, a.diagnostics(&db))" +--- +warning[source-cycle]: This file is part of a cycle in how the project's files load each other. + The package already loads its `R/` files in collation order, so a `source()` call between them is redundant. + Language analysis will be incomplete until the cycle is resolved. + --> w/pkg/R/a.R:1:1 + | +1 | library(pkga) + | ^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_successor.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_successor.snap new file mode 100644 index 000000000..f8cc0a97a --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_successor.snap @@ -0,0 +1,11 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"w/pkg/R/b.R\", BACK_EDGE_SUCCESSOR, b.diagnostics(&db))" +--- +warning[source-cycle]: This file is part of a cycle in how the project's files load each other. + The package already loads its `R/` files in collation order, so a `source()` call between them is redundant. + Language analysis will be incomplete until the cycle is resolved. + --> w/pkg/R/b.R:1:1 + | +1 | library(pkgb) + | ^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_reported_on_both_files.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_reported_on_both_files.snap index 57908d54f..6b0b9f729 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_reported_on_both_files.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_reported_on_both_files.snap @@ -2,7 +2,8 @@ source: crates/oak_db/src/tests/file_diagnostics.rs expression: "render(\"w/b.R\", b_source, b.diagnostics(&db))" --- -warning[source-cycle]: This file takes part in a cycle of mutual `source()` calls. +warning[source-cycle]: This file is part of a cycle in how the project's files load each other. + 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. Language analysis will be incomplete until the cycle is resolved. --> w/b.R:1:1 | diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_sourcing_file.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_sourcing_file.snap new file mode 100644 index 000000000..1c76bc085 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_sourcing_file.snap @@ -0,0 +1,11 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"w/R/a.R\", BACK_EDGE_SOURCING, a.diagnostics(&db))" +--- +warning[source-cycle]: This file is part of a cycle in how the project's files load each other. + This Shiny app already loads its `global.R` and `R/` files through `shiny::loadSupport()`, so a `source()` call between them is redundant. + Language analysis will be incomplete until the cycle is resolved. + --> w/R/a.R:1:1 + | +1 | library(pkga) + | ^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_successor.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_successor.snap new file mode 100644 index 000000000..f62279b43 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_successor.snap @@ -0,0 +1,11 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"w/R/b.R\", BACK_EDGE_SUCCESSOR, b.diagnostics(&db))" +--- +warning[source-cycle]: This file is part of a cycle in how the project's files load each other. + This Shiny app already loads its `global.R` and `R/` files through `shiny::loadSupport()`, so a `source()` call between them is redundant. + Language analysis will be incomplete until the cycle is resolved. + --> w/R/b.R:1:1 + | +1 | library(pkgb) + | ^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_sourcing_file.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_sourcing_file.snap new file mode 100644 index 000000000..a50d0922e --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_sourcing_file.snap @@ -0,0 +1,11 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"w/pkg/tests/testthat/helper.R\", TESTTHAT_BACK_EDGE_SOURCING,\nhelper.diagnostics(&db))" +--- +warning[source-cycle]: This file is part of a cycle in how the project's files load each other. + testthat already loads `helper*.R` and `setup*.R` before your tests, so a `source()` call between them is redundant. + Language analysis will be incomplete until the cycle is resolved. + --> w/pkg/tests/testthat/helper.R:1:1 + | +1 | library(pkga) + | ^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_successor.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_successor.snap new file mode 100644 index 000000000..c2d38f263 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_successor.snap @@ -0,0 +1,11 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"w/pkg/tests/testthat/setup.R\", BACK_EDGE_SUCCESSOR,\nsetup.diagnostics(&db))" +--- +warning[source-cycle]: This file is part of a cycle in how the project's files load each other. + testthat already loads `helper*.R` and `setup*.R` before your tests, so a `source()` call between them is redundant. + Language analysis will be incomplete until the cycle is resolved. + --> w/pkg/tests/testthat/setup.R:1:1 + | +1 | library(pkgb) + | ^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_with_three_participants.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_with_three_participants.snap new file mode 100644 index 000000000..d89ab8d45 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_with_three_participants.snap @@ -0,0 +1,11 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"w/a.R\", a_source, a.diagnostics(&db))" +--- +warning[source-cycle]: This file is part of a cycle in how the project's files load each other. + 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. + Language analysis will be incomplete until the cycle is resolved. + --> w/a.R:1:1 + | +1 | source("b.R") + | ^ From 693f70bc7a6e3a286c4fbe0d92ac8b3d5f90e9b4 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 8 Sep 2026 14:09:23 +0200 Subject: [PATCH 7/8] More cycle tests --- crates/oak_db/src/tests/file_diagnostics.rs | 155 ++++++++++++++++++++ crates/oak_db/src/tests/file_imports.rs | 122 +++++++++++++++ 2 files changed, 277 insertions(+) diff --git a/crates/oak_db/src/tests/file_diagnostics.rs b/crates/oak_db/src/tests/file_diagnostics.rs index 2c9a2a312..d5d262ff8 100644 --- a/crates/oak_db/src/tests/file_diagnostics.rs +++ b/crates/oak_db/src/tests/file_diagnostics.rs @@ -7,6 +7,7 @@ use salsa::Setter; use stdext::SortedVec; use crate::tests::diagnostic_render::render; +use crate::tests::file_imports::shape; use crate::tests::resolver::install_packages; use crate::tests::test_db::file_path; use crate::tests::test_db::library_root; @@ -870,6 +871,160 @@ fn test_diagnostic_source_cycle_testthat_support_successor() { )); } +/// Query `first`'s diagnostics before anything else touches `db`, then read +/// `second`'s diagnostics and both files' `imports()`. Returns rendered +/// diagnostics and import shapes for `first` then `second`, so callers can +/// compare across entry orders. +fn probe_entry_order( + db: &TestDb, + first: File, + first_path: &str, + first_source: &str, + second: File, + second_path: &str, + second_source: &str, +) -> (String, String, Vec, Vec) { + let first_render = render(first_path, first_source, first.diagnostics(db)); + let second_render = render(second_path, second_source, second.diagnostics(db)); + let first_shape = shape(db, first.imports(db)); + let second_shape = shape(db, second.imports(db)); + (first_render, second_render, first_shape, second_shape) +} + +#[test] +fn test_diagnostic_source_cycle_shiny_autoload_entry_order_does_not_matter() { + // PR 1393's central finding: which query salsa re-enters, and therefore + // which recovery handler fires, depends on which file is queried first. + // Build two separate databases, enter one via the sourcing file and the + // other via its successor, and require the same outcome either way. + let mut sourcing_first = TestDb::new(); + let (a1, b1) = shiny_back_edge(&mut sourcing_first); + let (a1_render, b1_render, a1_shape, b1_shape) = probe_entry_order( + &sourcing_first, + a1, + "w/R/a.R", + BACK_EDGE_SOURCING, + b1, + "w/R/b.R", + BACK_EDGE_SUCCESSOR, + ); + + let mut successor_first = TestDb::new(); + let (a2, b2) = shiny_back_edge(&mut successor_first); + let (b2_render, a2_render, b2_shape, a2_shape) = probe_entry_order( + &successor_first, + b2, + "w/R/b.R", + BACK_EDGE_SUCCESSOR, + a2, + "w/R/a.R", + BACK_EDGE_SOURCING, + ); + + assert_eq!(a1_render, a2_render); + assert_eq!(b1_render, b2_render); + assert_eq!(a1_shape, a2_shape); + assert_eq!(b1_shape, b2_shape); +} + +#[test] +fn test_diagnostic_source_cycle_package_collation_entry_order_does_not_matter() { + let mut sourcing_first = TestDb::new(); + let (a1, b1) = package_back_edge(&mut sourcing_first); + let (a1_render, b1_render, a1_shape, b1_shape) = probe_entry_order( + &sourcing_first, + a1, + "w/pkg/R/a.R", + PKG_BACK_EDGE_SOURCING, + b1, + "w/pkg/R/b.R", + BACK_EDGE_SUCCESSOR, + ); + + let mut successor_first = TestDb::new(); + let (a2, b2) = package_back_edge(&mut successor_first); + let (b2_render, a2_render, b2_shape, a2_shape) = probe_entry_order( + &successor_first, + b2, + "w/pkg/R/b.R", + BACK_EDGE_SUCCESSOR, + a2, + "w/pkg/R/a.R", + PKG_BACK_EDGE_SOURCING, + ); + + assert_eq!(a1_render, a2_render); + assert_eq!(b1_render, b2_render); + assert_eq!(a1_shape, a2_shape); + assert_eq!(b1_shape, b2_shape); +} + +#[test] +fn test_diagnostic_source_cycle_testthat_support_entry_order_does_not_matter() { + let mut sourcing_first = TestDb::new(); + let (helper1, setup1) = testthat_back_edge(&mut sourcing_first); + let (helper1_render, setup1_render, helper1_shape, setup1_shape) = probe_entry_order( + &sourcing_first, + helper1, + "w/pkg/tests/testthat/helper.R", + TESTTHAT_BACK_EDGE_SOURCING, + setup1, + "w/pkg/tests/testthat/setup.R", + BACK_EDGE_SUCCESSOR, + ); + + let mut successor_first = TestDb::new(); + let (helper2, setup2) = testthat_back_edge(&mut successor_first); + let (setup2_render, helper2_render, setup2_shape, helper2_shape) = probe_entry_order( + &successor_first, + setup2, + "w/pkg/tests/testthat/setup.R", + BACK_EDGE_SUCCESSOR, + helper2, + "w/pkg/tests/testthat/helper.R", + TESTTHAT_BACK_EDGE_SOURCING, + ); + + assert_eq!(helper1_render, helper2_render); + assert_eq!(setup1_render, setup2_render); + assert_eq!(helper1_shape, helper2_shape); + assert_eq!(setup1_shape, setup2_shape); +} + +#[test] +fn test_diagnostic_source_cycle_confined_to_its_own_workspace_root() { + // Two workspace roots: `w` runs a Shiny app with a back-edge cycle in its + // `R/` autoload, `h` is an unrelated healthy workspace. Recovery degrades + // every cycle participant, and `h`'s file is not one, so it must come out + // clean even though both roots share the same `WorkspaceRoots` and + // `LibraryRoots` inputs. + let mut db = TestDb::new(); + install_packages(&mut db, &["base", "shiny", "pkga", "pkgb", "pkgc"]); + + let cycling = workspace_root(&db, "w"); + let app = new_file(&db, "w/app.R", "shinyApp(ui, server)\n"); + let a = new_file(&db, "w/R/a.R", BACK_EDGE_SOURCING); + let b = new_file(&db, "w/R/b.R", BACK_EDGE_SUCCESSOR); + cycling.set_scripts(&mut db).to(vec![app, a, b]); + + let healthy = workspace_root(&db, "h"); + let healthy_source = "library(pkgc)\n"; + let healthy_file = new_file(&db, "h/main.R", healthy_source); + healthy.set_scripts(&mut db).to(vec![healthy_file]); + + db.workspace_roots() + .set_roots(&mut db) + .to(vec![cycling, healthy]); + + assert_eq!(a.diagnostics(&db).len(), 1); + assert_eq!(b.diagnostics(&db).len(), 1); + assert!(healthy_file.diagnostics(&db).is_empty()); + assert_eq!(shape(&db, healthy_file.imports(&db)), vec![ + "Package(pkgc)".to_string(), + "Package(base)".to_string(), + ]); +} + /// A Shiny app whose autoloaded `R/a.R` sources its collation successor /// `R/b.R`. Returns the two `R/` files, sourcing file first. fn shiny_back_edge(db: &mut TestDb) -> (File, File) { diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index 813329157..8457c4549 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -423,6 +423,128 @@ fn test_script_outside_r_directory_stays_standalone() { assert_eq!(shape(&db, b.imports(&db)), Vec::::new()); } +#[test] +fn test_loose_r_directory_scripts_stay_standalone() { + // Same shape as `test_script_outside_r_directory_stays_standalone`, but + // under `R/` instead of `scripts/`. The implicit alphabetical `R/` + // collation fallback is gone, so the directory name carries no special + // meaning for a loose script. + let mut db = TestDb::new(); + let root = workspace_root(&db, "ws"); + let a = File::new( + &db, + file_path("ws/R/a.R"), + FileRevision::zero(), + Some("a_val <- 1\n".to_string()), + None, + ); + let b = File::new( + &db, + file_path("ws/R/b.R"), + FileRevision::zero(), + Some("b_val <- 2\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + assert_eq!(shape(&db, a.imports(&db)), Vec::::new()); + assert_eq!(shape(&db, b.imports(&db)), Vec::::new()); +} + +/// One workspace root with a single loose script at `ws/{dir}/test.R`. +/// Returns its `imports()` shape and diagnostics count. +fn single_script_under(dir: &str) -> (Vec, usize) { + let mut db = TestDb::new(); + let root = workspace_root(&db, "ws"); + let file = File::new( + &db, + file_path(&format!("ws/{dir}/test.R")), + FileRevision::zero(), + Some("x <- 1\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![file]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + (shape(&db, file.imports(&db)), file.diagnostics(&db).len()) +} + +#[test] +fn test_single_r_directory_script_matches_an_ordinary_directory() { + // https://github.com/posit-dev/positron/issues/15631: a lone script under + // `R/` was reported to crash, and renaming `R/` to `Code/` was reported to + // fix it (never reproduced; see `test_single_r_file_does_not_panic` in + // `tests/workspace.rs`). `R/` is not a recognized loader directory for a + // loose script, so it must behave exactly like any other directory name. + let under_r = single_script_under("R"); + + // Pin the absolute result too, so the comparison can't pass by both sides + // breaking the same way. + assert_eq!(under_r, (Vec::new(), 0)); + assert_eq!(under_r, single_script_under("Code")); +} + +#[test] +fn test_separate_shiny_apps_in_different_workspace_roots_do_not_cross_collate() { + // Loader-based replacement for `test_separate_r_directories_do_not_cross_collate`, + // deleted along with the implicit alphabetical `R/` fallback it exercised. + // Two workspace roots, each running its own Shiny app with an `R/` + // autoload directory of the same basename, so a file in one root's `R/` + // never sees the other root's file of the same name. + let mut db = TestDb::new(); + install_packages(&mut db, &["base", "shiny", "pkga", "pkgb"]); + + let one = workspace_root(&db, "ws/one"); + let app_one = File::new( + &db, + file_path("ws/one/app.R"), + FileRevision::zero(), + Some("shinyApp(ui, server)\n".to_string()), + None, + ); + let a_one = File::new( + &db, + file_path("ws/one/R/a.R"), + FileRevision::zero(), + Some("library(pkga)\n".to_string()), + None, + ); + one.set_scripts(&mut db).to(vec![app_one, a_one]); + + let two = workspace_root(&db, "ws/two"); + let app_two = File::new( + &db, + file_path("ws/two/app.R"), + FileRevision::zero(), + Some("shinyApp(ui, server)\n".to_string()), + None, + ); + let a_two = File::new( + &db, + file_path("ws/two/R/a.R"), + FileRevision::zero(), + Some("library(pkgb)\n".to_string()), + None, + ); + two.set_scripts(&mut db).to(vec![app_two, a_two]); + + db.workspace_roots().set_roots(&mut db).to(vec![one, two]); + + assert_eq!(shape(&db, app_one.imports(&db)), vec![ + "File(a.R)".to_string(), + "Package(pkga)".to_string(), + "Package(shiny)".to_string(), + "Package(base)".to_string(), + ]); + assert_eq!(shape(&db, app_two.imports(&db)), vec![ + "File(a.R)".to_string(), + "Package(pkgb)".to_string(), + "Package(shiny)".to_string(), + "Package(base)".to_string(), + ]); +} + #[test] fn test_package_owned_r_file_excluded_from_collate_stays_standalone() { // An `R/` file left out of `Collate:` carries a package back-pointer but From 895245fb6415bfc482027016d6fca2df540e9a2a Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 16 Sep 2026 12:54:02 +0200 Subject: [PATCH 8/8] Tweak cycle lint message --- crates/oak_db/src/diagnostic.rs | 5 ++++- crates/oak_db/src/tests/contrib/shiny.rs | 8 ++++---- ...stic_source_cycle_package_collation_sourcing_file.snap | 3 ++- ...agnostic_source_cycle_package_collation_successor.snap | 3 ++- ...gnostic_source_cycle_shiny_autoload_sourcing_file.snap | 3 ++- ..._diagnostic_source_cycle_shiny_autoload_successor.snap | 3 ++- ...ostic_source_cycle_testthat_support_sourcing_file.snap | 3 ++- ...iagnostic_source_cycle_testthat_support_successor.snap | 3 ++- 8 files changed, 20 insertions(+), 11 deletions(-) diff --git a/crates/oak_db/src/diagnostic.rs b/crates/oak_db/src/diagnostic.rs index 3abc830b6..462eb2596 100644 --- a/crates/oak_db/src/diagnostic.rs +++ b/crates/oak_db/src/diagnostic.rs @@ -215,7 +215,10 @@ fn lower_uninstalled_package(package: &str, range: TextRange) -> Diagnostic { fn lower_source_cycle(loader: Option) -> Diagnostic { let cause = match loader { Some(LoaderInfo { name, loads }) => { - format!("{name} already loads {loads}, so a `source()` call between them is redundant.") + 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." diff --git a/crates/oak_db/src/tests/contrib/shiny.rs b/crates/oak_db/src/tests/contrib/shiny.rs index 3a830ba5f..af179c0fb 100644 --- a/crates/oak_db/src/tests/contrib/shiny.rs +++ b/crates/oak_db/src/tests/contrib/shiny.rs @@ -458,18 +458,18 @@ fn test_backward_source_into_collation_successor_cycles() { assert!(a.sourced_by(&db).is_empty()); assert!(b.sourced_by(&db).is_empty()); - // Both files carry a `SourceCycle` diagnostic with the same generic + // Both files carry a `SourceCycle` diagnostic with the same loader-specific // message, even though `b.R` has no `source()` call of its own: it is // degraded collaterally by the recovery, not because it takes part in - // mutual sourcing. The message's premise ("mutual `source()` calls") - // doesn't actually hold for this shape. + // mutual sourcing. assert_eq!(a.diagnostics(&db).len(), 1); assert_eq!(b.diagnostics(&db).len(), 1); assert_eq!( a.diagnostics(&db)[0].message(), "This file is part of a cycle in how the project's files load each other.\n\ This Shiny app already loads its `global.R` and `R/` files through \ - `shiny::loadSupport()`, so a `source()` call between them is redundant.\n\ + `shiny::loadSupport()`.\n\ + A `source()` call into a file that the loader also loads creates this cycle.\n\ Language analysis will be incomplete until the cycle is resolved." ); assert_eq!( diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_sourcing_file.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_sourcing_file.snap index 216011706..1a959c1f9 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_sourcing_file.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_sourcing_file.snap @@ -3,7 +3,8 @@ source: crates/oak_db/src/tests/file_diagnostics.rs expression: "render(\"w/pkg/R/a.R\", PKG_BACK_EDGE_SOURCING, a.diagnostics(&db))" --- warning[source-cycle]: This file is part of a cycle in how the project's files load each other. - The package already loads its `R/` files in collation order, so a `source()` call between them is redundant. + The package already loads its `R/` files in collation order. + A `source()` call into a file that the loader also loads creates this cycle. Language analysis will be incomplete until the cycle is resolved. --> w/pkg/R/a.R:1:1 | diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_successor.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_successor.snap index f8cc0a97a..495472e8a 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_successor.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_package_collation_successor.snap @@ -3,7 +3,8 @@ source: crates/oak_db/src/tests/file_diagnostics.rs expression: "render(\"w/pkg/R/b.R\", BACK_EDGE_SUCCESSOR, b.diagnostics(&db))" --- warning[source-cycle]: This file is part of a cycle in how the project's files load each other. - The package already loads its `R/` files in collation order, so a `source()` call between them is redundant. + The package already loads its `R/` files in collation order. + A `source()` call into a file that the loader also loads creates this cycle. Language analysis will be incomplete until the cycle is resolved. --> w/pkg/R/b.R:1:1 | diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_sourcing_file.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_sourcing_file.snap index 1c76bc085..08f7144a4 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_sourcing_file.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_sourcing_file.snap @@ -3,7 +3,8 @@ source: crates/oak_db/src/tests/file_diagnostics.rs expression: "render(\"w/R/a.R\", BACK_EDGE_SOURCING, a.diagnostics(&db))" --- warning[source-cycle]: This file is part of a cycle in how the project's files load each other. - This Shiny app already loads its `global.R` and `R/` files through `shiny::loadSupport()`, so a `source()` call between them is redundant. + This Shiny app already loads its `global.R` and `R/` files through `shiny::loadSupport()`. + A `source()` call into a file that the loader also loads creates this cycle. Language analysis will be incomplete until the cycle is resolved. --> w/R/a.R:1:1 | diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_successor.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_successor.snap index f62279b43..5b809f27b 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_successor.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_shiny_autoload_successor.snap @@ -3,7 +3,8 @@ source: crates/oak_db/src/tests/file_diagnostics.rs expression: "render(\"w/R/b.R\", BACK_EDGE_SUCCESSOR, b.diagnostics(&db))" --- warning[source-cycle]: This file is part of a cycle in how the project's files load each other. - This Shiny app already loads its `global.R` and `R/` files through `shiny::loadSupport()`, so a `source()` call between them is redundant. + This Shiny app already loads its `global.R` and `R/` files through `shiny::loadSupport()`. + A `source()` call into a file that the loader also loads creates this cycle. Language analysis will be incomplete until the cycle is resolved. --> w/R/b.R:1:1 | diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_sourcing_file.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_sourcing_file.snap index a50d0922e..f444b5b96 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_sourcing_file.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_sourcing_file.snap @@ -3,7 +3,8 @@ source: crates/oak_db/src/tests/file_diagnostics.rs expression: "render(\"w/pkg/tests/testthat/helper.R\", TESTTHAT_BACK_EDGE_SOURCING,\nhelper.diagnostics(&db))" --- warning[source-cycle]: This file is part of a cycle in how the project's files load each other. - testthat already loads `helper*.R` and `setup*.R` before your tests, so a `source()` call between them is redundant. + testthat already loads `helper*.R` and `setup*.R` before your tests. + A `source()` call into a file that the loader also loads creates this cycle. Language analysis will be incomplete until the cycle is resolved. --> w/pkg/tests/testthat/helper.R:1:1 | diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_successor.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_successor.snap index c2d38f263..31ea0a1b8 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_successor.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_testthat_support_successor.snap @@ -3,7 +3,8 @@ source: crates/oak_db/src/tests/file_diagnostics.rs expression: "render(\"w/pkg/tests/testthat/setup.R\", BACK_EDGE_SUCCESSOR,\nsetup.diagnostics(&db))" --- warning[source-cycle]: This file is part of a cycle in how the project's files load each other. - testthat already loads `helper*.R` and `setup*.R` before your tests, so a `source()` call between them is redundant. + testthat already loads `helper*.R` and `setup*.R` before your tests. + A `source()` call into a file that the loader also loads creates this cycle. Language analysis will be incomplete until the cycle is resolved. --> w/pkg/tests/testthat/setup.R:1:1 |