From 83c621e804b94c33ab433fc4af12711ccf8b5a8e Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 15 Sep 2026 18:02:44 +0200 Subject: [PATCH 1/4] Use a restricted `ResolverDb` in source resolution --- Cargo.toml | 3 + crates/oak_db/src/file_imports.rs | 24 ++-- crates/oak_db/src/imports.rs | 101 ++++++++------- crates/oak_db/src/imports/resolver_probe.rs | 77 ++++++++++++ crates/oak_db/src/lib.rs | 1 + crates/oak_db/src/resolver_db.rs | 117 ++++++++++++++++++ crates/oak_db/src/tests.rs | 2 + crates/oak_db/src/tests/file_imports.rs | 5 +- crates/oak_db/src/tests/resolver_boundary.rs | 64 ++++++++++ ...ary__prohibited_forms_fail_to_compile.snap | 15 +++ crates/oak_db/src/tests/source_firewall.rs | 108 ++++++++++++++++ crates/oak_db/src/tests/test_db.rs | 27 ++-- 12 files changed, 483 insertions(+), 61 deletions(-) create mode 100644 crates/oak_db/src/imports/resolver_probe.rs create mode 100644 crates/oak_db/src/resolver_db.rs create mode 100644 crates/oak_db/src/tests/resolver_boundary.rs create mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap create mode 100644 crates/oak_db/src/tests/source_firewall.rs diff --git a/Cargo.toml b/Cargo.toml index c2bd7fb95..6fdc24def 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,9 @@ edition = "2021" license = "MIT" authors = ["Posit Software, PBC"] +[workspace.lints.rust] +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(resolver_boundary, values("probe", "control"))'] } + [workspace.lints.clippy] result_large_err = "allow" too_many_arguments = "allow" diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 74ebe3f13..4de6e3f8e 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -18,6 +18,7 @@ use crate::load_context::LoadKind; use crate::load_context::SearchPathTail; use crate::recovery::record; use crate::recovery::Recovery; +use crate::resolver_db::FoundationDb; use crate::Db; use crate::File; use crate::Package; @@ -80,7 +81,7 @@ impl CrossFileLayers { /// layers (which outrank them) and the rest of the search path. pub(crate) fn lookup_order<'a>( &'a self, - db: &'a dyn Db, + db: FoundationDb<'_>, own: &'a [ImportLayer], ) -> impl Iterator + 'a { self.enclosing @@ -93,7 +94,7 @@ impl CrossFileLayers { } impl SearchPathTail { - fn layers(self, db: &dyn Db) -> Vec { + fn layers(self, db: FoundationDb<'_>) -> Vec { match self { SearchPathTail::Base => base_layer(db).into_iter().collect(), SearchPathTail::Default => default_search_path_layers(db), @@ -315,7 +316,7 @@ impl File { fn imports_in(self, db: &dyn Db, view: ImportView<'_>) -> Vec { let layers = self.resolution_layers(db, view.collation); let own = self.attach_layers(db, view.attaches); - layers.lookup_order(db, &own).collect() + layers.lookup_order(FoundationDb::new(db), &own).collect() } /// The file's own layers and the layers it inherits from the files that @@ -382,7 +383,7 @@ impl File { let own = self.attach_layers(db, view.attaches); self.layers_by_sourcing_file(db, view.collation) .into_iter() - .map(|layers| layers.lookup_order(db, &own).collect()) + .map(|layers| layers.lookup_order(FoundationDb::new(db), &own).collect()) .collect() } @@ -399,7 +400,14 @@ impl File { let own = self.attach_layers(db, AttachView::Anywhere); self.inherited_layers(db, CollationView::Deferred) .iter() - .map(|site| (site.file, site.layers.lookup_order(db, &own).collect())) + .map(|site| { + ( + site.file, + site.layers + .lookup_order(FoundationDb::new(db), &own) + .collect(), + ) + }) .collect() } @@ -443,7 +451,7 @@ impl File { pub(crate) fn standalone_imports(self, db: &dyn Db) -> Vec { let own = self.attach_layers(db, AttachView::Anywhere); self.cross_file_layers(db, CollationView::Deferred) - .lookup_order(db, &own) + .lookup_order(FoundationDb::new(db), &own) .collect() } @@ -780,13 +788,13 @@ fn extend_with_namespace_package_imports( /// `base`, always the last thing R searches. `None` when it isn't scanned into /// any root (the R system library is normally on `.libPaths()`, so it is). -fn base_layer(db: &dyn Db) -> Option { +fn base_layer(db: FoundationDb<'_>) -> Option { db.package_by_name("base").map(ImportLayer::Package) } /// The default startup search path as `Package` layers, `stats` first through /// `base` last. Packages absent from every root drop out. -fn default_search_path_layers(db: &dyn Db) -> Vec { +fn default_search_path_layers(db: FoundationDb<'_>) -> Vec { crate::search::DEFAULT_SEARCH_PATH_PACKAGES .iter() .filter_map(|name| db.package_by_name(name).map(ImportLayer::Package)) diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index 3b657ec71..df9e41fdd 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -16,11 +16,15 @@ use crate::directory::files_in_directory; use crate::directory::files_in_directory_recursive; use crate::file_imports::CollationView; use crate::file_imports::ImportLayer; +use crate::resolver_db::FoundationDb; +use crate::resolver_db::ResolverDb; use crate::Db; use crate::File; use crate::Package; use crate::RootKind; +mod resolver_probe; + /// Salsa-backed [`ImportsResolver`] consumed by the per-file semantic /// index builder. One instance per call to [`File::semantic_index`]. /// @@ -45,7 +49,7 @@ use crate::RootKind; /// [`File::semantic_index`]'s doc for the recovery behaviour (custom rebuild on /// `semantic_index()`, empty fallback on `exports()`). pub(crate) struct SalsaImportsResolver<'db> { - db: &'db dyn Db, + db: ResolverDb<'db>, /// The file currently being indexed. file: File, cache: EffectsCache, @@ -54,7 +58,7 @@ pub(crate) struct SalsaImportsResolver<'db> { impl<'db> SalsaImportsResolver<'db> { pub(crate) fn new(db: &'db dyn Db, file: File) -> Self { Self { - db, + db: ResolverDb::new(db), file, cache: EffectsCache::default(), } @@ -66,23 +70,18 @@ impl<'db> SalsaImportsResolver<'db> { // Sort to prevent an unrelated export from renumbering `Import` // definitions. `record_binding()` anchors every name at the same // `source()` call, so the original order has no semantic meaning. - let mut names: Vec = file - .exports(self.db) + let mut names: Vec = self + .db + .exports(file) .iter() .map(|(name, _)| name.to_string()) .collect(); names.sort(); - let packages: Vec = file - .attached_packages(self.db) - .iter() - .map(|name| name.text(self.db).to_string()) - .collect(); - SourceResolution { - url: file.path(self.db).to_url(), + url: self.db.foundation().file_path(file).to_url(), names, - packages, + packages: self.db.attached_package_names(file), } } } @@ -106,7 +105,7 @@ pub(crate) fn source_dir_scripts( path: String, walk: DirWalk, ) -> Vec { - let Some(anchor) = anchor_dir(db, file) else { + let Some(anchor) = anchor_dir(FoundationDb::new(db), file) else { return Vec::new(); }; let Some(target_path) = resolve_relative_to(&anchor, &path) else { @@ -145,7 +144,7 @@ impl EffectsCache { impl<'db> ImportsResolver for SalsaImportsResolver<'db> { fn resolve_source(&mut self, path: &str) -> Option { - let anchor = anchor_dir(self.db, self.file)?; + let anchor = anchor_dir(self.db.foundation(), self.file)?; let target_path = resolve_relative_to(&anchor, path)?; // TODO: a `source()` target outside every workspace root never becomes // a `File`, so `file_by_path()` misses it and the names it injects stay @@ -159,12 +158,14 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { // edit), plus GC to drop the orphan once the directive goes away. // TODO(diagnostics): Until we support out-of-workspace sourced files, // should we at least lint so user knows that we can't analyse the file? - let file = self.db.file_by_path(&target_path)?; + let file = self.db.foundation().file_by_path(&target_path)?; Some(self.source_resolution(file)) } fn resolve_source_dir(&mut self, path: &str, walk: DirWalk) -> Vec { - source_dir_scripts(self.db, self.file, path.to_string(), walk) + self.db + .foundation() + .source_dir_scripts(self.file, path.to_string(), walk) .iter() .copied() // Exclude sourcing file @@ -183,7 +184,7 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { } fn package_exists(&mut self, package: &str) -> bool { - self.db.package_by_name(package).is_some() + self.db.foundation().package_by_name(package).is_some() } } @@ -222,7 +223,8 @@ impl<'db> SalsaImportsResolver<'db> { /// reading a successor's `exports` doesn't cycle. Unlike the ambiguities the /// builder records, this one can't be seen from inside the file. fn resolve_effects_uncached(&self, name: &str, attached: &[String]) -> Option { - let layers = self.file.cross_file_layers(self.db, CollationView::Eager); + let layers = self.db.cross_file_layers(self.file, CollationView::Eager); + let foundation = self.db.foundation(); // The file's own attaches slot between the definition/namespace band // and the rest of the search path, exactly as in `File::imports`. @@ -232,10 +234,14 @@ impl<'db> SalsaImportsResolver<'db> { let own: Vec = attached .iter() .rev() - .filter_map(|package| self.db.package_by_name(package).map(ImportLayer::Package)) + .filter_map(|package| { + foundation + .package_by_name(package) + .map(ImportLayer::Package) + }) .collect(); - for layer in layers.lookup_order(self.db, &own) { + for layer in layers.lookup_order(foundation, &own) { if let ControlFlow::Break(effect) = layer_effect(self.db, &layer, name) { return effect.copied(); } @@ -260,7 +266,7 @@ pub(crate) fn resolve_effect( name: &str, ) -> Option<&'static EffectsHandlers> { for layer in layers { - if let ControlFlow::Break(effect) = layer_effect(db, layer, name) { + if let ControlFlow::Break(effect) = layer_effect(ResolverDb::new(db), layer, name) { return effect; } } @@ -272,14 +278,15 @@ pub(crate) fn resolve_effect( /// A binding without an effect stops the search because it shadows deeper /// layers. A nonbinding layer continues the search. fn layer_effect( - db: &dyn Db, + db: ResolverDb<'_>, layer: &ImportLayer, name: &str, ) -> ControlFlow> { + let foundation = db.foundation(); match layer { // A definition shadows any deeper effect. Own-file definitions never // reach here, the builder handles them before calling us. - ImportLayer::File(file) => match file.exports(db).get(name).is_some() { + ImportLayer::File(file) => match db.exports(*file).get(name).is_some() { true => ControlFlow::Break(None), false => ControlFlow::Continue(()), }, @@ -290,13 +297,13 @@ fn layer_effect( file, exports_so_far, } => { - let binds = exports_so_far.contains(name) && file.exports(db).get(name).is_some(); + let binds = exports_so_far.contains(name) && db.exports(*file).get(name).is_some(); match binds { true => ControlFlow::Break(None), false => ControlFlow::Continue(()), } }, - ImportLayer::Package(package) => match package_binding(db, *package, name) { + ImportLayer::Package(package) => match package_binding(foundation, *package, name) { PackageBinding::Effect(effects) => ControlFlow::Break(Some(effects)), PackageBinding::Shadow => ControlFlow::Break(None), PackageBinding::Absent => ControlFlow::Continue(()), @@ -304,17 +311,20 @@ fn layer_effect( // A NAMESPACE `importFrom` binds `name` unconditionally (that's what // the directive asserts), so it always shadows the search path // below. Its effect, if any, comes from the source package. - ImportLayer::From(importer) => match importer.imported_from(db).get(name) { - Some(source) => { - let effect = db.package_by_name(source).and_then(|package| { - match package_binding(db, package, name) { - PackageBinding::Effect(effects) => Some(effects), - PackageBinding::Shadow | PackageBinding::Absent => None, - } - }); - ControlFlow::Break(effect) - }, - None => ControlFlow::Continue(()), + ImportLayer::From(importer) => { + match foundation.package_imported_from(*importer).get(name) { + Some(source) => { + let effect = + foundation.package_by_name(source).and_then( + |package| match package_binding(foundation, package, name) { + PackageBinding::Effect(effects) => Some(effects), + PackageBinding::Shadow | PackageBinding::Absent => None, + }, + ); + ControlFlow::Break(effect) + }, + None => ControlFlow::Continue(()), + } }, } } @@ -323,8 +333,8 @@ fn layer_effect( /// only shadows, or nothing. The re-export chase is one hop through an /// `importFrom`, since a re-exported function's annotation lives under its /// original package, not the re-exporter. -fn package_binding(db: &dyn Db, package: Package, name: &str) -> PackageBinding { - let package_name = package.name(db).as_str(); +fn package_binding(db: FoundationDb<'_>, package: Package, name: &str) -> PackageBinding { + let package_name = db.package_name(package); if let Some(effects) = effects::lookup(package_name, name) { return PackageBinding::Effect(effects); } @@ -339,13 +349,13 @@ fn package_binding(db: &dyn Db, package: Package, name: &str) -> PackageBinding // export gate `Package::resolve` applies. A name it `importFrom`s // without re-exporting isn't visible to a caller that attaches or // imports this package (R errors "could not find function"). - let namespace = package.namespace(db); + let namespace = db.package_namespace(package); if !namespace.exports.contains_str(name) { return PackageBinding::Absent; } // Exports `name`, so it binds. Chase a re-export for the effect; a plain // own definition (no matching `importFrom`) only shadows. - match package.imported_from(db).get(name) { + match db.package_imported_from(package).get(name) { Some(source) => match effects::lookup(source, name) { Some(effects) => PackageBinding::Effect(effects), None => PackageBinding::Shadow, @@ -360,13 +370,16 @@ fn package_binding(db: &dyn Db, package: Package, name: &str) -> PackageBinding /// resolves `source("foo.R")` against `getwd()`, and IDEs (RStudio, Positron) /// `setwd()` to the project root, so workspace-root anchoring typically matches /// the runtime behaviour. -fn anchor_dir(db: &dyn Db, file: File) -> Option { - if let Some(root) = file.root(db).filter(|r| r.kind(db) == RootKind::Workspace) { +fn anchor_dir(db: FoundationDb<'_>, file: File) -> Option { + if let Some(root) = db + .file_root(file) + .filter(|root| db.root_kind(*root) == RootKind::Workspace) + { // Workspace roots are file URLs by construction. - return root.path(db).as_path().map(Utf8Path::to_path_buf); + return db.root_path(root).as_path().map(Utf8Path::to_path_buf); } - let parent = file.path(db).as_path()?.parent()?; + let parent = db.file_path(file).as_path()?.parent()?; Some(parent.to_path_buf()) } diff --git a/crates/oak_db/src/imports/resolver_probe.rs b/crates/oak_db/src/imports/resolver_probe.rs new file mode 100644 index 000000000..23357ecc5 --- /dev/null +++ b/crates/oak_db/src/imports/resolver_probe.rs @@ -0,0 +1,77 @@ +//! These probes use the resolver's database field so weakening its type +//! also changes which calls compile. +//! +//! The boundary tests compile this crate with `resolver_boundary = "probe"` +//! to check that recursive queries and access to the underlying database are +//! rejected. A separate build with `resolver_boundary = "control"` checks +//! that `allowed_calls()` compiles, so unrelated errors +//! cannot make a rejected call look like successful enforcement. + +use super::SalsaImportsResolver; +#[cfg(any(resolver_boundary = "probe", resolver_boundary = "control"))] +use crate::file_imports::CollationView; +#[cfg(resolver_boundary = "probe")] +use crate::Db; + +impl<'db> SalsaImportsResolver<'db> { + #[cfg(resolver_boundary = "probe")] + fn calls_semantic_index_directly(&self) { + let _ = self.file.semantic_index(self.db); + } + + #[cfg(resolver_boundary = "probe")] + fn calls_diagnostics_directly(&self) { + let _ = self.file.diagnostics(self.db); + } + + #[cfg(resolver_boundary = "probe")] + fn calls_imports_directly(&self) { + let _ = self.file.imports(self.db); + } + + #[cfg(resolver_boundary = "probe")] + fn escapes_to_dyn_db(&self) { + let _: &dyn Db = self.db; + } + + #[cfg(resolver_boundary = "probe")] + fn escapes_through_private_field(&self) { + let _: &dyn Db = self.db.foundation; + } + + #[cfg(resolver_boundary = "probe")] + fn foundation_calls_exports(&self) { + let _ = self.db.foundation().exports(self.file); + } + + #[cfg(resolver_boundary = "probe")] + fn foundation_calls_attached_packages(&self) { + let _ = self.db.foundation().attached_package_names(self.file); + } + + #[cfg(resolver_boundary = "probe")] + fn foundation_calls_cross_file_layers(&self) { + let _ = self + .db + .foundation() + .cross_file_layers(self.file, CollationView::Eager); + } + + #[cfg(resolver_boundary = "probe")] + fn foundation_escapes_to_dyn_db(&self) { + let _: &dyn Db = self.db.foundation(); + } + + #[cfg(resolver_boundary = "probe")] + fn foundation_escapes_through_private_field(&self) { + let _: &dyn Db = self.db.foundation().db; + } + + #[cfg(resolver_boundary = "control")] + fn allowed_calls(&self) { + let _ = self.db.exports(self.file); + let _ = self.db.attached_package_names(self.file); + let _ = self.db.cross_file_layers(self.file, CollationView::Eager); + let _ = self.db.foundation().file_path(self.file); + } +} diff --git a/crates/oak_db/src/lib.rs b/crates/oak_db/src/lib.rs index e07aae926..f551b9990 100644 --- a/crates/oak_db/src/lib.rs +++ b/crates/oak_db/src/lib.rs @@ -19,6 +19,7 @@ mod package; mod package_resolve; mod parse; mod recovery; +mod resolver_db; mod search; mod storage; mod workspace; diff --git a/crates/oak_db/src/resolver_db.rs b/crates/oak_db/src/resolver_db.rs new file mode 100644 index 000000000..cd8e6081b --- /dev/null +++ b/crates/oak_db/src/resolver_db.rs @@ -0,0 +1,117 @@ +//! Restricts database access during source and effect resolution. +//! +//! [`FoundationDb`] exposes operations that do not re-enter semantic analysis. +//! [`ResolverDb`] contains that wrapper and adds three recursive queries with +//! `cycle_result` handlers. Resolution can borrow the narrower capabilities +//! through [`ResolverDb::foundation()`] without exposing the underlying database. +//! +//! Both wrappers keep database access private to this module. Their forwarding +//! methods are trusted code, so changes require a transitive dependency review. +//! Recovery helpers must accept only [`FoundationDb`], not [`ResolverDb`]. + +use aether_path::FilePath; +use oak_package_metadata::namespace::Namespace; +use oak_semantic::effects::DirWalk; +use rustc_hash::FxHashMap; + +use crate::file_imports::CollationView; +use crate::file_imports::CrossFileLayers; +use crate::imports::source_dir_scripts; +use crate::Db; +use crate::File; +use crate::FileExports; +use crate::Package; +use crate::Root; +use crate::RootKind; + +#[derive(Clone, Copy)] +pub(crate) struct ResolverDb<'db> { + foundation: FoundationDb<'db>, +} + +impl<'db> ResolverDb<'db> { + pub(crate) fn new(db: &'db dyn Db) -> Self { + Self { + foundation: FoundationDb::new(db), + } + } + + pub(crate) fn foundation(self) -> FoundationDb<'db> { + self.foundation + } + + // These queries can re-enter semantic analysis and have recovery handlers. + + pub(crate) fn exports(self, file: File) -> &'db FileExports { + file.exports(self.foundation.db) + } + + /// Package names from `library()` calls at `file`'s own top level. + /// + /// Returning text keeps interned [`Name`](crate::Name) handles and their + /// database access inside this wrapper. + pub(crate) fn attached_package_names(self, file: File) -> Vec { + file.attached_packages(self.foundation.db) + .iter() + .map(|name| name.text(self.foundation.db).to_string()) + .collect() + } + + pub(crate) fn cross_file_layers(self, file: File, view: CollationView) -> &'db CrossFileLayers { + file.cross_file_layers(self.foundation.db, view) + } +} + +/// Inputs, per-root indices, and package metadata available without recursive +/// semantic queries. The raw database has no accessor, so callers cannot widen +/// this wrapper back to [`ResolverDb`] or `&dyn Db`. +#[derive(Clone, Copy)] +pub(crate) struct FoundationDb<'db> { + db: &'db dyn Db, +} + +impl<'db> FoundationDb<'db> { + pub(crate) fn new(db: &'db dyn Db) -> Self { + Self { db } + } + + pub(crate) fn file_by_path(self, path: &FilePath) -> Option { + Db::file_by_path(self.db, path) + } + + pub(crate) fn package_by_name(self, name: &str) -> Option { + Db::package_by_name(self.db, name) + } + + pub(crate) fn file_path(self, file: File) -> &'db FilePath { + file.path(self.db) + } + + pub(crate) fn file_root(self, file: File) -> Option { + file.root(self.db) + } + + pub(crate) fn root_kind(self, root: Root) -> RootKind { + root.kind(self.db) + } + + pub(crate) fn root_path(self, root: Root) -> &'db FilePath { + root.path(self.db) + } + + pub(crate) fn source_dir_scripts(self, file: File, path: String, walk: DirWalk) -> &'db [File] { + source_dir_scripts(self.db, file, path, walk) + } + + pub(crate) fn package_name(self, package: Package) -> &'db str { + package.name(self.db).as_str() + } + + pub(crate) fn package_namespace(self, package: Package) -> &'db Namespace { + package.namespace(self.db) + } + + pub(crate) fn package_imported_from(self, package: Package) -> &'db FxHashMap { + package.imported_from(self.db) + } +} diff --git a/crates/oak_db/src/tests.rs b/crates/oak_db/src/tests.rs index 59d1b0e85..2b998c703 100644 --- a/crates/oak_db/src/tests.rs +++ b/crates/oak_db/src/tests.rs @@ -17,6 +17,8 @@ mod inputs; mod package_resolve; mod recovery; mod resolver; +mod resolver_boundary; +mod source_firewall; pub(crate) mod test_db; mod tidy; mod workspace; diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index dcb78ce9c..bd88cdecb 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -4,6 +4,7 @@ use oak_package_metadata::namespace::Namespace; use salsa::Setter; use crate::file_imports::CollationView; +use crate::resolver_db::FoundationDb; use crate::tests::test_db::file_path; use crate::tests::test_db::library_root; use crate::tests::test_db::make_package; @@ -872,10 +873,10 @@ fn test_cross_file_layers_never_carries_inherited_layers() { for view in [CollationView::Eager, CollationView::Deferred] { let scan_side = helpers.cross_file_layers(&db, view); assert!(scan_side - .lookup_order(&db, &[]) + .lookup_order(FoundationDb::new(&db), &[]) .any(|layer| matches!(layer, ImportLayer::File(file) if file == sibling))); assert!(!scan_side - .lookup_order(&db, &[]) + .lookup_order(FoundationDb::new(&db), &[]) .any(|layer| matches!(layer, ImportLayer::SourcingFile { .. }))); } } diff --git a/crates/oak_db/src/tests/resolver_boundary.rs b/crates/oak_db/src/tests/resolver_boundary.rs new file mode 100644 index 000000000..1c3588197 --- /dev/null +++ b/crates/oak_db/src/tests/resolver_boundary.rs @@ -0,0 +1,64 @@ +//! Checks that the resolver's allowed calls compile and its prohibited calls +//! fail at the expected locations in [`crate::imports::resolver_probe`]. + +use std::env; +use std::process::Command; +use std::process::Output; + +/// Cargo reports diagnostic paths relative to the workspace root. +const PROBE_FILE: &str = "crates/oak_db/src/imports/resolver_probe.rs"; + +const PROHIBITED_FORM_COUNT: usize = 10; + +/// Passing the cfg through `cargo rustc` leaves dependencies cached. +/// Setting `RUSTFLAGS` would rebuild them too. `CARGO_TERM_COLOR` disables +/// color without conflicting with Cargo's rustc `--json` argument. +fn compile_with_cfg(value: &str) -> Output { + let cargo = env::var("CARGO").unwrap_or_else(|_| "cargo".to_string()); + Command::new(cargo) + .current_dir(env!("CARGO_MANIFEST_DIR")) + .env("CARGO_TERM_COLOR", "never") + .args([ + "rustc", + "-p", + "oak_db", + "--lib", + "--message-format=short", + "--", + "--cfg", + &format!("resolver_boundary=\"{value}\""), + "--emit=metadata", + ]) + .output() + .unwrap() +} + +#[test] +fn test_allowed_forms_compile() { + let output = compile_with_cfg("control"); + assert!(output.status.success()); +} + +#[test] +fn test_prohibited_forms_fail_to_compile() { + let output = compile_with_cfg("probe"); + assert!(!output.status.success()); + + // Use the same diagnostic paths on Windows and Unix for filtering and snapshots. + let stderr = String::from_utf8_lossy(&output.stderr).replace('\\', "/"); + // Only probe locations belong in the snapshot, not Cargo's error summary. + let mut lines: Vec<&str> = stderr + .lines() + .filter(|line| line.starts_with(PROBE_FILE)) + .collect(); + lines.sort_unstable(); + assert_eq!(lines.len(), PROHIBITED_FORM_COUNT); + + insta::with_settings!({description => "\ + Each line is one prohibited form rejected at the resolver or foundation boundary in \ + crates/oak_db/src/imports/resolver_probe.rs. Line numbers shift when that \ + module is edited. A NEW or MISSING line means the boundary itself changed and \ + needs review, not a blind `cargo insta accept`."}, { + insta::assert_snapshot!(lines.join("\n")); + }); +} diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap new file mode 100644 index 000000000..250cd646c --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap @@ -0,0 +1,15 @@ +--- +source: crates/oak_db/src/tests/resolver_boundary.rs +description: "Each line is one prohibited form rejected at the resolver or foundation boundary in crates/oak_db/src/imports/resolver_probe.rs. Line numbers shift when that module is edited. A NEW or MISSING line means the boundary itself changed and needs review, not a blind `cargo insta accept`." +expression: "lines.join(\"\\n\")" +--- +crates/oak_db/src/imports/resolver_probe.rs:19:42: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:24:39: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:29:35: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:34:26: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:39:34: error[E0616]: field `foundation` of struct `ResolverDb` is private: private field +crates/oak_db/src/imports/resolver_probe.rs:44:38: error[E0599]: no method named `exports` found for struct `FoundationDb<'db>` in the current scope: method not found in `FoundationDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:49:38: error[E0599]: no method named `attached_package_names` found for struct `FoundationDb<'db>` in the current scope +crates/oak_db/src/imports/resolver_probe.rs:57:14: error[E0599]: no method named `cross_file_layers` found for struct `FoundationDb<'db>` in the current scope: method not found in `FoundationDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:62:26: error[E0308]: mismatched types: expected `&dyn Db`, found `FoundationDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:67:47: error[E0616]: field `db` of struct `FoundationDb` is private: private field diff --git a/crates/oak_db/src/tests/source_firewall.rs b/crates/oak_db/src/tests/source_firewall.rs new file mode 100644 index 000000000..84a87b175 --- /dev/null +++ b/crates/oak_db/src/tests/source_firewall.rs @@ -0,0 +1,108 @@ +//! Body edits that preserve exports and attachments must not rebuild importers. +//! +//! Source resolution reads [`File::exports()`] and [`File::attached_packages()`]. +//! When both summaries compare equal after an edit, Salsa preserves their +//! change timestamps and reuses the importer's cached [`File::semantic_index()`]. +//! Changes to either summary must rebuild the importer. +//! +//! Per-file execution counts distinguish the target's necessary rebuild from +//! an unnecessary importer rebuild caused by reading the target's full index. + +use salsa::Setter; + +use crate::tests::file_imports::install_packages; +use crate::tests::test_db::file_path; +use crate::tests::test_db::workspace_root; +use crate::tests::test_db::TestDb; +use crate::DbInputs; +use crate::File; +use crate::FileRevision; + +const INDEX: &str = "File::semantic_index"; + +fn source_pair(helpers_text: &str) -> (TestDb, File, File) { + let mut db = TestDb::new(); + install_packages(&mut db, &["dplyr"]); + let root = workspace_root(&db, "w"); + + let main = File::new( + &db, + file_path("w/main.R"), + FileRevision::zero(), + Some("source(\"helpers.R\")\nf <- function() 1\n".to_string()), + None, + ); + let helpers = File::new( + &db, + file_path("w/helpers.R"), + FileRevision::zero(), + Some(helpers_text.to_string()), + None, + ); + + root.set_scripts(&mut db).to(vec![main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + (db, main, helpers) +} + +fn warm(db: &TestDb, main: File) -> usize { + let _ = main.semantic_index(db); + db.executions_for(INDEX, main) +} + +#[test] +fn test_body_edit_in_a_sourced_file_does_not_rebuild_the_importer_index() { + let (mut db, main, helpers) = source_pair("y <- function() 1\n"); + let before = warm(&db, main); + assert_eq!(before, 1); + assert_eq!(db.executions_for(INDEX, helpers), 1); + + helpers + .set_source_text_override(&mut db) + .to(Some("y <- function() 2\n".to_string())); + let _ = main.semantic_index(&db); + + assert_eq!(db.executions_for(INDEX, main), before); + assert_eq!(db.executions_for(INDEX, helpers), 2); +} + +#[test] +fn test_attach_preserving_body_edit_in_a_sourced_file_does_not_rebuild_the_importer_index() { + let (mut db, main, helpers) = source_pair("library(dplyr)\ny <- function() 1\n"); + let before = warm(&db, main); + + helpers + .set_source_text_override(&mut db) + .to(Some("library(dplyr)\ny <- function() 2\n".to_string())); + let _ = main.semantic_index(&db); + + assert_eq!(db.executions_for(INDEX, main), before); + assert_eq!(db.executions_for(INDEX, helpers), 2); +} + +#[test] +fn test_renaming_an_export_in_a_sourced_file_rebuilds_the_importer_index() { + let (mut db, main, helpers) = source_pair("y <- function() 1\n"); + let before = warm(&db, main); + + helpers + .set_source_text_override(&mut db) + .to(Some("z <- function() 1\n".to_string())); + let _ = main.semantic_index(&db); + + assert_eq!(db.executions_for(INDEX, main), before + 1); +} + +#[test] +fn test_adding_an_attach_to_a_sourced_file_rebuilds_the_importer_index() { + let (mut db, main, helpers) = source_pair("y <- function() 1\n"); + let before = warm(&db, main); + + helpers + .set_source_text_override(&mut db) + .to(Some("library(dplyr)\ny <- function() 1\n".to_string())); + let _ = main.semantic_index(&db); + + assert_eq!(db.executions_for(INDEX, main), before + 1); +} diff --git a/crates/oak_db/src/tests/test_db.rs b/crates/oak_db/src/tests/test_db.rs index 8cd03b9ce..2a3144382 100644 --- a/crates/oak_db/src/tests/test_db.rs +++ b/crates/oak_db/src/tests/test_db.rs @@ -15,6 +15,7 @@ use camino::Utf8Path; use camino::Utf8PathBuf; use oak_package_metadata::namespace::Namespace; use rustc_hash::FxHashMap; +use salsa::plumbing::AsId; use salsa::Setter; use crate::Db; @@ -71,20 +72,32 @@ impl TestDb { } } - /// Count `WillExecute` events whose `database_key`'s Debug form - /// contains `name`. Salsa's `DatabaseKeyIndex::fmt` resolves the - /// underlying function name only when a database is attached to the - /// current thread, so we wrap the scan in `salsa::attach`. + /// Matches query names by substring in the key's `Debug` representation. pub(super) fn executions(&self, name: &str) -> usize { + self.count_executions(|key| format!("{key:?}").contains(name)) + } + + /// Like [`TestDb::executions()`], restricted to `file`. + /// + /// Only valid when [`File`] is the query's sole key, so Salsa uses its ID + /// directly. Queries such as [`File::cross_file_layers()`] intern a tuple + /// of arguments instead, so comparing their key index to a file ID cannot + /// identify that file's executions. + pub(super) fn executions_for(&self, name: &str, file: File) -> usize { + let id = file.as_id(); + self.count_executions(|key| key.key_index() == id && format!("{key:?}").contains(name)) + } + + /// [`salsa::attach()`] lets the key's `Debug` formatter resolve query names + /// using this database. + fn count_executions(&self, matches: impl Fn(salsa::DatabaseKeyIndex) -> bool) -> usize { salsa::attach(self, || { self.events .lock() .unwrap() .iter() .filter(|event| match &event.kind { - salsa::EventKind::WillExecute { database_key } => { - format!("{database_key:?}").contains(name) - }, + salsa::EventKind::WillExecute { database_key } => matches(*database_key), _ => false, }) .count() From 3338fb5729cf3a1d943740233f7cc85ad82c93e9 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 15 Sep 2026 19:05:03 +0200 Subject: [PATCH 2/4] Restrict cycle recovery to foundation queries --- crates/ark/src/lsp/diagnostics.rs | 2 +- crates/ark/src/lsp/find_references.rs | 2 +- crates/ark/src/lsp/goto_definition.rs | 2 +- crates/ark/src/lsp/rename.rs | 2 +- crates/ark/src/lsp/tests/goto_definition.rs | 2 +- crates/ark/src/lsp/tests/rename.rs | 2 +- crates/ark/src/lsp/tests/sources.rs | 2 +- crates/ark/src/lsp/tests/state_handlers.rs | 2 +- crates/oak_db/src/db.rs | 63 +++++----- crates/oak_db/src/directory.rs | 8 +- crates/oak_db/src/file.rs | 37 +++--- crates/oak_db/src/file_exports.rs | 2 +- crates/oak_db/src/file_imports.rs | 95 ++++++++------- crates/oak_db/src/file_revision.rs | 2 +- crates/oak_db/src/imports.rs | 78 ++++++------- crates/oak_db/src/imports/resolver_probe.rs | 37 +++--- crates/oak_db/src/inputs.rs | 8 +- crates/oak_db/src/lib.rs | 1 + crates/oak_db/src/load_context.rs | 19 ++- .../oak_db/src/load_context/contrib/shiny.rs | 30 ++--- .../src/load_context/contrib/testthat.rs | 14 ++- crates/oak_db/src/name.rs | 2 +- crates/oak_db/src/package.rs | 24 ++-- crates/oak_db/src/resolver_db.rs | 109 ++++-------------- crates/oak_db/src/storage.rs | 6 +- crates/oak_db/src/tests/db.rs | 2 +- crates/oak_db/src/tests/file_imports.rs | 5 +- crates/oak_db/src/tests/file_root.rs | 2 +- crates/oak_db/src/tests/inputs.rs | 1 + crates/oak_db/src/tests/resolver_boundary.rs | 22 ++-- ...ary__prohibited_forms_fail_to_compile.snap | 27 +++-- .../oak_db__tests__tidy__salsa_inventory.snap | 47 ++++---- crates/oak_db/src/tests/test_db.rs | 6 +- .../oak_ide/tests/integration/base_sources.rs | 2 +- crates/oak_scan/src/tests/scheduler.rs | 2 +- crates/oak_scan/src/tests/sources.rs | 2 +- crates/oak_scan/src/tests/stale.rs | 2 +- crates/oak_scan/src/tests/watch.rs | 2 +- crates/oak_scan/src/tests/workspace.rs | 2 +- crates/oak_scan/tests/integration/library.rs | 2 +- 40 files changed, 317 insertions(+), 360 deletions(-) diff --git a/crates/ark/src/lsp/diagnostics.rs b/crates/ark/src/lsp/diagnostics.rs index 6a81adb1f..ac749159c 100644 --- a/crates/ark/src/lsp/diagnostics.rs +++ b/crates/ark/src/lsp/diagnostics.rs @@ -1271,8 +1271,8 @@ mod tests { use aether_path::FilePath; use harp::eval::RParseEvalOptions; - use oak_db::Db; use oak_db::OakDatabase; + use oak_db::SourceDb; use oak_scan::DbScan; use tempfile::TempDir; use tower_lsp_server::ls_types as lsp_types; diff --git a/crates/ark/src/lsp/find_references.rs b/crates/ark/src/lsp/find_references.rs index f3b89e6c9..b4874d06f 100644 --- a/crates/ark/src/lsp/find_references.rs +++ b/crates/ark/src/lsp/find_references.rs @@ -1,6 +1,6 @@ use aether_lsp_utils::proto::from_proto; use aether_lsp_utils::proto::to_proto; -use oak_db::Db; +use oak_db::SourceDb; use stdext::result::ResultExt; use tower_lsp_server::ls_types::Location; use tower_lsp_server::ls_types::ReferenceParams; diff --git a/crates/ark/src/lsp/goto_definition.rs b/crates/ark/src/lsp/goto_definition.rs index 7677e2e7a..6bb38c6e6 100644 --- a/crates/ark/src/lsp/goto_definition.rs +++ b/crates/ark/src/lsp/goto_definition.rs @@ -1,7 +1,7 @@ use aether_lsp_utils::proto::from_proto; use aether_lsp_utils::proto::to_proto; use aether_lsp_utils::proto::PositionEncoding; -use oak_db::Db; +use oak_db::SourceDb; use oak_ide::NavigationTarget; use stdext::result::ResultExt; use tower_lsp_server::ls_types::GotoDefinitionParams; diff --git a/crates/ark/src/lsp/rename.rs b/crates/ark/src/lsp/rename.rs index 01f563b59..d4ac11214 100644 --- a/crates/ark/src/lsp/rename.rs +++ b/crates/ark/src/lsp/rename.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use aether_lsp_utils::proto::from_proto; use aether_lsp_utils::proto::to_proto; use anyhow::Context; -use oak_db::Db; +use oak_db::SourceDb; use tower_lsp_server::ls_types as lsp_types; use tower_lsp_server::ls_types::PrepareRenameResponse; use tower_lsp_server::ls_types::RenameParams; diff --git a/crates/ark/src/lsp/tests/goto_definition.rs b/crates/ark/src/lsp/tests/goto_definition.rs index 9e182a996..96b8cab97 100644 --- a/crates/ark/src/lsp/tests/goto_definition.rs +++ b/crates/ark/src/lsp/tests/goto_definition.rs @@ -3,8 +3,8 @@ use std::sync::Arc; use aether_path::FilePath; use assert_matches::assert_matches; -use oak_db::Db; use oak_db::OakDatabase; +use oak_db::SourceDb; use oak_scan::DbScan; use tower_lsp_server::ls_types as lsp_types; use tower_lsp_server::ls_types::GotoDefinitionParams; diff --git a/crates/ark/src/lsp/tests/rename.rs b/crates/ark/src/lsp/tests/rename.rs index 50a9b96dc..2c8715f23 100644 --- a/crates/ark/src/lsp/tests/rename.rs +++ b/crates/ark/src/lsp/tests/rename.rs @@ -1,8 +1,8 @@ use aether_path::FilePath; -use oak_db::Db; use oak_db::DbInputs; use oak_db::Root; use oak_db::RootKind; +use oak_db::SourceDb; use salsa::Setter; use tower_lsp_server::ls_types as lsp_types; use tower_lsp_server::ls_types::PrepareRenameResponse; diff --git a/crates/ark/src/lsp/tests/sources.rs b/crates/ark/src/lsp/tests/sources.rs index 187c8a185..11c93d3fc 100644 --- a/crates/ark/src/lsp/tests/sources.rs +++ b/crates/ark/src/lsp/tests/sources.rs @@ -9,8 +9,8 @@ use std::sync::mpsc::Sender; use std::sync::Arc; use std::sync::Mutex; -use oak_db::Db; use oak_db::OakDatabase; +use oak_db::SourceDb; use oak_scan::DbScan; use serde_json::json; use serde_json::Value; diff --git a/crates/ark/src/lsp/tests/state_handlers.rs b/crates/ark/src/lsp/tests/state_handlers.rs index c2ce3b483..d7467de1b 100644 --- a/crates/ark/src/lsp/tests/state_handlers.rs +++ b/crates/ark/src/lsp/tests/state_handlers.rs @@ -11,8 +11,8 @@ use std::path::PathBuf; use aether_path::AbsPathBuf; use aether_path::FilePath; -use oak_db::Db; use oak_db::DbInputs; +use oak_db::SourceDb; use oak_scan::DbScan; use oak_scan::ScanRequest; use oak_scan::ScanScheduler; diff --git a/crates/oak_db/src/db.rs b/crates/oak_db/src/db.rs index 6e34cc03d..1f6c87e39 100644 --- a/crates/oak_db/src/db.rs +++ b/crates/oak_db/src/db.rs @@ -18,10 +18,8 @@ use crate::WorkspaceRoots; /// ([`crate::OakDatabase`], the test db) supplies singleton input handles /// and file reads. /// -/// Kept separate from [`Db`] (the query trait) so input accessors and derived -/// queries live on different traits. Mirrors rust-analyzer's `SourceDatabase` -/// / `DefDatabase` split: input plumbing on the base trait, derived queries -/// on the query trait. +/// [`SourceDb`] adds lookups over these inputs. [`Db`] additionally permits +/// recursive semantic queries. #[salsa::db] pub trait DbInputs: salsa::Database { /// Workspace folders opened by the editor. @@ -43,18 +41,15 @@ pub trait DbInputs: salsa::Database { fn read_to_string(&self, path: &Utf8Path) -> io::Result; } -/// Salsa database trait used throughout `oak_db`. Tracked queries take `&dyn -/// Db`, so query code never names the concrete db type. +/// Database access for source text, syntax, membership, and package metadata. +/// Queries accepting this trait cannot directly call semantic queries that +/// require [`Db`]. /// -/// Methods aren't memoized at this level: they delegate to free helpers -/// (`file_by_path_query` etc.) that walk per-root indices which *are* memoized, -/// so salsa records dep edges through those. -/// -/// Each concrete db type provides its own forwarding `impl Db`, which is -/// what lets `db.file_by_path(path)` work on both `&dyn Db` (via the trait -/// method) and concrete db references (via the type's impl). +/// Lookup methods delegate to tracked per-root indices, preserving their +/// invalidation boundaries. Recovery helpers also use this trait: keep its +/// implementations and their transitive dependencies free of semantic queries. #[salsa::db] -pub trait Db: DbInputs { +pub trait SourceDb: DbInputs { /// Look up the `File` interned at `path`, if any. /// /// Walks the per-root URL indices in workspace-then-library order, @@ -92,8 +87,12 @@ pub trait Db: DbInputs { fn live_roots(&self) -> &[LiveRoot]; } +/// Unrestricted database access for recursive semantic queries. +#[salsa::db] +pub trait Db: SourceDb {} + #[salsa::tracked(returns(ref))] -pub(crate) fn live_roots_query(db: &dyn Db) -> Vec { +pub(crate) fn live_roots_query(db: &dyn SourceDb) -> Vec { let mut roots: Vec = db .workspace_roots() .roots(db) @@ -154,7 +153,7 @@ pub fn all_used_files(db: &dyn Db) -> Vec { /// wide searches. LSP functionality should generally not depend on /// non-dependencies, prefer [`all_used_files()`] instead. #[salsa::tracked(returns(ref))] -pub fn all_known_files(db: &dyn Db) -> Vec { +pub fn all_known_files(db: &dyn SourceDb) -> Vec { let mut seen = FxHashSet::default(); let mut files = Vec::new(); @@ -180,7 +179,7 @@ pub fn all_known_files(db: &dyn Db) -> Vec { /// already in `seen`. When `dependencies` is `Some`, packages not in that set /// are skipped entirely. fn push_root_files( - db: &dyn Db, + db: &dyn SourceDb, files: &mut Vec, seen: &mut FxHashSet, root: Root, @@ -203,7 +202,7 @@ fn push_root_files( /// package files, plus orphan editor buffers. Library roots are excluded, so /// installed package symbols don't leak into e.g. workspace symbols. #[salsa::tracked(returns(ref))] -pub fn workspace_files(db: &dyn Db) -> Vec { +pub fn workspace_files(db: &dyn SourceDb) -> Vec { let mut files: Vec = Vec::new(); for &root in db.live_roots() { @@ -220,7 +219,7 @@ pub fn workspace_files(db: &dyn Db) -> Vec { /// The scripts held directly by workspace roots, in root order. /// Like [`workspace_files`] but without package files. #[salsa::tracked(returns(ref))] -pub(crate) fn workspace_scripts(db: &dyn Db) -> Vec { +pub(crate) fn workspace_scripts(db: &dyn SourceDb) -> Vec { db.workspace_roots() .roots(db) .iter() @@ -231,7 +230,7 @@ pub(crate) fn workspace_scripts(db: &dyn Db) -> Vec { /// Every file owned by a workspace root, including package files. Orphan /// buffers are excluded because directory loading is rooted on disk. #[salsa::tracked(returns(ref))] -pub(crate) fn workspace_root_files(db: &dyn Db) -> Vec { +pub(crate) fn workspace_root_files(db: &dyn SourceDb) -> Vec { let mut files: Vec = Vec::new(); for &root in db.workspace_roots().roots(db) { collect_root_files(db, &mut files, root); @@ -239,7 +238,7 @@ pub(crate) fn workspace_root_files(db: &dyn Db) -> Vec { files } -fn collect_root_files(db: &dyn Db, files: &mut Vec, r: Root) { +fn collect_root_files(db: &dyn SourceDb, files: &mut Vec, r: Root) { let owned = |f: File| root_by_file(db, f) == Some(r); files.extend(r.scripts(db).iter().copied().filter(|&f| owned(f))); @@ -249,13 +248,13 @@ fn collect_root_files(db: &dyn Db, files: &mut Vec, r: Root) { } } -/// Implementation of [`Db::file_by_path`]. Walks the per-root indices. +/// Implementation of [`SourceDb::file_by_path`]. Walks the per-root indices. /// /// Not itself salsa-tracked (its `&FilePath` argument isn't a salsa /// entity), but every step is: each [`root_path_index`] call returns a /// cached map, so adding a file to one root invalidates only that /// root's index. -pub(crate) fn file_by_path_query(db: &dyn Db, path: &FilePath) -> Option { +pub(crate) fn file_by_path_query(db: &dyn SourceDb, path: &FilePath) -> Option { for &root in db.live_roots() { let hit = match root { LiveRoot::Workspace(r) | LiveRoot::Library(r) => { @@ -270,10 +269,10 @@ pub(crate) fn file_by_path_query(db: &dyn Db, path: &FilePath) -> Option { None } -/// Implementation of [`Db::package_by_name`]. Same shape as +/// Implementation of [`SourceDb::package_by_name`]. Same shape as /// [`file_by_path_query`]; orphan has no packages, so it contributes /// nothing to the walk. -pub(crate) fn package_by_name_query(db: &dyn Db, name: &str) -> Option { +pub(crate) fn package_by_name_query(db: &dyn SourceDb, name: &str) -> Option { for &root in db.live_roots() { if let LiveRoot::Workspace(r) | LiveRoot::Library(r) = root { if let Some(&pkg) = root_package_index(db, r).get(name) { @@ -284,9 +283,9 @@ pub(crate) fn package_by_name_query(db: &dyn Db, name: &str) -> Option None } -/// Implementation of [`Db::root_by_package`]. Walks all live roots looking for +/// Implementation of [`SourceDb::root_by_package`]. Walks all live roots looking for /// `pkg` in their `packages` vec, picking the longest-path root on ties. -pub(crate) fn root_by_package_query(db: &dyn Db, pkg: Package) -> Option { +pub(crate) fn root_by_package_query(db: &dyn SourceDb, pkg: Package) -> Option { let mut best: Option<(Root, usize)> = None; for &root in db.live_roots() { let (LiveRoot::Workspace(r) | LiveRoot::Library(r)) = root else { @@ -315,7 +314,7 @@ pub(crate) fn root_by_package_query(db: &dyn Db, pkg: Package) -> Option { /// /// Returns `None` for orphan files (they live in no workspace or library /// root). [`File::root`] handles that case with a path-prefix fallback. -pub(crate) fn root_by_file(db: &dyn Db, file: File) -> Option { +pub(crate) fn root_by_file(db: &dyn SourceDb, file: File) -> Option { let mut best: Option<(Root, usize)> = None; let path = file.path(db); @@ -343,7 +342,7 @@ pub(crate) fn root_by_file(db: &dyn Db, file: File) -> Option { /// letter), which would silently collapse all depths to zero and degrade /// the tiebreaker into "first found wins". Depth is a structural property /// of the URL hierarchy, so the URL itself is the right source. -fn root_depth(db: &dyn Db, root: Root) -> usize { +fn root_depth(db: &dyn SourceDb, root: Root) -> usize { root.path(db) .to_url() .path_segments() @@ -356,7 +355,7 @@ fn root_depth(db: &dyn Db, root: Root) -> usize { /// `pkg.scripts` reachable from this root. Adding or removing a file /// in *this* root invalidates this entry; other roots stay cached. #[salsa::tracked(returns(ref))] -fn root_path_index(db: &dyn Db, root: Root) -> FxHashMap { +fn root_path_index(db: &dyn SourceDb, root: Root) -> FxHashMap { let mut map = FxHashMap::default(); for &file in root.scripts(db) { map.insert(file.path(db).clone(), file); @@ -374,7 +373,7 @@ fn root_path_index(db: &dyn Db, root: Root) -> FxHashMap { /// Orphan URL -> File index. Reads only `orphan_root().files`. #[salsa::tracked(returns(ref))] -fn orphan_path_index(db: &dyn Db) -> FxHashMap { +fn orphan_path_index(db: &dyn SourceDb) -> FxHashMap { let mut map = FxHashMap::default(); for &file in db.orphan_root().files(db) { map.insert(file.path(db).clone(), file); @@ -385,7 +384,7 @@ fn orphan_path_index(db: &dyn Db) -> FxHashMap { /// Per-root name -> Package index. Same granularity as /// [`root_path_index`]. #[salsa::tracked(returns(ref))] -fn root_package_index(db: &dyn Db, root: Root) -> FxHashMap { +fn root_package_index(db: &dyn SourceDb, root: Root) -> FxHashMap { let mut map = FxHashMap::default(); for &pkg in root.packages(db) { map.insert(pkg.name(db).clone(), pkg); diff --git a/crates/oak_db/src/directory.rs b/crates/oak_db/src/directory.rs index 50e17e3b4..7531df86f 100644 --- a/crates/oak_db/src/directory.rs +++ b/crates/oak_db/src/directory.rs @@ -9,14 +9,14 @@ use camino::Utf8Path; use crate::db::workspace_root_files; -use crate::Db; use crate::File; +use crate::SourceDb; /// Returns workspace files directly under `dir`, in `list.files()` load order. /// /// `sourceDir()`, Shiny's `loadSupport()`, and non-package `R/` collation use /// this order. [`collation_basename_key()`] mirrors their session-locale sort. -pub(crate) fn files_in_directory(db: &dyn Db, dir: &Utf8Path) -> Vec { +pub(crate) fn files_in_directory(db: &dyn SourceDb, dir: &Utf8Path) -> Vec { let mut files: Vec = workspace_root_files(db) .iter() .copied() @@ -31,7 +31,7 @@ pub(crate) fn files_in_directory(db: &dyn Db, dir: &Utf8Path) -> Vec { /// /// `list.files(recursive = TRUE)` sorts nested scripts by relative path. Case /// folding matches [`collation_basename_key()`]. -pub(crate) fn files_in_directory_recursive(db: &dyn Db, dir: &Utf8Path) -> Vec { +pub(crate) fn files_in_directory_recursive(db: &dyn SourceDb, dir: &Utf8Path) -> Vec { let mut keyed: Vec<(String, File)> = workspace_root_files(db) .iter() .copied() @@ -50,7 +50,7 @@ pub(crate) fn files_in_directory_recursive(db: &dyn Db, dir: &Utf8Path) -> Vec Option { +pub(crate) fn collation_basename_key(file: File, db: &dyn SourceDb) -> Option { file.path(db) .file_name() .map(|name| name.to_ascii_lowercase()) diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index 6a7d16f94..948ee01b1 100644 --- a/crates/oak_db/src/file.rs +++ b/crates/oak_db/src/file.rs @@ -17,6 +17,7 @@ use crate::FileRevision; use crate::Name; use crate::Package; use crate::Root; +use crate::SourceDb; /// A source file tracked by Salsa. /// @@ -79,7 +80,7 @@ impl File { /// /// A virtual path or an unreadable file yields empty text (matches ty). #[salsa::tracked(returns(ref), lru = 128)] - pub fn source_text(self, db: &dyn Db) -> String { + pub fn source_text(self, db: &dyn SourceDb) -> String { if let Some(text) = self.source_text_override(db) { return text.clone(); } @@ -122,7 +123,7 @@ impl File { /// memory cleanly. Derived queries (e.g. `semantic_index`) store /// `AstPtr`s rather than tree nodes, so they don't pin an evicted tree. #[salsa::tracked(returns(ref), lru = 128)] - pub(crate) fn parse(self, db: &dyn Db) -> OakParse { + pub(crate) fn parse(self, db: &dyn SourceDb) -> OakParse { OakParse::new(aether_parser::parse( self.source_text(db).as_str(), aether_parser::RParserOptions::default(), @@ -302,7 +303,7 @@ impl File { /// The root containing this file, if any. /// /// Packaged files ask the db which live root holds the package via - /// [`Db::root_by_package`]. That branch covers library files too, which + /// [`SourceDb::root_by_package`]. That branch covers library files too, which /// normally have a package. It also keeps the common case cheap: it /// depends on each root's package list, not its full file set. /// @@ -319,7 +320,7 @@ impl File { /// Callers that need to distinguish workspace from library roots /// inspect `root.kind(db)`. #[salsa::tracked(returns(copy))] - pub fn root(self, db: &dyn Db) -> Option { + pub fn root(self, db: &dyn SourceDb) -> Option { if let Some(pkg) = self.package(db) { return db.root_by_package(pkg); } @@ -332,7 +333,7 @@ impl File { /// every workspace folder. Private helper: the only caller is /// [`File::root`], as the fallback for an orphan file no scan has reached /// yet (path prefix is all we have until a scan lands). -fn root_by_path(db: &dyn Db, path: &FilePath) -> Option { +fn root_by_path(db: &dyn SourceDb, path: &FilePath) -> Option { // Virtual documents (e.g. untitled scheme) don't have roots let path = path.as_path()?; db.workspace_roots() @@ -374,35 +375,33 @@ fn build_semantic_index_inner(file: File, db: &dyn Db) -> SemanticIndex { fn attached_packages_cycle_result<'db>( db: &'db dyn Db, - id: salsa::Id, + _id: salsa::Id, file: File, ) -> Vec> { record(db, Recovery::AttachedPackages(file)); - attached_packages_fallback(db, id, file) + Vec::new() } fn attached_packages_anywhere_cycle_result<'db>( db: &'db dyn Db, - id: salsa::Id, + _id: salsa::Id, file: File, ) -> Vec> { record(db, Recovery::AttachedPackagesAnywhere(file)); - attached_packages_fallback(db, id, file) -} - -/// Return no attaches. [`File::semantic_index`] recovery rebuilds with -/// `NoopImportsResolver`, which emits [`SemanticDiagnostic::SourceCycle`] and -/// also reports no attaches. -fn attached_packages_fallback<'db>(db: &'db dyn Db, _id: salsa::Id, file: File) -> Vec> { - log::warn!( - "Cyclic attaches detected at {}. Reporting no attached packages.", - file.path(db), - ); Vec::new() } fn semantic_index_cycle_result(db: &dyn Db, _id: salsa::Id, file: File) -> SemanticIndex { record(db, Recovery::SemanticIndex(file)); + semantic_index_fallback(db, file) +} + +fn semantic_index_fallback(db: &dyn SourceDb, file: File) -> SemanticIndex { + #[cfg(resolver_boundary = "probe")] + let _ = file.semantic_index(db); + #[cfg(resolver_boundary = "probe")] + let _: &dyn Db = db; + log::warn!( "Cyclic `source()` detected at {}. Rebuilding without cross-file resolution.", file.path(db), diff --git a/crates/oak_db/src/file_exports.rs b/crates/oak_db/src/file_exports.rs index 6375dc6a7..2674f9c8d 100644 --- a/crates/oak_db/src/file_exports.rs +++ b/crates/oak_db/src/file_exports.rs @@ -58,7 +58,7 @@ impl File { /// /// Delegates the walk to [`SemanticIndex::exports`], then translates /// each `DefinitionKind::Import { file, name }` into - /// `ExportEntry::Import { file, name }` via [`Db::file_by_path`]. If + /// `ExportEntry::Import { file, name }` via [`crate::SourceDb::file_by_path`]. If /// the target file isn't interned yet, that Import is dropped /// silently. Expected, since [`SalsaImportsResolver`] only injects /// Imports when `file_by_path()` resolves the target. diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 4de6e3f8e..bae372e1e 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -18,10 +18,10 @@ use crate::load_context::LoadKind; use crate::load_context::SearchPathTail; use crate::recovery::record; use crate::recovery::Recovery; -use crate::resolver_db::FoundationDb; use crate::Db; use crate::File; use crate::Package; +use crate::SourceDb; /// A layer in a file's import chain. /// @@ -81,7 +81,7 @@ impl CrossFileLayers { /// layers (which outrank them) and the rest of the search path. pub(crate) fn lookup_order<'a>( &'a self, - db: FoundationDb<'_>, + db: &dyn SourceDb, own: &'a [ImportLayer], ) -> impl Iterator + 'a { self.enclosing @@ -94,7 +94,7 @@ impl CrossFileLayers { } impl SearchPathTail { - fn layers(self, db: FoundationDb<'_>) -> Vec { + fn layers(self, db: &dyn SourceDb) -> Vec { match self { SearchPathTail::Base => base_layer(db).into_iter().collect(), SearchPathTail::Default => default_search_path_layers(db), @@ -316,7 +316,7 @@ impl File { fn imports_in(self, db: &dyn Db, view: ImportView<'_>) -> Vec { let layers = self.resolution_layers(db, view.collation); let own = self.attach_layers(db, view.attaches); - layers.lookup_order(FoundationDb::new(db), &own).collect() + layers.lookup_order(db, &own).collect() } /// The file's own layers and the layers it inherits from the files that @@ -383,7 +383,7 @@ impl File { let own = self.attach_layers(db, view.attaches); self.layers_by_sourcing_file(db, view.collation) .into_iter() - .map(|layers| layers.lookup_order(FoundationDb::new(db), &own).collect()) + .map(|layers| layers.lookup_order(db, &own).collect()) .collect() } @@ -400,14 +400,7 @@ impl File { let own = self.attach_layers(db, AttachView::Anywhere); self.inherited_layers(db, CollationView::Deferred) .iter() - .map(|site| { - ( - site.file, - site.layers - .lookup_order(FoundationDb::new(db), &own) - .collect(), - ) - }) + .map(|site| (site.file, site.layers.lookup_order(db, &own).collect())) .collect() } @@ -451,7 +444,7 @@ impl File { pub(crate) fn standalone_imports(self, db: &dyn Db) -> Vec { let own = self.attach_layers(db, AttachView::Anywhere); self.cross_file_layers(db, CollationView::Deferred) - .lookup_order(FoundationDb::new(db), &own) + .lookup_order(db, &own) .collect() } @@ -493,11 +486,14 @@ impl File { /// `semantic_index()`. #[salsa::tracked(returns(ref), cycle_result = cross_file_layers_cycle_result)] pub(crate) fn cross_file_layers(self, db: &dyn Db, view: CollationView) -> CrossFileLayers { - lower_load_context( - db, - load_context(db, self, view), - PredecessorAttaches::Include, - ) + let context = load_context(db, self, view); + let mut layers = lower_load_context(db, &context); + // Deferred successors should outrank this file's own attaches. Keeping + // them below only loses names shadowed by a package a successor reattaches. + let mut attaches = predecessor_attach_layers(db, &context.visible_files); + attaches.append(&mut layers.attaches); + layers.attaches = attaches; + layers } /// The collation members of `self`'s own `R/` directory, in load order. @@ -507,7 +503,7 @@ impl File { /// `cross_file_layers` while `self`'s own semantic index is still being /// built. The query can't recurse into the index. #[salsa::tracked(returns(ref))] - pub(crate) fn collation_siblings(self, db: &dyn Db) -> Vec { + pub(crate) fn collation_siblings(self, db: &dyn SourceDb) -> Vec { let Some(dir) = self.path(db).as_path().and_then(Utf8Path::parent) else { return Vec::new(); }; @@ -525,7 +521,20 @@ fn cross_file_layers_cycle_result( view: CollationView, ) -> CrossFileLayers { record(db, Recovery::CrossFileLayers(file, view)); - lower_load_context(db, load_context(db, file, view), PredecessorAttaches::Skip) + cross_file_layers_fallback(db, file, view) +} + +fn cross_file_layers_fallback( + db: &dyn SourceDb, + file: File, + view: CollationView, +) -> CrossFileLayers { + #[cfg(resolver_boundary = "probe")] + let _ = file.attached_packages(db); + + let mut layers = lower_load_context(db, &load_context(db, file, view)); + layers.recovered_source_cycle = true; + layers } /// Return no inherited layers when this query is Salsa's repeated key in a @@ -681,19 +690,14 @@ fn loaded_before(db: &dyn Db, source_file: File, file: File, offsets: &[TextSize loaded } -pub(crate) enum PredecessorAttaches { - Include, - Skip, -} - /// Lowers a context while preserving resolver precedence. Visible definitions /// and NAMESPACE imports rank above the file's attaches, and loader-provided -/// search-path layers rank below them. -pub(crate) fn lower_load_context( - db: &dyn Db, - context: LoadContext, - predecessors: PredecessorAttaches, -) -> CrossFileLayers { +/// search-path layers rank below them. Predecessor attaches are added only by +/// the normal query, because reading them can re-enter semantic analysis. +pub(crate) fn lower_load_context(db: &dyn SourceDb, context: &LoadContext) -> CrossFileLayers { + #[cfg(resolver_boundary = "probe")] + let _ = predecessor_attach_layers(db, &context.visible_files); + let LoadContext { kind, visible_files, @@ -708,29 +712,20 @@ pub(crate) fn lower_load_context( .collect(); if let LoadKind::Namespace(package) = kind { let namespace = package.namespace(db); - extend_with_namespace_imports(package, namespace, &mut enclosing); + extend_with_namespace_imports(*package, namespace, &mut enclosing); extend_with_namespace_package_imports(db, namespace, &mut enclosing); } - // `Deferred` includes successor attaches that run later and should outrank this - // file's own. Ranking them below loses only names shadowed by a package a - // successor reattaches. - let recovered_source_cycle = matches!(&predecessors, PredecessorAttaches::Skip); - let mut attaches = match predecessors { - PredecessorAttaches::Include => predecessor_attach_layers(db, &visible_files), - PredecessorAttaches::Skip => Vec::new(), - }; - attaches.extend( - implicit_attaches - .iter() - .filter_map(|name| db.package_by_name(name).map(ImportLayer::Package)), - ); + let attaches = implicit_attaches + .iter() + .filter_map(|name| db.package_by_name(name).map(ImportLayer::Package)) + .collect(); CrossFileLayers { enclosing, attaches, tail: kind.search_path_tail(), - recovered_source_cycle, + recovered_source_cycle: false, } } @@ -775,7 +770,7 @@ fn extend_with_namespace_imports( /// Push one `Package` layer per `import(pkg)` directive in the namespace /// (bulk package imports). Missing packages are silently dropped. fn extend_with_namespace_package_imports( - db: &dyn Db, + db: &dyn SourceDb, namespace: &Namespace, layers: &mut Vec, ) { @@ -788,13 +783,13 @@ fn extend_with_namespace_package_imports( /// `base`, always the last thing R searches. `None` when it isn't scanned into /// any root (the R system library is normally on `.libPaths()`, so it is). -fn base_layer(db: FoundationDb<'_>) -> Option { +fn base_layer(db: &dyn SourceDb) -> Option { db.package_by_name("base").map(ImportLayer::Package) } /// The default startup search path as `Package` layers, `stats` first through /// `base` last. Packages absent from every root drop out. -fn default_search_path_layers(db: FoundationDb<'_>) -> Vec { +fn default_search_path_layers(db: &dyn SourceDb) -> Vec { crate::search::DEFAULT_SEARCH_PATH_PACKAGES .iter() .filter_map(|name| db.package_by_name(name).map(ImportLayer::Package)) diff --git a/crates/oak_db/src/file_revision.rs b/crates/oak_db/src/file_revision.rs index 56c63e2c7..b5374dabf 100644 --- a/crates/oak_db/src/file_revision.rs +++ b/crates/oak_db/src/file_revision.rs @@ -47,7 +47,7 @@ impl From for FileRevision { /// successful read that returns the same bytes backdates, so nothing downstream /// re-runs, and a deleted or evicted file leaves the live graph and stops being /// queried at all. -pub(crate) fn report_untracked_if_zero(db: &dyn crate::Db, revision: FileRevision) { +pub(crate) fn report_untracked_if_zero(db: &dyn crate::SourceDb, revision: FileRevision) { if revision == FileRevision::zero() { db.report_untracked_read(); } diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index df9e41fdd..32a0730cc 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -16,12 +16,12 @@ use crate::directory::files_in_directory; use crate::directory::files_in_directory_recursive; use crate::file_imports::CollationView; use crate::file_imports::ImportLayer; -use crate::resolver_db::FoundationDb; use crate::resolver_db::ResolverDb; use crate::Db; use crate::File; use crate::Package; use crate::RootKind; +use crate::SourceDb; mod resolver_probe; @@ -67,6 +67,7 @@ impl<'db> SalsaImportsResolver<'db> { /// What sourcing `file` brings in. The two reads this makes are the ones /// described on [`SalsaImportsResolver`]. fn source_resolution(&self, file: File) -> SourceResolution { + let source_db = self.db.as_source_db(); // Sort to prevent an unrelated export from renumbering `Import` // definitions. `record_binding()` anchors every name at the same // `source()` call, so the original order has no semantic meaning. @@ -79,7 +80,7 @@ impl<'db> SalsaImportsResolver<'db> { names.sort(); SourceResolution { - url: self.db.foundation().file_path(file).to_url(), + url: file.path(source_db).to_url(), names, packages: self.db.attached_package_names(file), } @@ -100,12 +101,12 @@ impl<'db> SalsaImportsResolver<'db> { /// built. #[salsa::tracked(returns(ref))] pub(crate) fn source_dir_scripts( - db: &dyn Db, + db: &dyn SourceDb, file: File, path: String, walk: DirWalk, ) -> Vec { - let Some(anchor) = anchor_dir(FoundationDb::new(db), file) else { + let Some(anchor) = anchor_dir(db, file) else { return Vec::new(); }; let Some(target_path) = resolve_relative_to(&anchor, &path) else { @@ -144,7 +145,7 @@ impl EffectsCache { impl<'db> ImportsResolver for SalsaImportsResolver<'db> { fn resolve_source(&mut self, path: &str) -> Option { - let anchor = anchor_dir(self.db.foundation(), self.file)?; + let anchor = anchor_dir(self.db.as_source_db(), self.file)?; let target_path = resolve_relative_to(&anchor, path)?; // TODO: a `source()` target outside every workspace root never becomes // a `File`, so `file_by_path()` misses it and the names it injects stay @@ -158,14 +159,12 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { // edit), plus GC to drop the orphan once the directive goes away. // TODO(diagnostics): Until we support out-of-workspace sourced files, // should we at least lint so user knows that we can't analyse the file? - let file = self.db.foundation().file_by_path(&target_path)?; + let file = self.db.as_source_db().file_by_path(&target_path)?; Some(self.source_resolution(file)) } fn resolve_source_dir(&mut self, path: &str, walk: DirWalk) -> Vec { - self.db - .foundation() - .source_dir_scripts(self.file, path.to_string(), walk) + source_dir_scripts(self.db.as_source_db(), self.file, path.to_string(), walk) .iter() .copied() // Exclude sourcing file @@ -184,7 +183,7 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { } fn package_exists(&mut self, package: &str) -> bool { - self.db.foundation().package_by_name(package).is_some() + self.db.as_source_db().package_by_name(package).is_some() } } @@ -224,7 +223,7 @@ impl<'db> SalsaImportsResolver<'db> { /// builder records, this one can't be seen from inside the file. fn resolve_effects_uncached(&self, name: &str, attached: &[String]) -> Option { let layers = self.db.cross_file_layers(self.file, CollationView::Eager); - let foundation = self.db.foundation(); + let source_db = self.db.as_source_db(); // The file's own attaches slot between the definition/namespace band // and the rest of the search path, exactly as in `File::imports`. @@ -234,14 +233,10 @@ impl<'db> SalsaImportsResolver<'db> { let own: Vec = attached .iter() .rev() - .filter_map(|package| { - foundation - .package_by_name(package) - .map(ImportLayer::Package) - }) + .filter_map(|package| source_db.package_by_name(package).map(ImportLayer::Package)) .collect(); - for layer in layers.lookup_order(foundation, &own) { + for layer in layers.lookup_order(source_db, &own) { if let ControlFlow::Break(effect) = layer_effect(self.db, &layer, name) { return effect.copied(); } @@ -282,7 +277,7 @@ fn layer_effect( layer: &ImportLayer, name: &str, ) -> ControlFlow> { - let foundation = db.foundation(); + let source_db = db.as_source_db(); match layer { // A definition shadows any deeper effect. Own-file definitions never // reach here, the builder handles them before calling us. @@ -303,7 +298,7 @@ fn layer_effect( false => ControlFlow::Continue(()), } }, - ImportLayer::Package(package) => match package_binding(foundation, *package, name) { + ImportLayer::Package(package) => match package_binding(source_db, *package, name) { PackageBinding::Effect(effects) => ControlFlow::Break(Some(effects)), PackageBinding::Shadow => ControlFlow::Break(None), PackageBinding::Absent => ControlFlow::Continue(()), @@ -311,20 +306,17 @@ fn layer_effect( // A NAMESPACE `importFrom` binds `name` unconditionally (that's what // the directive asserts), so it always shadows the search path // below. Its effect, if any, comes from the source package. - ImportLayer::From(importer) => { - match foundation.package_imported_from(*importer).get(name) { - Some(source) => { - let effect = - foundation.package_by_name(source).and_then( - |package| match package_binding(foundation, package, name) { - PackageBinding::Effect(effects) => Some(effects), - PackageBinding::Shadow | PackageBinding::Absent => None, - }, - ); - ControlFlow::Break(effect) - }, - None => ControlFlow::Continue(()), - } + ImportLayer::From(importer) => match importer.imported_from(source_db).get(name) { + Some(source) => { + let effect = source_db.package_by_name(source).and_then(|package| { + match package_binding(source_db, package, name) { + PackageBinding::Effect(effects) => Some(effects), + PackageBinding::Shadow | PackageBinding::Absent => None, + } + }); + ControlFlow::Break(effect) + }, + None => ControlFlow::Continue(()), }, } } @@ -333,8 +325,8 @@ fn layer_effect( /// only shadows, or nothing. The re-export chase is one hop through an /// `importFrom`, since a re-exported function's annotation lives under its /// original package, not the re-exporter. -fn package_binding(db: FoundationDb<'_>, package: Package, name: &str) -> PackageBinding { - let package_name = db.package_name(package); +fn package_binding(db: &dyn SourceDb, package: Package, name: &str) -> PackageBinding { + let package_name = package.name(db).as_str(); if let Some(effects) = effects::lookup(package_name, name) { return PackageBinding::Effect(effects); } @@ -349,13 +341,13 @@ fn package_binding(db: FoundationDb<'_>, package: Package, name: &str) -> Packag // export gate `Package::resolve` applies. A name it `importFrom`s // without re-exporting isn't visible to a caller that attaches or // imports this package (R errors "could not find function"). - let namespace = db.package_namespace(package); + let namespace = package.namespace(db); if !namespace.exports.contains_str(name) { return PackageBinding::Absent; } // Exports `name`, so it binds. Chase a re-export for the effect; a plain // own definition (no matching `importFrom`) only shadows. - match db.package_imported_from(package).get(name) { + match package.imported_from(db).get(name) { Some(source) => match effects::lookup(source, name) { Some(effects) => PackageBinding::Effect(effects), None => PackageBinding::Shadow, @@ -370,16 +362,16 @@ fn package_binding(db: FoundationDb<'_>, package: Package, name: &str) -> Packag /// resolves `source("foo.R")` against `getwd()`, and IDEs (RStudio, Positron) /// `setwd()` to the project root, so workspace-root anchoring typically matches /// the runtime behaviour. -fn anchor_dir(db: FoundationDb<'_>, file: File) -> Option { - if let Some(root) = db - .file_root(file) - .filter(|root| db.root_kind(*root) == RootKind::Workspace) +fn anchor_dir(db: &dyn SourceDb, file: File) -> Option { + if let Some(root) = file + .root(db) + .filter(|root| root.kind(db) == RootKind::Workspace) { // Workspace roots are file URLs by construction. - return db.root_path(root).as_path().map(Utf8Path::to_path_buf); + return root.path(db).as_path().map(Utf8Path::to_path_buf); } - let parent = db.file_path(file).as_path()?.parent()?; + let parent = file.path(db).as_path()?.parent()?; Some(parent.to_path_buf()) } diff --git a/crates/oak_db/src/imports/resolver_probe.rs b/crates/oak_db/src/imports/resolver_probe.rs index 23357ecc5..0a78d9588 100644 --- a/crates/oak_db/src/imports/resolver_probe.rs +++ b/crates/oak_db/src/imports/resolver_probe.rs @@ -7,6 +7,9 @@ //! that `allowed_calls()` compiles, so unrelated errors //! cannot make a rejected call look like successful enforcement. +#[cfg(resolver_boundary = "control")] +use aether_path::FilePath; + use super::SalsaImportsResolver; #[cfg(any(resolver_boundary = "probe", resolver_boundary = "control"))] use crate::file_imports::CollationView; @@ -36,35 +39,29 @@ impl<'db> SalsaImportsResolver<'db> { #[cfg(resolver_boundary = "probe")] fn escapes_through_private_field(&self) { - let _: &dyn Db = self.db.foundation; + let _: &dyn Db = self.db.db; } #[cfg(resolver_boundary = "probe")] - fn foundation_calls_exports(&self) { - let _ = self.db.foundation().exports(self.file); + fn sources_call_exports(&self) { + let _ = self.file.exports(self.db.as_source_db()); } #[cfg(resolver_boundary = "probe")] - fn foundation_calls_attached_packages(&self) { - let _ = self.db.foundation().attached_package_names(self.file); + fn sources_call_attached_packages(&self) { + let _ = self.file.attached_packages(self.db.as_source_db()); } #[cfg(resolver_boundary = "probe")] - fn foundation_calls_cross_file_layers(&self) { + fn sources_call_cross_file_layers(&self) { let _ = self - .db - .foundation() - .cross_file_layers(self.file, CollationView::Eager); - } - - #[cfg(resolver_boundary = "probe")] - fn foundation_escapes_to_dyn_db(&self) { - let _: &dyn Db = self.db.foundation(); + .file + .cross_file_layers(self.db.as_source_db(), CollationView::Eager); } #[cfg(resolver_boundary = "probe")] - fn foundation_escapes_through_private_field(&self) { - let _: &dyn Db = self.db.foundation().db; + fn sources_escape_to_dyn_db(&self) { + let _: &dyn Db = self.db.as_source_db(); } #[cfg(resolver_boundary = "control")] @@ -72,6 +69,12 @@ impl<'db> SalsaImportsResolver<'db> { let _ = self.db.exports(self.file); let _ = self.db.attached_package_names(self.file); let _ = self.db.cross_file_layers(self.file, CollationView::Eager); - let _ = self.db.foundation().file_path(self.file); + let _ = self.file.path(self.db.as_source_db()); + let _ = self.file.parse(self.db.as_source_db()); + } + + #[cfg(resolver_boundary = "control")] + fn borrowed_path(&self) -> &'db FilePath { + self.file.path(self.db.as_source_db()) } } diff --git a/crates/oak_db/src/inputs.rs b/crates/oak_db/src/inputs.rs index 916e0ca1a..79b6493fa 100644 --- a/crates/oak_db/src/inputs.rs +++ b/crates/oak_db/src/inputs.rs @@ -56,7 +56,7 @@ pub enum RootKind { /// [`StaleRoot`] aren't included -- they have separate access patterns /// (scanner upsert only, never analysis), so they stay as their own input. /// -/// `Db::live_roots()` yields these in lookup precedence (workspace first, then +/// `crate::SourceDb::live_roots()` yields these in lookup precedence (workspace first, then /// library, then orphan). /// /// TODO(salsa): this enum carries the workspace-vs-library distinction in its @@ -121,7 +121,7 @@ impl LibraryRoots { /// /// Singleton: there is one `OrphanRoot` per concrete database, lazily /// initialised by the implementation. The `files` field is what -/// [`crate::Db::file_by_path`] consults to find unanchored files. +/// [`crate::SourceDb::file_by_path`] consults to find unanchored files. #[salsa::input(debug)] pub struct OrphanRoot { /// **Placement invariant.** Files here must have `package(db) == @@ -150,8 +150,8 @@ impl OrphanRoot { /// agent / multi-repo workflows where the same workspace folder gets /// added and removed repeatedly across a session. /// -/// **Not consulted by analysis.** `Db::file_by_path` and -/// `Db::package_by_name` walk workspace / library roots and (for files) +/// **Not consulted by analysis.** `crate::SourceDb::file_by_path` and +/// `crate::SourceDb::package_by_name` walk workspace / library roots and (for files) /// `OrphanRoot` only. Entities in `StaleRoot` are invisible to /// completions, goto-def, etc. — they correspond to folders the user /// has explicitly removed. diff --git a/crates/oak_db/src/lib.rs b/crates/oak_db/src/lib.rs index f551b9990..8928f999b 100644 --- a/crates/oak_db/src/lib.rs +++ b/crates/oak_db/src/lib.rs @@ -32,6 +32,7 @@ pub use db::all_used_files; pub use db::workspace_files; pub use db::Db; pub use db::DbInputs; +pub use db::SourceDb; pub use definition::Definition; pub use diagnostic::Annotation; pub use diagnostic::Diagnostic; diff --git a/crates/oak_db/src/load_context.rs b/crates/oak_db/src/load_context.rs index 3fd240008..4d7268dba 100644 --- a/crates/oak_db/src/load_context.rs +++ b/crates/oak_db/src/load_context.rs @@ -10,9 +10,9 @@ use camino::Utf8Path; use crate::directory::collation_basename_key; use crate::file_imports::CollationView; -use crate::Db; use crate::File; use crate::Package; +use crate::SourceDb; /// Files, namespace imports, and packages supplied by a file's loader. /// @@ -52,7 +52,7 @@ const PACKAGE_LOADER: LoaderInfo = LoaderInfo { /// 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 { +pub(crate) fn loader(db: &dyn SourceDb, file: File) -> Option { load_context(db, file, CollationView::Deferred).loader } @@ -95,7 +95,10 @@ pub(crate) enum SearchPathTail { /// Selects the first matching loader. Classifications overlap, so `testthat` /// precedes package loading and package ownership precedes directory /// conventions. -pub(crate) fn load_context(db: &dyn Db, file: File, view: CollationView) -> LoadContext { +pub(crate) fn load_context(db: &dyn SourceDb, file: File, view: CollationView) -> LoadContext { + #[cfg(resolver_boundary = "probe")] + let _ = file.semantic_index(db); + if let Some(context) = contrib::testthat::load_context(db, file, view) { return context; } @@ -117,7 +120,7 @@ pub(crate) fn load_context(db: &dyn Db, file: File, view: CollationView) -> Load /// Package membership alone does not make a file loadable. `data-raw/`, `inst/`, /// and `R/` files omitted from `Collate:` are `package.scripts()` and remain /// standalone. -fn package_load_context(db: &dyn Db, file: File, view: CollationView) -> Option { +fn package_load_context(db: &dyn SourceDb, file: File, view: CollationView) -> Option { let package = file.package(db)?; let files = package.files(db); @@ -142,7 +145,11 @@ fn standalone_load_context() -> LoadContext { } /// The `R/`-directory collation members visible to `file`, in LIFO order. -pub(crate) fn collation_visible_files(db: &dyn Db, file: File, view: CollationView) -> Vec { +pub(crate) fn collation_visible_files( + db: &dyn SourceDb, + file: File, + view: CollationView, +) -> Vec { let files = file.collation_siblings(db); // Before scanning moves `file` out of `OrphanRoot`, it is absent from this @@ -177,7 +184,7 @@ pub(crate) fn visible_siblings( /// 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 { +pub(crate) fn in_r_directory(file: File, db: &dyn SourceDb) -> bool { let Some(path) = file.path(db).as_path() else { return false; }; diff --git a/crates/oak_db/src/load_context/contrib/shiny.rs b/crates/oak_db/src/load_context/contrib/shiny.rs index bb314cf17..1cfe1370c 100644 --- a/crates/oak_db/src/load_context/contrib/shiny.rs +++ b/crates/oak_db/src/load_context/contrib/shiny.rs @@ -13,8 +13,8 @@ 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; +use crate::SourceDb; const LOADER: LoaderInfo = LoaderInfo { name: "This Shiny app", @@ -25,7 +25,11 @@ const LOADER: LoaderInfo = LoaderInfo { /// `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 /// enclosing app's collation rather than treating it as an app root. -pub(crate) fn load_context(db: &dyn Db, file: File, view: CollationView) -> Option { +pub(crate) fn load_context( + db: &dyn SourceDb, + file: File, + view: CollationView, +) -> Option { if in_r_directory(file, db) { let autoload = shiny_autoload(db, file).as_deref()?; return Some(autoload_context(db, file, view, autoload)); @@ -40,7 +44,7 @@ pub(crate) fn load_context(db: &dyn Db, file: File, view: CollationView) -> Opti /// An `R/` file `loadSupport()` sources: the plain `R/` collation, plus /// whatever ran before the directory did. fn autoload_context( - db: &dyn Db, + db: &dyn SourceDb, file: File, view: CollationView, autoload: &[File], @@ -94,7 +98,7 @@ fn global_context() -> LoadContext { /// tracked [`is_shiny_entry_file()`] lets unchanged entry-point classification /// backdate callers after a source edit. #[salsa::tracked(returns(ref))] -pub(crate) fn shiny_autoload(db: &dyn Db, file: File) -> Option> { +pub(crate) fn shiny_autoload(db: &dyn SourceDb, file: File) -> Option> { // `R/` files get only `global.R` here because collation supplies siblings. // Disabled autoload removes both their Shiny layers and implicit attachment. // @@ -122,7 +126,7 @@ const SHINY_ENTRY_FILES: [(&str, &str); 3] = [ ]; /// Files `loadSupport()` evaluates before app code, in load order. -fn shiny_support_files(db: &dyn Db, app_dir: &Utf8Path) -> Vec { +fn shiny_support_files(db: &dyn SourceDb, app_dir: &Utf8Path) -> Vec { let mut files: Vec = shiny_global_file(db, app_dir).into_iter().collect(); // `_disable_autoload.R` leaves `global.R` in the support set. @@ -136,7 +140,7 @@ fn shiny_support_files(db: &dyn Db, app_dir: &Utf8Path) -> Vec { /// Whether `_disable_autoload.R` prevents `loadSupport()` from loading `R/` /// files. `global.R` remains in the support list. -fn r_autoload_disabled(db: &dyn Db, app_dir: &Utf8Path) -> bool { +fn r_autoload_disabled(db: &dyn SourceDb, app_dir: &Utf8Path) -> bool { files_in_directory(db, &app_dir.join("R")) .iter() .any(|file| is_named(*file, db, "_disable_autoload.R")) @@ -144,7 +148,7 @@ fn r_autoload_disabled(db: &dyn Db, app_dir: &Utf8Path) -> bool { /// Whether `file` is an app's `global.R`, which `loadSupport()` loads even when /// `_disable_autoload.R` is present. -fn is_shiny_global_file(file: File, db: &dyn Db) -> bool { +fn is_shiny_global_file(file: File, db: &dyn SourceDb) -> bool { if !is_named(file, db, "global.R") { return false; } @@ -156,7 +160,7 @@ fn is_shiny_global_file(file: File, db: &dyn Db) -> bool { /// The Shiny app directory whose `R/` holds `file`, if any. `None` unless /// `file` sits directly in an `R/` that an app encloses. -fn enclosing_app_dir(file: File, db: &dyn Db) -> Option<&Utf8Path> { +fn enclosing_app_dir(file: File, db: &dyn SourceDb) -> Option<&Utf8Path> { if !in_r_directory(file, db) { return None; } @@ -164,7 +168,7 @@ fn enclosing_app_dir(file: File, db: &dyn Db) -> Option<&Utf8Path> { is_shiny_dir(db, app_dir).then_some(app_dir) } -fn shiny_app_dir(file: File, db: &dyn Db) -> Option<&Utf8Path> { +fn shiny_app_dir(file: File, db: &dyn SourceDb) -> Option<&Utf8Path> { if !is_shiny_entry_file(db, file) { return None; } @@ -172,20 +176,20 @@ fn shiny_app_dir(file: File, db: &dyn Db) -> Option<&Utf8Path> { } /// Whether `dir` has a Shiny entry point and therefore autoloads its adjacent `R/`. -fn is_shiny_dir(db: &dyn Db, dir: &Utf8Path) -> bool { +fn is_shiny_dir(db: &dyn SourceDb, dir: &Utf8Path) -> bool { files_in_directory(db, dir) .iter() .any(|file| is_shiny_entry_file(db, *file)) } #[salsa::tracked(returns(copy))] -fn is_shiny_entry_file(db: &dyn Db, file: File) -> bool { +pub(crate) fn is_shiny_entry_file(db: &dyn SourceDb, file: File) -> bool { SHINY_ENTRY_FILES .iter() .any(|(name, marker)| is_named(file, db, name) && file.source_text(db).contains(marker)) } -fn shiny_global_file(db: &dyn Db, app_dir: &Utf8Path) -> Option { +fn shiny_global_file(db: &dyn SourceDb, app_dir: &Utf8Path) -> Option { files_in_directory(db, app_dir) .into_iter() .find(|file| is_named(*file, db, "global.R")) @@ -193,7 +197,7 @@ fn shiny_global_file(db: &dyn Db, app_dir: &Utf8Path) -> Option { /// Shiny matches these filenames case-insensitively, unlike the exact `R/` /// convention in [`in_r_directory()`]. -fn is_named(file: File, db: &dyn Db, name: &str) -> bool { +fn is_named(file: File, db: &dyn SourceDb, name: &str) -> bool { file.path(db) .file_name() .is_some_and(|basename| basename.eq_ignore_ascii_case(name)) diff --git a/crates/oak_db/src/load_context/contrib/testthat.rs b/crates/oak_db/src/load_context/contrib/testthat.rs index 8b1ea8e1b..401be7cfc 100644 --- a/crates/oak_db/src/load_context/contrib/testthat.rs +++ b/crates/oak_db/src/load_context/contrib/testthat.rs @@ -7,8 +7,8 @@ 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; +use crate::SourceDb; const LOADER: LoaderInfo = LoaderInfo { name: "testthat", @@ -22,7 +22,11 @@ const LOADER: LoaderInfo = LoaderInfo { /// Support files form their own collation. An `Eager` view keeps only /// source-order predecessors, while a `Deferred` view keeps every support file. /// Every `R/` file remains visible because package loading finishes first. -pub(crate) fn load_context(db: &dyn Db, file: File, view: CollationView) -> Option { +pub(crate) fn load_context( + db: &dyn SourceDb, + file: File, + view: CollationView, +) -> Option { let package = file.package(db)?; if !is_testthat_file(file, db) { return None; @@ -58,7 +62,7 @@ pub(crate) fn load_context(db: &dyn Db, file: File, view: CollationView) -> Opti /// True when `file` sits directly in a `tests/testthat/` directory, the /// layout testthat sources and runs files from. This is what separates a /// test file from an ordinary package script under e.g. `tests/` or `inst/`. -fn is_testthat_file(file: File, db: &dyn Db) -> bool { +fn is_testthat_file(file: File, db: &dyn SourceDb) -> bool { match file.path(db).as_file() { Some(path) => in_testthat_dir(path.as_path()), None => false, @@ -75,7 +79,7 @@ fn in_testthat_dir(path: &Utf8Path) -> bool { /// `testthat` loads `helper*.R` and `setup*.R` before tests, so their bindings /// are visible. Teardown files run afterward and are excluded. -fn is_testthat_support_file(file: File, db: &dyn Db) -> bool { +fn is_testthat_support_file(file: File, db: &dyn SourceDb) -> bool { if !is_testthat_file(file, db) { return false; } @@ -86,6 +90,6 @@ fn is_testthat_support_file(file: File, db: &dyn Db) -> bool { } /// Byte-wise basename sort key keeps support-file precedence platform-stable. -fn testthat_support_key(file: File, db: &dyn Db) -> Cow<'_, str> { +fn testthat_support_key(file: File, db: &dyn SourceDb) -> Cow<'_, str> { file.path(db).file_name().unwrap_or_default() } diff --git a/crates/oak_db/src/name.rs b/crates/oak_db/src/name.rs index 397755f84..f6edeccd7 100644 --- a/crates/oak_db/src/name.rs +++ b/crates/oak_db/src/name.rs @@ -3,7 +3,7 @@ use compact_str::CompactString; /// Interned identifier. /// /// Lets tracked queries cache symbols or packages by name cheaply. -/// Used by [`crate::Db::package_by_name`] and by [`crate::File::resolve`] so +/// Used by [`crate::SourceDb::package_by_name`] and by [`crate::File::resolve`] so /// repeated calls for the same name hit the salsa cache. /// /// The text is stored as `CompactString` so short identifiers stay inline diff --git a/crates/oak_db/src/package.rs b/crates/oak_db/src/package.rs index d1e8d7530..1981619ed 100644 --- a/crates/oak_db/src/package.rs +++ b/crates/oak_db/src/package.rs @@ -9,9 +9,9 @@ use rustc_hash::FxHashMap; use stdext::result::ResultExt; use crate::file_revision::report_untracked_if_zero; -use crate::Db; use crate::File; use crate::FileRevision; +use crate::SourceDb; #[salsa::input(debug)] pub struct Package { @@ -22,7 +22,7 @@ pub struct Package { /// URL distinguishes them. /// /// The package's owning [`Root`] is not stored as a field. It is - /// derived from live-graph containment via [`Db::root_by_package`]: a + /// derived from live-graph containment via [`SourceDb::root_by_package`]: a /// package belongs to whichever `Root.packages` currently holds it. /// Workspace-vs-library is then `root.kind(db)`. #[returns(ref)] @@ -99,7 +99,7 @@ impl Package { /// /// A missing or unparseable `NAMESPACE` yields an empty `Namespace`. #[salsa::tracked(returns(ref))] - pub fn namespace(self, db: &dyn Db) -> Namespace { + pub fn namespace(self, db: &dyn SourceDb) -> Namespace { if let Some(namespace) = self.namespace_override(db) { return namespace.clone(); } @@ -134,11 +134,11 @@ impl Package { /// directly instead of scanning [`Namespace::imports`]. /// /// The value is the source package's name, not a resolved `Package`: - /// resolving through [`Db::package_by_name`] depends on which roots are + /// resolving through [`SourceDb::package_by_name`] depends on which roots are /// live, and baking that in here would invalidate this index on every /// root change. Call sites resolve the name themselves. #[salsa::tracked(returns(ref))] - pub(crate) fn imported_from(self, db: &dyn Db) -> FxHashMap { + pub(crate) fn imported_from(self, db: &dyn SourceDb) -> FxHashMap { self.namespace(db) .imports .iter() @@ -153,7 +153,7 @@ impl Package { /// without changing `Version:` backdates here, so downstream isn't /// disturbed. #[salsa::tracked(returns(ref))] - pub fn version(self, db: &dyn Db) -> Option { + pub fn version(self, db: &dyn SourceDb) -> Option { self.description(db) .as_ref() .map(|description| description.version.clone()) @@ -164,7 +164,7 @@ impl Package { /// /// Build timestamps change on every install, so backdating probably isn't that /// important here, so we don't track this method. - pub fn built(self, db: &dyn Db) -> Option { + pub fn built(self, db: &dyn SourceDb) -> Option { self.description(db) .as_ref() .and_then(|description| description.built.clone()) @@ -174,7 +174,7 @@ impl Package { /// is missing or has no `Priority:` field (only base and recommended packages have /// one) #[salsa::tracked(returns(ref))] - pub fn priority(self, db: &dyn Db) -> Option { + pub fn priority(self, db: &dyn SourceDb) -> Option { self.description(db) .as_ref() .and_then(|description| description.priority.clone()) @@ -184,7 +184,7 @@ impl Package { /// is missing. Narrow query over [`Package::description`], same backdating story as /// [`Package::version`]. #[salsa::tracked(returns(ref))] - pub fn depends(self, db: &dyn Db) -> Option> { + pub fn depends(self, db: &dyn SourceDb) -> Option> { self.description(db) .as_ref() .map(|description| description.depends.clone()) @@ -195,7 +195,7 @@ impl Package { /// over [`Package::description`], same backdating story as /// [`Package::version`]. #[salsa::tracked(returns(ref))] - pub fn collation(self, db: &dyn Db) -> Option> { + pub fn collation(self, db: &dyn SourceDb) -> Option> { self.description(db) .as_ref() .and_then(|description| description.collate()) @@ -214,7 +214,7 @@ impl Package { /// the other direction: a real `DESCRIPTION` edit that doesn't touch /// `Version:` / `Collate:` re-runs this query but backdates there. #[salsa::tracked(returns(ref))] - pub(crate) fn description(self, db: &dyn Db) -> Option { + pub(crate) fn description(self, db: &dyn SourceDb) -> Option { // Depend on `description_revision()` so a bump forces a re-read report_untracked_if_zero(db, self.description_revision(db)); @@ -239,7 +239,7 @@ impl Package { /// Returns an empty `Index` for installed packages with missing or unreadable /// indexes. #[salsa::tracked(returns(ref))] - pub fn index(self, db: &dyn Db) -> Option { + pub fn index(self, db: &dyn SourceDb) -> Option { // Depend on `index_revision()` so a bump forces a re-read. // Workspace packages don't have an `INDEX`. let revision = self.index_revision(db)?; diff --git a/crates/oak_db/src/resolver_db.rs b/crates/oak_db/src/resolver_db.rs index cd8e6081b..ff164843c 100644 --- a/crates/oak_db/src/resolver_db.rs +++ b/crates/oak_db/src/resolver_db.rs @@ -1,117 +1,52 @@ -//! Restricts database access during source and effect resolution. +//! Restricts the recursive queries available during resolution. //! -//! [`FoundationDb`] exposes operations that do not re-enter semantic analysis. -//! [`ResolverDb`] contains that wrapper and adds three recursive queries with -//! `cycle_result` handlers. Resolution can borrow the narrower capabilities -//! through [`ResolverDb::foundation()`] without exposing the underlying database. +//! Source operations use [`SourceDb`] directly. This wrapper exposes only the +//! three recursive queries needed by [`SalsaImportsResolver`], each backed by +//! a `cycle_result` handler. Their bodies still require the full [`Db`]. //! -//! Both wrappers keep database access private to this module. Their forwarding -//! methods are trusted code, so changes require a transitive dependency review. -//! Recovery helpers must accept only [`FoundationDb`], not [`ResolverDb`]. - -use aether_path::FilePath; -use oak_package_metadata::namespace::Namespace; -use oak_semantic::effects::DirWalk; -use rustc_hash::FxHashMap; +//! The raw database is private, so callers cannot bypass these forwarding +//! methods. Recovery helpers receive only `&dyn SourceDb`. +//! +//! Forwarding bodies retain unrestricted access; review their transitive +//! dependencies when changing them. +//! +//! [`SalsaImportsResolver`]: crate::imports::SalsaImportsResolver use crate::file_imports::CollationView; use crate::file_imports::CrossFileLayers; -use crate::imports::source_dir_scripts; use crate::Db; use crate::File; use crate::FileExports; -use crate::Package; -use crate::Root; -use crate::RootKind; +use crate::SourceDb; #[derive(Clone, Copy)] pub(crate) struct ResolverDb<'db> { - foundation: FoundationDb<'db>, + db: &'db dyn Db, } impl<'db> ResolverDb<'db> { pub(crate) fn new(db: &'db dyn Db) -> Self { - Self { - foundation: FoundationDb::new(db), - } + Self { db } } - pub(crate) fn foundation(self) -> FoundationDb<'db> { - self.foundation + pub(crate) fn as_source_db(self) -> &'db dyn SourceDb { + self.db } - // These queries can re-enter semantic analysis and have recovery handlers. - pub(crate) fn exports(self, file: File) -> &'db FileExports { - file.exports(self.foundation.db) + file.exports(self.db) } - /// Package names from `library()` calls at `file`'s own top level. - /// - /// Returning text keeps interned [`Name`](crate::Name) handles and their - /// database access inside this wrapper. + /// Includes only top-level `library()` calls. Returning text keeps interned + /// [`Name`](crate::Name) handles and their database access inside this wrapper. pub(crate) fn attached_package_names(self, file: File) -> Vec { - file.attached_packages(self.foundation.db) + file.attached_packages(self.db) .iter() - .map(|name| name.text(self.foundation.db).to_string()) + .map(|name| name.text(self.db).to_string()) .collect() } pub(crate) fn cross_file_layers(self, file: File, view: CollationView) -> &'db CrossFileLayers { - file.cross_file_layers(self.foundation.db, view) - } -} - -/// Inputs, per-root indices, and package metadata available without recursive -/// semantic queries. The raw database has no accessor, so callers cannot widen -/// this wrapper back to [`ResolverDb`] or `&dyn Db`. -#[derive(Clone, Copy)] -pub(crate) struct FoundationDb<'db> { - db: &'db dyn Db, -} - -impl<'db> FoundationDb<'db> { - pub(crate) fn new(db: &'db dyn Db) -> Self { - Self { db } - } - - pub(crate) fn file_by_path(self, path: &FilePath) -> Option { - Db::file_by_path(self.db, path) - } - - pub(crate) fn package_by_name(self, name: &str) -> Option { - Db::package_by_name(self.db, name) - } - - pub(crate) fn file_path(self, file: File) -> &'db FilePath { - file.path(self.db) - } - - pub(crate) fn file_root(self, file: File) -> Option { - file.root(self.db) - } - - pub(crate) fn root_kind(self, root: Root) -> RootKind { - root.kind(self.db) - } - - pub(crate) fn root_path(self, root: Root) -> &'db FilePath { - root.path(self.db) - } - - pub(crate) fn source_dir_scripts(self, file: File, path: String, walk: DirWalk) -> &'db [File] { - source_dir_scripts(self.db, file, path, walk) - } - - pub(crate) fn package_name(self, package: Package) -> &'db str { - package.name(self.db).as_str() - } - - pub(crate) fn package_namespace(self, package: Package) -> &'db Namespace { - package.namespace(self.db) - } - - pub(crate) fn package_imported_from(self, package: Package) -> &'db FxHashMap { - package.imported_from(self.db) + file.cross_file_layers(self.db, view) } } diff --git a/crates/oak_db/src/storage.rs b/crates/oak_db/src/storage.rs index 90d1b907d..a630a009f 100644 --- a/crates/oak_db/src/storage.rs +++ b/crates/oak_db/src/storage.rs @@ -10,6 +10,7 @@ use crate::Db; use crate::DbInputs; use crate::LibraryRoots; use crate::OrphanRoot; +use crate::SourceDb; use crate::StaleRoot; use crate::WorkspaceRoots; @@ -117,7 +118,7 @@ impl DbInputs for OakDatabase { } #[salsa::db] -impl Db for OakDatabase { +impl SourceDb for OakDatabase { fn file_by_path(&self, path: &aether_path::FilePath) -> Option { crate::db::file_by_path_query(self, path) } @@ -134,3 +135,6 @@ impl Db for OakDatabase { crate::db::live_roots_query(self) } } + +#[salsa::db] +impl Db for OakDatabase {} diff --git a/crates/oak_db/src/tests/db.rs b/crates/oak_db/src/tests/db.rs index d8f15654f..40bcb3aeb 100644 --- a/crates/oak_db/src/tests/db.rs +++ b/crates/oak_db/src/tests/db.rs @@ -8,11 +8,11 @@ use crate::tests::test_db::file_path; use crate::tests::test_db::library_root; use crate::tests::test_db::workspace_root; use crate::tests::test_db::TestDb; -use crate::Db; use crate::DbInputs; use crate::File; use crate::FileRevision; use crate::Package; +use crate::SourceDb; #[test] fn test_file_by_path_finds_workspace_script() { diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index bd88cdecb..dcb78ce9c 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -4,7 +4,6 @@ use oak_package_metadata::namespace::Namespace; use salsa::Setter; use crate::file_imports::CollationView; -use crate::resolver_db::FoundationDb; use crate::tests::test_db::file_path; use crate::tests::test_db::library_root; use crate::tests::test_db::make_package; @@ -873,10 +872,10 @@ fn test_cross_file_layers_never_carries_inherited_layers() { for view in [CollationView::Eager, CollationView::Deferred] { let scan_side = helpers.cross_file_layers(&db, view); assert!(scan_side - .lookup_order(FoundationDb::new(&db), &[]) + .lookup_order(&db, &[]) .any(|layer| matches!(layer, ImportLayer::File(file) if file == sibling))); assert!(!scan_side - .lookup_order(FoundationDb::new(&db), &[]) + .lookup_order(&db, &[]) .any(|layer| matches!(layer, ImportLayer::SourcingFile { .. }))); } } diff --git a/crates/oak_db/src/tests/file_root.rs b/crates/oak_db/src/tests/file_root.rs index 6bba6faf7..c5b44d59f 100644 --- a/crates/oak_db/src/tests/file_root.rs +++ b/crates/oak_db/src/tests/file_root.rs @@ -81,7 +81,7 @@ fn test_root_dispatches_through_library_package_when_set() { 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- + // through `SourceDb::root_by_package` rather than falling back to the URL- // prefix walk against workspace roots. let file = File::new( &db, diff --git a/crates/oak_db/src/tests/inputs.rs b/crates/oak_db/src/tests/inputs.rs index 1e3fefa13..7d11b7178 100644 --- a/crates/oak_db/src/tests/inputs.rs +++ b/crates/oak_db/src/tests/inputs.rs @@ -11,6 +11,7 @@ use crate::FileRevision; use crate::OakDatabase; use crate::Package; use crate::Root; +use crate::SourceDb; fn make_workspace_package(db: &mut OakDatabase, name: &str) -> (Root, Package) { let root = workspace_root(db, &format!("workspace/{name}")); diff --git a/crates/oak_db/src/tests/resolver_boundary.rs b/crates/oak_db/src/tests/resolver_boundary.rs index 1c3588197..439be22e7 100644 --- a/crates/oak_db/src/tests/resolver_boundary.rs +++ b/crates/oak_db/src/tests/resolver_boundary.rs @@ -1,14 +1,19 @@ -//! Checks that the resolver's allowed calls compile and its prohibited calls -//! fail at the expected locations in [`crate::imports::resolver_probe`]. +//! Checks resolver and recovery restrictions through the actual database +//! parameters. The control build verifies that allowed operations compile. use std::env; use std::process::Command; use std::process::Output; /// Cargo reports diagnostic paths relative to the workspace root. -const PROBE_FILE: &str = "crates/oak_db/src/imports/resolver_probe.rs"; +const PROBE_FILES: &[&str] = &[ + "crates/oak_db/src/imports/resolver_probe.rs", + "crates/oak_db/src/file.rs", + "crates/oak_db/src/file_imports.rs", + "crates/oak_db/src/load_context.rs", +]; -const PROHIBITED_FORM_COUNT: usize = 10; +const PROHIBITED_FORM_COUNT: usize = 14; /// Passing the cfg through `cargo rustc` leaves dependencies cached. /// Setting `RUSTFLAGS` would rebuild them too. `CARGO_TERM_COLOR` disables @@ -49,16 +54,15 @@ fn test_prohibited_forms_fail_to_compile() { // Only probe locations belong in the snapshot, not Cargo's error summary. let mut lines: Vec<&str> = stderr .lines() - .filter(|line| line.starts_with(PROBE_FILE)) + .filter(|line| PROBE_FILES.iter().any(|path| line.starts_with(path))) .collect(); lines.sort_unstable(); assert_eq!(lines.len(), PROHIBITED_FORM_COUNT); insta::with_settings!({description => "\ - Each line is one prohibited form rejected at the resolver or foundation boundary in \ - crates/oak_db/src/imports/resolver_probe.rs. Line numbers shift when that \ - module is edited. A NEW or MISSING line means the boundary itself changed and \ - needs review, not a blind `cargo insta accept`."}, { + Each line is one prohibited form rejected in the resolver probes or an actual \ + recovery helper. Line numbers shift when those files are edited. A NEW or MISSING \ + line means the boundary itself changed and needs review, not a blind `cargo insta accept`."}, { insta::assert_snapshot!(lines.join("\n")); }); } diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap index 250cd646c..64f666864 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap @@ -1,15 +1,20 @@ --- source: crates/oak_db/src/tests/resolver_boundary.rs -description: "Each line is one prohibited form rejected at the resolver or foundation boundary in crates/oak_db/src/imports/resolver_probe.rs. Line numbers shift when that module is edited. A NEW or MISSING line means the boundary itself changed and needs review, not a blind `cargo insta accept`." +assertion_line: 66 +description: "Each line is one prohibited form rejected in the resolver probes or an actual recovery helper. Line numbers shift when those files are edited. A NEW or MISSING line means the boundary itself changed and needs review, not a blind `cargo insta accept`." expression: "lines.join(\"\\n\")" --- -crates/oak_db/src/imports/resolver_probe.rs:19:42: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:24:39: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:29:35: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:34:26: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:39:34: error[E0616]: field `foundation` of struct `ResolverDb` is private: private field -crates/oak_db/src/imports/resolver_probe.rs:44:38: error[E0599]: no method named `exports` found for struct `FoundationDb<'db>` in the current scope: method not found in `FoundationDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:49:38: error[E0599]: no method named `attached_package_names` found for struct `FoundationDb<'db>` in the current scope -crates/oak_db/src/imports/resolver_probe.rs:57:14: error[E0599]: no method named `cross_file_layers` found for struct `FoundationDb<'db>` in the current scope: method not found in `FoundationDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:62:26: error[E0308]: mismatched types: expected `&dyn Db`, found `FoundationDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:67:47: error[E0616]: field `db` of struct `FoundationDb` is private: private field +crates/oak_db/src/file.rs:401:33: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` +crates/oak_db/src/file.rs:403:22: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` +crates/oak_db/src/file_imports.rs:531:36: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` +crates/oak_db/src/file_imports.rs:700:39: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` +crates/oak_db/src/imports/resolver_probe.rs:22:42: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:27:39: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:32:35: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:37:26: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` +crates/oak_db/src/imports/resolver_probe.rs:42:34: error[E0616]: field `db` of struct `ResolverDb` is private: private field +crates/oak_db/src/imports/resolver_probe.rs:47:35: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` +crates/oak_db/src/imports/resolver_probe.rs:52:45: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` +crates/oak_db/src/imports/resolver_probe.rs:59:32: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` +crates/oak_db/src/imports/resolver_probe.rs:64:26: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` +crates/oak_db/src/load_context.rs:100:33: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__tidy__salsa_inventory.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__tidy__salsa_inventory.snap index a67dca3af..598c7af10 100644 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__tidy__salsa_inventory.snap +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__tidy__salsa_inventory.snap @@ -1,5 +1,6 @@ --- source: crates/oak_db/src/tests/tidy.rs +assertion_line: 8 description: "Updating this snapshot means the Salsa query surface changed. Before accepting it, review the changed query by following `doc/oak/salsa.md`." expression: "oak_tidy::salsa_inventory(&source_dir)" --- @@ -45,28 +46,28 @@ definition.rs: struct Definition<'db> [debug] kind: DefinitionKind [tracked, no_eq] == tracked queries == -db.rs: fn all_known_files(db: &dyn Db) -> Vec [returns(ref)] +db.rs: fn all_known_files(db: &dyn SourceDb) -> Vec [returns(ref)] db.rs: fn all_used_files(db: &dyn Db) -> Vec [returns(ref)] -db.rs: fn live_roots_query(db: &dyn Db) -> Vec [returns(ref)] -db.rs: fn orphan_path_index(db: &dyn Db) -> FxHashMap [returns(ref)] -db.rs: fn root_package_index(db: &dyn Db, root: Root) -> FxHashMap [returns(ref)] -db.rs: fn root_path_index(db: &dyn Db, root: Root) -> FxHashMap [returns(ref)] -db.rs: fn workspace_files(db: &dyn Db) -> Vec [returns(ref)] -db.rs: fn workspace_root_files(db: &dyn Db) -> Vec [returns(ref)] -db.rs: fn workspace_scripts(db: &dyn Db) -> Vec [returns(ref)] +db.rs: fn live_roots_query(db: &dyn SourceDb) -> Vec [returns(ref)] +db.rs: fn orphan_path_index(db: &dyn SourceDb) -> FxHashMap [returns(ref)] +db.rs: fn root_package_index(db: &dyn SourceDb, root: Root) -> FxHashMap [returns(ref)] +db.rs: fn root_path_index(db: &dyn SourceDb, root: Root) -> FxHashMap [returns(ref)] +db.rs: fn workspace_files(db: &dyn SourceDb) -> Vec [returns(ref)] +db.rs: fn workspace_root_files(db: &dyn SourceDb) -> Vec [returns(ref)] +db.rs: fn workspace_scripts(db: &dyn SourceDb) -> Vec [returns(ref)] definition.rs: fn File::definitions(self, db: &'db dyn Db) -> Definitions<'db> [returns(ref)] file.rs: fn File::attached_packages(self, db: &dyn Db) -> Vec> [returns(ref), cycle_result = attached_packages_cycle_result] file.rs: fn File::attached_packages_anywhere(self, db: &dyn Db) -> Vec> [returns(ref), cycle_result = attached_packages_anywhere_cycle_result] file.rs: fn File::diagnostics(self, db: &dyn Db) -> Vec [returns(ref)] file.rs: fn File::line_index(self, db: &dyn Db) -> biome_line_index::LineIndex [returns(ref)] file.rs: fn File::namespace_accessed_packages(self, db: &dyn Db) -> Vec> [returns(ref)] -file.rs: fn File::parse(self, db: &dyn Db) -> OakParse [returns(ref), lru = 128] -file.rs: fn File::root(self, db: &dyn Db) -> Option [returns(copy)] +file.rs: fn File::parse(self, db: &dyn SourceDb) -> OakParse [returns(ref), lru = 128] +file.rs: fn File::root(self, db: &dyn SourceDb) -> Option [returns(copy)] file.rs: fn File::semantic_index(self, db: &dyn Db) -> SemanticIndex [returns(ref), no_eq, cycle_result = semantic_index_cycle_result] -file.rs: fn File::source_text(self, db: &dyn Db) -> String [returns(ref), lru = 128] +file.rs: fn File::source_text(self, db: &dyn SourceDb) -> String [returns(ref), lru = 128] file.rs: fn File::used_packages(self, db: &dyn Db) -> Vec> [returns(ref)] file_exports.rs: fn File::exports(self, db: &dyn Db) -> FileExports [returns(ref), cycle_result = exports_cycle_result] -file_imports.rs: fn File::collation_siblings(self, db: &dyn Db) -> Vec [returns(ref)] +file_imports.rs: fn File::collation_siblings(self, db: &dyn SourceDb) -> Vec [returns(ref)] file_imports.rs: fn File::cross_file_layers(self, db: &dyn Db, view: CollationView) -> CrossFileLayers [returns(ref), cycle_result = cross_file_layers_cycle_result] file_imports.rs: fn File::imports(self, db: &dyn Db) -> Vec [returns(ref)] file_imports.rs: fn File::imports_by_sourcing_file(self, db: &dyn Db) -> Vec> [returns(ref)] @@ -77,17 +78,17 @@ file_source_site.rs: fn File::source_sites(self, db: &dyn Db) -> Vec file_source_site.rs: fn File::source_targets(self, db: &dyn Db) -> Vec [returns(ref)] file_source_site.rs: fn File::sourced_by(self, db: &dyn Db) -> Vec [returns(ref)] file_source_site.rs: fn sourcing_files_by_target(db: &dyn Db) -> FxHashMap> [returns(ref)] -imports.rs: fn source_dir_scripts(db: &dyn Db, file: File, path: String, walk: DirWalk) -> Vec [returns(ref)] -load_context/contrib/shiny.rs: fn is_shiny_entry_file(db: &dyn Db, file: File) -> bool [returns(copy)] -load_context/contrib/shiny.rs: fn shiny_autoload(db: &dyn Db, file: File) -> Option> [returns(ref)] -package.rs: fn Package::collation(self, db: &dyn Db) -> Option> [returns(ref)] -package.rs: fn Package::depends(self, db: &dyn Db) -> Option> [returns(ref)] -package.rs: fn Package::description(self, db: &dyn Db) -> Option [returns(ref)] -package.rs: fn Package::imported_from(self, db: &dyn Db) -> FxHashMap [returns(ref)] -package.rs: fn Package::index(self, db: &dyn Db) -> Option [returns(ref)] -package.rs: fn Package::namespace(self, db: &dyn Db) -> Namespace [returns(ref)] -package.rs: fn Package::priority(self, db: &dyn Db) -> Option [returns(ref)] -package.rs: fn Package::version(self, db: &dyn Db) -> Option [returns(ref)] +imports.rs: fn source_dir_scripts(db: &dyn SourceDb, file: File, path: String, walk: DirWalk) -> Vec [returns(ref)] +load_context/contrib/shiny.rs: fn is_shiny_entry_file(db: &dyn SourceDb, file: File) -> bool [returns(copy)] +load_context/contrib/shiny.rs: fn shiny_autoload(db: &dyn SourceDb, file: File) -> Option> [returns(ref)] +package.rs: fn Package::collation(self, db: &dyn SourceDb) -> Option> [returns(ref)] +package.rs: fn Package::depends(self, db: &dyn SourceDb) -> Option> [returns(ref)] +package.rs: fn Package::description(self, db: &dyn SourceDb) -> Option [returns(ref)] +package.rs: fn Package::imported_from(self, db: &dyn SourceDb) -> FxHashMap [returns(ref)] +package.rs: fn Package::index(self, db: &dyn SourceDb) -> Option [returns(ref)] +package.rs: fn Package::namespace(self, db: &dyn SourceDb) -> Namespace [returns(ref)] +package.rs: fn Package::priority(self, db: &dyn SourceDb) -> Option [returns(ref)] +package.rs: fn Package::version(self, db: &dyn SourceDb) -> Option [returns(ref)] package_resolve.rs: fn Package::resolve(self, db: &'db dyn Db, name: Name<'db>, visibility: NamespaceVisibility) -> Vec> [returns(clone), cycle_result = resolve_cycle_result] workspace.rs: fn all_package_dependencies(db: &dyn Db) -> Vec [returns(ref)] workspace.rs: fn all_workspace_file_dependencies(db: &dyn Db) -> Vec [returns(ref)] diff --git a/crates/oak_db/src/tests/test_db.rs b/crates/oak_db/src/tests/test_db.rs index 2a3144382..17c39a136 100644 --- a/crates/oak_db/src/tests/test_db.rs +++ b/crates/oak_db/src/tests/test_db.rs @@ -27,6 +27,7 @@ use crate::OrphanRoot; use crate::Package; use crate::Root; use crate::RootKind; +use crate::SourceDb; use crate::StaleRoot; use crate::WorkspaceRoots; @@ -137,7 +138,7 @@ impl DbInputs for TestDb { } #[salsa::db] -impl Db for TestDb { +impl SourceDb for TestDb { fn file_by_path(&self, path: &FilePath) -> Option { crate::db::file_by_path_query(self, path) } @@ -227,6 +228,9 @@ pub(super) fn make_package( (pkg, entities) } +#[salsa::db] +impl Db for TestDb {} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/oak_ide/tests/integration/base_sources.rs b/crates/oak_ide/tests/integration/base_sources.rs index fe135d846..1f4bfd4e7 100644 --- a/crates/oak_ide/tests/integration/base_sources.rs +++ b/crates/oak_ide/tests/integration/base_sources.rs @@ -28,8 +28,8 @@ use std::fs; use std::path::Path; -use oak_db::Db; use oak_db::OakDatabase; +use oak_db::SourceDb; use oak_scan::DbScan; use oak_source::SourceCache; diff --git a/crates/oak_scan/src/tests/scheduler.rs b/crates/oak_scan/src/tests/scheduler.rs index eda8006dd..364e94ffb 100644 --- a/crates/oak_scan/src/tests/scheduler.rs +++ b/crates/oak_scan/src/tests/scheduler.rs @@ -12,9 +12,9 @@ use std::fs; use std::path::Path; use aether_path::FilePath; -use oak_db::Db; use oak_db::DbInputs; use oak_db::OakDatabase; +use oak_db::SourceDb; use crate::lookup::package_by_path; use crate::scheduler::drain_scheduler; diff --git a/crates/oak_scan/src/tests/sources.rs b/crates/oak_scan/src/tests/sources.rs index f51eb1e1c..266bb7121 100644 --- a/crates/oak_scan/src/tests/sources.rs +++ b/crates/oak_scan/src/tests/sources.rs @@ -1,9 +1,9 @@ use std::fs; use std::path::Path; -use oak_db::Db; use oak_db::File; use oak_db::OakDatabase; +use oak_db::SourceDb; use crate::DbScan; diff --git a/crates/oak_scan/src/tests/stale.rs b/crates/oak_scan/src/tests/stale.rs index 55b4834ed..a296ffe54 100644 --- a/crates/oak_scan/src/tests/stale.rs +++ b/crates/oak_scan/src/tests/stale.rs @@ -5,7 +5,6 @@ use std::collections::HashSet; use aether_path::FilePath; -use oak_db::Db; use oak_db::DbInputs; use oak_db::File; use oak_db::FileRevision; @@ -13,6 +12,7 @@ use oak_db::OakDatabase; use oak_db::Package; use oak_db::Root; use oak_db::RootKind; +use oak_db::SourceDb; use salsa::Setter; use url::Url; diff --git a/crates/oak_scan/src/tests/watch.rs b/crates/oak_scan/src/tests/watch.rs index bc16e9be6..795b3be67 100644 --- a/crates/oak_scan/src/tests/watch.rs +++ b/crates/oak_scan/src/tests/watch.rs @@ -6,10 +6,10 @@ use std::path::PathBuf; use aether_path::FilePath; use filetime::set_file_mtime; use filetime::FileTime; -use oak_db::Db; use oak_db::DbInputs; use oak_db::OakDatabase; use oak_db::Root; +use oak_db::SourceDb; use crate::scheduler::drain_scheduler; use crate::DbScan; diff --git a/crates/oak_scan/src/tests/workspace.rs b/crates/oak_scan/src/tests/workspace.rs index c2adf6003..ab857350d 100644 --- a/crates/oak_scan/src/tests/workspace.rs +++ b/crates/oak_scan/src/tests/workspace.rs @@ -4,12 +4,12 @@ use std::path::Path; use std::path::PathBuf; use aether_path::FilePath; -use oak_db::Db; use oak_db::DbInputs; use oak_db::File; use oak_db::OakDatabase; use oak_db::Package; use oak_db::RootKind; +use oak_db::SourceDb; use crate::scheduler::drain_scheduler; use crate::DbScan; diff --git a/crates/oak_scan/tests/integration/library.rs b/crates/oak_scan/tests/integration/library.rs index 2f60a7e18..5ad30fcca 100644 --- a/crates/oak_scan/tests/integration/library.rs +++ b/crates/oak_scan/tests/integration/library.rs @@ -4,12 +4,12 @@ use std::path::Path; use std::path::PathBuf; use aether_path::FilePath; -use oak_db::Db; use oak_db::DbInputs; use oak_db::OakDatabase; use oak_db::Package; use oak_db::Root; use oak_db::RootKind; +use oak_db::SourceDb; use oak_package_metadata::namespace::Import; use oak_scan::DbScan; use oak_scan::RootExt; From dd2ebf9bed728468c25e213d2304c295abe69b01 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 17 Sep 2026 12:34:39 +0200 Subject: [PATCH 3/4] Remove `resolver_boundary` harness --- Cargo.toml | 3 - crates/oak_db/src/file.rs | 5 -- crates/oak_db/src/file_imports.rs | 6 -- crates/oak_db/src/imports.rs | 2 - crates/oak_db/src/imports/resolver_probe.rs | 80 ------------------- crates/oak_db/src/load_context.rs | 3 - crates/oak_db/src/tests.rs | 1 - crates/oak_db/src/tests/resolver_boundary.rs | 68 ---------------- ...ary__prohibited_forms_fail_to_compile.snap | 20 ----- 9 files changed, 188 deletions(-) delete mode 100644 crates/oak_db/src/imports/resolver_probe.rs delete mode 100644 crates/oak_db/src/tests/resolver_boundary.rs delete mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap diff --git a/Cargo.toml b/Cargo.toml index 6fdc24def..c2bd7fb95 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,9 +19,6 @@ edition = "2021" license = "MIT" authors = ["Posit Software, PBC"] -[workspace.lints.rust] -unexpected_cfgs = { level = "warn", check-cfg = ['cfg(resolver_boundary, values("probe", "control"))'] } - [workspace.lints.clippy] result_large_err = "allow" too_many_arguments = "allow" diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index 948ee01b1..e602a9e84 100644 --- a/crates/oak_db/src/file.rs +++ b/crates/oak_db/src/file.rs @@ -397,11 +397,6 @@ fn semantic_index_cycle_result(db: &dyn Db, _id: salsa::Id, file: File) -> Seman } fn semantic_index_fallback(db: &dyn SourceDb, file: File) -> SemanticIndex { - #[cfg(resolver_boundary = "probe")] - let _ = file.semantic_index(db); - #[cfg(resolver_boundary = "probe")] - let _: &dyn Db = db; - log::warn!( "Cyclic `source()` detected at {}. Rebuilding without cross-file resolution.", file.path(db), diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index bae372e1e..a9cfc0cfb 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -529,9 +529,6 @@ fn cross_file_layers_fallback( file: File, view: CollationView, ) -> CrossFileLayers { - #[cfg(resolver_boundary = "probe")] - let _ = file.attached_packages(db); - let mut layers = lower_load_context(db, &load_context(db, file, view)); layers.recovered_source_cycle = true; layers @@ -695,9 +692,6 @@ fn loaded_before(db: &dyn Db, source_file: File, file: File, offsets: &[TextSize /// search-path layers rank below them. Predecessor attaches are added only by /// the normal query, because reading them can re-enter semantic analysis. pub(crate) fn lower_load_context(db: &dyn SourceDb, context: &LoadContext) -> CrossFileLayers { - #[cfg(resolver_boundary = "probe")] - let _ = predecessor_attach_layers(db, &context.visible_files); - let LoadContext { kind, visible_files, diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index 32a0730cc..71145171a 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -23,8 +23,6 @@ use crate::Package; use crate::RootKind; use crate::SourceDb; -mod resolver_probe; - /// Salsa-backed [`ImportsResolver`] consumed by the per-file semantic /// index builder. One instance per call to [`File::semantic_index`]. /// diff --git a/crates/oak_db/src/imports/resolver_probe.rs b/crates/oak_db/src/imports/resolver_probe.rs deleted file mode 100644 index 0a78d9588..000000000 --- a/crates/oak_db/src/imports/resolver_probe.rs +++ /dev/null @@ -1,80 +0,0 @@ -//! These probes use the resolver's database field so weakening its type -//! also changes which calls compile. -//! -//! The boundary tests compile this crate with `resolver_boundary = "probe"` -//! to check that recursive queries and access to the underlying database are -//! rejected. A separate build with `resolver_boundary = "control"` checks -//! that `allowed_calls()` compiles, so unrelated errors -//! cannot make a rejected call look like successful enforcement. - -#[cfg(resolver_boundary = "control")] -use aether_path::FilePath; - -use super::SalsaImportsResolver; -#[cfg(any(resolver_boundary = "probe", resolver_boundary = "control"))] -use crate::file_imports::CollationView; -#[cfg(resolver_boundary = "probe")] -use crate::Db; - -impl<'db> SalsaImportsResolver<'db> { - #[cfg(resolver_boundary = "probe")] - fn calls_semantic_index_directly(&self) { - let _ = self.file.semantic_index(self.db); - } - - #[cfg(resolver_boundary = "probe")] - fn calls_diagnostics_directly(&self) { - let _ = self.file.diagnostics(self.db); - } - - #[cfg(resolver_boundary = "probe")] - fn calls_imports_directly(&self) { - let _ = self.file.imports(self.db); - } - - #[cfg(resolver_boundary = "probe")] - fn escapes_to_dyn_db(&self) { - let _: &dyn Db = self.db; - } - - #[cfg(resolver_boundary = "probe")] - fn escapes_through_private_field(&self) { - let _: &dyn Db = self.db.db; - } - - #[cfg(resolver_boundary = "probe")] - fn sources_call_exports(&self) { - let _ = self.file.exports(self.db.as_source_db()); - } - - #[cfg(resolver_boundary = "probe")] - fn sources_call_attached_packages(&self) { - let _ = self.file.attached_packages(self.db.as_source_db()); - } - - #[cfg(resolver_boundary = "probe")] - fn sources_call_cross_file_layers(&self) { - let _ = self - .file - .cross_file_layers(self.db.as_source_db(), CollationView::Eager); - } - - #[cfg(resolver_boundary = "probe")] - fn sources_escape_to_dyn_db(&self) { - let _: &dyn Db = self.db.as_source_db(); - } - - #[cfg(resolver_boundary = "control")] - fn allowed_calls(&self) { - let _ = self.db.exports(self.file); - let _ = self.db.attached_package_names(self.file); - let _ = self.db.cross_file_layers(self.file, CollationView::Eager); - let _ = self.file.path(self.db.as_source_db()); - let _ = self.file.parse(self.db.as_source_db()); - } - - #[cfg(resolver_boundary = "control")] - fn borrowed_path(&self) -> &'db FilePath { - self.file.path(self.db.as_source_db()) - } -} diff --git a/crates/oak_db/src/load_context.rs b/crates/oak_db/src/load_context.rs index 4d7268dba..5b53458ee 100644 --- a/crates/oak_db/src/load_context.rs +++ b/crates/oak_db/src/load_context.rs @@ -96,9 +96,6 @@ pub(crate) enum SearchPathTail { /// precedes package loading and package ownership precedes directory /// conventions. pub(crate) fn load_context(db: &dyn SourceDb, file: File, view: CollationView) -> LoadContext { - #[cfg(resolver_boundary = "probe")] - let _ = file.semantic_index(db); - if let Some(context) = contrib::testthat::load_context(db, file, view) { return context; } diff --git a/crates/oak_db/src/tests.rs b/crates/oak_db/src/tests.rs index 2b998c703..fa07fd7a3 100644 --- a/crates/oak_db/src/tests.rs +++ b/crates/oak_db/src/tests.rs @@ -17,7 +17,6 @@ mod inputs; mod package_resolve; mod recovery; mod resolver; -mod resolver_boundary; mod source_firewall; pub(crate) mod test_db; mod tidy; diff --git a/crates/oak_db/src/tests/resolver_boundary.rs b/crates/oak_db/src/tests/resolver_boundary.rs deleted file mode 100644 index 439be22e7..000000000 --- a/crates/oak_db/src/tests/resolver_boundary.rs +++ /dev/null @@ -1,68 +0,0 @@ -//! Checks resolver and recovery restrictions through the actual database -//! parameters. The control build verifies that allowed operations compile. - -use std::env; -use std::process::Command; -use std::process::Output; - -/// Cargo reports diagnostic paths relative to the workspace root. -const PROBE_FILES: &[&str] = &[ - "crates/oak_db/src/imports/resolver_probe.rs", - "crates/oak_db/src/file.rs", - "crates/oak_db/src/file_imports.rs", - "crates/oak_db/src/load_context.rs", -]; - -const PROHIBITED_FORM_COUNT: usize = 14; - -/// Passing the cfg through `cargo rustc` leaves dependencies cached. -/// Setting `RUSTFLAGS` would rebuild them too. `CARGO_TERM_COLOR` disables -/// color without conflicting with Cargo's rustc `--json` argument. -fn compile_with_cfg(value: &str) -> Output { - let cargo = env::var("CARGO").unwrap_or_else(|_| "cargo".to_string()); - Command::new(cargo) - .current_dir(env!("CARGO_MANIFEST_DIR")) - .env("CARGO_TERM_COLOR", "never") - .args([ - "rustc", - "-p", - "oak_db", - "--lib", - "--message-format=short", - "--", - "--cfg", - &format!("resolver_boundary=\"{value}\""), - "--emit=metadata", - ]) - .output() - .unwrap() -} - -#[test] -fn test_allowed_forms_compile() { - let output = compile_with_cfg("control"); - assert!(output.status.success()); -} - -#[test] -fn test_prohibited_forms_fail_to_compile() { - let output = compile_with_cfg("probe"); - assert!(!output.status.success()); - - // Use the same diagnostic paths on Windows and Unix for filtering and snapshots. - let stderr = String::from_utf8_lossy(&output.stderr).replace('\\', "/"); - // Only probe locations belong in the snapshot, not Cargo's error summary. - let mut lines: Vec<&str> = stderr - .lines() - .filter(|line| PROBE_FILES.iter().any(|path| line.starts_with(path))) - .collect(); - lines.sort_unstable(); - assert_eq!(lines.len(), PROHIBITED_FORM_COUNT); - - insta::with_settings!({description => "\ - Each line is one prohibited form rejected in the resolver probes or an actual \ - recovery helper. Line numbers shift when those files are edited. A NEW or MISSING \ - line means the boundary itself changed and needs review, not a blind `cargo insta accept`."}, { - insta::assert_snapshot!(lines.join("\n")); - }); -} diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap deleted file mode 100644 index 64f666864..000000000 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__resolver_boundary__prohibited_forms_fail_to_compile.snap +++ /dev/null @@ -1,20 +0,0 @@ ---- -source: crates/oak_db/src/tests/resolver_boundary.rs -assertion_line: 66 -description: "Each line is one prohibited form rejected in the resolver probes or an actual recovery helper. Line numbers shift when those files are edited. A NEW or MISSING line means the boundary itself changed and needs review, not a blind `cargo insta accept`." -expression: "lines.join(\"\\n\")" ---- -crates/oak_db/src/file.rs:401:33: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` -crates/oak_db/src/file.rs:403:22: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` -crates/oak_db/src/file_imports.rs:531:36: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` -crates/oak_db/src/file_imports.rs:700:39: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` -crates/oak_db/src/imports/resolver_probe.rs:22:42: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:27:39: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:32:35: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:37:26: error[E0308]: mismatched types: expected `&dyn Db`, found `ResolverDb<'_>` -crates/oak_db/src/imports/resolver_probe.rs:42:34: error[E0616]: field `db` of struct `ResolverDb` is private: private field -crates/oak_db/src/imports/resolver_probe.rs:47:35: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` -crates/oak_db/src/imports/resolver_probe.rs:52:45: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` -crates/oak_db/src/imports/resolver_probe.rs:59:32: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` -crates/oak_db/src/imports/resolver_probe.rs:64:26: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` -crates/oak_db/src/load_context.rs:100:33: error[E0308]: mismatched types: expected trait `Db`, found trait `SourceDb` From 0e49c9c0415e49c408a81851959cceaaf8a0d7d8 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 17 Sep 2026 15:32:31 +0200 Subject: [PATCH 4/4] Update salsa.md doc --- doc/oak/salsa.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/oak/salsa.md b/doc/oak/salsa.md index 9c6e3ed4f..e08d4865b 100644 --- a/doc/oak/salsa.md +++ b/doc/oak/salsa.md @@ -18,7 +18,7 @@ Every `oak_db` cycle handler must call `recovery::record()` with the query key b A query's body does not show every query it can reach. For example, `File::semantic_index()` calls `build_semantic_index()`, which eventually reaches `exports()` through `SalsaImportsResolver` and `oak_semantic`. `File::exports()` calls `semantic_index()` directly. -1. Follow calls through ordinary helpers, not only Salsa queries. Check `build_semantic_index()`, the collation helpers in `file_imports.rs`, `SalsaImportsResolver` methods in `imports.rs`, and `Db` / `DbInputs` methods in `storage.rs`. +1. Follow calls through ordinary helpers, not only Salsa queries. Record which database capability each helper accepts. A helper accepting only `&dyn SourceDb` cannot call `Db`-only semantic queries through that parameter. Recovery fallbacks rely on this restriction, so review `SourceDb` methods and their transitive dependencies for semantic-query calls, and do not widen fallback parameters to `&dyn Db`. `ResolverDb` is the deliberate exception: it wraps the full `Db` but exposes only three cycle-aware forwarding methods. Review each forwarding method's transitive dependencies when changing it. Check `build_semantic_index()`, the collation helpers in `file_imports.rs`, `SalsaImportsResolver` methods in `imports.rs`, `ResolverDb` methods in `resolver_db.rs`, and the `DbInputs`, `SourceDb`, and `Db` implementations in `storage.rs`. 2. List every production operation that can start a path to the query. Check diagnostics, file resolution, `resolve_at()`, `imports_at()`, package resolution, and workspace aggregates. 3. From each starting operation, trace whether execution can return to the same query key. 4. Check every starting operation, even after finding a cycle through another query. The query under review needs a handler if any path can return to it.