diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index af70377e2..5877d6c5e 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -174,9 +174,10 @@ impl File { /// body must precede the cursor, and conditional attaches remain limited to /// the arm that attaches them. /// - /// - **Top-level cursor (script)**: only `library()` calls that - /// have occurred before `offset`. Most recently attached comes - /// first. + /// - **Top-level cursor (script)**: `library()` calls before `offset`, + /// most recently attached first. A script in an `R/` directory also + /// sees its collation predecessors, most recently sourced first, the + /// same convention as a package file and as with `shiny.autoload.r`. /// /// - **Top-level cursor (package)**: only collation predecessors /// of this file. Most recently sourced predecessor comes @@ -246,12 +247,17 @@ impl File { match self.package(db) { // A `tests/testthat/` file: sees the whole package plus sourced // helpers, with testthat attached. - Some(package) if is_testthat_file(self, db) => testthat_load_layers(self, db, package), + Some(package) if is_testthat_file(self, db) => { + testthat_load_layers(self, db, package, view) + }, // A loadable `R/` file: sees collation siblings and the package // NAMESPACE. Some(package) if self.is_package_source(db, package) => { package_load_layers(self, db, package, view) }, + // A non-package script in an `R/` directory: collated + // alphabetically, exactly like a package `R/` with no `Collate:`. + None if in_r_directory(self, db) => script_collation_layers(self, db, view), // A standalone script, or a file with a package back-pointer that // isn't a loadable `R/` file (`data-raw/`, `inst/`, a non-collated // `R/` file): lives in the package but isn't loaded with it, so it @@ -271,6 +277,37 @@ impl File { fn is_package_source(self, db: &dyn Db, package: Package) -> bool { package.files(db).contains(&self) } + + /// The collation members of `self`'s own `R/` directory, in load order: + /// sorted by basename, ASCII case-insensitively, the same order a + /// package `R/` with no `Collate:` gets from + /// `oak_scan::packages::order_alphabetically`. + /// + /// Path-based only. The scan-time resolver + /// ([`SalsaImportsResolver`](crate::imports::SalsaImportsResolver)) calls + /// `cross_file_layers` while `self`'s own semantic index is still being + /// built. The query can't recurse into the index. + /// + /// Gathers candidates from workspace roots only (`root.scripts(db)` for + /// each). `OrphanRoot`, library roots, and `StaleRoot` don't contribute + /// collation siblings. + #[salsa::tracked(returns(ref))] + pub(crate) fn collation_siblings(self, db: &dyn Db) -> Vec { + let Some(dir) = self.path(db).as_path().and_then(Utf8Path::parent) else { + return Vec::new(); + }; + + let mut siblings: Vec = db + .workspace_roots() + .roots(db) + .iter() + .flat_map(|root| root.scripts(db).iter().copied()) + .filter(|file| file.path(db).as_path().and_then(Utf8Path::parent) == Some(dir)) + .collect(); + + siblings.sort_by_cached_key(|file| collation_basename_key(*file, db)); + siblings + } } fn package_load_layers( @@ -281,49 +318,95 @@ fn package_load_layers( ) -> CrossFileLayers { let files = package.files(db); - // The sibling `R/` files visible to this one, in LIFO order (latest-sourced - // first): a name defined late in the collation shadows the same name defined - // earlier. Self is excluded, its own top-level bindings come from `exports`, - // and including it here would cycle in `resolve` for unbound names. - let def_files: Vec = match view { - CollationView::Lazy => files.iter().rev().copied().filter(|f| *f != file).collect(), - CollationView::Eager => match files.iter().position(|f| *f == file) { - Some(pos) => files[..pos].iter().rev().copied().collect(), - None => { - // File claims membership but isn't in the package's `files`. - // Shouldn't happen. - log::warn!( - "File {file} has package back-pointer to {package} but is not in its files", - file = file.path(db), - package = package.name(db), - ); - files.iter().rev().copied().filter(|f| *f != file).collect() - }, - }, - }; + // `Collate:` order isn't derivable from file names. + let prefix_len = files.iter().position(|sibling| *sibling == file); + if prefix_len.is_none() && matches!(view, CollationView::Eager) { + // File claims package membership but isn't in `package.files()`. + // Shouldn't happen; see the placement invariant on `File.package`. + log::warn!( + "File {file} has package back-pointer to {package} but is not in its files", + file = file.path(db), + package = package.name(db), + ); + } + let siblings = visible_siblings(file, files, view, prefix_len); - let mut above: Vec = def_files.iter().copied().map(ImportLayer::File).collect(); + let mut above: Vec = siblings.iter().copied().map(ImportLayer::File).collect(); let namespace = package.namespace(db); extend_with_namespace_imports(package, namespace, &mut above); extend_with_namespace_package_imports(db, namespace, &mut above); - // Every def file's attaches go on the search path below the file's own. + // Every sibling's attaches go on the search path below the file's own. // For the `Lazy` view that includes successors, whose `library()` calls // actually run after this file's at load time and so outrank the file's own // attaches at runtime. We rank them below instead. Only matters when a // successor re-attaches a package that shadows one of this file's own // attaches, which is rare, and the direction we lose is the safe one. - let mut below = predecessor_attach_layers(db, &def_files); + let mut below = predecessor_attach_layers(db, &siblings); below.extend(base_layer(db)); CrossFileLayers { above, below } } +/// Load-time layers for a non-package script collated by the `R/` directory +/// convention (see `File::cross_file_layers`). Mirrors `package_load_layers`, +/// with `below` ending in the whole default search path rather than just `base`. +fn script_collation_layers(file: File, db: &dyn Db, view: CollationView) -> CrossFileLayers { + let files = file.collation_siblings(db); + + // `file` is missing from its own sibling list until the scanner moves it out + // of `OrphanRoot`. Cut on the sort key instead. + let own_key = collation_basename_key(file, db); + let prefix_len = + files.partition_point(|sibling| collation_basename_key(*sibling, db) < own_key); + let siblings = visible_siblings(file, files, view, Some(prefix_len)); + + let above: Vec = siblings.iter().copied().map(ImportLayer::File).collect(); + + let mut below = predecessor_attach_layers(db, &siblings); + below.extend(default_search_path_layers(db)); + CrossFileLayers { above, below } +} + +/// Files visible to `file`, ordered for LIFO lookup. A later-loaded collation +/// sibling shadows names from an earlier sibling. +/// +/// Excludes `file` because its top-level bindings come from `exports()`. +/// Including it would make `resolve()` cycle for unbound names. +/// +/// `prefix_len` counts collation files loaded before `file`, which an `Eager` +/// view retains. `None` means `file` is absent from the collation, so every +/// non-self sibling is returned in LIFO order to over-approximate visibility. +fn visible_siblings( + file: File, + collation: &[File], + view: CollationView, + prefix_len: Option, +) -> Vec { + match view { + CollationView::Lazy => collation + .iter() + .rev() + .copied() + .filter(|sibling| *sibling != file) + .collect(), + CollationView::Eager => match prefix_len { + Some(len) => collation[..len].iter().rev().copied().collect(), + None => collation + .iter() + .rev() + .copied() + .filter(|sibling| *sibling != file) + .collect(), + }, + } +} + /// Load-time layers visible to a `tests/testthat/` file, in R's LIFO priority /// order. /// /// A test file 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. So the layering, highest priority first, is: +/// the test environment. The layering, highest priority first, is: /// /// 1. helper/setup files (sourced into the test env, shadow everything), /// 2. the whole package's `R/` code, @@ -331,18 +414,30 @@ fn package_load_layers( /// 4. the file's own top-level `library()` calls (spliced in by the caller), /// 5. helper/setup and package attaches, then `testthat`, on the search path, /// 6. base. -fn testthat_load_layers(file: File, db: &dyn Db, package: Package) -> CrossFileLayers { - // testthat sources `helper*.R` / `setup*.R` sorted, so reversing gives LIFO - // precedence. Self is dropped when the file being analysed is itself a - // helper/setup file, same self-exclusion reasoning as `package_load_layers`. +/// +/// Support files form their own collation. An `Eager` view keeps only +/// source-order predecessors, while a `Lazy` view keeps every support file. +/// Every `R/` file remains visible because package loading finishes first. +fn testthat_load_layers( + file: File, + db: &dyn Db, + package: Package, + view: CollationView, +) -> CrossFileLayers { let mut support: Vec = package .scripts(db) .iter() .copied() - .filter(|f| *f != file && is_testthat_support_file(*f, db)) + .filter(|script| is_testthat_support_file(*script, db)) .collect(); - support.sort_by_cached_key(|f| testthat_support_key(*f, db)); - support.reverse(); + support.sort_by_cached_key(|script| testthat_support_key(*script, db)); + + // Test files run after every support file, so they use the full support prefix. + let prefix_len = support + .iter() + .position(|script| *script == file) + .unwrap_or(support.len()); + let support = visible_siblings(file, &support, view, Some(prefix_len)); // The whole package is loaded when tests run, so every `R/` file is visible. // Collation order reversed for LIFO, same as `package_load_layers`. @@ -413,6 +508,17 @@ fn in_testthat_dir(path: &Utf8Path) -> bool { parent.parent().and_then(Utf8Path::file_name) == Some("tests") } +/// True when `file` sits directly in an `R/` directory, the convention that +/// triggers script collation for a non-package file (see the match in +/// `File::cross_file_layers`). Case-sensitive: that's the convention on every +/// platform, and what the package scanner looks for. +fn in_r_directory(file: File, db: &dyn Db) -> bool { + let Some(path) = file.path(db).as_path() else { + return false; + }; + path.parent().and_then(Utf8Path::file_name) == Some("R") +} + /// testthat sources `helper*.R` and `setup*.R` from `tests/testthat/` into the /// test environment before running any test file, so their top-level bindings /// are visible to every test. testthat matches `^helper.*\.[rR]$` and @@ -439,6 +545,16 @@ fn testthat_support_key(file: File, db: &dyn Db) -> Cow<'_, str> { file.path(db).file_name().unwrap_or_default() } +/// Case-insensitive basename sort key for `collation_siblings`, matching +/// `oak_scan::packages::order_alphabetically`'s `basename_key` so a +/// non-package `R/` collates the same way as a package `R/` with no +/// `Collate:`. +fn collation_basename_key(file: File, db: &dyn Db) -> Option { + file.path(db) + .file_name() + .map(|name| name.to_ascii_lowercase()) +} + /// Push the `From` layer if `package`'s namespace has any `importFrom` entries. fn extend_with_namespace_imports( package: Package, diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index 1ab5657f9..2429fab79 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -615,3 +615,234 @@ 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(); + let root = workspace_root(&db, "ws"); + let a = File::new( + &db, + file_path("ws/scripts/a.R"), + FileRevision::zero(), + Some("a_val <- 1\n".to_string()), + None, + ); + let b = File::new( + &db, + file_path("ws/scripts/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_package_owned_r_file_excluded_from_collate_stays_standalone() { + // An `R/` file left out of `Collate:` carries a package back-pointer but + // isn't in `package.files()`, so it must keep resolving as a standalone + // script (same as `data-raw/`) and not take the new script-collation arm + // just because it sits in an `R/` directory. That arm is gated on + // `package(db) == None`. + let mut db = TestDb::new(); + 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 r_file = File::new( + &db, + file_path("w/pkg/R/a.R"), + FileRevision::zero(), + Some("internal <- 1\n".to_string()), + Some(pkg), + ); + let extra = File::new( + &db, + file_path("w/pkg/R/extra.R"), + FileRevision::zero(), + Some("x <- 1\n".to_string()), + Some(pkg), + ); + pkg.set_files(&mut db).to(vec![r_file]); + pkg.set_scripts(&mut db).to(vec![extra]); + workspace.set_packages(&mut db).to(vec![pkg]); + db.workspace_roots().set_roots(&mut db).to(vec![workspace]); + + 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() { + // Guards against reusing `package_load_layers`'s `base_layer` for the + // script path: a non-package script sees R's whole startup search path + // (stats, graphics, ..., base), not just `base`. `package_load_layers` + // uses `base_layer` because NAMESPACE supplies the rest for a package + // file; a script has no NAMESPACE. + 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. + 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]); + + let _ = a.imports(&db); + assert_eq!(db.executions("cross_file_layers"), 1); + + let elsewhere = File::new( + &db, + file_path("ws/other/z.R"), + FileRevision::zero(), + Some("z_val <- 1\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b, elsewhere]); + + let _ = a.imports(&db); + assert_eq!(db.executions("cross_file_layers"), 1); +} diff --git a/crates/oak_db/src/tests/file_imports_at.rs b/crates/oak_db/src/tests/file_imports_at.rs index cc5929fd3..5833dffbc 100644 --- a/crates/oak_db/src/tests/file_imports_at.rs +++ b/crates/oak_db/src/tests/file_imports_at.rs @@ -295,6 +295,57 @@ fn test_testthat_top_level_library_narrows_by_offset() { assert!(library_attaches(&db, &after).contains(&"cli".to_string())); } +/// Creates a `tests/testthat/` fixture with three support files and one test. +/// Returns `(helper_a, helper_b, setup_c, test_x)`. +fn testthat_support_workspace(db: &mut TestDb, helper_b: &str) -> (File, File, File, File) { + let pkg = install_workspace_package(db, "pkg"); + let path = |name: &str| format!("workspace/pkg/tests/testthat/{name}"); + + let helper_a = make_package_file(db, &path("helper-a.R"), "a_val <- 1\n", pkg); + let helper_b = make_package_file(db, &path("helper-b.R"), helper_b, pkg); + let setup_c = make_package_file(db, &path("setup-c.R"), "c_val <- 3\n", pkg); + let test_x = make_package_file(db, &path("test-x.R"), "x <- 1\n", pkg); + + pkg.set_scripts(db) + .to(vec![helper_a, helper_b, setup_c, test_x]); + (helper_a, helper_b, setup_c, test_x) +} + +#[test] +fn test_testthat_support_file_top_level_sees_only_earlier_support_files() { + // testthat sources support files in lexical order. `helper-b.R` runs before + // `setup-c.R`, so its top-level code cannot see that file. + let mut db = TestDb::new(); + let (helper_a, helper_b, _setup_c, _test_x) = + testthat_support_workspace(&mut db, "b_val <- 2\n"); + + let layers = helper_b.imports_at(&db, TextSize::from(0)); + assert_eq!(package_files(&layers), vec![helper_a]); +} + +#[test] +fn test_testthat_support_file_body_sees_every_support_file() { + // The function body runs after every support file is sourced. LIFO lookup + // therefore puts `setup-c.R` before `helper-a.R`. + let mut db = TestDb::new(); + let source = "f <- function() {\n inside\n}\n"; + let (helper_a, helper_b, setup_c, _test_x) = testthat_support_workspace(&mut db, source); + + let offset = TextSize::from(source.find("inside").unwrap() as u32); + let layers = helper_b.imports_at(&db, offset); + assert_eq!(package_files(&layers), vec![setup_c, helper_a]); +} + +#[test] +fn test_testthat_test_file_top_level_sees_every_support_file() { + // `test-x.R` runs after every support file, so its eager view retains all of them. + let mut db = TestDb::new(); + let (helper_a, helper_b, setup_c, test_x) = testthat_support_workspace(&mut db, "b_val <- 2\n"); + + let layers = test_x.imports_at(&db, TextSize::from(0)); + assert_eq!(package_files(&layers), vec![setup_c, helper_b, helper_a]); +} + #[test] fn test_library_in_function_scoped_source_is_visible_only_in_that_function() { // A sourced `library()` becomes an `Attach` in `source()`'s calling scope. @@ -735,3 +786,64 @@ fn test_attach_in_a_function_body_is_visible_later_in_that_body() { "cli".to_string() ]]); } + +#[test] +fn test_script_r_directory_top_level_sees_only_alphabetic_predecessor() { + let mut db = TestDb::new(); + let root = workspace_root(&db, "ws"); + 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]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + // `b.R` is alphabetically after `a.R`, so `a.R` is its collation + // predecessor. + let offset = TextSize::from(b_source.find('x').unwrap() as u32); + assert_eq!(package_files(&b.imports_at(&db, offset)), vec![a]); + + // `a.R` has no predecessor: it's first in collation order. + let offset = TextSize::from(0); + assert_eq!( + package_files(&a.imports_at(&db, offset)), + Vec::::new() + ); +} + +#[test] +fn test_script_r_directory_collation_is_case_insensitive() { + // Matches `oak_scan::packages::order_alphabetically`: basenames sort + // case-insensitively, so `a.R` collates before `Z.R` even though it's + // lexically greater in byte order. + let mut db = TestDb::new(); + let root = workspace_root(&db, "ws"); + 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]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + let offset = TextSize::from(z_source.len() as u32); + assert_eq!(package_files(&z_file.imports_at(&db, offset)), vec![a_file]); +} + +#[test] +fn test_script_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. + let mut db = TestDb::new(); + let root = workspace_root(&db, "ws"); + 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]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + let offset = TextSize::from(b_source.find('x').unwrap() as u32); + assert_eq!(package_files(&b.imports_at(&db, offset)), vec![a]); +}