diff --git a/crates/oak_db/src/diagnostic.rs b/crates/oak_db/src/diagnostic.rs index 80260f193..8f2288a0c 100644 --- a/crates/oak_db/src/diagnostic.rs +++ b/crates/oak_db/src/diagnostic.rs @@ -1,4 +1,5 @@ use biome_rowan::TextRange; +use biome_rowan::TextSize; use oak_semantic::semantic_index::AmbiguityReason; use oak_semantic::semantic_index::SemanticDiagnostic; @@ -57,6 +58,8 @@ pub enum DiagnosticKind { AmbiguousEffect, AmbiguousAttachOrder, UninstalledPackage, + SourceCycle, + InheritedShadow, } impl DiagnosticKind { @@ -66,6 +69,8 @@ impl DiagnosticKind { DiagnosticKind::AmbiguousEffect => "ambiguous-effect", DiagnosticKind::AmbiguousAttachOrder => "ambiguous-attach-order", DiagnosticKind::UninstalledPackage => "uninstalled-package", + DiagnosticKind::SourceCycle => "source-cycle", + DiagnosticKind::InheritedShadow => "inherited-shadow", } } @@ -74,6 +79,8 @@ impl DiagnosticKind { DiagnosticKind::AmbiguousEffect => Severity::Info, DiagnosticKind::AmbiguousAttachOrder => Severity::Info, DiagnosticKind::UninstalledPackage => Severity::Warning, + DiagnosticKind::SourceCycle => Severity::Warning, + DiagnosticKind::InheritedShadow => Severity::Info, } } @@ -82,6 +89,8 @@ impl DiagnosticKind { DiagnosticKind::AmbiguousEffect => true, DiagnosticKind::AmbiguousAttachOrder => true, DiagnosticKind::UninstalledPackage => true, + DiagnosticKind::SourceCycle => true, + DiagnosticKind::InheritedShadow => true, } } } @@ -108,6 +117,7 @@ pub(crate) fn lower_semantic_diagnostic(diagnostic: &SemanticDiagnostic) -> Diag SemanticDiagnostic::UninstalledPackage { package, range } => { lower_uninstalled_package(package, *range) }, + SemanticDiagnostic::SourceCycle => lower_source_cycle(), } } @@ -188,3 +198,16 @@ fn lower_uninstalled_package(package: &str, range: TextRange) -> Diagnostic { Vec::new(), ) } + +/// Anchored at the start of the file because the record carries no range. +/// Every file in the cycle gets its own diagnostic. +fn lower_source_cycle() -> Diagnostic { + Diagnostic::new( + DiagnosticKind::SourceCycle, + "This file takes part in a cycle of mutual `source()` calls. \ + Language analysis will be incomplete until the cycle is resolved." + .to_string(), + TextRange::empty(TextSize::from(0)), + Vec::new(), + ) +} diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index 1473b9c05..4d3b7ad95 100644 --- a/crates/oak_db/src/file.rs +++ b/crates/oak_db/src/file.rs @@ -1,11 +1,13 @@ use std::fs; use aether_path::FilePath; +use oak_semantic::semantic_index::SemanticDiagnostic; use oak_semantic::semantic_index::SemanticIndex; use crate::db::root_by_file; use crate::diagnostic::lower_semantic_diagnostic; use crate::diagnostic::Diagnostic; +use crate::file_diagnostics::inherited_shadow_diagnostics; use crate::file_revision::report_untracked_if_zero; use crate::imports::SalsaImportsResolver; use crate::parse::OakParse; @@ -153,24 +155,22 @@ impl File { /// dependency graph through both `semantic_index` and `exports`: /// `semantic_index(A) -> SalsaImportsResolver -> exports(B) -> /// semantic_index(B) -> SalsaImportsResolver -> exports(A) -> - /// semantic_index(A)`. Salsa picks one query to break the cycle and - /// panics with "set cycle_fn/cycle_initial" unless that query has a - /// handler. Both `semantic_index` and the narrow queries (`exports`, - /// `imports`, `resolve`) carry their own `cycle_result`. + /// semantic_index(A)`. /// /// The two handlers behave differently: /// - /// - `semantic_index` (this query, custom rebuild): the cycling - /// side is rebuilt with `NoopImportsResolver`. Cross-file - /// injection drops, but local analysis (scopes, use-def maps, - /// function bodies) is preserved. + /// - `semantic_index` (this query, custom rebuild): the file is rebuilt + /// with `NoopImportsResolver`. Cross-file injection drops, but local + /// analysis (scopes, use-def maps, function bodies) is preserved. /// - /// - `exports` / `imports` / `resolve` (FallbackImmediate, empty): - /// the cycling side gets an empty fallback for that query. + /// - `exports` (FallbackImmediate, empty): the file contributes no names + /// for the revision. /// - /// Which handler fires depends on which query salsa first re-enters. - /// R doesn't allow cyclic `source()`, so the asymmetric recovery is - /// acceptable. TODO(diagnostics): Lint `source()` cycles. + /// `FallbackImmediate` causes *every* participant in the cycle to fall + /// back, not just the one salsa re-entered. This means both ends of a + /// mutual `source()` pair rebuild under Noop and none of the source effects + /// resolve. A [`SemanticDiagnostic::SourceCycle`] is raised to document + /// this state. /// /// `no_eq` skips salsa's `values_equal` check after recomputation. /// Backdating at this level never triggered in practice anyway: `AstPtr` @@ -252,7 +252,7 @@ impl File { names } - /// Diagnostics derived from this file's semantic index. + /// Diagnostics for this file. /// /// Not keyed on user configuration, so the memo isn't duplicated per /// setting combination. Consumers filter on severity and @@ -262,11 +262,15 @@ impl File { /// `attached_packages()` and friends this query can't backdate. #[salsa::tracked(returns(ref))] pub fn diagnostics(self, db: &dyn Db) -> Vec { - self.semantic_index(db) + let mut diagnostics: Vec = self + .semantic_index(db) .diagnostics() .iter() .map(lower_semantic_diagnostic) - .collect() + .collect(); + + diagnostics.extend(inherited_shadow_diagnostics(db, self)); + diagnostics } /// The root containing this file, if any. @@ -349,6 +353,7 @@ fn semantic_index_cycle_result(db: &dyn Db, _id: salsa::Id, file: File) -> Seman ); let parsed = file.parse(db); oak_semantic::build_index(&parsed.tree(), oak_semantic::NoopImportsResolver) + .with_diagnostic(SemanticDiagnostic::SourceCycle) } /// Test-only recorder for the deepest `build_semantic_index` nesting. diff --git a/crates/oak_db/src/file_diagnostics.rs b/crates/oak_db/src/file_diagnostics.rs new file mode 100644 index 000000000..372db7a60 --- /dev/null +++ b/crates/oak_db/src/file_diagnostics.rs @@ -0,0 +1,137 @@ +use rustc_hash::FxHashSet; + +use crate::diagnostic::Diagnostic; +use crate::diagnostic::DiagnosticKind; +use crate::file_imports::CollationView; +use crate::file_imports::ImportLayer; +use crate::file_resolve::resolve_import_layer; +use crate::Db; +use crate::File; +use crate::Name; + +/// Diagnostics for effect ambiguities produced by source inheritance. +/// +/// ```r +/// # main.R # helpers.R +/// source <- identity source("more.R") +/// base::source("helpers.R") +/// ``` +/// +/// `helpers.R` is analysed on its own, so its `source("more.R")` reads as base +/// `source` and we follow it. But `main.R` binds `source` to `identity` before +/// sourcing `helpers.R`, so at runtime that call might do nothing at all. +/// We report this ambiguity with a diagnostic. +/// +/// A call is ambiguous when its callee resolves through [`File::imports`] (which +/// includes context inherited from the sourcing files) to a layer absent from +/// [`File::standalone_imports`] (the narrower view without inheritance). +pub(crate) fn inherited_shadow_diagnostics(db: &dyn Db, file: File) -> Vec { + if file.inherited_layers(db, CollationView::Lazy).is_empty() { + return Vec::new(); + } + + let inherited = file.imports(db); + let standalone = file.standalone_imports(db); + + let mut reported = FxHashSet::default(); + let mut diagnostics = Vec::new(); + + for call in file.semantic_index(db).semantic_calls() { + let Some(callee) = call.callee() else { + continue; + }; + // One `source()` call forwards one `Attach` per package attached in the + // target, all sharing its range. Report the site once. + if !reported.insert(call.range()) { + continue; + } + + let name = Name::new(db, callee); + + // A binding in this file wins on both sides, so there's nothing to + // disagree about. This is the same first step `File::resolve` takes. + if !file.resolve_export(db, name).is_empty() { + continue; + } + + let Some(resolved_layer) = resolve_layer(db, inherited, name) else { + continue; + }; + + if standalone.contains(&resolved_layer) { + continue; + } + + // A layer the standalone view lacks can only have come from an + // inherited band, so the site is always there to find. + let Some(sourcing) = sourcing_file(db, file, &resolved_layer) else { + continue; + }; + + // When nothing on the standalone path binds the callee, the scan fell + // through to base's builtins, which resolve by name whether or not base + // was scanned into a root. See `SalsaImportsResolver`. + let alone = match resolve_layer(db, &standalone, name) { + Some(layer) => describe_source(db, &layer), + None => "package `base`".to_string(), + }; + + diagnostics.push(Diagnostic::new( + DiagnosticKind::InheritedShadow, + format!( + "This `{callee}` call has an ambiguous effect. It resolves through {alone} when \ + the file is sourced on its own, and to {inherited} when sourced by `{sourcing}`.", + inherited = describe_source(db, &resolved_layer), + ), + call.range(), + Vec::new(), + )); + } + + diagnostics +} + +/// The first layer that binds `name`, i.e. the one a lookup would settle on. +fn resolve_layer<'db>( + db: &'db dyn Db, + layers: &[ImportLayer], + name: Name<'db>, +) -> Option { + layers + .iter() + .find(|layer| !resolve_import_layer(db, layer, name).is_empty()) + .cloned() +} + +/// The name of the file whose inherited band contributed `layer`. +/// +/// Names the direct sourcing file even for a layer that reaches `file` from +/// further up the chain, since [`build_inherited_layers`] folds each site's +/// grandparents into that site's own bands. +/// +/// [`build_inherited_layers`]: crate::file_imports +fn sourcing_file(db: &dyn Db, file: File, layer: &ImportLayer) -> Option { + file.inherited_layers(db, CollationView::Lazy) + .iter() + .find(|site| site.layers.above.contains(layer) || site.layers.below.contains(layer)) + .map(|site| { + site.file + .path(db) + .file_name() + .unwrap_or_default() + .to_string() + }) +} + +fn describe_source(db: &dyn Db, layer: &ImportLayer) -> String { + match layer { + ImportLayer::File(file) | ImportLayer::SourcingFile { file, .. } => { + format!( + "a binding in `{}`", + file.path(db).file_name().unwrap_or_default() + ) + }, + ImportLayer::Package(package) => format!("package `{}`", package.name(db)), + ImportLayer::From(importer) => format!("an import of `{}`", importer.name(db)), + } +} diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 5877d6c5e..34eb4ec6b 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -1,9 +1,11 @@ use std::borrow::Cow; +use std::slice; use biome_rowan::TextSize; use camino::Utf8Path; use oak_package_metadata::namespace::Namespace; use oak_semantic::semantic_index::AttachRegion; +use oak_semantic::semantic_index::ExportsAtSource; use oak_semantic::semantic_index::ScopeId; use oak_semantic::semantic_index::SemanticCall; use oak_semantic::semantic_index::SemanticCallKind; @@ -19,9 +21,17 @@ use crate::Package; /// package-name strings cross out of `oak_db` for resolution. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ImportLayer { - /// A predecessor file in a package's collation, or another workspace - /// file. Names are resolved through `file.exports(db)`. + /// A file whose top level has fully run by the time this layer is read: a + /// collation predecessor, or a sourcing file seen from a lazy context. + /// Names are resolved through `file.exports(db)`. File(File), + /// A file that sources the one being resolved, seen as of its `source()` + /// call. The rest of `file` hadn't run by then, so only `exports_so_far` + /// counts. + SourcingFile { + file: File, + exports_so_far: ExportsAtSource, + }, /// The package whose NAMESPACE declares `importFrom(pkg, name)` entries. /// [`Package::import_index`] says which entry, if any, binds a given name. From(Package), @@ -67,18 +77,19 @@ impl CrossFileLayers { /// Which of a file's own `library()` attaches a caller sees. #[derive(Clone, Copy)] -enum AttachView { +enum AttachView<'a> { /// Every attach in the file. /// /// Over-approximates on two axes. An attach in a function body counts even /// though the body may never run, and a conditional one counts even though /// its branch may not have been taken. Anywhere, - /// Attaches visible at `offset` in lazy `scope`. + /// Attaches visible at `offset` in the lazy scope `scope_id`. /// - /// An attach in `scope_id` or an enclosing lazy body applies only after its - /// `library()` call. The lazy view treats an unconditional top-level attach - /// as visible regardless of position, which over-approximates this case: + /// An attach in `scope_id` itself or an enclosing lazy body applies only after its + /// `library()` call. On the other hand, the lazy view treats an + /// unconditional top-level attach as visible regardless of position, which + /// over-approximates this case: /// /// ```r /// f <- function() { @@ -95,13 +106,40 @@ enum AttachView { /// Conditional attaches remain limited to their arm, and child or sibling /// bodies do not reach `scope_id`. Lazy { offset: TextSize, scope_id: ScopeId }, - /// The attaches that have run and still hold at `offset` in an eagerly - /// evaluated scope. Calls reached only by running a lazy body are dropped, - /// as are calls after the offset. - Eager(TextSize), + /// Attaches in eager scopes that are visible at any of `offsets`. + /// + /// Attaches in lazy bodies and after every offset are excluded. Multiple + /// offsets represent distinct `source()` sites, which may have different + /// attach views. + /// + /// ```r + /// library(pkga) + /// source("init.R") + /// + /// library(pkgb) + /// source("init.R") + /// ``` + /// + /// or + /// + /// ```r + /// library(pkga) + /// if (cond1) { + /// source("init.R") + /// } + /// library(pkgb) + /// ... + /// if (cond2) { + /// source("init.R") + /// } + /// ``` + /// + /// `init.R` runs twice, represented by an offset for each `source()` + /// call. It sees `pkga` in both cases but only the second run sees `pkgb`. + Eager(&'a [TextSize]), } -impl AttachView { +impl AttachView<'_> { fn sees(&self, index: &SemanticIndex, call: &SemanticCall, region: &AttachRegion) -> bool { match *self { AttachView::Anywhere => true, @@ -124,13 +162,22 @@ impl AttachView { region.contains(call, offset) }, }, - AttachView::Eager(offset) => { - index.scope_is_eager(call.scope()) && region.contains(call, offset) + AttachView::Eager(offsets) => { + index.scope_is_eager(call.scope()) && + offsets.iter().any(|&offset| region.contains(call, offset)) }, } } } +/// Layers that a sourcing file makes visible to the sourced file. `file` is the +/// file holding the `source()` call. +#[derive(Debug, Clone, PartialEq, Eq, salsa::SalsaValue)] +pub(crate) struct InheritedLayers { + pub file: File, + pub layers: CrossFileLayers, +} + /// The point in a package's load at which a file views its collation siblings. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub(crate) enum CollationView { @@ -162,7 +209,7 @@ impl File { /// of imports. #[salsa::tracked(returns(ref))] pub fn imports(self, db: &dyn Db) -> Vec { - let layers = self.cross_file_layers(db, CollationView::Lazy); + let layers = self.resolution_layers(db, CollationView::Lazy); let own = self.attach_layers(db, AttachView::Anywhere); layers.lookup_order(&own).cloned().collect() } @@ -198,7 +245,10 @@ impl File { let (collation, attaches) = if index.scope_is_eager(cursor_scope) { // Predecessors only, and own attaches narrowed to the calls that // have run by `offset`. - (CollationView::Eager, AttachView::Eager(offset)) + ( + CollationView::Eager, + AttachView::Eager(slice::from_ref(&offset)), + ) } else { (CollationView::Lazy, AttachView::Lazy { offset, @@ -206,17 +256,138 @@ impl File { }) }; - let layers = self.cross_file_layers(db, collation); + let layers = self.resolution_layers(db, collation); let own = self.attach_layers(db, attaches); layers.lookup_order(&own).cloned().collect() } + fn resolution_layers<'db>( + self, + db: &'db dyn Db, + view: CollationView, + ) -> Cow<'db, CrossFileLayers> { + let inherited = self.inherited_layers(db, view); + if inherited.is_empty() { + return Cow::Borrowed(self.cross_file_layers(db, view)); + } + + let mut above = Vec::new(); + let mut below = Vec::new(); + for site in inherited { + above.extend(site.layers.above.iter().cloned()); + below.extend(site.layers.below.iter().cloned()); + } + Cow::Owned(CrossFileLayers { above, below }) + } + + /// The lookup-ordered layers this file's lazy / end-of-file view sees, one + /// context per file that sources this one (see [`File::inherited_layers`]), + /// or a single context of [`File::cross_file_layers`] if no inheritance. + /// + /// The contexts of sourcing files are resolved as alternatives not as a + /// priority order. Symbols resolve in each context and are returned as a + /// union of results. + /// + /// Tracked query, firewall between `resolve()` and the `no_eq` + /// `semantic_index` read by `attach_layers()`. + #[salsa::tracked(returns(ref))] + pub(crate) fn imports_by_sourcing_file(self, db: &dyn Db) -> Vec> { + let own = self.attach_layers(db, AttachView::Anywhere); + self.layers_by_sourcing_file(db, CollationView::Lazy) + .into_iter() + .map(|context| context.lookup_order(&own).cloned().collect()) + .collect() + } + + /// [`File::imports_by_sourcing_file`], narrowed to `offset` the way + /// [`File::imports_at`] narrows [`File::imports`]. + /// + /// Not tracked because keying a cache on `(self, offset)` would add an entry + /// per cursor position. + pub(crate) fn imports_by_sourcing_file_at( + self, + db: &dyn Db, + offset: TextSize, + ) -> Vec> { + let index = self.semantic_index(db); + let (cursor_scope, _) = index.scope_at(offset); + + let (collation, attaches) = if index.scope_is_eager(cursor_scope) { + ( + CollationView::Eager, + AttachView::Eager(slice::from_ref(&offset)), + ) + } else { + (CollationView::Lazy, AttachView::Lazy { + offset, + scope_id: cursor_scope, + }) + }; + + let own = self.attach_layers(db, attaches); + self.layers_by_sourcing_file(db, collation) + .into_iter() + .map(|context| context.lookup_order(&own).cloned().collect()) + .collect() + } + + /// One context per file that sources this one, or a single context when + /// nothing does. + fn layers_by_sourcing_file(self, db: &dyn Db, view: CollationView) -> Vec<&CrossFileLayers> { + let inherited = self.inherited_layers(db, view); + if inherited.is_empty() { + return vec![self.cross_file_layers(db, view)]; + } + inherited.iter().map(|site| &site.layers).collect() + } + + /// The layers `self` inherits from each file that sources it, one entry per + /// file in `self.sourced_by(db)`, each recursively including what that file + /// itself inherits. That recursion is what makes inheritance transitive + /// across a `main.R -> setup.R -> helpers.R` chain. + /// + /// Empty for a file with an explicit load order. + /// + /// `cycle_result` is defensive. Resolving a source site reads the target's + /// `exports`, meaning that a source cycle is always also a `semantic_index` + /// cycle which has its own recovery. + #[salsa::tracked(returns(ref), cycle_result = + inherited_layers_cycle_result)] + pub(crate) fn inherited_layers(self, db: &dyn Db, view: CollationView) -> Vec { + if self.has_explicit_load_order(db) { + return Vec::new(); + } + + self.sourced_by(db) + .iter() + .map(|&sourcing_file| build_inherited_layers(db, self, sourcing_file, view)) + .collect() + } + + /// Whether something other than a `source()` call fixes load order, e.g. + /// package or testthat collation. + fn has_explicit_load_order(self, db: &dyn Db) -> bool { + let Some(package) = self.package(db) else { + return false; + }; + is_testthat_file(self, db) || self.is_package_source(db, package) + } + + /// Bare [`File::imports`], without inheritance. + pub(crate) fn standalone_imports(self, db: &dyn Db) -> Vec { + let own = self.attach_layers(db, AttachView::Anywhere); + self.cross_file_layers(db, CollationView::Lazy) + .lookup_order(&own) + .cloned() + .collect() + } + /// This file's own `library()` / `require()` attaches as `Package` layers, /// in LIFO order (latest-attached first), narrowed to what `view` admits. /// Reads the file's own semantic index. /// /// An attach to a package absent from every root is dropped (no entity). - fn attach_layers(self, db: &dyn Db, view: AttachView) -> Vec { + fn attach_layers(self, db: &dyn Db, view: AttachView<'_>) -> Vec { let index = self.semantic_index(db); index .semantic_calls() @@ -310,6 +481,101 @@ impl File { } } +fn inherited_layers_cycle_result( + _db: &dyn Db, + _id: salsa::Id, + _file: File, + _view: CollationView, +) -> Vec { + Vec::new() +} + +/// What `sourcing_file` contributes to `target`, its own bands plus what it +/// inherits in turn. +/// +/// `sourcing_file`'s own `below` band goes last because the default search +/// path lives at the end of it, and that has to stay at the bottom of the +/// whole chain. +fn build_inherited_layers( + db: &dyn Db, + file: File, + source_site: File, + view: CollationView, +) -> InheritedLayers { + let offsets = match view { + CollationView::Lazy => None, + CollationView::Eager => source_offsets(db, source_site, file), + }; + + let own_cross = source_site.cross_file_layers(db, view); + let grandparents = source_site.inherited_layers(db, view); + + let (own_attach, exports_so_far) = match offsets.as_deref() { + Some(offsets) => ( + source_site.attach_layers(db, AttachView::Eager(offsets)), + source_site.semantic_index(db).exports_at_sources(offsets), + ), + None => (source_site.attach_layers(db, AttachView::Anywhere), None), + }; + + // An unpinned `source()` call may run after any top-level binding, so use + // the whole file as over-approximation. + let source_layer = match exports_so_far { + Some(exports_so_far) => ImportLayer::SourcingFile { + file: source_site, + exports_so_far, + }, + None => ImportLayer::File(source_site), + }; + + let mut above = vec![source_layer]; + above.extend(own_cross.above.iter().cloned()); + above.extend( + grandparents + .iter() + .flat_map(|site| site.layers.above.iter().cloned()), + ); + + let mut below = own_attach; + below.extend( + grandparents + .iter() + .flat_map(|site| site.layers.below.iter().cloned()), + ); + below.extend(own_cross.below.iter().cloned()); + + InheritedLayers { + file: source_site, + layers: CrossFileLayers { above, below }, + } +} + +/// Returns source-order offsets for eager `source()` calls from `sourcing_file` to +/// `file`. +/// +/// `None` leaves the context unpinned when no calls target `file` or any call is +/// lazy. A lazy call can run after the rest of `sourcing_file`, so no offset bounds +/// its imports. +fn source_offsets(db: &dyn Db, sourcing_file: File, file: File) -> Option> { + let index = sourcing_file.semantic_index(db); + let mut offsets = Vec::new(); + + for site in sourcing_file.source_sites(db) { + if site.target() != Some(file) { + continue; + } + if !index.scope_is_eager(site.scope()) { + return None; + } + offsets.push(site.offset()); + } + + match offsets.is_empty() { + true => None, + false => Some(offsets), + } +} + fn package_load_layers( file: File, db: &dyn Db, diff --git a/crates/oak_db/src/file_resolve.rs b/crates/oak_db/src/file_resolve.rs index 0e8219afa..c235200c5 100644 --- a/crates/oak_db/src/file_resolve.rs +++ b/crates/oak_db/src/file_resolve.rs @@ -30,16 +30,19 @@ impl<'db> File { /// `source()`-forwarded entries. `ExportEntry::Import` is chased /// through `exports(target)` until it lands on a `Local`. Cycles in /// `source()` resolve to empty exports via `exports`'s `cycle_fn`. - /// 2. **`imports()` walk**: each layer is checked in priority order. - /// `File` siblings are checked via their exports chain only (not their - /// full `resolve`), to avoid the cycle that recursing would create. - /// `Package` and `From` layers call [`Package::resolve`] with - /// `Exported` visibility. + /// 2. **`imports_by_sourcing_file()` walk**: one context per file that + /// sources this one, each checked in priority order and the results + /// unioned across contexts. `File` siblings are checked via their + /// exports chain only (not their full `resolve`), to avoid the cycle + /// that recursing would create. `Package` and `From` layers call + /// [`Package::resolve`] with `Exported` visibility. /// - /// Returns every definition the name reaches in the first layer that binds - /// it, so a name with two top-level bindings yields both. The own-file - /// `exports()` chain shadows imports, matching R: if the file binds the - /// name at top level we stop there and never fall through to a package. + /// Returns every definition the name reaches in the first binding layer of + /// each context, so a name with two top-level bindings yields both, as does + /// a name bound in two different files that each source this one (see + /// [`resolve_per_sourcing_file`]). The own-file `exports()` chain shadows + /// imports, matching R: if the file binds the name at top level we stop + /// there and never fall through to a package. /// /// Each returned `Definition` is keyed by `(file, scope, name)`, so /// downstream queries that only depend on identity stay cached across @@ -56,26 +59,7 @@ impl<'db> File { return exported; } - // For each sibling `ImportLayer::File`, check the target's exports - // chain only. Recursing into `target.resolve()` would walk the - // target's imports, which include *this* file (sibling exclusion - // is per-file), and salsa would cycle on any unbound name. - // - // Exports-only is also what R's namespace semantics asks for. A - // package's namespace is the merged *exports* of its collation - // files, so "what does sibling B contribute to the namespace?" is - // exactly "what's in B's exports?". Package-wide NAMESPACE imports - // and the installed-package search path appear in this file's own - // `imports()` directly, as `From` / `Package` layers, so finding - // them does not require walking through siblings. - for layer in self.imports(db) { - let defs = resolve_import_layer(db, layer, name); - if !defs.is_empty() { - return defs; - } - } - - Vec::new() + resolve_per_sourcing_file(db, self.imports_by_sourcing_file(db), name) } /// Resolve the name at `offset` to its definition(s). @@ -121,23 +105,15 @@ impl<'db> File { } // Nothing local reaches the use, so resolve across files. - let file_scope = ScopeId::from(0); - if use_scope != file_scope { - // Function body: the lazy / end-of-file view the body sees at run time. + if !index.scope_is_eager(use_scope) { + // Lazy body: the end-of-file view it sees when it actually runs. return self.resolve(db, name); } - // Top level: collation predecessors / other visible files (exports-only - // chase, same as `resolve`'s imports walk). Avoids the sibling cycle and - // matches R's namespace semantics. - for layer in self.imports_at(db, offset) { - let defs = resolve_import_layer(db, &layer, name); - if !defs.is_empty() { - return defs; - } - } - - Vec::new() + // Eager scope: collation predecessors / other visible files + // (exports-only chase, same as `resolve`'s per-context walk). Avoids + // the sibling cycle and matches R's namespace semantics. + resolve_per_sourcing_file(db, &self.imports_by_sourcing_file_at(db, offset), name) } fn resolve_definition( @@ -257,7 +233,7 @@ impl<'db> File { /// *this* file, and salsa would cycle on any unbound name. Exports-only is also /// what R's namespace semantics asks for, a package's namespace is the merged /// exports of its collation files. -fn resolve_import_layer<'db>( +pub(crate) fn resolve_import_layer<'db>( db: &'db dyn Db, layer: &ImportLayer, name: Name<'db>, @@ -266,7 +242,19 @@ fn resolve_import_layer<'db>( // that package's exports. A `File` sibling is the exception, resolved // through its own exports chain. let package = match layer { - ImportLayer::File(target) => return target.resolve_export(db, name), + ImportLayer::File(file) => return file.resolve_export(db, name), + ImportLayer::SourcingFile { + file, + exports_so_far, + } => { + // Checking the snapshot here rather than inside `resolve_export()` + // prevents a `semantic_index` read. Since the index is `no_eq` this + // would compromise the `File::resolve` firewall. + if !exports_so_far.contains(name.text(db).as_str()) { + return Vec::new(); + } + return file.resolve_export(db, name); + }, ImportLayer::Package(package) => *package, ImportLayer::From(importer) => { match importer.imported_from(db).get(name.text(db).as_str()) { @@ -280,3 +268,36 @@ fn resolve_import_layer<'db>( }; package.resolve(db, name, NamespaceVisibility::Exported) } + +/// Resolve `name` in every context of [`File::imports_by_sourcing_file`] and +/// union the results. +/// +/// Within one sourcing context, first hit wins as usual. On the other hand, +/// two files sourcing the same target do not mask each other, they provide +/// alternative contexts. Contexts converging on one binding (a shared sourced +/// file, a search-path package) dedupe. +/// +/// `Vec::contains()` preserves deterministic insertion order. The linear scan +/// is acceptable because each context yields few definitions. +fn resolve_per_sourcing_file<'db>( + db: &'db dyn Db, + contexts: &[Vec], + name: Name<'db>, +) -> Vec> { + let mut results: Vec> = Vec::new(); + for context in contexts { + for layer in context { + let defs = resolve_import_layer(db, layer, name); + if defs.is_empty() { + continue; + } + for def in defs { + if !results.contains(&def) { + results.push(def); + } + } + break; + } + } + results +} diff --git a/crates/oak_db/src/file_source_site.rs b/crates/oak_db/src/file_source_site.rs new file mode 100644 index 000000000..d1958f131 --- /dev/null +++ b/crates/oak_db/src/file_source_site.rs @@ -0,0 +1,138 @@ +use aether_path::FilePath; +use biome_rowan::TextSize; +use oak_semantic::semantic_index::SemanticCallKind; +use oak_semantic::ScopeId; +use rustc_hash::FxHashMap; + +use crate::Db; +use crate::File; + +/// A `source()` call (or more generally a `Source` effect), from the file it's +/// written in to the file it names. +/// +/// Both ends are carried so that a site is enough on its own to anchor a +/// diagnostic in the sourcing file. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SourceSite { + file: File, + target: Option, + path: String, + offset: TextSize, + scope: ScopeId, +} + +impl SourceSite { + /// The file the `source()` call is written in. + pub fn file(&self) -> File { + self.file + } + + /// The workspace file the sourced path resolved to. `None` when the path + /// didn't resolve, or resolved outside the workspace. + pub fn target(&self) -> Option { + self.target + } + + /// The sourced path as written in the `source()` call. + pub fn path(&self) -> &str { + &self.path + } + + pub fn offset(&self) -> TextSize { + self.offset + } + + pub fn scope(&self) -> ScopeId { + self.scope + } +} + +#[salsa::tracked] +impl File { + /// The `source()` calls written in this file, each naming an immediate + /// target. A file sourced by a file this one sources gets no entry. + /// + /// A call that didn't resolve is kept anyway, so a consumer can report the + /// path the user wrote. + /// + /// No `cycle_result`: `semantic_index` construction does not call this query. + #[salsa::tracked(returns(ref))] + pub fn source_sites(self, db: &dyn Db) -> Vec { + self.semantic_index(db) + .semantic_calls() + .iter() + .filter_map(|call| { + let SemanticCallKind::Source { path, resolved } = call.kind() else { + return None; + }; + let target = resolved + .as_ref() + .and_then(|url| db.file_by_path(&FilePath::from_url(url))); + Some(SourceSite { + file: self, + target, + path: path.clone(), + offset: call.offset(), + scope: call.scope(), + }) + }) + .collect() + } + + /// The workspace files `self` sources, sorted by path and deduplicated. + /// + /// Omits source-call offsets so text edits that preserve targets do not + /// invalidate [`sourcing_files_by_target`]. + #[salsa::tracked(returns(ref))] + pub(crate) fn source_targets(self, db: &dyn Db) -> Vec { + let mut targets: Vec = self + .source_sites(db) + .iter() + .filter_map(|site| site.target()) + .collect(); + + targets.sort_by_cached_key(|target| target.path(db).to_string()); + targets.dedup(); + targets + } + + /// The workspace files that source `self`. + /// + /// Sorted by path and deduped. This is a firewall query that deliberately + /// does not carry offsets. Use [`File::source_sites`] to get the call + /// positions. + /// + /// No `cycle_result` since `semantic_index` construction does not read this query. + #[salsa::tracked(returns(ref))] + pub fn sourced_by(self, db: &dyn Db) -> Vec { + sourcing_files_by_target(db) + .get(&self) + .cloned() + .unwrap_or_default() + } +} + +/// For each file in the workspace, the files that source it. +/// +/// Sorted by path and deduped. `workspace_files()`' iteration order isn't +/// a stable contract, and an unstable order here would prevent +/// [`File::sourced_by`] from backdating. +#[salsa::tracked(returns(ref))] +fn sourcing_files_by_target(db: &dyn Db) -> FxHashMap> { + let mut by_target: FxHashMap> = FxHashMap::default(); + + for &file in crate::workspace_files(db) { + for &target in file.source_targets(db) { + by_target.entry(target).or_default().push(file); + } + } + + // Deduplicate because `workspace_files()` can include a file through its root + // and package. + for files in by_target.values_mut() { + files.sort_by_cached_key(|file| file.path(db).to_string()); + files.dedup(); + } + + by_target +} diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index 05f53a638..30b3d0959 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -37,11 +37,10 @@ use crate::RootKind; /// /// Cycles in `source()` chains run through this resolver: /// `semantic_index(A)` reads `exports(B)`, which reads `semantic_index(B)`, -/// which reads `exports(A)`, which reads `semantic_index(A)`. Each of -/// `semantic_index()`, `exports()`, `imports()`, and `resolve()` carries its -/// own `cycle_result`. See [`File::semantic_index`]'s doc for the asymmetric -/// recovery behaviour (custom rebuild on `semantic_index()`, empty fallback -/// on the narrow queries). +/// which reads `exports(A)`, which reads `semantic_index(A)`. `semantic_index()` +/// and `exports()` carry the `cycle_result` handlers that break it. See +/// [`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, /// The file currently being indexed. @@ -216,6 +215,20 @@ impl<'db> SalsaImportsResolver<'db> { true => ControlFlow::Break(None), false => ControlFlow::Continue(()), }, + // Unreachable today: only `build_inherited_layers()` makes these, + // and it runs once the file's index exists, while we walk + // `cross_file_layers()` during the build. + ImportLayer::SourcingFile { + file, + exports_so_far, + } => { + let binds = + exports_so_far.contains(name) && file.exports(self.db).get(name).is_some(); + match binds { + true => ControlFlow::Break(None), + false => ControlFlow::Continue(()), + } + }, ImportLayer::Package(package) => match self.package_binding(*package, name) { PackageBinding::Effect(effects) => ControlFlow::Break(Some(effects)), PackageBinding::Shadow => ControlFlow::Break(None), diff --git a/crates/oak_db/src/lib.rs b/crates/oak_db/src/lib.rs index 947637660..0656def07 100644 --- a/crates/oak_db/src/lib.rs +++ b/crates/oak_db/src/lib.rs @@ -2,10 +2,12 @@ mod db; mod definition; mod diagnostic; mod file; +mod file_diagnostics; mod file_exports; mod file_imports; mod file_resolve; mod file_revision; +mod file_source_site; mod identifier; mod imports; mod inputs; @@ -35,6 +37,7 @@ pub use file_exports::ExportEntry; pub use file_exports::FileExports; pub use file_imports::ImportLayer; pub use file_revision::FileRevision; +pub use file_source_site::SourceSite; pub use identifier::Identifier; pub use identifier::MemberKind; pub use identifier::NamespaceVisibility; diff --git a/crates/oak_db/src/tests.rs b/crates/oak_db/src/tests.rs index 989946dc9..62377acf9 100644 --- a/crates/oak_db/src/tests.rs +++ b/crates/oak_db/src/tests.rs @@ -8,6 +8,7 @@ mod file_imports_at; mod file_resolve; mod file_resolve_at; mod file_root; +mod file_source_site; mod identifier; mod inputs; mod package_resolve; diff --git a/crates/oak_db/src/tests/file_diagnostics.rs b/crates/oak_db/src/tests/file_diagnostics.rs index ead811abd..cfac4b597 100644 --- a/crates/oak_db/src/tests/file_diagnostics.rs +++ b/crates/oak_db/src/tests/file_diagnostics.rs @@ -1,12 +1,21 @@ //! Snapshot tests for diagnostics rendering. Each test is one case, with a //! comment explaining whether it's correct-by-design or a known gap. +use oak_package_metadata::namespace::Import; +use oak_package_metadata::namespace::Namespace; +use salsa::Setter; +use stdext::SortedVec; + use crate::tests::diagnostic_render::render; use crate::tests::resolver::install_packages; 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::DbInputs; use crate::File; use crate::FileRevision; +use crate::Package; fn new_file(db: &TestDb, name: &str, contents: &str) -> File { File::new( @@ -318,3 +327,270 @@ fn test_diagnostic_uninstalled_package_unconditional() { insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); } + +#[test] +fn test_diagnostic_source_cycle() { + // `a.R` and `b.R` source each other, which R can't run. The recovery that + // breaks the salsa cycle is the only place that knows, so the diagnostic is + // raised there and anchored at the start of the file: under + // `NoopImportsResolver` a bare `source()` isn't recognized as effectful, so + // there's no recorded call to point at. + let mut db = TestDb::new(); + let a_source = "source(\"b.R\")\n"; + let (a, _b) = cyclic_pair(&mut db, a_source, "source(\"a.R\")\n"); + + insta::assert_snapshot!(render("w/a.R", a_source, a.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_source_cycle_reported_on_both_files() { + // Salsa's `FallbackImmediate` hands every participant its fallback, not just + // the one it re-entered, so both files are rebuilt degraded and both warn. + // That's what lets a file-local diagnostic cover a cycle it can't name: + // whichever file the user has open carries its own copy. + let mut db = TestDb::new(); + let b_source = "source(\"a.R\")\n"; + let (a, b) = cyclic_pair(&mut db, "source(\"b.R\")\n", b_source); + + assert_eq!(a.diagnostics(&db).len(), 1); + insta::assert_snapshot!(render("w/b.R", b_source, b.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_inherited_shadow() { + // `a.R` binds `source` and then sources `b.R` through `base::source`, which + // no binding can shadow. Inside `b.R`, its own bare `source("c.R")` was + // analysed as base `source` (the scan knows nothing about source sites), but + // `b.R`'s `imports()` now reaches `a.R`'s `source <- identity` through the + // inherited band. That's the one call the two disagree about, so we say so + // instead of silently picking a side. + let mut db = TestDb::new(); + let root = workspace_root(&db, "w"); + let a = new_file(&db, "w/a.R", "source <- identity\nbase::source(\"b.R\")\n"); + let b_source = "foo <- 1\nsource(\"c.R\")\n"; + let b = new_file(&db, "w/b.R", b_source); + let c = new_file(&db, "w/c.R", "foo\n"); + root.set_scripts(&mut db).to(vec![a, b, c]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + insta::assert_snapshot!(render("w/b.R", b_source, b.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_no_inherited_shadow_for_ordinary_sourcing() { + // The check must not fire just because the inherited search path reaches + // `base`, which really does bind `source`. Both views settle on the same + // `Package(base)` layer, so there's no disagreement to report. + // + // `install_package_binding` rather than `install_packages`: the latter + // registers a package with no files, so no `Package` layer binds anything + // and the case this test is about wouldn't arise at all. + let mut db = TestDb::new(); + install_package_binding(&mut db, "base", &["source"]); + let root = workspace_root(&db, "w"); + let main = new_file(&db, "w/main.R", "source(\"helpers.R\")\n"); + let helpers_source = "source(\"more.R\")\n"; + let helpers = new_file(&db, "w/helpers.R", helpers_source); + let more = new_file(&db, "w/more.R", "x <- 1\n"); + root.set_scripts(&mut db).to(vec![main, helpers, more]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + insta::assert_snapshot!(render( + "w/helpers.R", + helpers_source, + helpers.diagnostics(&db) + )); +} + +#[test] +fn test_diagnostic_no_inherited_shadow_when_file_binds_the_name_itself() { + // `helpers.R` binds `source` itself, after the call, so the scan does treat + // the call as effectful. But `imports()` resolves `source` to that own + // binding rather than to the inherited one, so the inherited layer isn't what + // makes this call uncertain. Authored shadowing is `EffectAmbiguity`'s job. + let mut db = TestDb::new(); + let root = workspace_root(&db, "w"); + let main = new_file( + &db, + "w/main.R", + "source <- identity\nbase::source(\"helpers.R\")\n", + ); + let helpers_source = "source(\"more.R\")\nsource <- identity\n"; + let helpers = new_file(&db, "w/helpers.R", helpers_source); + let more = new_file(&db, "w/more.R", "x <- 1\n"); + root.set_scripts(&mut db).to(vec![main, helpers, more]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + insta::assert_snapshot!(render( + "w/helpers.R", + helpers_source, + helpers.diagnostics(&db) + )); +} + +#[test] +fn test_diagnostic_inherited_attach_shadows_a_callee() { + // `main.R` attaches a package exporting its own `library`, and that attach + // reaches `helpers.R` only through inheritance. So `helpers.R`'s scan + // resolved its `library(dplyr)` callee to base `library`, while `imports()` + // resolves it to `shadowr::library`. This is the case a check keyed on `File` + // layers alone misses, and attach ordering is most of what inheritance + // contributes. + // + // `shadowr` deliberately shadows `library` rather than `source`: shadowing + // `source` would also shadow `main.R`'s own `source("helpers.R")` call, which + // would stop being effectful and take the whole inheritance edge with it. + let mut db = TestDb::new(); + install_package_binding(&mut db, "base", &["source", "library"]); + install_package_binding(&mut db, "shadowr", &["library"]); + install_package_binding(&mut db, "dplyr", &[]); + let root = workspace_root(&db, "w"); + let main = new_file(&db, "w/main.R", "library(shadowr)\nsource(\"helpers.R\")\n"); + let helpers_source = "library(dplyr)\n"; + let helpers = new_file(&db, "w/helpers.R", helpers_source); + root.set_scripts(&mut db).to(vec![main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + insta::assert_snapshot!(render( + "w/helpers.R", + helpers_source, + helpers.diagnostics(&db) + )); +} + +#[test] +fn test_diagnostic_inherited_namespace_import_shadows_a_callee() { + // Same shape as `test_diagnostic_inherited_attach_shadows_a_callee`, but the + // sourcing side reaches `library` through `mypkg`'s NAMESPACE + // (`importFrom(shadowr, library)`) rather than through a `library()` call, + // pinning the `ImportLayer::From` arm of `describe_source`. + // + // `mypkg` imports `library` rather than `source` deliberately. Shadowing + // `source` would stop `main.R`'s own `source("helpers.R")` from being + // effectful and take the inheritance edge with it. + let mut db = TestDb::new(); + install_package_binding(&mut db, "base", &["source", "library"]); + install_package_binding(&mut db, "shadowr", &["library"]); + install_package_binding(&mut db, "dplyr", &[]); + + let root = workspace_root(&db, "w/mypkg"); + let namespace = Namespace { + imports: vec![Import { + name: "library".to_string(), + package: "shadowr".to_string(), + }], + ..Default::default() + }; + let pkg = Package::new( + &db, + file_path("w/mypkg/DESCRIPTION"), + "mypkg".to_string(), + FileRevision::zero(), + FileRevision::zero(), + None, + Some(namespace), + Vec::new(), + Vec::new(), + ); + let main = File::new( + &db, + file_path("w/mypkg/R/main.R"), + FileRevision::zero(), + Some("source(\"helpers.R\")\n".to_string()), + Some(pkg), + ); + pkg.set_files(&mut db).to(vec![main]); + root.set_packages(&mut db).to(vec![pkg]); + + let helpers_source = "library(dplyr)\n"; + let helpers = new_file(&db, "w/mypkg/helpers.R", helpers_source); + root.set_scripts(&mut db).to(vec![helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + // Fails fast if `source()` path anchoring changes, since the whole fixture + // rests on this edge existing. + assert_eq!(helpers.sourced_by(&db).as_slice(), [main]); + + insta::assert_snapshot!(render( + "w/mypkg/helpers.R", + helpers_source, + helpers.diagnostics(&db) + )); +} + +#[test] +fn test_diagnostic_no_inherited_shadow_for_own_attach_ordering() { + // `helpers.R`'s `library(dplyr)` runs inside `local()`, before its own + // top-level `library(shadowr)`, so the scan resolved that callee to base + // `library` while the end-of-file read view resolves it to + // `shadowr::library`. A real disagreement, but caused by `helpers.R`'s own + // ordering, not by anything it inherits, so blaming the source site would be + // wrong. Authored shadowing belongs to `EffectAmbiguity`. + let mut db = TestDb::new(); + install_package_binding(&mut db, "base", &["source", "library"]); + install_package_binding(&mut db, "shadowr", &["library"]); + install_package_binding(&mut db, "dplyr", &[]); + let root = workspace_root(&db, "w"); + let main = new_file(&db, "w/main.R", "source(\"helpers.R\")\n"); + let helpers_source = "local({\n library(dplyr)\n})\nlibrary(shadowr)\n"; + let helpers = new_file(&db, "w/helpers.R", helpers_source); + root.set_scripts(&mut db).to(vec![main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + insta::assert_snapshot!(render( + "w/helpers.R", + helpers_source, + helpers.diagnostics(&db) + )); +} + +/// Register an installed package that really binds and exports `symbols`, so +/// `Package::resolve` finds them. `install_packages` registers packages with no +/// files, which makes every `Package` layer inert. Additive across calls, so +/// several fixtures can coexist. +fn install_package_binding(db: &mut TestDb, name: &str, symbols: &[&str]) { + let root = library_root(db, &format!("libs/{name}")); + let namespace = Namespace { + exports: SortedVec::from_vec(symbols.iter().map(|s| s.to_string()).collect()), + ..Default::default() + }; + let pkg = Package::new( + db, + file_path(&format!("libs/{name}/DESCRIPTION")), + name.to_string(), + FileRevision::zero(), + FileRevision::zero(), + None, + Some(namespace), + Vec::new(), + Vec::new(), + ); + let contents: String = symbols + .iter() + .map(|symbol| format!("{symbol} <- function(...) NULL\n")) + .collect(); + let file = File::new( + db, + file_path(&format!("libs/{name}/R/exports.R")), + FileRevision::zero(), + Some(contents), + Some(pkg), + ); + pkg.set_files(db).to(vec![file]); + root.set_packages(db).to(vec![pkg]); + + let mut roots = db.library_roots().roots(db).clone(); + roots.push(root); + db.library_roots().set_roots(db).to(roots); +} + +/// Two workspace scripts at `w/a.R` and `w/b.R`, whose contents are expected to +/// `source()` each other. +fn cyclic_pair(db: &mut TestDb, a_source: &str, b_source: &str) -> (File, File) { + let root = workspace_root(db, "w"); + let a = new_file(db, "w/a.R", a_source); + let b = new_file(db, "w/b.R", b_source); + root.set_scripts(db).to(vec![a, b]); + db.workspace_roots().set_roots(db).to(vec![root]); + (a, b) +} diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index 2429fab79..815de22b7 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -1,7 +1,9 @@ +use biome_rowan::TextSize; use oak_package_metadata::namespace::Import; use oak_package_metadata::namespace::Namespace; use salsa::Setter; +use crate::file_imports::CollationView; use crate::tests::test_db::file_path; use crate::tests::test_db::library_root; use crate::tests::test_db::make_package; @@ -186,8 +188,8 @@ fn test_package_file_emits_namespace_and_collation_layers() { ImportLayer::Package(p) => { shape.push(format!("Package({})", p.name(&db))); }, - ImportLayer::File(f) => { - let url = f.path(&db).to_url(); + ImportLayer::File(file) | ImportLayer::SourcingFile { file, .. } => { + let url = file.path(&db).to_url(); shape.push(format!( "File({})", url.path().rsplit('/').next().unwrap_or("?") @@ -540,8 +542,8 @@ fn shape(db: &TestDb, layers: &[ImportLayer]) -> Vec { format!("From({entries:?})") }, ImportLayer::Package(p) => format!("Package({})", p.name(db)), - ImportLayer::File(f) => { - let url = f.path(db).to_url(); + ImportLayer::File(file) | ImportLayer::SourcingFile { file, .. } => { + let url = file.path(db).to_url(); format!("File({})", url.path().rsplit('/').next().unwrap_or("?")) }, }) @@ -846,3 +848,641 @@ fn test_cross_file_layers_backdates_on_unrelated_script_change() { let _ = a.imports(&db); assert_eq!(db.executions("cross_file_layers"), 1); } + +#[test] +fn test_sourced_file_inherits_sourcing_files_imports() { + let mut db = TestDb::new(); + let root = workspace_root(&db, "w"); + let main = File::new( + &db, + file_path("w/main.R"), + FileRevision::zero(), + Some("x <- 1\nsource(\"helpers.R\")\n".to_string()), + None, + ); + let helpers = File::new( + &db, + file_path("w/helpers.R"), + FileRevision::zero(), + Some("y <- 2\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + assert_eq!(shape(&db, helpers.imports(&db)), vec![ + "File(main.R)".to_string() + ]); +} + +#[test] +fn test_inherited_attach_sits_below_sourced_files_own_attaches() { + let mut db = TestDb::new(); + install_packages(&mut db, &["dplyr", "tibble"]); + let root = workspace_root(&db, "w"); + let main = File::new( + &db, + file_path("w/main.R"), + FileRevision::zero(), + Some("library(dplyr)\nsource(\"helpers.R\")\n".to_string()), + None, + ); + let helpers = File::new( + &db, + file_path("w/helpers.R"), + FileRevision::zero(), + Some("library(tibble)\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + // `helpers.R`'s own attach (tibble) outranks the inherited one (dplyr), + // same as a collation predecessor's attach would. + // + // Tibble appears twice because `source()` forwards a sourced file's own + // top-level attaches into the sourcing file's index, and that copy is + // indistinguishable from a `library()` written in `main.R`. The echo is + // unreachable under `resolve()`'s first-hit-wins search. + assert_eq!(shape(&db, helpers.imports(&db)), vec![ + "File(main.R)".to_string(), + "Package(tibble)".to_string(), + "Package(tibble)".to_string(), + "Package(dplyr)".to_string(), + ]); +} + +#[test] +fn test_inherited_imports_are_transitive() { + let mut db = TestDb::new(); + let root = workspace_root(&db, "w"); + let main = File::new( + &db, + file_path("w/main.R"), + FileRevision::zero(), + Some("source(\"setup.R\")\n".to_string()), + None, + ); + let setup = File::new( + &db, + file_path("w/setup.R"), + FileRevision::zero(), + Some("source(\"helpers.R\")\n".to_string()), + None, + ); + let helpers = File::new( + &db, + file_path("w/helpers.R"), + FileRevision::zero(), + Some("z <- 1\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, setup, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + // `setup.R` outranks `main.R`: it's the more immediate source site. + assert_eq!(shape(&db, helpers.imports(&db)), vec![ + "File(setup.R)".to_string(), + "File(main.R)".to_string(), + ]); +} + +#[test] +fn test_multiple_sourcing_files_appear_ordered_by_path() { + let mut db = TestDb::new(); + let root = workspace_root(&db, "w"); + let a_main = File::new( + &db, + file_path("w/a_main.R"), + FileRevision::zero(), + Some("source(\"helpers.R\")\n".to_string()), + None, + ); + let b_main = File::new( + &db, + file_path("w/b_main.R"), + FileRevision::zero(), + Some("source(\"helpers.R\")\n".to_string()), + None, + ); + let helpers = File::new( + &db, + file_path("w/helpers.R"), + FileRevision::zero(), + Some("z <- 1\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a_main, b_main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + assert_eq!(shape(&db, helpers.imports(&db)), vec![ + "File(a_main.R)".to_string(), + "File(b_main.R)".to_string(), + ]); +} + +#[test] +fn test_inheritance_replaces_collation_instead_of_adding_to_it() { + // `a.R` and `b.R` would collate together as a non-package `R/` directory + // (see `test_script_r_directory_siblings_see_each_other`). Once `main.R` + // explicitly sources `a.R`, `b.R` may never load, so it must drop out of + // `a.R`'s imports entirely rather than sit alongside the inherited band. + let mut db = TestDb::new(); + let root = workspace_root(&db, "ws"); + let a = File::new( + &db, + file_path("ws/R/a.R"), + FileRevision::zero(), + Some("a_val <- 1\n".to_string()), + None, + ); + let b = File::new( + &db, + file_path("ws/R/b.R"), + FileRevision::zero(), + Some("b_val <- 2\n".to_string()), + None, + ); + let main = File::new( + &db, + file_path("ws/main.R"), + FileRevision::zero(), + Some("source(\"R/a.R\")\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b, main]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + assert_eq!(shape(&db, a.imports(&db)), vec!["File(main.R)".to_string()]); +} + +#[test] +fn test_file_nobody_sources_keeps_its_own_cross_file_layers() { + let mut db = TestDb::new(); + install_packages(&mut db, &["dplyr", "base"]); + let root = workspace_root(&db, "ws"); + let a = File::new( + &db, + file_path("ws/R/a.R"), + FileRevision::zero(), + Some("library(dplyr)\n".to_string()), + None, + ); + let b = File::new( + &db, + file_path("ws/R/b.R"), + FileRevision::zero(), + Some("x <- 1\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + // Nobody sources `b.R`, so it keeps exactly the collation view + // `cross_file_layers` alone would give it. + assert_eq!(shape(&db, b.imports(&db)), vec![ + "File(a.R)".to_string(), + "Package(dplyr)".to_string(), + "Package(base)".to_string(), + ]); +} + +#[test] +fn test_cross_file_layers_never_carries_inherited_layers() { + // The scan-time resolver walks `cross_file_layers` while a file's own index + // is still being built, so an inherited layer there would make every file's + // index demand the workspace reverse map. `resolution_layers` splices + // inheritance in afterwards instead, which is why the read side below sees + // `main.R` and `cross_file_layers` doesn't. + // + // This is what makes the `SourcingFile` arm of `layer_effect` unreachable. + let mut db = TestDb::new(); + install_packages(&mut db, &["base"]); + let root = workspace_root(&db, "w"); + + let main = File::new( + &db, + file_path("w/main.R"), + FileRevision::zero(), + Some("cfg <- 1\nsource(\"R/helpers.R\")\n".to_string()), + None, + ); + // Collates before `helpers.R`, so the scan side has a real `File` layer to + // tell apart from a `SourcingFile` one. + let sibling = File::new( + &db, + file_path("w/R/a_sib.R"), + FileRevision::zero(), + Some("sib <- 1\n".to_string()), + None, + ); + let helpers_source = "top <- 2\n"; + let helpers = File::new( + &db, + file_path("w/R/helpers.R"), + FileRevision::zero(), + Some(helpers_source.to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, sibling, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + // The read side narrows `main.R` to what had run by its `source()` call. + let offset = TextSize::from(helpers_source.find("top").unwrap() as u32); + assert!(helpers + .imports_at(&db, offset) + .iter() + .any(|layer| { matches!(layer, ImportLayer::SourcingFile { file, .. } if *file == main) })); + + // The scan side has `File` layers but never a `SourcingFile`, either view. + for view in [CollationView::Eager, CollationView::Lazy] { + let scan_side = helpers.cross_file_layers(&db, view); + assert!(scan_side + .lookup_order(&[]) + .any(|layer| matches!(layer, ImportLayer::File(file) if *file == sibling))); + assert!(!scan_side + .lookup_order(&[]) + .any(|layer| matches!(layer, ImportLayer::SourcingFile { .. }))); + } +} + +#[test] +fn test_inherited_default_search_path_is_not_duplicated() { + let mut db = TestDb::new(); + install_packages(&mut db, &["base"]); + let root = workspace_root(&db, "w"); + let main = File::new( + &db, + file_path("w/main.R"), + FileRevision::zero(), + Some("source(\"helpers.R\")\n".to_string()), + None, + ); + let helpers = File::new( + &db, + file_path("w/helpers.R"), + FileRevision::zero(), + Some("z <- 1\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + // `base` comes once from `main.R`'s own `below` band, never duplicated by + // `helpers.R`'s own (replaced) `cross_file_layers`. + assert_eq!(shape(&db, helpers.imports(&db)), vec![ + "File(main.R)".to_string(), + "Package(base)".to_string(), + ]); +} + +#[test] +fn test_mutual_sourcing_devolves_to_standalone_scripts() { + // A mutual pair cycles `semantic_index` through `exports`, and the cycling + // side is rebuilt with `NoopImportsResolver`, whose `resolve_effects` + // defaults to `None`. So a bare `source()` isn't recognized as effectful on + // either side and no `SourceSite` survives. The pair never reaches + // `inherited_layers`' own `cycle_result`, and both files fall back to the + // standalone-script context. + let mut db = TestDb::new(); + install_packages(&mut db, &["base"]); + let root = workspace_root(&db, "w"); + let a = File::new( + &db, + file_path("w/a.R"), + FileRevision::zero(), + Some("source(\"b.R\")\n".to_string()), + None, + ); + let b = File::new( + &db, + file_path("w/b.R"), + FileRevision::zero(), + Some("source(\"a.R\")\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + assert_eq!(a.source_sites(&db), &Vec::new()); + assert_eq!(b.source_sites(&db), &Vec::new()); + assert_eq!(a.sourced_by(&db), &Vec::::new()); + assert_eq!(b.sourced_by(&db), &Vec::::new()); + assert_eq!( + shape(&db, a.imports(&db)), + vec!["Package(base)".to_string()] + ); + assert_eq!( + shape(&db, b.imports(&db)), + vec!["Package(base)".to_string()] + ); +} + +#[test] +fn test_qualified_mutual_sourcing_records_sites_but_no_edges() { + // `base::source()` stays recognized under `NoopImportsResolver`, whose + // `resolve_qualified_effects` defaults to `effects::lookup`, so unlike the + // bare-call pair above both sites survive in the forward projection. They + // resolve to nothing, though: `resolve_source` reads the target's + // `exports`, which is the empty cycle fallback. No target means no reverse + // edge, so inheritance sees a cyclic pair as two standalone scripts. + // + // This is what makes `inherited_layers`' own `cycle_result` unreachable + // rather than load-bearing. Every resolved source site is also a + // `semantic_index -> exports -> semantic_index` edge in the same direction, + // so a cycle in source edges always wipes the very targets that would let + // `inherited_layers` recurse into itself. + let mut db = TestDb::new(); + install_packages(&mut db, &["base"]); + let root = workspace_root(&db, "w"); + let a = File::new( + &db, + file_path("w/a.R"), + FileRevision::zero(), + Some("base::source(\"b.R\")\n".to_string()), + None, + ); + let b = File::new( + &db, + file_path("w/b.R"), + FileRevision::zero(), + Some("base::source(\"a.R\")\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + assert_eq!(a.source_sites(&db).len(), 1); + assert_eq!(b.source_sites(&db).len(), 1); + assert_eq!(a.source_sites(&db)[0].target(), None); + assert_eq!(b.source_sites(&db)[0].target(), None); + + assert_eq!(a.sourced_by(&db), &Vec::::new()); + assert_eq!(b.sourced_by(&db), &Vec::::new()); + assert_eq!( + shape(&db, a.imports(&db)), + vec!["Package(base)".to_string()] + ); + assert_eq!( + shape(&db, b.imports(&db)), + vec!["Package(base)".to_string()] + ); +} + +#[test] +fn test_package_r_file_ignores_source_sites() { + let mut db = TestDb::new(); + install_packages(&mut db, &["base"]); + + let workspace = workspace_root(&db, "w"); + let pkg = Package::new( + &db, + file_path("w/pkg/DESCRIPTION"), + "pkg".to_string(), + FileRevision::zero(), + FileRevision::zero(), + None, + None, + Vec::new(), + Vec::new(), + ); + let a = File::new( + &db, + file_path("w/pkg/R/a.R"), + FileRevision::zero(), + Some("a_val <- 1\n".to_string()), + Some(pkg), + ); + let b = File::new( + &db, + file_path("w/pkg/R/b.R"), + FileRevision::zero(), + Some("b_val <- 2\n".to_string()), + Some(pkg), + ); + let dev = File::new( + &db, + file_path("w/pkg/data-raw/dev.R"), + FileRevision::zero(), + Some("source(\"pkg/R/b.R\")\n".to_string()), + Some(pkg), + ); + pkg.set_files(&mut db).to(vec![a, b]); + pkg.set_scripts(&mut db).to(vec![dev]); + workspace.set_packages(&mut db).to(vec![pkg]); + db.workspace_roots().set_roots(&mut db).to(vec![workspace]); + + // `dev.R` really does source `b.R`, but `Collate:` already says when `b.R` + // loads, so it keeps its predecessor and NAMESPACE context. Inheriting + // `dev.R`'s instead would drop `File(a.R)`. + assert_eq!(b.sourced_by(&db), &vec![dev]); + assert_eq!(shape(&db, b.imports(&db)), vec![ + "File(a.R)".to_string(), + "Package(base)".to_string(), + ]); +} + +#[test] +fn test_testthat_file_ignores_source_sites() { + let mut db = TestDb::new(); + install_packages(&mut db, &["testthat", "base"]); + + let workspace = workspace_root(&db, "w"); + let pkg = Package::new( + &db, + file_path("w/pkg/DESCRIPTION"), + "pkg".to_string(), + FileRevision::zero(), + FileRevision::zero(), + None, + None, + Vec::new(), + Vec::new(), + ); + let r_file = File::new( + &db, + file_path("w/pkg/R/a.R"), + FileRevision::zero(), + Some("f <- 1\n".to_string()), + Some(pkg), + ); + let helper = File::new( + &db, + file_path("w/pkg/tests/testthat/helper-b.R"), + FileRevision::zero(), + Some("h <- 1\n".to_string()), + Some(pkg), + ); + let test_foo = File::new( + &db, + file_path("w/pkg/tests/testthat/test-foo.R"), + FileRevision::zero(), + Some("source(\"pkg/tests/testthat/helper-b.R\")\n".to_string()), + Some(pkg), + ); + pkg.set_files(&mut db).to(vec![r_file]); + pkg.set_scripts(&mut db).to(vec![helper, test_foo]); + workspace.set_packages(&mut db).to(vec![pkg]); + db.workspace_roots().set_roots(&mut db).to(vec![workspace]); + + // testthat sources helpers itself, before any test file runs, so an + // explicit `source()` in a test file doesn't change what the helper sees. + assert_eq!(helper.sourced_by(&db), &vec![test_foo]); + assert_eq!(shape(&db, helper.imports(&db)), vec![ + "File(a.R)".to_string(), + "Package(testthat)".to_string(), + "Package(base)".to_string(), + ]); +} + +#[test] +fn test_non_collated_package_file_still_inherits() { + let mut db = TestDb::new(); + install_packages(&mut db, &["base"]); + + let workspace = workspace_root(&db, "w"); + let pkg = Package::new( + &db, + file_path("w/pkg/DESCRIPTION"), + "pkg".to_string(), + FileRevision::zero(), + FileRevision::zero(), + None, + None, + Vec::new(), + Vec::new(), + ); + let r_file = File::new( + &db, + file_path("w/pkg/R/a.R"), + FileRevision::zero(), + Some("f <- 1\n".to_string()), + Some(pkg), + ); + let helpers = File::new( + &db, + file_path("w/pkg/data-raw/helpers.R"), + FileRevision::zero(), + Some("h <- 1\n".to_string()), + Some(pkg), + ); + let dev = File::new( + &db, + file_path("w/pkg/data-raw/dev.R"), + FileRevision::zero(), + Some("source(\"pkg/data-raw/helpers.R\")\n".to_string()), + Some(pkg), + ); + pkg.set_files(&mut db).to(vec![r_file]); + pkg.set_scripts(&mut db).to(vec![helpers, dev]); + workspace.set_packages(&mut db).to(vec![pkg]); + db.workspace_roots().set_roots(&mut db).to(vec![workspace]); + + // The gate is about load order, not about carrying a package back-pointer. + // A `data-raw/` script isn't loaded with the package, so a source site is + // the only thing that says anything about its environment. + assert_eq!(shape(&db, helpers.imports(&db)), vec![ + "File(dev.R)".to_string(), + "Package(base)".to_string(), + ]); +} + +#[test] +fn test_shadowed_source_call_still_contributes_an_edge() { + // ```r + // # a.R # b.R # c.R + // source <- identity foo <- 1 foo + // base::source("b.R") source("c.R") + // ``` + // + // Entered through `a.R`, `b.R`'s `source("c.R")` calls `identity` and `c.R` + // never loads. But R declares no entry point, and `b.R` run on its own + // really does source `c.R`. Each file's effects resolve in its standalone + // context, the one context that assumes nothing about callers, so the edge + // goes in the map and `c.R` inherits from both files up the chain. + let mut db = TestDb::new(); + let root = workspace_root(&db, "w"); + let a = File::new( + &db, + file_path("w/a.R"), + FileRevision::zero(), + Some("source <- identity\nbase::source(\"b.R\")\n".to_string()), + None, + ); + let b = File::new( + &db, + file_path("w/b.R"), + FileRevision::zero(), + Some("foo <- 1\nsource(\"c.R\")\n".to_string()), + None, + ); + let c = File::new( + &db, + file_path("w/c.R"), + FileRevision::zero(), + Some("foo\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b, c]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + assert_eq!(b.sourced_by(&db), &vec![a]); + assert_eq!(c.sourced_by(&db), &vec![b]); + + // `b.R` outranks `a.R`, being the more immediate source site. No packages + // are installed, so nothing follows the two `File` layers. + assert_eq!(shape(&db, c.imports(&db)), vec![ + "File(b.R)".to_string(), + "File(a.R)".to_string(), + ]); +} + +#[test] +fn test_body_edit_in_a_sourcing_file_does_not_invalidate_imports() { + // Inheritance reads the sourcing file's `attach_layers`, hence its `no_eq` + // `semantic_index`, so `inherited_layers` re-executes on any keystroke in + // `main.R`. The firewall is one level up: an edit that changes no attach and + // no source edge returns a value-equal `Vec`, so `helpers.R`'s `imports` + // backdates and everything downstream of it stays green. + 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("library(dplyr)\nsource(\"helpers.R\")\nf <- function() 1\n".to_string()), + None, + ); + let helpers = File::new( + &db, + file_path("w/helpers.R"), + FileRevision::zero(), + Some("y <- 2\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + let before = shape(&db, helpers.imports(&db)); + assert_eq!(before, vec![ + "File(main.R)".to_string(), + "Package(dplyr)".to_string(), + ]); + assert_eq!(db.executions("File::imports"), 1); + // One per file in the chain: `helpers.R`'s, and `main.R`'s own (empty) + // inheritance, which `build_inherited_layers` reads for transitivity. + assert_eq!(db.executions("File::inherited_layers"), 2); + + // Rewrite `f`'s body, leaving the `library()` call and the `source()` call + // untouched. + main.set_source_text_override(&mut db).to(Some( + "library(dplyr)\nsource(\"helpers.R\")\nf <- function() 2 + 2\n".to_string(), + )); + + assert_eq!(shape(&db, helpers.imports(&db)), before); + // `helpers.R`'s re-executed and backdated. `main.R`'s didn't: it reads only + // `sourced_by(main)`, which the edit left alone. + assert_eq!(db.executions("File::inherited_layers"), 3); + assert_eq!(db.executions("File::imports"), 1); +} diff --git a/crates/oak_db/src/tests/file_imports_at.rs b/crates/oak_db/src/tests/file_imports_at.rs index 5833dffbc..07cc897b9 100644 --- a/crates/oak_db/src/tests/file_imports_at.rs +++ b/crates/oak_db/src/tests/file_imports_at.rs @@ -103,12 +103,37 @@ fn package_files(layers: &[ImportLayer]) -> Vec { layers .iter() .filter_map(|layer| match layer { - ImportLayer::File(f) => Some(*f), + ImportLayer::File(file) | ImportLayer::SourcingFile { file, .. } => Some(*file), _ => None, }) .collect() } +/// Layer identities, sorted and deduped, for comparing two views that cover the +/// same layers in different orders. +fn layer_keys(db: &TestDb, layers: &[ImportLayer]) -> Vec { + let mut keys: Vec = layers.iter().map(|layer| layer_key(db, layer)).collect(); + keys.sort(); + keys.dedup(); + keys +} + +fn layer_key(db: &TestDb, layer: &ImportLayer) -> String { + match layer { + ImportLayer::File(file) => format!("File({})", file.path(db)), + ImportLayer::SourcingFile { + file, + exports_so_far, + } => { + let mut names: Vec<&str> = exports_so_far.names().collect(); + names.sort(); + format!("SourcingFile({}, {names:?})", file.path(db)) + }, + ImportLayer::Package(package) => format!("Package({})", package.name(db)), + ImportLayer::From(package) => format!("From({})", package.name(db)), + } +} + #[test] fn test_script_cursor_before_any_attach_sees_no_attached_packages() { let mut db = TestDb::new(); @@ -295,6 +320,31 @@ fn test_testthat_top_level_library_narrows_by_offset() { assert!(library_attaches(&db, &after).contains(&"cli".to_string())); } +#[test] +fn test_edit_above_a_source_call_backdates_the_eager_inherited_view() { + // An offset-only edit leaves `mid.R`'s inherited layers equal, so Salsa + // backdates them without re-resolving `leaf.R`. + let mut db = TestDb::new(); + let root = workspace_root(&db, "w"); + let main = make_file(&mut db, "w/main.R", "a_val <- 1\nsource(\"mid.R\")\n"); + let mid = make_file(&mut db, "w/mid.R", "source(\"leaf.R\")\n"); + let leaf = make_file(&mut db, "w/leaf.R", "use <- a_val\n"); + root.set_scripts(&mut db).to(vec![main, mid, leaf]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + let at_use = TextSize::from(7); + let before = layer_keys(&db, &leaf.imports_at(&db, at_use)); + let executions = db.executions("File::inherited_layers"); + + main.set_source_text_override(&mut db).to(Some( + "# a comment\na_val <- 1\nsource(\"mid.R\")\n".to_string(), + )); + + assert_eq!(layer_keys(&db, &leaf.imports_at(&db, at_use)), before); + + assert_eq!(db.executions("File::inherited_layers"), executions + 1); +} + /// Creates a `tests/testthat/` fixture with three support files and one test. /// Returns `(helper_a, helper_b, setup_c, test_x)`. fn testthat_support_workspace(db: &mut TestDb, helper_b: &str) -> (File, File, File, File) { @@ -847,3 +897,125 @@ fn test_script_r_directory_unplaced_file_still_sees_only_predecessors() { let offset = TextSize::from(b_source.find('x').unwrap() as u32); assert_eq!(package_files(&b.imports_at(&db, offset)), vec![a]); } + +#[test] +fn test_inherited_attach_is_offset_sensitive_via_source_call_position() { + // `main.R`'s `library(dplyr)` runs after its `source()` call, so a + // top-level cursor in `helpers.R` (Eager: attaches up to the source-call + // offset) doesn't see it, while a cursor in a function body (Lazy: + // end-of-file view) does. + let mut db = TestDb::new(); + install_packages(&mut db, &["dplyr"]); + let root = workspace_root(&db, "w"); + + let main_source = "source(\"helpers.R\")\nlibrary(dplyr)\n"; + let main = make_file(&mut db, "w/main.R", main_source); + + let helpers_source = "top <- 1\nf <- function() {\n body_stmt\n}\n"; + let helpers = make_file(&mut db, "w/helpers.R", helpers_source); + + root.set_scripts(&mut db).to(vec![main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + let top_offset = TextSize::from(helpers_source.find("top").unwrap() as u32); + let top_layers = helpers.imports_at(&db, top_offset); + assert!(!library_attaches(&db, &top_layers).contains(&"dplyr".to_string())); + + let body_offset = TextSize::from(helpers_source.find("body_stmt").unwrap() as u32); + let body_layers = helpers.imports_at(&db, body_offset); + assert!(library_attaches(&db, &body_layers).contains(&"dplyr".to_string())); +} + +#[test] +fn test_attach_in_eager_scope_is_visible_to_later_top_level_code() { + // `library()` attaches to the global search path whatever frame it runs in, + // so a call inside `local()` counts for code after the block exactly like a + // top-level one would. It's invisible before the block, though, same + // narrowing as any other attach. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "before\nlocal({\n library(cli)\n})\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + let before = TextSize::from(source.find("before").unwrap() as u32); + assert!(!library_attaches(&db, &file.imports_at(&db, before)).contains(&"cli".to_string())); + + let after = TextSize::from(source.find("after").unwrap() as u32); + assert!(library_attaches(&db, &file.imports_at(&db, after)).contains(&"cli".to_string())); +} + +#[test] +fn test_imports_at_covers_the_same_layers_as_the_per_sourcing_file_view() { + // Both views narrow to `offset` by the same rule and cover the same layers, + // grouped differently. `imports_at` is band-major, every sourcing file's + // `above` band, then own attaches, then every sourcing file's `below` band. + // The per-sourcing-file view is context-major, one file's `above` / own / + // `below` in full before the next file's. So the comparison below is over + // layer sets, not order. + // + // Nothing in production reads `imports_at` today (`resolve_at` moved to the + // per-sourcing-file view, completions will want the flat one), so this pins + // the two together against drift in the narrowing. + let mut db = TestDb::new(); + install_packages(&mut db, &["base", "dplyr", "rlang"]); + let root = workspace_root(&db, "w"); + + // `a.R` attaches a package and `b.R` doesn't, so the two contexts differ + // and band-major genuinely reorders against context-major. `helpers.R`'s + // own attach sits after the cursor, so an own-attach view that stopped + // narrowing would show up as an extra `rlang` layer on one side. + let a = make_file(&mut db, "w/a.R", "library(dplyr)\nsource(\"helpers.R\")\n"); + let b = make_file(&mut db, "w/b.R", "foo <- 1\nsource(\"helpers.R\")\n"); + let helpers = make_file(&mut db, "w/helpers.R", "foo\nlibrary(rlang)\n"); + root.set_scripts(&mut db).to(vec![a, b, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + let offset = TextSize::from(0); + let contexts = helpers.imports_by_sourcing_file_at(&db, offset); + assert_eq!(contexts.len(), 2); + + let flat = helpers.imports_at(&db, offset); + let grouped: Vec = contexts.into_iter().flatten().collect(); + assert_eq!(layer_keys(&db, &flat), layer_keys(&db, &grouped)); +} + +#[test] +fn test_attach_under_a_lazy_ancestor_stays_invisible() { + // The `local()` here is eager, but it sits in a function body, and nothing + // says that function was ever called. So the whole chain out to the file + // scope has to be eager, not just the attach's own scope. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "f <- function() {\n local({\n library(cli)\n })\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + let after = TextSize::from(source.find("after").unwrap() as u32); + assert!(!library_attaches(&db, &file.imports_at(&db, after)).contains(&"cli".to_string())); +} + +#[test] +fn test_inherited_attaches_rank_by_source_position_across_source_calls() { + // `pkgb` is visible only at the first `source()` call, but it attached after + // `pkga`. The merged contexts must preserve that precedence. + let mut db = TestDb::new(); + install_packages(&mut db, &["pkga", "pkgb"]); + let root = workspace_root(&db, "w"); + + let main = make_file( + &mut db, + "w/main.R", + "library(pkga)\nif (dev) {\n library(pkgb)\n source(\"helpers.R\")\n}\nsource(\"helpers.R\")\n", + ); + let helpers_source = "top <- 1\n"; + let helpers = make_file(&mut db, "w/helpers.R", helpers_source); + + root.set_scripts(&mut db).to(vec![main, helpers]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + let offset = TextSize::from(helpers_source.find("top").unwrap() as u32); + let attaches = library_attaches(&db, &helpers.imports_at(&db, offset)); + + assert_eq!(attaches, vec!["pkgb".to_string(), "pkga".to_string()]); +} diff --git a/crates/oak_db/src/tests/file_resolve_at.rs b/crates/oak_db/src/tests/file_resolve_at.rs index e10138080..1dda06e9c 100644 --- a/crates/oak_db/src/tests/file_resolve_at.rs +++ b/crates/oak_db/src/tests/file_resolve_at.rs @@ -589,3 +589,417 @@ fn test_conditional_library_resolves_only_inside_its_branch() { let after = TextSize::from(source.rfind("foo").unwrap() as u32); assert!(script.resolve_at(&db, after).is_empty()); } + +#[test] +fn test_local_block_does_not_see_a_library_call_after_it() { + // `local()` runs at its call site, so a cursor inside it sees the search + // path as of that point: `library(mypkg)` hasn't run yet, so `foo` isn't + // attached. + let mut db = TestDb::new(); + install_library_package(&mut db, "mypkg", &["foo"], &[( + "library/mypkg/R/a.R", + "foo <- function() 42\n", + )]); + + let (_ws_root, files) = setup_workspace_scripts(&mut db, "ws", &[( + "ws/script.R", + "local({\n foo\n})\nlibrary(mypkg)\n", + )]); + let script = files[0]; + let source = script.source_text(&db).clone(); + + let offset = TextSize::from(source.find(" foo").unwrap() as u32 + 2); + assert!(script.resolve_at(&db, offset).is_empty()); +} + +#[test] +fn test_local_block_sees_a_file_scope_binding_before_it() { + // A binding made before the block is already in place by the time + // `local()` runs, same as for a use at file scope. + let mut db = TestDb::new(); + let source = "x <- 1\nlocal({\n x\n})\n"; + let file = make_file(&mut db, "a.R", source); + + let offset = TextSize::from(source.rfind('x').unwrap() as u32); + let def = resolve_one(&db, file, offset); + + assert_eq!(def.file(&db), file); + let range = def.name_range(&db).expect("local has a name range"); + assert_eq!(usize::from(range.start()), 0); +} + +/// A workspace holding `main.R`, which sources `helpers.R`, plus whatever else +/// the caller lists. Returns the files in the given order. +fn setup_sourced(db: &mut TestDb, files: &[(&str, &str)]) -> Vec { + let root = workspace_root(db, "w"); + let entities: Vec = files + .iter() + .map(|(path, contents)| make_file(db, path, contents)) + .collect(); + root.set_scripts(db).to(entities.clone()); + db.workspace_roots().set_roots(db).to(vec![root]); + entities +} + +#[test] +fn test_sourced_file_sees_definitions_before_the_source_call() { + let mut db = TestDb::new(); + let helpers_source = "before\n"; + let files = setup_sourced(&mut db, &[ + ("w/main.R", "before <- 1\nsource(\"helpers.R\")\n"), + ("w/helpers.R", helpers_source), + ]); + let (main, helpers) = (files[0], files[1]); + + let def = resolve_one(&db, helpers, TextSize::from(0)); + assert_eq!(def.file(&db), main); + assert_eq!(def.name(&db).text(&db).as_str(), "before"); +} + +#[test] +fn test_sourced_file_top_level_does_not_see_definitions_after_the_source_call() { + // `after` hadn't run when `source()` executed, so `helpers.R`'s top level + // genuinely can't see it. Without narrowing this resolves, because + // `ImportLayer::File` goes through whole-file `exports()`. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ("w/main.R", "source(\"helpers.R\")\nafter <- 1\n"), + ("w/helpers.R", "after\n"), + ]); + let helpers = files[1]; + + assert!(helpers.resolve_at(&db, TextSize::from(0)).is_empty()); +} + +#[test] +fn test_sourced_file_function_body_still_sees_definitions_after_the_source_call() { + // A function defined in `helpers.R` can be called from `main.R` after + // `main.R` finished running, so it does see `after`. Narrowing is for the + // Eager view only. + let mut db = TestDb::new(); + let helpers_source = "f <- function() after\n"; + let files = setup_sourced(&mut db, &[ + ("w/main.R", "source(\"helpers.R\")\nafter <- 1\n"), + ("w/helpers.R", helpers_source), + ]); + let (main, helpers) = (files[0], files[1]); + + let offset = TextSize::from(helpers_source.find("after").unwrap() as u32); + let def = resolve_one(&db, helpers, offset); + assert_eq!(def.file(&db), main); +} + +#[test] +fn test_conditional_definition_before_the_source_call_stays_visible() { + // `maybe` is only maybe-bound at the `source()` call. We over-approximate + // and keep it, so an unknown-symbol diagnostic won't fire on a name that + // might well be there. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ("w/main.R", "if (cond) maybe <- 1\nsource(\"helpers.R\")\n"), + ("w/helpers.R", "maybe\n"), + ]); + let (main, helpers) = (files[0], files[1]); + + let def = resolve_one(&db, helpers, TextSize::from(0)); + assert_eq!(def.file(&db), main); +} + +#[test] +fn test_source_call_in_a_function_body_narrows_nothing() { + // Nothing says when (or whether) `load()` runs, so there's no program point + // to narrow to. `helpers.R` falls back to whole-file exports and sees + // `after`. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ( + "w/main.R", + "load <- function() source(\"helpers.R\")\nafter <- 1\n", + ), + ("w/helpers.R", "after\n"), + ]); + let (main, helpers) = (files[0], files[1]); + + let def = resolve_one(&db, helpers, TextSize::from(0)); + assert_eq!(def.file(&db), main); +} + +#[test] +fn test_both_sourcing_files_contribute_definitions() { + // Two files source the same target. Each is a separate possible runtime + // (in `a.R`'s run `foo` comes from `a.R`, in `b.R`'s run from `b.R`), so + // both bindings are visible from the sourced file, not just the + // alphabetically-first sourcing file. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ("w/a.R", "foo <- 1\nsource(\"helpers.R\")\n"), + ("w/b.R", "foo <- 2\nsource(\"helpers.R\")\n"), + ("w/helpers.R", "foo\n"), + ]); + let (a, b, helpers) = (files[0], files[1], files[2]); + + let defs = helpers.resolve_at(&db, TextSize::from(0)); + let def_files: Vec = defs.iter().map(|def| def.file(&db)).collect(); + assert_eq!(def_files, vec![a, b]); +} + +#[test] +fn test_offset_narrowing_applies_independently_per_sourcing_site() { + // `a.R` binds `foo` before its `source()` call, `b.R` only after. The + // union across sourcing files doesn't defeat the per-site narrowing: only + // `a.R`'s binding had run by the time `helpers.R`'s top level executed in + // `b.R`'s chain, so `b.R` contributes nothing. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ("w/a.R", "foo <- 1\nsource(\"helpers.R\")\n"), + ("w/b.R", "source(\"helpers.R\")\nfoo <- 2\n"), + ("w/helpers.R", "foo\n"), + ]); + let a = files[0]; + let helpers = files[2]; + + let def = resolve_one(&db, helpers, TextSize::from(0)); + assert_eq!(def.file(&db), a); +} + +#[test] +fn test_lazy_view_sees_both_sourcing_files() { + // A function body runs after the whole file, so unlike the offset-narrowed + // top-level case, it sees both sourcing files' bindings regardless of + // where each `source()` call sits in its own file. Exercises the tracked + // `imports_by_sourcing_file` path (via `File::resolve`), not the `_at` one. + let mut db = TestDb::new(); + let helpers_source = "f <- function() foo\n"; + let files = setup_sourced(&mut db, &[ + ("w/a.R", "foo <- 1\nsource(\"helpers.R\")\n"), + ("w/b.R", "foo <- 2\nsource(\"helpers.R\")\n"), + ("w/helpers.R", helpers_source), + ]); + let (a, b, helpers) = (files[0], files[1], files[2]); + + let offset = TextSize::from(helpers_source.find("foo").unwrap() as u32); + let defs = helpers.resolve_at(&db, offset); + let def_files: Vec = defs.iter().map(|def| def.file(&db)).collect(); + assert_eq!(def_files, vec![a, b]); +} + +#[test] +fn test_convergent_chains_dedupe_to_one_definition() { + // `a.R` and `b.R` both source `defs.R` before `helpers.R`, so both chains + // reach the same `foo` binding. The union must not report it twice. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ("w/defs.R", "foo <- 1\n"), + ("w/a.R", "source(\"defs.R\")\nsource(\"helpers.R\")\n"), + ("w/b.R", "source(\"defs.R\")\nsource(\"helpers.R\")\n"), + ("w/helpers.R", "foo\n"), + ]); + let defs = files[0]; + let helpers = files[3]; + + let def = resolve_one(&db, helpers, TextSize::from(0)); + assert_eq!(def.file(&db), defs); +} + +#[test] +fn test_narrowing_applies_at_each_hop_of_a_source_chain() { + // `setup.R` sources `helpers.R` before binding `late_setup`, and `main.R` + // sources `setup.R` before binding `late_main`, so `helpers.R`'s top level + // sees neither. `early_main` ran before both calls, so it does show up. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ( + "w/main.R", + "early_main <- 1\nsource(\"setup.R\")\nlate_main <- 2\n", + ), + ("w/setup.R", "source(\"helpers.R\")\nlate_setup <- 3\n"), + ("w/helpers.R", "early_main\n"), + ]); + let (main, helpers) = (files[0], files[2]); + + let def = resolve_one(&db, helpers, TextSize::from(0)); + assert_eq!(def.file(&db), main); + + for name in ["late_main", "late_setup"] { + helpers + .set_source_text_override(&mut db) + .to(Some(format!("{name}\n"))); + assert!(helpers.resolve_at(&db, TextSize::from(0)).is_empty()); + } +} + +/// Installs two library packages. `install_library_package()` replaces the +/// library-root list, so restore both roots after the second call. +fn install_two_library_packages( + db: &mut TestDb, + first: (&str, &[&str]), + second: (&str, &[&str]), +) -> (Package, Package) { + let bindings = |exports: &[&str]| { + exports + .iter() + .map(|name| format!("{name} <- function() 1\n")) + .collect::() + }; + + let (first_root, first_pkg) = install_library_package(db, first.0, first.1, &[( + &format!("library/{}/R/a.R", first.0), + &bindings(first.1), + )]); + let (second_root, second_pkg) = install_library_package(db, second.0, second.1, &[( + &format!("library/{}/R/a.R", second.0), + &bindings(second.1), + )]); + db.library_roots() + .set_roots(db) + .to(vec![first_root, second_root]); + + (first_pkg, second_pkg) +} + +#[test] +fn test_second_source_call_keeps_the_conditional_attach_of_the_first() { + // The first `source()` call must retain `pkga`. Its attachment is confined + // to the `if` arm and is not visible at the second call. + let mut db = TestDb::new(); + let (_root, pkg) = install_library_package(&mut db, "pkga", &["foo"], &[( + "library/pkga/R/a.R", + "foo <- function() 42\n", + )]); + let pkg_file = pkg.files(&db)[0]; + + let files = setup_sourced(&mut db, &[ + ( + "w/main.R", + "if (dev) {\n library(pkga)\n source(\"helpers.R\")\n}\nsource(\"helpers.R\")\n", + ), + ("w/helpers.R", "foo\n"), + ]); + let helpers = files[1]; + + let def = resolve_one(&db, helpers, TextSize::from(0)); + assert_eq!(def.file(&db), pkg_file); +} + +#[test] +fn test_source_call_in_a_function_body_keeps_attaches_after_it() { + // `load()` can run after `library(pkga)`, so its `source()` call cannot be + // bounded by its textual offset. + let mut db = TestDb::new(); + let (_root, pkg) = install_library_package(&mut db, "pkga", &["foo"], &[( + "library/pkga/R/a.R", + "foo <- function() 42\n", + )]); + let pkg_file = pkg.files(&db)[0]; + + let files = setup_sourced(&mut db, &[ + ( + "w/main.R", + "load <- function() source(\"helpers.R\")\nlibrary(pkga)\nload()\n", + ), + ("w/helpers.R", "foo\n"), + ]); + let helpers = files[1]; + + let def = resolve_one(&db, helpers, TextSize::from(0)); + assert_eq!(def.file(&db), pkg_file); +} + +#[test] +fn test_source_call_in_a_function_body_unpins_a_later_top_level_one() { + // A `source()` call in `load()` can run after `after` is defined despite + // preceding the top-level call textually, so both calls remain unpinned. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ( + "w/main.R", + "load <- function() source(\"helpers.R\")\nsource(\"helpers.R\")\nafter <- 1\n", + ), + ("w/helpers.R", "after\n"), + ]); + let (main, helpers) = (files[0], files[1]); + + let def = resolve_one(&db, helpers, TextSize::from(0)); + assert_eq!(def.file(&db), main); +} + +#[test] +fn test_source_twice_sees_the_definitions_of_both_call_sites() { + // The combined context includes `first` and `second`, but not `third`, + // because each `source()` execution sees only earlier bindings. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ( + "w/main.R", + "first <- 1\nsource(\"helpers.R\")\nsecond <- 2\nsource(\"helpers.R\")\nthird <- 3\n", + ), + ("w/helpers.R", "first\n"), + ]); + let (main, helpers) = (files[0], files[1]); + + for name in ["first", "second"] { + helpers + .set_source_text_override(&mut db) + .to(Some(format!("{name}\n"))); + assert_eq!(resolve_one(&db, helpers, TextSize::from(0)).file(&db), main); + } + + helpers + .set_source_text_override(&mut db) + .to(Some("third\n".to_string())); + assert!(helpers.resolve_at(&db, TextSize::from(0)).is_empty()); +} + +#[test] +fn test_source_twice_with_an_attach_between_keeps_both_packages() { + // The combined contexts retain `pkga` for `only_a` and place later-attached + // `pkgb` ahead of it for `shared`. + let mut db = TestDb::new(); + let (pkg_a, pkg_b) = install_two_library_packages( + &mut db, + ("pkga", &["only_a", "shared"]), + ("pkgb", &["shared"]), + ); + let (file_a, file_b) = (pkg_a.files(&db)[0], pkg_b.files(&db)[0]); + + let files = setup_sourced(&mut db, &[ + ( + "w/main.R", + "library(pkga)\nsource(\"helpers.R\")\nlibrary(pkgb)\nsource(\"helpers.R\")\n", + ), + ("w/helpers.R", "only_a\n"), + ]); + let helpers = files[1]; + + assert_eq!( + resolve_one(&db, helpers, TextSize::from(0)).file(&db), + file_a + ); + + helpers + .set_source_text_override(&mut db) + .to(Some("shared\n".to_string())); + assert_eq!( + resolve_one(&db, helpers, TextSize::from(0)).file(&db), + file_b + ); +} + +#[test] +fn test_source_call_in_an_else_arm_does_not_see_the_if_arm() { + // Restoring the pre-`if` flow state before the `else` arm leaves `x` unbound + // when `b.R` is sourced, so source-call visibility cannot be a prefix by rank. + let mut db = TestDb::new(); + let files = setup_sourced(&mut db, &[ + ( + "w/main.R", + "if (cond) {\n x <- 1\n source(\"a.R\")\n} else {\n source(\"b.R\")\n}\n", + ), + ("w/a.R", "x\n"), + ("w/b.R", "x\n"), + ]); + let (main, a, b) = (files[0], files[1], files[2]); + + assert_eq!(resolve_one(&db, a, TextSize::from(0)).file(&db), main); + assert!(b.resolve_at(&db, TextSize::from(0)).is_empty()); +} diff --git a/crates/oak_db/src/tests/file_source_site.rs b/crates/oak_db/src/tests/file_source_site.rs new file mode 100644 index 000000000..4b05508cc --- /dev/null +++ b/crates/oak_db/src/tests/file_source_site.rs @@ -0,0 +1,445 @@ +use oak_semantic::ScopeId; +use salsa::Setter; + +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; +use crate::SourceSite; + +/// Build a workspace root at `/w` populated with the given scripts. +/// Returns the file handles in the same order. Registers the root with +/// `WorkspaceRoots` so `file_by_path` finds the files. +fn setup_workspace(db: &mut TestDb, scripts: &[(&str, &str)]) -> Vec { + let root = workspace_root(db, "w"); + let files: Vec = scripts + .iter() + .map(|(name, contents)| { + File::new( + db, + file_path(name), + FileRevision::zero(), + Some(contents.to_string()), + None, + ) + }) + .collect(); + root.set_scripts(db).to(files.clone()); + db.workspace_roots().set_roots(db).to(vec![root]); + files +} + +#[test] +fn test_source_call_to_registered_file_resolves_target() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/b.R", "x <- 1\n"), + ("w/a.R", "source(\"b.R\")\n"), + ]); + let b = files[0]; + let a = files[1]; + + let sites = a.source_sites(&db); + assert_eq!(sites.len(), 1); + assert_eq!(sites[0].target(), Some(b)); + assert_eq!(sites[0].path(), "b.R"); + assert_eq!(sites[0].scope(), ScopeId::from(0)); +} + +#[test] +fn test_source_call_to_unregistered_path_keeps_site_with_no_target() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[("w/a.R", "source(\"nope.R\")\n")]); + let a = files[0]; + + let sites = a.source_sites(&db); + assert_eq!(sites.len(), 1); + assert_eq!(sites[0].target(), None); + assert_eq!(sites[0].path(), "nope.R"); +} + +#[test] +fn test_two_source_calls_produce_sites_in_call_order() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/b.R", "b_val <- 1\n"), + ("w/c.R", "c_val <- 2\n"), + ("w/a.R", "source(\"b.R\")\nsource(\"c.R\")\n"), + ]); + let a = files[2]; + + let sites = a.source_sites(&db); + assert_eq!(sites.len(), 2); + assert_eq!(sites[0].path(), "b.R"); + assert_eq!(sites[1].path(), "c.R"); + assert!(sites[0].offset() < sites[1].offset()); +} + +#[test] +fn test_source_call_inside_function_body_has_non_file_scope() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/helpers.R", "helper <- 1\n"), + ("w/a.R", "f <- function() source(\"helpers.R\")\n"), + ]); + let a = files[1]; + + let sites = a.source_sites(&db); + assert_eq!(sites.len(), 1); + assert_ne!(sites[0].scope(), ScopeId::from(0)); +} + +#[test] +fn test_source_sites_yields_immediate_target_only() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/c.R", "c_val <- 1\n"), + ("w/b.R", "source(\"c.R\")\n"), + ("w/a.R", "source(\"b.R\")\n"), + ]); + let b = files[1]; + let a = files[2]; + + // a sources b, b sources c. a's own semantic_calls() only records the + // source() call literally in a's text, so c never shows up here. + let sites = a.source_sites(&db); + assert_eq!(sites.len(), 1); + assert_eq!(sites[0].target(), Some(b)); + assert_eq!(sites[0].path(), "b.R"); +} + +#[test] +fn test_file_with_no_source_calls_has_empty_source_sites() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[("w/a.R", "x <- 1\n")]); + let a = files[0]; + + assert!(a.source_sites(&db).is_empty()); +} + +#[test] +fn test_source_sites_backdates_across_unrelated_edit() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/b.R", "x <- 1\n"), + ("w/a.R", "source(\"b.R\")\n"), + ]); + let a = files[1]; + + let before: Vec = a.source_sites(&db).clone(); + let _ = a.sourced_by(&db); + assert_eq!(db.executions("File::source_sites"), 2); + assert_eq!(db.executions("sourcing_files_by_target"), 1); + + // Appending an unrelated statement changes a's semantic_index but not + // its source() calls, so `source_sites` re-executes for `a` and returns + // an unchanged value. `sourcing_files_by_target` reads that value, so it + // backdates rather than re-executing. + a.set_source_text_override(&mut db) + .to(Some("source(\"b.R\")\nx <- 1\n".to_string())); + let after: Vec = a.source_sites(&db).clone(); + let _ = a.sourced_by(&db); + + assert_eq!(before, after); + assert_eq!(db.executions("File::source_sites"), 3); + assert_eq!(db.executions("sourcing_files_by_target"), 1); +} + +#[test] +fn test_source_call_registers_reverse_site() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "source(\"a.R\")\n"), + ]); + let a = files[0]; + let b = files[1]; + + assert_eq!(a.sourced_by(&db), &vec![b]); +} + +#[test] +fn test_two_sourcing_files_are_ordered_by_path() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/c.R", "source(\"a.R\")\n"), + ("w/b.R", "source(\"a.R\")\n"), + ]); + let a = files[0]; + let c = files[1]; + let b = files[2]; + + assert_eq!(a.sourced_by(&db), &vec![b, c]); +} + +#[test] +fn test_file_nobody_sources_is_sourced_by_nothing() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[("w/a.R", "x <- 1\n")]); + let a = files[0]; + + assert!(a.sourced_by(&db).is_empty()); +} + +#[test] +fn test_unresolved_source_call_contributes_no_reverse_site() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "source(\"nope.R\")\n"), + ]); + let a = files[0]; + + assert!(a.sourced_by(&db).is_empty()); +} + +#[test] +fn test_file_sourcing_same_target_twice_is_listed_once() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "source(\"a.R\")\nsource(\"a.R\")\n"), + ]); + let a = files[0]; + let b = files[1]; + + assert_eq!(a.sourced_by(&db), &vec![b]); + + // Both call positions are still reachable, just through the volatile + // forward query rather than the position-free reverse one. + let sites = b.source_sites(&db); + assert_eq!(sites.len(), 2); + assert!(sites[0].offset() < sites[1].offset()); +} + +#[test] +fn test_file_sourcing_itself_does_not_panic_or_recurse() { + // `source("a.R")` inside `a.R` cycles `semantic_index(a)` through + // `exports(a)`, so the index gets rebuilt with `NoopImportsResolver`. That + // resolver leaves `resolve_effects` at its default `None`, so a bare + // `source()` isn't recognized as effectful and no site is recorded. + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[("w/a.R", "source(\"a.R\")\n")]); + let a = files[0]; + + assert!(a.sourced_by(&db).is_empty()); +} + +#[test] +fn test_adding_source_call_in_new_file_updates_sourced_by() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "source(\"a.R\")\n"), + ]); + let a = files[0]; + let b = files[1]; + assert_eq!(a.sourced_by(&db).len(), 1); + + let root = db.workspace_roots().roots(&db)[0]; + let c = File::new( + &db, + file_path("w/c.R"), + FileRevision::zero(), + Some("source(\"a.R\")\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b, c]); + + assert_eq!(a.sourced_by(&db), &vec![b, c]); +} + +#[test] +fn test_edit_above_a_source_call_does_not_invalidate_sourced_by() { + // Inserting a line above `source("a.R")` shifts its offset, so `b`'s + // `source_sites` re-executes with a changed value. `sourced_by` carries no + // offsets, so it stays green through an edit that changed no source edge. + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "source(\"a.R\")\n"), + ]); + let a = files[0]; + let b = files[1]; + + let _ = a.sourced_by(&db); + let before = b.source_sites(&db)[0].offset(); + assert_eq!(db.executions("File::sourced_by"), 1); + + b.set_source_text_override(&mut db) + .to(Some("library(dplyr)\nsource(\"a.R\")\n".to_string())); + + let _ = a.sourced_by(&db); + assert!(b.source_sites(&db)[0].offset() > before); + assert_eq!(db.executions("File::sourced_by"), 1); +} + +#[test] +fn test_edit_above_a_source_call_does_not_rebuild_the_reverse_map() { + // Offset-only edits recompute `File::source_targets()` without invalidating + // `sourcing_files_by_target()`. + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "source(\"a.R\")\n"), + ]); + let a = files[0]; + let b = files[1]; + + let _ = a.sourced_by(&db); + assert_eq!(db.executions("sourcing_files_by_target"), 1); + assert_eq!(db.executions("File::source_targets"), 2); + + b.set_source_text_override(&mut db) + .to(Some("library(dplyr)\nsource(\"a.R\")\n".to_string())); + + let _ = a.sourced_by(&db); + assert_eq!(db.executions("sourcing_files_by_target"), 1); + assert_eq!(db.executions("File::source_targets"), 3); +} + +#[test] +fn test_retargeting_a_source_call_rebuilds_the_reverse_map() { + // A target change must invalidate `sourcing_files_by_target()` to remove the + // old reverse edge and add the new one. + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "source(\"a.R\")\n"), + ("w/c.R", "c_val <- 1\n"), + ]); + let a = files[0]; + let b = files[1]; + let c = files[2]; + + let _ = a.sourced_by(&db); + assert_eq!(db.executions("sourcing_files_by_target"), 1); + + b.set_source_text_override(&mut db) + .to(Some("source(\"c.R\")\n".to_string())); + + assert!(a.sourced_by(&db).is_empty()); + assert_eq!(c.sourced_by(&db), &vec![b]); + assert_eq!(db.executions("sourcing_files_by_target"), 2); +} + +#[test] +fn test_sourcing_files_by_target_firewalls_file_additions_from_sourced_by() { + // An unrelated body edit alone backdates all the way through (see the + // previous test). Adding a file anywhere changes `workspace_files` for + // real, forcing `sourcing_files_by_target` to re-execute, but `a`'s own + // entry is unaffected so `File::sourced_by` still backdates. + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "source(\"a.R\")\n"), + ]); + let a = files[0]; + let b = files[1]; + + let _ = a.sourced_by(&db); + assert_eq!(db.executions("sourcing_files_by_target"), 1); + assert_eq!(db.executions("File::sourced_by"), 1); + + let root = db.workspace_roots().roots(&db)[0]; + let elsewhere = File::new( + &db, + file_path("w/z.R"), + FileRevision::zero(), + Some("z_val <- 1\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b, elsewhere]); + + let _ = a.sourced_by(&db); + assert_eq!(db.executions("sourcing_files_by_target"), 2); + assert_eq!(db.executions("File::sourced_by"), 1); +} + +#[test] +fn test_adding_source_call_to_existing_file_invalidates_sourced_by() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[("w/a.R", "a_val <- 1\n"), ("w/b.R", "x <- 1\n")]); + let a = files[0]; + let b = files[1]; + + assert!(a.sourced_by(&db).is_empty()); + assert_eq!(db.executions("File::sourced_by"), 1); + + b.set_source_text_override(&mut db) + .to(Some("source(\"a.R\")\n".to_string())); + + assert_eq!(a.sourced_by(&db), &vec![b]); + assert_eq!(db.executions("sourcing_files_by_target"), 2); + assert_eq!(db.executions("File::sourced_by"), 2); +} + +#[test] +fn test_removing_source_call_invalidates_sourced_by() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "source(\"a.R\")\n"), + ]); + let a = files[0]; + let b = files[1]; + + assert_eq!(a.sourced_by(&db), &vec![b]); + assert_eq!(db.executions("File::sourced_by"), 1); + + b.set_source_text_override(&mut db) + .to(Some("x <- 1\n".to_string())); + + assert!(a.sourced_by(&db).is_empty()); + assert_eq!(db.executions("File::sourced_by"), 2); +} + +#[test] +fn test_retargeting_source_call_moves_the_edge() { + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[ + ("w/a.R", "a_val <- 1\n"), + ("w/b.R", "b_val <- 2\n"), + ("w/c.R", "source(\"a.R\")\n"), + ]); + let a = files[0]; + let b = files[1]; + let c = files[2]; + + assert_eq!(a.sourced_by(&db), &vec![c]); + assert!(b.sourced_by(&db).is_empty()); + + c.set_source_text_override(&mut db) + .to(Some("source(\"b.R\")\n".to_string())); + + assert!(a.sourced_by(&db).is_empty()); + assert_eq!(b.sourced_by(&db), &vec![c]); +} + +#[test] +fn test_registering_the_target_resolves_a_dangling_source_call() { + // `source_sites` resolves targets through `file_by_path`, so a site that + // named a file the scanner hadn't reached yet fills in once it lands in a + // root. + let mut db = TestDb::new(); + let files = setup_workspace(&mut db, &[("w/b.R", "source(\"a.R\")\n")]); + let b = files[0]; + + assert_eq!(b.source_sites(&db)[0].target(), None); + + let root = db.workspace_roots().roots(&db)[0]; + let a = File::new( + &db, + file_path("w/a.R"), + FileRevision::zero(), + Some("a_val <- 1\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![a, b]); + + assert_eq!(b.source_sites(&db)[0].target(), Some(a)); + assert_eq!(a.sourced_by(&db), &vec![b]); +} diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_inherited_attach_shadows_a_callee.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_inherited_attach_shadows_a_callee.snap new file mode 100644 index 000000000..edd815c60 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_inherited_attach_shadows_a_callee.snap @@ -0,0 +1,10 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +assertion_line: 432 +expression: "render(\"w/helpers.R\", helpers_source, helpers.diagnostics(&db))" +--- +info[inherited-shadow]: This `library` call has an ambiguous effect. It resolves through package `base` when the file is sourced on its own, and to package `shadowr` when sourced by `main.R`. + --> w/helpers.R:1:1 + | +1 | library(dplyr) + | ^^^^^^^^^^^^^^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_inherited_namespace_import_shadows_a_callee.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_inherited_namespace_import_shadows_a_callee.snap new file mode 100644 index 000000000..75654aaf5 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_inherited_namespace_import_shadows_a_callee.snap @@ -0,0 +1,10 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +assertion_line: 493 +expression: "render(\"w/mypkg/helpers.R\", helpers_source, helpers.diagnostics(&db))" +--- +info[inherited-shadow]: This `library` call has an ambiguous effect. It resolves through package `base` when the file is sourced on its own, and to an import of `mypkg` when sourced by `main.R`. + --> w/mypkg/helpers.R:1:1 + | +1 | library(dplyr) + | ^^^^^^^^^^^^^^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_inherited_shadow.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_inherited_shadow.snap new file mode 100644 index 000000000..c0dee1b8e --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_inherited_shadow.snap @@ -0,0 +1,10 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +assertion_line: 354 +expression: "render(\"w/b.R\", b_source, b.diagnostics(&db))" +--- +info[inherited-shadow]: This `source` call has an ambiguous effect. It resolves through package `base` when the file is sourced on its own, and to a binding in `a.R` when sourced by `a.R`. + --> w/b.R:2:1 + | +2 | source("c.R") + | ^^^^^^^^^^^^^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_no_inherited_shadow_for_ordinary_sourcing.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_no_inherited_shadow_for_ordinary_sourcing.snap new file mode 100644 index 000000000..4bd6a1229 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_no_inherited_shadow_for_ordinary_sourcing.snap @@ -0,0 +1,7 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +assertion_line: 368 +expression: "render(\"w/helpers.R\", helpers_source, helpers.diagnostics(&db))" +--- +w/helpers.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_no_inherited_shadow_for_own_attach_ordering.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_no_inherited_shadow_for_own_attach_ordering.snap new file mode 100644 index 000000000..bd21fa313 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_no_inherited_shadow_for_own_attach_ordering.snap @@ -0,0 +1,7 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +assertion_line: 458 +expression: "render(\"w/helpers.R\", helpers_source, helpers.diagnostics(&db))" +--- +w/helpers.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_no_inherited_shadow_when_file_binds_the_name_itself.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_no_inherited_shadow_when_file_binds_the_name_itself.snap new file mode 100644 index 000000000..79cc0ea99 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_no_inherited_shadow_when_file_binds_the_name_itself.snap @@ -0,0 +1,7 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +assertion_line: 386 +expression: "render(\"w/helpers.R\", helpers_source, helpers.diagnostics(&db))" +--- +w/helpers.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle.snap new file mode 100644 index 000000000..0c2d29973 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle.snap @@ -0,0 +1,9 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"w/a.R\", a_source, a.diagnostics(&db))" +--- +warning[source-cycle]: This file takes part in a cycle of mutual `source()` calls. Language analysis will be incomplete until the cycle is resolved. + --> w/a.R:1:1 + | +1 | source("b.R") + | ^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_reported_on_both_files.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_reported_on_both_files.snap new file mode 100644 index 000000000..e0ce294b5 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_source_cycle_reported_on_both_files.snap @@ -0,0 +1,9 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"w/b.R\", b_source, b.diagnostics(&db))" +--- +warning[source-cycle]: This file takes part in a cycle of mutual `source()` calls. Language analysis will be incomplete until the cycle is resolved. + --> w/b.R:1:1 + | +1 | source("a.R") + | ^ diff --git a/crates/oak_ide/tests/integration/find_references.rs b/crates/oak_ide/tests/integration/find_references.rs index d41574dba..cb65184e0 100644 --- a/crates/oak_ide/tests/integration/find_references.rs +++ b/crates/oak_ide/tests/integration/find_references.rs @@ -18,6 +18,7 @@ use crate::support::install_library_package_files; use crate::support::install_workspace_package; use crate::support::offset; use crate::support::pairs; +use crate::support::place_in_workspace_scripts; use crate::support::range; use crate::support::ranges; use crate::support::upsert; @@ -404,6 +405,35 @@ fn test_locally_scoped_stays_in_file() { ]); } +#[test] +fn test_cross_file_references_span_both_sourcing_files() { + // `a.R` and `b.R` each define their own `foo`, source `helpers.R`, and use + // `foo` again. Both sourcing files are separate possible runtimes of + // `helpers.R`, so a references search from `helpers.R`'s `foo` must cover + // both, not just the alphabetically-first one. + let mut db = OakDatabase::new(); + let a_source = "foo <- 1\nsource(\"helpers.R\")\nfoo\n"; + let b_source = "foo <- 2\nsource(\"helpers.R\")\nfoo\n"; + let a = upsert(&mut db, "a.R", a_source); + let b = upsert(&mut db, "b.R", b_source); + let helpers = upsert(&mut db, "helpers.R", "foo\n"); + place_in_workspace_scripts(&mut db, vec![a, b, helpers]); + + let a_use = a_source.rfind("foo").unwrap() as u32; + let b_use = b_source.rfind("foo").unwrap() as u32; + + let refs = find_references(&db, helpers, offset(0), true); + // helpers.R (cursor's file) first, then a.R and b.R alphabetically by + // path, def before use within each sourcing file. + assert_eq!(pairs(&refs), vec![ + (helpers, range(0, 3)), + (a, range(0, 3)), + (a, range(a_use, a_use + 3)), + (b, range(0, 3)), + (b, range(b_use, b_use + 3)), + ]); +} + // --- Bare name <-> namespace bridge --- #[test] @@ -568,3 +598,25 @@ fn test_cross_package_references_via_library() { range(use_start, use_start + 3) )]); } + +#[test] +fn test_cross_file_references_reach_past_a_lazy_source_call() { + // `load()` can source `helpers.R` after `after` is defined, so its context + // remains unpinned and references include `after` in `main.R`. + let mut db = OakDatabase::new(); + let main_source = + "load <- function() source(\"helpers.R\")\nsource(\"helpers.R\")\nafter <- 1\nafter\n"; + let main = upsert(&mut db, "main.R", main_source); + let helpers = upsert(&mut db, "helpers.R", "after\n"); + place_in_workspace_scripts(&mut db, vec![helpers, main]); + + let def = main_source.find("after <- 1").unwrap() as u32; + let use_ = main_source.rfind("after").unwrap() as u32; + + let refs = find_references(&db, helpers, offset(0), true); + assert_eq!(pairs(&refs), vec![ + (helpers, range(0, 5)), + (main, range(def, def + 5)), + (main, range(use_, use_ + 5)), + ]); +} diff --git a/crates/oak_ide/tests/integration/goto_definition.rs b/crates/oak_ide/tests/integration/goto_definition.rs index 7dc36ef09..1c9309f6e 100644 --- a/crates/oak_ide/tests/integration/goto_definition.rs +++ b/crates/oak_ide/tests/integration/goto_definition.rs @@ -16,6 +16,7 @@ use oak_db::OakDatabase; use oak_ide::goto_definition; use crate::support::install_library_package; +use crate::support::place_in_workspace_scripts; use crate::support::range; use crate::support::upsert; @@ -254,3 +255,28 @@ fn test_navigates_from_assign_definition_site() { // resolves `base`, so the operators aren't recognized at this layer until // the resolver walks the search path. } + +#[test] +fn test_navigates_through_a_conditional_attach_at_one_of_two_source_calls() { + // The attach at the first `source()` call must remain available even though + // it is not visible at the second call. Otherwise `foo` has no target. + let mut db = OakDatabase::new(); + let pkg_file = + install_library_package(&mut db, "mypkg", &["foo"], "a.R", "foo <- function() 42\n"); + + let main = upsert( + &mut db, + "main.R", + "if (dev) {\n library(mypkg)\n source(\"helpers.R\")\n}\nsource(\"helpers.R\")\n", + ); + let helpers = upsert(&mut db, "helpers.R", "foo\n"); + place_in_workspace_scripts(&mut db, vec![main, helpers]); + + let targets = goto_definition(&db, helpers, TextSize::from(0u32)); + assert_eq!(targets.len(), 1); + let target = &targets[0]; + + assert_eq!(target.file, pkg_file); + assert_eq!(target.name, "foo"); + assert_eq!(target.full_range, range(0, 3)); +} diff --git a/crates/oak_ide/tests/integration/rename.rs b/crates/oak_ide/tests/integration/rename.rs index 618263ae7..0d52cf17f 100644 --- a/crates/oak_ide/tests/integration/rename.rs +++ b/crates/oak_ide/tests/integration/rename.rs @@ -18,6 +18,7 @@ use crate::support::edit_ranges; use crate::support::install_library_package; use crate::support::install_workspace_package; use crate::support::offset; +use crate::support::place_in_workspace_scripts; use crate::support::range; use crate::support::upsert; @@ -258,6 +259,33 @@ fn test_rename_cross_file_workspace_scripts() { ]); } +#[test] +fn test_rename_spans_both_sourcing_files() { + // Same fixture as + // `find_references::test_cross_file_references_span_both_sourcing_files`. + // Leaving one sourcing file's `foo` untouched would break that file's own + // run of `helpers.R`, since it would still see the old name. + let mut db = OakDatabase::new(); + let a_source = "foo <- 1\nsource(\"helpers.R\")\nfoo\n"; + let b_source = "foo <- 2\nsource(\"helpers.R\")\nfoo\n"; + let a = upsert(&mut db, "a.R", a_source); + let b = upsert(&mut db, "b.R", b_source); + let helpers = upsert(&mut db, "helpers.R", "foo\n"); + place_in_workspace_scripts(&mut db, vec![a, b, helpers]); + + let a_use = a_source.rfind("foo").unwrap() as u32; + let b_use = b_source.rfind("foo").unwrap() as u32; + + let targets = rename(&db, helpers, offset(0), "bar").unwrap(); + assert_eq!(edit_pairs(&targets), vec![ + (helpers, range(0, 3)), + (a, range(0, 3)), + (a, range(a_use, a_use + 3)), + (b, range(0, 3)), + (b, range(b_use, b_use + 3)), + ]); +} + // --- rename: cross-file workspace package --- #[test] @@ -338,21 +366,6 @@ fn test_rename_succeeds_for_workspace_package_export_via_library() { // --- helpers for root / package wiring --- -fn place_in_workspace_scripts(db: &mut OakDatabase, files: Vec) { - // Root path must be an ancestor of the files' URLs (see `file_url`), as a - // real scan guarantees: `File::root` resolves an unpackaged file to the - // root whose scan reached it, and `source()` anchoring reads that root's - // path. - let raw = if cfg!(windows) { - "file:///C:/project/R/" - } else { - "file:///project/R/" - }; - let url = FilePath::from_url(&Url::parse(raw).unwrap()); - let root = Root::new(db, url, RootKind::Workspace, files, vec![]); - db.workspace_roots().set_roots(db).to(vec![root]); -} - /// Build a workspace package holding `files` (name, contents), each with the /// package back-pointer set, and register it under a workspace root. Returns /// the created `File`s in order. diff --git a/crates/oak_ide/tests/integration/support.rs b/crates/oak_ide/tests/integration/support.rs index 6934bf33b..0bbc5349a 100644 --- a/crates/oak_ide/tests/integration/support.rs +++ b/crates/oak_ide/tests/integration/support.rs @@ -68,6 +68,21 @@ pub fn edit_pairs(edits: &[RenameEdit]) -> Vec<(File, TextRange)> { edits.iter().map(|e| (e.file, e.range)).collect() } +pub fn place_in_workspace_scripts(db: &mut OakDatabase, files: Vec) { + // Root path must be an ancestor of the files' URLs (see `file_url`), as a + // real scan guarantees: `File::root` resolves an unpackaged file to the + // root whose scan reached it, and `source()` anchoring reads that root's + // path. + let raw = if cfg!(windows) { + "file:///C:/project/R/" + } else { + "file:///project/R/" + }; + let url = FilePath::from_url(&Url::parse(raw).unwrap()); + let root = Root::new(db, url, RootKind::Workspace, files, vec![]); + db.workspace_roots().set_roots(db).to(vec![root]); +} + /// Install `name` as a library package exporting `exports`, with one file at /// `R/{file_name}`. Returns the package file. pub fn install_library_package( diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index 0b03fd2de..6c001d79c 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -50,6 +50,7 @@ use scan::FlowState; use scan::OpenScope; use crate::resolver::ImportsResolver; +use crate::semantic_index::BindingTimelineBuilder; use crate::semantic_index::Definition; use crate::semantic_index::DefinitionId; use crate::semantic_index::EnclosingSnapshotId; @@ -182,6 +183,7 @@ struct WalkState { lazy_snapshots: FxHashMap<(ScopeId, SymbolId), (ScopeId, EnclosingSnapshotId)>, semantic_calls: Vec, namespace_accesses: Vec, + bindings_at_sources: BindingTimelineBuilder, } impl SemanticIndexBuilder { @@ -238,6 +240,7 @@ impl SemanticIndexBuilder { lazy_snapshots: FxHashMap::default(), semantic_calls: Vec::new(), namespace_accesses: Vec::new(), + bindings_at_sources: BindingTimelineBuilder::default(), }, } } @@ -289,6 +292,20 @@ impl SemanticIndexBuilder { Some(scope) } + /// The scan unit that controls when code in `scope` runs: that scope itself + /// when lazy, otherwise its nearest lazy ancestor. `None` means the code + /// runs while the file loads. + /// + /// Mid-build twin of [`SemanticIndex::enclosing_lazy_scope`], reading the + /// arena the walk is still filling in. + fn enclosing_lazy_scope(&self, scope: ScopeId) -> Option { + let mut current = scope; + while !self.scopes[current].kind.is_lazy() { + current = self.scopes[current].parent?; + } + Some(current) + } + /// Whether `scope` binds `name` anywhere, regardless of flow position: an /// already-recorded `IS_BOUND` definition or a pre-scanned assignment. The /// pre-scan covers definitions the walk hasn't reached yet in this scope. @@ -363,6 +380,7 @@ impl SemanticIndexBuilder { self.walk.namespace_accesses, self.diagnostics, file_final_bindings, + self.walk.bindings_at_sources, ) } } diff --git a/crates/oak_semantic/src/builder/walk.rs b/crates/oak_semantic/src/builder/walk.rs index 38470e819..3e20d79d7 100644 --- a/crates/oak_semantic/src/builder/walk.rs +++ b/crates/oak_semantic/src/builder/walk.rs @@ -504,12 +504,13 @@ impl SemanticIndexBuilder { // (sequential execution is guaranteed), but inside a function it's // only visible within that function and its children, since the // function might never be called. Same reasoning as `source()` calls. - let call_range = call.syntax().text_trimmed_range(); - let region = self.attach_region(call_range.start(), &package); + let range = call.syntax().text_trimmed_range(); + let region = self.attach_region(range.start(), &package); self.walk.semantic_calls.push(SemanticCall { kind: SemanticCallKind::Attach { package, region }, - range: call_range, + range, scope: self.current_scope, + callee: bare_callee_name(call), }); } @@ -544,6 +545,7 @@ impl SemanticIndexBuilder { fn walk_source_call(&mut self, call: &aether_syntax::RCall) { let range = call.syntax().text_trimmed_range(); let call_offset = range.start(); + let callee = bare_callee_name(call); // Read back what the scan cached: the sourced files, each with its // resolution. The scan is the single point that extracts the paths and @@ -553,6 +555,22 @@ impl SemanticIndexBuilder { None => return, }; + // Only a call that runs at load time pins down what the sourced + // file's top level can see. One inside a function body might never + // run, or might run after the rest of this file, so we record + // nothing there and let the sourced file fall back to whole-file + // exports. + if self.enclosing_lazy_scope(self.current_scope).is_none() { + let file_scope = ScopeId::from(0); + let symbols = &self.walk.symbol_tables[file_scope]; + let bound = self.walk.use_def_maps[file_scope] + .bound_symbol_ids() + .map(|symbol_id| symbols.symbol(symbol_id).name()); + self.walk + .bindings_at_sources + .record_source_call(call_offset, bound); + } + for SourcedFile { path, resolution } in sourced { // Record every sourced file, independent of whether it resolved. // `resolved` pins the canonical URL when resolution succeeded so @@ -563,6 +581,7 @@ impl SemanticIndexBuilder { kind: SemanticCallKind::Source { path, resolved }, range, scope: self.current_scope, + callee: callee.clone(), }); let Some(resolution) = resolution else { @@ -601,6 +620,10 @@ impl SemanticIndexBuilder { }, range, scope: self.current_scope, + // No callee: nothing is written at `range` under this name. + // The `source()` call that forwarded these carries it, so a + // consumer keying on the callee sees the site once. + callee: None, }); } } @@ -1006,3 +1029,14 @@ impl SemanticIndexBuilder { } } } + +/// The callee of `call` when it's written as a bare identifier. `None` for +/// anything else, including a `pkg::fn` callee: `::` names the package outright, +/// so no binding can shadow it. Mirrors the two cases +/// `resolve_effects_handlers` recognizes. +fn bare_callee_name(call: &RCall) -> Option { + match call.function().ok()? { + AnyRExpression::RIdentifier(ident) => Some(ident.name_text().to_string()), + _ => None, + } +} diff --git a/crates/oak_semantic/src/semantic_index.rs b/crates/oak_semantic/src/semantic_index.rs index 8b6a6d38a..69969a81b 100644 --- a/crates/oak_semantic/src/semantic_index.rs +++ b/crates/oak_semantic/src/semantic_index.rs @@ -13,6 +13,7 @@ use oak_core::range::Ranged; use oak_index_vec::define_index; use oak_index_vec::IndexVec; use rustc_hash::FxHashMap; +use smallvec::SmallVec; use url::Url; use crate::use_def_map::Bindings; @@ -95,6 +96,124 @@ pub struct SemanticIndex { // the file's exports (see `exports()`). Only the file scope's exit state is // ever needed, so we keep this one copy rather than per-scope state. final_bindings: IndexVec, + + bindings_at_sources: Arc, + + // Ranks load-time `source()` calls without storing offsets in import layers. + // The map and timeline are built together, so a lookup cannot mix versions. + source_ranks: FxHashMap, +} + +/// Zero-based source-order position of a load-time `source()` call. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub(crate) struct SourceRank(u32); + +/// Maps each file-scope name to the load-time `source()` calls that can see it. +/// +/// Use a bitmap rather than a low-water mark because visibility can be +/// nonmonotone in source order. Restoring the pre-branch flow state before +/// `else` hides `if`-arm bindings from calls in the `else` arm. +/// +/// Ranks avoid invalidating the timeline when text above a `source()` call +/// shifts offsets without changing bindings. Salsa backdates the equal value, +/// avoiding re-resolution of every transitively sourced file. +#[derive(Debug, Default, PartialEq, Eq)] +pub(crate) struct BindingTimeline { + visible_at: FxHashMap, +} + +/// Stores [`SourceRank`]s as a bitmap. The first 64 ranks remain inline, +/// avoiding a name set for every load-time `source()` call. +#[derive(Debug, Default, PartialEq, Eq)] +struct RankSet { + words: SmallVec<[u64; 1]>, +} + +impl RankSet { + fn insert(&mut self, rank: SourceRank) { + let (word, bit) = rank.position(); + if self.words.len() <= word { + self.words.resize(word + 1, 0); + } + self.words[word] |= 1 << bit; + } + + fn contains(&self, rank: SourceRank) -> bool { + let (word, bit) = rank.position(); + self.words + .get(word) + .is_some_and(|word| word & (1 << bit) != 0) + } +} + +impl SourceRank { + fn position(self) -> (usize, u32) { + (self.0 as usize / 64, self.0 % 64) + } +} + +/// Builds a [`BindingTimeline`] and maps source offsets to timeline ranks. +#[derive(Debug, Default)] +pub(crate) struct BindingTimelineBuilder { + timeline: BindingTimeline, + ranks: FxHashMap, +} + +impl BindingTimelineBuilder { + pub(crate) fn record_source_call<'a>( + &mut self, + offset: TextSize, + bound: impl Iterator, + ) { + let rank = SourceRank(self.ranks.len() as u32); + self.ranks.insert(offset, rank); + + for name in bound { + match self.timeline.visible_at.get_mut(name) { + Some(ranks) => ranks.insert(rank), + None => { + let mut ranks = RankSet::default(); + ranks.insert(rank); + self.timeline.visible_at.insert(name.to_owned(), ranks); + }, + } + } + } + + pub(crate) fn finish(self) -> (BindingTimeline, FxHashMap) { + (self.timeline, self.ranks) + } +} + +/// File-scope names visible at one or more load-time `source()` calls. +/// +/// Import layers retain the shared timeline and source-call ranks instead of +/// copying visible names. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ExportsAtSource { + timeline: Arc, + ranks: SmallVec<[SourceRank; 1]>, +} + +impl ExportsAtSource { + pub fn contains(&self, name: &str) -> bool { + self.timeline + .visible_at + .get(name) + .is_some_and(|visible| self.sees(visible)) + } + + pub fn names(&self) -> impl Iterator { + self.timeline + .visible_at + .iter() + .filter(|(_, visible)| self.sees(visible)) + .map(|(name, _)| name.as_str()) + } + + fn sees(&self, visible: &RankSet) -> bool { + self.ranks.iter().any(|&rank| visible.contains(rank)) + } } impl SemanticIndex { @@ -109,7 +228,10 @@ impl SemanticIndex { namespace_accesses: Vec, diagnostics: Vec, final_bindings: IndexVec, + bindings_at_sources: BindingTimelineBuilder, ) -> Self { + let (timeline, source_ranks) = bindings_at_sources.finish(); + Self { scopes, symbol_tables, @@ -121,9 +243,19 @@ impl SemanticIndex { namespace_accesses, diagnostics, final_bindings, + bindings_at_sources: Arc::new(timeline), + source_ranks, } } + /// Attach a diagnostic the builder couldn't have known about, for the + /// caller that drove the build. [`crate::NoopImportsResolver`] users report + /// why they fell back this way. + pub fn with_diagnostic(mut self, diagnostic: SemanticDiagnostic) -> Self { + self.diagnostics.push(diagnostic); + self + } + pub fn scope(&self, id: ScopeId) -> &Scope { &self.scopes[id] } @@ -213,7 +345,8 @@ impl SemanticIndex { } /// Whether `scope` runs during the file's own top-level execution, i.e. no - /// enclosing scope is lazy. + /// enclosing scope is lazy. Wider than "is the file scope", since a + /// `local()` or `test_that()` body runs at its call site. pub fn scope_is_eager(&self, scope_id: ScopeId) -> bool { self.enclosing_lazy_scope(scope_id).is_none() } @@ -246,6 +379,28 @@ impl SemanticIndex { &self.diagnostics } + /// Returns file-scope names visible at any load-time `source()` call in + /// `offsets`. + /// + /// Returns `None` if `offsets` is empty or an offset is not a load-time + /// `source()` call. Lazy calls can run after the file has finished, so their + /// view is not fixed. + pub fn exports_at_sources(&self, offsets: &[TextSize]) -> Option { + if offsets.is_empty() { + return None; + } + + let ranks: Option> = offsets + .iter() + .map(|offset| self.source_ranks.get(offset).copied()) + .collect(); + + Some(ExportsAtSource { + timeline: Arc::clone(&self.bindings_at_sources), + ranks: ranks?, + }) + } + /// Find the innermost scope containing `offset`. pub fn scope_at(&self, offset: biome_rowan::TextSize) -> (ScopeId, &Scope) { // Start at the file scope @@ -797,6 +952,7 @@ pub struct SemanticCall { pub(crate) kind: SemanticCallKind, pub(crate) range: TextRange, pub(crate) scope: ScopeId, + pub(crate) callee: Option, } /// Where an attach is known to hold. @@ -852,12 +1008,20 @@ impl SemanticCall { &self.kind } + /// The whole call's trimmed range, for diagnostics that want to point at it. + pub fn range(&self) -> TextRange { + self.range + } + + /// Where the call starts, which is what flow ordering compares. pub fn offset(&self) -> TextSize { self.range.start() } - pub fn range(&self) -> TextRange { - self.range + /// The callee as written, when it was a bare identifier. `None` for a + /// qualified callee like `base::source()`, which no binding can shadow. + pub fn callee(&self) -> Option<&str> { + self.callee.as_deref() } pub fn scope(&self) -> ScopeId { @@ -941,6 +1105,13 @@ pub enum SemanticDiagnostic { /// A `library()`/`require()` attach whose package doesn't resolve. `range` /// points at the attach call. UninstalledPackage { package: String, range: TextRange }, + + /// This file takes part in a cycle of `source()` calls, so it was indexed + /// with [`NoopImportsResolver`](crate::NoopImportsResolver) and sees nothing + /// from the files it sources. Carries no range: under that resolver a bare + /// `source()` isn't recognized as effectful, so there's no recorded call to + /// point at. + SourceCycle, } /// Why an [`AmbiguousEffect`](SemanticDiagnostic::AmbiguousEffect) could have @@ -1004,3 +1175,37 @@ impl<'a> Iterator for AncestorScopeIdsIter<'a> { Some(id) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn names(exports: &ExportsAtSource) -> Vec<&str> { + let mut names: Vec<&str> = exports.names().collect(); + names.sort(); + names + } + + #[test] + fn test_a_call_sees_only_the_names_recorded_against_it() { + let mut builder = BindingTimelineBuilder::default(); + builder.record_source_call(TextSize::from(10), ["a"].into_iter()); + builder.record_source_call(TextSize::from(20), ["a", "b"].into_iter()); + let (timeline, ranks) = builder.finish(); + let timeline = Arc::new(timeline); + + let early = ExportsAtSource { + timeline: Arc::clone(&timeline), + ranks: [ranks[&TextSize::from(10)]].into_iter().collect(), + }; + let late = ExportsAtSource { + timeline, + ranks: [ranks[&TextSize::from(20)]].into_iter().collect(), + }; + + assert_eq!(names(&early), vec!["a"]); + assert_eq!(names(&late), vec!["a", "b"]); + assert!(!early.contains("b")); + assert!(late.contains("b")); + } +} diff --git a/crates/oak_semantic/src/use_def_map.rs b/crates/oak_semantic/src/use_def_map.rs index 3530905ad..570ab0b4a 100644 --- a/crates/oak_semantic/src/use_def_map.rs +++ b/crates/oak_semantic/src/use_def_map.rs @@ -543,6 +543,19 @@ impl UseDefMapBuilder { &self.symbol_states } + /// Yields symbols with live definitions, matching + /// [`crate::semantic_index::SemanticIndex::exports()`]. Conditional bindings + /// such as `if (cond) foo <- 1` are included, which safely over-approximates. + /// + /// Yields IDs so callers borrow names until the timeline needs to retain a + /// new one. + pub(crate) fn bound_symbol_ids(&self) -> impl Iterator + '_ { + self.symbol_states + .iter() + .filter(|(_, bindings)| !bindings.definitions().is_empty()) + .map(|(symbol_id, _)| symbol_id) + } + /// Finalize into an immutable [`UseDefMap`]. pub(crate) fn finish(mut self, uses: &IndexVec) -> UseDefMap { self.finish_deferred_defs(uses);