diff --git a/crates/oak_db/src/db.rs b/crates/oak_db/src/db.rs index 9fd4bee16..eb41cef52 100644 --- a/crates/oak_db/src/db.rs +++ b/crates/oak_db/src/db.rs @@ -178,6 +178,29 @@ pub fn root_by_package_query(db: &dyn Db, pkg: Package) -> Option { best.map(|(root, _)| root) } +/// Resolve the [`Package`] that owns `file`, if any. +/// +/// Walks live roots' `File -> Package` indexes in workspace-then-library +/// order and returns the first hit. A file belongs to at most one package +/// *entity* (in the nested-root case both roots' `packages` hold the same +/// entity), so first-hit is unambiguous. The orphan bucket has no packages +/// and contributes nothing; stale entities are invisible by design, which +/// is what makes an evicted file's package association clear to `None`. +/// +/// Backs [`crate::File::package`], which is the derived replacement for the +/// old `File.package` back-pointer field: the container vecs are now the +/// single source of truth for file ownership. +pub fn package_by_file_query(db: &dyn Db, file: File) -> Option { + for &root in db.live_roots() { + if let LiveRoot::Workspace(r) | LiveRoot::Library(r) = root { + if let Some(&pkg) = root_file_package_index(db, r).get(&file) { + return Some(pkg); + } + } + } + None +} + /// Number of path segments in a root's URL. Used as the tiebreaker by /// [`root_by_package_query`] when nested roots both claim the same package. /// @@ -212,6 +235,21 @@ fn root_url_index(db: &dyn Db, root: Root) -> FxHashMap { map } +/// Per-root File -> owning Package index. Built from `root.packages` and +/// each package's `files`. Same per-root granularity as [`root_url_index`]: +/// adding or removing a file in this root invalidates only this entry. +/// Backs [`package_by_file_query`]. +#[salsa::tracked(returns(ref))] +fn root_file_package_index(db: &dyn Db, root: Root) -> FxHashMap { + let mut map = FxHashMap::default(); + for &pkg in root.packages(db) { + for &file in pkg.files(db) { + map.insert(file, pkg); + } + } + map +} + /// Orphan URL -> File index. Reads only `orphan_root().files`. #[salsa::tracked(returns(ref))] fn orphan_url_index(db: &dyn Db) -> FxHashMap { diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index d89fa555b..5eac38ea7 100644 --- a/crates/oak_db/src/file.rs +++ b/crates/oak_db/src/file.rs @@ -23,36 +23,18 @@ use crate::Root; /// The `url` field is a [`UrlId`], so the type system enforces "everything /// inside Salsa is a canonical URL". /// -/// `package` is a back-pointer to the [`Package`] this file belongs to, or -/// `None` for standalone scripts. Inverse of `Package.files`, so queries -/// answering "what package owns this file?" don't walk the forward edge. -/// Files with `package == None` are either standalone scripts under a -/// workspace root or orphan files not registered anywhere. -/// -/// # Placement invariant -/// -/// `File.package` and the file's physical location in a `Vec` are -/// expected to agree. A file with `package == Some(pkg)` should live in -/// `pkg.files`. A file with `package == None` should live in either some -/// `root.scripts` or `orphan_root().files`. The salsa setters (`set_url`, -/// `set_contents`, `set_package`) are `pub` because field visibility couples to -/// setter visibility in salsa but calling `set_package` directly leaves the -/// file in its old bucket and silently breaks this invariant. -/// -/// The scanner crate (`oak_scan`) wraps these setters in helpers that -/// maintain placement (move the file between `pkg.files`, -/// `root.scripts`, and `orphan_root().files` as `package` changes). -/// Callers that go around the helpers and use the salsa setters -/// directly must maintain placement themselves. +/// File ownership has a single source of truth: the forward edge. A file +/// belongs to whichever container currently holds it -- `pkg.files`, +/// `root.scripts`, or `orphan_root().files`. There is no stored +/// back-pointer to keep in sync. "What package owns this file?" is +/// answered by the derived [`File::package`] query, which walks the +/// per-root `File -> Package` indexes; "what root?" by [`File::root`]. #[salsa::input(debug)] pub struct File { #[returns(ref)] pub url: UrlId, #[returns(ref)] pub contents: String, - /// **Placement invariant.** Call this setter only through - /// `oak_scan`'s helpers; see the type-level doc above. - pub package: Option, } #[salsa::tracked] @@ -144,9 +126,23 @@ impl File { .collect() } + /// The [`Package`] that owns this file, or `None` for standalone + /// scripts and orphan / stale files. + /// + /// Derived from live-graph containment via + /// [`package_by_file_query`](crate::db::package_by_file_query): the + /// file belongs to whichever live `pkg.files` currently holds it. + /// This replaces the old `File.package` back-pointer field, so file + /// ownership has a single source of truth (the forward edge) and no + /// invariant to maintain by hand. + #[salsa::tracked] + pub fn package(self, db: &dyn Db) -> Option { + crate::db::package_by_file_query(db, self) + } + /// The root containing this file, if any. /// - /// If the file has a registered [`Package`], asks the db which live + /// If the file has an owning [`Package`], asks the db which live /// root holds it via [`Db::root_by_package`]. Otherwise falls back to a /// URL-prefix lookup against [`WorkspaceRoots`] (orphan files live /// under a workspace root or nowhere). Library files normally have diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 4fea76d12..f57ab22f3 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -128,10 +128,11 @@ fn narrow_script_top_level(file: File, db: &dyn Db, offset: TextSize) -> Vec Vec { let files = package.files(db); let Some(file_pos) = files.iter().position(|f| *f == file) else { - // File claims membership but isn't in the package's `files`. + // `package` was derived from `package.files` containing this file + // (via `File::package`), so the position is always found. // Shouldn't happen. log::warn!( - "File {file} has package back-pointer to {package} but is not in its files", + "File {file} resolved to package {package} but is not in its files", file = file.url(db), package = package.name(db), ); diff --git a/crates/oak_db/src/inputs.rs b/crates/oak_db/src/inputs.rs index 7e963b11a..7735c2b25 100644 --- a/crates/oak_db/src/inputs.rs +++ b/crates/oak_db/src/inputs.rs @@ -23,14 +23,14 @@ pub struct Root { pub path: UrlId, pub kind: RootKind, /// Top-level R scripts directly under this root. Each entry is a - /// `File` with `package(db) == None`. Always empty for `Library` - /// roots. + /// `File` not owned by any package (`package(db) == None`). Always + /// empty for `Library` roots. /// - /// **Placement invariant.** A file present here must have - /// `package(db) == None`, and a file with `package == None` must - /// live here, in another `Root.scripts`, or in - /// `OrphanRoot.files`. Call this setter only through `oak_scan`'s - /// helpers, which keep the back-pointer and the container in sync. + /// A file should live in exactly one container (`Root.scripts`, + /// `Package.files`, or `OrphanRoot.files`); `File::package` derives + /// ownership from that. Push files here only through `oak_scan`'s + /// helpers, which move a file out of its old container as it changes + /// homes. #[returns(ref)] pub scripts: Vec, /// Packages discovered under this root (workspace packages for @@ -121,9 +121,10 @@ impl LibraryRoots { /// [`crate::Db::file_by_url`] consults to find unanchored files. #[salsa::input(debug)] pub struct OrphanRoot { - /// **Placement invariant.** Files here must have `package(db) == - /// None`. Call this setter only through `oak_scan`'s helpers, - /// which keep the back-pointer and the container in sync. + /// Files here are owned by no package: since the orphan bucket has no + /// `packages`, `File::package` derives `None` for them. Push files + /// here only through `oak_scan`'s helpers, which move a file out of + /// its old container as it changes homes. #[returns(ref)] pub files: Vec, } @@ -204,10 +205,11 @@ pub struct Package { /// package doesn't invalidate tracked queries reading another /// package's files. /// - /// **Placement invariant.** A file present here must have - /// `package(db) == Some(self)`. Call this setter only through - /// `oak_scan`'s helpers, which keep the back-pointer and the - /// container in sync. + /// This vec is the source of truth for package membership: + /// `File::package` derives ownership by finding the file here. A file + /// should live in exactly one container, so push files here only + /// through `oak_scan`'s helpers, which move a file out of its old + /// container as it changes homes. #[returns(ref)] pub files: Vec, /// The basename ordering from `DESCRIPTION`'s `Collate` field, if diff --git a/crates/oak_db/src/tests/db.rs b/crates/oak_db/src/tests/db.rs index bfcfec2d9..8fa2540cc 100644 --- a/crates/oak_db/src/tests/db.rs +++ b/crates/oak_db/src/tests/db.rs @@ -14,7 +14,7 @@ use crate::Package; fn test_file_by_url_finds_workspace_script() { let mut db = TestDb::new(); let root = workspace_root(&db, "proj"); - let file = File::new(&db, file_url("proj/a.R"), String::new(), None); + let file = File::new(&db, file_url("proj/a.R"), String::new()); root.set_scripts(&mut db).to(vec![file]); db.workspace_roots().set_roots(&mut db).to(vec![root]); @@ -25,10 +25,9 @@ fn test_file_by_url_finds_workspace_script() { fn test_file_by_url_finds_workspace_package_file() { let mut db = TestDb::new(); let root = workspace_root(&db, "proj"); - // Construct the back-pointer + forward-edge pair: create the - // `Package` with an empty `files` list, then the `File` with the - // `package` set, then attach the file via `set_files`. Matches the - // shape `oak_scan`'s placement-preserving helpers will produce. + // Attach the file to the package via the forward edge (`set_files`), + // the single source of truth for membership. Matches the shape + // `oak_scan`'s placement-preserving helpers produce. let pkg = Package::new( &db, file_url("proj/DESCRIPTION"), @@ -38,7 +37,7 @@ fn test_file_by_url_finds_workspace_package_file() { vec![], None, ); - let file = File::new(&db, file_url("proj/R/foo.R"), String::new(), Some(pkg)); + let file = File::new(&db, file_url("proj/R/foo.R"), String::new()); pkg.set_files(&mut db).to(vec![file]); root.set_packages(&mut db).to(vec![pkg]); db.workspace_roots().set_roots(&mut db).to(vec![root]); @@ -59,7 +58,7 @@ fn test_file_by_url_finds_library_package_file() { vec![], None, ); - let file = File::new(&db, file_url("libs/foo/R/a.R"), String::new(), Some(pkg)); + let file = File::new(&db, file_url("libs/foo/R/a.R"), String::new()); pkg.set_files(&mut db).to(vec![file]); lib.set_packages(&mut db).to(vec![pkg]); db.library_roots().set_roots(&mut db).to(vec![lib]); @@ -70,7 +69,7 @@ fn test_file_by_url_finds_library_package_file() { #[test] fn test_file_by_url_finds_orphan_file() { let mut db = TestDb::new(); - let file = File::new(&db, file_url("untitled.R"), String::new(), None); + let file = File::new(&db, file_url("untitled.R"), String::new()); db.orphan_root().set_files(&mut db).to(vec![file]); assert_eq!(db.file_by_url(&file_url("untitled.R")), Some(file)); @@ -90,8 +89,8 @@ fn test_root_url_index_invalidates_per_root() { let mut db = TestDb::new(); let root_a = workspace_root(&db, "a"); let root_b = workspace_root(&db, "b"); - let file_a = File::new(&db, file_url("a/file.R"), String::new(), None); - let file_b = File::new(&db, file_url("b/file.R"), String::new(), None); + let file_a = File::new(&db, file_url("a/file.R"), String::new()); + let file_b = File::new(&db, file_url("b/file.R"), String::new()); root_a.set_scripts(&mut db).to(vec![file_a]); root_b.set_scripts(&mut db).to(vec![file_b]); db.workspace_roots() @@ -106,7 +105,7 @@ fn test_root_url_index_invalidates_per_root() { assert_eq!(db.executions("root_url_index"), 2); // Add a script to root B. B's index invalidates; A's stays cached. - let file_b2 = File::new(&db, file_url("b/other.R"), String::new(), None); + let file_b2 = File::new(&db, file_url("b/other.R"), String::new()); root_b.set_scripts(&mut db).to(vec![file_b, file_b2]); // Look up the file in A. A's index is still cached, no re-exec. diff --git a/crates/oak_db/src/tests/file.rs b/crates/oak_db/src/tests/file.rs index f2cf9c461..a0d60d21d 100644 --- a/crates/oak_db/src/tests/file.rs +++ b/crates/oak_db/src/tests/file.rs @@ -9,7 +9,7 @@ use crate::File; /// touching the orphan/workspace bucketing logic that's exercised in /// `oak_storage/tests/`. fn new_file(db: &mut TestDb, name: &str, contents: &str) -> File { - File::new(db, file_url(name), contents.to_string(), None) + File::new(db, file_url(name), contents.to_string()) } #[test] @@ -56,7 +56,7 @@ fn test_semantic_index_matches_oak_semantic() { let db = TestDb::new(); let source = "x <- 1\nx\n"; let url = file_url("a.R"); - let file = File::new(&db, url.clone(), source.to_string(), None); + let file = File::new(&db, url.clone(), source.to_string()); let via_salsa = file.semantic_index(&db); diff --git a/crates/oak_db/src/tests/file_exports.rs b/crates/oak_db/src/tests/file_exports.rs index e9d4a2571..1705e6a20 100644 --- a/crates/oak_db/src/tests/file_exports.rs +++ b/crates/oak_db/src/tests/file_exports.rs @@ -16,7 +16,7 @@ fn setup_workspace(db: &mut TestDb, scripts: &[(&str, &str)]) -> Vec { let root = workspace_root(db, "w"); let files: Vec = scripts .iter() - .map(|(name, contents)| File::new(db, file_url(name), contents.to_string(), None)) + .map(|(name, contents)| File::new(db, file_url(name), contents.to_string())) .collect(); root.set_scripts(db).to(files.clone()); db.workspace_roots().set_roots(db).to(vec![root]); diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index 5b894336d..a7e783c8d 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -51,7 +51,7 @@ fn test_script_with_no_attaches_returns_only_default_search_path() { let base = packages[0]; let stats = packages[1]; - let file = File::new(&db, file_url("a.R"), "x <- 1\n".to_string(), None); + let file = File::new(&db, file_url("a.R"), "x <- 1\n".to_string()); let layers = file.imports(&db); // Only `stats` and `base` are registered in this test; the other @@ -77,7 +77,6 @@ fn test_script_attach_produces_package_exports_layer_in_lifo_order() { &db, file_url("a.R"), "library(dplyr)\nlibrary(ggplot2)\n".to_string(), - None, ); let layers = file.imports(&db); @@ -98,7 +97,7 @@ fn test_script_attach_produces_package_exports_layer_in_lifo_order() { fn test_script_attach_to_unregistered_package_drops_layer() { let db = TestDb::new(); // No `dplyr` in any library root. - let file = File::new(&db, file_url("a.R"), "library(dplyr)\n".to_string(), None); + let file = File::new(&db, file_url("a.R"), "library(dplyr)\n".to_string()); let layers = file.imports(&db); assert!(layers.is_empty()); @@ -132,18 +131,8 @@ fn test_package_file_emits_namespace_and_collation_layers() { Vec::new(), None, ); - let first = File::new( - &db, - file_url("w/pkg/R/_a.R"), - "first <- 1\n".to_string(), - Some(pkg), - ); - let second = File::new( - &db, - file_url("w/pkg/R/b.R"), - "second <- 2\n".to_string(), - Some(pkg), - ); + let first = File::new(&db, file_url("w/pkg/R/_a.R"), "first <- 1\n".to_string()); + let second = File::new(&db, file_url("w/pkg/R/b.R"), "second <- 2\n".to_string()); pkg.set_files(&mut db).to(vec![first, second]); workspace.set_packages(&mut db).to(vec![pkg]); db.workspace_roots().set_roots(&mut db).to(vec![workspace]); @@ -189,7 +178,7 @@ fn test_imports_is_cached_per_file() { let mut db = TestDb::new(); let _ = install_packages(&mut db, &["dplyr"]); - let file = File::new(&db, file_url("a.R"), "library(dplyr)\n".to_string(), None); + let file = File::new(&db, file_url("a.R"), "library(dplyr)\n".to_string()); let _ = file.imports(&db); let _ = file.imports(&db); diff --git a/crates/oak_db/src/tests/file_imports_at.rs b/crates/oak_db/src/tests/file_imports_at.rs index 5dcba7a10..63416f6f6 100644 --- a/crates/oak_db/src/tests/file_imports_at.rs +++ b/crates/oak_db/src/tests/file_imports_at.rs @@ -12,11 +12,7 @@ use crate::ImportLayer; use crate::Package; fn make_file(db: &mut TestDb, path: &str, contents: &str) -> File { - File::new(db, file_url(path), contents.to_string(), None) -} - -fn make_package_file(db: &mut TestDb, path: &str, contents: &str, package: Package) -> File { - File::new(db, file_url(path), contents.to_string(), Some(package)) + File::new(db, file_url(path), contents.to_string()) } /// Register a set of installed packages on `LibraryRoots`, one library @@ -148,10 +144,10 @@ fn test_package_top_level_sees_predecessor_files_only() { let _ = install_packages(&mut db, &["base"]); let pkg = install_workspace_package(&mut db, "pkg"); - let a = make_package_file(&mut db, "/w/pkg/R/a.R", "first <- 1\n", pkg); + let a = make_file(&mut db, "/w/pkg/R/a.R", "first <- 1\n"); let b_source = "x <- 1\n"; - let b = make_package_file(&mut db, "/w/pkg/R/b.R", b_source, pkg); - let c = make_package_file(&mut db, "/w/pkg/R/c.R", "second <- 2\n", pkg); + let b = make_file(&mut db, "/w/pkg/R/b.R", b_source); + let c = make_file(&mut db, "/w/pkg/R/c.R", "second <- 2\n"); pkg.set_files(&mut db).to(vec![a, b, c]); // Cursor at top-level in b. Only a (the predecessor in `Package.files`) @@ -167,10 +163,10 @@ fn test_package_function_body_sees_other_package_files_in_lifo_order() { let _ = install_packages(&mut db, &["base"]); let pkg = install_workspace_package(&mut db, "pkg"); - let a = make_package_file(&mut db, "/w/pkg/R/a.R", "first <- 1\n", pkg); + let a = make_file(&mut db, "/w/pkg/R/a.R", "first <- 1\n"); let b_source = "f <- function() {\n x\n}\n"; - let b = make_package_file(&mut db, "/w/pkg/R/b.R", b_source, pkg); - let c = make_package_file(&mut db, "/w/pkg/R/c.R", "second <- 2\n", pkg); + let b = make_file(&mut db, "/w/pkg/R/b.R", b_source); + let c = make_file(&mut db, "/w/pkg/R/c.R", "second <- 2\n"); pkg.set_files(&mut db).to(vec![a, b, c]); // Cursor inside f's body. Full lazy view (same as `imports()`). @@ -190,10 +186,10 @@ fn test_package_top_level_predecessors_appear_in_lifo_order() { let _ = install_packages(&mut db, &["base"]); let pkg = install_workspace_package(&mut db, "pkg"); - let a = make_package_file(&mut db, "/w/pkg/R/a.R", "first <- 1\n", pkg); - let b = make_package_file(&mut db, "/w/pkg/R/b.R", "second <- 2\n", pkg); + let a = make_file(&mut db, "/w/pkg/R/a.R", "first <- 1\n"); + let b = make_file(&mut db, "/w/pkg/R/b.R", "second <- 2\n"); let c_source = "x <- 1\n"; - let c = make_package_file(&mut db, "/w/pkg/R/c.R", c_source, pkg); + let c = make_file(&mut db, "/w/pkg/R/c.R", c_source); pkg.set_files(&mut db).to(vec![a, b, c]); let offset = TextSize::from(c_source.find('x').unwrap() as u32); @@ -209,7 +205,7 @@ fn test_package_namespace_and_base_layers_always_visible() { let base = packages[0]; let pkg = install_workspace_package(&mut db, "pkg"); - let file = make_package_file(&mut db, "/w/pkg/R/a.R", "x <- 1\n", pkg); + let file = make_file(&mut db, "/w/pkg/R/a.R", "x <- 1\n"); pkg.set_files(&mut db).to(vec![file]); let layers = file.imports_at(&db, TextSize::from(0)); diff --git a/crates/oak_db/src/tests/file_resolve.rs b/crates/oak_db/src/tests/file_resolve.rs index 00269c779..af842f973 100644 --- a/crates/oak_db/src/tests/file_resolve.rs +++ b/crates/oak_db/src/tests/file_resolve.rs @@ -15,7 +15,7 @@ fn setup_workspace(db: &mut TestDb, scripts: &[(&str, &str)]) -> Vec { let root = workspace_root(db, "w"); let files: Vec = scripts .iter() - .map(|(name, contents)| File::new(db, file_url(name), contents.to_string(), None)) + .map(|(name, contents)| File::new(db, file_url(name), contents.to_string())) .collect(); root.set_scripts(db).to(files.clone()); db.workspace_roots().set_roots(db).to(vec![root]); @@ -289,18 +289,8 @@ fn test_resolve_unbound_name_in_package_does_not_cycle() { None, ); - let a = File::new( - &db, - file_url("/w/pkg/R/a.R"), - "x <- 1\n".to_string(), - Some(pkg), - ); - let b = File::new( - &db, - file_url("/w/pkg/R/b.R"), - "y <- 2\n".to_string(), - Some(pkg), - ); + let a = File::new(&db, file_url("/w/pkg/R/a.R"), "x <- 1\n".to_string()); + let b = File::new(&db, file_url("/w/pkg/R/b.R"), "y <- 2\n".to_string()); 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]); @@ -328,17 +318,11 @@ fn test_resolve_walks_package_files_for_lazy_lookups() { None, ); - let a = File::new( - &db, - file_url("/w/pkg/R/a.R"), - "shared <- 1\n".to_string(), - Some(pkg), - ); + let a = File::new(&db, file_url("/w/pkg/R/a.R"), "shared <- 1\n".to_string()); let b = File::new( &db, file_url("/w/pkg/R/b.R"), "use_shared <- function() shared\n".to_string(), - Some(pkg), ); pkg.set_files(&mut db).to(vec![a, b]); workspace.set_packages(&mut db).to(vec![pkg]); diff --git a/crates/oak_db/src/tests/file_resolve_at.rs b/crates/oak_db/src/tests/file_resolve_at.rs index 395ccb58a..01559e390 100644 --- a/crates/oak_db/src/tests/file_resolve_at.rs +++ b/crates/oak_db/src/tests/file_resolve_at.rs @@ -11,11 +11,7 @@ use crate::Package; use crate::Root; fn make_file(db: &mut TestDb, path: &str, contents: &str) -> File { - File::new(db, file_url(path), contents.to_string(), None) -} - -fn make_package_file(db: &mut TestDb, path: &str, contents: &str, package: Package) -> File { - File::new(db, file_url(path), contents.to_string(), Some(package)) + File::new(db, file_url(path), contents.to_string()) } /// Set up a workspace root with the given scripts (top-level files with @@ -146,9 +142,9 @@ fn test_resolves_package_sibling_predecessor() { let mut db = TestDb::new(); let (_root, pkg) = install_workspace_package(&mut db, "pkg"); - let a = make_package_file(&mut db, "workspace/pkg/R/a.R", "shared <- 1\n", pkg); + let a = make_file(&mut db, "workspace/pkg/R/a.R", "shared <- 1\n"); let b_source = "use_shared <- function() shared\n"; - let b = make_package_file(&mut db, "workspace/pkg/R/b.R", b_source, pkg); + let b = make_file(&mut db, "workspace/pkg/R/b.R", b_source); pkg.set_files(&mut db).to(vec![a, b]); // Cursor on `shared` inside `b`'s function body. Lexical walk finds diff --git a/crates/oak_db/src/tests/file_root.rs b/crates/oak_db/src/tests/file_root.rs index 094b8c1f5..7faa65ab3 100644 --- a/crates/oak_db/src/tests/file_root.rs +++ b/crates/oak_db/src/tests/file_root.rs @@ -12,7 +12,7 @@ use crate::Package; #[test] fn test_root_returns_none_for_orphan_file_outside_workspace() { let db = OakDatabase::new(); - let file = File::new(&db, file_url("orphan.R"), String::new(), None); + let file = File::new(&db, file_url("orphan.R"), String::new()); assert_eq!(file.root(&db), None); } @@ -23,7 +23,7 @@ fn test_root_finds_containing_workspace_for_orphan_file() { let workspace = workspace_root(&db, "proj"); db.workspace_roots().set_roots(&mut db).to(vec![workspace]); - let file = File::new(&db, file_url("proj/scripts/foo.R"), String::new(), None); + let file = File::new(&db, file_url("proj/scripts/foo.R"), String::new()); assert_eq!(file.root(&db), Some(workspace)); } @@ -36,10 +36,10 @@ fn test_root_returns_longest_prefix_for_orphan_file() { .set_roots(&mut db) .to(vec![outer, inner]); - let inner_file = File::new(&db, file_url("proj/inner/foo.R"), String::new(), None); + let inner_file = File::new(&db, file_url("proj/inner/foo.R"), String::new()); assert_eq!(inner_file.root(&db), Some(inner)); - let outer_file = File::new(&db, file_url("proj/foo.R"), String::new(), None); + let outer_file = File::new(&db, file_url("proj/foo.R"), String::new()); assert_eq!(outer_file.root(&db), Some(outer)); } @@ -56,18 +56,14 @@ fn test_root_dispatches_through_library_package_when_set() { Vec::new(), None, ); + // File placed in `pkg.files`. `root()` derives the package from that + // containment and dispatches through `Db::root_by_package` rather than + // falling back to the URL-prefix walk against workspace roots. + let file = File::new(&db, file_url("libs/mypkg/R/foo.R"), String::new()); + pkg.set_files(&mut db).to(vec![file]); pkg_root.set_packages(&mut db).to(vec![pkg]); db.library_roots().set_roots(&mut db).to(vec![pkg_root]); - // File created with package back-pointer set. `root()` dispatches - // through `Db::root_by_package` rather than falling back to the URL- - // prefix walk against workspace roots. - let file = File::new( - &db, - file_url("libs/mypkg/R/foo.R"), - String::new(), - Some(pkg), - ); assert_eq!(file.root(&db), Some(pkg_root)); } @@ -75,7 +71,7 @@ fn test_root_dispatches_through_library_package_when_set() { fn test_root_dispatches_through_workspace_package_when_set() { // Same dispatch as the library case, but the owning root is a // `Workspace` kind. The URL-prefix fallback is *not* consulted here - // because `package` is set. + // because the file belongs to a package. let mut db = OakDatabase::new(); let pkg_root = workspace_root(&db, "proj"); let pkg = Package::new( @@ -87,9 +83,10 @@ fn test_root_dispatches_through_workspace_package_when_set() { Vec::new(), None, ); + let file = File::new(&db, file_url("proj/R/foo.R"), String::new()); + pkg.set_files(&mut db).to(vec![file]); pkg_root.set_packages(&mut db).to(vec![pkg]); db.workspace_roots().set_roots(&mut db).to(vec![pkg_root]); - let file = File::new(&db, file_url("proj/R/foo.R"), String::new(), Some(pkg)); assert_eq!(file.root(&db), Some(pkg_root)); } diff --git a/crates/oak_db/src/tests/inputs.rs b/crates/oak_db/src/tests/inputs.rs index efa628bf0..bbc394ae2 100644 --- a/crates/oak_db/src/tests/inputs.rs +++ b/crates/oak_db/src/tests/inputs.rs @@ -42,7 +42,7 @@ fn make_installed_package(db: &mut OakDatabase, name: &str) -> (Root, Package) { } fn make_script(db: &mut OakDatabase, name: &str) -> File { - File::new(db, file_url(name), String::new(), None) + File::new(db, file_url(name), String::new()) } #[test] diff --git a/crates/oak_db/src/tests/resolver.rs b/crates/oak_db/src/tests/resolver.rs index 4f4f6370b..72c5207e2 100644 --- a/crates/oak_db/src/tests/resolver.rs +++ b/crates/oak_db/src/tests/resolver.rs @@ -11,7 +11,7 @@ use crate::File; use crate::Root; fn make_script(db: &mut TestDb, name: &str, contents: &str) -> File { - File::new(db, file_url(name), contents.to_string(), None) + File::new(db, file_url(name), contents.to_string()) } /// Build a fresh workspace root, attach the given scripts, register @@ -328,9 +328,8 @@ fn test_source_anchors_relative_to_workspace_root() { &db, file_url("proj/sub/a.R"), "source(\"b.R\")\n".to_string(), - None, ); - let b = File::new(&db, file_url("proj/b.R"), "x <- 1\n".to_string(), None); + let b = File::new(&db, file_url("proj/b.R"), "x <- 1\n".to_string()); root.set_scripts(&mut db).to(vec![a, b]); db.workspace_roots().set_roots(&mut db).to(vec![root]); @@ -343,13 +342,8 @@ fn test_source_anchors_to_parent_dir_when_no_workspace() { // Calling file isn't under any workspace root, so the anchor // falls back to the file's own parent directory. let mut db = TestDb::new(); - let a = File::new( - &db, - file_url("dir/a.R"), - "source(\"b.R\")\n".to_string(), - None, - ); - let b = File::new(&db, file_url("dir/b.R"), "x <- 1\n".to_string(), None); + let a = File::new(&db, file_url("dir/a.R"), "source(\"b.R\")\n".to_string()); + let b = File::new(&db, file_url("dir/b.R"), "x <- 1\n".to_string()); db.orphan_root().set_files(&mut db).to(vec![a, b]); let index = a.semantic_index(&db); @@ -365,9 +359,8 @@ fn test_source_path_with_parent_dir_segments() { &db, file_url("dir/sub/a.R"), "source(\"../b.R\")\n".to_string(), - None, ); - let b = File::new(&db, file_url("dir/b.R"), "x <- 1\n".to_string(), None); + let b = File::new(&db, file_url("dir/b.R"), "x <- 1\n".to_string()); db.orphan_root().set_files(&mut db).to(vec![a, b]); let index = a.semantic_index(&db); diff --git a/crates/oak_scan/src/inputs.rs b/crates/oak_scan/src/inputs.rs index bbc4e8ea0..88df692b2 100644 --- a/crates/oak_scan/src/inputs.rs +++ b/crates/oak_scan/src/inputs.rs @@ -9,12 +9,11 @@ //! entities for goto-def) stable across changes that don't actually //! touch a given file's content. //! -//! The trade-off is a small placement invariant: `file.package` must -//! agree with which container Vec holds the file (`pkg.files`, -//! `root.scripts`, or `orphan_root().files`). Outside callers should -//! not call `file.set_package(...)` directly. This crate is the only -//! intended caller of the placement-affecting setters on `oak_db`'s -//! input structs. +//! Placement is single-source-of-truth: a file belongs to whichever +//! container Vec holds it (`pkg.files`, `root.scripts`, or +//! `orphan_root().files`), and `File::package` derives ownership from +//! that. These helpers keep a file in exactly one container as it moves, +//! so the derived lookup stays unambiguous. use std::collections::HashSet; use std::path::PathBuf; @@ -168,20 +167,19 @@ impl RootExt for Root { fn upsert_file(db: &mut DB, package: Option, entry: FileEntry) -> File { if let Some(old) = db.file_by_url(&entry.url) { - // Two cleanups before handing the file to the caller, which will place - // it in a new container: + // The caller will place this file in `package`'s `files` vec. Two + // cleanups keep the file in exactly one live container, so the + // derived `File::package` stays unambiguous: // - // - If the package backpointer changed and the old package was Some, - // the old package's `files` vec still references this file. Drop it, - // otherwise that `Package` would carry a stale entry until its next - // wholesale rescan. + // - If the file currently belongs to a *different* package, drop it + // from that package's `files` vec; otherwise both would list it. + // Normally a no-op, since a file's package is fixed by its path: + // this only fires in the pathological nested-root case where two + // packages claim the same URL. // - // - If the file was in `OrphanRoot.files` (typically because the editor - // had it open before a scan classified it), remove it. The placement - // invariant for orphan says `file.package == None`, and we're about - // to set the package. + // - If the file was in `OrphanRoot.files` (typically because the + // editor had it open before a scan classified it), remove it. let old_package = old.package(db); - old.set_package(db).to(package); if old_package != package { if let Some(old_pkg) = old_package { remove_from_pkg_files(db, old_pkg, old); @@ -194,14 +192,13 @@ fn upsert_file(db: &mut DB, package: Option, entry: if let Some(stale) = stale_file_by_url(db, &entry.url) { // Resurrecting an evicted File. Restore disk contents (the editor-owned // variant lives in `orphan_root` instead; this branch only sees - // scanner-discovered files). + // scanner-discovered files). The caller places it into `package.files`. stale.set_contents(db).to(entry.contents); - stale.set_package(db).to(package); remove_from_stale_files(db, stale); return stale; } - File::new(db, entry.url, entry.contents, package) + File::new(db, entry.url, entry.contents) } fn remove_from_pkg_files(db: &mut DB, pkg: Package, file: File) { diff --git a/crates/oak_scan/src/stale.rs b/crates/oak_scan/src/stale.rs index 0fb076a8d..f6ff49332 100644 --- a/crates/oak_scan/src/stale.rs +++ b/crates/oak_scan/src/stale.rs @@ -10,7 +10,8 @@ //! its contents until `didClose`. //! //! `OrphanRoot` has no `packages` field, so an evicted package file -//! loses its package association: `file.package` clears to `None` and +//! loses its package association: with the file in orphan and its old +//! package no longer in a live root, `File::package` derives `None` and //! analysis treats it as a standalone script for as long as the //! workspace is removed. If the workspace comes back, `upsert_file` //! finds the same `File` via `OrphanRoot` and re-promotes it into @@ -57,14 +58,9 @@ pub(crate) fn set_root_stale( all_files.extend(pkg.files(db).iter().copied()); } - // Clear `file.package` first: by the time these files land in their new - // home, their old `Package` entity is itself in stale and the backpointer - // would lie about live containment. Setting to `None` here keeps the - // placement invariant honest. - for &file in &all_files { - file.set_package(db).to(None); - } - + // Ownership is derived from live containment, so there's no back-pointer + // to clear: once these files leave the root (to orphan / stale) and the + // root's packages are emptied below, `File::package` derives `None`. let (to_orphan, to_stale): (Vec, Vec) = all_files .into_iter() .partition(|f| editor_owned.is_some_and(|owned| owned.contains(f.url(db)))); diff --git a/crates/oak_scan/src/tests/stale.rs b/crates/oak_scan/src/tests/stale.rs index 756461492..c9147578c 100644 --- a/crates/oak_scan/src/tests/stale.rs +++ b/crates/oak_scan/src/tests/stale.rs @@ -25,7 +25,7 @@ fn file_url(s: &str) -> UrlId { fn test_set_stale_routes_editor_owned_to_orphan() { let mut db = OakDatabase::new(); let root = Root::new(&db, file_url("/proj"), RootKind::Workspace, vec![], vec![]); - let file = File::new(&db, file_url("/proj/foo.R"), "x".to_string(), None); + let file = File::new(&db, file_url("/proj/foo.R"), "x".to_string()); root.set_scripts(&mut db).to(vec![file]); db.workspace_roots().set_roots(&mut db).to(vec![root]); @@ -42,7 +42,7 @@ fn test_set_stale_routes_editor_owned_to_orphan() { fn test_set_stale_routes_non_editor_owned_to_stale() { let mut db = OakDatabase::new(); let root = Root::new(&db, file_url("/proj"), RootKind::Workspace, vec![], vec![]); - let file = File::new(&db, file_url("/proj/foo.R"), "x".to_string(), None); + let file = File::new(&db, file_url("/proj/foo.R"), "x".to_string()); root.set_scripts(&mut db).to(vec![file]); db.workspace_roots().set_roots(&mut db).to(vec![root]); @@ -68,7 +68,7 @@ fn test_set_stale_clears_package_on_editor_owned_package_file() { vec![], None, ); - let file = File::new(&db, file_url("/proj/R/a.R"), "x".to_string(), Some(pkg)); + let file = File::new(&db, file_url("/proj/R/a.R"), "x".to_string()); pkg.set_files(&mut db).to(vec![file]); root.set_packages(&mut db).to(vec![pkg]); db.workspace_roots().set_roots(&mut db).to(vec![root]); diff --git a/crates/oak_scan/tests/library.rs b/crates/oak_scan/tests/library.rs index 6d05f0a2b..2042417d6 100644 --- a/crates/oak_scan/tests/library.rs +++ b/crates/oak_scan/tests/library.rs @@ -471,7 +471,7 @@ fn test_upsert_re_promotes_editor_owned_file_from_orphan() { // Editor opens the file before any scan -> orphan. let r_url = file_url("/lib/pkg/R/a.R"); - let file = File::new(&db, r_url.clone(), "editor content".to_string(), None); + let file = File::new(&db, r_url.clone(), "editor content".to_string()); db.orphan_root().set_files(&mut db).to(vec![file]); assert_eq!(file.package(&db), None); @@ -489,8 +489,12 @@ fn test_upsert_re_promotes_editor_owned_file_from_orphan() { }], None, ); + // Wire the package into a live root so `File::package` (derived from + // live containment) can see it, mirroring the real scanner. + register_package(&mut db, lib, pkg); - // Same `File` entity, editor content preserved, package backpointer set. + // Same `File` entity, editor content preserved, package ownership + // derived from `pkg.files`. let pkg_file = pkg.files(&db)[0]; assert_eq!(pkg_file, file); assert_eq!(file.contents(&db), "editor content");