From decd4f2b5b4e7dc835dd38005c38badeb2deb0ad Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 25 Jun 2026 11:45:32 +0200 Subject: [PATCH 01/12] Support NSE functions in semantic index --- crates/oak_db/src/imports.rs | 18 + crates/oak_db/src/tests/file.rs | 25 + crates/oak_semantic/src/builder.rs | 304 +++++-- .../oak_semantic/src/builder/builder_nse.rs | 305 +++++++ crates/oak_semantic/src/effects.rs | 33 + crates/oak_semantic/src/effects_registry.rs | 68 ++ crates/oak_semantic/src/lib.rs | 3 + crates/oak_semantic/src/resolver.rs | 31 +- crates/oak_semantic/src/semantic_index.rs | 45 +- crates/oak_semantic/src/use_def_map.rs | 111 ++- .../tests/integration/builder_nse.rs | 861 ++++++++++++++++++ crates/oak_semantic/tests/integration/main.rs | 2 + .../tests/integration/resolvers.rs | 55 ++ 13 files changed, 1780 insertions(+), 81 deletions(-) create mode 100644 crates/oak_semantic/src/builder/builder_nse.rs create mode 100644 crates/oak_semantic/src/effects.rs create mode 100644 crates/oak_semantic/src/effects_registry.rs create mode 100644 crates/oak_semantic/tests/integration/builder_nse.rs create mode 100644 crates/oak_semantic/tests/integration/resolvers.rs diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index 535540d024..7335c2dc17 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -2,6 +2,8 @@ use aether_path::FilePath; use camino::Utf8Component; use camino::Utf8Path; use camino::Utf8PathBuf; +use oak_semantic::effects_registry; +use oak_semantic::Effects; use oak_semantic::ImportsResolver; use oak_semantic::SourceResolution; use url::Url; @@ -83,6 +85,22 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { packages, }) } + + fn resolve_effects( + &mut self, + name: &str, + _attached: &[String], + _lazy: bool, + ) -> Option { + // Base is the always-attached layer at the bottom of the search path, + // resolved through the same registry lookup as any package. + // + // TODO!: walk the rest of the search path too (flow-order attaches, + // package siblings via `who_defines`, NAMESPACE imports, re-export chase). + effects_registry::lookup("base", name) + .copied() + .map(Effects::nse) + } } /// Anchor directory for relative `source("path")` arguments. diff --git a/crates/oak_db/src/tests/file.rs b/crates/oak_db/src/tests/file.rs index d0b55182f1..f3294d4818 100644 --- a/crates/oak_db/src/tests/file.rs +++ b/crates/oak_db/src/tests/file.rs @@ -140,6 +140,31 @@ fn test_semantic_index_matches_oak_semantic() { assert_eq!(via_salsa, &direct); } +#[test] +fn test_semantic_index_recognizes_bare_base_nse() { + // Base NSE resolves through the real `SalsaImportsResolver` (base-only + // `resolve_effects`): a bare `local()` still pushes a nested NSE scope, so + // `x` lands there rather than at file scope. + use oak_semantic::semantic_index::NseScope; + use oak_semantic::semantic_index::NseTiming; + use oak_semantic::semantic_index::ScopeId; + use oak_semantic::semantic_index::ScopeKind; + + let mut db = TestDb::new(); + let file = new_file(&mut db, "a.R", "local({\n x <- 1\n})\n"); + + let index = file.semantic_index(&db); + let file_scope = ScopeId::from(0); + let local_scope = ScopeId::from(1); + + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert!(index.symbols(file_scope).get("x").is_none()); + assert!(index.symbols(local_scope).get("x").is_some()); +} + #[test] fn test_semantic_index_backdates_on_equivalent_content_set() { let mut db = TestDb::new(); diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index 77752eb613..8a50158b9f 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -27,7 +27,7 @@ use oak_core::syntax_ext::RStringValueExt; use oak_index_vec::Idx; use oak_index_vec::IndexVec; use rustc_hash::FxHashMap; -use smallvec::SmallVec; +use rustc_hash::FxHashSet; use crate::resolver::ImportsResolver; use crate::semantic_index::Definition; @@ -37,6 +37,8 @@ use crate::semantic_index::EnclosingSnapshotId; use crate::semantic_index::EnclosingSnapshotKey; use crate::semantic_index::NamespaceAccess; use crate::semantic_index::NamespaceAccessKind; +use crate::semantic_index::NseScope; +use crate::semantic_index::NseTiming; use crate::semantic_index::Scope; use crate::semantic_index::ScopeId; use crate::semantic_index::ScopeKind; @@ -49,14 +51,73 @@ use crate::semantic_index::Use; use crate::semantic_index::UseId; use crate::use_def_map::UseDefMapBuilder; +mod builder_nse; + /// Build a [`SemanticIndex`] from a parsed R file with cross-file /// information supplied by `resolver`. See [`ImportsResolver`] for the /// available impls. +/// +/// NSE scopes (`local()`, `test_that()`, ...) require a two-phase build. +/// The first walk keeps everything flat and discovers which calls are NSE. +/// If none are found, that result is final. Otherwise we re-walk with known +/// nested NSE scope bodies. pub fn build_index(root: &RRoot, resolver: impl ImportsResolver) -> SemanticIndex { let range = root.syntax().text_trimmed_range(); + + // First walk: discover which calls are NSE, if any. let mut builder = SemanticIndexBuilder::new(range, resolver); builder.pre_scan_scope(root.syntax()); builder.collect_expression_list(&root.expressions()); + + if !builder.found_nse { + return builder.finish(); + } + + // Re-walk until the set of NSE scope bodies stabilizes. One re-walk is + // typically enough to reach convergence. More walks are needed only when + // pushing an NSE scope unmasks a callee. For instance in: + // + // ``` + // local({ with <- identity }); + // with(df, y) + // ``` + // + // The call to `with()` is recognized only once a re-walk has moved the + // `with` assignment into the `local()` scope. Each such level costs one + // extra walk. Convergence relies on decisions never flipping back from NSE + // to not-NSE, see `is_locally_bound()`. + // + // The loop terminates on its own because the set can only grow. Each + // re-walk is seeded with the previous set and only inserts. The cap only + // guards against pathological files. + // + // An important caveat is that each walk re-analyzes the whole file, so our + // passes never get cheaper, which is fine since rewalks should be rare. + // That's the opposite of Rust-Analyzer's fixpoint, where each pass touches + // only the shrinking unresolved frontier, which is why RA can afford a much + // larger cap of 8192 passes: + // https://github.com/rust-lang/rust-analyzer/blob/abb1301c/crates/hir-def/src/nameres/collector.rs#L61 + const MAX_NSE_ITERATIONS: usize = 64; + for i in 0..MAX_NSE_ITERATIONS { + let prev_ranges = std::mem::take(&mut builder.nse_nested_ranges); + let resolver = builder.resolver; + builder = SemanticIndexBuilder::new_rewalk(range, prev_ranges.clone(), resolver); + builder.pre_scan_scope(root.syntax()); + builder.collect_expression_list(&root.expressions()); + + if builder.nse_nested_ranges == prev_ranges { + if i >= 5 { + log::trace!("NSE re-walk converged after {i} iterations in range {range:?}"); + } + return builder.finish(); + } + } + + // Hitting the cap means the returned index is inconsistent, not merely + // degraded, and valid R should never reach it. `error!` matches that. + log::error!( + "NSE re-walk did not converge after {MAX_NSE_ITERATIONS} iterations in range {range:?}" + ); builder.finish() } @@ -74,11 +135,38 @@ struct SemanticIndexBuilder { enclosing_snapshots: FxHashMap, semantic_calls: Vec, namespace_accesses: Vec, + // The `Nested` NSE scope bodies found so far, as a set of ranges. This is + // the re-walk loop's fixpoint state. Each walk seeds it from the previous + // iteration, grows it as `record_nse_arg_decision()` recognizes more scopes, + // and stops once a walk no longer finds any NSE range. + nse_nested_ranges: FxHashSet, + // `true` once any scope-pushing NSE combo is found. Triggers the re-walk. + found_nse: bool, + // Whether to push NSE scopes at call sites. Only the re-walk does. On the + // first walk the pre-scan hasn't learned which bodies to skip, so it still + // records a nested body's definitions (e.g. `x` from `local({x <- 1})`) + // into the parent scope. Pushing the child scope on that walk too would + // then register `x`'s enclosing snapshot against the parent, one scope too + // high. + is_rewalk: bool, resolver: R, } impl SemanticIndexBuilder { fn new(range: TextRange, resolver: R) -> Self { + Self::new_impl(range, FxHashSet::default(), false, resolver) + } + + fn new_rewalk(range: TextRange, nse_nested_ranges: FxHashSet, resolver: R) -> Self { + Self::new_impl(range, nse_nested_ranges, true, resolver) + } + + fn new_impl( + range: TextRange, + nse_nested_ranges: FxHashSet, + is_rewalk: bool, + resolver: R, + ) -> Self { let mut scopes = IndexVec::new(); let mut symbol_tables = IndexVec::new(); let mut definitions = IndexVec::new(); @@ -116,6 +204,9 @@ impl SemanticIndexBuilder { enclosing_snapshots: FxHashMap::default(), semantic_calls: Vec::new(), namespace_accesses: Vec::new(), + nse_nested_ranges, + found_nse: false, + is_rewalk, resolver, } } @@ -162,6 +253,17 @@ impl SemanticIndexBuilder { kind: DefinitionKind, range: TextRange, ) { + // `Nse(Current, Lazy)` scopes don't own any definitions. We add the + // definitions to the real enclosing owner scope. Note that `Current + + // Eager` never reaches here because it doesn't push a scope. + if matches!( + self.scopes[self.current_scope].kind, + ScopeKind::Nse(NseScope::Current, NseTiming::Lazy) + ) { + self.add_definition_to_owner(name, flags, kind, range); + return; + } + let symbol_id = self.symbol_tables[self.current_scope].intern(name, flags); let def_id = self.definitions[self.current_scope].push(Definition { symbol: symbol_id, @@ -172,6 +274,59 @@ impl SemanticIndexBuilder { self.use_def_maps[self.current_scope].record_definition(symbol_id, def_id); } + /// Route a definition from a `Current + Lazy` scope to the scope that + /// owns it. That's the nearest ancestor scope which holds its own + /// definitions. A chain of `Current + Lazy` scopes (e.g. `on_load()` nested + /// in `on_load()`) is skipped: each one routes to its own owner, so they + /// all land in the same outer scope. + pub(super) fn add_definition_to_owner( + &mut self, + name: &str, + flags: SymbolFlags, + kind: DefinitionKind, + range: TextRange, + ) { + let Some(target_scope) = self.definition_owner() else { + stdext::debug_panic!("Current + Lazy scope has no parent"); + return; + }; + + let symbol_id = self.symbol_tables[target_scope].intern(name, flags); + let def_id = self.definitions[target_scope].push(Definition { + symbol: symbol_id, + kind, + range, + }); + + self.use_def_maps[target_scope].ensure_symbol(symbol_id); + + // Deferred: the body executes at an unknown later time, so the + // definition shouldn't shadow what's already live. This is the same + // mechanism as `<<-`. + // + // Known imprecision: the deferred def is visible to ALL uses in + // the parent scope (with `may_be_unbound: true`), including + // file-level uses that run before the lazy body executes. Ideally + // these defs would only be reachable from lazy scopes (functions), + // not from eager/file-level code. + self.use_def_maps[target_scope].record_deferred_definition(symbol_id, def_id); + } + + /// The scope that owns definitions of a `Current + Lazy` NSE scope. The + /// climb is iterative to handle e.g. `on_load(on_load(...))`. Every other + /// scope kind (`File`, `Function`, `Nse(Nested, _)`) owns its definitions + /// and stops the climb. + fn definition_owner(&self) -> Option { + let mut scope = self.scopes[self.current_scope].parent?; + while matches!( + self.scopes[scope].kind, + ScopeKind::Nse(NseScope::Current, NseTiming::Lazy) + ) { + scope = self.scopes[scope].parent?; + } + Some(scope) + } + // Super-assignment is lexically in the current scope but binds in an // ancestor. We record the definition in the current scope and append // it to the target scope's use-def map (without shadowing prior @@ -271,24 +426,30 @@ impl SemanticIndexBuilder { return; }; + // Eager vs lazy snapshot for this free variable. Eager snapshots are + // precise, lazy ones over-approximate. For instance in: + // + // ``` + // x <- 1 + // local({ x }) + // x <- 2 + // ``` + // + // The eager body in `local()` captures `x <- 1` but not `x <- 2`. If + // the body was inside a lazy context like `function()` instead, the use + // of `x` could run at any time and we'd fall back to the accumulated + // union `{1, 2}`, which is an over-approximation. + // + // A precise enclosing snapshot requires eagerness throughout, which we + // track with `all_eager`. + let mut all_eager = !self.scopes[self.current_scope].kind.is_lazy(); + loop { - let found_by_flag = self.symbol_tables[current_scope] - .id(name) - .is_some_and(|sym_id| { - self.symbol_tables[current_scope] - .symbol(sym_id) - .flags() - .contains(SymbolFlags::IS_BOUND) - }); - - let found_by_prescan = self.pre_scans[current_scope].has_name(name); - - if found_by_flag || found_by_prescan { + if self.scope_binds_anywhere(current_scope, name) { // Intern with empty flags: we just need a stable `SymbolId` for - // the lookup key. If found via `found_by_flag`, the symbol - // already exists with `IS_BOUND`. If found via pre-scan only, - // the later `add_definition` call during the full walk will set - // `IS_BOUND`. + // the lookup key. If the symbol was found via its `IS_BOUND` + // flag, it already exists. If found via pre-scan only, the later + // `add_definition()` call during the full walk will set `IS_BOUND`. let enclosing_symbol_id = self.symbol_tables[current_scope].intern(name, SymbolFlags::empty()); @@ -297,14 +458,22 @@ impl SemanticIndexBuilder { } self.use_def_maps[current_scope].ensure_symbol(enclosing_symbol_id); - let snapshot_id = self.use_def_maps[current_scope] - .register_enclosing_snapshot(enclosing_symbol_id); + + let snapshot_id = if all_eager { + self.use_def_maps[current_scope].register_eager_snapshot(enclosing_symbol_id) + } else { + self.use_def_maps[current_scope].register_lazy_snapshot(enclosing_symbol_id) + }; self.enclosing_snapshots .insert(use_key, (current_scope, snapshot_id)); return; } + if self.scopes[current_scope].kind.is_lazy() { + all_eager = false; + } + let Some(parent) = self.scopes[current_scope].parent else { return; }; @@ -312,6 +481,36 @@ impl SemanticIndexBuilder { } } + /// Whether `scope` binds `name` in its flow state so far: some definition + /// reaches this point on the control-flow paths up to here. A name never + /// interned in `scope` has nothing binding it, so it counts as unbound. + /// + /// Used for eager scopes, see + /// [`scope_binds_anywhere`](Self::scope_binds_anywhere) for the + /// flow-insensitive variant for lazy scopes. + fn scope_binds_so_far(&self, scope: ScopeId, name: &str) -> bool { + match self.symbol_tables[scope].id(name) { + Some(symbol_id) => !self.use_def_maps[scope].is_unbound(symbol_id), + None => false, + } + } + + /// Whether `scope` binds `name` anywhere, regardless of flow position: an + /// already-recorded `IS_BOUND` definition or a pre-scanned assignment. The + /// pre-scan covers definitions the walk hasn't reached yet in this scope. + /// + /// Used for lazy scopes, see `scope_binds_so_far` for the flow-sensitive + /// variant for eager scopes. + fn scope_binds_anywhere(&self, scope: ScopeId, name: &str) -> bool { + let found_by_flag = self.symbol_tables[scope].id(name).is_some_and(|sym_id| { + self.symbol_tables[scope] + .symbol(sym_id) + .flags() + .contains(SymbolFlags::IS_BOUND) + }); + found_by_flag || self.pre_scans[scope].has_name(name) + } + // --- Recursive descent --- fn collect_expression_list(&mut self, list: &RExpressionList) { @@ -366,16 +565,20 @@ impl SemanticIndexBuilder { // clauses contain `RIdentifier` nodes that should not be recorded // as uses. AnyRExpression::RCall(call) => { + // Record the callee as a use (a no-op for `pkg::fn`) before + // resolving NSE. That interns the callee symbol, so + // `resolve_nse()` can look it up by name and read whether it's + // bound at this point. if let Ok(func) = call.function() { self.collect_expression(&func); } - if let Ok(args) = call.arguments() { + + if let Some(annotation) = self.resolve_nse(call) { + self.collect_nse_call(call, annotation) + } else if let Ok(args) = call.arguments() { self.collect_arguments(&args.items()); } - // TODO(nse): When eager NSE scopes land (e.g. `local()`) we should - // also consider nested scopes as long as they're not lazy (e.g. - // function definitions or NSE calls that don't evaluate - // immediately. + self.collect_semantic_call(call); }, AnyRExpression::RSubset(subset) => { @@ -567,19 +770,31 @@ impl SemanticIndexBuilder { /// definition yet. Must stay in sync with the full walk's definition /// handling: any construct that calls `add_definition` should have a /// corresponding entry here. - fn pre_scan_scope(&mut self, node: &RSyntaxNode) { - let mut preorder = node.preorder(); + fn pre_scan_scope(&mut self, root: &RSyntaxNode) { + let mut preorder = root.preorder(); while let Some(event) = preorder.next() { let WalkEvent::Enter(node) = event else { continue; }; + let is_root = &node == root; let Some(expr) = AnyRExpression::cast(node) else { continue; }; + + // On the re-walk, skip nested NSE scope bodies, just like we skip + // function bodies: their definitions belong to the child scope, not + // the scope being pre-scanned. The root is the scope being + // pre-scanned, which may itself be an NSE body, so it's never + // skipped. `nse_nested_ranges` is empty on the first walk. + if !is_root && + self.nse_nested_ranges + .contains(&expr.syntax().text_trimmed_range()) + { + preorder.skip_subtree(); + continue; + } + match &expr { - // NSE scopes (e.g. `local({...})`) will also need to - // be skipped here once recognized, since their - // definitions belong to a child scope. AnyRExpression::RFunctionDefinition(_) => { preorder.skip_subtree(); }, @@ -589,15 +804,14 @@ impl SemanticIndexBuilder { let right = is_right_assignment(bin); let target = if right { bin.right() } else { bin.left() }; if let Ok(target) = target { - if let Some((name, range)) = assignment_name(&target) { - self.pre_scans[self.current_scope].add(name, range); + if let Some((name, _)) = assignment_name(&target) { + self.pre_scans[self.current_scope].add(name); } } }, AnyRExpression::RForStatement(stmt) => { if let Ok(variable) = stmt.variable() { - self.pre_scans[self.current_scope] - .add(variable.name_text(), variable.syntax().text_trimmed_range()); + self.pre_scans[self.current_scope].add(variable.name_text()); } }, _ => {}, @@ -948,36 +1162,22 @@ impl SemanticIndexBuilder { /// semantics don't match across definitions, we pick one and lint). Intra-scope /// resolution is linear and uses the current `symbol_states` directly instead. struct PreScanScope { - _defs: Vec, - by_name: FxHashMap>, -} - -/// A single definition site found during the pre-scan. Fields are not -/// read yet but will be used for NSE lookup. -struct PreScanDef { - _name: String, - _range: TextRange, + by_name: FxHashSet, } impl PreScanScope { fn new() -> Self { Self { - _defs: Vec::new(), - by_name: FxHashMap::default(), + by_name: FxHashSet::default(), } } - fn add(&mut self, name: String, range: TextRange) { - let idx = self._defs.len(); - self.by_name.entry(name.clone()).or_default().push(idx); - self._defs.push(PreScanDef { - _name: name, - _range: range, - }); + fn add(&mut self, name: String) { + self.by_name.insert(name); } fn has_name(&self, name: &str) -> bool { - self.by_name.contains_key(name) + self.by_name.contains(name) } } diff --git a/crates/oak_semantic/src/builder/builder_nse.rs b/crates/oak_semantic/src/builder/builder_nse.rs new file mode 100644 index 0000000000..3b887d6ccd --- /dev/null +++ b/crates/oak_semantic/src/builder/builder_nse.rs @@ -0,0 +1,305 @@ +use aether_syntax::AnyRArgumentName; +use aether_syntax::AnyRExpression; +use aether_syntax::RArgumentList; +use aether_syntax::RCall; +use biome_rowan::AstNode; +use biome_rowan::AstSeparatedList; +use oak_core::syntax_ext::AnyRSelectorExt; +use oak_core::syntax_ext::RIdentifierExt; +use oak_core::syntax_ext::RStringValueExt; +use stdext::debug_panic; + +use super::SemanticIndexBuilder; +use crate::effects::Effects; +use crate::effects::NseAnnotation; +use crate::effects::NseArgument; +use crate::effects_registry; +use crate::resolver::ImportsResolver; +use crate::semantic_index::NseScope; +use crate::semantic_index::NseTiming; +use crate::semantic_index::ScopeKind; +use crate::semantic_index::SymbolId; + +impl SemanticIndexBuilder { + /// Resolve a call's callee to an NSE annotation. + /// + /// Two cases resolve here: + /// - A bare identifier. If the callee is unbound, it is resolved + /// through the cross-file `ImportsResolver::resolve_effects()` method. + /// If bound locally, we'll resolve the annotations here - TODO(nse, annotations). + /// - 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 + /// per-package knowledge (the static registry, plus cross-file knowledge + /// like the re-export chase once that lands). + pub(super) fn resolve_nse(&mut self, call: &RCall) -> Option { + let func = call.function().ok()?; + + match &func { + AnyRExpression::RIdentifier(ident) => { + let name = ident.name_text(); + + // Bail early if it is known that no package annotates this name + // with effects. This speeds up the common case of no known annotations. + if !effects_registry::is_annotated(&name) { + return None; + } + + let Some(symbol_id) = self.symbol_tables[self.current_scope].id(&name) else { + debug_panic!( + "Callee `{name}` not interned: collect_expression should have run first" + ); + return None; + }; + + // First check for a local definition (which in the future will + // potentially contain NSE annotations) + if self.is_locally_bound(&name) { + return self + .resolve_effects(symbol_id) + .and_then(|effects| effects.nse); + } + + // Now check imports since the symbol is locally unbound + self.resolver + .resolve_effects(&name, &[], false) + .and_then(|effects| effects.nse) + }, + + AnyRExpression::RNamespaceExpression(ns_expr) => { + let left = ns_expr.left().ok()?; + let right = ns_expr.right().ok()?; + let pkg = left.identifier_text()?; + let func_name = right.identifier_text()?; + + if !effects_registry::is_annotated(&func_name) { + return None; + } + + self.resolver + .resolve_qualified_effects(&pkg, &func_name) + .and_then(|effects| effects.nse) + }, + + _ => None, + } + } + + /// Local resolver for declared effects, mirroring the imports revoler's + /// `resolve_effects()` method on the cross-file side. + /// TODO(nse, annotations): always `None` until `declare()` parsing lands. + fn resolve_effects(&self, _symbol_id: SymbolId) -> Option { + None + } + + /// Whether the current scope or an enclosing one binds `name`, shadowing + /// the base NSE callee. The current scope is always flow-precise. For + /// ancestors, crossing a lazy scope (e.g. a function body) loses the + /// accuracy because we don't know when the lazy scope runs and need to + /// consider the whole scope bindings, not just the ones currently live. + /// + /// Invariant: The eager/lazy decision must match the decision in + /// `register_enclosing_snapshot()`. If they disagree, a call flips between + /// NSE and not-NSE across re-walks and the fixpoint never settles. + fn is_locally_bound(&self, name: &str) -> bool { + if self.scope_binds_so_far(self.current_scope, name) { + return true; + } + + let Some(mut scope) = self.scopes[self.current_scope].parent else { + return false; + }; + let mut all_eager = !self.scopes[self.current_scope].kind.is_lazy(); + + loop { + let bound = if all_eager { + self.scope_binds_so_far(scope, name) + } else { + self.scope_binds_anywhere(scope, name) + }; + if bound { + return true; + } + + if self.scopes[scope].kind.is_lazy() { + all_eager = false; + } + + let Some(parent) = self.scopes[scope].parent else { + return false; + }; + scope = parent; + } + } + + /// Process a call already recognized as NSE. Match its arguments against the + /// annotation, then handle each scoped argument. + /// + /// For every scoped argument we record the decision. That sets `found_nse`. + /// For `Nested` arguments it also notes the body range which allows + /// pre-scans to skip it. + /// + /// How we walk the body depends on the phase. The first walk keeps it flat + /// (no nested scope), so its definitions land in the current scope. The + /// re-walk pushes the NSE scope and walks the body inside it. + pub(super) fn collect_nse_call(&mut self, call: &RCall, annotation: NseAnnotation) { + let Ok(args) = call.arguments() else { + return; + }; + let items = args.items(); + let nse_args = self.match_nse_args(&items, annotation); + + for (i, item) in items.iter().enumerate() { + let Ok(arg) = item else { continue }; + let Some(value) = arg.value() else { continue }; + + let Some(nse_arg) = nse_args[i] else { + self.collect_expression(&value); + continue; + }; + + self.record_nse_argument(nse_arg, &value); + + if self.is_rewalk { + // On rewalks, we push nested NSE scopes and collect definitions there + self.collect_nse_argument(nse_arg, &value); + } else { + // On the first walk, keep flat and collect definitions in the current scope + self.collect_expression(&value); + } + } + } + + /// Match a call's arguments against an NSE annotation. Returns, per argument + /// in call order, the scoped argument it matched (if any). Named arguments + /// match first, then unmatched positions fill by call-site position. + /// + /// FIXME: This is a stopgap helper. In the future, `Effects` will be + /// returned from the resolvers with the function signature, and we'll + /// implement a proper argument matching routine. + fn match_nse_args( + &self, + items: &RArgumentList, + annotation: NseAnnotation, + ) -> Vec> { + let arg_count = items.iter().count(); + let mut nse_args: Vec> = vec![None; arg_count]; + let mut consumed = vec![false; annotation.arguments.len()]; + + // Named pass + for (i, item) in items.iter().enumerate() { + let Ok(arg) = item else { continue }; + if let Some(nse_idx) = match_named_arg(&arg, &annotation, &consumed) { + consumed[nse_idx] = true; + nse_args[i] = Some(&annotation.arguments[nse_idx]); + } + } + + // Positional pass. Only unnamed args reach the match, and none of them + // were set by the named pass, so no need to re-check `nse_args[i]`. + let mut position = 0usize; + for (i, item) in items.iter().enumerate() { + let Ok(arg) = item else { + position += 1; + continue; + }; + if arg.name_clause().is_some() { + position += 1; + continue; + } + if let Some(scoped_idx) = match_positional_arg(&annotation, position, &consumed) { + consumed[scoped_idx] = true; + nse_args[i] = Some(&annotation.arguments[scoped_idx]); + } + position += 1; + } + + nse_args + } + + fn record_nse_argument(&mut self, scoped: &NseArgument, value: &AnyRExpression) { + match (scoped.scope, scoped.timing) { + // Doesn't push a scope, nothing to record. + (NseScope::Current, NseTiming::Eager) => {}, + // Routes to the parent. No body range to skip, but still a virtual scope. + (NseScope::Current, NseTiming::Lazy) => { + self.found_nse = true; + }, + // Note the body range so pre-scans skip it. + (NseScope::Nested, _) => { + self.found_nse = true; + self.nse_nested_ranges + .insert(value.syntax().text_trimmed_range()); + }, + } + } + + /// Walk a single NSE argument body, pushing a scope when appropriate. + fn collect_nse_argument(&mut self, nse_arg: &NseArgument, value: &AnyRExpression) { + match (nse_arg.scope, nse_arg.timing) { + (NseScope::Current, NseTiming::Eager) => { + self.collect_expression(value); + }, + + (nse_scope, nse_timing) => { + let kind = ScopeKind::Nse(nse_scope, nse_timing); + let scope = self.push_scope(kind, value.syntax().text_trimmed_range()); + + // Only `Nested` scopes hold their own definitions and get pre-scanned + if nse_scope == NseScope::Nested { + self.pre_scan_scope(value.syntax()); + } + + self.collect_expression(value); + self.pop_scope(scope); + }, + } + } +} + +/// Match a named argument against the annotation's arguments. Returns the +/// index into `annotation.arguments` if matched. +/// +/// Should we do partial argument matching? Or rely on partial matching being linted? +fn match_named_arg( + arg: &aether_syntax::RArgument, + annotation: &NseAnnotation, + consumed: &[bool], +) -> Option { + let clause = arg.name_clause()?; + let name = clause.name().ok()?; + let name_text = match &name { + AnyRArgumentName::RIdentifier(ident) => ident.name_text(), + AnyRArgumentName::RStringValue(s) => s.string_text()?, + _ => return None, + }; + annotation + .arguments + .iter() + .enumerate() + .find(|(i, nse_arg)| !consumed[*i] && nse_arg.name == name_text.as_str()) + .map(|(i, _)| i) +} + +/// Match an unnamed argument at `position` against the annotation's arguments. +/// Returns the index into `annotation.arguments` if matched. +/// +/// FIXME: This matches positionally on call-site position only: an unnamed +/// argument at position N matches an annotation argument declared at position +/// N. It doesn't replicate R's full matching, where named arguments are pulled +/// out first and the rest fill the remaining formals in order. So `test_that({ +/// ... }, desc = "d")`, with the block at position 0 but the `code` formal at +/// position 1, won't match. Good enough without the callee's formal list; +/// revisit if it misses real cases. +fn match_positional_arg( + annotation: &NseAnnotation, + position: usize, + consumed: &[bool], +) -> Option { + annotation + .arguments + .iter() + .enumerate() + .find(|(i, scoped)| !consumed[*i] && scoped.position == position) + .map(|(i, _)| i) +} diff --git a/crates/oak_semantic/src/effects.rs b/crates/oak_semantic/src/effects.rs new file mode 100644 index 0000000000..d576b5161b --- /dev/null +++ b/crates/oak_semantic/src/effects.rs @@ -0,0 +1,33 @@ +use crate::semantic_index::NseScope; +use crate::semantic_index::NseTiming; + +/// Effects of a resolved function. +/// +/// Currently only records NSE effects. In the future this will include other +/// effects such as `attach` (for e.g. `library()`) and `assign` (for the +/// eponymous function). +#[derive(Debug, Clone, Copy, Default)] +pub struct Effects { + pub nse: Option, +} + +impl Effects { + pub fn nse(nse: NseAnnotation) -> Self { + Self { nse: Some(nse) } + } +} + +/// Annotation describing how an NSE function's arguments create scopes. +#[derive(Debug, Clone, Copy)] +pub struct NseAnnotation { + pub arguments: &'static [NseArgument], +} + +/// A single argument that creates an NSE scope. +#[derive(Debug)] +pub struct NseArgument { + pub name: &'static str, + pub position: usize, + pub scope: NseScope, + pub timing: NseTiming, +} diff --git a/crates/oak_semantic/src/effects_registry.rs b/crates/oak_semantic/src/effects_registry.rs new file mode 100644 index 0000000000..d1afdae87e --- /dev/null +++ b/crates/oak_semantic/src/effects_registry.rs @@ -0,0 +1,68 @@ +use crate::effects::NseAnnotation; +use crate::effects::NseArgument; +use crate::semantic_index::NseScope::Current; +use crate::semantic_index::NseScope::Nested; +use crate::semantic_index::NseTiming::Eager; +use crate::semantic_index::NseTiming::Lazy; + +struct Entry { + package: &'static str, + function: &'static str, + annotation: NseAnnotation, +} + +/// Look up the NSE annotation for a `(package, function)` pair. +pub fn lookup(package: &str, function: &str) -> Option<&'static NseAnnotation> { + REGISTRY + .iter() + .find(|e| e.package == package && e.function == function) + .map(|e| &e.annotation) +} + +/// Whether any registry entry annotates `name`. This is the bare-callee front +/// gate: an unannotated name can't resolve to an effect no matter which provider +/// wins, so recognition skips resolution entirely. +pub fn is_annotated(name: &str) -> bool { + REGISTRY.iter().any(|e| e.function == name) +} + +/// One registry entry. Each `(name, position, scope, laziness)` tuple is a +/// scoped argument; list more than one for a function that scopes several. +macro_rules! entry { + ($pkg:literal, $func:literal, $(($name:literal, $pos:literal, $scope:expr, $timing:expr)),+ $(,)?) => { + Entry { + package: $pkg, + function: $func, + annotation: NseAnnotation { + arguments: &[$(NseArgument { + name: $name, + position: $pos, + scope: $scope, + timing: $timing, + }),+], + }, + } + }; +} + +static REGISTRY: &[Entry] = &[ + // base + entry!("base", "evalq", ("expr", 0, Current, Eager)), + entry!("base", "local", ("expr", 0, Nested, Eager)), + entry!("base", "with", ("expr", 1, Nested, Eager)), + entry!("base", "with.default", ("expr", 1, Nested, Eager)), + entry!("base", "within", ("expr", 1, Nested, Eager)), + entry!("base", "within.data.frame", ("expr", 1, Nested, Eager)), + // rlang + entry!("rlang", "on_load", ("expr", 0, Current, Lazy)), + // shiny + entry!("shiny", "observe", ("x", 0, Nested, Lazy)), + entry!("shiny", "reactive", ("x", 0, Nested, Lazy)), + entry!("shiny", "renderPlot", ("expr", 0, Nested, Lazy)), + entry!("shiny", "renderPrint", ("expr", 0, Nested, Lazy)), + entry!("shiny", "renderTable", ("expr", 0, Nested, Lazy)), + entry!("shiny", "renderText", ("expr", 0, Nested, Lazy)), + entry!("shiny", "renderUI", ("expr", 0, Nested, Lazy)), + // testthat + entry!("testthat", "test_that", ("code", 1, Nested, Eager)), +]; diff --git a/crates/oak_semantic/src/lib.rs b/crates/oak_semantic/src/lib.rs index 9994569506..233eb97eb3 100644 --- a/crates/oak_semantic/src/lib.rs +++ b/crates/oak_semantic/src/lib.rs @@ -1,9 +1,12 @@ pub mod builder; +pub mod effects; +pub mod effects_registry; pub mod resolver; pub mod semantic_index; pub mod use_def_map; pub use builder::build_index; +pub use effects::Effects; pub use resolver::ImportsResolver; pub use resolver::NoopImportsResolver; pub use resolver::SourceResolution; diff --git a/crates/oak_semantic/src/resolver.rs b/crates/oak_semantic/src/resolver.rs index bd54ccb3dc..65b442c0b1 100644 --- a/crates/oak_semantic/src/resolver.rs +++ b/crates/oak_semantic/src/resolver.rs @@ -1,5 +1,8 @@ use url::Url; +use crate::effects::Effects; +use crate::effects_registry; + /// The result of resolving a `source()` call. Returned by /// [`ImportsResolver::resolve_source`]. #[derive(Clone)] @@ -28,14 +31,15 @@ pub struct SourceResolution { /// for isolated indexing (CLI tools, unit tests). /// - `oak_db::SalsaImportsResolver`: salsa-backed lookup against the source graph. /// -/// The trait grows along two axes as new analyses land: +/// The trait has three queries: /// /// - [`resolve_source`](ImportsResolver::resolve_source) is the bulk /// query, "enumerate every name this `source("path")` brings in," used /// to inject `DefinitionKind::Import` entries at each source() offset. -/// - A future `resolve_name(scope, name)` is the point query, "find -/// the import that resolves this specific name in this scope," used by -/// NSE call-site analysis. +/// - [`resolve_effects`](ImportsResolver::resolve_effects) resolves a bare +/// callee against imports, e.g. the search path, and returns known effects. +/// - [`resolve_qualified_effects`](ImportsResolver::resolve_qualified_effects) +/// resolves the effects of a `pkg::fn` (or `:::) callee against a named package. pub trait ImportsResolver { /// Resolve a `source("path")` call to the target file's exported names /// and transitive `library()` attachments. The path is the literal @@ -43,6 +47,25 @@ pub trait ImportsResolver { /// anchoring it (workspace root, calling file's directory, ...). /// Returns `None` when the target can't be located. fn resolve_source(&mut self, path: &str) -> Option; + + /// Resolve a bare callee `name` to its effects. The builder state is passed + /// in because the resolver can't query our own semantic index without + /// creating a cycle: + /// + /// - `attached`: packages attached at this point, in flow order. + /// - `lazy`: whether the callee sits in a lazy context like a function. + fn resolve_effects(&mut self, name: &str, attached: &[String], lazy: bool) -> Option { + let _ = (name, attached, lazy); + None + } + + /// Resolve a namespace-qualified callee `pkg::fn` (or equivalently with + /// `:::`) to its effects. + fn resolve_qualified_effects(&mut self, package: &str, name: &str) -> Option { + effects_registry::lookup(package, name) + .copied() + .map(Effects::nse) + } } /// Resolver that returns nothing. The builder skips all cross-file diff --git a/crates/oak_semantic/src/semantic_index.rs b/crates/oak_semantic/src/semantic_index.rs index 12ca4d7d44..9f3406cb62 100644 --- a/crates/oak_semantic/src/semantic_index.rs +++ b/crates/oak_semantic/src/semantic_index.rs @@ -366,11 +366,6 @@ impl SemanticIndex { /// symbol's `SymbolId` in the nested scope's symbol table (not the enclosing /// scope's), so consumers can do an O(1) lookup directly from a `UseId` without /// re-walking the ancestor chain. -/// -/// When we implement NSE, we will add a `laziness: ScopeLaziness` field to -/// distinguish lazy snapshots (functions, accumulated union via watchers) from -/// eager snapshots (NSE scopes like `local()`, point-in-time capture at the -/// call site). Currently all nested scopes are lazy, so the field is omitted. #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct EnclosingSnapshotKey { pub nested_scope: ScopeId, @@ -384,9 +379,9 @@ pub struct EnclosingSnapshotKey { // by the file: walking `parent` from any scope eventually reaches the // `File` scope which itself has `parent: None`. // -// Currently only `function()` creates a new scope. In the future, constructs -// like `local()`, `with()`, `within()` may also create scopes (determined -// by function declarations resolved via salsa queries). +// `function()` creates `Function` scopes. NSE constructs like `local()`, +// `with()`, `test_that()` create `Nse` scopes, recognized by resolving the +// call target against for their effects annotations during the walk. #[derive(Debug, PartialEq, Eq)] pub struct Scope { pub(crate) parent: Option, @@ -405,6 +400,40 @@ pub enum ScopeKind { // cross-file resolution (package namespace, session, etc.) takes over. File, Function, + Nse(NseScope, NseTiming), +} + +/// Where definitions in an NSE scope land. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum NseScope { + /// Definitions go to the current (parent) environment. + /// e.g. `rlang::on_load()` + Current, + /// Definitions go to a nested environment. + /// e.g. `local()`, `test_that()`, `with()` + Nested, +} + +/// Whether an NSE scope evaluates eagerly (at the call site) or lazily +/// (at an unknown later time). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum NseTiming { + Eager, + Lazy, +} + +impl ScopeKind { + /// Whether free variables in this scope resolve against the union of all + /// enclosing definitions (lazy) or against a point-in-time snapshot at the + /// call site (eager). `Function` bodies run at an unknown later time, so + /// they're always lazy. + pub fn is_lazy(self) -> bool { + match self { + ScopeKind::File => false, + ScopeKind::Function => true, + ScopeKind::Nse(_, laziness) => laziness == NseTiming::Lazy, + } + } } impl Scope { diff --git a/crates/oak_semantic/src/use_def_map.rs b/crates/oak_semantic/src/use_def_map.rs index c66a6eb784..afa97aedb9 100644 --- a/crates/oak_semantic/src/use_def_map.rs +++ b/crates/oak_semantic/src/use_def_map.rs @@ -186,10 +186,12 @@ use crate::semantic_index::UseId; // The consumer combines both: the local bindings and the enclosing // snapshot give the full picture of what `x` could be. // -// For eager NSE scopes (e.g. `local()`), the snapshot will be even more -// precise: since the body executes at the call site, the snapshot is a -// point-in-time capture with no watcher, reflecting exactly the linear state. -// No union over-approximation needed. +// For eager NSE scopes (e.g. `local()`), the snapshot is more precise: since +// the body executes at the call site, it's a point-in-time capture reflecting +// exactly the linear state, with no union over definitions that come later in +// the enclosing scope. The one exception is a `<<-` inside the eager body: it +// mutates the enclosing binding mid-run, so its watcher fires for deferred +// definitions only (see `register_eager_snapshot()`). /// The immutable use-def map for a single scope. For each use site, stores the /// set of definitions that can reach it through control flow. @@ -304,7 +306,11 @@ pub(crate) struct UseDefMapBuilder { // Currently used for `<<-` extra definitions in ancestor scopes. deferred_defs: Vec<(SymbolId, DefinitionId)>, enclosing_snapshots: IndexVec, - snapshot_watchers: FxHashMap>, + // Snapshots subscribed to every definition of a symbol (lazy snapshots). + def_watchers: FxHashMap>, + // Snapshots subscribed only to deferred (`<<-`) definitions of a symbol + // (eager snapshots). + deferred_def_watchers: FxHashMap>, } impl UseDefMapBuilder { @@ -314,7 +320,8 @@ impl UseDefMapBuilder { bindings_by_use: IndexVec::new(), deferred_defs: Vec::new(), enclosing_snapshots: IndexVec::new(), - snapshot_watchers: FxHashMap::default(), + def_watchers: FxHashMap::default(), + deferred_def_watchers: FxHashMap::default(), } } @@ -333,7 +340,12 @@ impl UseDefMapBuilder { /// live definitions for that symbol. pub(crate) fn record_definition(&mut self, symbol_id: SymbolId, def_id: DefinitionId) { self.symbol_states[symbol_id].record_definition(def_id); - self.update_enclosing_snapshots(symbol_id, def_id); + Self::notify_watchers( + &mut self.enclosing_snapshots, + &self.def_watchers, + symbol_id, + def_id, + ); } /// After visiting a loop body, retroactively patch uses so that @@ -396,7 +408,20 @@ impl UseDefMapBuilder { pub(crate) fn record_deferred_definition(&mut self, symbol_id: SymbolId, def_id: DefinitionId) { self.symbol_states[symbol_id].add_definition(def_id); self.deferred_defs.push((symbol_id, def_id)); - self.update_enclosing_snapshots(symbol_id, def_id); + // A deferred def reaches both channels: lazy snapshots (like any def) + // and eager snapshots (they subscribe to deferred defs only). + Self::notify_watchers( + &mut self.enclosing_snapshots, + &self.def_watchers, + symbol_id, + def_id, + ); + Self::notify_watchers( + &mut self.enclosing_snapshots, + &self.deferred_def_watchers, + symbol_id, + def_id, + ); } /// Record a use of `symbol_id`. Clones the current live bindings for that @@ -453,28 +478,80 @@ impl UseDefMapBuilder { self.symbol_states[symbol_id].may_be_unbound() } + /// Returns `true` if `symbol_id` is definitely unbound at this point: no + /// definition reaches it on any control-flow path. + pub(crate) fn is_unbound(&self, symbol_id: SymbolId) -> bool { + let state = &self.symbol_states[symbol_id]; + state.may_be_unbound() && state.definitions().is_empty() + } + /// Register an enclosing snapshot for `symbol_id`. The snapshot starts from /// the current flow state (prior shadowing applied). A watcher is /// registered so that each subsequent definition of this symbol we /// encounter is conservatively merged in, because we can't know statically /// when the nested scope will be called. - pub(crate) fn register_enclosing_snapshot( - &mut self, - symbol_id: SymbolId, - ) -> EnclosingSnapshotId { + pub(crate) fn register_lazy_snapshot(&mut self, symbol_id: SymbolId) -> EnclosingSnapshotId { let bindings = self.symbol_states[symbol_id].clone(); let id = self.enclosing_snapshots.push(bindings); - self.snapshot_watchers + self.def_watchers.entry(symbol_id).or_default().push(id); + id + } + + /// Register a point-in-time enclosing snapshot for `symbol_id`. Used for + /// eager NSE scopes like `local()`: the body runs at the call site, so the + /// snapshot reflects exactly the linear state, with no union over the + /// definitions that come later in the enclosing scope. + /// + /// Unlike [`register_lazy_snapshot`](Self::register_lazy_snapshot), this + /// watcher fires only on deferred (`<<-`) definitions. A `<<-` inside the + /// eager body changes the binding while the body runs, and uses later in + /// the body must see that change. A plain `<-` after the eager call on the + /// other hand runs once the body has finished, so it stays out of the + /// snapshot. One snapshot is shared by every use of the symbol in the body, + /// so a use before the `<<-` picks it up too. That is a known over-approximation. + /// + /// ```r + /// x <- 1 + /// local({ + /// x # {1, 2}: Should be {1} but shares one snapshot with the use below + /// x <<- 2 # deferred def, folded into the snapshot + /// x # {1, 2} + /// }) + /// x <- 3 # plain `<-` after the call, stays out of the snapshot + /// ``` + /// + /// The watcher keys on the symbol, not on where the def came from. It also + /// picks up deferred defs from outside this body. One is a `<<-` in a + /// function defined after the eager call. Another is a `Current + Lazy` + /// routing like `rlang::on_load`. Neither can reach the body, which already + /// ran. Both are safe over-approximations, the same kind as the pre-`<<-` + /// use above. + /// + /// ```r + /// x <- 1 + /// local({ x }) # {1} + /// f <- function() { x <<- 2 } # {1, 2}: f's `<<-` can't reach the finished body + /// rlang::on_load({ x <- 3 }) # {1, 2, 3}: routed def can't reach it either + /// ``` + pub(crate) fn register_eager_snapshot(&mut self, symbol_id: SymbolId) -> EnclosingSnapshotId { + let bindings = self.symbol_states[symbol_id].clone(); + let id = self.enclosing_snapshots.push(bindings); + self.deferred_def_watchers .entry(symbol_id) .or_default() .push(id); id } - fn update_enclosing_snapshots(&mut self, symbol_id: SymbolId, def_id: DefinitionId) { - if let Some(watchers) = self.snapshot_watchers.get(&symbol_id) { - for &snapshot_id in watchers { - self.enclosing_snapshots[snapshot_id].add_definition(def_id); + fn notify_watchers( + enclosing_snapshots: &mut IndexVec, + watchers: &FxHashMap>, + symbol_id: SymbolId, + def_id: DefinitionId, + ) { + if let Some(ids) = watchers.get(&symbol_id) { + for &snapshot_id in ids { + enclosing_snapshots[snapshot_id].add_definition(def_id); } } } diff --git a/crates/oak_semantic/tests/integration/builder_nse.rs b/crates/oak_semantic/tests/integration/builder_nse.rs new file mode 100644 index 0000000000..ca7da3531e --- /dev/null +++ b/crates/oak_semantic/tests/integration/builder_nse.rs @@ -0,0 +1,861 @@ +use aether_parser::parse; +use aether_parser::RParserOptions; +use oak_semantic::build_index; +use oak_semantic::semantic_index::DefinitionId; +use oak_semantic::semantic_index::NseScope; +use oak_semantic::semantic_index::NseTiming; +use oak_semantic::semantic_index::ScopeId; +use oak_semantic::semantic_index::ScopeKind; +use oak_semantic::semantic_index::SemanticIndex; +use oak_semantic::semantic_index::SymbolFlags; +use oak_semantic::semantic_index::UseId; +use oak_semantic::NoopImportsResolver; + +use crate::resolvers::TestImportsResolver; + +fn index(source: &str) -> SemanticIndex { + build_with(source, TestImportsResolver::with_base()) +} + +fn build_with(source: &str, resolver: impl oak_semantic::ImportsResolver) -> SemanticIndex { + let parsed = parse(source, RParserOptions::default()); + + if parsed.has_error() { + panic!("source has syntax errors: {source}"); + } + + build_index(&parsed.tree(), resolver) +} + +// --- NSE scopes --- + +#[test] +fn test_nse_local_creates_nested_eager_scope() { + let index = index( + "\ +local({ + x <- 1 +}) +", + ); + let file = ScopeId::from(0); + let local_scope = ScopeId::from(1); + + // `local` is used at file scope + assert_eq!(index.symbols(file).len(), 1); + assert_eq!( + index.symbols(file).get("local").unwrap().flags(), + SymbolFlags::IS_USED + ); + + // `x` is defined inside the NSE scope, not at file level + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(local_scope).parent(), Some(file)); + assert_eq!(index.symbols(local_scope).len(), 1); + assert_eq!( + index.symbols(local_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_local_definition_not_in_parent() { + // Definitions inside `local()` should NOT leak to the file scope. + let index = index( + "\ +local({ + x <- 1 +}) +x +", + ); + let file = ScopeId::from(0); + + // `x` at file scope is only IS_USED (from the bare `x` on the last line), + // not IS_BOUND (from the assignment inside local). + let x = index.symbols(file).get("x").unwrap(); + assert_eq!(x.flags(), SymbolFlags::IS_USED); +} + +#[test] +fn test_nse_evalq_no_scope_push() { + // `evalq` is Current + Eager: no scope push, walk body in place. + let index = index( + "\ +evalq({ + x <- 1 +}) +", + ); + let file = ScopeId::from(0); + + // Only the file scope exists (plus no child scopes) + assert_eq!( + index.symbols(file).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + // evalq is used, x is bound + assert_eq!(index.symbols(file).len(), 2); +} + +#[test] +fn test_nse_namespace_qualified_call() { + // `testthat::test_that` should be recognized via namespace resolution. + let index = index( + r#" +testthat::test_that("description", { + x <- 1 +}) +"#, + ); + let file = ScopeId::from(0); + let test_scope = ScopeId::from(1); + + // File scope has no symbols (namespace expressions don't record uses) + assert_eq!(index.symbols(file).len(), 0); + + // Test scope contains `x` + assert_eq!( + index.scope(test_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!( + index.symbols(test_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_shadowed_name_no_scope() { + // If `local` is locally defined (shadowed), it's not recognized as NSE. + let index = index( + "\ +local <- identity +local({ + x <- 1 +}) +", + ); + let file = ScopeId::from(0); + + // `local` is defined at file scope, shadowing the base function. + // No NSE scope should be created. `x` is defined at file scope. + assert_eq!( + index.symbols(file).get("local").unwrap().flags(), + SymbolFlags::IS_BOUND.union(SymbolFlags::IS_USED) + ); + assert_eq!( + index.symbols(file).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_ancestor_shadowed_name_no_scope() { + // A `local` binding in an ENCLOSING scope shadows the base function too, + // even when the call site sits in a nested scope where `local` is free. + let index = index( + "\ +local <- function(x) x +f <- function() { + local({ + y <- 1 + }) +} +", + ); + let file = ScopeId::from(0); + let identity_fn = ScopeId::from(1); + let f_scope = ScopeId::from(2); + + // Only three scopes: no NSE scope is pushed for the shadowed `local()`. + assert_eq!(index.scope_ids().count(), 3); + assert_eq!(index.scope(file).kind(), ScopeKind::File); + assert_eq!(index.scope(identity_fn).kind(), ScopeKind::Function); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + + // `local` is bound at file scope. + assert!(index + .symbols(file) + .get("local") + .unwrap() + .flags() + .contains(SymbolFlags::IS_BOUND)); + + // `y` is defined flat in `f`, not moved into an NSE child scope. + assert_eq!( + index.symbols(f_scope).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_forward_def_visible_to_nested_function() { + // A function defined inside an eager NSE body that references a name bound + // LATER in that same body must still resolve to the NSE scope. This relies + // on the NSE scope's own pre-scan seeing the forward definition, which the + // pre-scan must collect despite the body range being a Nested NSE range. + let index = index( + "\ +local({ + f <- function() x + x <- 1 +}) +", + ); + let local_scope = ScopeId::from(1); + let f_scope = ScopeId::from(2); + + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + + // `x` inside `f` resolves to the `local` scope (not the file scope), and + // its lazy snapshot picks up `x <- 1` (DefinitionId 1 in the local scope: + // `f` is DefinitionId 0, `x` is DefinitionId 1). + let x_sym = index.uses(f_scope)[UseId::from(0)].symbol(); + let (enclosing_scope, bindings) = index.enclosing_bindings(f_scope, x_sym).unwrap(); + assert_eq!(enclosing_scope, local_scope); + assert_eq!(bindings.definitions(), &[DefinitionId::from(1)]); +} + +#[test] +fn test_nse_rewalk_moves_definitions() { + // The re-walk should correctly move definitions from the parent scope + // into the NSE child scope. + let index = index( + "\ +x <- 0 +local({ + y <- 1 +}) +z <- 2 +", + ); + let file = ScopeId::from(0); + let local_scope = ScopeId::from(1); + + // File scope: x, z are bound; local is used; y is NOT in file scope + assert!(index.symbols(file).get("x").is_some()); + assert!(index.symbols(file).get("z").is_some()); + assert!(index.symbols(file).get("local").is_some()); + assert!(index.symbols(file).get("y").is_none()); + + // local scope: y is bound + assert_eq!( + index.symbols(local_scope).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_test_that_second_arg() { + // `test_that` has the scoped arg at position 1 (the `code` parameter). + // The first argument (description) should be processed normally. + let index = index( + r#" +testthat::test_that("description", { + x <- 1 + y +}) +"#, + ); + let test_scope = ScopeId::from(1); + + // Inside the test scope + assert_eq!( + index.symbols(test_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + assert_eq!( + index.symbols(test_scope).get("y").unwrap().flags(), + SymbolFlags::IS_USED + ); +} + +#[test] +fn test_nse_named_argument_matching() { + // Named argument matching: `code = {...}` should be recognized. + let index = index( + r#" +testthat::test_that(code = { + x <- 1 +}, desc = "foo") +"#, + ); + let test_scope = ScopeId::from(1); + + assert_eq!( + index.scope(test_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!( + index.symbols(test_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_nested_function_inside_local() { + // A function defined inside `local()` creates a nested Function scope. + let index = index( + "\ +local({ + f <- function(x) x +}) +", + ); + let local_scope = ScopeId::from(1); + let fun_scope = ScopeId::from(2); + + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(fun_scope).kind(), ScopeKind::Function); + assert_eq!(index.scope(fun_scope).parent(), Some(local_scope)); +} + +#[test] +fn test_nse_prescan_skips_nested_bodies() { + // The pre-scan for the file scope should NOT include definitions from + // inside `local()` bodies on the re-walk. This means a function defined + // AFTER the local() call should not see `x` from inside local via the + // pre-scan. + let index = index( + "\ +local({ + x <- 1 +}) +f <- function() x +", + ); + let file = ScopeId::from(0); + let fun_scope = ScopeId::from(2); + + // `x` should NOT be in the file scope + assert!(index.symbols(file).get("x").is_none()); + + // In `f`, `x` is free and unbound -- no enclosing snapshot should find it + // in the file scope. + assert_eq!( + index.enclosing_bindings(fun_scope, index.uses(fun_scope)[UseId::from(0)].symbol()), + None + ); +} + +#[test] +fn test_nse_eager_snapshot_precise() { + // Eager NSE scope at file level should see a point-in-time snapshot: + // only definitions that precede the call site, not later ones. + let index = index( + "\ +x <- 1 +local({ + x +}) +x <- 2 +", + ); + let local_scope = ScopeId::from(1); + + // `x` inside local is free. Its enclosing snapshot should be eager + // (point-in-time). At the call site, only `x <- 1` (DefinitionId 0) is + // live. `x <- 2` (DefinitionId 2) comes after and should NOT be included. + let (enclosing_scope, bindings) = index + .enclosing_bindings( + local_scope, + index.uses(local_scope)[UseId::from(0)].symbol(), + ) + .unwrap(); + assert_eq!(enclosing_scope, ScopeId::from(0)); + assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); + assert!(!bindings.may_be_unbound()); +} + +#[test] +fn test_nse_lazy_snapshot_accumulates() { + // Lazy NSE scope (e.g. inside a function) should accumulate definitions + // via watchers, just like function scopes do. + let index = index( + "\ +x <- 1 +f <- function() { + x +} +x <- 2 +", + ); + let fun_scope = ScopeId::from(1); + + // Function is lazy: snapshot includes both x <- 1 and x <- 2. + let (_, bindings) = index + .enclosing_bindings(fun_scope, index.uses(fun_scope)[UseId::from(0)].symbol()) + .unwrap(); + assert_eq!(bindings.definitions(), &[ + DefinitionId::from(0), + DefinitionId::from(2) + ]); +} + +#[test] +fn test_nse_current_lazy_routes_defs_to_parent() { + // `rlang::on_load` is Current + Lazy: a scope is pushed (for lazy + // snapshot resolution) but definitions route to the parent. + let index = index( + "\ +rlang::on_load({ + x <- 1 +}) +", + ); + let file = ScopeId::from(0); + let nse_scope = ScopeId::from(1); + + // `x` is routed to the file scope + assert_eq!( + index.symbols(file).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + // The NSE scope exists with `Current + Lazy` kind + assert_eq!( + index.scope(nse_scope).kind(), + ScopeKind::Nse(NseScope::Current, NseTiming::Lazy) + ); + assert_eq!(index.scope(nse_scope).parent(), Some(file)); + + // `x` is not in the child scope's symbol table (routed to parent) + assert!(index.symbols(nse_scope).get("x").is_none()); +} + +#[test] +fn test_nse_current_lazy_deferred_definition() { + // `on_load` definitions are deferred (like `<<-`): they add to the set + // of live definitions without shadowing what's already there. + let index = index( + "\ +x <- 1 +rlang::on_load({ + x <- 2 +}) +f <- function() x +", + ); + let fun_scope = ScopeId::from(2); + + // `f` is lazy, so its snapshot for `x` should include BOTH defs: + // `x <- 1` (file-level) and `x <- 2` (from on_load, deferred). + // If on_load's definition shadowed, we'd only see `x <- 2`. + let (enclosing_scope, bindings) = index + .enclosing_bindings(fun_scope, index.uses(fun_scope)[UseId::from(0)].symbol()) + .unwrap(); + assert_eq!(enclosing_scope, ScopeId::from(0)); + assert_eq!(bindings.definitions(), &[ + DefinitionId::from(0), + DefinitionId::from(1) + ]); +} + +#[test] +fn test_nse_rewalk_convergence_unmasked_call() { + // Pathological case: redefining `local` inside a `local()` body unmasks + // a later `local()` call on the re-walk. The convergence loop handles + // this: the first re-walk discovers the second call, the second re-walk + // has the correct pre-scan skip set. + let index = index( + "\ +local({ + local <- identity +}) +local({ + x <- 1 +}) +", + ); + let file = ScopeId::from(0); + let first_local = ScopeId::from(1); + let second_local = ScopeId::from(2); + + // Both calls create Nested + Eager scopes + assert_eq!( + index.scope(first_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!( + index.scope(second_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + + // `local <- identity` is in the first scope, not at file level + assert!(index + .symbols(file) + .get("local") + .unwrap() + .flags() + .contains(SymbolFlags::IS_USED)); + assert!(!index + .symbols(file) + .get("local") + .unwrap() + .flags() + .contains(SymbolFlags::IS_BOUND)); + + // `x <- 1` is in the second scope, not at file level + assert!(index.symbols(file).get("x").is_none()); + assert_eq!( + index.symbols(second_local).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_rewalk_convergence_ancestor_unmask_across_function() { + // Convergence where the ancestor shadowing check flips across iterations, + // and where a single re-walk would leave an observable mistake. + // + // `local <- identity` starts at file scope, so the `local()` call inside + // `f` sees an enclosing binding and is not NSE. The first re-walk moves + // `local <- identity` into the outer local scope, unmasking base `local` + // for the call in `f`, so that call becomes NSE and its body range is + // recorded. But during that same re-walk, `f`'s pre-scan hasn't been told + // to skip the inner local body yet, so it still collects `x`, which would + // give `g`'s free `x` a bogus enclosing snapshot in `f`. Only the next + // re-walk, with the inner body range known, pre-scans `f` without `x` and + // leaves `g`'s `x` correctly unresolved (the sibling `local()` binds `x` in + // its own env, invisible to `g`). + let index = index( + "\ +local({ + local <- identity +}) +f <- function() { + g <- function() x + local({ + x <- 1 + }) +} +", + ); + let file = ScopeId::from(0); + let outer_local = ScopeId::from(1); + let f_scope = ScopeId::from(2); + let g_scope = ScopeId::from(3); + let inner_local = ScopeId::from(4); + + assert_eq!(index.scope_ids().count(), 5); + assert_eq!( + index.scope(outer_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(outer_local).parent(), Some(file)); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + assert_eq!(index.scope(g_scope).kind(), ScopeKind::Function); + assert_eq!(index.scope(g_scope).parent(), Some(f_scope)); + assert_eq!( + index.scope(inner_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(inner_local).parent(), Some(f_scope)); + + // `x <- 1` lands in the inner local scope, not in `f`. + assert!(index.symbols(f_scope).get("x").is_none()); + assert_eq!( + index.symbols(inner_local).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + // `g`'s free `x` resolves to nothing: the sibling `local()` binds `x` in + // its own scope, not in `f`. A single re-walk would wrongly point it at a + // stray `x` in `f`'s pre-scan. + let g_x = index.uses(g_scope)[UseId::from(0)].symbol(); + assert_eq!(index.enclosing_bindings(g_scope, g_x), None); +} + +#[test] +fn test_nse_local_inside_function() { + // `local()` inside a function: the function boundary is lazy, so the + // eager snapshot precision of `local` is bounded by the function's + // laziness. Free variables in `local` resolve through the function's + // lazy snapshot. + let index = index( + "\ +x <- 1 +f <- function() { + local({ + x + }) +} +x <- 2 +", + ); + let fun_scope = ScopeId::from(1); + let local_scope = ScopeId::from(2); + + assert_eq!(index.scope(fun_scope).kind(), ScopeKind::Function); + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(local_scope).parent(), Some(fun_scope)); + + // `x` is free in the local scope, resolves through to the function scope, + // then to the file scope. The function scope is lazy, so both defs are + // visible despite `local` being eager. + let (enclosing_scope, bindings) = index + .enclosing_bindings( + local_scope, + index.uses(local_scope)[UseId::from(0)].symbol(), + ) + .unwrap(); + assert_eq!(enclosing_scope, ScopeId::from(0)); + assert_eq!(bindings.definitions(), &[ + DefinitionId::from(0), + DefinitionId::from(2) + ]); +} + +#[test] +fn test_nse_nested_local_scopes() { + // Nested `local()` inside `local()`: both create child scopes. + let index = index( + "\ +local({ + x <- 1 + local({ + y <- 2 + }) +}) +", + ); + let file = ScopeId::from(0); + let outer_local = ScopeId::from(1); + let inner_local = ScopeId::from(2); + + assert_eq!( + index.scope(outer_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(outer_local).parent(), Some(file)); + assert_eq!( + index.scope(inner_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(inner_local).parent(), Some(outer_local)); + + // Definitions land in their respective scopes + assert!(index.symbols(file).get("x").is_none()); + assert!(index.symbols(file).get("y").is_none()); + assert_eq!( + index.symbols(outer_local).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + assert!(index.symbols(outer_local).get("y").is_none()); + assert_eq!( + index.symbols(inner_local).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_super_assignment_inside_local() { + // `<<-` inside `local()` should target the grandparent (file scope), + // not the local scope itself. + let index = index( + "\ +x <- 1 +local({ + x <<- 2 +}) +", + ); + let file = ScopeId::from(0); + let local_scope = ScopeId::from(1); + + // `x` is bound at file scope (from `x <- 1` and the `<<-`) + assert!(index + .symbols(file) + .get("x") + .unwrap() + .flags() + .contains(SymbolFlags::IS_BOUND)); + + // `x` in the local scope is IS_SUPER_BOUND (the `<<-` site) + assert!(index + .symbols(local_scope) + .get("x") + .unwrap() + .flags() + .contains(SymbolFlags::IS_SUPER_BOUND)); +} + +#[test] +fn test_nse_eager_super_assignment_visible_to_later_use() { + // A `<<-` inside an eager NSE body mutates the enclosing binding mid-run, + // so uses after it must see the `<<-` definition. The eager snapshot is + // shared across all uses of the free variable, so it accumulates the `<<-` + // (a safe over-approximation for the earlier use, correct for the later). + let index = index( + "\ +x <- 1 +local({ + x + x <<- 2 + x +}) +", + ); + let file = ScopeId::from(0); + let local_scope = ScopeId::from(1); + + // The enclosing snapshot for `x` (in the file scope) carries both the + // initial `x <- 1` (DefinitionId 0) and the `<<-` target (DefinitionId 1). + let x_sym = index.uses(local_scope)[UseId::from(0)].symbol(); + let (enclosing_scope, bindings) = index.enclosing_bindings(local_scope, x_sym).unwrap(); + assert_eq!(enclosing_scope, file); + assert_eq!(bindings.definitions(), &[ + DefinitionId::from(0), + DefinitionId::from(1) + ]); +} + +#[test] +fn test_nse_eager_snapshot_absorbs_unrelated_super_assignment() { + // The eager snapshot keys on the symbol in the enclosing scope, so it + // can't tell a `<<-` inside the body from one in a function defined after + // the call. `f`'s `<<-` is recorded on the file scope while its body is + // walked, firing the eager watcher, so `local()`'s snapshot over-includes + // it even though it can't reach the already-run body. + let index = index( + "\ +x <- 1 +local({ + x +}) +f <- function() { + x <<- 2 +} +", + ); + let file = ScopeId::from(0); + let local_scope = ScopeId::from(1); + + // File-scope defs in allocation order: `x <- 1` (0), then f's `<<-` + // target (1, recorded before f's own def since `collect_assignment` walks + // the value side first), then `f` (2). The snapshot absorbs the `<<-`. + let x_sym = index.uses(local_scope)[UseId::from(0)].symbol(); + let (enclosing_scope, bindings) = index.enclosing_bindings(local_scope, x_sym).unwrap(); + assert_eq!(enclosing_scope, file); + assert_eq!(bindings.definitions(), &[ + DefinitionId::from(0), + DefinitionId::from(1) + ]); +} + +#[test] +fn test_nse_eager_snapshot_absorbs_unrelated_routed_definition() { + // `on_load` is `Current + Lazy`, so its `x <- 2` routes to the file scope + // as a deferred def. That fires the eager watcher, so `local()`'s snapshot + // over-includes it, even though the routed def can't reach the already-run + // body. + let index = index( + "\ +x <- 1 +local({ + x +}) +rlang::on_load({ + x <- 2 +}) +", + ); + let file = ScopeId::from(0); + let local_scope = ScopeId::from(1); + + // File-scope defs: `x <- 1` (0) and the `on_load`-routed `x <- 2` (1). + let x_sym = index.uses(local_scope)[UseId::from(0)].symbol(); + let (enclosing_scope, bindings) = index.enclosing_bindings(local_scope, x_sym).unwrap(); + assert_eq!(enclosing_scope, file); + assert_eq!(bindings.definitions(), &[ + DefinitionId::from(0), + DefinitionId::from(1) + ]); +} + +// --- Resolver-driven recognition --- + +#[test] +fn test_nse_noop_resolver_bare_local_stays_flat() { + // Under Noop, `resolve_effects` returns `None`, so a bare `local` isn't + // recognized as NSE: no scope is pushed and `x` stays at file scope. + let index = build_with( + "\ +local({ + x <- 1 +}) +", + NoopImportsResolver, + ); + 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_noop_resolver_namespaced_local_pushes_scope() { + // `pkg::fn` resolves through the resolver's default `resolve_qualified_effects`, + // which reads the static registry. `::` names the package, so there's no + // shadowing and no cross-file context needed, hence `base::local` is + // recognized as NSE even under Noop. + let index = build_with( + "\ +base::local({ + x <- 1 +}) +", + NoopImportsResolver, + ); + let local_scope = ScopeId::from(1); + + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!( + index.symbols(local_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_front_gate_skips_resolver_for_unannotated_name() { + // A bare callee whose name no package annotates never reaches the resolver: + // the `is_annotated_name` gate short-circuits before consultation. + let resolver = TestImportsResolver::with_base(); + let consultations = resolver.consultations(); + + build_with("frobnicate({ x <- 1 })", resolver); + + assert_eq!(consultations.get(), 0); +} + +#[test] +fn test_nse_front_gate_consults_resolver_for_annotated_name() { + // An annotated bare callee does reach the resolver (contrast with the gate + // test above). + let resolver = TestImportsResolver::with_base(); + let consultations = resolver.consultations(); + + build_with("local({ x <- 1 })", resolver); + + assert!(consultations.get() > 0); +} diff --git a/crates/oak_semantic/tests/integration/main.rs b/crates/oak_semantic/tests/integration/main.rs index da7516d89d..0eca846fff 100644 --- a/crates/oak_semantic/tests/integration/main.rs +++ b/crates/oak_semantic/tests/integration/main.rs @@ -1,2 +1,4 @@ mod builder; +mod builder_nse; +mod resolvers; mod use_def_map; diff --git a/crates/oak_semantic/tests/integration/resolvers.rs b/crates/oak_semantic/tests/integration/resolvers.rs new file mode 100644 index 0000000000..44d2b7099c --- /dev/null +++ b/crates/oak_semantic/tests/integration/resolvers.rs @@ -0,0 +1,55 @@ +use std::cell::Cell; +use std::rc::Rc; + +use oak_semantic::effects_registry; +use oak_semantic::Effects; +use oak_semantic::ImportsResolver; +use oak_semantic::SourceResolution; + +/// Test resolver: an explicit search path resolved against the registry. +/// +/// Resolves a bare callee by walking `attached` (LIFO) then its own +/// always-attached packages, returning the first package's registry annotation +/// for the name. Base is a normal entry in the always-attached list, not a +/// special case. Flat: no re-export chase, that's the salsa resolver's job. +pub struct TestImportsResolver { + /// Packages always on the search path, base last. These stand in for the + /// non-flow layers (base, default search path) the salsa resolver derives. + always_attached: Vec, + /// Count of `resolve_effects` consultations, so tests can assert the front + /// gate keeps unannotated names off the resolver. + consultations: Rc>, +} + +impl TestImportsResolver { + /// Resolver with base always attached: the minimum for the bare base NSE + /// functions (`local`, `with`, `within`, `evalq`) to resolve. + pub fn with_base() -> Self { + Self { + always_attached: vec![String::from("base")], + consultations: Rc::new(Cell::new(0)), + } + } + + /// A handle to the consultation counter. Clone it before moving the + /// resolver into `build_index`, then read it after the build. + pub fn consultations(&self) -> Rc> { + Rc::clone(&self.consultations) + } +} + +impl ImportsResolver for TestImportsResolver { + fn resolve_source(&mut self, _path: &str) -> Option { + None + } + + fn resolve_effects(&mut self, name: &str, attached: &[String], _lazy: bool) -> Option { + self.consultations.set(self.consultations.get() + 1); + attached + .iter() + .rev() + .chain(self.always_attached.iter()) + .find_map(|pkg| effects_registry::lookup(pkg, name).copied()) + .map(Effects::nse) + } +} From 50dd33184024b64b8910caecc90fa65a6a88c7b5 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 9 Jul 2026 10:06:06 +0200 Subject: [PATCH 02/12] Use a two-pass approach --- crates/oak_semantic/src/builder.rs | 652 ++++++++++++------ .../oak_semantic/src/builder/builder_nse.rs | 329 ++++++--- crates/oak_semantic/src/semantic_index.rs | 21 + crates/oak_semantic/src/use_def_map.rs | 7 - .../tests/integration/builder_nse.rs | 471 ++++++++++++- .../tests/integration/resolvers.rs | 21 +- 6 files changed, 1136 insertions(+), 365 deletions(-) diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index 8a50158b9f..49402c01d6 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -6,6 +6,7 @@ use aether_syntax::AnyRParameterName; use aether_syntax::AnyRValue; use aether_syntax::RArgumentList; use aether_syntax::RBinaryExpression; +use aether_syntax::RCall; use aether_syntax::RExpressionList; use aether_syntax::RFunctionDefinition; use aether_syntax::RNamespaceExpression; @@ -29,7 +30,9 @@ use oak_index_vec::IndexVec; use rustc_hash::FxHashMap; use rustc_hash::FxHashSet; +use crate::effects::NseAnnotation; use crate::resolver::ImportsResolver; +use crate::resolver::SourceResolution; use crate::semantic_index::Definition; use crate::semantic_index::DefinitionId; use crate::semantic_index::DefinitionKind; @@ -44,6 +47,7 @@ use crate::semantic_index::ScopeId; use crate::semantic_index::ScopeKind; use crate::semantic_index::SemanticCall; use crate::semantic_index::SemanticCallKind; +use crate::semantic_index::SemanticDiagnostic; use crate::semantic_index::SemanticIndex; use crate::semantic_index::SymbolFlags; use crate::semantic_index::SymbolTableBuilder; @@ -57,67 +61,22 @@ mod builder_nse; /// information supplied by `resolver`. See [`ImportsResolver`] for the /// available impls. /// -/// NSE scopes (`local()`, `test_that()`, ...) require a two-phase build. -/// The first walk keeps everything flat and discovers which calls are NSE. -/// If none are found, that result is final. Otherwise we re-walk with known -/// nested NSE scope bodies. +/// Each scope is built in two local phases. First a scan pass over the +/// scope's direct level decides which calls are NSE, in flow order, and +/// collects the scope's bound names (see [`scan_expression`]). Then the walk +/// reuses those decisions and pushes NSE scopes inline as it reaches them +/// ([`collect_expression`]). Walking `local({...})` inline means a later call +/// sees the scope-push in the same pass, so there is no whole-file re-walk. +/// +/// [`scan_expression`]: SemanticIndexBuilder::scan_expression +/// [`collect_expression`]: SemanticIndexBuilder::collect_expression pub fn build_index(root: &RRoot, resolver: impl ImportsResolver) -> SemanticIndex { let range = root.syntax().text_trimmed_range(); - // First walk: discover which calls are NSE, if any. let mut builder = SemanticIndexBuilder::new(range, resolver); - builder.pre_scan_scope(root.syntax()); + builder.begin_scan(); + builder.scan_expression_list(&root.expressions()); builder.collect_expression_list(&root.expressions()); - - if !builder.found_nse { - return builder.finish(); - } - - // Re-walk until the set of NSE scope bodies stabilizes. One re-walk is - // typically enough to reach convergence. More walks are needed only when - // pushing an NSE scope unmasks a callee. For instance in: - // - // ``` - // local({ with <- identity }); - // with(df, y) - // ``` - // - // The call to `with()` is recognized only once a re-walk has moved the - // `with` assignment into the `local()` scope. Each such level costs one - // extra walk. Convergence relies on decisions never flipping back from NSE - // to not-NSE, see `is_locally_bound()`. - // - // The loop terminates on its own because the set can only grow. Each - // re-walk is seeded with the previous set and only inserts. The cap only - // guards against pathological files. - // - // An important caveat is that each walk re-analyzes the whole file, so our - // passes never get cheaper, which is fine since rewalks should be rare. - // That's the opposite of Rust-Analyzer's fixpoint, where each pass touches - // only the shrinking unresolved frontier, which is why RA can afford a much - // larger cap of 8192 passes: - // https://github.com/rust-lang/rust-analyzer/blob/abb1301c/crates/hir-def/src/nameres/collector.rs#L61 - const MAX_NSE_ITERATIONS: usize = 64; - for i in 0..MAX_NSE_ITERATIONS { - let prev_ranges = std::mem::take(&mut builder.nse_nested_ranges); - let resolver = builder.resolver; - builder = SemanticIndexBuilder::new_rewalk(range, prev_ranges.clone(), resolver); - builder.pre_scan_scope(root.syntax()); - builder.collect_expression_list(&root.expressions()); - - if builder.nse_nested_ranges == prev_ranges { - if i >= 5 { - log::trace!("NSE re-walk converged after {i} iterations in range {range:?}"); - } - return builder.finish(); - } - } - - // Hitting the cap means the returned index is inconsistent, not merely - // degraded, and valid R should never reach it. `error!` matches that. - log::error!( - "NSE re-walk did not converge after {MAX_NSE_ITERATIONS} iterations in range {range:?}" - ); builder.finish() } @@ -131,48 +90,43 @@ struct SemanticIndexBuilder { uses: IndexVec>, use_def_maps: IndexVec, current_scope: ScopeId, - pre_scans: IndexVec, + bound_names: IndexVec, enclosing_snapshots: FxHashMap, semantic_calls: Vec, namespace_accesses: Vec, - // The `Nested` NSE scope bodies found so far, as a set of ranges. This is - // the re-walk loop's fixpoint state. Each walk seeds it from the previous - // iteration, grows it as `record_nse_arg_decision()` recognizes more scopes, - // and stops once a walk no longer finds any NSE range. - nse_nested_ranges: FxHashSet, - // `true` once any scope-pushing NSE combo is found. Triggers the re-walk. - found_nse: bool, - // Whether to push NSE scopes at call sites. Only the re-walk does. On the - // first walk the pre-scan hasn't learned which bodies to skip, so it still - // records a nested body's definitions (e.g. `x` from `local({x <- 1})`) - // into the parent scope. Pushing the child scope on that walk too would - // then register `x`'s enclosing snapshot against the parent, one scope too - // high. - is_rewalk: bool, + // The NSE effect each call resolved to, keyed by the call's range. Filled + // in flow order by the scan. Absence means "not NSE". + // + // File-global on purpose, not per-scope: a `TextRange` is unique across the + // file, so entries from different scopes can't collide. + nse_annotations: FxHashMap, + // Resolved `source()` calls, keyed by the call's range. The scan fills + // this once per call (consulting `resolve_source`), the walk reads it back, + // so the resolver is queried exactly once per `source()` call site. + source_resolutions: FxHashMap, + // Names bound so far in the scope currently being scanned, tracked + // flow-precisely (if/else restore, loop union). This is the scan + // pass's own flow state, standing in for the walk's use-def state which + // isn't built yet. Reset at each scope's `begin_scan()`. + bound_so_far: FxHashSet, + // The enclosing eager environment captured when each child scope is + // entered, keyed by the child's range. Recorded from `bound_so_far` when + // the parent's scan reaches the child's definition point. + eager_bindings: FxHashMap>, + // Diagnostics collected during the build and logged on `finish()`. A minimal + // channel for now, no user-facing surface. + diagnostics: Vec, resolver: R, } impl SemanticIndexBuilder { fn new(range: TextRange, resolver: R) -> Self { - Self::new_impl(range, FxHashSet::default(), false, resolver) - } - - fn new_rewalk(range: TextRange, nse_nested_ranges: FxHashSet, resolver: R) -> Self { - Self::new_impl(range, nse_nested_ranges, true, resolver) - } - - fn new_impl( - range: TextRange, - nse_nested_ranges: FxHashSet, - is_rewalk: bool, - resolver: R, - ) -> Self { let mut scopes = IndexVec::new(); let mut symbol_tables = IndexVec::new(); let mut definitions = IndexVec::new(); let mut uses = IndexVec::new(); let mut use_def_maps = IndexVec::new(); - let mut pre_scans = IndexVec::new(); + let mut bound_names = IndexVec::new(); // The descendants range starts empty (`n+1..n+1`). `pop_scope` later // fills in `descendants.end` with the current arena length. Everything @@ -191,7 +145,7 @@ impl SemanticIndexBuilder { definitions.push(IndexVec::new()); uses.push(IndexVec::new()); use_def_maps.push(UseDefMapBuilder::new()); - pre_scans.push(PreScanScope::new()); + bound_names.push(BoundNames::new()); Self { scopes, @@ -200,13 +154,15 @@ impl SemanticIndexBuilder { uses, use_def_maps, current_scope: file_scope, - pre_scans, + bound_names, enclosing_snapshots: FxHashMap::default(), semantic_calls: Vec::new(), namespace_accesses: Vec::new(), - nse_nested_ranges, - found_nse: false, - is_rewalk, + nse_annotations: FxHashMap::default(), + source_resolutions: FxHashMap::default(), + bound_so_far: FxHashSet::default(), + eager_bindings: FxHashMap::default(), + diagnostics: Vec::new(), resolver, } } @@ -231,7 +187,7 @@ impl SemanticIndexBuilder { self.definitions.push(IndexVec::new()); self.uses.push(IndexVec::new()); self.use_def_maps.push(UseDefMapBuilder::new()); - self.pre_scans.push(PreScanScope::new()); + self.bound_names.push(BoundNames::new()); id } @@ -481,26 +437,9 @@ impl SemanticIndexBuilder { } } - /// Whether `scope` binds `name` in its flow state so far: some definition - /// reaches this point on the control-flow paths up to here. A name never - /// interned in `scope` has nothing binding it, so it counts as unbound. - /// - /// Used for eager scopes, see - /// [`scope_binds_anywhere`](Self::scope_binds_anywhere) for the - /// flow-insensitive variant for lazy scopes. - fn scope_binds_so_far(&self, scope: ScopeId, name: &str) -> bool { - match self.symbol_tables[scope].id(name) { - Some(symbol_id) => !self.use_def_maps[scope].is_unbound(symbol_id), - None => false, - } - } - /// Whether `scope` binds `name` anywhere, regardless of flow position: an /// already-recorded `IS_BOUND` definition or a pre-scanned assignment. The /// pre-scan covers definitions the walk hasn't reached yet in this scope. - /// - /// Used for lazy scopes, see `scope_binds_so_far` for the flow-sensitive - /// variant for eager scopes. fn scope_binds_anywhere(&self, scope: ScopeId, name: &str) -> bool { let found_by_flag = self.symbol_tables[scope].id(name).is_some_and(|sym_id| { self.symbol_tables[scope] @@ -508,7 +447,300 @@ impl SemanticIndexBuilder { .flags() .contains(SymbolFlags::IS_BOUND) }); - found_by_flag || self.pre_scans[scope].has_name(name) + found_by_flag || self.bound_names[scope].binds(name) + } + + /// Record the eager environment for a child scope (function body, NSE + /// argument) about to be created at `range`, 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). + pub(super) fn record_eager_bindings(&mut self, range: TextRange) { + self.eager_bindings.insert(range, self.bound_so_far.clone()); + } + + // --- Scan pass --- + + /// Reset the flow-precise binding state for a fresh scope's scan. + /// + /// Seeds it with two things: + /// + /// - The enclosing eager environment captured when this scope was entered + /// (`eager_bindings`). The parent's own scan was seeded the same way, + /// so this is transitively complete: it holds every eager binding + /// visible from an ancestor at this scope's definition point. + /// - The scope's own already-bound names. For a function scope that's the + /// parameters, recorded just before the scan runs. For file and NSE scopes + /// nothing local is bound yet. + /// + /// Parameter defaults are a special case: they are scanned before the params + /// are recorded, so `collect_function` seeds the full formal set by hand + /// (all formals bind at once in R, so a default sees every parameter name). + pub(super) fn begin_scan(&mut self) { + self.bound_so_far.clear(); + + let range = self.scopes[self.current_scope].range; + if let Some(entry) = self.eager_bindings.get(&range) { + self.bound_so_far.extend(entry.iter().cloned()); + } + + for (_id, symbol) in self.symbol_tables[self.current_scope].iter() { + if symbol.flags().contains(SymbolFlags::IS_BOUND) { + self.bound_so_far.insert(symbol.name().to_string()); + } + } + } + + pub(super) fn scan_expression_list(&mut self, list: &RExpressionList) { + for expr in list.iter() { + self.scan_expression(&expr); + } + } + + /// Scan for NSE calls and collect the scope's bound names, in flow order. + /// + /// Runs before the walk of a scope. It decides NSE-ness at each call the + /// same way the walk's [`is_locally_bound`](Self::is_locally_bound) would, + /// records the decision in `nse_annotations` for the walk to reuse, and adds + /// non-skipped definition names to `bound_names`. The bound names must be + /// complete before the walk descends into any child scope, because a lazy + /// child body can reference an ancestor def the ancestor's walk hasn't + /// reached yet. + /// + /// The scan matches the walk's scope boundaries: + /// + /// - Function and `Nested` NSE bodies are child scopes, scanned + /// separately in their own context. + /// - A `Current + Lazy` body is also a child scope, scanned separately for + /// the same reason: NSE resolution needs the child's own flow context, + /// which differs from this scope's. + /// - A `Current + Eager` body pushes no scope, so it stays part of this + /// scope's direct level and is scanned through transparently. + /// + /// Branch analysis is precise. In `if (c) local <- f else local({ y <- 1 + /// })` the else branch sees an NSE call because `local` is unbound on the + /// else path, which prevents `y` from leaking into the scope. + pub(super) fn scan_expression(&mut self, expr: &AnyRExpression) { + match expr { + AnyRExpression::RFunctionDefinition(func) => { + // A function body is a child scope, scanned when it's entered. + // Record this scope's eager bindings now so that when we later + // resolve an NSE callee inside the body, we can check whether one + // of them shadows it (see `bound_at_entry`). + self.record_eager_bindings(func.syntax().text_trimmed_range()); + }, + + AnyRExpression::RBracedExpressions(braced) => { + self.scan_expression_list(&braced.expressions()); + }, + + AnyRExpression::RBinaryExpression(bin) => { + if is_assignment(bin) { + let right = is_right_assignment(bin); + + // Value side first, mirroring `collect_assignment`: it may + // hold NSE calls or nested defs that flow before the binding. + let value = if right { bin.left() } else { bin.right() }; + if let Ok(value) = value { + self.scan_expression(&value); + } + + let target = if right { bin.right() } else { bin.left() }; + if let Ok(target) = target { + match assignment_name(&target) { + // `<<-` binds in an ancestor, not here, so it doesn't + // shadow a callee in this scope (matching the walk). + Some((name, _)) if !is_super_assignment(bin) => { + self.record_binding(name); + }, + Some(_) => {}, + // Complex target (`x$foo <- v`): no binding, but the + // target may hold NSE calls. + None => self.scan_expression(&target), + } + } + } else { + if let Ok(lhs) = bin.left() { + self.scan_expression(&lhs); + } + if let Ok(rhs) = bin.right() { + self.scan_expression(&rhs); + } + } + }, + + AnyRExpression::RCall(call) => { + if let Ok(func) = call.function() { + self.scan_expression(&func); + } + self.scan_call(call); + self.scan_semantic_call(call); + }, + + AnyRExpression::RForStatement(stmt) => { + // The for-variable is always bound (R sets it to NULL for empty + // sequences), so it binds before the body regardless of flow. + if let Ok(variable) = stmt.variable() { + self.record_binding(variable.name_text()); + } + 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); + } + }, + + AnyRExpression::RIfStatement(stmt) => { + if let Ok(condition) = stmt.condition() { + self.scan_expression(&condition); + } + + let pre_if = self.bound_so_far.clone(); + + if let Ok(consequence) = stmt.consequence() { + self.scan_expression(&consequence); + } + + let post_if = std::mem::replace(&mut self.bound_so_far, pre_if); + + if let Some(else_clause) = stmt.else_clause() { + if let Ok(alternative) = else_clause.alternative() { + self.scan_expression(&alternative); + } + } + + // Both branches' bindings are live afterwards. + self.bound_so_far.extend(post_if); + }, + + // `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. + _ => { + self.scan_descendants(expr.syntax()); + }, + } + } + + /// Walk descendant nodes of `expr`, scanning the outermost + /// `AnyRExpression` children. The scan analog of + /// `collect_descendants`. + fn scan_descendants(&mut self, node: &RSyntaxNode) { + let mut preorder = node.preorder(); + preorder.next(); + + while let Some(event) = preorder.next() { + let WalkEvent::Enter(node) = event else { + continue; + }; + if let Some(expr) = node.cast::() { + self.scan_expression(&expr); + preorder.skip_subtree(); + } + } + } + + fn scan_parameter_defaults(&mut self, params: &RParameters) { + // Seed `bound_so_far` with every parameter names so a callee inside a + // default value sees the full formal set + for param in params.items().iter() { + let Ok(param) = param else { continue }; + let Ok(name) = param.name() else { continue }; + let text = match &name { + AnyRParameterName::RIdentifier(ident) => ident.name_text(), + AnyRParameterName::RDots(_) => String::from("..."), + AnyRParameterName::RDotDotI(ddi) => ddi.syntax().text_trimmed().to_string(), + }; + self.bound_so_far.insert(text); + } + + for param in params.items().iter() { + let Ok(param) = param else { continue }; + let Some(default) = param.default() else { + continue; + }; + if let Ok(value) = default.value() { + self.scan_expression(&value); + } + } + } + + /// Scan-time analog of [`collect_semantic_call`]. + /// + /// Only `source()` needs handling here. Its injected bindings shadow NSE + /// callees, and the walk injects them too late for a later call in the same + /// scope to see. `library()`/`require()` attaches don't affect the scan + /// decisions yet, so they stay with the walk. + /// + /// [`collect_semantic_call`]: Self::collect_semantic_call + fn scan_semantic_call(&mut self, call: &aether_syntax::RCall) { + let Ok(AnyRExpression::RIdentifier(ident)) = call.function() else { + return; + }; + if ident.name_text() == "source" { + self.scan_source_call(call); + } + } + + /// Resolve a `source()` call once, cache it, and bind the sourced names. + /// + /// The binding is eager: `source()` runs at its position, so the sourced + /// names ARE bound afterwards and can shadow a later NSE callee (e.g. a + /// sourced `local` masking base `local`). The resolution is cached by call + /// range so the walk reuses it instead of consulting the resolver again. + fn scan_source_call(&mut self, call: &aether_syntax::RCall) { + let Some(path) = self.parse_source_path(call) else { + return; + }; + let Some(resolution) = self.resolver.resolve_source(&path) else { + return; + }; + + for name in &resolution.names { + self.record_binding(name.clone()); + } + + self.source_resolutions + .insert(call.syntax().text_trimmed_range(), resolution); + } + + /// Record a binding in the scan's flow state. + /// + /// The flow-precise `bound_so_far` set always learns the name, so a + /// later callee in this scope sees it shadowed. The bound names only get it + /// when the current scope owns it. A `Current + Lazy` scope routes its defs + /// to the owner, so the name is added to the owner's bound names instead, the + /// same routing `add_definition_to_owner` does during the walk. + fn record_binding(&mut self, name: String) { + self.record_owner_name(name.clone()); + self.bound_so_far.insert(name); + } + + /// Route a binding NAME into its owner scope's bound names, matching + /// `add_definition`'s routing. A `Current + Lazy` scope routes to + /// `definition_owner()`, every other scope owns its bindings. + /// + /// Split from `record_binding` so `scan_lazy_owner_bindings` can add + /// a deferred body's names to the owner's bound names without also marking them + /// bound in `bound_so_far` (see that helper for why). + fn record_owner_name(&mut self, name: String) { + if let Some(target) = match self.scopes[self.current_scope].kind { + ScopeKind::Nse(NseScope::Current, NseTiming::Lazy) => self.definition_owner(), + _ => Some(self.current_scope), + } { + self.bound_names[target].add(name); + } + } + + fn nse_effect(&self, call: &RCall) -> Option { + self.nse_annotations + .get(&call.syntax().text_trimmed_range()) + .copied() } // --- Recursive descent --- @@ -566,14 +798,12 @@ impl SemanticIndexBuilder { // as uses. AnyRExpression::RCall(call) => { // Record the callee as a use (a no-op for `pkg::fn`) before - // resolving NSE. That interns the callee symbol, so - // `resolve_nse()` can look it up by name and read whether it's - // bound at this point. + // handling NSE. if let Ok(func) = call.function() { self.collect_expression(&func); } - if let Some(annotation) = self.resolve_nse(call) { + if let Some(annotation) = self.nse_effect(call) { self.collect_nse_call(call, annotation) } else if let Ok(args) = call.arguments() { self.collect_arguments(&args.items()); @@ -753,72 +983,28 @@ impl SemanticIndexBuilder { let scope = self.push_scope(ScopeKind::Function, fun.syntax().text_trimmed_range()); 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 + // name regardless of position: `function(local, b = local(...))` is + // not NSE. So we seed the whole formal set into `bound_so_far` + // up front rather than flow-ordered, then scan each default. + self.begin_scan(); + self.scan_parameter_defaults(¶ms); + + // `collect_parameters` adds the parameter definitions and walks + // each default in source order, finding the NSE decisions the scan + // above recorded. self.collect_parameters(¶ms); } if let Ok(body) = fun.body() { - self.pre_scan_scope(body.syntax()); + self.begin_scan(); + self.scan_expression(&body); self.collect_expression(&body); } self.pop_scope(scope); } - /// Pre-scan a scope to collect all definition names (skipping nested - /// function bodies). Runs before the full walk so that enclosing - /// snapshot registration can find where free variables are bound, - /// even when the walk in the parent scope hasn't reached the - /// definition yet. Must stay in sync with the full walk's definition - /// handling: any construct that calls `add_definition` should have a - /// corresponding entry here. - fn pre_scan_scope(&mut self, root: &RSyntaxNode) { - let mut preorder = root.preorder(); - while let Some(event) = preorder.next() { - let WalkEvent::Enter(node) = event else { - continue; - }; - let is_root = &node == root; - let Some(expr) = AnyRExpression::cast(node) else { - continue; - }; - - // On the re-walk, skip nested NSE scope bodies, just like we skip - // function bodies: their definitions belong to the child scope, not - // the scope being pre-scanned. The root is the scope being - // pre-scanned, which may itself be an NSE body, so it's never - // skipped. `nse_nested_ranges` is empty on the first walk. - if !is_root && - self.nse_nested_ranges - .contains(&expr.syntax().text_trimmed_range()) - { - preorder.skip_subtree(); - continue; - } - - match &expr { - AnyRExpression::RFunctionDefinition(_) => { - preorder.skip_subtree(); - }, - AnyRExpression::RBinaryExpression(bin) - if is_assignment(bin) && !is_super_assignment(bin) => - { - let right = is_right_assignment(bin); - let target = if right { bin.right() } else { bin.left() }; - if let Ok(target) = target { - if let Some((name, _)) = assignment_name(&target) { - self.pre_scans[self.current_scope].add(name); - } - } - }, - AnyRExpression::RForStatement(stmt) => { - if let Ok(variable) = stmt.variable() { - self.pre_scans[self.current_scope].add(variable.name_text()); - } - }, - _ => {}, - } - } - } - fn collect_parameters(&mut self, params: &RParameters) { for param in params.items().iter() { let Ok(param) = param else { continue }; @@ -1014,52 +1200,18 @@ impl SemanticIndexBuilder { // regardless to keep the sourcing mechanism simple. A future diagnostic // should suggest `local = TRUE` in nested contexts. fn collect_source_call(&mut self, call: &aether_syntax::RCall) { - let Ok(args) = call.arguments() else { + let Some(path) = self.parse_source_path(call) else { return; }; - let mut path: Option = None; - let mut bail = false; - - for item in args.items().iter() { - let Ok(arg) = item else { continue }; - - if let Some(name_clause) = arg.name_clause() { - let Ok(AnyRArgumentName::RIdentifier(name_ident)) = name_clause.name() else { - continue; - }; - if name_ident.name_text() == "local" { - if let Some(value) = arg.value() { - match value { - // TRUE/FALSE are fine, we resolve uniformly. For - // the FALSE in nested context case, we'll emit a - // diagnostic. - AnyRExpression::RTrueExpression(_) | - AnyRExpression::RFalseExpression(_) => {}, - // With anything else (environment, non-statically - // resolvable expression) is not we need to bail. - _ => bail = true, - } - } - } - } else if path.is_none() { - // First positional argument: the file path - if let Some(AnyRExpression::AnyRValue(AnyRValue::RStringValue(s))) = arg.value() { - path = s.string_text(); - } - } - } - - if bail { - return; - } - - let Some(path) = path else { - return; - }; + let range = call.syntax().text_trimmed_range(); + let call_offset = range.start(); - let call_offset = call.syntax().text_trimmed_range().start(); - let resolution = self.resolver.resolve_source(&path); + // Read the resolution the scan already computed. The scan is the + // single point that consults `resolve_source`, so the walk never + // re-resolves. A cache miss means the scan bailed or the resolver + // returned `None`, both of which record the call with `resolved: None`. + let resolution = self.source_resolutions.get(&range).cloned(); // Record every `source()` call site, independent of whether the // resolution was successful. `resolved` pins the canonical URL when @@ -1113,9 +1265,62 @@ impl SemanticIndexBuilder { } } + /// Parse the file path out of a `source("path")` call. + /// + /// Shared by the scan and the walk so they agree on which calls are + /// statically analyzable. Returns `None` when there's no positional path, + /// or when `local =` is set to something other than TRUE/FALSE (an + /// environment or a non-literal expression we can't follow). + fn parse_source_path(&self, call: &aether_syntax::RCall) -> Option { + let args = call.arguments().ok()?; + + let mut path: Option = None; + + for item in args.items().iter() { + let Ok(arg) = item else { continue }; + + if let Some(name_clause) = arg.name_clause() { + let Ok(AnyRArgumentName::RIdentifier(name_ident)) = name_clause.name() else { + continue; + }; + if name_ident.name_text() == "local" { + if let Some(value) = arg.value() { + match value { + // TRUE/FALSE are fine, we resolve uniformly. For + // the FALSE in nested context case, we'll emit a + // diagnostic. + AnyRExpression::RTrueExpression(_) | + AnyRExpression::RFalseExpression(_) => {}, + // Anything else (environment, non-statically + // resolvable expression) means we bail. + _ => return None, + } + } + } + } else if path.is_none() { + // First positional argument: the file path + if let Some(AnyRExpression::AnyRValue(AnyRValue::RStringValue(s))) = arg.value() { + path = s.string_text(); + } + } + } + + path + } + fn finish(mut self) -> SemanticIndex { self.scopes[ScopeId::from(0)].descendants.end = self.scopes.next_id(); + // TODO(diagnostics): Diagnostics are not surfaced yet, so log them for now + for diagnostic in &self.diagnostics { + match diagnostic { + SemanticDiagnostic::LazyShadowAmbiguity { name, range } => log::warn!( + "NSE lazy-shadow ambiguity: callee `{name}` at {range:?} is recognized \ + as NSE, but a lazy-crossed ancestor binds it with undetermined timing" + ), + } + } + let symbol_tables = self .symbol_tables .into_iter() @@ -1142,30 +1347,19 @@ impl SemanticIndexBuilder { self.enclosing_snapshots, self.semantic_calls, self.namespace_accesses, + self.diagnostics, file_final_bindings, ) } } -/// All definitions in a scope, collected before the full walk. Skips nested -/// function bodies (those belong to child scopes). Two consumers: -/// -/// - Enclosing snapshots: `has_name()` checks whether a symbol will be -/// defined in an ancestor scope (even when the ancestor's walk hasn't reached -/// that definition yet), so that `register_enclosing_snapshot()` can find the -/// right ancestor for free variables. -/// - NSE resolution: With NSE, each function call potentially pushes a scope -/// (which can be lazy or eager). We need to resolve the called function's -/// semantic during the walk. Inside lazy scopes (e.g. function bodies), -/// `by_name` provides the complete set of parent definitions so that the -/// function can be resolved against all the parent scope's definitions (if NSE -/// semantics don't match across definitions, we pick one and lint). Intra-scope -/// resolution is linear and uses the current `symbol_states` directly instead. -struct PreScanScope { +/// All definitions in a scope, collected by the scan pass before the +/// walk. Skips child-scope bodies (nested functions and `Nested` NSE bodies). +struct BoundNames { by_name: FxHashSet, } -impl PreScanScope { +impl BoundNames { fn new() -> Self { Self { by_name: FxHashSet::default(), @@ -1176,7 +1370,7 @@ impl PreScanScope { self.by_name.insert(name); } - fn has_name(&self, name: &str) -> bool { + fn binds(&self, name: &str) -> bool { self.by_name.contains(name) } } diff --git a/crates/oak_semantic/src/builder/builder_nse.rs b/crates/oak_semantic/src/builder/builder_nse.rs index 3b887d6ccd..0f2e08e47c 100644 --- a/crates/oak_semantic/src/builder/builder_nse.rs +++ b/crates/oak_semantic/src/builder/builder_nse.rs @@ -3,12 +3,17 @@ use aether_syntax::AnyRExpression; use aether_syntax::RArgumentList; use aether_syntax::RCall; use biome_rowan::AstNode; +use biome_rowan::AstNodeList; use biome_rowan::AstSeparatedList; +use biome_rowan::TextRange; use oak_core::syntax_ext::AnyRSelectorExt; use oak_core::syntax_ext::RIdentifierExt; use oak_core::syntax_ext::RStringValueExt; -use stdext::debug_panic; +use super::assignment_name; +use super::is_assignment; +use super::is_right_assignment; +use super::is_super_assignment; use super::SemanticIndexBuilder; use crate::effects::Effects; use crate::effects::NseAnnotation; @@ -18,21 +23,179 @@ use crate::resolver::ImportsResolver; use crate::semantic_index::NseScope; use crate::semantic_index::NseTiming; use crate::semantic_index::ScopeKind; -use crate::semantic_index::SymbolId; +use crate::semantic_index::SemanticDiagnostic; impl SemanticIndexBuilder { + /// Scan a call for effects (e.g. NSE scopes) and record its decision for + /// the walk to reuse. + /// + /// If the callee resolves to an NSE annotation, the annotation is stored in + /// `nse_annotations` keyed by the call's range. Arguments evaluated in nested + /// calls are scanned accordingly. Otherwise all arguments are scanned in + /// the current scope. + /// + /// We only fully scan `Current + Eager` arguments here. The child scopes + /// created by `Nested` and `Current + Lazy` bodies are scanned by the later + /// walk because callee resolution needs the child's own flow context. + pub(super) fn scan_call(&mut self, call: &RCall) { + let Some(annotation) = self.resolve_nse(call) else { + if let Ok(args) = call.arguments() { + for item in args.items().iter() { + let Ok(arg) = item else { continue }; + if let Some(value) = arg.value() { + self.scan_expression(&value); + } + } + } + return; + }; + + self.nse_annotations + .insert(call.syntax().text_trimmed_range(), annotation); + + let Ok(args) = call.arguments() else { + return; + }; + let items = args.items(); + let nse_args = self.match_nse_arguments(&items, annotation); + + for (i, item) in items.iter().enumerate() { + let Ok(arg) = item else { continue }; + let Some(value) = arg.value() else { continue }; + + match nse_args[i] { + None => self.scan_expression(&value), + Some(nse_arg) => match (nse_arg.scope, nse_arg.timing) { + (NseScope::Current, NseTiming::Eager) => self.scan_expression(&value), + // e.g. `on_load({ ... })`. Its body runs later, so its defs + // land in the enclosing scope. We don't resolve the body's + // calls here. The walk does that once it enters the child + // scope. But we do grab the names it defines now, so the + // owner's bound names are complete before the walk reaches a sibling. + (NseScope::Current, NseTiming::Lazy) => { + self.record_eager_bindings(value.syntax().text_trimmed_range()); + self.scan_lazy_owner_bindings(&value); + }, + // A `Nested` body is a child scope scanned when it's entered. + // Capture this scope's eager bindings for its callee + // resolution, same as a function body. + (NseScope::Nested, _) => { + self.record_eager_bindings(value.syntax().text_trimmed_range()); + }, + }, + } + } + } + + /// Copy the names a `Current + Lazy` body defines into the owner's + /// bound names, without marking them bound in the scan's flow state. + /// + /// This feeds enclosing snapshots only. A free variable elsewhere can + /// resolve to a name an `on_load({ ... })` defines in the owner, and + /// `register_enclosing_snapshot()` reads `bound_names` to find that ancestor. + /// The scan doesn't descend into these bodies otherwise, so their names + /// would only reach the owner when the walk later gets to the call, too late + /// for a sibling scanned before then. Collecting them now keeps the owner's + /// bound names complete before the walk touches any sibling. + /// + /// NSE shadow resolution does not read `bound_names`, so an incomplete + /// collection here can't flip an NSE decision. `is_locally_bound` reads the + /// captured eager bindings, which exclude deferred names by construction. + /// + /// We cover the realistic shapes, direct assignments and control flow, e.g. + /// `on_load({ x <- 1 })`. We stop at nested calls and function bodies + /// however, so we only add names the walk will also route, never a phantom. + /// `register_enclosing_snapshot` reads `binds()` as "a real definition + /// exists", so a phantom would send it chasing a binding that isn't there. + /// The price is a binding buried in a nested transparent call, e.g. + /// `on_load({ evalq(helper <- ...) })`, which we miss here, so a free + /// variable can't resolve to it. TODO(nse): We could potentially walk + /// transparent (Current) nested calls to collect those too. + /// + /// The names go to `bound_names` only, never to `bound_so_far`. The body + /// runs at some later time, so at an eager position after the call these + /// names aren't bound yet, and an eager callee there must still treat them + /// as unbound. + fn scan_lazy_owner_bindings(&mut self, expr: &AnyRExpression) { + match expr { + AnyRExpression::RBracedExpressions(braced) => { + for expr in braced.expressions().iter() { + self.scan_lazy_owner_bindings(&expr); + } + }, + + AnyRExpression::RBinaryExpression(bin) => { + // `<<-` binds in an ancestor, not the owner, so it's not routed + // here (matching `add_definition`). + if !is_assignment(bin) || is_super_assignment(bin) { + return; + } + let target = if is_right_assignment(bin) { + bin.right() + } else { + bin.left() + }; + if let Ok(target) = target { + if let Some((name, _)) = assignment_name(&target) { + self.record_owner_name(name); + } + } + }, + + AnyRExpression::RIfStatement(stmt) => { + if let Ok(consequence) = stmt.consequence() { + self.scan_lazy_owner_bindings(&consequence); + } + if let Some(else_clause) = stmt.else_clause() { + if let Ok(alternative) = else_clause.alternative() { + self.scan_lazy_owner_bindings(&alternative); + } + } + }, + + AnyRExpression::RForStatement(stmt) => { + if let Ok(variable) = stmt.variable() { + self.record_owner_name(variable.name_text()); + } + if let Ok(body) = stmt.body() { + self.scan_lazy_owner_bindings(&body); + } + }, + + AnyRExpression::RWhileStatement(stmt) => { + if let Ok(body) = stmt.body() { + self.scan_lazy_owner_bindings(&body); + } + }, + + AnyRExpression::RRepeatStatement(stmt) => { + if let Ok(body) = stmt.body() { + self.scan_lazy_owner_bindings(&body); + } + }, + + // Stop everywhere else: function bodies are child scopes, and a + // call's arguments aren't part of this scope's direct level. + _ => {}, + } + } + /// Resolve a call's callee to an NSE annotation. /// /// Two cases resolve here: - /// - A bare identifier. If the callee is unbound, it is resolved - /// through the cross-file `ImportsResolver::resolve_effects()` method. - /// If bound locally, we'll resolve the annotations here - TODO(nse, annotations). + /// - 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. /// - 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 /// per-package knowledge (the static registry, plus cross-file knowledge /// like the re-export chase once that lands). - pub(super) fn resolve_nse(&mut self, call: &RCall) -> Option { + /// + /// The bound check reads the scan pass's flow-precise binding state + /// for the current scope, so this must run during the scan, not the walk. + fn resolve_nse(&mut self, call: &RCall) -> Option { let func = call.function().ok()?; match &func { @@ -45,25 +208,35 @@ impl SemanticIndexBuilder { return None; } - let Some(symbol_id) = self.symbol_tables[self.current_scope].id(&name) else { - debug_panic!( - "Callee `{name}` not interned: collect_expression should have run first" - ); - return None; - }; - - // First check for a local definition (which in the future will - // potentially contain NSE annotations) - if self.is_locally_bound(&name) { + // First check for a local definition (which in the future may + // contain NSE annotations 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. + if self.bound_so_far.contains(&name) { return self - .resolve_effects(symbol_id) + .resolve_local_effects(&name) .and_then(|effects| effects.nse); } // Now check imports since the symbol is locally unbound - self.resolver + let nse = self + .resolver .resolve_effects(&name, &[], false) - .and_then(|effects| effects.nse) + .and_then(|effects| effects.nse)?; + + // The callee is unbound by any eager binding, so it is NSE. + // 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. + if self.is_lazily_shadowed(&name) { + self.record_lazy_shadow_ambiguity(name, call.syntax().text_trimmed_range()); + } + Some(nse) }, AnyRExpression::RNamespaceExpression(ns_expr) => { @@ -85,87 +258,61 @@ impl SemanticIndexBuilder { } } - /// Local resolver for declared effects, mirroring the imports revoler's + /// 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. - fn resolve_effects(&self, _symbol_id: SymbolId) -> Option { + fn resolve_local_effects(&self, _name: &str) -> Option { None } - /// Whether the current scope or an enclosing one binds `name`, shadowing - /// the base NSE callee. The current scope is always flow-precise. For - /// ancestors, crossing a lazy scope (e.g. a function body) loses the - /// accuracy because we don't know when the lazy scope runs and need to - /// consider the whole scope bindings, not just the ones currently live. + /// Detect ambiguities caused by laziness. /// - /// Invariant: The eager/lazy decision must match the decision in - /// `register_enclosing_snapshot()`. If they disagree, a call flips between - /// NSE and not-NSE across re-walks and the fixpoint never settles. - fn is_locally_bound(&self, name: &str) -> bool { - if self.scope_binds_so_far(self.current_scope, name) { - return true; - } - - let Some(mut scope) = self.scopes[self.current_scope].parent else { - return false; - }; - let mut all_eager = !self.scopes[self.current_scope].kind.is_lazy(); - - loop { - let bound = if all_eager { - self.scope_binds_so_far(scope, name) - } else { - self.scope_binds_anywhere(scope, name) - }; - if bound { + /// We've decided `name` is NSE because it was locally unbound at the + /// current flow cursor, and eager-flow resolution found an NSE effect. If + /// we're in a lazy context, that decision could be wrong: an enclosing + /// scope may bind `name` with a timing we can't pin down, either a later + /// assignment, or one from another deferred body that could run before or + /// after us We detect this ambiguity here so it can be linted. + fn is_lazily_shadowed(&self, name: &str) -> bool { + let mut scope = self.current_scope; + let mut crossed_lazy = self.scopes[scope].kind.is_lazy(); + + while let Some(parent) = self.scopes[scope].parent { + if crossed_lazy && self.scope_binds_anywhere(parent, name) { return true; } - if self.scopes[scope].kind.is_lazy() { - all_eager = false; + if self.scopes[parent].kind.is_lazy() { + crossed_lazy = true; } - - let Some(parent) = self.scopes[scope].parent else { - return false; - }; scope = parent; } + + false } - /// Process a call already recognized as NSE. Match its arguments against the - /// annotation, then handle each scoped argument. - /// - /// For every scoped argument we record the decision. That sets `found_nse`. - /// For `Nested` arguments it also notes the body range which allows - /// pre-scans to skip it. - /// - /// How we walk the body depends on the phase. The first walk keeps it flat - /// (no nested scope), so its definitions land in the current scope. The - /// re-walk pushes the NSE scope and walks the body inside it. + fn record_lazy_shadow_ambiguity(&mut self, name: String, range: TextRange) { + self.diagnostics + .push(SemanticDiagnostic::LazyShadowAmbiguity { name, range }); + } + + /// Process a call the scan pass decided is NSE. Match its arguments + /// against the annotation, then handle each scoped argument, pushing NSE + /// scopes inline. pub(super) fn collect_nse_call(&mut self, call: &RCall, annotation: NseAnnotation) { let Ok(args) = call.arguments() else { return; }; let items = args.items(); - let nse_args = self.match_nse_args(&items, annotation); + let nse_args = self.match_nse_arguments(&items, annotation); for (i, item) in items.iter().enumerate() { let Ok(arg) = item else { continue }; let Some(value) = arg.value() else { continue }; - let Some(nse_arg) = nse_args[i] else { - self.collect_expression(&value); - continue; - }; - - self.record_nse_argument(nse_arg, &value); - - if self.is_rewalk { - // On rewalks, we push nested NSE scopes and collect definitions there - self.collect_nse_argument(nse_arg, &value); - } else { - // On the first walk, keep flat and collect definitions in the current scope - self.collect_expression(&value); + match nse_args[i] { + None => self.collect_expression(&value), + Some(nse_arg) => self.collect_nse_argument(nse_arg, &value), } } } @@ -177,7 +324,7 @@ impl SemanticIndexBuilder { /// FIXME: This is a stopgap helper. In the future, `Effects` will be /// returned from the resolvers with the function signature, and we'll /// implement a proper argument matching routine. - fn match_nse_args( + fn match_nse_arguments( &self, items: &RArgumentList, annotation: NseAnnotation, @@ -217,23 +364,6 @@ impl SemanticIndexBuilder { nse_args } - fn record_nse_argument(&mut self, scoped: &NseArgument, value: &AnyRExpression) { - match (scoped.scope, scoped.timing) { - // Doesn't push a scope, nothing to record. - (NseScope::Current, NseTiming::Eager) => {}, - // Routes to the parent. No body range to skip, but still a virtual scope. - (NseScope::Current, NseTiming::Lazy) => { - self.found_nse = true; - }, - // Note the body range so pre-scans skip it. - (NseScope::Nested, _) => { - self.found_nse = true; - self.nse_nested_ranges - .insert(value.syntax().text_trimmed_range()); - }, - } - } - /// Walk a single NSE argument body, pushing a scope when appropriate. fn collect_nse_argument(&mut self, nse_arg: &NseArgument, value: &AnyRExpression) { match (nse_arg.scope, nse_arg.timing) { @@ -245,11 +375,12 @@ impl SemanticIndexBuilder { let kind = ScopeKind::Nse(nse_scope, nse_timing); let scope = self.push_scope(kind, value.syntax().text_trimmed_range()); - // Only `Nested` scopes hold their own definitions and get pre-scanned - if nse_scope == NseScope::Nested { - self.pre_scan_scope(value.syntax()); - } - + // Scan the child body before walking it. A `Current + Lazy` + // scope routes its defs to the owner and holds no bound names of its + // own, which `record_binding` handles; the scan still runs to + // record the body's NSE decisions in the child's flow context. + self.begin_scan(); + self.scan_expression(value); self.collect_expression(value); self.pop_scope(scope); }, diff --git a/crates/oak_semantic/src/semantic_index.rs b/crates/oak_semantic/src/semantic_index.rs index 9f3406cb62..209f686379 100644 --- a/crates/oak_semantic/src/semantic_index.rs +++ b/crates/oak_semantic/src/semantic_index.rs @@ -85,6 +85,10 @@ pub struct SemanticIndex { // `package:::symbol` namespace_accesses: Vec, + // Diagnostics surfaced during indexing, for downstream consumers to turn + // into user-facing diagnostics. + diagnostics: Vec, + // The file scope's exit flow state: for each top-level symbol, the // definitions still in effect once the file has run top to bottom. This is // the file's exports (see `exports()`). Only the file scope's exit state is @@ -102,6 +106,7 @@ impl SemanticIndex { enclosing_snapshots: FxHashMap, semantic_calls: Vec, namespace_accesses: Vec, + diagnostics: Vec, final_bindings: IndexVec, ) -> Self { Self { @@ -113,6 +118,7 @@ impl SemanticIndex { enclosing_snapshots, semantic_calls, namespace_accesses, + diagnostics, final_bindings, } } @@ -190,6 +196,12 @@ impl SemanticIndex { &self.namespace_accesses } + /// Diagnostics surfaced during indexing, for downstream consumers to turn + /// into user-facing diagnostics. + pub fn diagnostics(&self) -> &[SemanticDiagnostic] { + &self.diagnostics + } + /// Find the innermost scope containing `offset`. pub fn scope_at(&self, offset: biome_rowan::TextSize) -> (ScopeId, &Scope) { // Start at the file scope @@ -793,6 +805,15 @@ pub enum NamespaceAccessKind { Internal, } +/// A diagnostic surfaced while building the semantic index, for downstream +/// consumers to turn into user-facing diagnostics. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SemanticDiagnostic { + /// An NSE call recognized in a lazy context whose binding is overwritten + /// later on (in subsequent parent code or in another lazy context). + LazyShadowAmbiguity { name: String, range: TextRange }, +} + // --- Iterators --- pub struct ChildScopeIdsIter<'a> { diff --git a/crates/oak_semantic/src/use_def_map.rs b/crates/oak_semantic/src/use_def_map.rs index afa97aedb9..890dc3835f 100644 --- a/crates/oak_semantic/src/use_def_map.rs +++ b/crates/oak_semantic/src/use_def_map.rs @@ -478,13 +478,6 @@ impl UseDefMapBuilder { self.symbol_states[symbol_id].may_be_unbound() } - /// Returns `true` if `symbol_id` is definitely unbound at this point: no - /// definition reaches it on any control-flow path. - pub(crate) fn is_unbound(&self, symbol_id: SymbolId) -> bool { - let state = &self.symbol_states[symbol_id]; - state.may_be_unbound() && state.definitions().is_empty() - } - /// Register an enclosing snapshot for `symbol_id`. The snapshot starts from /// the current flow state (prior shadowing applied). A watcher is /// registered so that each subsequent definition of this symbol we diff --git a/crates/oak_semantic/tests/integration/builder_nse.rs b/crates/oak_semantic/tests/integration/builder_nse.rs index ca7da3531e..9dca96e008 100644 --- a/crates/oak_semantic/tests/integration/builder_nse.rs +++ b/crates/oak_semantic/tests/integration/builder_nse.rs @@ -6,6 +6,7 @@ use oak_semantic::semantic_index::NseScope; use oak_semantic::semantic_index::NseTiming; use oak_semantic::semantic_index::ScopeId; use oak_semantic::semantic_index::ScopeKind; +use oak_semantic::semantic_index::SemanticDiagnostic; use oak_semantic::semantic_index::SemanticIndex; use oak_semantic::semantic_index::SymbolFlags; use oak_semantic::semantic_index::UseId; @@ -225,9 +226,9 @@ local({ } #[test] -fn test_nse_rewalk_moves_definitions() { - // The re-walk should correctly move definitions from the parent scope - // into the NSE child scope. +fn test_nse_moves_definitions_into_nested_scope() { + // Definitions inside an NSE body land in the NSE child scope, not the + // parent, even with sibling definitions on either side at file level. let index = index( "\ x <- 0 @@ -323,10 +324,9 @@ local({ #[test] fn test_nse_prescan_skips_nested_bodies() { - // The pre-scan for the file scope should NOT include definitions from - // inside `local()` bodies on the re-walk. This means a function defined - // AFTER the local() call should not see `x` from inside local via the - // pre-scan. + // The file scope's bound names must NOT include definitions from inside + // `local()` bodies. This means a function defined AFTER the local() call + // should not see `x` from inside local via the bound names. let index = index( "\ local({ @@ -463,11 +463,11 @@ f <- function() x } #[test] -fn test_nse_rewalk_convergence_unmasked_call() { - // Pathological case: redefining `local` inside a `local()` body unmasks - // a later `local()` call on the re-walk. The convergence loop handles - // this: the first re-walk discovers the second call, the second re-walk - // has the correct pre-scan skip set. +fn test_nse_unmasked_call_via_nested_scope() { + // Redefining `local` inside a `local()` body doesn't shadow a later + // `local()` call: the rebind lands in the first body's NSE scope, so it + // never enters the file's bound names. The scan walks the first `local()` + // inline, so the second call sees `local` still unbound in the same pass. let index = index( "\ local({ @@ -515,20 +515,14 @@ local({ } #[test] -fn test_nse_rewalk_convergence_ancestor_unmask_across_function() { - // Convergence where the ancestor shadowing check flips across iterations, - // and where a single re-walk would leave an observable mistake. - // - // `local <- identity` starts at file scope, so the `local()` call inside - // `f` sees an enclosing binding and is not NSE. The first re-walk moves - // `local <- identity` into the outer local scope, unmasking base `local` - // for the call in `f`, so that call becomes NSE and its body range is - // recorded. But during that same re-walk, `f`'s pre-scan hasn't been told - // to skip the inner local body yet, so it still collects `x`, which would - // give `g`'s free `x` a bogus enclosing snapshot in `f`. Only the next - // re-walk, with the inner body range known, pre-scans `f` without `x` and - // leaves `g`'s `x` correctly unresolved (the sibling `local()` binds `x` in - // its own env, invisible to `g`). +fn test_nse_ancestor_unmask_across_function() { + // The `local <- identity` rebind lives inside the outer `local()` body, so + // it never enters the file's bound names. The scan of `f`'s body therefore + // sees base `local` unbound and marks the inner `local()` NSE, cutting its + // body out of `f`'s bound names in the same pass. So `x <- 1` lands in the + // inner NSE scope and `g`'s free `x` stays unresolved (the sibling + // `local()` binds `x` in its own env, invisible to `g`). The old re-walk + // needed a second iteration to reach this; the scan gets it in one. let index = index( "\ local({ @@ -571,12 +565,113 @@ f <- function() { ); // `g`'s free `x` resolves to nothing: the sibling `local()` binds `x` in - // its own scope, not in `f`. A single re-walk would wrongly point it at a - // stray `x` in `f`'s pre-scan. + // its own scope, not in `f`. Flow-insensitive bound names would wrongly + // point it at a stray `x` in `f`. let g_x = index.uses(g_scope)[UseId::from(0)].symbol(); assert_eq!(index.enclosing_bindings(g_scope, g_x), None); } +#[test] +fn test_nse_sibling_branch_flow_precise() { + // Flow-precise scan across `if`/`else`. `local` is bound only on the + // consequence path, so on the else path base `local` is still unbound and + // `local({...})` is NSE. Flow-insensitive bound names would see `local` + // bound (from the consequence) and miss the NSE call, leaking `y` into the + // file scope. + let index = index( + "\ +if (c) local <- identity else local({ + y <- 1 +}) +", + ); + let file = ScopeId::from(0); + let nse_scope = ScopeId::from(1); + + // Only the file scope and the else branch's NSE scope exist. + assert_eq!(index.scope_ids().count(), 2); + assert_eq!( + index.scope(nse_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(nse_scope).parent(), Some(file)); + + // `y` lands in the NSE scope, not the file scope. + assert!(index.symbols(file).get("y").is_none()); + assert_eq!( + index.symbols(nse_scope).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + // `local` is bound at file scope (from the consequence branch). + assert!(index + .symbols(file) + .get("local") + .unwrap() + .flags() + .contains(SymbolFlags::IS_BOUND)); +} + +#[test] +fn test_nse_eager_lazy_split_on_later_binding() { + // A later file-level `local <- identity` does NOT shadow `local()` inside the + // function `f`. `f`'s body is lazy, so its run time relative to the binding + // is unknown, and `is_locally_bound` reads only `f`'s eager predecessors (the + // predecessor snapshot, empty here). So the lazy `local()` is optimistically + // NSE and `x` moves into its own scope. The genuine ambiguity (does `f` run + // before or after the binding?) is the overturn lint's job, not a shadow. + // + // The eager `local()` at file scope is NSE too, but for a determined reason: + // it runs before the binding, so its flow-precise state has `local` unbound. + let index = index( + "\ +f <- function() { + local({ + x <- 1 + }) +} +local({ + y <- 1 +}) +local <- identity +", + ); + let file = ScopeId::from(0); + let f_scope = ScopeId::from(1); + let f_local = ScopeId::from(2); + let eager_local = ScopeId::from(3); + + // Four scopes: file, `f`, the NSE `local()` in `f`, and the eager `local()`. + assert_eq!(index.scope_ids().count(), 4); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + + // Lazy `local()` in `f` is NSE (later binding is not a predecessor), so `x` + // moves into its own scope. + assert_eq!( + index.scope(f_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(f_local).parent(), Some(f_scope)); + assert!(index.symbols(f_scope).get("x").is_none()); + assert_eq!( + index.symbols(f_local).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + // Eager file-level `local()` runs before the binding, so it is NSE and `y` + // lands in its own scope. + assert_eq!( + index.scope(eager_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(eager_local).parent(), Some(file)); + assert!(index.symbols(f_scope).get("y").is_none()); + assert_eq!( + index.symbols(eager_local).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + #[test] fn test_nse_local_inside_function() { // `local()` inside a function: the function boundary is lazy, so the @@ -859,3 +954,323 @@ fn test_nse_front_gate_consults_resolver_for_annotated_name() { assert!(consultations.get() > 0); } + +// --- source() bindings visible to the scan --- + +#[test] +fn test_nse_sourced_name_shadows_base_callee() { + // A `source()`-injected `local` shadows base `local`, so the later + // `local({...})` is NOT NSE. The scan binds the sourced names eagerly + // (source() runs at its position), so the later callee sees the shadow in + // the same pass, even though the walk injects the Import def later. + let index = build_with( + "\ +source(\"utils.R\") +local({ + x <- 1 +}) +", + TestImportsResolver::with_base().with_source("utils.R", &["local"]), + ); + let file = ScopeId::from(0); + + // No NSE scope: the sourced `local` shadows base, so `x` stays flat. + assert_eq!(index.scope_ids().count(), 1); + assert_eq!( + index.symbols(file).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_sourced_file_without_name_leaves_callee_nse() { + // Same shape, but the sourced file does not define `local`, so base + // `local` is unshadowed and `local({...})` IS NSE. + let index = build_with( + "\ +source(\"utils.R\") +local({ + x <- 1 +}) +", + TestImportsResolver::with_base().with_source("utils.R", &["other"]), + ); + let file = ScopeId::from(0); + let local_scope = ScopeId::from(1); + + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(local_scope).parent(), Some(file)); + assert!(index.symbols(file).get("x").is_none()); + assert_eq!( + index.symbols(local_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +// --- Current + Lazy owner bindings visible before the walk --- + +#[test] +fn test_nse_on_load_binding_order_independent() { + // A `local` bound inside a `Current + Lazy` `on_load` body is deferred + // (lazy-provenance), so it is not a precise predecessor for the lazy `local()` + // in a sibling function `f`. Both bodies run in an order the engine can't + // know, so whether the shadow holds when `f` runs is undetermined. The + // predecessor snapshot excludes the deferred `local`, so `f`'s `local()` is + // optimistically NSE in both orderings (the overturn lint, pending, flags the + // ambiguity). `x` moves into its own scope regardless of order. + let first = index( + "\ +f <- function() local({ x <- 1 }) +rlang::on_load({ local <- identity }) +", + ); + // Walk order: file, f, f's `local()` scope, on_load. + let f_first = ScopeId::from(1); + let f_local_first = ScopeId::from(2); + assert_eq!(first.scope_ids().count(), 4); + assert_eq!(first.scope(f_first).kind(), ScopeKind::Function); + assert_eq!( + first.scope(f_local_first).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(first.scope(f_local_first).parent(), Some(f_first)); + assert!(first.symbols(f_first).get("x").is_none()); + assert_eq!( + first.symbols(f_local_first).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + let second = index( + "\ +rlang::on_load({ local <- identity }) +f <- function() local({ x <- 1 }) +", + ); + // Walk order: file, on_load, f, f's `local()` scope. + let f_second = ScopeId::from(2); + let f_local_second = ScopeId::from(3); + assert_eq!(second.scope_ids().count(), 4); + assert_eq!(second.scope(f_second).kind(), ScopeKind::Function); + assert_eq!( + second.scope(f_local_second).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(second.scope(f_local_second).parent(), Some(f_second)); + assert!(second.symbols(f_second).get("x").is_none()); + assert_eq!( + second.symbols(f_local_second).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_on_load_nested_binding_order_independent() { + // Same as the direct-binding case above, but the shadowing binding is buried + // in a nested transparent call (`evalq(...)`). It makes no difference to the + // NSE decision: the predecessor snapshot reads only `f`'s eager predecessors, + // and `on_load`'s deferred `local` is not one of them however it is written. + // So `f`'s `local()` is optimistically NSE in both orderings and `x` moves + // into its own scope. (Under the old whole-scope read this case was + // order-dependent, because it hinged on whether the walk had routed `local` + // to the owner's bound names before it reached `f`.) + + // `f` before the `on_load`. + let first = index( + "\ +f <- function() local({ x <- 1 }) +rlang::on_load({ evalq(local <- identity) }) +", + ); + let f = ScopeId::from(1); + let nested = ScopeId::from(2); + assert_eq!(first.scope_ids().count(), 4); + assert_eq!(first.scope(f).kind(), ScopeKind::Function); + assert_eq!( + first.scope(nested).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(first.scope(nested).parent(), Some(f)); + assert!(first.symbols(f).get("x").is_none()); + assert_eq!( + first.symbols(nested).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + // `f` after the `on_load`: same result, `local()` is still NSE. + let second = index( + "\ +rlang::on_load({ evalq(local <- identity) }) +f <- function() local({ x <- 1 }) +", + ); + let f_second = ScopeId::from(2); + let nested_second = ScopeId::from(3); + assert_eq!(second.scope_ids().count(), 4); + assert_eq!(second.scope(f_second).kind(), ScopeKind::Function); + assert_eq!( + second.scope(nested_second).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(second.scope(nested_second).parent(), Some(f_second)); + assert!(second.symbols(f_second).get("x").is_none()); + assert_eq!( + second.symbols(nested_second).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_on_load_deferred_binding_unbound_at_eager_position() { + // The eager stance: `on_load`'s `local` reaches only the owner's bound names, + // never `bound_so_far`. A file-level (eager) `local()` after the + // `on_load` runs before the deferred body, so it treats `local` as unbound + // and IS NSE. Contrast with the lazy sibling case above. + let index = index( + "\ +rlang::on_load({ local <- identity }) +local({ x <- 1 }) +", + ); + let file = ScopeId::from(0); + let on_load_scope = ScopeId::from(1); + let local_scope = ScopeId::from(2); + + assert_eq!(index.scope_ids().count(), 3); + assert_eq!( + index.scope(on_load_scope).kind(), + ScopeKind::Nse(NseScope::Current, NseTiming::Lazy) + ); + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(local_scope).parent(), Some(file)); + assert!(index.symbols(file).get("x").is_none()); + assert_eq!( + index.symbols(local_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +// --- NSE calls in parameter defaults --- + +#[test] +fn test_nse_parameter_default_pushes_scope() { + // An NSE call in a parameter default is recognized and pushes its scope. + let index = index("f <- function(a = local({ x <- 1 })) a\n"); + let f_scope = ScopeId::from(1); + let local_scope = ScopeId::from(2); + + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(local_scope).parent(), Some(f_scope)); + + // `x` lands in the default's NSE scope, not the function scope. + assert!(index.symbols(f_scope).get("x").is_none()); + assert_eq!( + index.symbols(local_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_parameter_default_shadowed_by_param() { + // All formals bind at once, so a `local` parameter shadows base `local` in + // a later default, regardless of order: `local({...})` is NOT NSE and `x` + // stays flat in the function scope. + let index = index("f <- function(local, a = local({ x <- 1 })) a\n"); + 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 + ); +} + +// --- Lazy shadow ambiguity diagnostics --- + +#[test] +fn test_diagnostic_lazy_shadow_later_eager_binding() { + // `f`'s `local()` is optimistically NSE, but a later file-level `local` + // binding could shadow it depending on when `f` runs. Flagged. + let source = "\ +f <- function() local({ x <- 1 }) +local <- identity +"; + let index = index(source); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::LazyShadowAmbiguity { name, range } => { + assert_eq!(name, "local"); + let start = u32::from(range.start()) as usize; + let end = u32::from(range.end()) as usize; + assert_eq!(&source[start..end], "local({ x <- 1 })"); + }, + } +} + +#[test] +fn test_diagnostic_lazy_shadow_on_load_binding() { + // A deferred `on_load` binding of `local` and a lazy sibling's `local()` + // run in an unknown order. Flagged. + let index = index( + "\ +f <- function() local({ x <- 1 }) +rlang::on_load({ local <- identity }) +", + ); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::LazyShadowAmbiguity { name, .. } => assert_eq!(name, "local"), + } +} + +#[test] +fn test_diagnostic_none_at_eager_position() { + // The file-level `local()` runs before the `on_load` hook fires, so its + // "unbound" reading is determined, not a guess. No diagnostic. + let index = index( + "\ +rlang::on_load({ local <- identity }) +local({ x <- 1 }) +", + ); + assert!(index.diagnostics().is_empty()); +} + +#[test] +fn test_diagnostic_none_when_callee_unbound_everywhere() { + // `local` is never bound anywhere, so the NSE decision is certain and + // nothing competes with it. No diagnostic. + let index = index( + "\ +x <- 1 +f <- function() local({ x }) +", + ); + assert!(index.diagnostics().is_empty()); +} + +#[test] +fn test_diagnostic_none_with_eager_predecessor() { + // `local` is bound before `f` is defined, a sure shadow, so `f`'s `local()` + // is not NSE at all. No diagnostic. + let index = index( + "\ +local <- identity +f <- function() local({ x }) +", + ); + assert!(index.diagnostics().is_empty()); +} diff --git a/crates/oak_semantic/tests/integration/resolvers.rs b/crates/oak_semantic/tests/integration/resolvers.rs index 44d2b7099c..99fe15eb2f 100644 --- a/crates/oak_semantic/tests/integration/resolvers.rs +++ b/crates/oak_semantic/tests/integration/resolvers.rs @@ -1,10 +1,12 @@ use std::cell::Cell; +use std::collections::HashMap; use std::rc::Rc; use oak_semantic::effects_registry; use oak_semantic::Effects; use oak_semantic::ImportsResolver; use oak_semantic::SourceResolution; +use url::Url; /// Test resolver: an explicit search path resolved against the registry. /// @@ -19,6 +21,8 @@ pub struct TestImportsResolver { /// Count of `resolve_effects` consultations, so tests can assert the front /// gate keeps unannotated names off the resolver. consultations: Rc>, + /// `source()` paths this resolver knows, mapped to the names they export. + sources: HashMap, } impl TestImportsResolver { @@ -28,9 +32,22 @@ impl TestImportsResolver { Self { always_attached: vec![String::from("base")], consultations: Rc::new(Cell::new(0)), + sources: HashMap::new(), } } + /// Register a sourced file at `path` exporting `names`, so `resolve_source` + /// returns a resolution for it. The URL is synthesized from the path. + pub fn with_source(mut self, path: &str, names: &[&str]) -> Self { + let resolution = SourceResolution { + url: Url::parse(&format!("file:///{path}")).unwrap(), + names: names.iter().map(|name| name.to_string()).collect(), + packages: vec![], + }; + self.sources.insert(path.to_string(), resolution); + self + } + /// A handle to the consultation counter. Clone it before moving the /// resolver into `build_index`, then read it after the build. pub fn consultations(&self) -> Rc> { @@ -39,8 +56,8 @@ impl TestImportsResolver { } impl ImportsResolver for TestImportsResolver { - fn resolve_source(&mut self, _path: &str) -> Option { - None + fn resolve_source(&mut self, path: &str) -> Option { + self.sources.get(path).cloned() } fn resolve_effects(&mut self, name: &str, attached: &[String], _lazy: bool) -> Option { From 3c5544d900344dc8133f727d2ecb148e6ca59350 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 10 Jul 2026 08:37:06 +0200 Subject: [PATCH 03/12] Finish eager linear scan --- crates/oak_semantic/src/builder.rs | 150 ++++++++++----- .../oak_semantic/src/builder/builder_nse.rs | 106 ++++++++-- .../tests/integration/builder_nse.rs | 181 ++++++++++++++++++ .../tests/integration/resolvers.rs | 16 +- 4 files changed, 388 insertions(+), 65 deletions(-) diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index 49402c01d6..cd00919a14 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -84,6 +84,7 @@ pub fn build_index(root: &RRoot, resolver: impl ImportsResolver) -> SemanticInde // parallel arrays are pushed in lockstep so they stay indexed by the same // `ScopeId`. struct SemanticIndexBuilder { + resolver: R, scopes: IndexVec, symbol_tables: IndexVec, definitions: IndexVec>, @@ -94,29 +95,24 @@ struct SemanticIndexBuilder { enclosing_snapshots: FxHashMap, semantic_calls: Vec, namespace_accesses: Vec, - // The NSE effect each call resolved to, keyed by the call's range. Filled - // in flow order by the scan. Absence means "not NSE". - // - // File-global on purpose, not per-scope: a `TextRange` is unique across the - // file, so entries from different scopes can't collide. - nse_annotations: FxHashMap, - // Resolved `source()` calls, keyed by the call's range. The scan fills - // this once per call (consulting `resolve_source`), the walk reads it back, - // so the resolver is queried exactly once per `source()` call site. - source_resolutions: FxHashMap, + // Per-call facts resolved by the scanner in flow order, keyed by the call's + // range. See `CallResolution`. + call_resolutions: FxHashMap, // Names bound so far in the scope currently being scanned, tracked // flow-precisely (if/else restore, loop union). This is the scan // pass's own flow state, standing in for the walk's use-def state which // isn't built yet. Reset at each scope's `begin_scan()`. bound_so_far: FxHashSet, - // The enclosing eager environment captured when each child scope is - // entered, keyed by the child's range. Recorded from `bound_so_far` when - // the parent's scan reaches the child's definition point. - eager_bindings: FxHashMap>, + // Names inherited from enclosing scopes at this scope's entry point, keyed + // by the scope's range. Captured from `bound_so_far`, and read by + // `begin_scan()` to seed the scope's own scan. + inherited_at_entry: FxHashMap>, + // Bound names of Eager + Nested bodies like `local()` are discovered inline + // by the scanner. See `EagerNestedDescent`. + descent: EagerNestedDescent, // Diagnostics collected during the build and logged on `finish()`. A minimal // channel for now, no user-facing surface. diagnostics: Vec, - resolver: R, } impl SemanticIndexBuilder { @@ -158,10 +154,10 @@ impl SemanticIndexBuilder { enclosing_snapshots: FxHashMap::default(), semantic_calls: Vec::new(), namespace_accesses: Vec::new(), - nse_annotations: FxHashMap::default(), - source_resolutions: FxHashMap::default(), + call_resolutions: FxHashMap::default(), bound_so_far: FxHashSet::default(), - eager_bindings: FxHashMap::default(), + inherited_at_entry: FxHashMap::default(), + descent: EagerNestedDescent::default(), diagnostics: Vec::new(), resolver, } @@ -450,13 +446,15 @@ impl SemanticIndexBuilder { found_by_flag || self.bound_names[scope].binds(name) } - /// Record the eager environment for a child scope (function body, NSE - /// argument) about to be created at `range`, 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). - pub(super) fn record_eager_bindings(&mut self, range: TextRange) { - self.eager_bindings.insert(range, self.bound_so_far.clone()); + /// Record the names 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). + pub(super) fn record_inherited_at_entry(&mut self, range: TextRange) { + self.inherited_at_entry + .insert(range, self.bound_so_far.clone()); } // --- Scan pass --- @@ -465,9 +463,9 @@ impl SemanticIndexBuilder { /// /// Seeds it with two things: /// - /// - The enclosing eager environment captured when this scope was entered - /// (`eager_bindings`). The parent's own scan was seeded the same way, - /// so this is transitively complete: it holds every eager binding + /// - The names inherited from enclosing scopes, captured when this scope was + /// entered (`inherited_at_entry`). The parent's own scan was seeded the same + /// way, so this is transitively complete: it holds every eager binding /// visible from an ancestor at this scope's definition point. /// - The scope's own already-bound names. For a function scope that's the /// parameters, recorded just before the scan runs. For file and NSE scopes @@ -480,7 +478,7 @@ impl SemanticIndexBuilder { self.bound_so_far.clear(); let range = self.scopes[self.current_scope].range; - if let Some(entry) = self.eager_bindings.get(&range) { + if let Some(entry) = self.inherited_at_entry.get(&range) { self.bound_so_far.extend(entry.iter().cloned()); } @@ -501,21 +499,24 @@ impl SemanticIndexBuilder { /// /// Runs before the walk of a scope. It decides NSE-ness at each call the /// same way the walk's [`is_locally_bound`](Self::is_locally_bound) would, - /// records the decision in `nse_annotations` for the walk to reuse, and adds + /// records the decision in `call_resolutions` for the walk to reuse, and adds /// non-skipped definition names to `bound_names`. The bound names must be /// complete before the walk descends into any child scope, because a lazy /// child body can reference an ancestor def the ancestor's walk hasn't /// reached yet. /// - /// The scan matches the walk's scope boundaries: + /// A scan unit is the file or a lazy body (function, `Nested + Lazy`, + /// `Current + Lazy`). Each unit is scanned once. Within a unit the scan + /// descends through every eager boundary it meets, in flow order: /// - /// - Function and `Nested` NSE bodies are child scopes, scanned - /// separately in their own context. - /// - A `Current + Lazy` body is also a child scope, scanned separately for - /// the same reason: NSE resolution needs the child's own flow context, - /// which differs from this scope's. /// - A `Current + Eager` body pushes no scope, so it stays part of this /// scope's direct level and is scanned through transparently. + /// - A `Nested + Eager` body is descended into with a save/restore of + /// `bound_so_far`, and the names it binds are left pending for the walk to + /// install without re-scanning. + /// - Function and lazy bodies (`Nested + Lazy`, `Current + Lazy`) are their + /// own scan units, scanned separately when the walk enters them, because + /// NSE resolution there needs the child's own flow context. /// /// Branch analysis is precise. In `if (c) local <- f else local({ y <- 1 /// })` the else branch sees an NSE call because `local` is unbound on the @@ -524,10 +525,10 @@ impl SemanticIndexBuilder { match expr { AnyRExpression::RFunctionDefinition(func) => { // A function body is a child scope, scanned when it's entered. - // Record this scope's eager bindings now so that when we later - // resolve an NSE callee inside the body, we can check whether one - // of them shadows it (see `bound_at_entry`). - self.record_eager_bindings(func.syntax().text_trimmed_range()); + // Record the names it inherits now so that when we later resolve + // an NSE callee inside the body, we can check whether one of them + // shadows it (see `inherited_at_entry`). + self.record_inherited_at_entry(func.syntax().text_trimmed_range()); }, AnyRExpression::RBracedExpressions(braced) => { @@ -705,8 +706,10 @@ impl SemanticIndexBuilder { self.record_binding(name.clone()); } - self.source_resolutions - .insert(call.syntax().text_trimmed_range(), resolution); + self.call_resolutions + .entry(call.syntax().text_trimmed_range()) + .or_default() + .source = Some(resolution); } /// Record a binding in the scan's flow state. @@ -722,13 +725,20 @@ impl SemanticIndexBuilder { } /// Route a binding NAME into its owner scope's bound names, matching - /// `add_definition`'s routing. A `Current + Lazy` scope routes to - /// `definition_owner()`, every other scope owns its bindings. + /// `add_definition`'s routing. When a descent is open the name goes to the + /// descent top, which is always an eager `Nested` body scanned inline and so + /// owns its bindings. Otherwise a `Current + Lazy` scope routes to + /// `definition_owner()` and every other scope owns its bindings. /// /// Split from `record_binding` so `scan_lazy_owner_bindings` can add /// a deferred body's names to the owner's bound names without also marking them /// bound in `bound_so_far` (see that helper for why). fn record_owner_name(&mut self, name: String) { + if let Some(bound) = self.descent.open.last_mut() { + bound.add(name); + return; + } + if let Some(target) = match self.scopes[self.current_scope].kind { ScopeKind::Nse(NseScope::Current, NseTiming::Lazy) => self.definition_owner(), _ => Some(self.current_scope), @@ -738,9 +748,9 @@ impl SemanticIndexBuilder { } fn nse_effect(&self, call: &RCall) -> Option { - self.nse_annotations + self.call_resolutions .get(&call.syntax().text_trimmed_range()) - .copied() + .and_then(|resolution| resolution.nse) } // --- Recursive descent --- @@ -1211,7 +1221,10 @@ impl SemanticIndexBuilder { // single point that consults `resolve_source`, so the walk never // re-resolves. A cache miss means the scan bailed or the resolver // returned `None`, both of which record the call with `resolved: None`. - let resolution = self.source_resolutions.get(&range).cloned(); + let resolution = self + .call_resolutions + .get(&range) + .and_then(|resolution| resolution.source.clone()); // Record every `source()` call site, independent of whether the // resolution was successful. `resolved` pins the canonical URL when @@ -1353,6 +1366,49 @@ impl SemanticIndexBuilder { } } +/// What the scan resolved a single call to, for the walk to reuse. A call can +/// carry both facts at once. +/// +/// - `nse`: the NSE effect the call resolved to, filled in flow order. `None` +/// means "not NSE". +/// - `source`: the resolution of a `source()` call. The scan fills it once +/// (consulting `resolve_source`), the walk reads it back, so the resolver is +/// queried exactly once per `source()` call site. +#[derive(Default)] +struct CallResolution { + nse: Option, + source: Option, +} + +/// Tracks eager `Nested` NSE bodies scanned inline during the scan. +/// +/// An eager `Nested` body like `local()` runs immediately at its call site, so +/// we scan it inline instead of deferring it to the walk. `open` is the stack +/// of bodies being scanned right now, with the innermost on top. +/// `record_owner_name()` routes a binding to the top so names land on the body +/// that owns them. When a descent finishes, its names move to `pending`, keyed +/// by the body's range. +/// +/// `pending` is keyed by range rather than written straight into +/// `bound_names[scope]` because the body's arena scope doesn't exist yet: the +/// walk allocates scopes in preorder, and allocating one mid-scan would break +/// the `Scope::descendants` invariant. The range is the body's pre-arena +/// identity until the walk pushes its scope. +/// +/// Once the walk pushes that scope, it installs the pending names into it +/// instead of re-scanning. It does this before collecting the body, because a +/// lazy child inside (a function or lazy NSE body) runs later than the walk +/// reaches it, so it can reference a binding defined further down this scope. +/// Resolving that name checks whether an enclosing scope binds it +/// (`scope_binds_anywhere()`), and the walk hasn't recorded that binding yet, so +/// the scan-populated bound set has to be complete up front. That's the reason +/// the scan collects bound names ahead of the walk at all. +#[derive(Default)] +struct EagerNestedDescent { + open: Vec, + pending: FxHashMap, +} + /// All definitions in a scope, collected by the scan pass before the /// walk. Skips child-scope bodies (nested functions and `Nested` NSE bodies). struct BoundNames { diff --git a/crates/oak_semantic/src/builder/builder_nse.rs b/crates/oak_semantic/src/builder/builder_nse.rs index 0f2e08e47c..154b4aa351 100644 --- a/crates/oak_semantic/src/builder/builder_nse.rs +++ b/crates/oak_semantic/src/builder/builder_nse.rs @@ -14,6 +14,7 @@ use super::assignment_name; use super::is_assignment; use super::is_right_assignment; use super::is_super_assignment; +use super::BoundNames; use super::SemanticIndexBuilder; use crate::effects::Effects; use crate::effects::NseAnnotation; @@ -29,14 +30,17 @@ impl SemanticIndexBuilder { /// Scan a call for effects (e.g. NSE scopes) and record its decision for /// the walk to reuse. /// - /// If the callee resolves to an NSE annotation, the annotation is stored in - /// `nse_annotations` keyed by the call's range. Arguments evaluated in nested - /// calls are scanned accordingly. Otherwise all arguments are scanned in - /// the current scope. + /// If the callee resolves to an NSE annotation, the annotation is recorded + /// in `call_resolutions` under the call's range (as the entry's `nse`). + /// Arguments evaluated in nested calls are scanned accordingly. Otherwise + /// all arguments are scanned in the current scope. /// - /// We only fully scan `Current + Eager` arguments here. The child scopes - /// created by `Nested` and `Current + Lazy` bodies are scanned by the later - /// walk because callee resolution needs the child's own flow context. + /// `Current + Eager` and `Nested + Eager` arguments are scanned here: + /// `Current + Eager` transparently, `Nested + Eager` by descending into the + /// body and holding the names it binds as pending. `Nested + Lazy` and + /// `Current + Lazy` bodies are their own scan units and deferred to the walk + /// because resolution of effects in these lazy scopes needs the child's own + /// flow context. pub(super) fn scan_call(&mut self, call: &RCall) { let Some(annotation) = self.resolve_nse(call) else { if let Ok(args) = call.arguments() { @@ -50,8 +54,10 @@ impl SemanticIndexBuilder { return; }; - self.nse_annotations - .insert(call.syntax().text_trimmed_range(), annotation); + self.call_resolutions + .entry(call.syntax().text_trimmed_range()) + .or_default() + .nse = Some(annotation); let Ok(args) = call.arguments() else { return; @@ -66,21 +72,46 @@ impl SemanticIndexBuilder { match nse_args[i] { None => self.scan_expression(&value), Some(nse_arg) => match (nse_arg.scope, nse_arg.timing) { + // Calls like `evalq()` (NseScope::Current, NseTiming::Eager) => self.scan_expression(&value), - // e.g. `on_load({ ... })`. Its body runs later, so its defs + + // Calls like `on_load()`. Its body runs later, so its defs // land in the enclosing scope. We don't resolve the body's // calls here. The walk does that once it enters the child // scope. But we do grab the names it defines now, so the // owner's bound names are complete before the walk reaches a sibling. (NseScope::Current, NseTiming::Lazy) => { - self.record_eager_bindings(value.syntax().text_trimmed_range()); + self.record_inherited_at_entry(value.syntax().text_trimmed_range()); self.scan_lazy_owner_bindings(&value); }, - // A `Nested` body is a child scope scanned when it's entered. - // Capture this scope's eager bindings for its callee + + // Calls like `local()`. Its body runs eagerly at the call + // site, so its environment IS the current `bound_so_far`. + // Descend now, holding the names bound in this scope as + // pending so the walk has access to them. No `bound_so_far` + // reset: the child sees exactly what `begin_scan()` would + // have seeded. + // No `record_inherited_at_entry()`: eager `Nested` bodies are + // never scanned at walk time, so nothing would read it. + (NseScope::Nested, NseTiming::Eager) => { + let old = self.bound_so_far.clone(); + + let range = value.syntax().text_trimmed_range(); + self.descent.open.push(BoundNames::new()); + self.scan_expression(&value); + if let Some(bound) = self.descent.open.pop() { + self.descent.pending.insert(range, bound); + } + + self.bound_so_far = old; + }, + + // Calls like `reactive()`. Its body runs at an unknown + // later time, so it's a child scope scanned when the walk + // enters it. Record the names it inherits for its callee // resolution, same as a function body. - (NseScope::Nested, _) => { - self.record_eager_bindings(value.syntax().text_trimmed_range()); + (NseScope::Nested, NseTiming::Lazy) => { + self.record_inherited_at_entry(value.syntax().text_trimmed_range()); }, }, } @@ -223,10 +254,14 @@ impl SemanticIndexBuilder { .and_then(|effects| effects.nse); } - // Now check imports since the symbol is locally unbound + // Now check imports since the symbol is locally unbound. The + // arena's `current_scope` is the scan unit's scope (the descent + // pushes no arena scopes), so its laziness is the "am I in a lazy + // context" test the resolver needs. + let lazy = self.scopes[self.current_scope].kind.is_lazy(); let nse = self .resolver - .resolve_effects(&name, &[], false) + .resolve_effects(&name, &[], lazy) .and_then(|effects| effects.nse)?; // The callee is unbound by any eager binding, so it is NSE. @@ -365,12 +400,49 @@ impl SemanticIndexBuilder { } /// Walk a single NSE argument body, pushing a scope when appropriate. + /// + /// `Current + Eager` stays in the current scope. `Nested + Eager` was + /// already scanned by the descent, so we install its pending names and only + /// walk. The remaining lazy bodies are their own scan units that we scan + /// here on entry. fn collect_nse_argument(&mut self, nse_arg: &NseArgument, value: &AnyRExpression) { match (nse_arg.scope, nse_arg.timing) { + // Calls like `evalq()` (NseScope::Current, NseTiming::Eager) => { self.collect_expression(value); }, + // Calls like `local()` + (NseScope::Nested, NseTiming::Eager) => { + let range = value.syntax().text_trimmed_range(); + let kind = ScopeKind::Nse(NseScope::Nested, NseTiming::Eager); + let scope = self.push_scope(kind, range); + + // Install the pending names the descent recorded for this body, + // before collecting so lazy children inside can see them via + // `scope_binds_anywhere()`. + match self.descent.pending.remove(&range) { + Some(bound) => self.bound_names[scope] = bound, + None => { + // An eager NSE scope is reachable only through the scan + // unit that descended into it, so the pending set must + // exist. If not this is a builder bug. In release + // builds we still scan the body here so the walk can + // proceed. This fallback runs with an empty eager + // environment and its shadow decisions are more + // degraded than a real lazy unit's. + stdext::debug_panic!( + "Missing pending bound names for eager NSE body at {range:?}" + ); + self.begin_scan(); + self.scan_expression(value); + }, + } + + self.collect_expression(value); + self.pop_scope(scope); + }, + (nse_scope, nse_timing) => { let kind = ScopeKind::Nse(nse_scope, nse_timing); let scope = self.push_scope(kind, value.syntax().text_trimmed_range()); diff --git a/crates/oak_semantic/tests/integration/builder_nse.rs b/crates/oak_semantic/tests/integration/builder_nse.rs index 9dca96e008..21a111b648 100644 --- a/crates/oak_semantic/tests/integration/builder_nse.rs +++ b/crates/oak_semantic/tests/integration/builder_nse.rs @@ -1274,3 +1274,184 @@ f <- function() local({ x }) ); assert!(index.diagnostics().is_empty()); } + +// --- Eager linear scan: descent and pending names --- + +#[test] +fn test_nse_descent_consults_each_call_once() { + // The inner `local` sits inside the outer `local`'s eager body. The descent + // scans it once and the walk installs the pending names instead of + // re-scanning, so each of the two calls reaches the resolver exactly once. + let resolver = TestImportsResolver::with_base(); + let consultations = resolver.consultations(); + + build_with("local({ local({ x <- 1 }) })", resolver); + + assert_eq!(consultations.get(), 2); +} + +#[test] +fn test_nse_descent_current_lazy_owner_routes_to_descent_top() { + // A `Current + Lazy` body (`on_load`) inside an eager `local` body binds `x`. + // During the descent, `record_owner_name` must route `x` to the descent top + // (local), not to the current scope. `scan_lazy_owner_bindings` runs while + // the arena's `current_scope` is still the file, so only the descent-top + // shortcut lands `x` in local's pending names. + // + // We pin it through a FORWARD reference: `f` uses `x` before `on_load` binds + // it, so the walk resolves the use through local's `bound_names` (the pending + // set), not through an already-recorded definition. If the routing regressed, + // `x` would land in the file and the use would resolve to the file scope. + let index = index( + "\ +local({ + f <- function() x + rlang::on_load({ x <- 1 }) +}) +", + ); + let local_scope = ScopeId::from(1); + let f_scope = ScopeId::from(2); + + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + + let x_sym = index.uses(f_scope)[UseId::from(0)].symbol(); + let (enclosing_scope, _bindings) = index.enclosing_bindings(f_scope, x_sym).unwrap(); + assert_eq!(enclosing_scope, local_scope); +} + +#[test] +fn test_nse_descent_snapshot_through_pending_scope() { + // The descent records `y` as pending for `local`'s scope; the walk installs + // it before walking `f`, so `f`'s use of `y` resolves to the enclosing + // snapshot in `local`. + let index = index( + "\ +local({ + y <- 1 + f <- function() y +}) +", + ); + let file = ScopeId::from(0); + let local_scope = ScopeId::from(1); + let f_scope = ScopeId::from(2); + + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(local_scope).parent(), Some(file)); + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + assert_eq!(index.scope(f_scope).parent(), Some(local_scope)); + + // `y` lands in local's scope. + assert_eq!( + index.symbols(local_scope).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); + + // `f`'s use of `y` resolves to local's snapshot. In local, `y` is + // DefinitionId 0 (`f` is DefinitionId 1). + let y_sym = index.uses(f_scope)[UseId::from(0)].symbol(); + let (enclosing_scope, bindings) = index.enclosing_bindings(f_scope, y_sym).unwrap(); + assert_eq!(enclosing_scope, local_scope); + assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); +} + +#[test] +fn test_nse_descent_eager_under_lazy() { + // `local` resolves during `f`'s walk-time scan (unit = `f`), which descends + // into the body and records its names as pending. `x` lands in local's + // Nested+Eager scope, not in `f`. + let index = index( + "\ +f <- function() { + local({ + x <- 1 + }) +} +", + ); + let f_scope = ScopeId::from(1); + let local_scope = ScopeId::from(2); + + assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); + assert_eq!( + index.scope(local_scope).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(local_scope).parent(), Some(f_scope)); + + assert!(index.symbols(f_scope).get("x").is_none()); + assert_eq!( + index.symbols(local_scope).get("x").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_descent_nested_eager_in_eager() { + // `local({ local({ y <- 1 }) })`: descent stack depth 2, each body's names + // pending under its own range. `y` lands in the inner scope. + let index = index( + "\ +local({ + local({ + y <- 1 + }) +}) +", + ); + let file = ScopeId::from(0); + let outer_local = ScopeId::from(1); + let inner_local = ScopeId::from(2); + + assert_eq!(index.scope_ids().count(), 3); + assert_eq!( + index.scope(outer_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(outer_local).parent(), Some(file)); + assert_eq!( + index.scope(inner_local).kind(), + ScopeKind::Nse(NseScope::Nested, NseTiming::Eager) + ); + assert_eq!(index.scope(inner_local).parent(), Some(outer_local)); + + assert!(index.symbols(outer_local).get("y").is_none()); + assert_eq!( + index.symbols(inner_local).get("y").unwrap().flags(), + SymbolFlags::IS_BOUND + ); +} + +#[test] +fn test_nse_descent_lazy_flag_eager_vs_lazy_context() { + // An eager callee at file scope consults with `lazy = false`; the same + // callee inside a function body consults with `lazy = true`. + let resolver = TestImportsResolver::with_base(); + let log = resolver.consultation_log(); + + build_with( + "\ +local({ x <- 1 }) +f <- function() { + local({ y <- 1 }) +} +", + resolver, + ); + + let records = log.borrow(); + let local_lazy: Vec = records + .iter() + .filter(|(name, _lazy)| name == "local") + .map(|(_name, lazy)| *lazy) + .collect(); + assert_eq!(local_lazy, vec![false, true]); +} diff --git a/crates/oak_semantic/tests/integration/resolvers.rs b/crates/oak_semantic/tests/integration/resolvers.rs index 99fe15eb2f..b84d849422 100644 --- a/crates/oak_semantic/tests/integration/resolvers.rs +++ b/crates/oak_semantic/tests/integration/resolvers.rs @@ -1,4 +1,5 @@ use std::cell::Cell; +use std::cell::RefCell; use std::collections::HashMap; use std::rc::Rc; @@ -21,6 +22,9 @@ pub struct TestImportsResolver { /// Count of `resolve_effects` consultations, so tests can assert the front /// gate keeps unannotated names off the resolver. consultations: Rc>, + /// Per-consultation `(name, lazy)` records, so tests can pin the `lazy` flag + /// the builder derives from the callee's context. + consultation_log: Rc>>, /// `source()` paths this resolver knows, mapped to the names they export. sources: HashMap, } @@ -32,6 +36,7 @@ impl TestImportsResolver { Self { always_attached: vec![String::from("base")], consultations: Rc::new(Cell::new(0)), + consultation_log: Rc::new(RefCell::new(Vec::new())), sources: HashMap::new(), } } @@ -53,6 +58,12 @@ impl TestImportsResolver { pub fn consultations(&self) -> Rc> { Rc::clone(&self.consultations) } + + /// A handle to the per-consultation `(name, lazy)` log. Clone it before + /// moving the resolver into `build_index`, then read it after the build. + pub fn consultation_log(&self) -> Rc>> { + Rc::clone(&self.consultation_log) + } } impl ImportsResolver for TestImportsResolver { @@ -60,8 +71,11 @@ impl ImportsResolver for TestImportsResolver { self.sources.get(path).cloned() } - fn resolve_effects(&mut self, name: &str, attached: &[String], _lazy: bool) -> Option { + fn resolve_effects(&mut self, name: &str, attached: &[String], lazy: bool) -> Option { self.consultations.set(self.consultations.get() + 1); + self.consultation_log + .borrow_mut() + .push((name.to_string(), lazy)); attached .iter() .rev() From 72aa4332a8910c794f5fda0c9f46cb8c8f57d411 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 16 Jul 2026 12:54:06 +0200 Subject: [PATCH 04/12] Add missing test --- .../tests/integration/builder_nse.rs | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/crates/oak_semantic/tests/integration/builder_nse.rs b/crates/oak_semantic/tests/integration/builder_nse.rs index 21a111b648..30559e99ed 100644 --- a/crates/oak_semantic/tests/integration/builder_nse.rs +++ b/crates/oak_semantic/tests/integration/builder_nse.rs @@ -1455,3 +1455,34 @@ f <- function() { .collect(); assert_eq!(local_lazy, vec![false, true]); } + +#[test] +fn test_nse_descent_eager_in_eager_in_function_stays_lazy() { + // An eager `local` nested inside another eager `local` inside a function + // still consults with `lazy = true`. Laziness is a property of the enclosing + // scan unit (the function), which the descent preserves by keeping + // `current_scope` on the function while it scans both eager bodies inline. If + // the inner `local` were resolved against its immediate eager scope instead, + // `is_lazy()` would read `false` and the flag would regress. + let resolver = TestImportsResolver::with_base(); + let log = resolver.consultation_log(); + + build_with( + "\ +f <- function() { + local({ + local({ x <- 1 }) + }) +} +", + resolver, + ); + + let records = log.borrow(); + let local_lazy: Vec = records + .iter() + .filter(|(name, _lazy)| name == "local") + .map(|(_name, lazy)| *lazy) + .collect(); + assert_eq!(local_lazy, vec![true, true]); +} From 25978921cb926c6bed00a09a69369236bc219e45 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 17 Jul 2026 10:28:29 +0200 Subject: [PATCH 05/12] Fix `annotates()` gate --- crates/oak_semantic/src/builder/builder_nse.rs | 14 +++++++------- crates/oak_semantic/src/effects_registry.rs | 5 ++++- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/crates/oak_semantic/src/builder/builder_nse.rs b/crates/oak_semantic/src/builder/builder_nse.rs index 154b4aa351..2a9ddda4e6 100644 --- a/crates/oak_semantic/src/builder/builder_nse.rs +++ b/crates/oak_semantic/src/builder/builder_nse.rs @@ -233,12 +233,6 @@ impl SemanticIndexBuilder { AnyRExpression::RIdentifier(ident) => { let name = ident.name_text(); - // Bail early if it is known that no package annotates this name - // with effects. This speeds up the common case of no known annotations. - if !effects_registry::is_annotated(&name) { - return None; - } - // First check for a local definition (which in the future may // contain NSE annotations that we resolve here) // @@ -254,6 +248,12 @@ impl SemanticIndexBuilder { .and_then(|effects| effects.nse); } + // Bail early if it is known that no package annotates this name + // with effects. This speeds up the common case of no known annotations. + if !effects_registry::annotates(&name) { + return None; + } + // Now check imports since the symbol is locally unbound. The // arena's `current_scope` is the scan unit's scope (the descent // pushes no arena scopes), so its laziness is the "am I in a lazy @@ -280,7 +280,7 @@ impl SemanticIndexBuilder { let pkg = left.identifier_text()?; let func_name = right.identifier_text()?; - if !effects_registry::is_annotated(&func_name) { + if !effects_registry::annotates(&func_name) { return None; } diff --git a/crates/oak_semantic/src/effects_registry.rs b/crates/oak_semantic/src/effects_registry.rs index d1afdae87e..efdada24ec 100644 --- a/crates/oak_semantic/src/effects_registry.rs +++ b/crates/oak_semantic/src/effects_registry.rs @@ -22,7 +22,10 @@ pub fn lookup(package: &str, function: &str) -> Option<&'static NseAnnotation> { /// Whether any registry entry annotates `name`. This is the bare-callee front /// gate: an unannotated name can't resolve to an effect no matter which provider /// wins, so recognition skips resolution entirely. -pub fn is_annotated(name: &str) -> bool { +/// +/// TODO: Should be a workspace-wide Salsa-cached query (similar to: does this +/// function dispatches). +pub fn annotates(name: &str) -> bool { REGISTRY.iter().any(|e| e.function == name) } From e96f08099c936b76dedd1ed803a10dd1aadc3bc7 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 17 Jul 2026 11:38:46 +0200 Subject: [PATCH 06/12] Upstream naming changes from PR7 --- crates/oak_semantic/src/builder.rs | 6 +++--- .../oak_semantic/src/builder/builder_nse.rs | 20 +++++++++---------- crates/oak_semantic/src/effects.rs | 10 +++++----- crates/oak_semantic/src/effects_registry.rs | 12 +++++------ 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index cd00919a14..2717029c15 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -30,7 +30,7 @@ use oak_index_vec::IndexVec; use rustc_hash::FxHashMap; use rustc_hash::FxHashSet; -use crate::effects::NseAnnotation; +use crate::effects::ArgumentsAnnotation; use crate::resolver::ImportsResolver; use crate::resolver::SourceResolution; use crate::semantic_index::Definition; @@ -747,7 +747,7 @@ impl SemanticIndexBuilder { } } - fn nse_effect(&self, call: &RCall) -> Option { + fn nse_effect(&self, call: &RCall) -> Option { self.call_resolutions .get(&call.syntax().text_trimmed_range()) .and_then(|resolution| resolution.nse) @@ -1376,7 +1376,7 @@ impl SemanticIndexBuilder { /// queried exactly once per `source()` call site. #[derive(Default)] struct CallResolution { - nse: Option, + nse: Option, source: Option, } diff --git a/crates/oak_semantic/src/builder/builder_nse.rs b/crates/oak_semantic/src/builder/builder_nse.rs index 2a9ddda4e6..636a0b1323 100644 --- a/crates/oak_semantic/src/builder/builder_nse.rs +++ b/crates/oak_semantic/src/builder/builder_nse.rs @@ -16,9 +16,9 @@ use super::is_right_assignment; use super::is_super_assignment; use super::BoundNames; use super::SemanticIndexBuilder; +use crate::effects::Argument; +use crate::effects::ArgumentsAnnotation; use crate::effects::Effects; -use crate::effects::NseAnnotation; -use crate::effects::NseArgument; use crate::effects_registry; use crate::resolver::ImportsResolver; use crate::semantic_index::NseScope; @@ -226,7 +226,7 @@ impl SemanticIndexBuilder { /// /// The bound check reads the scan pass's flow-precise binding state /// for the current scope, so this must run during the scan, not the walk. - fn resolve_nse(&mut self, call: &RCall) -> Option { + fn resolve_nse(&mut self, call: &RCall) -> Option { let func = call.function().ok()?; match &func { @@ -334,7 +334,7 @@ impl SemanticIndexBuilder { /// Process a call the scan pass decided is NSE. Match its arguments /// against the annotation, then handle each scoped argument, pushing NSE /// scopes inline. - pub(super) fn collect_nse_call(&mut self, call: &RCall, annotation: NseAnnotation) { + pub(super) fn collect_nse_call(&mut self, call: &RCall, annotation: ArgumentsAnnotation) { let Ok(args) = call.arguments() else { return; }; @@ -362,10 +362,10 @@ impl SemanticIndexBuilder { fn match_nse_arguments( &self, items: &RArgumentList, - annotation: NseAnnotation, - ) -> Vec> { + annotation: ArgumentsAnnotation, + ) -> Vec> { let arg_count = items.iter().count(); - let mut nse_args: Vec> = vec![None; arg_count]; + let mut nse_args: Vec> = vec![None; arg_count]; let mut consumed = vec![false; annotation.arguments.len()]; // Named pass @@ -405,7 +405,7 @@ impl SemanticIndexBuilder { /// already scanned by the descent, so we install its pending names and only /// walk. The remaining lazy bodies are their own scan units that we scan /// here on entry. - fn collect_nse_argument(&mut self, nse_arg: &NseArgument, value: &AnyRExpression) { + fn collect_nse_argument(&mut self, nse_arg: &Argument, value: &AnyRExpression) { match (nse_arg.scope, nse_arg.timing) { // Calls like `evalq()` (NseScope::Current, NseTiming::Eager) => { @@ -466,7 +466,7 @@ impl SemanticIndexBuilder { /// Should we do partial argument matching? Or rely on partial matching being linted? fn match_named_arg( arg: &aether_syntax::RArgument, - annotation: &NseAnnotation, + annotation: &ArgumentsAnnotation, consumed: &[bool], ) -> Option { let clause = arg.name_clause()?; @@ -495,7 +495,7 @@ fn match_named_arg( /// position 1, won't match. Good enough without the callee's formal list; /// revisit if it misses real cases. fn match_positional_arg( - annotation: &NseAnnotation, + annotation: &ArgumentsAnnotation, position: usize, consumed: &[bool], ) -> Option { diff --git a/crates/oak_semantic/src/effects.rs b/crates/oak_semantic/src/effects.rs index d576b5161b..9042a0bd46 100644 --- a/crates/oak_semantic/src/effects.rs +++ b/crates/oak_semantic/src/effects.rs @@ -8,24 +8,24 @@ use crate::semantic_index::NseTiming; /// eponymous function). #[derive(Debug, Clone, Copy, Default)] pub struct Effects { - pub nse: Option, + pub nse: Option, } impl Effects { - pub fn nse(nse: NseAnnotation) -> Self { + pub fn nse(nse: ArgumentsAnnotation) -> Self { Self { nse: Some(nse) } } } /// Annotation describing how an NSE function's arguments create scopes. #[derive(Debug, Clone, Copy)] -pub struct NseAnnotation { - pub arguments: &'static [NseArgument], +pub struct ArgumentsAnnotation { + pub arguments: &'static [Argument], } /// A single argument that creates an NSE scope. #[derive(Debug)] -pub struct NseArgument { +pub struct Argument { pub name: &'static str, pub position: usize, pub scope: NseScope, diff --git a/crates/oak_semantic/src/effects_registry.rs b/crates/oak_semantic/src/effects_registry.rs index efdada24ec..0beb0cb635 100644 --- a/crates/oak_semantic/src/effects_registry.rs +++ b/crates/oak_semantic/src/effects_registry.rs @@ -1,5 +1,5 @@ -use crate::effects::NseAnnotation; -use crate::effects::NseArgument; +use crate::effects::Argument; +use crate::effects::ArgumentsAnnotation; use crate::semantic_index::NseScope::Current; use crate::semantic_index::NseScope::Nested; use crate::semantic_index::NseTiming::Eager; @@ -8,11 +8,11 @@ use crate::semantic_index::NseTiming::Lazy; struct Entry { package: &'static str, function: &'static str, - annotation: NseAnnotation, + annotation: ArgumentsAnnotation, } /// Look up the NSE annotation for a `(package, function)` pair. -pub fn lookup(package: &str, function: &str) -> Option<&'static NseAnnotation> { +pub fn lookup(package: &str, function: &str) -> Option<&'static ArgumentsAnnotation> { REGISTRY .iter() .find(|e| e.package == package && e.function == function) @@ -36,8 +36,8 @@ macro_rules! entry { Entry { package: $pkg, function: $func, - annotation: NseAnnotation { - arguments: &[$(NseArgument { + annotation: ArgumentsAnnotation { + arguments: &[$(Argument { name: $name, position: $pos, scope: $scope, From db194842762c399d650c0796687ddbd6ab8a31df Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 17 Jul 2026 14:05:38 +0200 Subject: [PATCH 07/12] Add module-level doc about the walks --- crates/oak_semantic/src/builder.rs | 36 +++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index 2717029c15..a9a5616f13 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -1,3 +1,30 @@ +//! Builds the [`SemanticIndex`] for one R file. +//! +//! The builder splits work by "scan unit": the file or a lazy body (a function, +//! a lazy NSE body like `reactive()`). A unit is coarser than a scope. An eager +//! scope nested inside it, like `local({ ... })`, is part of the same scan unit, +//! while a lazy body starts a new one. +//! +//! Each scan unit is built in two passes: a scan, then a walk. The walk is the +//! pass that writes the arenas (scopes, symbols, definitions, uses, use-def +//! maps). It can only write them correctly if it already knows two things about +//! the scope it's in, and neither is knowable at its own cursor: +//! +//! - Which calls are NSE, so it can push the scope for `local({ ... })` inline +//! as it reaches the call. That turns on whether the callee is shadowed at +//! that point in the flow. +//! +//! - The complete set of names the scope binds, so it can resolve a nested +//! scope's free variable to an ancestor binding. A lazy body (a function, a +//! `reactive()`) can reference a definition the ancestor's own walk hasn't +//! reached yet. That ancestor lookup is what the walk records as an enclosing +//! 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. + use std::sync::Arc; use aether_syntax::AnyRArgumentName; @@ -61,12 +88,9 @@ mod builder_nse; /// information supplied by `resolver`. See [`ImportsResolver`] for the /// available impls. /// -/// Each scope is built in two local phases. First a scan pass over the -/// scope's direct level decides which calls are NSE, in flow order, and -/// collects the scope's bound names (see [`scan_expression`]). Then the walk -/// reuses those decisions and pushes NSE scopes inline as it reaches them -/// ([`collect_expression`]). Walking `local({...})` inline means a later call -/// sees the scope-push in the same pass, so there is no whole-file re-walk. +/// See the module docs for the scan/walk split. The scan +/// ([`scan_expression`]) runs first over each scope, then the walk +/// ([`collect_expression`]) reuses its decisions and pushes NSE scopes inline. /// /// [`scan_expression`]: SemanticIndexBuilder::scan_expression /// [`collect_expression`]: SemanticIndexBuilder::collect_expression From 923e5e6cafadd3f2b3161602b48ae62008e61487 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 17 Jul 2026 14:47:08 +0200 Subject: [PATCH 08/12] Revise naming of scan state fields --- crates/oak_semantic/src/builder.rs | 120 ++++++++++++------ .../oak_semantic/src/builder/builder_nse.rs | 30 ++--- 2 files changed, 98 insertions(+), 52 deletions(-) diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index a9a5616f13..347a537e4f 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -122,21 +122,19 @@ struct SemanticIndexBuilder { // Per-call facts resolved by the scanner in flow order, keyed by the call's // range. See `CallResolution`. call_resolutions: FxHashMap, - // Names bound so far in the scope currently being scanned, tracked - // flow-precisely (if/else restore, loop union). This is the scan - // pass's own flow state, standing in for the walk's use-def state which - // isn't built yet. Reset at each scope's `begin_scan()`. - bound_so_far: FxHashSet, + // Diagnostics collected during the build and logged on `finish()`. A minimal + // channel for now, no user-facing surface. + diagnostics: Vec, + // The scan's flow-precise binding state for the scope being scanned, reset + // at each scope's `begin_scan()`. See [`FlowState`]. + flow_state: FlowState, // Names inherited from enclosing scopes at this scope's entry point, keyed - // by the scope's range. Captured from `bound_so_far`, and read by + // by the scope's range. Captured from `flow_state`, and read by // `begin_scan()` to seed the scope's own scan. - inherited_at_entry: FxHashMap>, + enclosing_flow: FxHashMap, // Bound names of Eager + Nested bodies like `local()` are discovered inline // by the scanner. See `EagerNestedDescent`. - descent: EagerNestedDescent, - // Diagnostics collected during the build and logged on `finish()`. A minimal - // channel for now, no user-facing surface. - diagnostics: Vec, + eager_descent: EagerNestedDescent, } impl SemanticIndexBuilder { @@ -179,9 +177,9 @@ impl SemanticIndexBuilder { semantic_calls: Vec::new(), namespace_accesses: Vec::new(), call_resolutions: FxHashMap::default(), - bound_so_far: FxHashSet::default(), - inherited_at_entry: FxHashMap::default(), - descent: EagerNestedDescent::default(), + flow_state: FlowState::default(), + enclosing_flow: FxHashMap::default(), + eager_descent: EagerNestedDescent::default(), diagnostics: Vec::new(), resolver, } @@ -472,13 +470,13 @@ impl SemanticIndexBuilder { /// Record the names 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 + /// in `begin_scan`. Called during the scan, where `flow_state` 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). - pub(super) fn record_inherited_at_entry(&mut self, range: TextRange) { - self.inherited_at_entry - .insert(range, self.bound_so_far.clone()); + pub(super) fn record_enclosing_flow(&mut self, range: TextRange) { + self.enclosing_flow + .insert(range, self.flow_state.snapshot()); } // --- Scan pass --- @@ -488,7 +486,7 @@ impl SemanticIndexBuilder { /// Seeds it with two things: /// /// - The names inherited from enclosing scopes, captured when this scope was - /// entered (`inherited_at_entry`). The parent's own scan was seeded the same + /// entered (`enclosing_flow`). The parent's own scan was seeded the same /// way, so this is transitively complete: it holds every eager binding /// visible from an ancestor at this scope's definition point. /// - The scope's own already-bound names. For a function scope that's the @@ -499,16 +497,16 @@ impl SemanticIndexBuilder { /// are recorded, so `collect_function` seeds the full formal set by hand /// (all formals bind at once in R, so a default sees every parameter name). pub(super) fn begin_scan(&mut self) { - self.bound_so_far.clear(); - let range = self.scopes[self.current_scope].range; - if let Some(entry) = self.inherited_at_entry.get(&range) { - self.bound_so_far.extend(entry.iter().cloned()); + + match self.enclosing_flow.get(&range).cloned() { + Some(entry) => self.flow_state.restore(entry), + None => self.flow_state.clear(), } for (_id, symbol) in self.symbol_tables[self.current_scope].iter() { if symbol.flags().contains(SymbolFlags::IS_BOUND) { - self.bound_so_far.insert(symbol.name().to_string()); + self.flow_state.bind(symbol.name().to_string()); } } } @@ -536,7 +534,7 @@ impl SemanticIndexBuilder { /// - A `Current + Eager` body pushes no scope, so it stays part of this /// scope's direct level and is scanned through transparently. /// - A `Nested + Eager` body is descended into with a save/restore of - /// `bound_so_far`, and the names it binds are left pending for the walk to + /// `flow_state`, and the names it binds are left pending for the walk to /// install without re-scanning. /// - Function and lazy bodies (`Nested + Lazy`, `Current + Lazy`) are their /// own scan units, scanned separately when the walk enters them, because @@ -551,8 +549,8 @@ impl SemanticIndexBuilder { // A function body is a child scope, scanned when it's entered. // Record the names it inherits now so that when we later resolve // an NSE callee inside the body, we can check whether one of them - // shadows it (see `inherited_at_entry`). - self.record_inherited_at_entry(func.syntax().text_trimmed_range()); + // shadows it (see `enclosing_flow`). + self.record_enclosing_flow(func.syntax().text_trimmed_range()); }, AnyRExpression::RBracedExpressions(braced) => { @@ -624,13 +622,14 @@ impl SemanticIndexBuilder { self.scan_expression(&condition); } - let pre_if = self.bound_so_far.clone(); + let pre_if = self.flow_state.snapshot(); if let Ok(consequence) = stmt.consequence() { self.scan_expression(&consequence); } - let post_if = std::mem::replace(&mut self.bound_so_far, pre_if); + let post_if = self.flow_state.snapshot(); + self.flow_state.restore(pre_if); if let Some(else_clause) = stmt.else_clause() { if let Ok(alternative) = else_clause.alternative() { @@ -639,7 +638,7 @@ impl SemanticIndexBuilder { } // Both branches' bindings are live afterwards. - self.bound_so_far.extend(post_if); + self.flow_state.merge(post_if); }, // `while`/`repeat` loops, subsets, extractions, parentheses, unary @@ -671,7 +670,7 @@ impl SemanticIndexBuilder { } fn scan_parameter_defaults(&mut self, params: &RParameters) { - // Seed `bound_so_far` with every parameter names so a callee inside a + // Seed `flow_state` with every parameter names so a callee inside a // default value sees the full formal set for param in params.items().iter() { let Ok(param) = param else { continue }; @@ -681,7 +680,7 @@ impl SemanticIndexBuilder { AnyRParameterName::RDots(_) => String::from("..."), AnyRParameterName::RDotDotI(ddi) => ddi.syntax().text_trimmed().to_string(), }; - self.bound_so_far.insert(text); + self.flow_state.bind(text); } for param in params.items().iter() { @@ -738,14 +737,14 @@ impl SemanticIndexBuilder { /// Record a binding in the scan's flow state. /// - /// The flow-precise `bound_so_far` set always learns the name, so a + /// The flow-precise `flow_state` always learns the name, so a /// later callee in this scope sees it shadowed. The bound names only get it /// when the current scope owns it. A `Current + Lazy` scope routes its defs /// to the owner, so the name is added to the owner's bound names instead, the /// same routing `add_definition_to_owner` does during the walk. fn record_binding(&mut self, name: String) { self.record_owner_name(name.clone()); - self.bound_so_far.insert(name); + self.flow_state.bind(name); } /// Route a binding NAME into its owner scope's bound names, matching @@ -756,9 +755,9 @@ impl SemanticIndexBuilder { /// /// Split from `record_binding` so `scan_lazy_owner_bindings` can add /// a deferred body's names to the owner's bound names without also marking them - /// bound in `bound_so_far` (see that helper for why). + /// bound in `flow_state` (see that helper for why). fn record_owner_name(&mut self, name: String) { - if let Some(bound) = self.descent.open.last_mut() { + if let Some(bound) = self.eager_descent.open.last_mut() { bound.add(name); return; } @@ -1020,7 +1019,7 @@ impl SemanticIndexBuilder { // Scan the default values before collecting them. R binds all // formals into the frame at once, so a default sees every parameter // name regardless of position: `function(local, b = local(...))` is - // not NSE. So we seed the whole formal set into `bound_so_far` + // not NSE. So we seed the whole formal set into `flow_state` // up front rather than flow-ordered, then scan each default. self.begin_scan(); self.scan_parameter_defaults(¶ms); @@ -1404,6 +1403,53 @@ struct CallResolution { source: Option, } +/// The scan's flow-precise binding state: which names are bound at the current +/// point of the current scan unit, in flow order. +/// +/// 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. +#[derive(Clone, Default)] +struct FlowState { + bound: FxHashSet, +} + +impl FlowState { + /// Save the current state, to rewind to or to seed a child scan unit from. + fn snapshot(&self) -> FlowState { + self.clone() + } + + /// Rewind to `snapshot`, dropping any bindings recorded since it was taken. + fn restore(&mut self, snapshot: 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); + } + + /// Record `name` as bound from here on. + fn bind(&mut self, name: String) { + self.bound.insert(name); + } + + /// Whether `name` is bound at the current point. + fn is_bound(&self, name: &str) -> bool { + self.bound.contains(name) + } + + /// Drop all bindings, to start a fresh scan unit (see `begin_scan()`). + fn clear(&mut self) { + self.bound.clear(); + } +} + /// Tracks eager `Nested` NSE bodies scanned inline during the scan. /// /// An eager `Nested` body like `local()` runs immediately at its call site, so diff --git a/crates/oak_semantic/src/builder/builder_nse.rs b/crates/oak_semantic/src/builder/builder_nse.rs index 636a0b1323..2a6bedc8ed 100644 --- a/crates/oak_semantic/src/builder/builder_nse.rs +++ b/crates/oak_semantic/src/builder/builder_nse.rs @@ -81,29 +81,29 @@ impl SemanticIndexBuilder { // scope. But we do grab the names it defines now, so the // owner's bound names are complete before the walk reaches a sibling. (NseScope::Current, NseTiming::Lazy) => { - self.record_inherited_at_entry(value.syntax().text_trimmed_range()); + self.record_enclosing_flow(value.syntax().text_trimmed_range()); self.scan_lazy_owner_bindings(&value); }, // Calls like `local()`. Its body runs eagerly at the call - // site, so its environment IS the current `bound_so_far`. + // site, so its environment IS the current `flow_state`. // Descend now, holding the names bound in this scope as - // pending so the walk has access to them. No `bound_so_far` + // pending so the walk has access to them. No `flow_state` // reset: the child sees exactly what `begin_scan()` would // have seeded. - // No `record_inherited_at_entry()`: eager `Nested` bodies are + // No `record_enclosing_flow()`: eager `Nested` bodies are // never scanned at walk time, so nothing would read it. (NseScope::Nested, NseTiming::Eager) => { - let old = self.bound_so_far.clone(); + let old = self.flow_state.snapshot(); let range = value.syntax().text_trimmed_range(); - self.descent.open.push(BoundNames::new()); + self.eager_descent.open.push(BoundNames::new()); self.scan_expression(&value); - if let Some(bound) = self.descent.open.pop() { - self.descent.pending.insert(range, bound); + if let Some(bound) = self.eager_descent.open.pop() { + self.eager_descent.pending.insert(range, bound); } - self.bound_so_far = old; + self.flow_state.restore(old); }, // Calls like `reactive()`. Its body runs at an unknown @@ -111,7 +111,7 @@ impl SemanticIndexBuilder { // enters it. Record the names it inherits for its callee // resolution, same as a function body. (NseScope::Nested, NseTiming::Lazy) => { - self.record_inherited_at_entry(value.syntax().text_trimmed_range()); + self.record_enclosing_flow(value.syntax().text_trimmed_range()); }, }, } @@ -143,7 +143,7 @@ impl SemanticIndexBuilder { /// variable can't resolve to it. TODO(nse): We could potentially walk /// transparent (Current) nested calls to collect those too. /// - /// The names go to `bound_names` only, never to `bound_so_far`. The body + /// The names go to `bound_names` only, never to `flow_state`. The body /// runs at some later time, so at an eager position after the call these /// names aren't bound yet, and an eager callee there must still treat them /// as unbound. @@ -236,13 +236,13 @@ impl SemanticIndexBuilder { // First check for a local definition (which in the future may // contain NSE annotations that we resolve here) // - // Looked up from `bound_so_far` which already carries every + // Looked up from `flow_state` 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` + // bindings are excluded. A forward one isn't in `flow_state` // yet, and a deferred one (`on_load`, `<<-`) never enters it. - if self.bound_so_far.contains(&name) { + if self.flow_state.is_bound(&name) { return self .resolve_local_effects(&name) .and_then(|effects| effects.nse); @@ -421,7 +421,7 @@ impl SemanticIndexBuilder { // Install the pending names the descent recorded for this body, // before collecting so lazy children inside can see them via // `scope_binds_anywhere()`. - match self.descent.pending.remove(&range) { + match self.eager_descent.pending.remove(&range) { Some(bound) => self.bound_names[scope] = bound, None => { // An eager NSE scope is reachable only through the scan From 230e9ce8fce74fa60c5a3f1fc14a50c90be6e877 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 22 Jul 2026 14:31:54 +0200 Subject: [PATCH 09/12] Record overwrite site for lazy-shadow diagnostics --- crates/oak_semantic/src/builder.rs | 100 +++++++++++++----- .../oak_semantic/src/builder/builder_nse.rs | 42 ++++++-- crates/oak_semantic/src/semantic_index.rs | 13 ++- .../tests/integration/builder_nse.rs | 17 ++- 4 files changed, 126 insertions(+), 46 deletions(-) diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index 347a537e4f..5bd3af1f31 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -77,6 +77,7 @@ use crate::semantic_index::SemanticCallKind; use crate::semantic_index::SemanticDiagnostic; use crate::semantic_index::SemanticIndex; use crate::semantic_index::SymbolFlags; +use crate::semantic_index::SymbolId; use crate::semantic_index::SymbolTableBuilder; use crate::semantic_index::Use; use crate::semantic_index::UseId; @@ -459,13 +460,39 @@ impl SemanticIndexBuilder { /// already-recorded `IS_BOUND` definition or a pre-scanned assignment. The /// pre-scan covers definitions the walk hasn't reached yet in this scope. fn scope_binds_anywhere(&self, scope: ScopeId, name: &str) -> bool { - let found_by_flag = self.symbol_tables[scope].id(name).is_some_and(|sym_id| { - self.symbol_tables[scope] - .symbol(sym_id) - .flags() - .contains(SymbolFlags::IS_BOUND) - }); - found_by_flag || self.bound_names[scope].binds(name) + self.walked_binding(scope, name).is_some() || self.bound_names[scope].binds(name) + } + + /// The site where `scope` binds `name`, matching what + /// [`scope_binds_anywhere`](Self::scope_binds_anywhere) counts as a binding + /// (so it returns `Some` on exactly the same names). Prefers the + /// scan-collected site in `bound_names`, falling back to the range of an + /// already-walked `IS_BOUND` definition (e.g. a parameter, which the scan + /// seeds straight into `flow_state` without a `bound_names` entry). Used to + /// point the lazy-shadow diagnostic at the overwrite. + fn scope_binding_range(&self, scope: ScopeId, name: &str) -> Option { + if let Some(range) = self.bound_names[scope].binding_range(name) { + return Some(range); + } + + // `IS_BOUND` always has a matching `Definition` row (see the invariant + // in `resolve_symbol()`), so the find never misses when the flag is set. + let sym_id = self.walked_binding(scope, name)?; + self.definitions[scope] + .iter() + .find(|(_id, def)| def.symbol == sym_id) + .map(|(_id, def)| def.range) + } + + /// The symbol `name` interns to in `scope`, if the walk has already recorded + /// an `IS_BOUND` definition for it. + fn walked_binding(&self, scope: ScopeId, name: &str) -> Option { + let sym_id = self.symbol_tables[scope].id(name)?; + self.symbol_tables[scope] + .symbol(sym_id) + .flags() + .contains(SymbolFlags::IS_BOUND) + .then_some(sym_id) } /// Record the names a child scope (function body, NSE argument) about to be @@ -573,8 +600,8 @@ impl SemanticIndexBuilder { match assignment_name(&target) { // `<<-` binds in an ancestor, not here, so it doesn't // shadow a callee in this scope (matching the walk). - Some((name, _)) if !is_super_assignment(bin) => { - self.record_binding(name); + Some((name, range)) if !is_super_assignment(bin) => { + self.record_binding(name, range); }, Some(_) => {}, // Complex target (`x$foo <- v`): no binding, but the @@ -604,7 +631,10 @@ impl SemanticIndexBuilder { // The for-variable is always bound (R sets it to NULL for empty // sequences), so it binds before the body regardless of flow. if let Ok(variable) = stmt.variable() { - self.record_binding(variable.name_text()); + self.record_binding( + variable.name_text(), + variable.syntax().text_trimmed_range(), + ); } if let Ok(sequence) = stmt.sequence() { self.scan_expression(&sequence); @@ -725,14 +755,14 @@ impl SemanticIndexBuilder { return; }; + // Sourced names originate in another file, so they have no binding site + // here. Anchor the overwrite range at the `source()` call instead. + let range = call.syntax().text_trimmed_range(); for name in &resolution.names { - self.record_binding(name.clone()); + self.record_binding(name.clone(), range); } - self.call_resolutions - .entry(call.syntax().text_trimmed_range()) - .or_default() - .source = Some(resolution); + self.call_resolutions.entry(range).or_default().source = Some(resolution); } /// Record a binding in the scan's flow state. @@ -742,8 +772,8 @@ impl SemanticIndexBuilder { /// when the current scope owns it. A `Current + Lazy` scope routes its defs /// to the owner, so the name is added to the owner's bound names instead, the /// same routing `add_definition_to_owner` does during the walk. - fn record_binding(&mut self, name: String) { - self.record_owner_name(name.clone()); + fn record_binding(&mut self, name: String, range: TextRange) { + self.record_owner_name(name.clone(), range); self.flow_state.bind(name); } @@ -756,9 +786,9 @@ impl SemanticIndexBuilder { /// Split from `record_binding` so `scan_lazy_owner_bindings` can add /// a deferred body's names to the owner's bound names without also marking them /// bound in `flow_state` (see that helper for why). - fn record_owner_name(&mut self, name: String) { + fn record_owner_name(&mut self, name: String, range: TextRange) { if let Some(bound) = self.eager_descent.open.last_mut() { - bound.add(name); + bound.add(name, range); return; } @@ -766,7 +796,7 @@ impl SemanticIndexBuilder { ScopeKind::Nse(NseScope::Current, NseTiming::Lazy) => self.definition_owner(), _ => Some(self.current_scope), } { - self.bound_names[target].add(name); + self.bound_names[target].add(name, range); } } @@ -1350,9 +1380,14 @@ impl SemanticIndexBuilder { // TODO(diagnostics): Diagnostics are not surfaced yet, so log them for now for diagnostic in &self.diagnostics { match diagnostic { - SemanticDiagnostic::LazyShadowAmbiguity { name, range } => log::warn!( - "NSE lazy-shadow ambiguity: callee `{name}` at {range:?} is recognized \ - as NSE, but a lazy-crossed ancestor binds it with undetermined timing" + SemanticDiagnostic::LazyShadowAmbiguity { + name, + call_range, + overwrite_range, + } => log::warn!( + "NSE lazy-shadow ambiguity: callee `{name}` at {call_range:?} is recognized \ + as NSE, but a lazy-crossed ancestor binds it at {overwrite_range:?} with \ + undetermined timing" ), } } @@ -1481,23 +1516,32 @@ struct EagerNestedDescent { /// All definitions in a scope, collected by the scan pass before the /// walk. Skips child-scope bodies (nested functions and `Nested` NSE bodies). +/// +/// Keeps each name's earliest binding site in scan order, which is source +/// order within the scope. A name bound several times reads as bound +/// throughout, and this earliest site is what the lazy-shadow diagnostic points +/// at, once `is_lazily_shadowed` has picked the nearest binding ancestor. struct BoundNames { - by_name: FxHashSet, + by_name: FxHashMap, } impl BoundNames { fn new() -> Self { Self { - by_name: FxHashSet::default(), + by_name: FxHashMap::default(), } } - fn add(&mut self, name: String) { - self.by_name.insert(name); + fn add(&mut self, name: String, range: TextRange) { + self.by_name.entry(name).or_insert(range); } fn binds(&self, name: &str) -> bool { - self.by_name.contains(name) + self.by_name.contains_key(name) + } + + fn binding_range(&self, name: &str) -> Option { + self.by_name.get(name).copied() } } diff --git a/crates/oak_semantic/src/builder/builder_nse.rs b/crates/oak_semantic/src/builder/builder_nse.rs index 2a6bedc8ed..daa6ad85bc 100644 --- a/crates/oak_semantic/src/builder/builder_nse.rs +++ b/crates/oak_semantic/src/builder/builder_nse.rs @@ -167,8 +167,8 @@ impl SemanticIndexBuilder { bin.left() }; if let Ok(target) = target { - if let Some((name, _)) = assignment_name(&target) { - self.record_owner_name(name); + if let Some((name, range)) = assignment_name(&target) { + self.record_owner_name(name, range); } } }, @@ -186,7 +186,10 @@ impl SemanticIndexBuilder { AnyRExpression::RForStatement(stmt) => { if let Ok(variable) = stmt.variable() { - self.record_owner_name(variable.name_text()); + self.record_owner_name( + variable.name_text(), + variable.syntax().text_trimmed_range(), + ); } if let Ok(body) = stmt.body() { self.scan_lazy_owner_bindings(&body); @@ -268,8 +271,12 @@ impl SemanticIndexBuilder { // 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. - if self.is_lazily_shadowed(&name) { - self.record_lazy_shadow_ambiguity(name, call.syntax().text_trimmed_range()); + if let Some(overwrite_range) = self.is_lazily_shadowed(&name) { + self.record_lazy_shadow_ambiguity( + name, + call.syntax().text_trimmed_range(), + overwrite_range, + ); } Some(nse) }, @@ -308,13 +315,17 @@ impl SemanticIndexBuilder { /// scope may bind `name` with a timing we can't pin down, either a later /// assignment, or one from another deferred body that could run before or /// after us We detect this ambiguity here so it can be linted. - fn is_lazily_shadowed(&self, name: &str) -> bool { + /// + /// Returns the site of the shadowing binding. + fn is_lazily_shadowed(&self, name: &str) -> Option { let mut scope = self.current_scope; let mut crossed_lazy = self.scopes[scope].kind.is_lazy(); while let Some(parent) = self.scopes[scope].parent { - if crossed_lazy && self.scope_binds_anywhere(parent, name) { - return true; + if crossed_lazy { + if let Some(range) = self.scope_binding_range(parent, name) { + return Some(range); + } } if self.scopes[parent].kind.is_lazy() { @@ -323,12 +334,21 @@ impl SemanticIndexBuilder { scope = parent; } - false + None } - fn record_lazy_shadow_ambiguity(&mut self, name: String, range: TextRange) { + fn record_lazy_shadow_ambiguity( + &mut self, + name: String, + call_range: TextRange, + overwrite_range: TextRange, + ) { self.diagnostics - .push(SemanticDiagnostic::LazyShadowAmbiguity { name, range }); + .push(SemanticDiagnostic::LazyShadowAmbiguity { + name, + call_range, + overwrite_range, + }); } /// Process a call the scan pass decided is NSE. Match its arguments diff --git a/crates/oak_semantic/src/semantic_index.rs b/crates/oak_semantic/src/semantic_index.rs index 209f686379..196e94d45c 100644 --- a/crates/oak_semantic/src/semantic_index.rs +++ b/crates/oak_semantic/src/semantic_index.rs @@ -809,9 +809,16 @@ pub enum NamespaceAccessKind { /// consumers to turn into user-facing diagnostics. #[derive(Debug, Clone, PartialEq, Eq)] pub enum SemanticDiagnostic { - /// An NSE call recognized in a lazy context whose binding is overwritten - /// later on (in subsequent parent code or in another lazy context). - LazyShadowAmbiguity { name: String, range: TextRange }, + /// An NSE call recognized in a lazy context whose callee is also bound by a + /// lazy-crossed ancestor with undetermined timing, so the NSE decision is a + /// guess. `call_range` points at the NSE 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 { + name: String, + call_range: TextRange, + overwrite_range: TextRange, + }, } // --- Iterators --- diff --git a/crates/oak_semantic/tests/integration/builder_nse.rs b/crates/oak_semantic/tests/integration/builder_nse.rs index 30559e99ed..50eab4218c 100644 --- a/crates/oak_semantic/tests/integration/builder_nse.rs +++ b/crates/oak_semantic/tests/integration/builder_nse.rs @@ -1209,11 +1209,20 @@ local <- identity let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::LazyShadowAmbiguity { name, range } => { + SemanticDiagnostic::LazyShadowAmbiguity { + name, + call_range, + overwrite_range, + } => { assert_eq!(name, "local"); - let start = u32::from(range.start()) as usize; - let end = u32::from(range.end()) as usize; - assert_eq!(&source[start..end], "local({ x <- 1 })"); + + 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({ x <- 1 })"); + + let overwrite_start = u32::from(overwrite_range.start()) as usize; + let overwrite_end = u32::from(overwrite_range.end()) as usize; + assert_eq!(&source[overwrite_start..overwrite_end], "local"); }, } } From e44b1a8bda78237fff8d166c0d86493dced788e7 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 22 Jul 2026 14:41:50 +0200 Subject: [PATCH 10/12] Address code review --- crates/oak_semantic/src/semantic_index.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/oak_semantic/src/semantic_index.rs b/crates/oak_semantic/src/semantic_index.rs index 196e94d45c..9b1217f3cf 100644 --- a/crates/oak_semantic/src/semantic_index.rs +++ b/crates/oak_semantic/src/semantic_index.rs @@ -430,7 +430,12 @@ pub enum NseScope { /// (at an unknown later time). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum NseTiming { + /// Expression that runs at the call site. Free variables resolve against + /// the linear state right there. E.g. `local()`, `evalq()`, `test_that()`. Eager, + /// Expression that runs at an unknown later time, so free variables resolve + /// against the accumulated union of enclosing definitions. E.g. + /// `shiny::reactive()`, `rlang::on_load()`. Lazy, } @@ -443,7 +448,7 @@ impl ScopeKind { match self { ScopeKind::File => false, ScopeKind::Function => true, - ScopeKind::Nse(_, laziness) => laziness == NseTiming::Lazy, + ScopeKind::Nse(_, timing) => timing == NseTiming::Lazy, } } } From d01275fe1b3629ddb72821f1c1086cc88201dede Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 22 Jul 2026 15:03:34 +0200 Subject: [PATCH 11/12] Rename watchers for clarity --- crates/oak_semantic/src/use_def_map.rs | 35 +++++++++++++------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/crates/oak_semantic/src/use_def_map.rs b/crates/oak_semantic/src/use_def_map.rs index 890dc3835f..c2e9f6da2a 100644 --- a/crates/oak_semantic/src/use_def_map.rs +++ b/crates/oak_semantic/src/use_def_map.rs @@ -306,11 +306,13 @@ pub(crate) struct UseDefMapBuilder { // Currently used for `<<-` extra definitions in ancestor scopes. deferred_defs: Vec<(SymbolId, DefinitionId)>, enclosing_snapshots: IndexVec, - // Snapshots subscribed to every definition of a symbol (lazy snapshots). - def_watchers: FxHashMap>, - // Snapshots subscribed only to deferred (`<<-`) definitions of a symbol - // (eager snapshots). - deferred_def_watchers: FxHashMap>, + // Lazy snapshots (e.g. `reactive()`), notified of every definition of a + // symbol so they fold in the whole union. + lazy_watchers: FxHashMap>, + // Eager snapshots (e.g. `local()`), notified only of deferred (`<<-`) + // definitions. A point-in-time capture ignores later plain defs, but a + // `<<-` mutates the binding while the eager body runs, so it must fold in. + eager_watchers: FxHashMap>, } impl UseDefMapBuilder { @@ -320,8 +322,8 @@ impl UseDefMapBuilder { bindings_by_use: IndexVec::new(), deferred_defs: Vec::new(), enclosing_snapshots: IndexVec::new(), - def_watchers: FxHashMap::default(), - deferred_def_watchers: FxHashMap::default(), + lazy_watchers: FxHashMap::default(), + eager_watchers: FxHashMap::default(), } } @@ -340,9 +342,11 @@ impl UseDefMapBuilder { /// live definitions for that symbol. pub(crate) fn record_definition(&mut self, symbol_id: SymbolId, def_id: DefinitionId) { self.symbol_states[symbol_id].record_definition(def_id); + // A plain def reaches lazy snapshots only. Eager snapshots are + // point-in-time and ignore defs that land after the call. Self::notify_watchers( &mut self.enclosing_snapshots, - &self.def_watchers, + &self.lazy_watchers, symbol_id, def_id, ); @@ -408,17 +412,17 @@ impl UseDefMapBuilder { pub(crate) fn record_deferred_definition(&mut self, symbol_id: SymbolId, def_id: DefinitionId) { self.symbol_states[symbol_id].add_definition(def_id); self.deferred_defs.push((symbol_id, def_id)); - // A deferred def reaches both channels: lazy snapshots (like any def) - // and eager snapshots (they subscribe to deferred defs only). + // A deferred def reaches both watcher sets: lazy snapshots (like any + // def) and eager snapshots (which take deferred defs only). Self::notify_watchers( &mut self.enclosing_snapshots, - &self.def_watchers, + &self.lazy_watchers, symbol_id, def_id, ); Self::notify_watchers( &mut self.enclosing_snapshots, - &self.deferred_def_watchers, + &self.eager_watchers, symbol_id, def_id, ); @@ -486,7 +490,7 @@ impl UseDefMapBuilder { pub(crate) fn register_lazy_snapshot(&mut self, symbol_id: SymbolId) -> EnclosingSnapshotId { let bindings = self.symbol_states[symbol_id].clone(); let id = self.enclosing_snapshots.push(bindings); - self.def_watchers.entry(symbol_id).or_default().push(id); + self.lazy_watchers.entry(symbol_id).or_default().push(id); id } @@ -529,10 +533,7 @@ impl UseDefMapBuilder { pub(crate) fn register_eager_snapshot(&mut self, symbol_id: SymbolId) -> EnclosingSnapshotId { let bindings = self.symbol_states[symbol_id].clone(); let id = self.enclosing_snapshots.push(bindings); - self.deferred_def_watchers - .entry(symbol_id) - .or_default() - .push(id); + self.eager_watchers.entry(symbol_id).or_default().push(id); id } From 66397f3b22dab1c27c362256076c28d8b09e76e1 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 22 Jul 2026 16:44:37 +0200 Subject: [PATCH 12/12] Refactor eager snapshot to be per-use --- crates/oak_db/src/tests/resolver.rs | 6 +- crates/oak_semantic/src/builder.rs | 50 ++++++--- crates/oak_semantic/src/semantic_index.rs | 27 +++-- crates/oak_semantic/src/use_def_map.rs | 61 ++++------ .../tests/integration/builder_nse.rs | 106 ++++++++---------- .../tests/integration/use_def_map.rs | 96 ++++------------ 6 files changed, 142 insertions(+), 204 deletions(-) diff --git a/crates/oak_db/src/tests/resolver.rs b/crates/oak_db/src/tests/resolver.rs index 8d7b416884..b0ba0ca8be 100644 --- a/crates/oak_db/src/tests/resolver.rs +++ b/crates/oak_db/src/tests/resolver.rs @@ -198,9 +198,8 @@ fn test_closure_capture_with_source_before_function() { let bindings = fn_map.bindings_at_use(use_id); assert!(bindings.may_be_unbound()); - let symbol = index.uses(fn_scope)[use_id].symbol(); let (enclosing_scope, enclosing_bindings) = index - .enclosing_bindings(fn_scope, symbol) + .enclosing_bindings(fn_scope, use_id) .expect("`helper` should have an enclosing snapshot at the file scope"); assert_eq!(enclosing_scope, file_scope); assert!(!enclosing_bindings.definitions().is_empty()); @@ -321,8 +320,7 @@ fn test_closure_capture_with_source_after_function() { let fn_scope = ScopeId::from(1); let use_id = oak_semantic::UseId::from(0); - let symbol = index.uses(fn_scope)[use_id].symbol(); - assert!(index.enclosing_bindings(fn_scope, symbol).is_some()); + assert!(index.enclosing_bindings(fn_scope, use_id).is_some()); } #[test] diff --git a/crates/oak_semantic/src/builder.rs b/crates/oak_semantic/src/builder.rs index 5bd3af1f31..4a297802a2 100644 --- a/crates/oak_semantic/src/builder.rs +++ b/crates/oak_semantic/src/builder.rs @@ -118,6 +118,9 @@ struct SemanticIndexBuilder { current_scope: ScopeId, bound_names: IndexVec, enclosing_snapshots: FxHashMap, + // Snapshots shared across every use of a free variable in lazy contexts, + // keyed by (nested scope, nested symbol). + lazy_snapshots: FxHashMap<(ScopeId, SymbolId), (ScopeId, EnclosingSnapshotId)>, semantic_calls: Vec, namespace_accesses: Vec, // Per-call facts resolved by the scanner in flow order, keyed by the call's @@ -175,6 +178,7 @@ impl SemanticIndexBuilder { current_scope: file_scope, bound_names, enclosing_snapshots: FxHashMap::default(), + lazy_snapshots: FxHashMap::default(), semantic_calls: Vec::new(), namespace_accesses: Vec::new(), call_resolutions: FxHashMap::default(), @@ -386,15 +390,11 @@ impl SemanticIndexBuilder { // Associate free variables with the enclosing snapshot where the // variable is defined if self.use_def_maps[self.current_scope].is_may_be_unbound(symbol_id) { - let use_key = EnclosingSnapshotKey { - nested_scope: self.current_scope, - nested_symbol: symbol_id, - }; - self.register_enclosing_snapshot(name, use_key); + self.register_enclosing_snapshot(name, symbol_id, use_id); } } - fn register_enclosing_snapshot(&mut self, name: &str, use_key: EnclosingSnapshotKey) { + fn register_enclosing_snapshot(&mut self, name: &str, nested_symbol: SymbolId, use_id: UseId) { // We're looking for a parent definition for this scope's free variable // so start from parent let Some(mut current_scope) = self.scopes[self.current_scope].parent else { @@ -427,20 +427,38 @@ impl SemanticIndexBuilder { // `add_definition()` call during the full walk will set `IS_BOUND`. let enclosing_symbol_id = self.symbol_tables[current_scope].intern(name, SymbolFlags::empty()); - - if self.enclosing_snapshots.contains_key(&use_key) { - return; - } - self.use_def_maps[current_scope].ensure_symbol(enclosing_symbol_id); - let snapshot_id = if all_eager { - self.use_def_maps[current_scope].register_eager_snapshot(enclosing_symbol_id) + let entry = if all_eager { + // Eager: a fresh point-in-time snapshot per use, no dedup and + // no watcher. Two uses at different points in the body can + // capture different enclosing states (e.g. either side of a + // `<<-`), so they can't share. + let snapshot_id = self.use_def_maps[current_scope] + .register_eager_snapshot(enclosing_symbol_id); + (current_scope, snapshot_id) } else { - self.use_def_maps[current_scope].register_lazy_snapshot(enclosing_symbol_id) + // Lazy: every use of this symbol resolves to the same + // growing snapshot, so dedup on (nested scope, nested symbol) + // and reuse it across uses. + let dedup_key = (self.current_scope, nested_symbol); + + if let Some(&entry) = self.lazy_snapshots.get(&dedup_key) { + entry + } else { + let snapshot_id = self.use_def_maps[current_scope] + .register_lazy_snapshot(enclosing_symbol_id); + let entry = (current_scope, snapshot_id); + self.lazy_snapshots.insert(dedup_key, entry); + entry + } + }; + + let use_key = EnclosingSnapshotKey { + nested_scope: self.current_scope, + nested_use: use_id, }; - self.enclosing_snapshots - .insert(use_key, (current_scope, snapshot_id)); + self.enclosing_snapshots.insert(use_key, entry); return; } diff --git a/crates/oak_semantic/src/semantic_index.rs b/crates/oak_semantic/src/semantic_index.rs index 9b1217f3cf..5d4a71cc69 100644 --- a/crates/oak_semantic/src/semantic_index.rs +++ b/crates/oak_semantic/src/semantic_index.rs @@ -335,8 +335,7 @@ impl SemanticIndex { let local = bindings.definitions().iter().map(move |&d| (scope_id, d)); let enclosing = if bindings.may_be_unbound() { - let symbol_id = self.uses(scope_id)[use_id].symbol(); - self.enclosing_bindings(scope_id, symbol_id) + self.enclosing_bindings(scope_id, use_id) } else { None }; @@ -349,10 +348,10 @@ impl SemanticIndex { /// Resolve a free variable's bindings from the enclosing scope. /// - /// When a use in `scope` may be unbound (`may_be_unbound: true`), some - /// control-flow paths fall through to an enclosing scope. This looks up - /// the enclosing snapshot that was registered during the build and - /// returns the ancestor scope and its bindings. This covers both purely + /// When the use `use_id` in `scope` may be unbound (`may_be_unbound: true`), + /// some control-flow paths fall through to an enclosing scope. This looks up + /// the enclosing snapshot that was registered for that use during the build + /// and returns the ancestor scope and its bindings. This covers both purely /// free variables (no local definitions) and conditionally defined /// variables (local definitions exist but don't cover all paths). /// @@ -362,11 +361,11 @@ impl SemanticIndex { pub fn enclosing_bindings( &self, scope: ScopeId, - symbol: SymbolId, + use_id: UseId, ) -> Option<(ScopeId, &Bindings)> { let key = EnclosingSnapshotKey { nested_scope: scope, - nested_symbol: symbol, + nested_use: use_id, }; let &(enclosing_scope, snapshot_id) = self.enclosing_snapshots.get(&key)?; let bindings = self.use_def_maps[enclosing_scope].enclosing_snapshot(snapshot_id); @@ -375,13 +374,17 @@ impl SemanticIndex { } /// Key for looking up an enclosing snapshot. Keyed by the nested scope and the -/// symbol's `SymbolId` in the nested scope's symbol table (not the enclosing -/// scope's), so consumers can do an O(1) lookup directly from a `UseId` without -/// re-walking the ancestor chain. +/// `UseId` of the free variable in that scope, so consumers do an O(1) lookup +/// straight from a use without re-walking the ancestor chain. +/// +/// Keyed per use, not per symbol, because eager snapshots (e.g. `local()`) are +/// point-in-time: two uses of the same free variable at different points in an +/// eager body can see different enclosing states, so each gets its own +/// snapshot. Lazy uses of one symbol still share a single snapshot. #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct EnclosingSnapshotKey { pub nested_scope: ScopeId, - pub nested_symbol: SymbolId, + pub nested_use: UseId, } // --- Scope --- diff --git a/crates/oak_semantic/src/use_def_map.rs b/crates/oak_semantic/src/use_def_map.rs index c2e9f6da2a..259eaa4156 100644 --- a/crates/oak_semantic/src/use_def_map.rs +++ b/crates/oak_semantic/src/use_def_map.rs @@ -189,9 +189,10 @@ use crate::semantic_index::UseId; // For eager NSE scopes (e.g. `local()`), the snapshot is more precise: since // the body executes at the call site, it's a point-in-time capture reflecting // exactly the linear state, with no union over definitions that come later in -// the enclosing scope. The one exception is a `<<-` inside the eager body: it -// mutates the enclosing binding mid-run, so its watcher fires for deferred -// definitions only (see `register_eager_snapshot()`). +// the enclosing scope. Each use in the body captures its own snapshot, so a use +// after a `<<-` in the body sees the mutated binding while an earlier use does +// not. Eager snapshots take no watcher: they never fold in a later definition +// (see `register_eager_snapshot()`). /// The immutable use-def map for a single scope. For each use site, stores the /// set of definitions that can reach it through control flow. @@ -307,12 +308,9 @@ pub(crate) struct UseDefMapBuilder { deferred_defs: Vec<(SymbolId, DefinitionId)>, enclosing_snapshots: IndexVec, // Lazy snapshots (e.g. `reactive()`), notified of every definition of a - // symbol so they fold in the whole union. + // symbol so they fold in the whole union. Eager snapshots (e.g. `local()`) + // are point-in-time and subscribe to nothing. lazy_watchers: FxHashMap>, - // Eager snapshots (e.g. `local()`), notified only of deferred (`<<-`) - // definitions. A point-in-time capture ignores later plain defs, but a - // `<<-` mutates the binding while the eager body runs, so it must fold in. - eager_watchers: FxHashMap>, } impl UseDefMapBuilder { @@ -323,7 +321,6 @@ impl UseDefMapBuilder { deferred_defs: Vec::new(), enclosing_snapshots: IndexVec::new(), lazy_watchers: FxHashMap::default(), - eager_watchers: FxHashMap::default(), } } @@ -412,20 +409,15 @@ impl UseDefMapBuilder { pub(crate) fn record_deferred_definition(&mut self, symbol_id: SymbolId, def_id: DefinitionId) { self.symbol_states[symbol_id].add_definition(def_id); self.deferred_defs.push((symbol_id, def_id)); - // A deferred def reaches both watcher sets: lazy snapshots (like any - // def) and eager snapshots (which take deferred defs only). + // A deferred def reaches lazy snapshots like any other def. Eager + // snapshots take no watcher, so a `<<-` inside an eager body reaches + // them only through the point-in-time clone of a use that follows it. Self::notify_watchers( &mut self.enclosing_snapshots, &self.lazy_watchers, symbol_id, def_id, ); - Self::notify_watchers( - &mut self.enclosing_snapshots, - &self.eager_watchers, - symbol_id, - def_id, - ); } /// Record a use of `symbol_id`. Clones the current live bindings for that @@ -500,41 +492,34 @@ impl UseDefMapBuilder { /// definitions that come later in the enclosing scope. /// /// Unlike [`register_lazy_snapshot`](Self::register_lazy_snapshot), this - /// watcher fires only on deferred (`<<-`) definitions. A `<<-` inside the - /// eager body changes the binding while the body runs, and uses later in - /// the body must see that change. A plain `<-` after the eager call on the - /// other hand runs once the body has finished, so it stays out of the - /// snapshot. One snapshot is shared by every use of the symbol in the body, - /// so a use before the `<<-` picks it up too. That is a known over-approximation. + /// takes no watcher. The clone is the whole answer. Each use in the eager + /// body registers its own snapshot, so it captures the enclosing state at + /// that exact point in the flow. A `<<-` inside the body is already live in + /// `symbol_states` by the time a later use clones, so that use sees it while + /// an earlier one does not: /// /// ```r /// x <- 1 /// local({ - /// x # {1, 2}: Should be {1} but shares one snapshot with the use below - /// x <<- 2 # deferred def, folded into the snapshot - /// x # {1, 2} + /// x # {1}: cloned before the `<<-` + /// x <<- 2 # deferred def, now live in the enclosing state + /// x # {1, 2}: cloned after the `<<-` /// }) - /// x <- 3 # plain `<-` after the call, stays out of the snapshot /// ``` /// - /// The watcher keys on the symbol, not on where the def came from. It also - /// picks up deferred defs from outside this body. One is a `<<-` in a - /// function defined after the eager call. Another is a `Current + Lazy` - /// routing like `rlang::on_load`. Neither can reach the body, which already - /// ran. Both are safe over-approximations, the same kind as the pre-`<<-` - /// use above. + /// And since nothing fires later, a definition recorded after the body has + /// run never folds in, which is correct: the eager body is already done. /// /// ```r /// x <- 1 /// local({ x }) # {1} - /// f <- function() { x <<- 2 } # {1, 2}: f's `<<-` can't reach the finished body - /// rlang::on_load({ x <- 3 }) # {1, 2, 3}: routed def can't reach it either + /// f <- function() { x <<- 2 } # f's `<<-` can't reach the finished body + /// rlang::on_load({ x <- 3 }) # routed def can't reach it either + /// x <- 4 # plain `<-` after the call, out of the snapshot /// ``` pub(crate) fn register_eager_snapshot(&mut self, symbol_id: SymbolId) -> EnclosingSnapshotId { let bindings = self.symbol_states[symbol_id].clone(); - let id = self.enclosing_snapshots.push(bindings); - self.eager_watchers.entry(symbol_id).or_default().push(id); - id + self.enclosing_snapshots.push(bindings) } fn notify_watchers( diff --git a/crates/oak_semantic/tests/integration/builder_nse.rs b/crates/oak_semantic/tests/integration/builder_nse.rs index 50eab4218c..2a16af9486 100644 --- a/crates/oak_semantic/tests/integration/builder_nse.rs +++ b/crates/oak_semantic/tests/integration/builder_nse.rs @@ -219,8 +219,7 @@ local({ // `x` inside `f` resolves to the `local` scope (not the file scope), and // its lazy snapshot picks up `x <- 1` (DefinitionId 1 in the local scope: // `f` is DefinitionId 0, `x` is DefinitionId 1). - let x_sym = index.uses(f_scope)[UseId::from(0)].symbol(); - let (enclosing_scope, bindings) = index.enclosing_bindings(f_scope, x_sym).unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(f_scope, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, local_scope); assert_eq!(bindings.definitions(), &[DefinitionId::from(1)]); } @@ -343,10 +342,7 @@ f <- function() x // In `f`, `x` is free and unbound -- no enclosing snapshot should find it // in the file scope. - assert_eq!( - index.enclosing_bindings(fun_scope, index.uses(fun_scope)[UseId::from(0)].symbol()), - None - ); + assert_eq!(index.enclosing_bindings(fun_scope, UseId::from(0)), None); } #[test] @@ -368,10 +364,7 @@ x <- 2 // (point-in-time). At the call site, only `x <- 1` (DefinitionId 0) is // live. `x <- 2` (DefinitionId 2) comes after and should NOT be included. let (enclosing_scope, bindings) = index - .enclosing_bindings( - local_scope, - index.uses(local_scope)[UseId::from(0)].symbol(), - ) + .enclosing_bindings(local_scope, UseId::from(0)) .unwrap(); assert_eq!(enclosing_scope, ScopeId::from(0)); assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); @@ -394,9 +387,7 @@ x <- 2 let fun_scope = ScopeId::from(1); // Function is lazy: snapshot includes both x <- 1 and x <- 2. - let (_, bindings) = index - .enclosing_bindings(fun_scope, index.uses(fun_scope)[UseId::from(0)].symbol()) - .unwrap(); + let (_, bindings) = index.enclosing_bindings(fun_scope, UseId::from(0)).unwrap(); assert_eq!(bindings.definitions(), &[ DefinitionId::from(0), DefinitionId::from(2) @@ -452,9 +443,7 @@ f <- function() x // `f` is lazy, so its snapshot for `x` should include BOTH defs: // `x <- 1` (file-level) and `x <- 2` (from on_load, deferred). // If on_load's definition shadowed, we'd only see `x <- 2`. - let (enclosing_scope, bindings) = index - .enclosing_bindings(fun_scope, index.uses(fun_scope)[UseId::from(0)].symbol()) - .unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(fun_scope, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, ScopeId::from(0)); assert_eq!(bindings.definitions(), &[ DefinitionId::from(0), @@ -567,8 +556,7 @@ f <- function() { // `g`'s free `x` resolves to nothing: the sibling `local()` binds `x` in // its own scope, not in `f`. Flow-insensitive bound names would wrongly // point it at a stray `x` in `f`. - let g_x = index.uses(g_scope)[UseId::from(0)].symbol(); - assert_eq!(index.enclosing_bindings(g_scope, g_x), None); + assert_eq!(index.enclosing_bindings(g_scope, UseId::from(0)), None); } #[test] @@ -703,10 +691,7 @@ x <- 2 // then to the file scope. The function scope is lazy, so both defs are // visible despite `local` being eager. let (enclosing_scope, bindings) = index - .enclosing_bindings( - local_scope, - index.uses(local_scope)[UseId::from(0)].symbol(), - ) + .enclosing_bindings(local_scope, UseId::from(0)) .unwrap(); assert_eq!(enclosing_scope, ScopeId::from(0)); assert_eq!(bindings.definitions(), &[ @@ -791,10 +776,9 @@ local({ #[test] fn test_nse_eager_super_assignment_visible_to_later_use() { - // A `<<-` inside an eager NSE body mutates the enclosing binding mid-run, - // so uses after it must see the `<<-` definition. The eager snapshot is - // shared across all uses of the free variable, so it accumulates the `<<-` - // (a safe over-approximation for the earlier use, correct for the later). + // A `<<-` inside an eager NSE body mutates the enclosing binding mid-run. + // Each use captures its own point-in-time snapshot, so the use before the + // `<<-` sees only `x <- 1` while the use after it also sees the `<<-`. let index = index( "\ x <- 1 @@ -808,24 +792,29 @@ local({ let file = ScopeId::from(0); let local_scope = ScopeId::from(1); - // The enclosing snapshot for `x` (in the file scope) carries both the - // initial `x <- 1` (DefinitionId 0) and the `<<-` target (DefinitionId 1). - let x_sym = index.uses(local_scope)[UseId::from(0)].symbol(); - let (enclosing_scope, bindings) = index.enclosing_bindings(local_scope, x_sym).unwrap(); - assert_eq!(enclosing_scope, file); - assert_eq!(bindings.definitions(), &[ + // Use 0 is before the `<<-`: only `x <- 1` (DefinitionId 0). + let (before_scope, before) = index + .enclosing_bindings(local_scope, UseId::from(0)) + .unwrap(); + assert_eq!(before_scope, file); + assert_eq!(before.definitions(), &[DefinitionId::from(0)]); + + // Use 1 is after the `<<-`: `x <- 1` (0) and the `<<-` target (1). + let (after_scope, after) = index + .enclosing_bindings(local_scope, UseId::from(1)) + .unwrap(); + assert_eq!(after_scope, file); + assert_eq!(after.definitions(), &[ DefinitionId::from(0), DefinitionId::from(1) ]); } #[test] -fn test_nse_eager_snapshot_absorbs_unrelated_super_assignment() { - // The eager snapshot keys on the symbol in the enclosing scope, so it - // can't tell a `<<-` inside the body from one in a function defined after - // the call. `f`'s `<<-` is recorded on the file scope while its body is - // walked, firing the eager watcher, so `local()`'s snapshot over-includes - // it even though it can't reach the already-run body. +fn test_nse_eager_snapshot_excludes_unrelated_super_assignment() { + // The eager snapshot is point-in-time with no watcher, so a `<<-` in a + // function defined after the `local()` call can't fold into it. `f`'s body + // runs at an unknown later time and can't reach the already-run eager body. let index = index( "\ x <- 1 @@ -840,24 +829,21 @@ f <- function() { let file = ScopeId::from(0); let local_scope = ScopeId::from(1); - // File-scope defs in allocation order: `x <- 1` (0), then f's `<<-` - // target (1, recorded before f's own def since `collect_assignment` walks - // the value side first), then `f` (2). The snapshot absorbs the `<<-`. - let x_sym = index.uses(local_scope)[UseId::from(0)].symbol(); - let (enclosing_scope, bindings) = index.enclosing_bindings(local_scope, x_sym).unwrap(); + // Only `x <- 1` (DefinitionId 0). The `<<-` target recorded later while f's + // body is walked is excluded, since the eager snapshot took no watcher. + let (enclosing_scope, bindings) = index + .enclosing_bindings(local_scope, UseId::from(0)) + .unwrap(); assert_eq!(enclosing_scope, file); - assert_eq!(bindings.definitions(), &[ - DefinitionId::from(0), - DefinitionId::from(1) - ]); + assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); } #[test] -fn test_nse_eager_snapshot_absorbs_unrelated_routed_definition() { +fn test_nse_eager_snapshot_excludes_unrelated_routed_definition() { // `on_load` is `Current + Lazy`, so its `x <- 2` routes to the file scope - // as a deferred def. That fires the eager watcher, so `local()`'s snapshot - // over-includes it, even though the routed def can't reach the already-run - // body. + // as a deferred def recorded after `local()`. The eager snapshot takes no + // watcher, so the routed def is excluded, correct since it can't reach the + // already-run body. let index = index( "\ x <- 1 @@ -872,14 +858,12 @@ rlang::on_load({ let file = ScopeId::from(0); let local_scope = ScopeId::from(1); - // File-scope defs: `x <- 1` (0) and the `on_load`-routed `x <- 2` (1). - let x_sym = index.uses(local_scope)[UseId::from(0)].symbol(); - let (enclosing_scope, bindings) = index.enclosing_bindings(local_scope, x_sym).unwrap(); + // Only `x <- 1` (DefinitionId 0). The routed `x <- 2` is excluded. + let (enclosing_scope, bindings) = index + .enclosing_bindings(local_scope, UseId::from(0)) + .unwrap(); assert_eq!(enclosing_scope, file); - assert_eq!(bindings.definitions(), &[ - DefinitionId::from(0), - DefinitionId::from(1) - ]); + assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); } // --- Resolver-driven recognition --- @@ -1328,8 +1312,7 @@ local({ ); assert_eq!(index.scope(f_scope).kind(), ScopeKind::Function); - let x_sym = index.uses(f_scope)[UseId::from(0)].symbol(); - let (enclosing_scope, _bindings) = index.enclosing_bindings(f_scope, x_sym).unwrap(); + let (enclosing_scope, _bindings) = index.enclosing_bindings(f_scope, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, local_scope); } @@ -1366,8 +1349,7 @@ local({ // `f`'s use of `y` resolves to local's snapshot. In local, `y` is // DefinitionId 0 (`f` is DefinitionId 1). - let y_sym = index.uses(f_scope)[UseId::from(0)].symbol(); - let (enclosing_scope, bindings) = index.enclosing_bindings(f_scope, y_sym).unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(f_scope, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, local_scope); assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); } diff --git a/crates/oak_semantic/tests/integration/use_def_map.rs b/crates/oak_semantic/tests/integration/use_def_map.rs index 94af04e065..6d7fc0409e 100644 --- a/crates/oak_semantic/tests/integration/use_def_map.rs +++ b/crates/oak_semantic/tests/integration/use_def_map.rs @@ -965,9 +965,7 @@ f <- function() x let fun = ScopeId::from(1); // `x` in the function is free, resolves to file scope - let (enclosing_scope, bindings) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, ScopeId::from(0)); assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); assert_not!(bindings.may_be_unbound()); @@ -986,9 +984,7 @@ x <- 1 // `x` is defined after `f` in the file scope. The pre-scan finds it. // The snapshot is initialized at f's definition point (x unbound) // then updated when x <- 1 is encountered. - let (enclosing_scope, bindings) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, ScopeId::from(0)); assert_eq!(bindings.definitions(), &[DefinitionId::from(1)]); assert!(bindings.may_be_unbound()); @@ -1007,9 +1003,7 @@ x <- 2 // Lazy snapshot: union of all defs from definition point onward. // Initialized with {x <- 1}, updated with {x <- 2}. - let (_, bindings) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); + let (_, bindings) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); assert_eq!(bindings.definitions(), &[ DefinitionId::from(0), DefinitionId::from(2) @@ -1031,9 +1025,7 @@ f <- function() { let fun = ScopeId::from(1); // `x` is locally bound in the function, not free - assert!(index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .is_none()); + assert!(index.enclosing_bindings(fun, UseId::from(0)).is_none()); } #[test] @@ -1047,9 +1039,7 @@ f <- function(x) x let fun = ScopeId::from(1); // `x` is a parameter, not free - assert!(index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .is_none()); + assert!(index.enclosing_bindings(fun, UseId::from(0)).is_none()); } #[test] @@ -1067,9 +1057,7 @@ f <- function() { // x is free in g. f (scope 1) has no binding for x, so the lookup // skips f entirely and resolves to the file scope (scope 0). - let (enclosing_scope, bindings) = index - .enclosing_bindings(g_scope, index.uses(g_scope)[UseId::from(0)].symbol()) - .unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(g_scope, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, ScopeId::from(0)); assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); assert_not!(bindings.may_be_unbound()); @@ -1090,9 +1078,7 @@ f <- function() { // x is free in g. Both the file scope (scope 0) and f (scope 1) bind x, // but f is the nearest enclosing scope with a binding, so it wins. - let (enclosing_scope, bindings) = index - .enclosing_bindings(g_scope, index.uses(g_scope)[UseId::from(0)].symbol()) - .unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(g_scope, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, ScopeId::from(1)); assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); assert_not!(bindings.may_be_unbound()); @@ -1110,9 +1096,7 @@ f <- function() x // x is conditionally defined. The snapshot captures the state at f's // definition point: {x <- 1, may_be_unbound: true} - let (_, bindings) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); + let (_, bindings) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); assert!(bindings.may_be_unbound()); } @@ -1130,9 +1114,7 @@ g <- function() { x <<- 2 } // The <<- from g adds a def to the file scope. The watcher on x // should update f's snapshot to include this def. - let (_, bindings) = index - .enclosing_bindings(f_scope, index.uses(f_scope)[UseId::from(0)].symbol()) - .unwrap(); + let (_, bindings) = index.enclosing_bindings(f_scope, UseId::from(0)).unwrap(); assert_eq!(bindings.definitions(), &[ DefinitionId::from(0), DefinitionId::from(2) @@ -1150,9 +1132,7 @@ f <- function() x let fun = ScopeId::from(1); // x is not defined anywhere in the file. No enclosing snapshot. - assert!(index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .is_none()); + assert!(index.enclosing_bindings(fun, UseId::from(0)).is_none()); } #[test] @@ -1178,9 +1158,7 @@ f <- function(cond) { assert!(local.may_be_unbound()); // The enclosing snapshot should also be registered, capturing x <- 1. - let (enclosing_scope, bindings) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(1)].symbol()) - .unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(fun, UseId::from(1)).unwrap(); assert_eq!(enclosing_scope, ScopeId::from(0)); assert_eq!(bindings.definitions(), &[DefinitionId::from(0)]); assert_not!(bindings.may_be_unbound()); @@ -1206,9 +1184,7 @@ f <- function() { assert!(local.may_be_unbound()); // Enclosing snapshot registered for the fallthrough path. - let (enclosing_scope, _) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); + let (enclosing_scope, _) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, ScopeId::from(0)); } @@ -1232,9 +1208,7 @@ f <- function() { assert_not!(local.may_be_unbound()); // No enclosing snapshot needed. - assert!(index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .is_none()); + assert!(index.enclosing_bindings(fun, UseId::from(0)).is_none()); } #[test] @@ -1252,15 +1226,11 @@ f <- function() { let fun = ScopeId::from(1); // Two independent free variables, each gets its own snapshot - let (scope_x, bindings_x) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); + let (scope_x, bindings_x) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); assert_eq!(scope_x, ScopeId::from(0)); assert_eq!(bindings_x.definitions(), &[DefinitionId::from(0)]); - let (scope_y, bindings_y) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(1)].symbol()) - .unwrap(); + let (scope_y, bindings_y) = index.enclosing_bindings(fun, UseId::from(1)).unwrap(); assert_eq!(scope_y, ScopeId::from(0)); assert_eq!(bindings_y.definitions(), &[DefinitionId::from(1)]); } @@ -1279,12 +1249,8 @@ f <- function() { let fun = ScopeId::from(1); // Both uses of `x` are free and resolve to the same enclosing snapshot - let (scope1, bindings1) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); - let (scope2, bindings2) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(1)].symbol()) - .unwrap(); + let (scope1, bindings1) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); + let (scope2, bindings2) = index.enclosing_bindings(fun, UseId::from(1)).unwrap(); assert_eq!(scope1, scope2); assert_eq!(bindings1, bindings2); } @@ -1307,9 +1273,7 @@ x <- 2 // x is free in f, resolves to file scope. The lazy snapshot // captures both x <- 1 (from initialization) and x <- 2 (from // watcher update). - let (enclosing_scope, bindings) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, ScopeId::from(0)); assert_eq!(bindings.definitions(), &[ DefinitionId::from(0), @@ -1338,12 +1302,8 @@ f <- function() { assert!(local0.definitions().is_empty()); assert!(local0.may_be_unbound()); - let (scope0, bindings0) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); - let (scope1, bindings1) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(1)].symbol()) - .unwrap(); + let (scope0, bindings0) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); + let (scope1, bindings1) = index.enclosing_bindings(fun, UseId::from(1)).unwrap(); assert_eq!(scope0, scope1); assert_eq!(bindings0, bindings1); assert_eq!(bindings0.definitions(), &[DefinitionId::from(0)]); @@ -1368,9 +1328,7 @@ f <- function(cond) { // f (scope 1, conditional x <- 2) bind x. f is the nearest enclosing // scope with a binding, so it wins. The snapshot captures f's state at // g's definition point: {x <- 2, may_be_unbound: true}. - let (enclosing_scope, bindings) = index - .enclosing_bindings(g_scope, index.uses(g_scope)[UseId::from(0)].symbol()) - .unwrap(); + let (enclosing_scope, bindings) = index.enclosing_bindings(g_scope, UseId::from(0)).unwrap(); assert_eq!(enclosing_scope, ScopeId::from(1)); assert_eq!(bindings.definitions(), &[DefinitionId::from(1)]); assert!(bindings.may_be_unbound()); @@ -1389,9 +1347,7 @@ f <- function() x // x <- 0 was shadowed by x <- 1 before f was defined. // The snapshot should contain only x <- 1, not both. - let (_, bindings) = index - .enclosing_bindings(fun, index.uses(fun)[UseId::from(0)].symbol()) - .unwrap(); + let (_, bindings) = index.enclosing_bindings(fun, UseId::from(0)).unwrap(); assert_eq!(bindings.definitions(), &[DefinitionId::from(1)]); assert_not!(bindings.may_be_unbound()); } @@ -1410,9 +1366,7 @@ g <- function() x // f is defined after x <- 1. Its snapshot is initialized with {x <- 1}, // then the watcher adds x <- 2: snapshot {x <- 1, x <- 2}. let f_scope = ScopeId::from(1); - let (_, f_bindings) = index - .enclosing_bindings(f_scope, index.uses(f_scope)[UseId::from(0)].symbol()) - .unwrap(); + let (_, f_bindings) = index.enclosing_bindings(f_scope, UseId::from(0)).unwrap(); assert_eq!(f_bindings.definitions(), &[ DefinitionId::from(0), DefinitionId::from(2) @@ -1423,9 +1377,7 @@ g <- function() x // initialized with {x <- 2} only. No subsequent definitions, so it // stays {x <- 2}. let g_scope = ScopeId::from(2); - let (_, g_bindings) = index - .enclosing_bindings(g_scope, index.uses(g_scope)[UseId::from(0)].symbol()) - .unwrap(); + let (_, g_bindings) = index.enclosing_bindings(g_scope, UseId::from(0)).unwrap(); assert_eq!(g_bindings.definitions(), &[DefinitionId::from(2)]); assert_not!(g_bindings.may_be_unbound()); }