diff --git a/crates/oak_db/src/definition.rs b/crates/oak_db/src/definition.rs index 02c4ce804..4a53cd7dd 100644 --- a/crates/oak_db/src/definition.rs +++ b/crates/oak_db/src/definition.rs @@ -1,5 +1,6 @@ use aether_syntax::RBinaryExpression; use aether_syntax::RSyntaxKind; +use aether_syntax::RSyntaxNode; use biome_rowan::AstNode; use biome_rowan::TextRange; use oak_semantic::semantic_index::DefinitionId; @@ -55,35 +56,40 @@ impl<'db> Definition<'db> { pub fn name_range(self, db: &'db dyn crate::Db) -> Option { let parse = self.file(db).parse(db); let root = parse.tree().syntax().clone(); - - let name_node = match self.kind(db) { - DefinitionKind::Assignment(ptr) | DefinitionKind::SuperAssignment(ptr) => { - let node = ptr.to_node(&root); - // Right-assign (`rhs -> x`, `rhs ->> x`) puts the target on - // the right, every other form (`x <- rhs`, `x <<- rhs`, - // `x = rhs`) puts it on the left. - let target = if is_right_assignment(&node) { - node.right().ok()? - } else { - node.left().ok()? - }; - target.into_syntax() - }, - DefinitionKind::Parameter(ptr) => { - let node = ptr.to_node(&root); - node.name().ok()?.into_syntax() - }, - DefinitionKind::ForVariable(ptr) => { - let node = ptr.to_node(&root); - node.variable().ok()?.into_syntax() - }, - DefinitionKind::Import { .. } => return None, - DefinitionKind::Assign { name, .. } => name.to_node(&root).into_syntax(), - }; - Some(name_node.text_trimmed_range()) + Some(name_node(self.kind(db), &root)?.text_trimmed_range()) } } +/// The syntax node of the bound name for `kind`. +fn name_node(kind: &DefinitionKind, root: &RSyntaxNode) -> Option { + let node = match kind { + DefinitionKind::Assignment(ptr) | DefinitionKind::SuperAssignment(ptr) => { + let node = ptr.to_node(root); + // Right-assign (`rhs -> x`, `rhs ->> x`) puts the target on + // the right, every other form (`x <- rhs`, `x <<- rhs`, + // `x = rhs`) puts it on the left. + let target = if is_right_assignment(&node) { + node.right().ok()? + } else { + node.left().ok()? + }; + target.into_syntax() + }, + DefinitionKind::Parameter(ptr) => { + let node = ptr.to_node(root); + node.name().ok()?.into_syntax() + }, + DefinitionKind::ForVariable(ptr) => { + let node = ptr.to_node(root); + node.variable().ok()?.into_syntax() + }, + DefinitionKind::Assign { name, .. } => name.to_node(root).into_syntax(), + // No name token at the binding site + DefinitionKind::Import { .. } => return None, + }; + Some(node) +} + fn is_right_assignment(node: &RBinaryExpression) -> bool { node.operator().is_ok_and(|op| { matches!( diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index c5f22627c..1ed1776de 100644 --- a/crates/oak_db/src/file.rs +++ b/crates/oak_db/src/file.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use aether_path::FilePath; use biome_line_index::LineIndex; use biome_rowan::TextRange; +use oak_semantic::semantic_index::AmbiguityReason; use oak_semantic::semantic_index::ScopeId; use oak_semantic::semantic_index::SemanticDiagnostic; use oak_semantic::semantic_index::SemanticIndex; @@ -362,13 +363,28 @@ fn build_semantic_index_inner(file: File, db: &dyn Db) -> SemanticIndex { let line_index = file.line_index(db); for diagnostic in diagnostics { - match diagnostic { - SemanticDiagnostic::LazyShadowAmbiguity { - name, - call_range, - overwrite_range, - } => { - let call = format_line_col(line_index, *call_range); + if let SemanticDiagnostic::AmbiguousAttachOrder { packages, range } = diagnostic { + let at = format_line_col(line_index, *range); + log::warn!( + "Ambiguous attach order in {path}:{at}: the branches attach {packages} in \ + different orders.", + packages = packages.join(", ") + ); + continue; + } + + let SemanticDiagnostic::EffectAmbiguity { + name, + call_range, + reason, + } = diagnostic + else { + continue; + }; + let call = format_line_col(line_index, *call_range); + + match reason { + AmbiguityReason::LazyShadow { overwrite_range } => { let overwrite = format_line_col(line_index, *overwrite_range); log::warn!( "Lazy-shadow ambiguity in {path}:{call}: callee `{name}` is recognized \ @@ -376,6 +392,25 @@ fn build_semantic_index_inner(file: File, db: &dyn Db) -> SemanticIndex { undetermined timing" ) }, + AmbiguityReason::ConditionalShadow { binding_range } => { + let binding = format_line_col(line_index, *binding_range); + log::warn!( + "Conditional-shadow ambiguity in {path}:{call}: callee `{name}` is \ + recognized as effectful, but a conditional local binding at {binding} \ + could shadow it on some path" + ) + }, + AmbiguityReason::ConditionalAttach { + package, + attach_range, + } => { + let attach = format_line_col(line_index, *attach_range); + log::warn!( + "Conditional-attach ambiguity in {path}:{call}: callee `{name}` is read as \ + plain because `{package}`, attached at {attach}, dropped at a branch or \ + loop join. It would be effectful on the path where that attach ran" + ) + }, } } } diff --git a/crates/oak_db/src/file_imports.rs b/crates/oak_db/src/file_imports.rs index 1a3820003..9e7228b07 100644 --- a/crates/oak_db/src/file_imports.rs +++ b/crates/oak_db/src/file_imports.rs @@ -4,8 +4,11 @@ use std::collections::HashMap; use biome_rowan::TextSize; use camino::Utf8Path; use oak_package_metadata::namespace::Namespace; +use oak_semantic::semantic_index::AttachRegion; +use oak_semantic::semantic_index::ScopeId; +use oak_semantic::semantic_index::SemanticCall; use oak_semantic::semantic_index::SemanticCallKind; -use oak_semantic::ScopeId; +use oak_semantic::semantic_index::SemanticIndex; use crate::Db; use crate::File; @@ -67,6 +70,72 @@ impl CrossFileLayers { } } +/// Which of a file's own `library()` attaches a caller sees. +#[derive(Clone, Copy)] +enum AttachView { + /// Every attach in the file. + /// + /// Over-approximates on two axes. An attach in a function body counts even + /// though the body may never run, and a conditional one counts even though + /// its branch may not have been taken. + Anywhere, + /// Attaches visible at `offset` in lazy `scope`. + /// + /// An attach in `scope_id` or an enclosing lazy body applies only after its + /// `library()` call. The lazy view treats an unconditional top-level attach + /// as visible regardless of position, which over-approximates this case: + /// + /// ```r + /// f <- function() { + /// cli_alert("x") + /// } + /// + /// f() + /// library(cli) + /// ``` + /// + /// `f()` runs before `library(cli)`, so the attach is unavailable at + /// `cli_alert()`. In the future, call analysis could detect that order. + /// + /// Conditional attaches remain limited to their arm, and child or sibling + /// bodies do not reach `scope_id`. + Lazy { offset: TextSize, scope_id: ScopeId }, + /// The attaches that have run and still hold at `offset` in an eagerly + /// evaluated scope. Calls reached only by running a lazy body are dropped, + /// as are calls after the offset. + Eager(TextSize), +} + +impl AttachView { + fn sees(&self, index: &SemanticIndex, call: &SemanticCall, region: &AttachRegion) -> bool { + match *self { + AttachView::Anywhere => true, + AttachView::Lazy { + offset, + scope_id: scope, + } => match index.enclosing_lazy_scope(call.scope()) { + // The lazy view treats unconditional top-level attaches as + // preceding every body. Conditional attaches stay in their arm. + None => match region { + AttachRegion::Unconditional => true, + AttachRegion::Conditional { .. } => region.contains(call, offset), + }, + // A lazy body's attach reaches only that body and its descendants, + // and only after the call returns. + Some(unit) => { + index + .ancestor_scope_ids(scope) + .any(|ancestor| ancestor == unit) && + region.contains(call, offset) + }, + }, + AttachView::Eager(offset) => { + index.scope_is_eager(call.scope()) && region.contains(call, offset) + }, + } + } +} + /// The point in a package's load at which a file views its collation siblings. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub(crate) enum CollationView { @@ -80,10 +149,12 @@ pub(crate) enum CollationView { #[salsa::tracked] impl File { - /// The import layers visible to this file at end-of-file, in R's lookup - /// (LIFO) priority order. Symbols that don't have local bindings (are - /// unbound in the file's semantic index) can be resolved against these - /// imports. + /// Every import layer this file could see, in R's lookup (LIFO) priority + /// order. Symbols that don't have local bindings (are unbound in the file's + /// semantic index) can be resolved against these imports. + /// + /// Over-approximates: every attach in the file contributes a layer, + /// including ones in a function body or in a branch that wasn't taken. /// /// `library()` calls further down the file come earlier in the returned /// `Vec`, and collation files later in the package come earlier too. The @@ -97,17 +168,16 @@ impl File { #[salsa::tracked(returns(ref))] pub fn imports(self, db: &dyn Db) -> Vec { let layers = self.cross_file_layers(db, CollationView::Lazy); - let own = self.attach_layers(db, None); + let own = self.attach_layers(db, AttachView::Anywhere); layers.splice_own_attaches(own) } /// Import layers visible at an `offset` in a file: /// - /// - **Cursor in lazy context**: returns the full lazy view. Lazy - /// contexts like functions are treated as if they run after the - /// file is fully sourced (over-approximation). Any `library()` / - /// collation entry is potentially visible regardless of where it - /// appears relative to the cursor. + /// - **Cursor in lazy context**: every collation sibling and unconditional + /// top-level attach is visible. Attaches from the current or enclosing lazy + /// body must precede the cursor, and conditional attaches remain limited to + /// the arm that attaches them. /// /// - **Top-level cursor (script)**: only `library()` calls that /// have occurred before `offset`. Most recently attached comes @@ -120,54 +190,51 @@ impl File { /// Plain method rather than `#[salsa::tracked]`. Tracking would key the /// cache on `(self, offset)`, creating one entry per cursor position. /// Skipping the cache is fine because the body just reads already-cached - /// subqueries (`imports`, `semantic_index`) and applies an O(n) filter. + /// subqueries (`cross_file_layers`, `semantic_index`) and applies an O(n) + /// filter. pub fn imports_at(self, db: &dyn Db, offset: TextSize) -> Vec { let index = self.semantic_index(db); - let file_scope = ScopeId::from(0); let (cursor_scope, _) = index.scope_at(offset); - // Cursor in lazy context. EOF view, same as `imports()`. - if cursor_scope != file_scope { - return self.imports(db).clone(); - } + // An eager scope runs during the file's own top-level execution, so a + // cursor in a `local()` block sees the search path as of that point, + // the same as one at file scope. + let (collation, attaches) = if index.scope_is_eager(cursor_scope) { + // Predecessors only, and own attaches narrowed to the calls that + // have run by `offset`. + (CollationView::Eager, AttachView::Eager(offset)) + } else { + (CollationView::Lazy, AttachView::Lazy { + offset, + scope_id: cursor_scope, + }) + }; - // Top-level cursor: predecessors only, and own attaches narrowed to the - // calls that have run by `offset`. - let layers = self.cross_file_layers(db, CollationView::Eager); - let own = self.attach_layers(db, Some(offset)); + let layers = self.cross_file_layers(db, collation); + let own = self.attach_layers(db, attaches); layers.splice_own_attaches(own) } /// This file's own `library()` / `require()` attaches as `Package` layers, - /// in LIFO order (latest-attached first). Reads the file's own semantic - /// index. `before` selects which calls to include: - /// - /// - `None`: every attach. The end-of-file view, used for lazy contexts. - /// - `Some(offset)`: only top-level (file-scope) calls that have run by - /// `offset`. Calls nested in a block (e.g. inside `test_that({})`) are - /// dropped, as are calls after the offset. + /// in LIFO order (latest-attached first), narrowed to what `view` admits. + /// Reads the file's own semantic index. /// /// An attach to a package absent from every root is dropped (no entity). - fn attach_layers(self, db: &dyn Db, before: Option) -> Vec { + fn attach_layers(self, db: &dyn Db, view: AttachView) -> Vec { let index = self.semantic_index(db); - let file_scope = ScopeId::from(0); index .semantic_calls() .iter() .rev() - .filter(|call| match before { - Some(offset) => call.scope() == file_scope && call.offset() < offset, - None => true, - }) .filter_map(|call| match call.kind() { - SemanticCallKind::Attach { package } => { - db.package_by_name(package).map(ImportLayer::Package) - }, + SemanticCallKind::Attach { package, region } => Some((call, package, region)), // A `library()` inside the sourced file is forwarded separately // by the semantic index builder as its own `Attach`, scoped to // this `source()`. SemanticCallKind::Source { .. } => None, }) + .filter(|(call, _, region)| view.sees(index, call, region)) + .filter_map(|(_, package, _)| db.package_by_name(package).map(ImportLayer::Package)) .collect() } diff --git a/crates/oak_db/src/tests/file_imports_at.rs b/crates/oak_db/src/tests/file_imports_at.rs index 6bbd77690..cc5929fd3 100644 --- a/crates/oak_db/src/tests/file_imports_at.rs +++ b/crates/oak_db/src/tests/file_imports_at.rs @@ -297,15 +297,13 @@ fn test_testthat_top_level_library_narrows_by_offset() { #[test] fn test_library_in_function_scoped_source_is_visible_only_in_that_function() { - // A `library()` inside a file that's `source()`d from a function body is - // forwarded by the builder as an `Attach` scoped to that `source()` call, - // so its attach layer is visible inside the function (the lazy / EOF view) - // but not at file scope before or after it. + // A sourced `library()` becomes an `Attach` in `source()`'s calling scope. + // It appears after `source()` returns, but does not escape the function. let mut db = TestDb::new(); install_packages(&mut db, &["dplyr"]); let helpers = make_file(&mut db, "w/helpers.R", "library(dplyr)\n"); - let script_src = "before\nf <- function() {\n source(\"helpers.R\")\n}\nafter\n"; + let script_src = "before\nf <- function() {\n source(\"helpers.R\")\n inside\n}\nafter\n"; let script = make_file(&mut db, "w/script.R", script_src); let root = workspace_root(&db, "w"); @@ -318,6 +316,422 @@ fn test_library_in_function_scoped_source_is_visible_only_in_that_function() { }; assert!(!at("before").contains(&"dplyr".to_string())); - assert!(at("source").contains(&"dplyr".to_string())); + assert!(!at("source").contains(&"dplyr".to_string())); + assert!(at("inside").contains(&"dplyr".to_string())); assert!(!at("after").contains(&"dplyr".to_string())); } + +/// The attaches visible at each `needle` in `source`, one entry per needle. +fn attaches_at(db: &TestDb, file: File, source: &str, needles: &[&str]) -> Vec> { + needles + .iter() + .map(|needle| { + let offset = TextSize::from(source.find(needle).unwrap() as u32); + library_attaches(db, &file.imports_at(db, offset)) + }) + .collect() +} + +#[test] +fn test_conditional_attach_holds_only_inside_its_branch() { + // `library(cli)` runs only when `cond` is true, so past the branch nothing + // says cli is attached. It covers its own arm and stops at the closing + // brace, the same narrowing the scan applies to `attached_so_far`. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "before\nif (cond) {\n library(cli)\n inside\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + let no_attach: Vec = Vec::new(); + assert_eq!( + attaches_at(&db, file, source, &["before", "inside", "after"]), + vec![no_attach.clone(), vec!["cli".to_string()], no_attach] + ); +} + +#[test] +fn test_conditional_attach_does_not_reach_the_sibling_branch() { + // The arm that attaches ends before the `else` starts, so the alternative + // resolves against the search path as it was before the `if`. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "if (cond) {\n library(cli)\n taken\n} else {\n other\n}\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["taken", "other"]), vec![ + vec!["cli".to_string()], + Vec::::new() + ]); +} + +#[test] +fn test_attach_on_both_branches_holds_after_the_if() { + // Both arms attach `cli`, so the join carries one attach past the `if`. + // This distinguishes effect regions from treating every attach inside an `if` + // as conditional. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "if (cond) {\n library(cli)\n} else {\n library(cli)\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["after"]), vec![vec![ + "cli".to_string() + ]]); +} + +#[test] +fn test_attach_on_both_branches_does_not_reach_earlier_uses_in_either_arm() { + // Each arm's attach applies only after its own call. The joined attach begins + // at the `else` call, so it does not reach `second`. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = + "if (cond) {\n first\n library(cli)\n} else {\n second\n library(cli)\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + let no_attach: Vec = Vec::new(); + assert_eq!( + attaches_at(&db, file, source, &["first", "second", "after"]), + vec![no_attach.clone(), no_attach, vec!["cli".to_string()]] + ); +} + +#[test] +fn test_join_matches_arms_per_package_not_wholesale() { + // The consequence attaches an extra package. Matching is per package, so cli + // carries past the `if` while rlang stays capped at the arm that attached it. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli", "rlang"]); + + let source = + "if (cond) {\n library(cli)\n library(rlang)\n inside\n} else {\n library(cli)\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside", "after"]), vec![ + vec!["rlang".to_string(), "cli".to_string()], + vec!["cli".to_string()] + ]); +} + +#[test] +fn test_join_caps_a_package_attached_only_by_the_else_arm() { + // Mirror of the above with the extra package in the `else`. Being the arm + // that closes the `if` doesn't carry rlang past it, since the consequence + // never attached it. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli", "rlang"]); + + let source = + "if (cond) {\n library(cli)\n} else {\n library(cli)\n library(rlang)\n inside\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside", "after"]), vec![ + vec!["rlang".to_string(), "cli".to_string()], + vec!["cli".to_string()] + ]); +} + +#[test] +fn test_join_takes_the_else_arm_order_when_the_arms_attach_in_different_orders() { + // Both arms attach both packages, so both carry past the `if`, but the arms + // disagree on which was attached last. One layer order has to stand for both + // paths, and it's the `else` arm's calls that carry the packages out. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli", "rlang"]); + + let source = "if (cond) {\n library(cli)\n library(rlang)\n} else {\n \ + library(rlang)\n library(cli)\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["after"]), vec![vec![ + "cli".to_string(), + "rlang".to_string() + ]]); +} + +#[test] +fn test_attach_rejoined_inside_an_arm_carries_through_the_outer_join() { + // The inner `if` rejoins cli onto its own `else` call, which the outer join + // then sees as the consequence arm's attach. So the outer `else` carries cli + // out, and the inner calls stay capped at the arm they ran in. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "if (a) {\n if (b) library(cli) else library(cli)\n} else {\n before\n \ + library(cli)\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["before", "after"]), vec![ + Vec::::new(), + vec!["cli".to_string()] + ]); +} + +#[test] +fn test_attach_on_every_arm_of_an_else_if_chain_holds_after_the_chain() { + // An `else if` nests a whole `if` in the alternative, so each join sees the + // arm below it already rejoined. The final `else` closes the outer `if` too, + // which is what lets its attach carry the chain. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "if (a) {\n first\n library(cli)\n} else if (b) {\n second\n \ + library(cli)\n} else {\n third\n library(cli)\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + let no_attach: Vec = Vec::new(); + assert_eq!( + attaches_at(&db, file, source, &["first", "second", "third", "after"]), + vec![no_attach.clone(), no_attach.clone(), no_attach, vec![ + "cli".to_string() + ]] + ); +} + +#[test] +fn test_attach_on_all_but_one_arm_of_an_else_if_chain_drops_at_the_chain() { + // One arm without the attach breaks the chain, so nothing holds afterwards + // even though the last arm attaches. Closing the outer `if` isn't on its own + // enough to carry an attach past it. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "if (a) {\n library(cli)\n} else if (b) {\n middle\n} else {\n \ + library(cli)\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + let no_attach: Vec = Vec::new(); + assert_eq!(attaches_at(&db, file, source, &["middle", "after"]), vec![ + no_attach.clone(), + no_attach + ]); +} + +#[test] +fn test_attach_in_loop_body_does_not_hold_after_the_loop() { + // An empty sequence means the body never runs, so the attach doesn't + // survive the loop even though no branch is involved. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "for (i in xs) {\n library(cli)\n inside\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside", "after"]), vec![ + vec!["cli".to_string()], + Vec::::new() + ]); +} + +#[test] +fn test_attach_on_both_branches_inside_a_loop_drops_at_the_loop_join() { + // Both `if` arms attach `cli`, but a loop body may not run. The attach ends + // at the loop's closing brace. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "while (cond) {\n if (x) library(cli) else library(cli)\n inside\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside", "after"]), vec![ + vec!["cli".to_string()], + Vec::::new() + ]); +} + +#[test] +fn test_conditional_attach_reaches_only_a_body_defined_in_its_branch() { + // A lazy body ignores source order, so it sees a file-scope attach wherever + // that attach sits. A conditional one is different: only a body defined + // inside the arm is guaranteed to run with the package attached. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = + "if (cond) {\n library(cli)\n f <- function() guarded\n}\ng <- function() unguarded\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!( + attaches_at(&db, file, source, &["guarded", "unguarded"]), + vec![vec!["cli".to_string()], Vec::::new()] + ); +} + +#[test] +fn test_conditional_attach_inside_a_body_holds_only_in_its_arm() { + // The arm narrowing is the same inside a lazy body as at file scope: `taken` + // runs with cli attached, `after` only might. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "g <- function() {\n if (cond) {\n library(cli)\n taken\n }\n after\n}\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["taken", "after"]), vec![ + vec!["cli".to_string()], + Vec::::new() + ]); +} + +#[test] +fn test_cursor_in_local_narrows_to_calls_that_have_run() { + // `local()` runs at its call site, so a cursor inside it sees the search + // path as of that point, not the end-of-file view a function body gets. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "local({\n inside\n})\nlibrary(cli)\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside"]), vec![Vec::< + String, + >::new( + )]); +} + +#[test] +fn test_attach_in_local_is_visible_after_the_local() { + // The block runs during the file's own top-level execution, so its + // `library()` is on the search path afterwards, the same as one written + // directly at top level. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "local({\n library(cli)\n})\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["after"]), vec![vec![ + "cli".to_string() + ]]); +} + +#[test] +fn test_attach_in_function_body_is_not_visible_after_it() { + // The body may never run, so its `library()` stays out of the top-level + // view. Guards the eager-scope widening against swallowing lazy scopes. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "f <- function() {\n library(cli)\n}\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["after"]), vec![Vec::< + String, + >::new( + )]); +} + +#[test] +fn test_attach_in_a_function_body_is_not_visible_in_a_sibling_body() { + // `g()` and `h()` can run in either order, so `g()`'s attach does not reach + // `h()`. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "g <- function() {\n library(cli)\n}\nh <- function() {\n inside\n}\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside"]), vec![Vec::< + String, + >::new( + )]); +} + +#[test] +fn test_attach_from_a_local_block_reaches_the_rest_of_the_body() { + // `local()` runs during `g()`, so its `library()` reaches code after the + // block. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "g <- function() {\n local({ library(cli) })\n inside\n}\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside"]), vec![vec![ + "cli".to_string() + ]]); +} + +#[test] +fn test_attach_later_in_an_enclosing_body_is_not_visible_in_a_nested_body() { + // `h()` can run before `g()` reaches the later `library()`, so the attach + // does not reach `h()`. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "g <- function() {\n h <- function() inside\n library(cli)\n}\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside"]), vec![Vec::< + String, + >::new( + )]); +} + +#[test] +fn test_attach_in_a_function_body_is_visible_in_a_body_it_encloses() { + // `outer()` creates `inner` only after the preceding `library()` runs, so + // the attach reaches `inner()`. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "outer <- function() {\n library(cli)\n inner <- function() inside\n}\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside"]), vec![vec![ + "cli".to_string() + ]]); +} + +#[test] +fn test_attach_in_a_function_body_is_not_visible_earlier_in_that_body() { + // `g()` executes `inside` before its later `library()`, so no attach reaches + // `inside`. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "g <- function() {\n inside\n library(cli)\n}\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside"]), vec![Vec::< + String, + >::new( + )]); +} + +#[test] +fn test_attach_is_not_visible_on_the_attaching_call() { + // The attach begins after `library()` returns, so offsets in the multiline + // call see no attach. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "library(\n cli\n)\nafter\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!( + attaches_at(&db, file, source, &["library", "cli", "after"]), + vec![Vec::::new(), Vec::::new(), vec![ + "cli".to_string() + ]] + ); +} + +#[test] +fn test_attach_in_a_function_body_is_visible_later_in_that_body() { + // `library()` returns before the later `inside` expression runs in `g()`, so + // the attach is visible there. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli"]); + + let source = "g <- function() {\n library(cli)\n inside\n}\n"; + let file = make_file(&mut db, "a.R", source); + + assert_eq!(attaches_at(&db, file, source, &["inside"]), vec![vec![ + "cli".to_string() + ]]); +} diff --git a/crates/oak_db/src/tests/file_resolve_at.rs b/crates/oak_db/src/tests/file_resolve_at.rs index 8d42cc259..e10138080 100644 --- a/crates/oak_db/src/tests/file_resolve_at.rs +++ b/crates/oak_db/src/tests/file_resolve_at.rs @@ -562,3 +562,30 @@ fn test_namespace_importfrom_makes_export_resolve_in_package_file() { assert_eq!(def.file(&db), ext_file); assert_eq!(def.name(&db).text(&db).as_str(), "baz"); } + +#[test] +fn test_conditional_library_resolves_only_inside_its_branch() { + // The reported symptom of the conditional-attach bug was goto-definition, + // which reaches the attach layers through `resolve_at` rather than + // `imports_at`. Nothing says the branch ran, so the use after it resolves + // to nothing while the one inside the arm still lands in the package. + let mut db = TestDb::new(); + let (_root, pkg) = install_library_package(&mut db, "mypkg", &["foo"], &[( + "library/mypkg/R/a.R", + "foo <- function() 42\n", + )]); + let pkg_file = pkg.files(&db)[0]; + + let (_ws_root, files) = setup_workspace_scripts(&mut db, "ws", &[( + "ws/script.R", + "if (cond) {\n library(mypkg)\n foo\n}\nfoo\n", + )]); + let script = files[0]; + let source = script.source_text(&db).clone(); + + let inside = TextSize::from(source.find(" foo").unwrap() as u32 + 2); + assert_eq!(resolve_one(&db, script, inside).file(&db), pkg_file); + + let after = TextSize::from(source.rfind("foo").unwrap() as u32); + assert!(script.resolve_at(&db, after).is_empty()); +} diff --git a/crates/oak_db/src/tests/resolver.rs b/crates/oak_db/src/tests/resolver.rs index fb0e0b148..b204d3698 100644 --- a/crates/oak_db/src/tests/resolver.rs +++ b/crates/oak_db/src/tests/resolver.rs @@ -614,7 +614,7 @@ fn test_library_in_sourced_file_records_attach_call() { .semantic_calls() .iter() .filter_map(|c| match c.kind() { - SemanticCallKind::Attach { package } => Some(package.as_str()), + SemanticCallKind::Attach { package, .. } => Some(package.as_str()), _ => None, }) .collect(); @@ -640,7 +640,7 @@ fn test_library_propagates_transitively_through_source_chains() { .semantic_calls() .iter() .filter_map(|c| match c.kind() { - SemanticCallKind::Attach { package } => Some(package.as_str()), + SemanticCallKind::Attach { package, .. } => Some(package.as_str()), _ => None, }) .collect(); diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index 5aa29b155..0b03fd2de 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -21,9 +21,10 @@ //! snapshot. //! //! So there are two flow states, on purpose. The scan's flow state tracks only -//! eager bindings and is allowed to stay coarse (across `if` branches it -//! over-approximates to "bound on some path"). The walk builds the precise -//! structures, such as the use-def map. +//! eager bindings and is allowed to stay coarse: at an `if` join it only keeps +//! the names consistently bound on every path. The walk builds the precise +//! structures, such as the use-def map, where conditionality is recorded as +//! `may_be_unbound`. use std::sync::Arc; @@ -34,6 +35,7 @@ use aether_syntax::RRoot; use aether_syntax::RSyntaxKind; use biome_rowan::AstNode; use biome_rowan::TextRange; +use biome_rowan::TextSize; use oak_core::syntax_ext::RIdentifierExt; use oak_core::syntax_ext::RStringValueExt; use oak_index_vec::Idx; @@ -43,6 +45,7 @@ use scan::BindingSites; use scan::BodyScan; use scan::CallResolution; use scan::DeferredBody; +use scan::FlowAttaches; use scan::FlowState; use scan::OpenScope; @@ -111,7 +114,8 @@ struct SemanticIndexBuilder { /// /// - An eager callee is shadowed only by bindings that already ran. /// `bound_so_far` reflects this view. It rewinds at branch joins and is -/// reseeded for each scan unit. +/// reseeded for each scan unit. Forward bindings (defined later) and +/// deferred bindings (`on.exit()`, `<<-`) don't enter `bound_so_far`. /// - A lazy body runs after its scope has finished and resolves symbols /// in the whole scope. `bound_anywhere` reflects this view. /// @@ -130,14 +134,32 @@ struct ScanState { // What the scan prepared for each child body, keyed by the body's range. // See [`BodyScan`]. body_scans: FxHashMap, - // Packages attached in eager flow order (file level and eager NSE descents), - // appended only when `!is_lazy()`. Append-only, never restored across a - // descent or branch: attaches hit the global search path, they aren't scoped - // like `bound_so_far`. An eager callee reads the flow-precise prefix during - // the file scan. A lazy callee reads the complete set during the walk (which - // runs after the file scan finishes), so this doubles as the end-of-file - // attach view. - attached_flow: Vec, + // Packages attached on every path so far, the attach analog of + // `bound_so_far` (file level and eager NSE descents, appended only when + // `!is_lazy()`). A `library()` on only one branch, or in a loop body that + // may not run, drops at the join. An eager callee reads the flow-precise + // prefix during the file scan. A lazy callee reads the end-of-file value + // during the walk, paired with `attached_inherited` for what was live where + // that body was defined. + attached_so_far: FlowAttaches, + // Where a conditional attach stops holding, keyed by the attaching call's + // offset, holding the (package, end) pairs recorded at that offset (almost + // always one; a `source()` forwarding several packages can produce + // several). Written when a branch or loop join drops the attach from + // `attached_so_far`, and read by the walk onto the `SemanticCall` so an + // offset-based consumer narrows the same way the scan does. + attach_effect_ends: FxHashMap>, + // Attaches that were live where the current scan unit was defined, cleared + // and reseeded by `begin_scan()`. A lazy body inherits them, e.g. in + // `if (cond) { library(shiny); f <- function() reactive({ ... }) }`. Empty + // for the file scope and for any unit defined outside a branch. + attached_inherited: Vec, + // Every package attached on any eager path, paired with the attaching + // call's range, in attach order. Unlike `attached_so_far`, this is never + // dropped or truncated at a branch or loop join. Used to probe whether an + // effect decision based on the linear view is ambiguous across paths + // (`record_conditional_attach_ambiguity()`). + attached_anywhere: Vec<(String, TextRange)>, // Per-call facts resolved by the scanner in flow order, keyed by the call's // range. See `CallResolution`. call_resolutions: FxHashMap, @@ -200,7 +222,10 @@ impl SemanticIndexBuilder { call_resolutions: FxHashMap::default(), bound_so_far: FlowState::default(), body_scans: FxHashMap::default(), - attached_flow: Vec::new(), + attached_so_far: FlowAttaches::default(), + attach_effect_ends: FxHashMap::default(), + attached_inherited: Vec::new(), + attached_anywhere: Vec::new(), open_scopes: Vec::new(), deferred_bodies: Vec::new(), }, diff --git a/crates/oak_semantic/src/builder/effects.rs b/crates/oak_semantic/src/builder/effects.rs index c8c94b74c..df8949035 100644 --- a/crates/oak_semantic/src/builder/effects.rs +++ b/crates/oak_semantic/src/builder/effects.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use aether_syntax::AnyRExpression; use aether_syntax::RBinaryExpression; use aether_syntax::RCall; @@ -16,6 +18,7 @@ use crate::effects::EffectSite; use crate::effects::Effects; use crate::effects::EffectsHandlers; use crate::resolver::ImportsResolver; +use crate::semantic_index::AmbiguityReason; use crate::semantic_index::ScopeId; use crate::semantic_index::SemanticDiagnostic; @@ -56,7 +59,7 @@ impl SemanticIndexBuilder { /// - A bare identifier. If bound locally it goes through the local /// [`resolve_local_effects`](Self::resolve_local_effects). Otherwise the /// cross-file `ImportsResolver::resolve_effects()` resolves it across the - /// search path, against the attach set in `attached_flow`. + /// search path, against the attach set in `attached_so_far`. /// - A `pkg::fn` namespace expression, resolved through /// `ImportsResolver::resolve_qualified_effects()`. `::` names the package, /// so there's no search-path disambiguation; the resolver answers from @@ -100,14 +103,7 @@ impl SemanticIndexBuilder { /// diagnostic. fn resolve_symbol_effects(&mut self, sym: &str, range: TextRange) -> Option { // First check for a local definition (which in the future may - // carry declared effects that we resolve here) - // - // Looked up from `bound_so_far` which already carries every - // eager binding visible here: the scope's own flow-precise - // bindings so far, plus the enclosing eager environment seeded - // at `begin_scan()`. Forward and deferred (lazy-routed) - // bindings are excluded. A forward one isn't in `bound_so_far` - // yet, and a deferred one (`on_load`, `<<-`) never enters it. + // carry declared effects that we resolve here). if self.scan.bound_so_far.is_bound(sym) { return self.resolve_local_effects(sym); } @@ -120,22 +116,23 @@ impl SemanticIndexBuilder { // Now check imports since the symbol is locally unbound let lazy = self.scan_is_lazy(); - let effects = self - .resolver - .resolve_effects(sym, &self.scan.attached_flow, lazy)?; + let attached = attach_search_path( + &self.scan.attached_inherited, + self.scan.attached_so_far.packages(), + ); + let effects = self.resolver.resolve_effects(sym, &attached, lazy); + + let Some(effects) = effects else { + // The search path didn't resolve. Probe whether it would have if a + // dropped attach had survived the join, so we can flag it. + self.record_conditional_attach_ambiguity(sym, range, lazy); + return None; + }; // The callee is unbound by any eager binding, so its effect // holds. If a lazy-crossed ancestor binds it whole-scope, that // binding's timing relative to this deferred body is // undetermined, so the decision is a guess. Flag it. - // - // TODO(diagnostics): a symmetric attach ambiguity is out of - // scope here. A callee resolved not-effectful could be flipped - // by an attach from a sibling lazy body (`g <- function() - // library(shiny); f <- function() reactive({...}`). Detecting it - // needs the complete set of lazy-context attaches, a post-pass - // rather than this local ancestor check, so it belongs in the - // future salsa diagnostics query where this lint should move too. if let Some(overwrite_range) = self.is_lazily_shadowed(sym) { self.record_lazy_shadow_ambiguity(sym.to_string(), range, overwrite_range); } @@ -145,7 +142,12 @@ impl SemanticIndexBuilder { /// Local resolver for declared effects, mirroring the imports resolver's /// `resolve_effects()` method on the cross-file side. - /// TODO(nse, annotations): always `None` until `declare()` parsing lands. + /// + /// TODO(nse, annotations): Resolve effects declare()'d on local functions. + /// + /// TODO(nse, inference): Infer effects from local function bodies. Calling + /// `g()` should apply the attach in `g <- function() library(shiny)`. Mutual + /// recursion needs a fixed point. fn resolve_local_effects(&self, _name: &str) -> Option { None } @@ -270,11 +272,82 @@ impl SemanticIndexBuilder { call_range: TextRange, overwrite_range: TextRange, ) { - self.diagnostics - .push(SemanticDiagnostic::LazyShadowAmbiguity { - name, + self.diagnostics.push(SemanticDiagnostic::EffectAmbiguity { + name, + call_range, + reason: AmbiguityReason::LazyShadow { overwrite_range }, + }); + } + + /// Probe whether `sym`'s effect still resolves after a conditional + /// attach. If the case, we record the ambiguity for diagnostics. + /// + /// This handles the eager case, where the dropped attach and the callee are + /// both reachable from the same scan. What's still open is the lazy-sibling + /// case (`g <- function() library(shiny); f <- function() reactive({...})`), + /// which needs the complete set of lazy-context attaches from a post-pass, + /// not this call-site probe. That belongs in the future salsa diagnostics + /// query where this lint family should move too. + fn record_conditional_attach_ambiguity( + &mut self, + sym: &str, + call_range: TextRange, + lazy: bool, + ) { + // A package in `attached_anywhere` but off the search path means it was + // dropped at a branch or loop join + let search_path = attach_search_path( + &self.scan.attached_inherited, + self.scan.attached_so_far.packages(), + ); + let dropped: Vec<(String, TextRange)> = self + .scan + .attached_anywhere + .iter() + .filter(|(package, _)| !search_path.contains(package)) + .cloned() + .collect(); + + // Probe one package at a time, most recent first, so the diagnostic + // mentions the attach that would actually have carried the effect. + for (package, attach_range) in dropped.into_iter().rev() { + if self + .resolver + .resolve_effects(sym, std::slice::from_ref(&package), lazy) + .is_none() + { + continue; + } + + self.diagnostics.push(SemanticDiagnostic::EffectAmbiguity { + name: sym.to_string(), call_range, - overwrite_range, + reason: AmbiguityReason::ConditionalAttach { + package, + attach_range, + }, }); + return; + } } } + +/// The packages seen in a scan unit: what it inherited at its definition point, +/// then the eager linear set. Used for resolution of effect annotations within +/// that scan unit. +/// +/// The two halves only differ for a lazy body defined inside a branch that +/// attached: the join dropped that package from `attached_so_far`, and the +/// inherited half is what keeps it reachable. +pub(super) fn attach_search_path<'a>( + inherited: &'a [String], + so_far: &'a [String], +) -> Cow<'a, [String]> { + if inherited.is_empty() { + return Cow::Borrowed(so_far); + } + + let mut path = inherited.to_vec(); + path.extend_from_slice(so_far); + Cow::Owned(path) +} diff --git a/crates/oak_semantic/src/builder/scan.rs b/crates/oak_semantic/src/builder/scan.rs index 0992e6df0..ec108817f 100644 --- a/crates/oak_semantic/src/builder/scan.rs +++ b/crates/oak_semantic/src/builder/scan.rs @@ -14,12 +14,14 @@ use biome_rowan::AstNodeList; use biome_rowan::AstSeparatedList; use biome_rowan::SyntaxNodeCast; use biome_rowan::TextRange; +use biome_rowan::TextSize; use biome_rowan::WalkEvent; use oak_core::syntax_ext::RIdentifierExt; use rustc_hash::FxHashMap; use rustc_hash::FxHashSet; use super::assignment_name; +use super::effects::attach_search_path; use super::is_assignment; use super::is_right_assignment; use super::is_super_assignment; @@ -30,10 +32,12 @@ use crate::effects::ResolvedArgumentEffects; use crate::effects::ScopeContext; use crate::resolver::ImportsResolver; use crate::resolver::SourceResolution; +use crate::semantic_index::AmbiguityReason; use crate::semantic_index::EvalEnv; use crate::semantic_index::EvalTiming; use crate::semantic_index::ScopeId; use crate::semantic_index::ScopeKind; +use crate::semantic_index::SemanticDiagnostic; use crate::semantic_index::SymbolFlags; // Traversal @@ -59,16 +63,25 @@ impl SemanticIndexBuilder { match self.scan.body_scans.get(&range) { // The file scope has no entry: nothing is inherited, so start clean. - None => self.scan.bound_so_far.clear(), - Some(BodyScan::Deferred(snapshot)) => { - let snapshot = snapshot.clone(); - self.scan.bound_so_far.restore(snapshot); + None => { + self.scan.bound_so_far.clear(); + self.scan.attached_inherited.clear(); + }, + Some(BodyScan::Deferred { + bound_so_far, + attached_inherited, + }) => { + let bound_so_far = bound_so_far.clone(); + let attached_inherited = attached_inherited.clone(); + self.scan.bound_so_far.restore(bound_so_far); + self.scan.attached_inherited = attached_inherited; }, Some(BodyScan::Scanned(_)) => { // A body scanned inline by an eager descent is installed by the // walk without a re-scan, so `begin_scan()` should never meet one. stdext::debug_panic!("`begin_scan()` on an already-scanned body at {range:?}"); self.scan.bound_so_far.clear(); + self.scan.attached_inherited.clear(); }, } @@ -185,15 +198,12 @@ impl SemanticIndexBuilder { variable.syntax().text_trimmed_range(), ); } + if let Ok(sequence) = stmt.sequence() { self.scan_expression(&sequence); } - // A loop body only adds bindings (a name bound inside still - // "reaches" on the ran path), so no restore is needed, unlike - // the two-branch `if`/`else` below. - if let Ok(body) = stmt.body() { - self.scan_expression(&body); - } + + self.scan_loop_body(stmt.body().ok()); }, AnyRExpression::RIfStatement(stmt) => { @@ -201,35 +211,169 @@ impl SemanticIndexBuilder { self.scan_expression(&condition); } - let pre_if = self.scan.bound_so_far.snapshot(); - - if let Ok(consequence) = stmt.consequence() { - self.scan_expression(&consequence); - } - - let post_if = self.scan.bound_so_far.snapshot(); - self.scan.bound_so_far.restore(pre_if); + let alternative = stmt + .else_clause() + .and_then(|else_clause| else_clause.alternative().ok()); + self.scan_branch( + stmt.consequence().ok(), + alternative, + stmt.syntax().text_trimmed_range(), + ); + }, - if let Some(else_clause) = stmt.else_clause() { - if let Ok(alternative) = else_clause.alternative() { - self.scan_expression(&alternative); - } + AnyRExpression::RWhileStatement(stmt) => { + if let Ok(condition) = stmt.condition() { + self.scan_expression(&condition); } - // Both branches' bindings are live afterwards. - self.scan.bound_so_far.merge(post_if); + self.scan_loop_body(stmt.body().ok()); }, - // `while`/`repeat` loops, subsets, extractions, parentheses, unary - // ops, and literals: recurse into child expressions. Loops need no - // flow restore (see the `for` arm). Identifiers and dots are leaves - // with no bindings or calls, so they fall through to a no-op walk. + // `repeat` loops, subsets, extractions, parentheses, unary ops, and + // literals: recurse into child expressions. The body of a `repeat` + // expression always runs once, so its bindings are definite. + // Identifiers and dots are leaves with no bindings or calls, so + // they fall through to a no-op walk. _ => { self.scan_descendants(expr.syntax()); }, } } + fn scan_branch( + &mut self, + consequence: Option, + alternative: Option, + range: TextRange, + ) { + let pre = self.scan.bound_so_far.snapshot(); + // A `library()` in one branch must not be visible in the sibling + // branch, so peel each side's attaches off at this mark and re-add only + // what survives the join. + let attach_watermark = self.scan.attached_so_far.len(); + + let consequence_end = self.scan_conditional(consequence.as_ref()); + + let post = self.scan.bound_so_far.snapshot(); + let consequence_attaches = self.scan.attached_so_far.split_off(attach_watermark); + self.scan.bound_so_far.restore(pre); + + let alternative_end = self.scan_conditional(alternative.as_ref()); + + self.scan.bound_so_far.merge(post); + let alternative_attaches = self.scan.attached_so_far.split_off(attach_watermark); + + self.join_attaches( + consequence_attaches, + consequence_end, + alternative_attaches, + alternative_end, + range, + ); + } + + /// Scan a loop body, which may not run at all (an empty `for` sequence, a + /// `while` condition false on entry). Effects in the body like a binding or + /// `library()` call don't persist past the loop. + fn scan_loop_body(&mut self, body: Option) { + let attach_watermark = self.scan.attached_so_far.len(); + let pre_body = self.scan.bound_so_far.snapshot(); + + let body_end = self.scan_conditional(body.as_ref()); + + self.scan.bound_so_far.merge(pre_body); + let dropped = self.scan.attached_so_far.split_off(attach_watermark); + self.end_attach_effects(dropped, body_end); + } + + /// Scan a region that may not run (a branch arm, a loop body) and return + /// where it ends, the point past which its effects like attaches no longer hold. + fn scan_conditional(&mut self, region: Option<&AnyRExpression>) -> Option { + let region = region?; + self.scan_expression(region); + Some(region.syntax().text_trimmed_range().end()) + } + + /// Rejoin the two `if` arms' attaches. An attach in only one of the arms + /// ends at that arm's close because it does not run on every path. When + /// both arms attach a package, retain the `else` attach since every path + /// has the package after its call. + fn join_attaches( + &mut self, + consequence: Vec, + consequence_end: Option, + alternative: Vec, + alternative_end: Option, + range: TextRange, + ) { + // The `else` arm is the `if`'s final child, so its attach reaches the + // closing brace. End the consequence attach at its arm so it cannot reach + // the `else` arm. + let rejoined = |site: &AttachSite| { + consequence + .iter() + .any(|other| other.package == site.package) + }; + let (rejoined, dropped): (Vec<_>, Vec<_>) = alternative.into_iter().partition(rejoined); + + self.record_attach_order_ambiguity(&consequence, &rejoined, range); + + self.end_attach_effects(consequence, consequence_end); + self.end_attach_effects(dropped, alternative_end); + + for site in rejoined { + self.scan.attached_so_far.push(site.package, site.offset); + } + } + + /// Record a diagnostic when packages attached on both `if` arms have different + /// search orders. The scanner retains the `else` order, so masked names after + /// the join may resolve differently on the consequence path. + /// + /// Compare only packages attached on every path through both arms. A package + /// attached conditionally within an arm may still reorder another on some path, + /// but this join cannot distinguish that from exclusive sibling arms. + /// TODO(diagnostics): Resolution should detect and lint a use outside the + /// package's effect region. + fn record_attach_order_ambiguity( + &mut self, + consequence: &[AttachSite], + rejoined: &[AttachSite], + range: TextRange, + ) { + if rejoined.len() < 2 { + return; + } + + let packages: FxHashSet<&str> = rejoined.iter().map(|site| site.package.as_str()).collect(); + let kept = search_order(rejoined, &packages); + if search_order(consequence, &packages) == kept { + return; + } + + self.diagnostics + .push(SemanticDiagnostic::AmbiguousAttachOrder { + packages: kept.into_iter().map(String::from).collect(), + range, + }); + } + + /// End the effect of each attach in `sites` at `end`, the close of the + /// branch arm or loop body that made it. `end` is `None` only for an absent + /// arm, which attaches nothing. + fn end_attach_effects(&mut self, sites: Vec, end: Option) { + let Some(end) = end else { + return; + }; + for site in sites { + self.scan + .attach_effect_ends + .entry(site.offset) + .or_default() + .push((site.package, end)); + } + } + /// Walk descendant nodes of `expr`, scanning the outermost /// `AnyRExpression` children. The scan analog of /// `walk_descendants`. @@ -271,14 +415,26 @@ impl SemanticIndexBuilder { }; if let Some(package) = attach { + let call_range = call.syntax().text_trimmed_range(); self.scan .call_resolutions - .entry(call.syntax().text_trimmed_range()) + .entry(call_range) .or_default() .attach = Some(package.clone()); + + // Keep every eager attach here even after a branch or loop removes + // it from `attached_so_far`. If a later effect lookup fails, we + // emit a lint when one of these dropped attaches would have + // resolved it. Lazy-body attaches stay out because this scan cannot + // establish their order relative to other lazy bodies. if !self.scopes[self.current_scope].kind.is_lazy() { - self.scan.attached_flow.push(package); + self.scan + .attached_anywhere + .push((package.clone(), call_range)); } + + // Make this attach available to later calls in the current scan unit. + self.scan.attached_so_far.push(package, call_range.start()); } // Cache each recognized path with its resolution. The walk reads them @@ -326,6 +482,12 @@ impl SemanticIndexBuilder { return; }; + // We resolved an effect for a callee that isn't bound on every path. If + // it's bound on *some* earlier path here (a conditional shadow that + // dropped out of the eager linear view), the scope-shape decision is + // ambiguous, so flag it before descending into the body. + self.record_conditional_shadow_ambiguity(call, &arg_effects); + let Ok(args) = call.arguments() else { return; }; @@ -353,9 +515,15 @@ impl SemanticIndexBuilder { // end of this scan unit, once the owner's lexical // environment is fully known. See `scan_deferred_bodies()`. (EvalEnv::Current, EvalTiming::Lazy) => { + let attached_inherited = attach_search_path( + &self.scan.attached_inherited, + self.scan.attached_so_far.packages(), + ) + .into_owned(); self.scan.deferred_bodies.push(DeferredBody { body: value.clone(), bound_so_far: self.scan.bound_so_far.snapshot(), + attached_inherited, }); }, @@ -410,6 +578,82 @@ impl SemanticIndexBuilder { .arguments = Some(arg_effects); } + /// Flag an NSE call whose scope only exists on some branches, because a + /// *conditional* binding earlier in this scope shadows its callee. + /// + /// Take `if (cond) local <- identity` then `local({ y <- 1 })`. On the + /// branch where the shadow held, `local` is the user's own standard + /// evaluation function. The scope shape ambiguously depends on `cond`, + /// which we record here. + /// + /// A conditional shadow is a binding on some path through this scope but not + /// on every path, so we test both halves directly: bound somewhere in the + /// scope, and not bound on every path. A binding on every path is a definite + /// shadow that suppresses the effect, so that call is a plain local one and + /// never reaches here. + /// + /// Two things narrow what fires. The effect must actually build a scope, so + /// we skip `evalq()` (the `Current + Eager` quadrant, which builds none). + /// And we check only the current scan scope, so a conditional shadow in an + /// enclosing scope slips through. In + /// + /// ```r + /// if (cond) local <- identity + /// with(d, { + /// local({ y <- 1 }) + /// }) + /// ``` + /// + /// the `local({...})` call sits in the `with()` scope, which never bound + /// `local`, so nothing flags it even though the shape still depends on + /// `cond`. The `with()` body must be eager for this to go silent: a lazy + /// body (a function, `reactive()`) instead trips the lazy-shadow diagnostic, + /// which resolves against the ancestor union. Reachability constraints will + /// close the eager gap by matching the binding's guard against the call's, + /// rather than tracking a coarse per-scope set. + /// + /// The bindings of `on_load()` or `on.exit()` are not visible yet because + /// their bodies are scanned after this call. That lazy-timed shadow is left + /// to the lazy-shadow diagnostic. + fn record_conditional_shadow_ambiguity( + &mut self, + call: &RCall, + arg_effects: &ResolvedArgumentEffects, + ) { + let creates_scope = arg_effects.iter().flatten().any(|effect| { + matches!( + effect, + ResolvedArgumentEffect::EvalQ { env, timing } + if !matches!((*env, *timing), (EvalEnv::Current, EvalTiming::Eager)) + ) + }); + if !creates_scope { + return; + } + + let Ok(AnyRExpression::RIdentifier(ident)) = call.function() else { + return; + }; + let name = ident.name_text(); + + // A conditional shadow is a binding on some path but not all: present in + // this scope's union (`scan_scope_binding_range()`) yet absent from + // `bound_so_far`. + let Some(binding_range) = self.scan_scope_binding_range(&name) else { + return; + }; + if self.scan.bound_so_far.is_bound(&name) { + return; + } + + let call_range = call.syntax().text_trimmed_range(); + self.diagnostics.push(SemanticDiagnostic::EffectAmbiguity { + name, + call_range, + reason: AmbiguityReason::ConditionalShadow { binding_range }, + }); + } + /// Scan the `Current + Lazy` bodies queued since `watermark`, now that the /// enclosing unit's `bound_anywhere` is complete. Runs with the owner's /// frame context still live (the arena `current_scope`, plus any open eager @@ -427,8 +671,15 @@ impl SemanticIndexBuilder { break; } - for DeferredBody { body, bound_so_far } in batch { + for DeferredBody { + body, + bound_so_far, + attached_inherited, + } in batch + { let old = self.scan.bound_so_far.snapshot(); + let old_attached = + std::mem::replace(&mut self.scan.attached_inherited, attached_inherited); self.scan.bound_so_far.restore(bound_so_far); self.scan.open_scopes.push(OpenScope { @@ -440,6 +691,7 @@ impl SemanticIndexBuilder { self.scan.open_scopes.pop(); self.scan.bound_so_far.restore(old); + self.scan.attached_inherited = old_attached; } } } @@ -513,7 +765,12 @@ impl SemanticIndexBuilder { // context, matching `scan_attach_call`'s `!is_lazy()` gate. if !self.scopes[self.current_scope].kind.is_lazy() { for pkg in &resolution.packages { - self.scan.attached_flow.push(pkg.clone()); + self.scan + .attached_anywhere + .push((pkg.clone(), source_range)); + self.scan + .attached_so_far + .push(pkg.clone(), source_range.start()); } } @@ -533,6 +790,16 @@ impl SemanticIndexBuilder { } } + /// The site where the current evaluation frame binds `name`, matching + /// what [`scan_scope_binds`](Self::scan_scope_binds) counts as a binding + /// (so it returns `Some` on exactly the same names). + fn scan_scope_binding_range(&self, name: &str) -> Option { + match self.scan_scope() { + ScanScope::Open(scope) => scope.binding_range(name), + ScanScope::Arena(scope) => self.scope_binding_range(scope, name), + } + } + fn scan_scope_is_global(&self) -> bool { match self.scan_scope() { ScanScope::Open(_) => false, @@ -559,16 +826,26 @@ impl SemanticIndexBuilder { // State management impl SemanticIndexBuilder { - /// Record the names a child scope (function body, NSE argument) about to be + /// Record what a child scope (function body, NSE argument) about to be /// created at `range` inherits from its ancestors, to seed the child's scan /// in `begin_scan`. Called during the scan, where `bound_so_far` is the /// parent's flow-precise state at the child's definition point (already /// carrying the parent's own inherited ancestors, so the child inherits /// transitively). + /// + /// The attaches are captured as the parent's whole search path, so a nested + /// body inherits a branch-local attach through however many levels of + /// definition sit between them. fn record_enclosing_flow(&mut self, range: TextRange) { - self.scan - .body_scans - .insert(range, BodyScan::Deferred(self.scan.bound_so_far.snapshot())); + let attached_inherited = attach_search_path( + &self.scan.attached_inherited, + self.scan.attached_so_far.packages(), + ) + .into_owned(); + self.scan.body_scans.insert(range, BodyScan::Deferred { + bound_so_far: self.scan.bound_so_far.snapshot(), + attached_inherited, + }); } /// Record a binding in both scan binding views. @@ -646,22 +923,97 @@ impl ScopeContext for ScanBindings<'_, R> { } } -/// The scan's flow-precise binding state: which names are bound at the current -/// point of the current scan unit, in flow order. +/// One package attached by a `library()` / `require()` call, or forwarded by a +/// `source()` one. `offset` is the attaching call's start, which pairs with +/// `package` to identify the [`SemanticCall`] the walk emits for it. +/// +/// [`SemanticCall`]: crate::semantic_index::SemanticCall +#[derive(Clone)] +pub(super) struct AttachSite { + pub(super) package: String, + pub(super) offset: TextSize, +} + +/// Return the selected packages in reverse R lookup order after applying +/// `sites`. Reattaching a selected package moves it to the end because R searches +/// its most recent attach first. +fn search_order<'a>(sites: &'a [AttachSite], packages: &FxHashSet<&str>) -> Vec<&'a str> { + let mut order: Vec<&str> = Vec::new(); + + for site in sites { + let package = site.package.as_str(); + if !packages.contains(package) { + continue; + } + order.retain(|kept| *kept != package); + order.push(package); + } + + order +} + +/// The packages attached on every path reaching the current point, each with +/// its call site. The attach analog of [`FlowState`]. +/// +/// The packages sit in their own `Vec` so [`attach_search_path`] can hand the +/// resolver a `&[String]` without rebuilding the search path at every effect +/// resolution. The offsets ride alongside so a branch or loop join can name the +/// attaches it drops. +#[derive(Default)] +pub(super) struct FlowAttaches { + packages: Vec, + offsets: Vec, +} + +impl FlowAttaches { + /// The attached packages in attach order, the search path at this point. + pub(super) fn packages(&self) -> &[String] { + &self.packages + } + + pub(super) fn len(&self) -> usize { + self.packages.len() + } + + pub(super) fn push(&mut self, package: String, offset: TextSize) { + self.packages.push(package); + self.offsets.push(offset); + } + + pub(super) fn truncate(&mut self, watermark: usize) { + self.packages.truncate(watermark); + self.offsets.truncate(watermark); + } + + /// Remove and return everything attached since `mark`, in attach order. + pub(super) fn split_off(&mut self, watermark: usize) -> Vec { + let offsets = self.offsets.split_off(watermark); + self.packages + .split_off(watermark) + .into_iter() + .zip(offsets) + .map(|(package, offset)| AttachSite { package, offset }) + .collect() + } +} + +/// The scan's eager linear binding state: the names bound on every path reaching +/// the current point of the current scan unit. Branch and loop joins +/// intersection-merge, so a binding made on only some paths is visible inside +/// its branch and then dropped at the join. /// /// It's the scan's own flow state, a coarse variant of the walk's use-def map, -/// which isn't built yet. It answers one question, "is this name bound here?", -/// so the scan can tell whether a callee is shadowed at each call and decide -/// whether a call is NSE. It tracks only eager bindings, and it is allowed to -/// stay coarse: `merge()` unions the two sides of an `if`, so that a single -/// branch marks a name as bound. +/// which isn't built yet. `is_bound` answers "is this name definitely bound +/// here?" so the scan can tell whether a callee is shadowed and decide whether a +/// call is NSE. A conditional shadow drops out here, so it falls through to the +/// effect and the walk records the ambiguity. #[derive(Clone, Default)] pub(super) struct FlowState { bound: FxHashSet, } impl FlowState { - /// Whether `name` is bound at the current point. + /// Whether `name` is bound on every path reaching the current point. pub(super) fn is_bound(&self, name: &str) -> bool { self.bound.contains(name) } @@ -676,13 +1028,13 @@ impl FlowState { *self = snapshot; } - /// Union `snapshot` in, so a name reads as bound here if it was bound on - /// either path. This is the `if`/`else` join. - fn merge(&mut self, snapshot: FlowState) { - self.bound.extend(snapshot.bound); + /// Intersect another path's state at a branch or loop join: keep only names + /// bound on both paths, so a binding made on just one path drops out. + fn merge(&mut self, other: FlowState) { + self.bound.retain(|name| other.bound.contains(name)); } - /// Record `name` as bound from here on. + /// Record `name` as bound from here on this path. fn bind(&mut self, name: String) { self.bound.insert(name); } @@ -717,7 +1069,10 @@ pub(super) struct OpenScope { pub(super) enum BodyScan { /// A walk-time scan unit (function body, `Nested + Lazy` like `reactive()`). /// The walk seeds `begin_scan()` from this snapshot. - Deferred(FlowState), + Deferred { + bound_so_far: FlowState, + attached_inherited: Vec, + }, /// Already scanned inline by an eager `Nested` descent (e.g. `local()`). Scanned(BindingSites), } @@ -729,6 +1084,8 @@ pub(super) struct DeferredBody { pub(super) body: AnyRExpression, /// `bound_so_far` captured at the call site, the body's inherited eager env. pub(super) bound_so_far: FlowState, + /// The attach search path at the call site, the body's inherited attaches. + pub(super) attached_inherited: Vec, } /// All definitions in a scope, collected by the scan pass before the @@ -770,3 +1127,63 @@ enum ScanScope<'a> { Open(&'a BindingSites), Arena(ScopeId), } + +#[cfg(test)] +mod tests { + use super::FlowState; + + #[test] + fn test_bind_is_bound() { + let mut state = FlowState::default(); + state.bind("x".to_string()); + assert!(state.is_bound("x")); + } + + #[test] + fn test_merge_one_branch_binding_drops() { + // `if (c) x <- 1`: the else path is empty, the if path binds `x`, so + // `x` isn't on every path and drops at the join. + let mut else_path = FlowState::default(); + let mut if_path = FlowState::default(); + if_path.bind("x".to_string()); + else_path.merge(if_path); + assert!(!else_path.is_bound("x")); + } + + #[test] + fn test_merge_both_branches_binding_persists() { + // `if (c) x <- 1 else x <- 2`: both paths bind `x`, so it persists. + let mut else_path = FlowState::default(); + else_path.bind("x".to_string()); + let mut if_path = FlowState::default(); + if_path.bind("x".to_string()); + else_path.merge(if_path); + assert!(else_path.is_bound("x")); + } + + #[test] + fn test_merge_keeps_pre_branch_binding() { + // `x <- 1; if (c) y <- 1`: `x` is bound before, `y` only in the branch. + let mut pre = FlowState::default(); + pre.bind("x".to_string()); + let mut if_path = pre.clone(); + if_path.bind("y".to_string()); + let mut else_path = pre; + else_path.merge(if_path); + assert!(else_path.is_bound("x")); + assert!(!else_path.is_bound("y")); + } + + #[test] + fn test_merge_loop_body_binding_drops() { + // `for (i in xs) z <- 1`: `i` is bound before the body, `z` only inside, + // so `z` drops when the body merges with the pre-loop state. + let mut pre_body = FlowState::default(); + pre_body.bind("i".to_string()); + let mut post_body = pre_body.clone(); + post_body.bind("z".to_string()); + post_body.merge(pre_body); + assert!(post_body.is_bound("i")); + assert!(!post_body.is_bound("z")); + } +} diff --git a/crates/oak_semantic/src/builder/walk.rs b/crates/oak_semantic/src/builder/walk.rs index 7fce1620f..38470e819 100644 --- a/crates/oak_semantic/src/builder/walk.rs +++ b/crates/oak_semantic/src/builder/walk.rs @@ -20,6 +20,7 @@ use biome_rowan::AstPtr; use biome_rowan::AstSeparatedList; use biome_rowan::SyntaxNodeCast; use biome_rowan::TextRange; +use biome_rowan::TextSize; use biome_rowan::WalkEvent; use oak_core::syntax_ext::AnyRSelectorExt; use oak_core::syntax_ext::RIdentifierExt; @@ -36,6 +37,7 @@ use crate::effects::ResolvedArgumentEffect; use crate::effects::ResolvedArgumentEffects; use crate::effects::TargetAccess; use crate::resolver::ImportsResolver; +use crate::semantic_index::AttachRegion; use crate::semantic_index::Definition; use crate::semantic_index::DefinitionKind; use crate::semantic_index::EnclosingSnapshotKey; @@ -185,19 +187,7 @@ impl SemanticIndexBuilder { self.walk_expression(&sequence); } - let pre_loop = self.walk.use_def_maps[self.current_scope].snapshot(); - - if let Ok(body) = stmt.body() { - let first_use = self.walk.uses[self.current_scope].next_id(); - self.walk_expression(&body); - self.walk.use_def_maps[self.current_scope].finish_loop_defs( - &pre_loop, - first_use, - &self.walk.uses[self.current_scope], - ); - } - - self.walk.use_def_maps[self.current_scope].merge(pre_loop); + self.walk_loop_body(stmt.body().ok(), true); }, AnyRExpression::RIfStatement(stmt) => { @@ -206,26 +196,10 @@ impl SemanticIndexBuilder { self.walk_expression(&condition); } - let pre_if = self.walk.use_def_maps[self.current_scope].snapshot(); - - // If-body (consequence) - if let Ok(consequence) = stmt.consequence() { - self.walk_expression(&consequence); - } - - let post_if = self.walk.use_def_maps[self.current_scope].snapshot(); - self.walk.use_def_maps[self.current_scope].restore(pre_if); - - // Else-body (alternative), if present. If absent, the - // "else path" is just the pre-if state we restored to. - if let Some(else_clause) = stmt.else_clause() { - if let Ok(alternative) = else_clause.alternative() { - self.walk_expression(&alternative); - } - } - - // After: definitions from both branches are live - self.walk.use_def_maps[self.current_scope].merge(post_if); + let alternative = stmt + .else_clause() + .and_then(|else_clause| else_clause.alternative().ok()); + self.walk_branch(stmt.consequence().ok(), alternative); }, AnyRExpression::RWhileStatement(stmt) => { @@ -233,34 +207,13 @@ impl SemanticIndexBuilder { self.walk_expression(&condition); } - let pre_loop = self.walk.use_def_maps[self.current_scope].snapshot(); - - if let Ok(body) = stmt.body() { - let first_use = self.walk.uses[self.current_scope].next_id(); - self.walk_expression(&body); - self.walk.use_def_maps[self.current_scope].finish_loop_defs( - &pre_loop, - first_use, - &self.walk.uses[self.current_scope], - ); - } - // Body may not execute - self.walk.use_def_maps[self.current_scope].merge(pre_loop); + self.walk_loop_body(stmt.body().ok(), true); }, AnyRExpression::RRepeatStatement(stmt) => { // Body always executes at least once, so no merge with pre-loop state. - if let Ok(body) = stmt.body() { - let pre_loop = self.walk.use_def_maps[self.current_scope].snapshot(); - let first_use = self.walk.uses[self.current_scope].next_id(); - self.walk_expression(&body); - self.walk.use_def_maps[self.current_scope].finish_loop_defs( - &pre_loop, - first_use, - &self.walk.uses[self.current_scope], - ); - } + self.walk_loop_body(stmt.body().ok(), false); }, AnyRExpression::RBogusExpression(_) => {}, @@ -287,6 +240,54 @@ impl SemanticIndexBuilder { } } + fn walk_branch( + &mut self, + consequence: Option, + alternative: Option, + ) { + let pre = self.walk.use_def_maps[self.current_scope].snapshot(); + + if let Some(consequence) = consequence { + self.walk_expression(&consequence); + } + + let post = self.walk.use_def_maps[self.current_scope].snapshot(); + self.walk.use_def_maps[self.current_scope].restore(pre); + + if let Some(alternative) = alternative { + self.walk_expression(&alternative); + } + + self.walk.use_def_maps[self.current_scope].merge(post); + } + + // A loop body can run again, so a definition low in the body reaches a + // use above it on the next iteration. The forward walk records the use + // before reaching that definition, which `finish_loop_defs()` patches into + // the use afterward. + // + // `may_skip_body` is true when the body might run zero times, e.g. with + // `for`/`while`. In that case the pre-loop state must stay live alongside + // what the body bound. `repeat` runs at least once, so the body's state + // replaces it. + fn walk_loop_body(&mut self, body: Option, may_skip_body: bool) { + let pre_loop = self.walk.use_def_maps[self.current_scope].snapshot(); + + if let Some(body) = body { + let first_use = self.walk.uses[self.current_scope].next_id(); + self.walk_expression(&body); + self.walk.use_def_maps[self.current_scope].finish_loop_defs( + &pre_loop, + first_use, + &self.walk.uses[self.current_scope], + ); + } + + if may_skip_body { + self.walk.use_def_maps[self.current_scope].merge(pre_loop); + } + } + // Walk descendant nodes of `expr`, collecting the outermost // `AnyRExpression` nodes and recursing into them via `walk_expression`. // This skips intermediate wrapper nodes (e.g. `RElseClause`) while @@ -312,6 +313,10 @@ impl SemanticIndexBuilder { let watermark = self.scan.deferred_bodies.len(); let scope = self.push_scope(ScopeKind::Function, fun.syntax().text_trimmed_range()); + // Keep track of attaches made in the function context. We'll discard + // them upon leaving the lazy context. + let attached = self.scan.attached_so_far.len(); + if let Ok(params) = fun.parameters() { // Scan the default values before collecting them. R binds all // formals into the frame at once, so a default sees every parameter @@ -334,6 +339,8 @@ impl SemanticIndexBuilder { self.walk_expression(&body); } + // Discard attaches made in the lazy context. + self.scan.attached_so_far.truncate(attached); self.pop_scope(scope); } @@ -497,14 +504,29 @@ impl SemanticIndexBuilder { // (sequential execution is guaranteed), but inside a function it's // only visible within that function and its children, since the // function might never be called. Same reasoning as `source()` calls. - let call_offset = call.syntax().text_trimmed_range().start(); + let call_range = call.syntax().text_trimmed_range(); + let region = self.attach_region(call_range.start(), &package); self.walk.semantic_calls.push(SemanticCall { - kind: SemanticCallKind::Attach { package }, - offset: call_offset, + kind: SemanticCallKind::Attach { package, region }, + range: call_range, scope: self.current_scope, }); } + /// Where the attach of `package` at `offset` holds, as the scan recorded + /// it at a branch or loop join. `Unconditional` for an attach on every path. + fn attach_region(&self, offset: TextSize, package: &str) -> AttachRegion { + let end = self.scan.attach_effect_ends.get(&offset).and_then(|ends| { + ends.iter() + .find_map(|(site_package, end)| (site_package == package).then_some(*end)) + }); + + match end { + Some(end) => AttachRegion::Conditional { end }, + None => AttachRegion::Unconditional, + } + } + // `source("file.R")` creates `DefinitionKind::Import` forwarding // bindings in the current scope for each top-level name exported by // the target file. These participate in the use-def map like normal @@ -539,7 +561,7 @@ impl SemanticIndexBuilder { let resolved = resolution.as_ref().map(|r| r.url.clone()); self.walk.semantic_calls.push(SemanticCall { kind: SemanticCallKind::Source { path, resolved }, - offset: call_offset, + range, scope: self.current_scope, }); @@ -567,15 +589,17 @@ impl SemanticIndexBuilder { ); } - // `library()` calls inside the sourced file attach packages to R's - // global search path at runtime, the same as a `library()` written - // here directly would. Emit them as `Attach` semantic calls scoped - // to this `source()`'s offset so scope-layer composition treats - // them identically to local `library()` calls. + // A sourced `library()` attaches to R's global search path. Model it + // as an `Attach` in this `source()` call's scope so import resolution + // handles it like a local `library()` call. for pkg in resolution.packages { + let region = self.attach_region(call_offset, &pkg); self.walk.semantic_calls.push(SemanticCall { - kind: SemanticCallKind::Attach { package: pkg }, - offset: call_offset, + kind: SemanticCallKind::Attach { + package: pkg, + region, + }, + range, scope: self.current_scope, }); } @@ -699,11 +723,18 @@ impl SemanticIndexBuilder { let kind = ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Lazy); let scope = self.push_scope(kind, value.syntax().text_trimmed_range()); + // Keep track of attach state. We discard any attaches made in + // the lazy context upon leaving it. + let attached = self.scan.attached_so_far.len(); + self.begin_scan(); let watermark = self.scan.deferred_bodies.len(); self.scan_expression(value); self.scan_deferred_bodies(watermark); self.walk_expression(value); + + // Discard attaches made in the lazy context. + self.scan.attached_so_far.truncate(attached); self.pop_scope(scope); }, } diff --git a/crates/oak_semantic/src/semantic_index.rs b/crates/oak_semantic/src/semantic_index.rs index 338fe5e57..a3b21e2db 100644 --- a/crates/oak_semantic/src/semantic_index.rs +++ b/crates/oak_semantic/src/semantic_index.rs @@ -189,7 +189,7 @@ impl SemanticIndex { .iter() .filter(|call| self.scope_is_eager(call.scope)) .filter_map(|call| match &call.kind { - SemanticCallKind::Attach { package } => Some(package.as_str()), + SemanticCallKind::Attach { package, .. } => Some(package.as_str()), SemanticCallKind::Source { .. } => None, }) .collect() @@ -206,7 +206,7 @@ impl SemanticIndex { self.semantic_calls .iter() .filter_map(|call| match &call.kind { - SemanticCallKind::Attach { package } => Some(package.as_str()), + SemanticCallKind::Attach { package, .. } => Some(package.as_str()), SemanticCallKind::Source { .. } => None, }) .collect() @@ -214,18 +214,18 @@ impl SemanticIndex { /// Whether `scope` runs during the file's own top-level execution, i.e. no /// enclosing scope is lazy. - fn scope_is_eager(&self, scope_id: ScopeId) -> bool { - let mut ancestor_id = Some(scope_id); - - while let Some(id) = ancestor_id { - let ancestor_scope = self.scope(id); - if ancestor_scope.kind.is_lazy() { - return false; - } - ancestor_id = ancestor_scope.parent; - } + pub fn scope_is_eager(&self, scope_id: ScopeId) -> bool { + self.enclosing_lazy_scope(scope_id).is_none() + } - true + /// The scan unit that controls when code in `scope_id` runs. + /// + /// Returns `scope_id` when it is lazy, otherwise its nearest lazy ancestor. + /// `None` means the code runs while the file loads. An eager `local()` block + /// remains in its enclosing lazy scan unit. + pub fn enclosing_lazy_scope(&self, scope_id: ScopeId) -> Option { + self.ancestor_scope_ids(scope_id) + .find(|&id| self.scope(id).kind.is_lazy()) } /// Cross-file call sites (`library()`, `source()`, …) recorded @@ -795,15 +795,48 @@ impl Ranged for Use { #[derive(Debug, Clone, PartialEq, Eq)] pub struct SemanticCall { pub(crate) kind: SemanticCallKind, - pub(crate) offset: TextSize, + pub(crate) range: TextRange, pub(crate) scope: ScopeId, } +/// Where an attach is known to hold. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AttachRegion { + /// From the call to its scope's end when every path to a later offset has the + /// package attached. This includes calls that run on every path and the `else` + /// call when both `if` arms attach the package. + Unconditional, + /// From the call to `end` only, the close of the arm or loop body that + /// made it. Nothing outside is known, in either direction. + /// + /// The start isn't a field: it's always the call's own offset, so + /// [`contains`](Self::contains) takes the `call` rather than storing a + /// redundant start that could disagree with it. + Conditional { end: TextSize }, +} + +impl AttachRegion { + /// Whether the attach from `call` holds at `offset`. + /// + /// The package attaches after the call returns, so the region starts at + /// `call.range().end()` and excludes every offset in `library(foo)`. + pub fn contains(&self, call: &SemanticCall, offset: TextSize) -> bool { + call.range().end() <= offset && + match self { + AttachRegion::Unconditional => true, + AttachRegion::Conditional { end } => offset <= *end, + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum SemanticCallKind { /// `library(pkg)` or `require(pkg)`: attaches a package to the /// search path. Contributes a fallback layer for unbound symbols. - Attach { package: String }, + Attach { + package: String, + region: AttachRegion, + }, /// `source("path")`: injects the sourced file's top-level /// bindings into the current scope. Local-scope semantics, not /// search-path semantics. @@ -820,7 +853,11 @@ impl SemanticCall { } pub fn offset(&self) -> TextSize { - self.offset + self.range.start() + } + + pub fn range(&self) -> TextRange { + self.range } pub fn scope(&self) -> ScopeId { @@ -882,16 +919,49 @@ pub enum NamespaceAccessKind { /// consumers to turn into user-facing diagnostics. #[derive(Debug, Clone, PartialEq, Eq)] pub enum SemanticDiagnostic { - /// An effectful call (NSE scope or attach) recognized in a lazy context - /// whose callee is also bound by a lazy-crossed ancestor with undetermined - /// timing, so the decision is a guess. `call_range` points at the call we - /// recognized, `overwrite_range` at the ancestor binding that could - /// invalidate it (a later assignment in parent code, or one from another - /// lazy context). - LazyShadowAmbiguity { + /// An effect decision (NSE scope or attach) settled on the eager-linear + /// reading even though another reading was possible. `call_range` points at + /// the call we decided about, which may or may not have come out effectful; + /// `reason` says what made it ambiguous and where the competing site is. + EffectAmbiguity { name: String, call_range: TextRange, - overwrite_range: TextRange, + reason: AmbiguityReason, + }, + /// Both `if` arms attach the same packages in different orders. The scanner + /// retains the `else` arm's order in `packages`, but R searches the most recent + /// attach first, so masked names after the `if` depend on the selected arm. + /// + /// Emit this even without a current name collision. A later package upgrade + /// could introduce one without a source change. The `range` covers the `if`. + AmbiguousAttachOrder { + packages: Vec, + range: TextRange, + }, +} + +/// What made an [`EffectAmbiguity`](SemanticDiagnostic::EffectAmbiguity) +/// ambiguous, and where the competing site is. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AmbiguityReason { + /// The callee is bound by a lazy-crossed ancestor with undetermined + /// timing: a later assignment in parent code, or one from another + /// deferred body that could run before or after us. `overwrite_range` + /// points at that ancestor binding. + LazyShadow { overwrite_range: TextRange }, + /// The callee is shadowed by a *conditional* local binding in the same + /// scope (`if (cond) local <- identity; local({...})`). The eager linear + /// scan dropped the conditional binding and resolved the effect, so the + /// scope shape is condition-dependent. `binding_range` points at the + /// conditional binding. + ConditionalShadow { binding_range: TextRange }, + /// The callee would have been effectful, but the `library()`/`require()` + /// that annotates it was attached on only some paths and dropped at a + /// branch or loop join, so we read the call as plain. `attach_range` points + /// at that conditional attach. + ConditionalAttach { + package: String, + attach_range: TextRange, }, } diff --git a/crates/oak_semantic/src/use_def_map.rs b/crates/oak_semantic/src/use_def_map.rs index 259eaa415..3530905ad 100644 --- a/crates/oak_semantic/src/use_def_map.rs +++ b/crates/oak_semantic/src/use_def_map.rs @@ -20,7 +20,9 @@ use crate::semantic_index::UseId; // `DefinitionId`s that are "live". A fresh scope starts with every symbol in // the "unbound" state: empty definition set, `may_be_unbound: true`. // The "may_be_unbound" flag tracks whether there exists some control flow path -// where no definition was reached. +// where no definition was reached. It's a rudimentary version of the +// reachability constraints that you can find in ty, which record under what +// condition a variable use is bound or unbound. // // ```r // if (cond) { diff --git a/crates/oak_semantic/tests/integration/contrib/base.rs b/crates/oak_semantic/tests/integration/contrib/base.rs index df52f424a..e3aba9958 100644 --- a/crates/oak_semantic/tests/integration/contrib/base.rs +++ b/crates/oak_semantic/tests/integration/contrib/base.rs @@ -4,6 +4,8 @@ use biome_rowan::AstNode; use oak_semantic::build_index; use oak_semantic::effects; use oak_semantic::effects::SourceAnnotation; +use oak_semantic::semantic_index::AmbiguityReason; +use oak_semantic::semantic_index::AttachRegion; use oak_semantic::semantic_index::DefinitionId; use oak_semantic::semantic_index::DefinitionKind; use oak_semantic::semantic_index::EvalEnv; @@ -473,7 +475,8 @@ fn test_quote_suppresses_assign_effect() { fn test_directive_library_identifier() { let index = index_with_base("library(dplyr)"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }]); } @@ -481,7 +484,8 @@ fn test_directive_library_identifier() { fn test_directive_library_string() { let index = index_with_base("library(\"tidyr\")"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "tidyr".into() + package: "tidyr".into(), + region: AttachRegion::Unconditional, }]); } @@ -489,7 +493,8 @@ fn test_directive_library_string() { fn test_directive_library_single_quoted_string() { let index = index_with_base("library('ggplot2')"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "ggplot2".into() + package: "ggplot2".into(), + region: AttachRegion::Unconditional, }]); } @@ -497,7 +502,8 @@ fn test_directive_library_single_quoted_string() { fn test_directive_require() { let index = index_with_base("require(data.table)"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "data.table".into() + package: "data.table".into(), + region: AttachRegion::Unconditional, }]); } @@ -506,13 +512,16 @@ fn test_directive_multiple_libraries() { let index = index_with_base("library(dplyr)\nlibrary(tidyr)\nrequire(ggplot2)"); assert_eq!(semantic_call_kinds(&index), [ &SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }, &SemanticCallKind::Attach { - package: "tidyr".into() + package: "tidyr".into(), + region: AttachRegion::Unconditional, }, &SemanticCallKind::Attach { - package: "ggplot2".into() + package: "ggplot2".into(), + region: AttachRegion::Unconditional, }, ]); } @@ -522,7 +531,8 @@ fn test_directive_named_argument() { // The package binds the `package` formal by name. let index = index_with_base("library(package = dplyr)"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }]); } @@ -532,7 +542,8 @@ fn test_directive_multiple_arguments() { // no formal we track. let index = index_with_base("library(dplyr, warn.conflicts = FALSE)"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }]); } @@ -542,7 +553,8 @@ fn test_directive_character_only_string() { // A string literal resolves to its text. let index = index_with_base("library(\"dplyr\", character.only = TRUE)"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }]); } @@ -561,7 +573,8 @@ fn test_directive_character_only_false_is_quoted() { // text is the package name. let index = index_with_base("library(dplyr, character.only = FALSE)"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }]); } @@ -576,7 +589,8 @@ fn test_directive_library_in_function_scope() { // library() in a function body now records a scoped directive let index = index_with_base("f <- function() { library(dplyr) }"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }]); let semantic_calls = index.semantic_calls(); assert_ne!(semantic_calls[0].scope(), ScopeId::from(0)); @@ -687,14 +701,16 @@ fn test_source_and_library_calls_coexist() { let index = index_with_base("library(dplyr)\nsource(\"helpers.R\")\nrequire(tidyr)"); assert_eq!(semantic_call_kinds(&index), [ &SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }, &SemanticCallKind::Source { path: "helpers.R".into(), resolved: None, }, &SemanticCallKind::Attach { - package: "tidyr".into() + package: "tidyr".into(), + region: AttachRegion::Unconditional, }, ]); } @@ -874,7 +890,8 @@ fn test_fixme_directive_declare_library_transparent() { // FIXME: We should declare `declare()` as a quoting function. let index = index_with_base("declare(library(dplyr))"); assert_eq!(semantic_call_kinds(&index), [&SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }]); } @@ -904,7 +921,8 @@ fn test_directive_declare_mixed_with_bare() { index_with_base("library(dplyr)\ndeclare(source(\"helpers.R\"))\nsource(\"utils.R\")"); assert_eq!(semantic_call_kinds(&index), [ &SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }, &SemanticCallKind::Source { path: "helpers.R".into(), @@ -1046,7 +1064,8 @@ fn test_source_resolver_packages_become_attach_calls() { resolved: Some(Url::parse("file:///test/helpers.R").unwrap()), }, &SemanticCallKind::Attach { - package: "dplyr".into() + package: "dplyr".into(), + region: AttachRegion::Unconditional, }, ]); } @@ -1201,7 +1220,8 @@ fn test_source_resolver_multiple_files_each_emitted_and_injected() { resolved: Some(Url::parse("file:///a.R").unwrap()), }, &SemanticCallKind::Attach { - package: "pkgA".into() + package: "pkgA".into(), + region: AttachRegion::Unconditional, }, &SemanticCallKind::Source { path: "b.R".into(), @@ -1366,6 +1386,66 @@ local({ ); } +#[test] +fn test_nse_conditionally_shadowed_name_builds_scope_and_lints() { + // Eager linear view: a conditional shadow (`local` bound on only one path) + // drops out of `bound_so_far` at the branch join, so the scan resolves the + // effect and builds the NSE scope with `y` scoped inside. We settle on that + // reading (no mirror) and record an ambiguity diagnostic, because a local + // binding could have suppressed the effect on the `cond` path. + let source = "\ +if (cond) local <- identity +local({ + y <- 1 +}) +y +"; + let index = index_with_base(source); + let file = ScopeId::from(0); + let local_scope = ScopeId::from(1); + + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Eager) + ); + assert_eq!( + index.symbols(local_scope).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + // Not mirrored: `y` at file scope is only the trailing use, and it resolves + // to nothing (the binding lives in the NSE scope in the chosen reading). + assert_eq!( + index.symbols(file).get("y").unwrap().flags(), + SymbolFlags::IS_USED + ); + let (_, use_id, _) = index + .uses_of("y") + .into_iter() + .find(|(scope, _, _)| *scope == file) + .unwrap(); + let bindings = index.use_def_map(file).bindings_at_use(use_id); + assert!(bindings.may_be_unbound()); + assert!(bindings.definitions().is_empty()); + + // The conditional shadow of `local` is flagged. + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::EffectAmbiguity { + name, + call_range, + reason: AmbiguityReason::ConditionalShadow { .. }, + } => { + assert_eq!(name, "local"); + let start = u32::from(call_range.start()) as usize; + let end = u32::from(call_range.end()) as usize; + assert_eq!(&source[start..end], "local({\n y <- 1\n})"); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + #[test] fn test_nse_ancestor_shadowed_name_no_scope() { // A `local` binding in an ENCLOSING scope shadows the base function too, @@ -1405,6 +1485,139 @@ f <- function() { ); } +#[test] +fn test_conditional_shadow_ancestor_lazy_is_lint_covered() { + // A conditional shadow of `local` in an ENCLOSING scope, with the NSE call + // inside a lazy nested scope (a function body). The conditional-shadow + // diagnostic is same-scope only and doesn't fire, but the file isn't silent: + // the lazy-shadow diagnostic catches it, because inside a lazy body `local` + // resolves against the ancestor union where the conditional binding lives. + let source = "\ +if (cond) local <- identity +f <- function() local({ + y <- 1 +}) +"; + let index = index_with_base(source); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::EffectAmbiguity { + name, + call_range, + reason: AmbiguityReason::LazyShadow { overwrite_range }, + } => { + assert_eq!(name, "local"); + let call_start = u32::from(call_range.start()) as usize; + let call_end = u32::from(call_range.end()) as usize; + assert_eq!(&source[call_start..call_end], "local({\n y <- 1\n})"); + let ov_start = u32::from(overwrite_range.start()) as usize; + let ov_end = u32::from(overwrite_range.end()) as usize; + assert_eq!(&source[ov_start..ov_end], "local"); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + +#[test] +fn test_conditional_shadow_same_scope_in_eager_nse() { + // The conditional-shadow diagnostic works one level down too. Here the + // conditional binding and the shadowed call sit in the SAME eager NSE scope + // (the outer `local` body), so the inner `local({...})` is flagged just as + // it would be at file scope. + let source = "\ +local({ + if (cond) local <- identity + local({ + y <- 1 + }) +}) +"; + let index = index_with_base(source); + + let outer = ScopeId::from(1); + let inner = ScopeId::from(2); + assert_eq!( + index.scope(outer).kind(), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Eager) + ); + assert_eq!( + index.scope(inner).kind(), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Eager) + ); + assert_eq!( + index.symbols(inner).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::EffectAmbiguity { + name, + call_range, + reason: AmbiguityReason::ConditionalShadow { .. }, + } => { + assert_eq!(name, "local"); + let start = u32::from(call_range.start()) as usize; + let end = u32::from(call_range.end()) as usize; + assert_eq!(&source[start..end], "local({\n y <- 1\n })"); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + +#[test] +#[ignore = "known blind spot: an eager-nested ancestor conditional shadow is \ + unflagged until reachability constraints land"] +fn test_conditional_shadow_ancestor_eager_blind_spot() { + // `local` is conditionally shadowed in an ENCLOSING scope and the call sits + // in a nested EAGER scope (a `with()` body) that never binds `local`. This + // is the genuine zero-coverage case: same-scope detection can't see the + // ancestor's binding, and the eager body means the lazy-shadow diagnostic + // doesn't apply either, so today no diagnostic fires. Reachability + // constraints should flag the inner call, matching the binding's `cond` + // guard against the call's. + let source = "\ +if (cond) local <- identity +with(d, { + local({ + y <- 1 + }) +}) +"; + let index = index_with_base(source); + + // The NSE scope is still built (we settle on the effectful reading), so the + // only thing missing today is the ambiguity diagnostic. + let inner = ScopeId::from(2); + assert_eq!( + index.scope(inner).kind(), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Eager) + ); + assert_eq!( + index.symbols(inner).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::EffectAmbiguity { + name, + call_range, + reason: AmbiguityReason::ConditionalShadow { .. }, + } => { + assert_eq!(name, "local"); + let start = u32::from(call_range.start()) as usize; + let end = u32::from(call_range.end()) as usize; + assert_eq!(&source[start..end], "local({\n y <- 1\n })"); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + #[test] fn test_nse_forward_def_visible_to_nested_function() { // A function defined inside an eager NSE body that references a name bound @@ -2181,10 +2394,10 @@ local <- identity let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::LazyShadowAmbiguity { + SemanticDiagnostic::EffectAmbiguity { name, call_range, - overwrite_range, + reason: AmbiguityReason::LazyShadow { overwrite_range }, } => { assert_eq!(name, "local"); @@ -2196,6 +2409,7 @@ local <- identity let overwrite_end = u32::from(overwrite_range.end()) as usize; assert_eq!(&source[overwrite_start..overwrite_end], "local"); }, + other => panic!("unexpected diagnostic: {other:?}"), } } @@ -2404,6 +2618,139 @@ f <- function() { // --- Attach tracking --- +#[test] +fn test_attach_order_ambiguity_flagged_when_arms_disagree() { + let source = "\ +if (cond) { + library(cli) + library(rlang) +} else { + library(rlang) + library(cli) +} +"; + let index = index_with_base(source); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::AmbiguousAttachOrder { packages, range } => { + assert_eq!(packages, &vec!["rlang".to_string(), "cli".to_string()]); + let start = u32::from(range.start()) as usize; + let end = u32::from(range.end()) as usize; + assert!(source[start..end].starts_with("if (cond) {")); + assert!(source[start..end].ends_with("library(cli)\n}")); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + +#[test] +fn test_attach_order_ambiguity_not_flagged_when_arms_agree() { + let index = index_with_base( + "\ +if (cond) { + library(cli) + library(rlang) +} else { + library(cli) + library(rlang) +} +", + ); + + assert!(index.diagnostics().is_empty()); +} + +#[test] +fn test_attach_order_ambiguity_sees_an_attach_rejoined_by_a_nested_if() { + // cli reaches the outer join on the inner `if`'s `else` call, so it competes + // with rlang there as if it had been attached directly in the arm. + let index = index_with_base( + "\ +if (a) { + if (b) library(cli) else library(cli) + library(rlang) +} else { + library(rlang) + library(cli) +} +", + ); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::AmbiguousAttachOrder { packages, .. } => { + assert_eq!(packages, &vec!["rlang".to_string(), "cli".to_string()]); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + +#[test] +fn test_attach_order_ambiguity_silent_across_a_conditional_attach() { + // Nothing fires, though the paths that attach both disagree: `a && b` puts + // cli below rlang, `!a` puts it above. cli doesn't survive the consequence + // arm's own join, so it isn't among the packages this join compares. + // + // The join is the wrong place to catch it. Comparing every package attached + // anywhere in an arm would flag this one, but also `if (b) library(cli) else + // library(rlang)`, where no single path attaches both. A use that resolves to + // a conditionally attached package from outside that package's region is the + // signal that separates them, and it lives in resolution, not here. + let index = index_with_base( + "\ +if (a) { + if (b) library(cli) + library(rlang) +} else { + library(rlang) + library(cli) +} +", + ); + + assert!(index.diagnostics().is_empty()); +} + +#[test] +fn test_attach_order_ambiguity_ignores_a_reattach_that_settles_the_order() { + // The consequence attaches cli twice. R searches the latest attach of a + // package first, so both arms end up finding cli before rlang and the arms + // agree after all. + let index = index_with_base( + "\ +if (cond) { + library(cli) + library(rlang) + library(cli) +} else { + library(rlang) + library(cli) +} +", + ); + + assert!(index.diagnostics().is_empty()); +} + +#[test] +fn test_attach_order_ambiguity_ignores_a_package_only_one_arm_attaches() { + let index = index_with_base( + "\ +if (cond) { + library(rlang) + library(cli) +} else { + library(cli) +} +", + ); + + assert!(index.diagnostics().is_empty()); +} + #[test] fn test_nse_attach_eager_body_inside_lazy_body_is_deferred() { // `local` is eager, but it sits inside `f`'s function body, so reaching @@ -2511,14 +2858,9 @@ reactive({ } #[test] -fn test_nse_attach_within_lazy_body_not_yet_supported() { - // Sequential-within-one-lazy-body: when `f` runs, `library(shiny)` runs - // before `reactive`, so `reactive` is determinately NSE. We don't promote it - // today: `attached_flow` only grows in eager context, so the attach inside - // `f` (a lazy body) isn't visible to `reactive` in the same body. The attach - // is still recorded as a `SemanticCall::Attach`. This could be supported by - // tracking a per-unit attach set seeded from the EOF view, parallel to - // `bound_so_far`; deferred for now. +fn test_nse_attach_within_lazy_body_applies_to_later_calls() { + // The attach precedes `reactive()` whenever `f()` runs, so shiny's + // annotation makes `reactive()` NSE. let index = index_with_base( "\ f <- function() { @@ -2530,14 +2872,83 @@ f <- function() { ", ); let f_scope = ScopeId::from(1); + let reactive_scope = ScopeId::from(2); + + assert_eq!(index.scope_ids().count(), 3); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + assert_eq!( + index.scope(reactive_scope).kind(), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Lazy) + ); + assert_eq!(index.scope(reactive_scope).parent(), Some(f_scope)); + assert!(index.symbols(f_scope).get("x").is_none()); + assert_eq!( + index.symbols(reactive_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + // The nested-body history includes shiny, but the file's final search path + // does not. + assert_eq!(index.attached_packages_anywhere(), vec!["shiny"]); + assert!(index.attached_packages().is_empty()); +} + +#[test] +fn test_nse_attach_within_lazy_body_does_not_escape_it() { + // `g()` cannot change the search path used by `h()` or the file scope. + let index = index_with_base( + "\ +g <- function() { + library(shiny) +} +h <- function() { + reactive({ + x <- 1 + }) +} +reactive({ + y <- 1 +}) +", + ); + let file = ScopeId::from(0); + let h_scope = ScopeId::from(2); + + assert_eq!(index.scope_ids().count(), 3); + assert_eq!(index.scope(h_scope).kind(), ScopeKind::Function); + assert_eq!( + index.symbols(h_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + assert_eq!( + index.symbols(file).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_todo_effects_of_a_called_function_are_not_applied() { + // At runtime, `g()` attaches shiny before `reactive()` runs. The index does + // not infer local-function effects, so `reactive()` is not recognized as NSE. + let index = index_with_base( + "\ +g <- function() library(shiny) +g() +reactive({ + x <- 1 +}) +", + ); + let file = ScopeId::from(0); - // The attach is recorded (scoped to `f`), but not fed to `reactive`, and - // not counted at the file's top level: only `attached_packages_anywhere()` - // sees a `library()` buried in a function body. assert_eq!(index.attached_packages_anywhere(), vec!["shiny"]); assert!(index.attached_packages().is_empty()); + assert_eq!(index.scope_ids().count(), 2); - assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + assert_eq!( + index.symbols(file).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); } // --- `on.exit` (Current + Lazy) --- diff --git a/crates/oak_semantic/tests/integration/contrib/rlang.rs b/crates/oak_semantic/tests/integration/contrib/rlang.rs index 3abb06067..ac0268889 100644 --- a/crates/oak_semantic/tests/integration/contrib/rlang.rs +++ b/crates/oak_semantic/tests/integration/contrib/rlang.rs @@ -1,3 +1,4 @@ +use oak_semantic::semantic_index::AmbiguityReason; use oak_semantic::semantic_index::DefinitionId; use oak_semantic::semantic_index::DefinitionKind; use oak_semantic::semantic_index::EvalEnv; @@ -239,7 +240,12 @@ rlang::on_load({ local <- identity }) let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::LazyShadowAmbiguity { name, .. } => assert_eq!(name, "local"), + SemanticDiagnostic::EffectAmbiguity { + name, + reason: AmbiguityReason::LazyShadow { .. }, + .. + } => assert_eq!(name, "local"), + other => panic!("unexpected diagnostic: {other:?}"), } } @@ -401,3 +407,28 @@ f <- function() { ); assert_eq!(index.scope(defer_scope).parent(), Some(f_scope)); } + +#[test] +fn test_nse_coguarded_attach_reaches_deferred_body() { + // `on_load` bodies are drained after the enclosing unit's scan, so the join + // has already dropped shiny from the linear set. The body inherits the + // attach from its call site, matching a function body defined in the branch. + let index = index_with_attached( + "\ +if (cond) { + library(shiny) + rlang::on_load(reactive({ + x <- 1 + })) +} +", + &["rlang", "shiny"], + ); + + let kinds: Vec<_> = index.scope_ids().map(|id| index.scope(id).kind()).collect(); + assert_eq!(kinds, vec![ + ScopeKind::File, + ScopeKind::Nse(EvalEnv::Current, EvalTiming::Lazy), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Lazy), + ]); +} diff --git a/crates/oak_semantic/tests/integration/contrib/shiny.rs b/crates/oak_semantic/tests/integration/contrib/shiny.rs index 00693f998..9e235c1e5 100644 --- a/crates/oak_semantic/tests/integration/contrib/shiny.rs +++ b/crates/oak_semantic/tests/integration/contrib/shiny.rs @@ -1,7 +1,9 @@ +use oak_semantic::semantic_index::AmbiguityReason; use oak_semantic::semantic_index::EvalEnv; use oak_semantic::semantic_index::EvalTiming; use oak_semantic::semantic_index::ScopeId; use oak_semantic::semantic_index::ScopeKind; +use oak_semantic::semantic_index::SemanticDiagnostic; use oak_semantic::semantic_index::SymbolFlags; use crate::common::index_with_base as index; @@ -134,6 +136,214 @@ reactive({ ); } +#[test] +fn test_nse_attach_does_not_leak_across_branches() { + // `library(shiny)` in the `if` branch must not be visible in the `else` + // branch. On the `else` path shiny never attached, so `reactive` is not NSE + // and `x` stays at file scope. Without branch-scoped attaches the `else` + // would wrongly see shiny leaked from the `if`. + let index = index( + "\ +if (cond) { + library(shiny) +} else { + reactive({ + x <- 1 + }) +} +", + ); + let file = ScopeId::from(0); + + // The public attach list reports shiny (it's emitted per attach call, so it + // sees the attach on the `if` path), but the `else` scan never saw shiny. + assert_eq!(index.attached_packages(), vec!["shiny"]); + assert_eq!(index.scope_ids().count(), 1); + assert_eq!( + index.symbols(file).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_conditional_attach_drops_at_join() { + // A `library(shiny)` on only the `if` path isn't attached on every path, so + // it drops at the join, the same as a one-branch binding dropping from + // `bound_so_far`. The later `reactive` then resolves against an attach set + // without shiny, so it is not NSE and `x` stays at file scope. + let index = index( + "\ +if (cond) library(shiny) +reactive({ + x <- 1 +}) +", + ); + let file = ScopeId::from(0); + + assert_eq!(index.scope_ids().count(), 1); + assert_eq!( + index.symbols(file).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + // The public attach list still reports shiny (emitted per attach call, + // independent of the flow-join drop). + assert_eq!(index.attached_packages(), vec!["shiny"]); +} + +#[test] +fn test_nse_attach_on_both_branches_survives_join() { + // Attached on both paths, so shiny is on every path and survives the join. + // The later `reactive` resolves to shiny's NSE annotation and `x` is scoped. + // Note that a more realistic version of this would be + // `library(myshinyfork)` in the `else` branch. This would be sound but we + // don't support this (unseen in the wild?) pattern. + let index = index( + "\ +if (cond) library(shiny) else library(shiny) +reactive({ + x <- 1 +}) +", + ); + let reactive_scope = ScopeId::from(1); + + assert_eq!(index.scope_ids().count(), 2); + assert_eq!( + index.scope(reactive_scope).kind(), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Lazy) + ); + assert_eq!( + index.symbols(reactive_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_attach_in_loop_body_drops_after_loop() { + // A `library(shiny)` in a `for` body isn't attached on every path (the body + // may not run), so it drops after the loop. The later `reactive` is not NSE + // and `x` stays at file scope. + let index = index( + "\ +for (i in pkgs) library(shiny) +reactive({ + x <- 1 +}) +", + ); + let file = ScopeId::from(0); + + assert_eq!(index.scope_ids().count(), 1); + assert_eq!( + index.symbols(file).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_coguarded_attach_reaches_lazy_body() { + // `f` is defined in the same branch that attached shiny, so reaching its + // definition implies the guard held: if `f` exists at all, shiny is attached + // whenever it runs. The join drops shiny from the linear set before the walk + // scans `f`'s body, so the body inherits the attach from its definition + // point instead. + let index = index( + "\ +if (cond) { + library(shiny) + f <- function() reactive({ + x <- 1 + }) +} +", + ); + let f_scope = ScopeId::from(1); + let reactive_scope = ScopeId::from(2); + + assert_eq!(index.scope_ids().count(), 3); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + assert_eq!( + index.scope(reactive_scope).kind(), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Lazy) + ); + assert_eq!( + index.symbols(reactive_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_coguarded_attach_reaches_doubly_nested_lazy_body() { + // The inherited attach passes through each definition point, so it survives + // however many function levels sit between the `library()` and the callee. + let index = index( + "\ +if (cond) { + library(shiny) + f <- function() function() reactive({ + x <- 1 + }) +} +", + ); + let reactive_scope = ScopeId::from(3); + + assert_eq!(index.scope_ids().count(), 4); + assert_eq!( + index.scope(reactive_scope).kind(), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Lazy) + ); +} + +#[test] +fn test_nse_conditional_attach_not_inherited_by_unguarded_body() { + // `f` is defined outside the guard, so its existence says nothing about + // whether shiny attached. Nothing is inherited and the every-path set has + // dropped shiny, so `reactive` is not NSE and `x` stays in `f`. + let index = index( + "\ +if (cond) library(shiny) +f <- function() reactive({ + x <- 1 +}) +", + ); + let f_scope = ScopeId::from(1); + + assert_eq!(index.scope_ids().count(), 2); + assert_eq!( + index.symbols(f_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_coguarded_attach_not_inherited_after_branch() { + // The inherited attach belongs to bodies defined inside the branch, not to + // later ones. `f` comes after the join, so it doesn't pick up shiny from the + // branch that `g` was defined in. + let index = index( + "\ +if (cond) { + library(shiny) + g <- function() 1 +} +f <- function() reactive({ + x <- 1 +}) +", + ); + let f_scope = ScopeId::from(2); + + assert_eq!(index.scope_ids().count(), 3); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + assert_eq!( + index.symbols(f_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + #[test] fn test_nse_attach_local_shadow_still_wins() { // A local `reactive` def shadows shiny's, so the call is not NSE even with @@ -157,3 +367,173 @@ reactive({ SymbolFlags::IS_BOUND ); } + +// --- Conditional attach ambiguity diagnostics --- + +#[test] +fn test_nse_conditional_attach_ambiguity_flagged() { + // `library(shiny)` on only the `if` path drops at the join (see + // `test_nse_conditional_attach_drops_at_join`), so `reactive` resolves as + // effectless. That's ambiguous: on the `cond` path shiny was attached, so + // the effect could have held. Flag it, pointing at the dropped attach. + let source = "\ +if (cond) library(shiny) +reactive({ + x <- 1 +}) +"; + let index = index(source); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::EffectAmbiguity { + name, + call_range, + reason: + AmbiguityReason::ConditionalAttach { + package, + attach_range, + }, + } => { + assert_eq!(name, "reactive"); + assert_eq!(package, "shiny"); + + let call_start = u32::from(call_range.start()) as usize; + let call_end = u32::from(call_range.end()) as usize; + assert_eq!(&source[call_start..call_end], "reactive({\n x <- 1\n})"); + + let attach_start = u32::from(attach_range.start()) as usize; + let attach_end = u32::from(attach_range.end()) as usize; + assert_eq!(&source[attach_start..attach_end], "library(shiny)"); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + +#[test] +fn test_nse_conditional_attach_names_the_responsible_package() { + // Two attaches drop, but only shiny annotates `reactive`. The diagnostic + // has to name shiny and point at its `library()`, not at whichever dropped + // attach happens to come last. + let source = "\ +if (a) library(shiny) +if (b) library(testthat) +reactive({ + x <- 1 +}) +"; + let index = index(source); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::EffectAmbiguity { + reason: + AmbiguityReason::ConditionalAttach { + package, + attach_range, + }, + .. + } => { + assert_eq!(package, "shiny"); + let start = u32::from(attach_range.start()) as usize; + let end = u32::from(attach_range.end()) as usize; + assert_eq!(&source[start..end], "library(shiny)"); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + +#[test] +fn test_nse_coguarded_attach_no_ambiguity() { + // Attach and use are co-guarded (see `test_nse_coguarded_attach_reaches_lazy_body`): + // reaching `f`'s definition already implies shiny is attached, so + // `attached_inherited` resolves `reactive` on the first probe. The + // conditional-attach probe never runs, so nothing is flagged even though + // the attach is conditional in isolation. + let index = index( + "\ +if (cond) { + library(shiny) + f <- function() reactive({ + x <- 1 + }) +} +", + ); + + assert!(index.diagnostics().is_empty()); + + let f_scope = ScopeId::from(1); + let reactive_scope = ScopeId::from(2); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + assert_eq!( + index.scope(reactive_scope).kind(), + ScopeKind::Nse(EvalEnv::Nested, EvalTiming::Lazy) + ); + assert_eq!( + index.symbols(reactive_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_unconditional_attach_no_ambiguity() { + // shiny is attached unconditionally, so nothing ever drops at a join and + // the first resolution already succeeds. No diagnostic. + let index = index( + "\ +library(shiny) +reactive({ + x <- 1 +}) +", + ); + + assert!(index.diagnostics().is_empty()); +} + +#[test] +fn test_nse_loop_attach_ambiguity_flagged() { + // A `library(shiny)` in a `for` body drops after the loop (the body may not + // run), the same as the branch case, so it's just as flaggable. + let index = index( + "\ +for (i in pkgs) library(shiny) +reactive({ + x <- 1 +}) +", + ); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::EffectAmbiguity { + name, + reason: AmbiguityReason::ConditionalAttach { package, .. }, + .. + } => { + assert_eq!(name, "reactive"); + assert_eq!(package, "shiny"); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + +#[test] +fn test_nse_attach_absent_no_ambiguity() { + // shiny is never attached anywhere, so `attached_anywhere` is empty and the + // conditional-attach probe finds nothing either. `reactive` stays + // non-NSE, silently. + let index = index( + "\ +reactive({ + x <- 1 +}) +", + ); + + assert!(index.diagnostics().is_empty()); +}