From b440f926b52843578fd2ac37a92aa4b6c09fba08 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Jul 2026 14:55:00 +0200 Subject: [PATCH 1/8] Introduce `SourcePath` for directory support --- crates/oak_semantic/src/builder/scan.rs | 9 ++-- crates/oak_semantic/src/effects.rs | 42 +++++++++++++++---- crates/oak_semantic/src/effects/contrib.rs | 5 ++- .../oak_semantic/tests/integration/common.rs | 17 ++++++-- .../tests/integration/contrib/base.rs | 6 ++- 5 files changed, 62 insertions(+), 17 deletions(-) diff --git a/crates/oak_semantic/src/builder/scan.rs b/crates/oak_semantic/src/builder/scan.rs index ce8b59b8d..f0e3c8a9f 100644 --- a/crates/oak_semantic/src/builder/scan.rs +++ b/crates/oak_semantic/src/builder/scan.rs @@ -446,14 +446,17 @@ impl SemanticIndexBuilder { // can see them. if let Some(paths) = source { let range = call.syntax().text_trimmed_range(); - for path in paths { - let resolution = self.scan_source_call(&path, range); + for sourced in paths { + let resolution = self.scan_source_call(&sourced.path, range); self.scan .call_resolutions .entry(range) .or_default() .source - .push(SourcedFile { path, resolution }); + .push(SourcedFile { + path: sourced.path, + resolution, + }); } } diff --git a/crates/oak_semantic/src/effects.rs b/crates/oak_semantic/src/effects.rs index 472d8a191..8d030232a 100644 --- a/crates/oak_semantic/src/effects.rs +++ b/crates/oak_semantic/src/effects.rs @@ -51,9 +51,9 @@ pub struct Effects { pub arguments: Option, /// Attach a package pub attach: Option, - /// Source one or more files. A vector so a collation-style callee can name + /// Source one or more paths. A vector so a collation-style callee can name /// several; base `source` resolves to one. - pub source: Option>, + pub source: Option>, /// Bind one or more names in the current scope (`assign("x", value)`). A /// vector so a multi-binding callee stays expressible; base `assign` and /// `delayedAssign` resolve to one. @@ -80,7 +80,7 @@ pub struct AssignBinding { pub struct EffectsHandlers { pub arguments: Option<&'static dyn EffectHandler>, pub attach: Option<&'static dyn EffectHandler>, - pub source: Option<&'static dyn EffectHandler>>, + pub source: Option<&'static dyn EffectHandler>>, pub assign: Option<&'static dyn AssignHandler>, } @@ -369,21 +369,40 @@ impl EffectHandler for ArgumentsAnnotation { } } -/// Declares how a source function (`source()`) names the file it reads, and -/// serves as the default [`EffectHandler`] for it by pulling that path out of a -/// call. +/// A path a source call names, and what that path points at. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SourcePath { + pub path: String, + pub target: SourceTarget, +} + +/// What a source function's path argument points at. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SourceTarget { + /// A single file, as base `source()` takes. + File, + /// A directory, every R file in it. The resolver expands the path into one + /// target per file, since listing it needs workspace knowledge a handler + /// doesn't have. + Dir, +} + +/// Declares how a source function (`source()`) names what it reads, and serves +/// as the default [`EffectHandler`] for it by pulling that path out of a call. #[derive(Debug, Clone, Copy)] pub struct SourceAnnotation { /// Which positional argument holds the path, counting only unnamed /// arguments (0 for base `source`). Other source-like functions may put the /// path elsewhere, so it's configured per entry rather than assumed. pub position: usize, + /// Whether that argument names a file or a directory. + pub target: SourceTarget, } impl EffectHandler for SourceAnnotation { - type Output = Vec; + type Output = Vec; - fn resolve(&self, call: &RCall, ctx: &CallContext<'_>) -> Option> { + fn resolve(&self, call: &RCall, ctx: &CallContext<'_>) -> Option> { let args = call.arguments().ok()?; // The path is matched positionally among unnamed arguments rather than @@ -433,7 +452,12 @@ impl EffectHandler for SourceAnnotation { positional += 1; } - path.map(|resolved| vec![resolved]) + path.map(|path| { + vec![SourcePath { + path, + target: self.target, + }] + }) } } diff --git a/crates/oak_semantic/src/effects/contrib.rs b/crates/oak_semantic/src/effects/contrib.rs index d1c049b4b..f526c7700 100644 --- a/crates/oak_semantic/src/effects/contrib.rs +++ b/crates/oak_semantic/src/effects/contrib.rs @@ -80,7 +80,10 @@ macro_rules! source { effects: $crate::effects::EffectsHandlers { arguments: None, attach: None, - source: Some(&$crate::effects::SourceAnnotation { position: $pos }), + source: Some(&$crate::effects::SourceAnnotation { + position: $pos, + target: $crate::effects::SourceTarget::File, + }), assign: None, }, } diff --git a/crates/oak_semantic/tests/integration/common.rs b/crates/oak_semantic/tests/integration/common.rs index 4e60e5583..cbf3acb1f 100644 --- a/crates/oak_semantic/tests/integration/common.rs +++ b/crates/oak_semantic/tests/integration/common.rs @@ -9,6 +9,8 @@ use oak_semantic::effects::CallContext; use oak_semantic::effects::EffectHandler; use oak_semantic::effects::EffectSite; use oak_semantic::effects::RangedAstPtr; +use oak_semantic::effects::SourcePath; +use oak_semantic::effects::SourceTarget; use oak_semantic::effects::TargetAccess; use oak_semantic::semantic_index::DefinitionKind; use oak_semantic::semantic_index::ScopeId; @@ -73,10 +75,19 @@ pub(crate) struct CollationHandler; pub(crate) static COLLATION_HANDLER: CollationHandler = CollationHandler; impl EffectHandler for CollationHandler { - type Output = Vec; + type Output = Vec; - fn resolve(&self, _call: &RCall, _ctx: &CallContext<'_>) -> Option> { - Some(vec!["a.R".into(), "b.R".into()]) + fn resolve(&self, _call: &RCall, _ctx: &CallContext<'_>) -> Option> { + Some(vec![ + SourcePath { + path: "a.R".into(), + target: SourceTarget::File, + }, + SourcePath { + path: "b.R".into(), + target: SourceTarget::File, + }, + ]) } } diff --git a/crates/oak_semantic/tests/integration/contrib/base.rs b/crates/oak_semantic/tests/integration/contrib/base.rs index 782150efe..92b5b8f48 100644 --- a/crates/oak_semantic/tests/integration/contrib/base.rs +++ b/crates/oak_semantic/tests/integration/contrib/base.rs @@ -4,6 +4,7 @@ use biome_rowan::AstNode; use oak_semantic::build_index; use oak_semantic::effects; use oak_semantic::effects::SourceAnnotation; +use oak_semantic::effects::SourceTarget; use oak_semantic::semantic_index::AmbiguityReason; use oak_semantic::semantic_index::AttachRegion; use oak_semantic::semantic_index::DefinitionId; @@ -115,7 +116,10 @@ impl ImportsResolver for MultiFileResolver { /// positional slot, exercising the configurable `position`. struct PositionResolver; -static SOURCE_AT_POSITION_1: SourceAnnotation = SourceAnnotation { position: 1 }; +static SOURCE_AT_POSITION_1: SourceAnnotation = SourceAnnotation { + position: 1, + target: SourceTarget::File, +}; impl ImportsResolver for PositionResolver { fn resolve_source(&mut self, _path: &str) -> Option { From 41dd1c02d9b6bde6f30ad7be6271e5748b9c6ef5 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Jul 2026 15:18:48 +0200 Subject: [PATCH 2/8] Support `sourceDir()` calls with stopgap approach --- crates/oak_db/src/file_imports.rs | 42 ++++---- crates/oak_db/src/imports.rs | 56 +++++++---- crates/oak_semantic/src/builder/effects.rs | 3 + crates/oak_semantic/src/builder/scan.rs | 58 +++++++---- crates/oak_semantic/src/effects.rs | 20 ++++ crates/oak_semantic/src/resolver.rs | 10 ++ .../tests/integration/contrib/base.rs | 97 +++++++++++++++++++ 7 files changed, 230 insertions(+), 56 deletions(-) diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 34eb4ec6b..6649d53f4 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -449,35 +449,18 @@ impl File { package.files(db).contains(&self) } - /// The collation members of `self`'s own `R/` directory, in load order: - /// sorted by basename, ASCII case-insensitively, the same order a - /// package `R/` with no `Collate:` gets from - /// `oak_scan::packages::order_alphabetically`. + /// The collation members of `self`'s own `R/` directory, in load order. /// /// Path-based only. The scan-time resolver /// ([`SalsaImportsResolver`](crate::imports::SalsaImportsResolver)) calls /// `cross_file_layers` while `self`'s own semantic index is still being /// built. The query can't recurse into the index. - /// - /// Gathers candidates from workspace roots only (`root.scripts(db)` for - /// each). `OrphanRoot`, library roots, and `StaleRoot` don't contribute - /// collation siblings. #[salsa::tracked(returns(ref))] pub(crate) fn collation_siblings(self, db: &dyn Db) -> Vec { let Some(dir) = self.path(db).as_path().and_then(Utf8Path::parent) else { return Vec::new(); }; - - let mut siblings: Vec = db - .workspace_roots() - .roots(db) - .iter() - .flat_map(|root| root.scripts(db).iter().copied()) - .filter(|file| file.path(db).as_path().and_then(Utf8Path::parent) == Some(dir)) - .collect(); - - siblings.sort_by_cached_key(|file| collation_basename_key(*file, db)); - siblings + files_in_directory(db, dir) } } @@ -811,7 +794,26 @@ fn testthat_support_key(file: File, db: &dyn Db) -> Cow<'_, str> { file.path(db).file_name().unwrap_or_default() } -/// Case-insensitive basename sort key for `collation_siblings`, matching +/// The workspace files sitting directly in `dir`, in load order: sorted by +/// basename, ASCII case-insensitively, the same order a package `R/` with no +/// `Collate:` gets from `oak_scan::packages::order_alphabetically`. +/// +/// Gathers from workspace roots only (`root.scripts(db)` for each). +/// `OrphanRoot`, library roots, and `StaleRoot` don't contribute. +pub(crate) fn files_in_directory(db: &dyn Db, dir: &Utf8Path) -> Vec { + let mut files: Vec = db + .workspace_roots() + .roots(db) + .iter() + .flat_map(|root| root.scripts(db).iter().copied()) + .filter(|file| file.path(db).as_path().and_then(Utf8Path::parent) == Some(dir)) + .collect(); + + files.sort_by_cached_key(|file| collation_basename_key(*file, db)); + files +} + +/// Case-insensitive basename sort key for [`files_in_directory`], matching /// `oak_scan::packages::order_alphabetically`'s `basename_key` so a /// non-package `R/` collates the same way as a package `R/` with no /// `Collate:`. diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index 30b3d0959..c600c5f06 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -11,6 +11,7 @@ use oak_semantic::SourceResolution; use rustc_hash::FxHashMap; use url::Url; +use crate::file_imports::files_in_directory; use crate::file_imports::CollationView; use crate::file_imports::ImportLayer; use crate::Db; @@ -56,6 +57,34 @@ impl<'db> SalsaImportsResolver<'db> { cache: EffectsCache::default(), } } + + fn source_resolution(&self, file: File) -> SourceResolution { + let names: Vec = file + .exports(self.db) + .iter() + .map(|(name, _)| name.to_string()) + .collect(); + + let packages: Vec = file + .attached_packages(self.db) + .iter() + .map(|name| name.text(self.db).to_string()) + .collect(); + + SourceResolution { + url: file.path(self.db).to_url(), + names, + packages, + } + } + + /// The workspace files a `SourceTarget::Dir` path names, in load order. + /// `None` when the path doesn't anchor. + fn files_in_source_dir(&self, path: &str) -> Option> { + let anchor = anchor_dir(self.db, self.file)?; + let target_path = resolve_relative_to(&anchor, path)?; + Some(files_in_directory(self.db, target_path.as_path()?)) + } } /// Per-build memo for `resolve_effects`, keyed on `(name, attached)`. @@ -97,24 +126,17 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { // TODO(diagnostics): Until we support out-of-workspace sourced files, // should we at least lint so user knows that we can't analyse the file? let file = self.db.file_by_path(&target_path)?; + Some(self.source_resolution(file)) + } - let names: Vec = file - .exports(self.db) - .iter() - .map(|(name, _)| name.to_string()) - .collect(); - - let packages: Vec = file - .attached_packages(self.db) - .iter() - .map(|name| name.text(self.db).to_string()) - .collect(); - - Some(SourceResolution { - url: target_path.to_url(), - names, - packages, - }) + fn resolve_source_dir(&mut self, path: &str) -> Vec { + self.files_in_source_dir(path) + .unwrap_or_default() + .into_iter() + // Exclude sourcing file + .filter(|file| *file != self.file) + .map(|file| self.source_resolution(file)) + .collect() } fn resolve_effects(&mut self, name: &str, attached: &[String]) -> Option { diff --git a/crates/oak_semantic/src/builder/effects.rs b/crates/oak_semantic/src/builder/effects.rs index b6235f56c..0785777c7 100644 --- a/crates/oak_semantic/src/builder/effects.rs +++ b/crates/oak_semantic/src/builder/effects.rs @@ -77,6 +77,9 @@ impl SemanticIndexBuilder { match &func { AnyRExpression::RIdentifier(ident) => { let name = ident.name_text(); + if let Some(effects) = effects::source_dir_idiom(&name) { + return Some(*effects); + } self.resolve_symbol_effects(&name, call.syntax().text_trimmed_range()) }, diff --git a/crates/oak_semantic/src/builder/scan.rs b/crates/oak_semantic/src/builder/scan.rs index f0e3c8a9f..ff814d747 100644 --- a/crates/oak_semantic/src/builder/scan.rs +++ b/crates/oak_semantic/src/builder/scan.rs @@ -30,6 +30,8 @@ use crate::effects::AssignBinding; use crate::effects::ResolvedArgumentEffect; use crate::effects::ResolvedArgumentEffects; use crate::effects::ScopeContext; +use crate::effects::SourcePath; +use crate::effects::SourceTarget; use crate::resolver::ImportsResolver; use crate::resolver::SourceResolution; use crate::semantic_index::AmbiguityReason; @@ -447,16 +449,24 @@ impl SemanticIndexBuilder { if let Some(paths) = source { let range = call.syntax().text_trimmed_range(); for sourced in paths { - let resolution = self.scan_source_call(&sourced.path, range); - self.scan - .call_resolutions - .entry(range) - .or_default() - .source - .push(SourcedFile { + let resolutions = self.scan_source_call(&sourced, range); + let entries = &mut self.scan.call_resolutions.entry(range).or_default().source; + + // Nothing resolved, so keep the path as written for a consumer + // to report. A directory that holds no scanned R files lands + // here too. + if resolutions.is_empty() { + entries.push(SourcedFile { path: sourced.path, - resolution, + resolution: None, }); + continue; + } + + entries.extend(resolutions.into_iter().map(|resolution| SourcedFile { + path: sourced.path.clone(), + resolution: Some(resolution), + })); } } @@ -756,22 +766,34 @@ impl SemanticIndexBuilder { } } - /// Resolve one sourced `path`, bind the names it brings in, and return its - /// resolution for the caller to cache. - /// - /// The binding is eager: `source()` runs at its position, so the sourced - /// names are bound afterwards and can shadow a later NSE callee (e.g. a - /// sourced `local` masking base `local`). Returns `None` when the resolver + /// Resolve one `sourced` path, bind the names it brings in, and return its + /// resolutions for the caller to cache. A directory yields one per R file + /// in it, in load order; a file yields at most one. Empty when the resolver /// can't locate the target. /// /// [`scan_call`]: Self::scan_call fn scan_source_call( &mut self, - path: &str, + sourced: &SourcePath, source_range: TextRange, - ) -> Option { - let resolution = self.resolver.resolve_source(path)?; + ) -> Vec { + let resolutions = match sourced.target { + SourceTarget::File => self + .resolver + .resolve_source(&sourced.path) + .into_iter() + .collect(), + SourceTarget::Dir => self.resolver.resolve_source_dir(&sourced.path), + }; + + for resolution in &resolutions { + self.record_source_resolution(resolution, source_range); + } + resolutions + } + /// Bind contributions of a sourced file. + fn record_source_resolution(&mut self, resolution: &SourceResolution, source_range: TextRange) { // Sourced names originate in another file, so they have no binding site // here. Anchor the overwrite range at the `source()` call instead. for name in &resolution.names { @@ -791,8 +813,6 @@ impl SemanticIndexBuilder { .push(pkg.clone(), source_range.start()); } } - - Some(resolution) } /// Whether the current evaluation frame binds `name` (see [`scan_scope`]). diff --git a/crates/oak_semantic/src/effects.rs b/crates/oak_semantic/src/effects.rs index 8d030232a..e7be39a17 100644 --- a/crates/oak_semantic/src/effects.rs +++ b/crates/oak_semantic/src/effects.rs @@ -100,6 +100,26 @@ pub fn annotates(name: &str) -> bool { INDEX.contains_key(name) } +/// HACK: This matches a `sourceDir()` call syntactically. See `?source` for the +/// definition of `sourceDir()` that people copy around: +/// https://github.com/search?q=sourceDir+language%3AR&type=code +/// This is a stopgap workaround until we can infer source effects around a +/// `list.files()` loop. +pub fn source_dir_idiom(name: &str) -> Option<&'static EffectsHandlers> { + if name != "sourceDir" { + return None; + } + Some(&EffectsHandlers { + arguments: None, + attach: None, + source: Some(&SourceAnnotation { + position: 0, + target: SourceTarget::Dir, + }), + assign: None, + }) +} + /// Resolver for an effect of a call. /// /// The single interface behind every effect kind (NSE, attach, source). diff --git a/crates/oak_semantic/src/resolver.rs b/crates/oak_semantic/src/resolver.rs index a3b044208..f60559ea6 100644 --- a/crates/oak_semantic/src/resolver.rs +++ b/crates/oak_semantic/src/resolver.rs @@ -50,6 +50,16 @@ pub trait ImportsResolver { /// Returns `None` when the target can't be located. fn resolve_source(&mut self, path: &str) -> Option; + /// Resolve a directory path to one resolution per R file in it, in the + /// order they load. `path` is anchored the same way [`resolve_source`] + /// anchors a file path. + /// + /// [`resolve_source`]: ImportsResolver::resolve_source + fn resolve_source_dir(&mut self, path: &str) -> Vec { + let _ = path; + Vec::new() + } + /// Resolve a bare callee `name` to its effects. `attached` is the packages /// attached at this point, in flow order. The builder passes it in because /// the resolver can't query our own semantic index without creating a diff --git a/crates/oak_semantic/tests/integration/contrib/base.rs b/crates/oak_semantic/tests/integration/contrib/base.rs index 92b5b8f48..4eb2c2883 100644 --- a/crates/oak_semantic/tests/integration/contrib/base.rs +++ b/crates/oak_semantic/tests/integration/contrib/base.rs @@ -88,6 +88,22 @@ impl ImportsResolver for MapResolver { } } +/// Expands any directory to `files`, standing in for the workspace listing. +/// Needs no `resolve_effects`: `sourceDir` is matched on its name. +struct DirResolver { + files: Vec, +} + +impl ImportsResolver for DirResolver { + fn resolve_source(&mut self, _path: &str) -> Option { + None + } + + fn resolve_source_dir(&mut self, _path: &str) -> Vec { + self.files.clone() + } +} + /// Resolves `source` to the multi-file [`CollationHandler`] and maps the /// collated paths through `sources`. struct MultiFileResolver { @@ -1245,6 +1261,87 @@ fn test_source_resolver_multiple_files_each_emitted_and_injected() { } } +#[test] +fn test_source_dir_expands_to_one_call_per_file() { + // A `SourceTarget::Dir` path routes to `resolve_source_dir` and expands to + // one `Source` call per file, in the order the resolver lists them, each + // followed by its own forwarded packages. Every call keeps the directory + // path as written, since no per-file path appears in the source text. + let files = vec![ + SourceResolution { + url: Url::parse("file:///R/a.R").unwrap(), + names: vec!["a_name".into()], + packages: vec!["pkgA".into()], + }, + SourceResolution { + url: Url::parse("file:///R/b.R").unwrap(), + names: vec!["b_name".into()], + packages: vec![], + }, + ]; + let code = "sourceDir(\"R\")\na_name\nb_name\n"; + let index = build_test_index(code, DirResolver { files }); + + assert_eq!(semantic_call_kinds(&index), [ + &SemanticCallKind::Source { + path: "R".into(), + resolved: Some(Url::parse("file:///R/a.R").unwrap()), + }, + &SemanticCallKind::Attach { + package: "pkgA".into(), + region: AttachRegion::Unconditional, + }, + &SemanticCallKind::Source { + path: "R".into(), + resolved: Some(Url::parse("file:///R/b.R").unwrap()), + }, + ]); + + // Both files' names are injected and resolve at their uses. + // Uses: sourceDir(0), a_name(1), b_name(2) + let file = ScopeId::from(0); + let map = index.use_def_map(file); + for use_index in [1, 2] { + let bindings = map.bindings_at_use(UseId::from(use_index)); + assert_eq!(bindings.definitions().len(), 1); + let def = &index.definitions(file)[bindings.definitions()[0]]; + assert!(matches!(def.kind(), DefinitionKind::Import { .. })); + } +} + +#[test] +fn test_source_dir_recognized_despite_a_local_definition() { + // `sourceDir` is the worked example in `?source`, so the caller has pasted + // the definition into their own file. Ordinary resolution reads that as a + // shadowing local binding and drops the effect, which is why the name is + // matched syntactically. + let files = vec![SourceResolution { + url: Url::parse("file:///R/a.R").unwrap(), + names: vec!["a_name".into()], + packages: vec![], + }]; + let code = "sourceDir <- function(path) NULL\nsourceDir(\"R\")\na_name\n"; + let index = build_test_index(code, DirResolver { files }); + + assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Source { + path: "R".into(), + resolved: Some(Url::parse("file:///R/a.R").unwrap()), + }]); +} + +#[test] +fn test_source_dir_with_no_files_records_the_path_unresolved() { + // A directory the resolver expands to nothing still records the path the + // user wrote, so a consumer can report it. Same shape as a file path that + // didn't resolve. + let index = build_test_index("sourceDir(\"R\")\n", DirResolver { files: vec![] }); + + assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Source { + path: "R".into(), + resolved: None, + }]); +} + #[test] fn test_source_resolver_honors_configured_path_position() { // A `SourceAnnotation` with `position: 1` takes the path from the second From 29bc613c301e69ac886b4a8445ad4c2b9a5f0f69 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Jul 2026 15:38:24 +0200 Subject: [PATCH 3/8] Add support for `targets::tar_source()` --- crates/oak_db/src/db.rs | 11 ++ crates/oak_db/src/file_imports.rs | 26 +++- crates/oak_db/src/imports.rs | 41 ++++-- crates/oak_db/src/tests/file_imports.rs | 114 +++++++++++++++ crates/oak_semantic/src/builder/scan.rs | 5 + crates/oak_semantic/src/effects.rs | 20 ++- crates/oak_semantic/src/effects/contrib.rs | 18 ++- .../src/effects/contrib/targets.rs | 13 ++ .../oak_semantic/tests/integration/contrib.rs | 1 + .../tests/integration/contrib/base.rs | 1 + .../tests/integration/contrib/targets.rs | 133 ++++++++++++++++++ 11 files changed, 359 insertions(+), 24 deletions(-) create mode 100644 crates/oak_semantic/src/effects/contrib/targets.rs create mode 100644 crates/oak_semantic/tests/integration/contrib/targets.rs diff --git a/crates/oak_db/src/db.rs b/crates/oak_db/src/db.rs index 96a7d07b9..7f691d4b3 100644 --- a/crates/oak_db/src/db.rs +++ b/crates/oak_db/src/db.rs @@ -209,6 +209,17 @@ pub fn workspace_files(db: &dyn Db) -> Vec { files } +/// The scripts held directly by workspace roots, in root order. +/// Like [`workspace_files`] but without package files. +#[salsa::tracked(returns(ref))] +pub(crate) fn workspace_scripts(db: &dyn Db) -> Vec { + db.workspace_roots() + .roots(db) + .iter() + .flat_map(|root| root.scripts(db).iter().copied()) + .collect() +} + fn collect_root_files(db: &dyn Db, files: &mut Vec, r: Root) { let owned = |f: File| root_by_file(db, f) == Some(r); files.extend(r.scripts(db).iter().copied().filter(|&f| owned(f))); diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 6649d53f4..ffd097ee8 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -11,6 +11,7 @@ use oak_semantic::semantic_index::SemanticCall; use oak_semantic::semantic_index::SemanticCallKind; use oak_semantic::semantic_index::SemanticIndex; +use crate::db::workspace_scripts; use crate::Db; use crate::File; use crate::Package; @@ -797,15 +798,10 @@ fn testthat_support_key(file: File, db: &dyn Db) -> Cow<'_, str> { /// The workspace files sitting directly in `dir`, in load order: sorted by /// basename, ASCII case-insensitively, the same order a package `R/` with no /// `Collate:` gets from `oak_scan::packages::order_alphabetically`. -/// -/// Gathers from workspace roots only (`root.scripts(db)` for each). -/// `OrphanRoot`, library roots, and `StaleRoot` don't contribute. pub(crate) fn files_in_directory(db: &dyn Db, dir: &Utf8Path) -> Vec { - let mut files: Vec = db - .workspace_roots() - .roots(db) + let mut files: Vec = workspace_scripts(db) .iter() - .flat_map(|root| root.scripts(db).iter().copied()) + .copied() .filter(|file| file.path(db).as_path().and_then(Utf8Path::parent) == Some(dir)) .collect(); @@ -813,6 +809,22 @@ pub(crate) fn files_in_directory(db: &dyn Db, dir: &Utf8Path) -> Vec { files } +/// The workspace files anywhere under `dir`, approximating the order returned by +/// `list.files(dir, recursive = TRUE)`. +pub(crate) fn files_in_directory_recursive(db: &dyn Db, dir: &Utf8Path) -> Vec { + let mut keyed: Vec<(String, File)> = workspace_scripts(db) + .iter() + .copied() + .filter_map(|file| { + let relative = file.path(db).as_path()?.strip_prefix(dir).ok()?; + Some((relative.as_str().to_ascii_lowercase(), file)) + }) + .collect(); + + keyed.sort_by(|(left, _), (right, _)| left.cmp(right)); + keyed.into_iter().map(|(_, file)| file).collect() +} + /// Case-insensitive basename sort key for [`files_in_directory`], matching /// `oak_scan::packages::order_alphabetically`'s `basename_key` so a /// non-package `R/` collates the same way as a package `R/` with no diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index c600c5f06..2b600ccea 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -11,7 +11,7 @@ use oak_semantic::SourceResolution; use rustc_hash::FxHashMap; use url::Url; -use crate::file_imports::files_in_directory; +use crate::file_imports::files_in_directory_recursive; use crate::file_imports::CollationView; use crate::file_imports::ImportLayer; use crate::Db; @@ -58,6 +58,8 @@ impl<'db> SalsaImportsResolver<'db> { } } + /// What sourcing `file` brings in. The two reads this makes are the ones + /// described on [`SalsaImportsResolver`]. fn source_resolution(&self, file: File) -> SourceResolution { let names: Vec = file .exports(self.db) @@ -77,14 +79,31 @@ impl<'db> SalsaImportsResolver<'db> { packages, } } +} - /// The workspace files a `SourceTarget::Dir` path names, in load order. - /// `None` when the path doesn't anchor. - fn files_in_source_dir(&self, path: &str) -> Option> { - let anchor = anchor_dir(self.db, self.file)?; - let target_path = resolve_relative_to(&anchor, path)?; - Some(files_in_directory(self.db, target_path.as_path()?)) - } +/// The workspace files a `SourceTarget::Dir` path names, in load order. Empty +/// when the path doesn't anchor. +/// +/// Tracked to give the directory listing a backdating point, the role +/// [`File::collation_siblings`] plays for the `R/` convention. The listing +/// filters every root's scripts, so without a memo here a file appearing +/// anywhere in the workspace would re-run the whole `semantic_index` of every +/// file holding a directory source, `_targets.R` being the one that hurts. +/// +/// Reads only inputs, so it's safe to call while `file`'s own index is being +/// built. +#[salsa::tracked(returns(ref))] +pub(crate) fn source_dir_scripts(db: &dyn Db, file: File, path: String) -> Vec { + let Some(anchor) = anchor_dir(db, file) else { + return Vec::new(); + }; + let Some(target_path) = resolve_relative_to(&anchor, &path) else { + return Vec::new(); + }; + let Some(dir) = target_path.as_path() else { + return Vec::new(); + }; + files_in_directory_recursive(db, dir) } /// Per-build memo for `resolve_effects`, keyed on `(name, attached)`. @@ -130,9 +149,9 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { } fn resolve_source_dir(&mut self, path: &str) -> Vec { - self.files_in_source_dir(path) - .unwrap_or_default() - .into_iter() + source_dir_scripts(self.db, self.file, path.to_string()) + .iter() + .copied() // Exclude sourcing file .filter(|file| *file != self.file) .map(|file| self.source_resolution(file)) diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index 815de22b7..4d9546d5e 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -1486,3 +1486,117 @@ fn test_body_edit_in_a_sourcing_file_does_not_invalidate_imports() { assert_eq!(db.executions("File::inherited_layers"), 3); assert_eq!(db.executions("File::imports"), 1); } + +// --- `SourceTarget::Dir` invalidation --- + +/// A workspace with `main.R` sourcing an `R/` directory, plus `a.R` and `b.R` +/// in it. Returns `(db, root, main, a, b)`. +fn source_dir_workspace() -> (TestDb, crate::Root, File, File, File) { + let mut db = TestDb::new(); + let root = workspace_root(&db, "ws"); + let main = File::new( + &db, + file_path("ws/main.R"), + FileRevision::zero(), + Some("sourceDir(\"R\")\n".to_string()), + None, + ); + let a = File::new( + &db, + file_path("ws/R/a.R"), + FileRevision::zero(), + Some("a_val <- function() 1\n".to_string()), + None, + ); + let b = File::new( + &db, + file_path("ws/R/b.R"), + FileRevision::zero(), + Some("b_val <- 2\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, a, b]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + (db, root, main, a, b) +} + +#[test] +fn test_source_dir_injects_every_file_in_the_directory() { + let (db, _root, main, _a, _b) = source_dir_workspace(); + let mut names: Vec<&str> = main.exports(&db).iter().map(|(name, _)| name).collect(); + names.sort(); + assert_eq!(names, vec!["a_val", "b_val"]); +} + +#[test] +fn test_source_dir_backdates_on_a_body_edit_in_the_directory() { + let (mut db, _root, main, a, _b) = source_dir_workspace(); + let _ = main.exports(&db); + let before = db.executions("File::semantic_index"); + + // A body edit leaves `a.R`'s top-level names alone, so `exports` backdates + // and nothing demands `main.R`'s index again. + a.set_source_text_override(&mut db) + .to(Some("a_val <- function() 999\n".to_string())); + let _ = main.exports(&db); + + assert_eq!(db.executions("File::semantic_index"), before + 1); +} + +#[test] +fn test_source_dir_backdates_on_a_file_added_outside_the_directory() { + let (mut db, root, main, a, b) = source_dir_workspace(); + let _ = main.exports(&db); + let before = db.executions("File::semantic_index"); + + // The listing reads every root's scripts, so `source_dir_scripts` re-runs. + // It returns the same files, so `main.R`'s index is never demanded again. + let elsewhere = File::new( + &db, + file_path("ws/other/z.R"), + FileRevision::zero(), + Some("z_val <- 1\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, a, b, elsewhere]); + let _ = main.exports(&db); + + assert_eq!(db.executions("File::semantic_index"), before); +} + +#[test] +fn test_source_dir_picks_up_a_file_added_inside_the_directory() { + let (mut db, root, main, a, b) = source_dir_workspace(); + let _ = main.exports(&db); + + let added = File::new( + &db, + file_path("ws/R/c.R"), + FileRevision::zero(), + Some("c_val <- 3\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, a, b, added]); + + let mut names: Vec<&str> = main.exports(&db).iter().map(|(name, _)| name).collect(); + names.sort(); + assert_eq!(names, vec!["a_val", "b_val", "c_val"]); +} + +#[test] +fn test_source_dir_recurses_into_subdirectories() { + // `tar_source()` recurses, and the listing is shared with `sourceDir()`. + let (mut db, root, main, a, b) = source_dir_workspace(); + let nested = File::new( + &db, + file_path("ws/R/models/fit.R"), + FileRevision::zero(), + Some("fit_val <- 4\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![main, a, b, nested]); + + let mut names: Vec<&str> = main.exports(&db).iter().map(|(name, _)| name).collect(); + names.sort(); + assert_eq!(names, vec!["a_val", "b_val", "fit_val"]); +} diff --git a/crates/oak_semantic/src/builder/scan.rs b/crates/oak_semantic/src/builder/scan.rs index ff814d747..5e190594a 100644 --- a/crates/oak_semantic/src/builder/scan.rs +++ b/crates/oak_semantic/src/builder/scan.rs @@ -784,6 +784,11 @@ impl SemanticIndexBuilder { .into_iter() .collect(), SourceTarget::Dir => self.resolver.resolve_source_dir(&sourced.path), + // A file has precedence over a directory of the same name. + SourceTarget::FileOrDir => match self.resolver.resolve_source(&sourced.path) { + Some(resolution) => vec![resolution], + None => self.resolver.resolve_source_dir(&sourced.path), + }, }; for resolution in &resolutions { diff --git a/crates/oak_semantic/src/effects.rs b/crates/oak_semantic/src/effects.rs index e7be39a17..65b6401db 100644 --- a/crates/oak_semantic/src/effects.rs +++ b/crates/oak_semantic/src/effects.rs @@ -115,6 +115,7 @@ pub fn source_dir_idiom(name: &str) -> Option<&'static EffectsHandlers> { source: Some(&SourceAnnotation { position: 0, target: SourceTarget::Dir, + default_path: None, }), assign: None, }) @@ -401,10 +402,11 @@ pub struct SourcePath { pub enum SourceTarget { /// A single file, as base `source()` takes. File, - /// A directory, every R file in it. The resolver expands the path into one - /// target per file, since listing it needs workspace knowledge a handler - /// doesn't have. + /// All R files in a directory (recursively). Dir, + /// Either. Whereas `source()` only supports files, `targets::tar_source()` + /// supports both. + FileOrDir, } /// Declares how a source function (`source()`) names what it reads, and serves @@ -417,6 +419,9 @@ pub struct SourceAnnotation { pub position: usize, /// Whether that argument names a file or a directory. pub target: SourceTarget, + /// Default path if no argument is suppolied (`tar_source()` defaults to + /// `files = "R"`). + pub default_path: Option<&'static str>, } impl EffectHandler for SourceAnnotation { @@ -425,6 +430,15 @@ impl EffectHandler for SourceAnnotation { fn resolve(&self, call: &RCall, ctx: &CallContext<'_>) -> Option> { let args = call.arguments().ok()?; + if args.items().iter().next().is_none() { + return self.default_path.map(|path| { + vec![SourcePath { + path: path.to_string(), + target: self.target, + }] + }); + } + // The path is matched positionally among unnamed arguments rather than // through [`CallContext::match_arguments`], for two reasons. We need to // inspect the `local =` value to bail on non-static calls, which diff --git a/crates/oak_semantic/src/effects/contrib.rs b/crates/oak_semantic/src/effects/contrib.rs index f526c7700..2c8c2eb33 100644 --- a/crates/oak_semantic/src/effects/contrib.rs +++ b/crates/oak_semantic/src/effects/contrib.rs @@ -5,6 +5,7 @@ mod magrittr; mod rlang; mod s7; mod shiny; +mod targets; mod testthat; mod withr; @@ -71,10 +72,16 @@ macro_rules! quoted { } pub(crate) use quoted; -/// A source entry: `(path-argument position)`. The function reads and evaluates -/// another file, injecting its top-level names into the caller. +/// A source entry: `(path-argument position)`, optionally what that argument +/// names (a [`SourceTarget`] variant, `File` by default) and what the function +/// reads when called with no arguments. +/// +/// [`SourceTarget`]: crate::effects::SourceTarget macro_rules! source { ($func:literal, $pos:literal) => { + $crate::effects::contrib::source!($func, $pos, File, None) + }; + ($func:literal, $pos:literal, $target:ident, $default:expr) => { $crate::effects::contrib::Entry { function: $func, effects: $crate::effects::EffectsHandlers { @@ -82,7 +89,8 @@ macro_rules! source { attach: None, source: Some(&$crate::effects::SourceAnnotation { position: $pos, - target: $crate::effects::SourceTarget::File, + target: $crate::effects::SourceTarget::$target, + default_path: $default, }), assign: None, }, @@ -149,6 +157,10 @@ pub(super) static REGISTRY: &[PackageEntries] = &[ name: "shiny", functions: shiny::ENTRIES, }, + PackageEntries { + name: "targets", + functions: targets::ENTRIES, + }, PackageEntries { name: "testthat", functions: testthat::ENTRIES, diff --git a/crates/oak_semantic/src/effects/contrib/targets.rs b/crates/oak_semantic/src/effects/contrib/targets.rs new file mode 100644 index 000000000..bf0654e41 --- /dev/null +++ b/crates/oak_semantic/src/effects/contrib/targets.rs @@ -0,0 +1,13 @@ +use crate::effects::contrib::source; +use crate::effects::contrib::Entry; + +pub(super) static ENTRIES: &[Entry] = &[ + // `tar_source(files = "R")` runs every R script under `files`, which is how + // a `_targets.R` pipeline sees its helper functions. Each element of + // `files` may be a script or a directory, and the bare `tar_source()` that + // most pipelines write relies on the default. + // + // `files` is a character vector, so `tar_source(c("R", "utils"))` names + // several paths. Only a single literal is read today. + source!("tar_source", 0, FileOrDir, Some("R")), +]; diff --git a/crates/oak_semantic/tests/integration/contrib.rs b/crates/oak_semantic/tests/integration/contrib.rs index d5d2a3743..b31ff517e 100644 --- a/crates/oak_semantic/tests/integration/contrib.rs +++ b/crates/oak_semantic/tests/integration/contrib.rs @@ -3,4 +3,5 @@ mod magrittr; mod rlang; mod s7; mod shiny; +mod targets; mod testthat; diff --git a/crates/oak_semantic/tests/integration/contrib/base.rs b/crates/oak_semantic/tests/integration/contrib/base.rs index 4eb2c2883..0740437a7 100644 --- a/crates/oak_semantic/tests/integration/contrib/base.rs +++ b/crates/oak_semantic/tests/integration/contrib/base.rs @@ -135,6 +135,7 @@ struct PositionResolver; static SOURCE_AT_POSITION_1: SourceAnnotation = SourceAnnotation { position: 1, target: SourceTarget::File, + default_path: None, }; impl ImportsResolver for PositionResolver { diff --git a/crates/oak_semantic/tests/integration/contrib/targets.rs b/crates/oak_semantic/tests/integration/contrib/targets.rs new file mode 100644 index 000000000..d0f0ec416 --- /dev/null +++ b/crates/oak_semantic/tests/integration/contrib/targets.rs @@ -0,0 +1,133 @@ +use aether_parser::parse; +use aether_parser::RParserOptions; +use oak_semantic::build_index; +use oak_semantic::effects; +use oak_semantic::semantic_index::SemanticCallKind; +use oak_semantic::semantic_index::SemanticIndex; +use oak_semantic::EffectsHandlers; +use oak_semantic::ImportsResolver; +use oak_semantic::SourceResolution; +use url::Url; + +use crate::common::semantic_call_kinds; + +/// Resolves `tar_source` against the targets registry entry, and stands in for +/// the workspace listing: `files` are what a directory expands to, `file` is +/// what a path resolving to a script gives. +struct TargetsResolver { + file: Option, + files: Vec, +} + +impl TargetsResolver { + fn with_dir(files: Vec) -> Self { + Self { file: None, files } + } +} + +impl ImportsResolver for TargetsResolver { + fn resolve_source(&mut self, _path: &str) -> Option { + self.file.clone() + } + + fn resolve_source_dir(&mut self, _path: &str) -> Vec { + self.files.clone() + } + + fn resolve_effects(&mut self, name: &str, _: &[String]) -> Option { + effects::lookup("targets", name) + .or_else(|| effects::lookup("base", name)) + .copied() + } +} + +fn index(source: &str, resolver: TargetsResolver) -> SemanticIndex { + let parsed = parse(source, RParserOptions::default()); + if parsed.has_error() { + panic!("source has syntax errors: {source}"); + } + build_index(&parsed.tree(), resolver) +} + +fn resolution(url: &str, name: &str) -> SourceResolution { + SourceResolution { + url: Url::parse(url).unwrap(), + names: vec![name.to_string()], + packages: vec![], + } +} + +fn sourced(path: &str, url: &str) -> SemanticCallKind { + SemanticCallKind::Source { + path: path.to_string(), + resolved: Some(Url::parse(url).unwrap()), + } +} + +#[test] +fn test_tar_source_no_arguments_uses_the_default_directory() { + // The bare `tar_source()` that most `_targets.R` pipelines write relies on + // `files = "R"`, so the default has to stand in for an absent argument. + let files = vec![ + resolution("file:///R/a.R", "a_name"), + resolution("file:///R/b.R", "b_name"), + ]; + let index = index("tar_source()\n", TargetsResolver::with_dir(files)); + + assert_eq!(semantic_call_kinds(&index), [ + &sourced("R", "file:///R/a.R"), + &sourced("R", "file:///R/b.R"), + ]); +} + +#[test] +fn test_tar_source_positional_directory() { + let files = vec![resolution("file:///code/a.R", "a_name")]; + let index = index("tar_source(\"code\")\n", TargetsResolver::with_dir(files)); + + assert_eq!(semantic_call_kinds(&index), [&sourced( + "code", + "file:///code/a.R" + )]); +} + +#[test] +fn test_tar_source_qualified_call_is_recognized() { + let files = vec![resolution("file:///R/a.R", "a_name")]; + let index = index("targets::tar_source()\n", TargetsResolver::with_dir(files)); + + assert_eq!(semantic_call_kinds(&index), [&sourced( + "R", + "file:///R/a.R" + )]); +} + +#[test] +fn test_tar_source_path_naming_a_script_resolves_as_a_file() { + // `files` takes scripts as well as directories, so a `FileOrDir` target + // tries the file first and only falls back to a listing. + let resolver = TargetsResolver { + file: Some(resolution("file:///R/utils.R", "util")), + files: vec![resolution("file:///unused.R", "unused")], + }; + let index = index("tar_source(\"R/utils.R\")\n", resolver); + + assert_eq!(semantic_call_kinds(&index), [&sourced( + "R/utils.R", + "file:///R/utils.R" + )]); +} + +#[test] +fn test_tar_source_named_files_argument_is_not_recognized() { + // `files = "code"` isn't read yet (the path is matched positionally), and + // the default must not step in for it. Inventing `"R"` here would source a + // directory the call explicitly overrode. + let files = vec![resolution("file:///R/a.R", "a_name")]; + let index = index( + "tar_source(files = \"code\")\n", + TargetsResolver::with_dir(files), + ); + + assert_eq!(semantic_call_kinds(&index), Vec::<&SemanticCallKind>::new()); +} From 326252b992c7f4310d8912d0f19bfe26ea57cc72 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Jul 2026 16:06:01 +0200 Subject: [PATCH 4/8] Don't warn in LSP log if package is not installed --- crates/ark/src/lsp/diagnostics.rs | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/crates/ark/src/lsp/diagnostics.rs b/crates/ark/src/lsp/diagnostics.rs index a66653477..6a81adb1f 100644 --- a/crates/ark/src/lsp/diagnostics.rs +++ b/crates/ark/src/lsp/diagnostics.rs @@ -995,7 +995,7 @@ fn handle_package_attach_call(node: Node, context: &mut DiagnosticContext) -> an let package_name = package_node.get_identifier_or_string_text(context.contents())?; let attach_pos = node.end_position(); - insert_package_exports(package_name, attach_pos, context)?; + insert_package_exports(package_name, attach_pos, context); // Also attach packages from `Depends` field, if any if let Some(package_names) = context @@ -1004,7 +1004,7 @@ fn handle_package_attach_call(node: Node, context: &mut DiagnosticContext) -> an .and_then(|package| package.depends(context.db).as_ref()) { for package_name in package_names.iter() { - insert_package_exports(package_name, attach_pos, context)?; + insert_package_exports(package_name, attach_pos, context); } } @@ -1050,21 +1050,16 @@ fn handle_package_attach_call(node: Node, context: &mut DiagnosticContext) -> an _ => vec![], }; for package_name in attach_field { - insert_package_exports(package_name, attach_pos, context)?; + insert_package_exports(package_name, attach_pos, context); } Ok(()) } -fn insert_package_exports( - package_name: &str, - attach_pos: Point, - context: &mut DiagnosticContext, -) -> anyhow::Result<()> { +fn insert_package_exports(package_name: &str, attach_pos: Point, context: &mut DiagnosticContext) { let Some(package) = context.db.package_by_name(package_name) else { - return Err(anyhow::anyhow!( - "Can't get exports from package {package_name} because it is not installed." - )); + // Package is not installed. This is linted via another path. + return; }; // Start from explicit `NAMESPACE` exports @@ -1083,8 +1078,6 @@ fn insert_package_exports( .entry(attach_pos) .or_default() .extend(exports); - - Ok(()) } fn recurse_subset_or_subset2( From b2c4655afce4b9d0e18c0806267f63dc96a03b38 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Jul 2026 16:51:34 +0200 Subject: [PATCH 5/8] Fix symbol resolution of predecessors during eager collation --- crates/oak_db/src/file_imports.rs | 43 +++++++++++++++- crates/oak_db/src/tests/file_resolve_at.rs | 60 ++++++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index ffd097ee8..08572f0ce 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -512,7 +512,18 @@ fn build_inherited_layers( None => ImportLayer::File(source_site), }; - let mut above = vec![source_layer]; + // One Source effect can load several files (`sourceDir()`, `tar_source()`). + // Keep track of already sourced files, since their eager top-level context + // can see what's already been sourced. + let mut above: Vec = match offsets.as_deref() { + Some(offsets) => loaded_before(db, source_site, file, offsets) + .into_iter() + .map(ImportLayer::File) + .collect(), + None => Vec::new(), + }; + + above.push(source_layer); above.extend(own_cross.above.iter().cloned()); above.extend( grandparents @@ -560,6 +571,36 @@ fn source_offsets(db: &dyn Db, sourcing_file: File, file: File) -> Option Vec { + let sites = source_file.source_sites(db); + let mut loaded: Vec = Vec::new(); + + // Descending, so the latest call's targets rank first. + for &offset in offsets.iter().rev() { + let targets: Vec = sites + .iter() + .filter(|site| site.offset() == offset) + .filter_map(|site| site.target()) + .collect(); + let Some(own) = targets.iter().position(|target| *target == file) else { + continue; + }; + + for &target in targets[..own].iter().rev() { + if !loaded.contains(&target) { + loaded.push(target); + } + } + } + + loaded +} + fn package_load_layers( file: File, db: &dyn Db, diff --git a/crates/oak_db/src/tests/file_resolve_at.rs b/crates/oak_db/src/tests/file_resolve_at.rs index 1dda06e9c..215872397 100644 --- a/crates/oak_db/src/tests/file_resolve_at.rs +++ b/crates/oak_db/src/tests/file_resolve_at.rs @@ -1003,3 +1003,63 @@ fn test_source_call_in_an_else_arm_does_not_see_the_if_arm() { assert_eq!(resolve_one(&db, a, TextSize::from(0)).file(&db), main); assert!(b.resolve_at(&db, TextSize::from(0)).is_empty()); } + +// --- ordered expansion of a directory source --- + +/// `main.R` sources the whole `R/` directory in one call, which loads `a.R` +/// then `b.R`. Returns `(db, a, b)`. +fn dir_source_workspace(a_contents: &str, b_contents: &str) -> (TestDb, File, File) { + let mut db = TestDb::new(); + let root = workspace_root(&db, "ws"); + let main = make_file(&mut db, "ws/main.R", "sourceDir(\"R\")\n"); + let a = make_file(&mut db, "ws/R/a.R", a_contents); + let b = make_file(&mut db, "ws/R/b.R", b_contents); + root.set_scripts(&mut db).to(vec![main, a, b]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + (db, a, b) +} + +#[test] +fn test_dir_source_top_level_sees_a_file_loaded_earlier_in_the_call() { + // `a.R` has fully run by the time `b.R` starts, so a top-level use in + // `b.R` resolves. The sourcing file's snapshot is taken before the call + // and holds neither name, so this can only come from the call's own + // ordered target list. + let (db, a, b) = dir_source_workspace("a_val <- 1\n", "use <- a_val\n"); + let def = resolve_one(&db, b, TextSize::from(7)); + assert_eq!(def.file(&db), a); +} + +#[test] +fn test_dir_source_top_level_does_not_see_a_file_loaded_later_in_the_call() { + // The mirror image: `b.R` hasn't run when `a.R`'s top level does, so this + // stays unresolved. Without it the test above would pass just as well on + // an implementation that made every target visible to every other. + let (db, a, _b) = dir_source_workspace("use <- b_val\n", "b_val <- 2\n"); + assert!(a.resolve_at(&db, TextSize::from(7)).is_empty()); +} + +#[test] +fn test_dir_source_function_body_sees_a_file_loaded_later_in_the_call() { + // A body runs after the whole call, so the lazy view has everything. + let (db, a, b) = dir_source_workspace("f <- function() b_val\n", "b_val <- 2\n"); + let def = resolve_one(&db, a, TextSize::from(17)); + assert_eq!(def.file(&db), b); +} + +#[test] +fn test_dir_sourced_twice_still_sees_the_files_loaded_earlier_in_each_call() { + // Each `sourceDir()` call is its own group of same-offset targets. `b.R` + // must see `a.R` through the group it belongs to, so the earlier call can't + // be skipped just because a later one also loads both files. + let mut db = TestDb::new(); + let root = workspace_root(&db, "ws"); + let main = make_file(&mut db, "ws/main.R", "sourceDir(\"R\")\nsourceDir(\"R\")\n"); + let a = make_file(&mut db, "ws/R/a.R", "a_val <- 1\n"); + let b = make_file(&mut db, "ws/R/b.R", "use <- a_val\n"); + root.set_scripts(&mut db).to(vec![main, a, b]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + + let def = resolve_one(&db, b, TextSize::from(7)); + assert_eq!(def.file(&db), a); +} From e2f4cfd216f450e73c999781dce78f1f0fdb6468 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 4 Aug 2026 11:58:35 +0200 Subject: [PATCH 6/8] Don't recurse into folders with `sourceDir()` --- crates/oak_db/src/imports.rs | 23 ++++++--- crates/oak_db/src/tests/file_imports.rs | 49 +++++++++++++++++-- crates/oak_semantic/src/builder/scan.rs | 6 +-- crates/oak_semantic/src/effects.rs | 24 ++++++--- crates/oak_semantic/src/effects/contrib.rs | 12 ++--- .../src/effects/contrib/targets.rs | 21 +++++--- crates/oak_semantic/src/resolver.rs | 11 +++-- .../tests/integration/contrib/base.rs | 5 +- .../tests/integration/contrib/targets.rs | 5 +- 9 files changed, 118 insertions(+), 38 deletions(-) diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index 2b600ccea..28fc71602 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -5,12 +5,14 @@ use camino::Utf8Component; use camino::Utf8Path; use camino::Utf8PathBuf; use oak_semantic::effects; +use oak_semantic::effects::DirWalk; use oak_semantic::EffectsHandlers; use oak_semantic::ImportsResolver; use oak_semantic::SourceResolution; use rustc_hash::FxHashMap; use url::Url; +use crate::file_imports::files_in_directory; use crate::file_imports::files_in_directory_recursive; use crate::file_imports::CollationView; use crate::file_imports::ImportLayer; @@ -81,8 +83,9 @@ impl<'db> SalsaImportsResolver<'db> { } } -/// The workspace files a `SourceTarget::Dir` path names, in load order. Empty -/// when the path doesn't anchor. +/// Returns scripts from the workspace directory named by `path`, in load order. +/// `walk` determines whether nested directories are included. Returns no files +/// when `path` cannot resolve to a directory. /// /// Tracked to give the directory listing a backdating point, the role /// [`File::collation_siblings`] plays for the `R/` convention. The listing @@ -93,7 +96,12 @@ impl<'db> SalsaImportsResolver<'db> { /// Reads only inputs, so it's safe to call while `file`'s own index is being /// built. #[salsa::tracked(returns(ref))] -pub(crate) fn source_dir_scripts(db: &dyn Db, file: File, path: String) -> Vec { +pub(crate) fn source_dir_scripts( + db: &dyn Db, + file: File, + path: String, + walk: DirWalk, +) -> Vec { let Some(anchor) = anchor_dir(db, file) else { return Vec::new(); }; @@ -103,7 +111,10 @@ pub(crate) fn source_dir_scripts(db: &dyn Db, file: File, path: String) -> Vec files_in_directory(db, dir), + DirWalk::Recursive => files_in_directory_recursive(db, dir), + } } /// Per-build memo for `resolve_effects`, keyed on `(name, attached)`. @@ -148,8 +159,8 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { Some(self.source_resolution(file)) } - fn resolve_source_dir(&mut self, path: &str) -> Vec { - source_dir_scripts(self.db, self.file, path.to_string()) + fn resolve_source_dir(&mut self, path: &str, walk: DirWalk) -> Vec { + source_dir_scripts(self.db, self.file, path.to_string(), walk) .iter() .copied() // Exclude sourcing file diff --git a/crates/oak_db/src/tests/file_imports.rs b/crates/oak_db/src/tests/file_imports.rs index 4d9546d5e..05eee5db7 100644 --- a/crates/oak_db/src/tests/file_imports.rs +++ b/crates/oak_db/src/tests/file_imports.rs @@ -1520,6 +1520,37 @@ fn source_dir_workspace() -> (TestDb, crate::Root, File, File, File) { (db, root, main, a, b) } +/// Creates a bare `tar_source()` pipeline with `R/a.R` and `R/models/fit.R`. +fn tar_source_workspace() -> (TestDb, crate::Root, File) { + let mut db = TestDb::new(); + install_packages(&mut db, &["targets"]); + let root = workspace_root(&db, "ws"); + let pipeline = File::new( + &db, + file_path("ws/_targets.R"), + FileRevision::zero(), + Some("library(targets)\ntar_source()\n".to_string()), + None, + ); + let a = File::new( + &db, + file_path("ws/R/a.R"), + FileRevision::zero(), + Some("a_val <- 1\n".to_string()), + None, + ); + let nested = File::new( + &db, + file_path("ws/R/models/fit.R"), + FileRevision::zero(), + Some("fit_val <- 4\n".to_string()), + None, + ); + root.set_scripts(&mut db).to(vec![pipeline, a, nested]); + db.workspace_roots().set_roots(&mut db).to(vec![root]); + (db, root, pipeline) +} + #[test] fn test_source_dir_injects_every_file_in_the_directory() { let (db, _root, main, _a, _b) = source_dir_workspace(); @@ -1584,8 +1615,9 @@ fn test_source_dir_picks_up_a_file_added_inside_the_directory() { } #[test] -fn test_source_dir_recurses_into_subdirectories() { - // `tar_source()` recurses, and the listing is shared with `sourceDir()`. +fn test_source_dir_does_not_recurse_into_subdirectories() { + // `sourceDir()` leaves `list.files()` at its `recursive = FALSE` default, + // so `R/models/fit.R` is excluded. let (mut db, root, main, a, b) = source_dir_workspace(); let nested = File::new( &db, @@ -1598,5 +1630,16 @@ fn test_source_dir_recurses_into_subdirectories() { let mut names: Vec<&str> = main.exports(&db).iter().map(|(name, _)| name).collect(); names.sort(); - assert_eq!(names, vec!["a_val", "b_val", "fit_val"]); + assert_eq!(names, vec!["a_val", "b_val"]); +} + +#[test] +fn test_tar_source_recurses_into_subdirectories() { + // `tar_source()` includes nested scripts because `file_list_files()` calls + // `list.files(recursive = TRUE)` for directories. + let (db, _root, pipeline) = tar_source_workspace(); + + let mut names: Vec<&str> = pipeline.exports(&db).iter().map(|(name, _)| name).collect(); + names.sort(); + assert_eq!(names, vec!["a_val", "fit_val"]); } diff --git a/crates/oak_semantic/src/builder/scan.rs b/crates/oak_semantic/src/builder/scan.rs index 5e190594a..35a9b12ff 100644 --- a/crates/oak_semantic/src/builder/scan.rs +++ b/crates/oak_semantic/src/builder/scan.rs @@ -783,11 +783,11 @@ impl SemanticIndexBuilder { .resolve_source(&sourced.path) .into_iter() .collect(), - SourceTarget::Dir => self.resolver.resolve_source_dir(&sourced.path), + SourceTarget::Dir(walk) => self.resolver.resolve_source_dir(&sourced.path, walk), // A file has precedence over a directory of the same name. - SourceTarget::FileOrDir => match self.resolver.resolve_source(&sourced.path) { + SourceTarget::FileOrDir(walk) => match self.resolver.resolve_source(&sourced.path) { Some(resolution) => vec![resolution], - None => self.resolver.resolve_source_dir(&sourced.path), + None => self.resolver.resolve_source_dir(&sourced.path, walk), }, }; diff --git a/crates/oak_semantic/src/effects.rs b/crates/oak_semantic/src/effects.rs index 65b6401db..0d34648c5 100644 --- a/crates/oak_semantic/src/effects.rs +++ b/crates/oak_semantic/src/effects.rs @@ -105,6 +105,9 @@ pub fn annotates(name: &str) -> bool { /// https://github.com/search?q=sourceDir+language%3AR&type=code /// This is a stopgap workaround until we can infer source effects around a /// `list.files()` loop. +/// +/// The copied `sourceDir()` idiom leaves `list.files()` at its +/// `recursive = FALSE` default, so nested scripts are excluded. pub fn source_dir_idiom(name: &str) -> Option<&'static EffectsHandlers> { if name != "sourceDir" { return None; @@ -114,7 +117,7 @@ pub fn source_dir_idiom(name: &str) -> Option<&'static EffectsHandlers> { attach: None, source: Some(&SourceAnnotation { position: 0, - target: SourceTarget::Dir, + target: SourceTarget::Dir(DirWalk::Shallow), default_path: None, }), assign: None, @@ -402,11 +405,20 @@ pub struct SourcePath { pub enum SourceTarget { /// A single file, as base `source()` takes. File, - /// All R files in a directory (recursively). - Dir, - /// Either. Whereas `source()` only supports files, `targets::tar_source()` - /// supports both. - FileOrDir, + /// R files in a directory. [`DirWalk`] determines whether descendants count. + Dir(DirWalk), + /// A file or directory. `source()` takes only files, while + /// `targets::tar_source()` takes both. + FileOrDir(DirWalk), +} + +/// Controls whether directory source targets include descendants. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum DirWalk { + /// Direct children only, matching `list.files()` without `recursive = TRUE`. + Shallow, + /// Every R file below the directory, matching `list.files(recursive = TRUE)`. + Recursive, } /// Declares how a source function (`source()`) names what it reads, and serves diff --git a/crates/oak_semantic/src/effects/contrib.rs b/crates/oak_semantic/src/effects/contrib.rs index 2c8c2eb33..6a626c156 100644 --- a/crates/oak_semantic/src/effects/contrib.rs +++ b/crates/oak_semantic/src/effects/contrib.rs @@ -72,16 +72,16 @@ macro_rules! quoted { } pub(crate) use quoted; -/// A source entry: `(path-argument position)`, optionally what that argument -/// names (a [`SourceTarget`] variant, `File` by default) and what the function -/// reads when called with no arguments. +/// Declares a source function's path argument, [`SourceTarget`], and +/// no-argument default. The target defaults to [`SourceTarget::File`]. /// /// [`SourceTarget`]: crate::effects::SourceTarget +/// [`SourceTarget::File`]: crate::effects::SourceTarget::File macro_rules! source { ($func:literal, $pos:literal) => { - $crate::effects::contrib::source!($func, $pos, File, None) + $crate::effects::contrib::source!($func, $pos, $crate::effects::SourceTarget::File, None) }; - ($func:literal, $pos:literal, $target:ident, $default:expr) => { + ($func:literal, $pos:literal, $target:expr, $default:expr) => { $crate::effects::contrib::Entry { function: $func, effects: $crate::effects::EffectsHandlers { @@ -89,7 +89,7 @@ macro_rules! source { attach: None, source: Some(&$crate::effects::SourceAnnotation { position: $pos, - target: $crate::effects::SourceTarget::$target, + target: $target, default_path: $default, }), assign: None, diff --git a/crates/oak_semantic/src/effects/contrib/targets.rs b/crates/oak_semantic/src/effects/contrib/targets.rs index bf0654e41..f22ee6a2e 100644 --- a/crates/oak_semantic/src/effects/contrib/targets.rs +++ b/crates/oak_semantic/src/effects/contrib/targets.rs @@ -1,13 +1,20 @@ use crate::effects::contrib::source; use crate::effects::contrib::Entry; +use crate::effects::DirWalk; +use crate::effects::SourceTarget; pub(super) static ENTRIES: &[Entry] = &[ - // `tar_source(files = "R")` runs every R script under `files`, which is how - // a `_targets.R` pipeline sees its helper functions. Each element of - // `files` may be a script or a directory, and the bare `tar_source()` that - // most pipelines write relies on the default. + // `tar_source()` loads scripts from `files`, defaulting to `R`. A path may + // name a script or directory. // - // `files` is a character vector, so `tar_source(c("R", "utils"))` names - // several paths. Only a single literal is read today. - source!("tar_source", 0, FileOrDir, Some("R")), + // Directory paths recurse because `file_list_files()` calls + // `list.files(recursive = TRUE)`. + // + // The scanner reads one literal even though `files` is a character vector. + source!( + "tar_source", + 0, + SourceTarget::FileOrDir(DirWalk::Recursive), + Some("R") + ), ]; diff --git a/crates/oak_semantic/src/resolver.rs b/crates/oak_semantic/src/resolver.rs index f60559ea6..28f8af934 100644 --- a/crates/oak_semantic/src/resolver.rs +++ b/crates/oak_semantic/src/resolver.rs @@ -1,6 +1,7 @@ use url::Url; use crate::effects; +use crate::effects::DirWalk; use crate::effects::EffectsHandlers; /// The result of resolving a `source()` call. Returned by @@ -50,13 +51,13 @@ pub trait ImportsResolver { /// Returns `None` when the target can't be located. fn resolve_source(&mut self, path: &str) -> Option; - /// Resolve a directory path to one resolution per R file in it, in the - /// order they load. `path` is anchored the same way [`resolve_source`] - /// anchors a file path. + /// Resolve a directory path to one resolution per R file in load order. + /// `path` uses the same anchor as [`resolve_source`]. `walk` determines + /// whether nested directories are included. /// /// [`resolve_source`]: ImportsResolver::resolve_source - fn resolve_source_dir(&mut self, path: &str) -> Vec { - let _ = path; + fn resolve_source_dir(&mut self, path: &str, walk: DirWalk) -> Vec { + let _ = (path, walk); Vec::new() } diff --git a/crates/oak_semantic/tests/integration/contrib/base.rs b/crates/oak_semantic/tests/integration/contrib/base.rs index 0740437a7..aa9e2e851 100644 --- a/crates/oak_semantic/tests/integration/contrib/base.rs +++ b/crates/oak_semantic/tests/integration/contrib/base.rs @@ -3,6 +3,7 @@ use aether_parser::RParserOptions; use biome_rowan::AstNode; use oak_semantic::build_index; use oak_semantic::effects; +use oak_semantic::effects::DirWalk; use oak_semantic::effects::SourceAnnotation; use oak_semantic::effects::SourceTarget; use oak_semantic::semantic_index::AmbiguityReason; @@ -99,7 +100,9 @@ impl ImportsResolver for DirResolver { None } - fn resolve_source_dir(&mut self, _path: &str) -> Vec { + fn resolve_source_dir(&mut self, _path: &str, walk: DirWalk) -> Vec { + // `sourceDir()` leaves `list.files()` at its `recursive = FALSE` default. + assert_eq!(walk, DirWalk::Shallow); self.files.clone() } } diff --git a/crates/oak_semantic/tests/integration/contrib/targets.rs b/crates/oak_semantic/tests/integration/contrib/targets.rs index d0f0ec416..58873925e 100644 --- a/crates/oak_semantic/tests/integration/contrib/targets.rs +++ b/crates/oak_semantic/tests/integration/contrib/targets.rs @@ -2,6 +2,7 @@ use aether_parser::parse; use aether_parser::RParserOptions; use oak_semantic::build_index; use oak_semantic::effects; +use oak_semantic::effects::DirWalk; use oak_semantic::semantic_index::SemanticCallKind; use oak_semantic::semantic_index::SemanticIndex; use oak_semantic::EffectsHandlers; @@ -30,7 +31,9 @@ impl ImportsResolver for TargetsResolver { self.file.clone() } - fn resolve_source_dir(&mut self, _path: &str) -> Vec { + fn resolve_source_dir(&mut self, _path: &str, walk: DirWalk) -> Vec { + // `tar_source()` lists directory arguments with `recursive = TRUE`. + assert_eq!(walk, DirWalk::Recursive); self.files.clone() } From c8b33fd99e5e2bb10b9bf7c85f77a9515a23c7e0 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 4 Aug 2026 12:10:33 +0200 Subject: [PATCH 7/8] Use C collation in packages --- crates/oak_db/src/file_imports.rs | 22 +++++++++++++--------- crates/oak_db/src/package.rs | 14 +++++++------- crates/oak_db/src/tests/file_imports_at.rs | 6 +++--- crates/oak_scan/src/packages.rs | 19 ++++++++++--------- crates/oak_scan/src/tests/packages.rs | 17 +++++++++++++---- crates/oak_scan/src/tests/workspace.rs | 8 ++++---- 6 files changed, 50 insertions(+), 36 deletions(-) diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 08572f0ce..2882d18ee 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -836,9 +836,10 @@ fn testthat_support_key(file: File, db: &dyn Db) -> Cow<'_, str> { file.path(db).file_name().unwrap_or_default() } -/// The workspace files sitting directly in `dir`, in load order: sorted by -/// basename, ASCII case-insensitively, the same order a package `R/` with no -/// `Collate:` gets from `oak_scan::packages::order_alphabetically`. +/// Returns workspace scripts directly under `dir`, in `list.files()` load order. +/// +/// `sourceDir()`, Shiny's `loadSupport()`, and non-package `R/` collation use +/// this order. [`collation_basename_key()`] mirrors their session-locale sort. pub(crate) fn files_in_directory(db: &dyn Db, dir: &Utf8Path) -> Vec { let mut files: Vec = workspace_scripts(db) .iter() @@ -850,8 +851,10 @@ pub(crate) fn files_in_directory(db: &dyn Db, dir: &Utf8Path) -> Vec { files } -/// The workspace files anywhere under `dir`, approximating the order returned by -/// `list.files(dir, recursive = TRUE)`. +/// Returns workspace scripts below `dir` in `targets::tar_source()` load order. +/// +/// `list.files(recursive = TRUE)` sorts nested scripts by relative path. Case +/// folding matches [`collation_basename_key()`]. pub(crate) fn files_in_directory_recursive(db: &dyn Db, dir: &Utf8Path) -> Vec { let mut keyed: Vec<(String, File)> = workspace_scripts(db) .iter() @@ -866,10 +869,11 @@ pub(crate) fn files_in_directory_recursive(db: &dyn Db, dir: &Utf8Path) -> Vec Option { file.path(db) .file_name() diff --git a/crates/oak_db/src/package.rs b/crates/oak_db/src/package.rs index 231685a9e..27a5db0c2 100644 --- a/crates/oak_db/src/package.rs +++ b/crates/oak_db/src/package.rs @@ -56,13 +56,13 @@ pub struct Package { /// editing a package's `NAMESPACE` live. #[returns(ref)] pub namespace_override: Option, - /// R source files belonging to this package (the `R/*.R` files), in - /// R's load order. When DESCRIPTION's `Collate:` directive is - /// present, this is exactly the files it lists, in that order; - /// files in `R/` not listed are excluded (matching R's loader, - /// Writing R Extensions §1.1.1). When `Collate:` is absent, files - /// are in case-insensitive alphabetical order. TODO(diagnostics): - /// Lint files missing from collation. + /// R source files belonging to this package (the `R/*.R` files), in R's + /// load order. When DESCRIPTION's `Collate:` directive is present, this is + /// exactly the files it lists, in that order; files in `R/` not listed are + /// excluded (matching R's loader, Writing R Extensions §1.1.1). When + /// `Collate:` is absent, files are in raw basename byte order, matching R's + /// `LC_COLLATE=C` installation collation. + /// TODO(diagnostics): Lint files missing from collation. /// /// Per-package granularity: adding or removing a file in one /// package doesn't invalidate tracked queries reading another diff --git a/crates/oak_db/src/tests/file_imports_at.rs b/crates/oak_db/src/tests/file_imports_at.rs index 07cc897b9..39b291166 100644 --- a/crates/oak_db/src/tests/file_imports_at.rs +++ b/crates/oak_db/src/tests/file_imports_at.rs @@ -862,9 +862,9 @@ fn test_script_r_directory_top_level_sees_only_alphabetic_predecessor() { #[test] fn test_script_r_directory_collation_is_case_insensitive() { - // Matches `oak_scan::packages::order_alphabetically`: basenames sort - // case-insensitively, so `a.R` collates before `Z.R` even though it's - // lexically greater in byte order. + // Non-package `R/` files use `list.files()` collation, so `a.R` precedes + // `Z.R` in a UTF-8 session locale. Package installation's `LC_COLLATE=C` + // reverses them. let mut db = TestDb::new(); let root = workspace_root(&db, "ws"); let z_source = "z_val <- 1\n"; diff --git a/crates/oak_scan/src/packages.rs b/crates/oak_scan/src/packages.rs index 143efebf7..a79011b7d 100644 --- a/crates/oak_scan/src/packages.rs +++ b/crates/oak_scan/src/packages.rs @@ -192,11 +192,11 @@ fn read_workspace_package(package_dir: &Path) -> Option { } } - // `file_imports()` in `oak_db` reads `package.files` order as the collation - // chain, so a file only sees the files ordered before it. `Collate:` is R's - // explicit load order; without it R loads `R/` in case-insensitive - // alphabetical order. R/ files left out of a `Collate:` directive aren't - // loaded into the namespace, so they move to `scripts` rather than `files`. + // `package.files` supplies cross-file lookup order, so a file sees only its + // predecessors. `Collate:` supplies that order explicitly. Without it, + // `tools:::.install_package_code_files()` forces `LC_COLLATE=C` and lists + // `R/` in byte order. Files omitted from `Collate:` are not loaded into the + // namespace and become `scripts`. let (loadable, leftover) = match package.collation.as_deref() { Some(order) => order_by_collation(files, order), None => order_alphabetically(files), @@ -308,8 +308,7 @@ fn order_by_collation( ) } -/// All files are loadable, in case-insensitive alphabetical order by basename. -/// No leftover: without `Collate:`, R loads every R/ file. +/// Without `Collate:`, every `R/` file is loadable in raw basename byte order. fn order_alphabetically(mut files: Vec<(PathBuf, FileEntry)>) -> (Vec, Vec) { files.sort_by_cached_key(|(path, _)| basename_key(path)); ( @@ -318,9 +317,11 @@ fn order_alphabetically(mut files: Vec<(PathBuf, FileEntry)>) -> (Vec ) } -/// Case-insensitive sort key from a path's basename. +/// Raw basename byte-order key for R's `LC_COLLATE=C` package installation. +/// `AllGenerics.R` precedes `aaa-utils.R`, letting S4 packages load generics +/// before methods. fn basename_key(path: &Path) -> Option { - path.file_name().map(|name| name.to_ascii_lowercase()) + path.file_name().map(|name| name.to_os_string()) } /// Walk a workspace root for its top-level scripts: every `.R` file that isn't diff --git a/crates/oak_scan/src/tests/packages.rs b/crates/oak_scan/src/tests/packages.rs index 299c0326a..5467311e2 100644 --- a/crates/oak_scan/src/tests/packages.rs +++ b/crates/oak_scan/src/tests/packages.rs @@ -51,17 +51,26 @@ fn test_empty_r_directory_yields_no_files() { assert!(scripts.is_empty()); } -/// Without `Collate:`, every R file is loadable, ordered case-insensitively by -/// basename so the result doesn't depend on `read_dir` order. +/// Without `Collate:`, R loads every file in raw basename byte order. +/// Under `LC_COLLATE=C`, `AllGenerics.R` precedes `aaa-utils.R`, so S4 packages +/// define generics before methods. #[test] fn test_without_collation_all_files_are_loadable_and_sorted() { let tmp = tempfile::tempdir().unwrap(); let r = tmp.path().join("R"); - write_r_dir(&r, &[("zebra.R", "1"), ("Apple.R", "1"), ("mango.R", "1")]); + write_r_dir(&r, &[ + ("zebra.R", "1"), + ("aaa-utils.R", "1"), + ("AllGenerics.R", "1"), + ]); let (files, scripts) = read_package_sources(&r, None); - assert_eq!(names(&files), vec!["Apple.R", "mango.R", "zebra.R"]); + assert_eq!(names(&files), vec![ + "AllGenerics.R", + "aaa-utils.R", + "zebra.R" + ]); assert!(scripts.is_empty()); } diff --git a/crates/oak_scan/src/tests/workspace.rs b/crates/oak_scan/src/tests/workspace.rs index 2bb1beea1..c2adf6003 100644 --- a/crates/oak_scan/src/tests/workspace.rs +++ b/crates/oak_scan/src/tests/workspace.rs @@ -633,20 +633,20 @@ fn test_set_workspace_paths_stale_no_duplicates_across_cycles() { #[test] fn test_scan_orders_files_alphabetically_without_collate() { - // Default behavior: case-insensitive alphabetical filename order. - // Matches R's load order for packages without an explicit `Collate:`. + // Package installation uses raw basename byte order under `LC_COLLATE=C` + // when `Collate:` is absent. let tmp = tempfile::tempdir().unwrap(); write_package(&tmp.path().join("pkg"), "pkg", &[ ("zzz.R", "z <- 1\n"), ("aaa.R", "a <- 1\n"), - ("mmm.R", "m <- 1\n"), + ("Mmm.R", "m <- 1\n"), ]); let mut db = OakDatabase::new(); set_workspace_paths(&mut db, &[tmp.path().to_path_buf()], &HashSet::new()); let pkg = db.workspace_roots().roots(&db)[0].packages(&db)[0]; - assert_eq!(file_basenames(&db, pkg), vec!["aaa.R", "mmm.R", "zzz.R"]); + assert_eq!(file_basenames(&db, pkg), vec!["Mmm.R", "aaa.R", "zzz.R"]); } #[test] From 0dbd67f5f0cf9b0d9919f55c0bb29a0420c17933 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 4 Aug 2026 13:34:23 +0200 Subject: [PATCH 8/8] Perform full argument matching in effects handlers --- crates/oak_db/src/tests/file_diagnostics.rs | 15 +- ...diagnostic_gap_named_arg_before_block.snap | 6 - ...med_arg_before_block_scopes_correctly.snap | 13 ++ crates/oak_semantic/src/effects.rs | 188 ++++++------------ crates/oak_semantic/src/effects/contrib.rs | 40 ++-- .../oak_semantic/src/effects/contrib/base.rs | 22 +- .../src/effects/contrib/base/bquote.rs | 19 +- .../src/effects/contrib/base/library.rs | 19 +- .../src/effects/contrib/base/substitute.rs | 15 +- .../oak_semantic/src/effects/contrib/rlang.rs | 4 +- .../oak_semantic/src/effects/contrib/shiny.rs | 14 +- .../src/effects/contrib/targets.rs | 3 +- .../src/effects/contrib/testthat.rs | 3 +- .../oak_semantic/src/effects/contrib/withr.rs | 2 +- .../tests/integration/contrib/base.rs | 14 +- .../tests/integration/contrib/targets.rs | 37 +++- 16 files changed, 178 insertions(+), 236 deletions(-) delete mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_named_arg_before_block.snap create mode 100644 crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_named_arg_before_block_scopes_correctly.snap diff --git a/crates/oak_db/src/tests/file_diagnostics.rs b/crates/oak_db/src/tests/file_diagnostics.rs index cfac4b597..c609b9409 100644 --- a/crates/oak_db/src/tests/file_diagnostics.rs +++ b/crates/oak_db/src/tests/file_diagnostics.rs @@ -208,17 +208,10 @@ f <- function() reactive({ x <- 1 }) } #[test] -fn test_diagnostic_gap_named_arg_before_block() { - // R matches named arguments first, so `desc = "d"` binds to the `desc` - // formal, and the unnamed block then fills the remaining `code` formal, - // which is formal position 1, even though the block sits at call - // position 0. `match_positional()` in `crates/oak_semantic/src/effects.rs` - // only matches a positional argument to a formal declared at that exact - // call position, so it never finds `code` here and no scope gets pushed - // for `x <- 1`. Confirmed by direct comparison: this source yields one - // scope, versus two for the same call with `code` in its normal - // position, so `x` resolves at file scope instead of inside - // `test_that()`. +fn test_diagnostic_named_arg_before_block_scopes_correctly() { + // After `desc` binds by name, the block fills `code` despite appearing first. + // Its `test_that()` resolution is conditionally shadowed and must still gain + // the `code` scope. let mut db = TestDb::new(); install_packages(&mut db, &["testthat"]); let source = "\ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_named_arg_before_block.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_named_arg_before_block.snap deleted file mode 100644 index 879ba7dc2..000000000 --- a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_named_arg_before_block.snap +++ /dev/null @@ -1,6 +0,0 @@ ---- -source: crates/oak_db/src/tests/file_diagnostics.rs -expression: "render(\"a.R\", source, file.diagnostics(&db))" ---- -a.R -(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_named_arg_before_block_scopes_correctly.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_named_arg_before_block_scopes_correctly.snap new file mode 100644 index 000000000..766121e93 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_named_arg_before_block_scopes_correctly.snap @@ -0,0 +1,13 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +info[ambiguous-effect]: Ambiguous reading of effectful `test_that()`. + A conditional assignment could shadow `test_that` on some paths and change its effect. + --> a.R:5:1 + | +3 | test_that <- identity + | --------- conditional assignment to `test_that` +4 | } +5 | test_that({ x <- 1 }, desc = "d") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/crates/oak_semantic/src/effects.rs b/crates/oak_semantic/src/effects.rs index 0d34648c5..cb31471bd 100644 --- a/crates/oak_semantic/src/effects.rs +++ b/crates/oak_semantic/src/effects.rs @@ -116,7 +116,8 @@ pub fn source_dir_idiom(name: &str) -> Option<&'static EffectsHandlers> { arguments: None, attach: None, source: Some(&SourceAnnotation { - position: 0, + formals: &["path"], + path: "path", target: SourceTarget::Dir(DirWalk::Shallow), default_path: None, }), @@ -224,13 +225,10 @@ impl<'a> CallContext<'a> { self.scope.is_none_or(|scope| scope.is_global()) } - /// Match `call`'s arguments to `formals`, returning for each call argument - /// in order the index into `formals` it bound to. Named arguments match - /// first, then the rest fill by position. - /// - /// A stopgap: without the callee's full formal list, a positional argument - /// only binds a formal declared at that exact position. - pub fn match_arguments(&self, call: &RCall, formals: &[Formal]) -> Vec> { + /// Match `call` arguments to `formals`, returning a formal index for each + /// call argument. Exact named matches consume slots first, then unnamed + /// arguments fill unconsumed slots in signature order. + pub fn match_arguments(&self, call: &RCall, formals: Formals) -> Vec> { let Ok(args) = call.arguments() else { return Vec::new(); }; @@ -251,21 +249,21 @@ impl<'a> CallContext<'a> { // Positional pass. Only unnamed args reach the match, and none of them // were set by the named pass, so no need to re-check `matched[i]`. - let mut position = 0usize; + let mut next_slot = 0usize; for (i, item) in items.iter().enumerate() { - let Ok(arg) = item else { - position += 1; - continue; - }; + let Ok(arg) = item else { continue }; if arg.name_clause().is_some() { - position += 1; continue; } - if let Some(formal_idx) = match_positional(formals, position, &consumed) { - consumed[formal_idx] = true; - matched[i] = Some(formal_idx); + while next_slot < consumed.len() && consumed[next_slot] { + next_slot += 1; } - position += 1; + let Some(formal_idx) = (next_slot < formals.len()).then_some(next_slot) else { + continue; + }; + consumed[formal_idx] = true; + matched[i] = Some(formal_idx); + next_slot += 1; } matched @@ -301,17 +299,10 @@ impl<'a> CallContext<'a> { } } -/// A formal a handler wants to locate in a call, by name and by its position in -/// the callee's signature. -/// -/// TODO(nse): `position` is a stopgap that stems from our annotation registry -/// listing only its scoped formals. Once `match_arguments` is signature-aware -/// it gets the callee's full ordered formals, and this collapses to a list of -/// names where the index is the position. -pub struct Formal { - pub name: &'static str, - pub position: usize, -} +/// The initial formal names needed to match the arguments this handler reads. +/// Include every earlier slot so unnamed arguments bind correctly. Stop before +/// `...`, because later R formals are matched by name rather than position. +pub type Formals = &'static [&'static str]; /// A call's resolved argument effects: for each argument in call order, the /// effect it resolved to, or `None` for a plain (standard-eval) argument. @@ -334,14 +325,13 @@ pub enum ResolvedArgumentEffect { /// default [`EffectHandler`] for it by matching the declaration to a call. #[derive(Debug, Clone, Copy)] pub struct ArgumentsAnnotation { + pub formals: Formals, pub arguments: &'static [Argument], } -/// A single annotated argument: its effect, plus where to find it in a call. #[derive(Debug)] pub struct Argument { pub name: &'static str, - pub position: usize, pub effect: ArgumentEffect, } @@ -373,21 +363,17 @@ impl EffectHandler for ArgumentsAnnotation { type Output = ResolvedArgumentEffects; fn resolve(&self, call: &RCall, ctx: &CallContext<'_>) -> Option { - let arguments = self.arguments; - let formals: Vec = arguments - .iter() - .map(|arg| Formal { - name: arg.name, - position: arg.position, - }) - .collect(); - - // The match yields a formal index per call argument - let matched = ctx.match_arguments(call, &formals); + let matched = ctx.match_arguments(call, self.formals); Some( matched .into_iter() - .map(|formal| formal.map(|i| arguments[i].effect.resolve())) + .map(|formal_idx| { + let name = self.formals[formal_idx?]; + self.arguments + .iter() + .find(|argument| argument.name == name) + .map(|argument| argument.effect.resolve()) + }) .collect(), ) } @@ -425,10 +411,8 @@ pub enum DirWalk { /// as the default [`EffectHandler`] for it by pulling that path out of a call. #[derive(Debug, Clone, Copy)] pub struct SourceAnnotation { - /// Which positional argument holds the path, counting only unnamed - /// arguments (0 for base `source`). Other source-like functions may put the - /// path elsewhere, so it's configured per entry rather than assumed. - pub position: usize, + pub formals: Formals, + pub path: &'static str, /// Whether that argument names a file or a directory. pub target: SourceTarget, /// Default path if no argument is suppolied (`tar_source()` defaults to @@ -440,70 +424,39 @@ impl EffectHandler for SourceAnnotation { type Output = Vec; fn resolve(&self, call: &RCall, ctx: &CallContext<'_>) -> Option> { + let matched = ctx.match_arguments(call, self.formals); let args = call.arguments().ok()?; + let values: Vec> = args + .items() + .iter() + .map(|item| item.ok().and_then(|arg| arg.value())) + .collect(); - if args.items().iter().next().is_none() { - return self.default_path.map(|path| { - vec![SourcePath { - path: path.to_string(), - target: self.target, - }] - }); - } - - // The path is matched positionally among unnamed arguments rather than - // through [`CallContext::match_arguments`], for two reasons. We need to - // inspect the `local =` value to bail on non-static calls, which - // argument matching doesn't do. And counting unnamed arguments is robust - // to a named argument coming first (e.g. `source(echo = TRUE, "x.R")`), - // which the call-position matching isn't yet. A named `file =` therefore - // isn't recognized today. - // - // TODO(nse): once `match_arguments` is signature-aware (see `Formal`), - // the leading-named-arg robustness comes for free and this scan could - // fold onto it, keeping only the `local =` bail on top. - let mut path: Option = None; - let mut positional = 0; - - for item in args.items().iter() { - let Ok(arg) = item else { continue }; - - if let Some(name_clause) = arg.name_clause() { - let Ok(AnyRArgumentName::RIdentifier(name_ident)) = name_clause.name() else { - continue; - }; - if name_ident.name_text() == "local" { - if let Some(value) = arg.value() { - match value { - // TRUE/FALSE are fine, we resolve uniformly. For - // the FALSE in nested context case, we'll emit a - // diagnostic. - AnyRExpression::RTrueExpression(_) | - AnyRExpression::RFalseExpression(_) => {}, - // Anything else (environment, non-statically - // resolvable expression) means the call isn't - // statically analyzable, so it's not recognized. - _ => return None, - } - } - } - continue; - } + let bound_value = |formal_name: &str| -> Option<&AnyRExpression> { + let formal_idx = self.formals.iter().position(|name| *name == formal_name)?; + let call_idx = matched.iter().position(|idx| *idx == Some(formal_idx))?; + values[call_idx].as_ref() + }; - if positional == self.position { - path = arg - .value() - .and_then(|value| ctx.resolve_static_string(&value)); + if let Some(local) = bound_value("local") { + match local { + AnyRExpression::RTrueExpression(_) | AnyRExpression::RFalseExpression(_) => {}, + // Only literal `TRUE` and `FALSE` make the source scope statically + // known. + _ => return None, } - positional += 1; } - path.map(|path| { - vec![SourcePath { - path, - target: self.target, - }] - }) + let path = match bound_value(self.path) { + // An explicit dynamic path suppresses the default. + Some(value) => ctx.resolve_static_string(value)?, + None => self.default_path?.to_string(), + }; + + Some(vec![SourcePath { + path, + target: self.target, + }]) } } @@ -530,9 +483,6 @@ impl AssignHandler for AssignAnnotation { // `assign(x, value, ...)`). // // FIXME: A named `value =` isn't captured yet. - // TODO(nse): Fold onto `match_arguments()` once it's signature-aware, - // same as `source` (see `SourceAnnotation::resolve`), keeping only the - // `envir`/`pos` bail and the value-after-name read on top. let mut name: Option<(String, RangedAstPtr)> = None; let mut value_expr: Option> = None; let mut positional = 0; @@ -607,7 +557,7 @@ impl AssignHandler for BindingOperatorHandler { /// formal. /// /// Should we do partial argument matching? Or rely on partial matching being linted? -fn match_named(arg: &RArgument, formals: &[Formal], consumed: &[bool]) -> Option { +fn match_named(arg: &RArgument, formals: Formals, consumed: &[bool]) -> Option { let clause = arg.name_clause()?; let name = clause.name().ok()?; let name_text = match &name { @@ -618,24 +568,6 @@ fn match_named(arg: &RArgument, formals: &[Formal], consumed: &[bool]) -> Option formals .iter() .enumerate() - .find(|(i, formal)| !consumed[*i] && formal.name == name_text.as_str()) - .map(|(i, _)| i) -} - -/// Match an unnamed argument at `position` against `formals`. Returns the index -/// of the matched formal. -/// -/// FIXME: This matches positionally on call-site position only: an unnamed -/// argument at position N matches a formal declared at position N. It doesn't -/// replicate R's full matching, where named arguments are pulled out first and -/// the rest fill the remaining formals in order. So `test_that({ ... }, desc = -/// "d")`, with the block at position 0 but the `code` formal at position 1, -/// won't match. Good enough without the callee's formal list; revisit if it -/// misses real cases. -fn match_positional(formals: &[Formal], position: usize, consumed: &[bool]) -> Option { - formals - .iter() - .enumerate() - .find(|(i, formal)| !consumed[*i] && formal.position == position) + .find(|(i, formal_name)| !consumed[*i] && **formal_name == name_text.as_str()) .map(|(i, _)| i) } diff --git a/crates/oak_semantic/src/effects/contrib.rs b/crates/oak_semantic/src/effects/contrib.rs index 6a626c156..86eb77a0d 100644 --- a/crates/oak_semantic/src/effects/contrib.rs +++ b/crates/oak_semantic/src/effects/contrib.rs @@ -22,17 +22,19 @@ pub(crate) struct PackageEntries { pub(super) functions: &'static [Entry], } -/// An NSE entry. Each `(name, position, scope, laziness)` tuple is a scoped -/// argument; list more than one for a function that scopes several. +/// Declares non-standard evaluation effects. macro_rules! nse { - ($func:literal, $(($name:literal, $pos:literal, $scope:expr, $timing:expr)),+ $(,)?) => { + ($func:literal, $(($name:literal, $scope:expr, $timing:expr)),+ $(,)?) => { + $crate::effects::contrib::nse!($func, [$($name),+], $(($name, $scope, $timing)),+) + }; + ($func:literal, [$($formal:literal),+ $(,)?], $(($name:literal, $scope:expr, $timing:expr)),+ $(,)?) => { $crate::effects::contrib::Entry { function: $func, effects: $crate::effects::EffectsHandlers { arguments: Some(&$crate::effects::ArgumentsAnnotation { + formals: &[$($formal),+], arguments: &[$($crate::effects::Argument { name: $name, - position: $pos, effect: $crate::effects::ArgumentEffect::EvalQ { env: $scope, timing: $timing, @@ -48,18 +50,19 @@ macro_rules! nse { } pub(crate) use nse; -/// A quoted entry. Each `(name, position)` names an argument captured -/// unevaluated: its symbols aren't uses and nothing in it runs. `quote`, -/// `bquote`. +/// Declares arguments that remain unevaluated. macro_rules! quoted { - ($func:literal, $(($name:literal, $pos:literal)),+ $(,)?) => { + ($func:literal, $($name:literal),+ $(,)?) => { + $crate::effects::contrib::quoted!($func, [$($name),+], $($name),+) + }; + ($func:literal, [$($formal:literal),+ $(,)?], $($name:literal),+ $(,)?) => { $crate::effects::contrib::Entry { function: $func, effects: $crate::effects::EffectsHandlers { arguments: Some(&$crate::effects::ArgumentsAnnotation { + formals: &[$($formal),+], arguments: &[$($crate::effects::Argument { name: $name, - position: $pos, effect: $crate::effects::ArgumentEffect::Quote, }),+], }), @@ -72,23 +75,30 @@ macro_rules! quoted { } pub(crate) use quoted; -/// Declares a source function's path argument, [`SourceTarget`], and -/// no-argument default. The target defaults to [`SourceTarget::File`]. +/// Declares a source function's signature prefix, path formal, [`SourceTarget`], +/// and no-argument default. The target defaults to [`SourceTarget::File`]. /// /// [`SourceTarget`]: crate::effects::SourceTarget /// [`SourceTarget::File`]: crate::effects::SourceTarget::File macro_rules! source { - ($func:literal, $pos:literal) => { - $crate::effects::contrib::source!($func, $pos, $crate::effects::SourceTarget::File, None) + ($func:literal, [$($formal:literal),+ $(,)?], $path:literal) => { + $crate::effects::contrib::source!( + $func, + [$($formal),+], + $path, + $crate::effects::SourceTarget::File, + None + ) }; - ($func:literal, $pos:literal, $target:expr, $default:expr) => { + ($func:literal, [$($formal:literal),+ $(,)?], $path:literal, $target:expr, $default:expr) => { $crate::effects::contrib::Entry { function: $func, effects: $crate::effects::EffectsHandlers { arguments: None, attach: None, source: Some(&$crate::effects::SourceAnnotation { - position: $pos, + formals: &[$($formal),+], + path: $path, target: $target, default_path: $default, }), diff --git a/crates/oak_semantic/src/effects/contrib/base.rs b/crates/oak_semantic/src/effects/contrib/base.rs index 20ef45f42..f76318acd 100644 --- a/crates/oak_semantic/src/effects/contrib/base.rs +++ b/crates/oak_semantic/src/effects/contrib/base.rs @@ -19,18 +19,22 @@ use crate::semantic_index::EvalTiming::Lazy; pub(crate) static ENTRIES: &[Entry] = &[ // base NSE - nse!("evalq", ("expr", 0, Current, Eager)), + nse!("evalq", ("expr", Current, Eager)), // `on.exit(expr)` captures `expr` and runs it in the current function's // frame when the function exits. Bindings land in that frame (`Current`) at // an unknown later time (`Lazy`), the same shape as `rlang::on_load()`. - nse!("on.exit", ("expr", 0, Current, Lazy)), - nse!("local", ("expr", 0, Nested, Eager)), - nse!("with", ("expr", 1, Nested, Eager)), - nse!("with.default", ("expr", 1, Nested, Eager)), - nse!("within", ("expr", 1, Nested, Eager)), - nse!("within.data.frame", ("expr", 1, Nested, Eager)), + nse!("on.exit", ("expr", Current, Lazy)), + nse!("local", ("expr", Nested, Eager)), + nse!("with", ["data", "expr"], ("expr", Nested, Eager)), + nse!("with.default", ["data", "expr"], ("expr", Nested, Eager)), + nse!("within", ["data", "expr"], ("expr", Nested, Eager)), + nse!( + "within.data.frame", + ["data", "expr"], + ("expr", Nested, Eager) + ), // base quote - quoted!("quote", ("expr", 0)), + quoted!("quote", "expr"), // `bquote` quotes `expr` too, but its `.()` holes escape to evaluation, so // it needs a handler rather than a static per-argument effect. Entry { @@ -58,7 +62,7 @@ pub(crate) static ENTRIES: &[Entry] = &[ attach_entry("library"), attach_entry("require"), // base source - source!("source", 0), + source!("source", ["file", "local"], "file"), // base assign assign!("assign", 0), assign!("delayedAssign", 0), diff --git a/crates/oak_semantic/src/effects/contrib/base/bquote.rs b/crates/oak_semantic/src/effects/contrib/base/bquote.rs index 3bdc8a40b..a950c2259 100644 --- a/crates/oak_semantic/src/effects/contrib/base/bquote.rs +++ b/crates/oak_semantic/src/effects/contrib/base/bquote.rs @@ -7,7 +7,7 @@ use oak_core::syntax_ext::RIdentifierExt; use crate::effects::CallContext; use crate::effects::EffectHandler; -use crate::effects::Formal; +use crate::effects::Formals; use crate::effects::ResolvedArgumentEffect; use crate::effects::ResolvedArgumentEffects; @@ -22,19 +22,8 @@ impl EffectHandler for BquoteHandler { type Output = ResolvedArgumentEffects; fn resolve(&self, call: &RCall, ctx: &CallContext<'_>) -> Option { - // `bquote(expr, where, splice)`: only `expr` (the first positional) is - // quoted. The other arguments are ordinary values. - let formals = [ - Formal { - name: "expr", - position: 0, - }, - Formal { - name: "splice", - position: 2, - }, - ]; - let matched = ctx.match_arguments(call, &formals); + let formals: Formals = &["expr", "where", "splice"]; + let matched = ctx.match_arguments(call, formals); let args = call.arguments().ok()?; let values: Vec> = args @@ -46,7 +35,7 @@ impl EffectHandler for BquoteHandler { // `..()` only splices under `splice = TRUE`. let splice = matched .iter() - .position(|formal| *formal == Some(1)) + .position(|formal| *formal == Some(2)) .and_then(|i| values.get(i)) .and_then(|value| value.as_ref()) .and_then(|value| ctx.resolve_static_bool(value)) diff --git a/crates/oak_semantic/src/effects/contrib/base/library.rs b/crates/oak_semantic/src/effects/contrib/base/library.rs index b63e0831a..44bdfcf73 100644 --- a/crates/oak_semantic/src/effects/contrib/base/library.rs +++ b/crates/oak_semantic/src/effects/contrib/base/library.rs @@ -4,7 +4,7 @@ use biome_rowan::AstSeparatedList; use crate::effects::CallContext; use crate::effects::EffectHandler; -use crate::effects::Formal; +use crate::effects::Formals; /// Handler for `library()` and `require()`. Names the attached package from the /// first argument, read as quoted (the symbol or string as written, so @@ -19,19 +19,8 @@ impl EffectHandler for LibraryHandler { type Output = String; fn resolve(&self, call: &RCall, ctx: &CallContext<'_>) -> Option { - // `character.only` sits at signature position 4 in both callees; in - // practice it's passed by name. - let formals = [ - Formal { - name: "package", - position: 0, - }, - Formal { - name: "character.only", - position: 4, - }, - ]; - let matched = ctx.match_arguments(call, &formals); + let formals: Formals = &["package", "help", "pos", "lib.loc", "character.only"]; + let matched = ctx.match_arguments(call, formals); let args = call.arguments().ok()?; let values: Vec> = args @@ -48,7 +37,7 @@ impl EffectHandler for LibraryHandler { let character_only = matched .iter() - .position(|formal| *formal == Some(1)) + .position(|formal| *formal == Some(4)) .and_then(|i| values.get(i)) .and_then(|value| value.as_ref()) .and_then(|value| ctx.resolve_static_bool(value)) diff --git a/crates/oak_semantic/src/effects/contrib/base/substitute.rs b/crates/oak_semantic/src/effects/contrib/base/substitute.rs index 9203a8325..6de2f16e2 100644 --- a/crates/oak_semantic/src/effects/contrib/base/substitute.rs +++ b/crates/oak_semantic/src/effects/contrib/base/substitute.rs @@ -10,7 +10,7 @@ use oak_core::syntax_ext::RIdentifierExt; use crate::effects::CallContext; use crate::effects::EffectHandler; -use crate::effects::Formal; +use crate::effects::Formals; use crate::effects::ResolvedArgumentEffect; use crate::effects::ResolvedArgumentEffects; @@ -28,17 +28,8 @@ impl EffectHandler for SubstituteHandler { fn resolve(&self, call: &RCall, ctx: &CallContext<'_>) -> Option { // `substitute(expr, env)`: only `expr` (formal 0) is quoted, everything // else is a plain value. - let formals = [ - Formal { - name: "expr", - position: 0, - }, - Formal { - name: "env", - position: 1, - }, - ]; - let matched = ctx.match_arguments(call, &formals); + let formals: Formals = &["expr", "env"]; + let matched = ctx.match_arguments(call, formals); let expr_pos = matched.iter().position(|formal| *formal == Some(0))?; // Only the default `env`, the current frame, is one we can query. Any diff --git a/crates/oak_semantic/src/effects/contrib/rlang.rs b/crates/oak_semantic/src/effects/contrib/rlang.rs index aee91ce5e..9f6ef2b00 100644 --- a/crates/oak_semantic/src/effects/contrib/rlang.rs +++ b/crates/oak_semantic/src/effects/contrib/rlang.rs @@ -7,9 +7,9 @@ use crate::semantic_index::EvalTiming::Lazy; pub(crate) static ENTRIES: &[Entry] = &[ assign_op!("%<~%", Write), - nse!("on_load", ("expr", 0, Current, Lazy)), + nse!("on_load", ("expr", Current, Lazy)), // `defer(expr, env = caller_env())` runs `expr` in the caller frame when it // exits. Written in a function, that frame is the function itself, so it's // `Current + Lazy` like `on.exit`. A non-default `env` isn't modeled. - nse!("defer", ("expr", 0, Current, Lazy)), + nse!("defer", ("expr", Current, Lazy)), ]; diff --git a/crates/oak_semantic/src/effects/contrib/shiny.rs b/crates/oak_semantic/src/effects/contrib/shiny.rs index 1d9002429..abbd03199 100644 --- a/crates/oak_semantic/src/effects/contrib/shiny.rs +++ b/crates/oak_semantic/src/effects/contrib/shiny.rs @@ -4,11 +4,11 @@ use crate::semantic_index::EvalEnv::Nested; use crate::semantic_index::EvalTiming::Lazy; pub(crate) static ENTRIES: &[Entry] = &[ - nse!("observe", ("x", 0, Nested, Lazy)), - nse!("reactive", ("x", 0, Nested, Lazy)), - nse!("renderPlot", ("expr", 0, Nested, Lazy)), - nse!("renderPrint", ("expr", 0, Nested, Lazy)), - nse!("renderTable", ("expr", 0, Nested, Lazy)), - nse!("renderText", ("expr", 0, Nested, Lazy)), - nse!("renderUI", ("expr", 0, Nested, Lazy)), + nse!("observe", ("x", Nested, Lazy)), + nse!("reactive", ("x", Nested, Lazy)), + nse!("renderPlot", ("expr", Nested, Lazy)), + nse!("renderPrint", ("expr", Nested, Lazy)), + nse!("renderTable", ("expr", Nested, Lazy)), + nse!("renderText", ("expr", Nested, Lazy)), + nse!("renderUI", ("expr", Nested, Lazy)), ]; diff --git a/crates/oak_semantic/src/effects/contrib/targets.rs b/crates/oak_semantic/src/effects/contrib/targets.rs index f22ee6a2e..c3e2e9000 100644 --- a/crates/oak_semantic/src/effects/contrib/targets.rs +++ b/crates/oak_semantic/src/effects/contrib/targets.rs @@ -13,7 +13,8 @@ pub(super) static ENTRIES: &[Entry] = &[ // The scanner reads one literal even though `files` is a character vector. source!( "tar_source", - 0, + ["files"], + "files", SourceTarget::FileOrDir(DirWalk::Recursive), Some("R") ), diff --git a/crates/oak_semantic/src/effects/contrib/testthat.rs b/crates/oak_semantic/src/effects/contrib/testthat.rs index 66e471973..37e3a1467 100644 --- a/crates/oak_semantic/src/effects/contrib/testthat.rs +++ b/crates/oak_semantic/src/effects/contrib/testthat.rs @@ -3,4 +3,5 @@ use crate::effects::contrib::Entry; use crate::semantic_index::EvalEnv::Nested; use crate::semantic_index::EvalTiming::Eager; -pub(crate) static ENTRIES: &[Entry] = &[nse!("test_that", ("code", 1, Nested, Eager))]; +pub(crate) static ENTRIES: &[Entry] = + &[nse!("test_that", ["desc", "code"], ("code", Nested, Eager))]; diff --git a/crates/oak_semantic/src/effects/contrib/withr.rs b/crates/oak_semantic/src/effects/contrib/withr.rs index 27ce932e5..59c0d5daf 100644 --- a/crates/oak_semantic/src/effects/contrib/withr.rs +++ b/crates/oak_semantic/src/effects/contrib/withr.rs @@ -7,5 +7,5 @@ pub(crate) static ENTRIES: &[Entry] = &[ // `defer(expr, envir = parent.frame())` runs `expr` in the caller frame when // it exits, the same `Current + Lazy` shape as `on.exit`. A non-default // `envir` isn't modeled. - nse!("defer", ("expr", 0, Current, Lazy)), + nse!("defer", ("expr", Current, Lazy)), ]; diff --git a/crates/oak_semantic/tests/integration/contrib/base.rs b/crates/oak_semantic/tests/integration/contrib/base.rs index aa9e2e851..d71046c43 100644 --- a/crates/oak_semantic/tests/integration/contrib/base.rs +++ b/crates/oak_semantic/tests/integration/contrib/base.rs @@ -131,12 +131,12 @@ impl ImportsResolver for MultiFileResolver { } } -/// Resolves `source` to a [`SourceAnnotation`] whose path sits at the second -/// positional slot, exercising the configurable `position`. +/// Provides a `source()` annotation whose `file` path formal is second. struct PositionResolver; -static SOURCE_AT_POSITION_1: SourceAnnotation = SourceAnnotation { - position: 1, +static SOURCE_PATH_SECOND: SourceAnnotation = SourceAnnotation { + formals: &["ignored", "file"], + path: "file", target: SourceTarget::File, default_path: None, }; @@ -151,7 +151,7 @@ impl ImportsResolver for PositionResolver { return Some(EffectsHandlers { arguments: None, attach: None, - source: Some(&SOURCE_AT_POSITION_1), + source: Some(&SOURCE_PATH_SECOND), assign: None, }); } @@ -1347,9 +1347,7 @@ fn test_source_dir_with_no_files_records_the_path_unresolved() { } #[test] -fn test_source_resolver_honors_configured_path_position() { - // A `SourceAnnotation` with `position: 1` takes the path from the second - // positional argument, not the first. +fn test_source_resolver_finds_path_formal_not_first() { let index = build_test_index("source(\"ignored\", \"real.R\")", PositionResolver); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Source { path: "real.R".into(), diff --git a/crates/oak_semantic/tests/integration/contrib/targets.rs b/crates/oak_semantic/tests/integration/contrib/targets.rs index 58873925e..f55fa7e4f 100644 --- a/crates/oak_semantic/tests/integration/contrib/targets.rs +++ b/crates/oak_semantic/tests/integration/contrib/targets.rs @@ -122,15 +122,42 @@ fn test_tar_source_path_naming_a_script_resolves_as_a_file() { } #[test] -fn test_tar_source_named_files_argument_is_not_recognized() { - // `files = "code"` isn't read yet (the path is matched positionally), and - // the default must not step in for it. Inventing `"R"` here would source a - // directory the call explicitly overrode. - let files = vec![resolution("file:///R/a.R", "a_name")]; +fn test_tar_source_named_files_argument_is_recognized() { + let files = vec![resolution("file:///code/a.R", "a_name")]; let index = index( "tar_source(files = \"code\")\n", TargetsResolver::with_dir(files), ); + assert_eq!(semantic_call_kinds(&index), [&sourced( + "code", + "file:///code/a.R" + )]); +} + +#[test] +fn test_tar_source_change_directory_false_uses_the_default_directory() { + // `change_directory` does not bind `files`, so `files` uses its `"R"` default. + let files = vec![resolution("file:///R/a.R", "a_name")]; + let index = index( + "tar_source(change_directory = FALSE)\n", + TargetsResolver::with_dir(files), + ); + + assert_eq!(semantic_call_kinds(&index), [&sourced( + "R", + "file:///R/a.R" + )]); +} + +#[test] +fn test_tar_source_dynamic_files_argument_is_not_recognized() { + // A dynamic `files` value overrides the `"R"` default but produces no source call. + let files = vec![resolution("file:///R/a.R", "a_name")]; + let index = index( + "tar_source(files = some_var)\n", + TargetsResolver::with_dir(files), + ); + assert_eq!(semantic_call_kinds(&index), Vec::<&SemanticCallKind>::new()); }