Handle ambiguous effects more reliably and emit diagnostics for these

crates/oak_db/src/definition.rs

@@ -1,5 +1,6 @@
 use aether_syntax::RBinaryExpression;
 use aether_syntax::RSyntaxKind;
+use aether_syntax::RSyntaxNode;
 use biome_rowan::AstNode;
 use biome_rowan::TextRange;
 use oak_semantic::semantic_index::DefinitionId;
@@ -55,35 +56,40 @@ impl<'db> Definition<'db> {
     pub fn name_range(self, db: &'db dyn crate::Db) -> Option<TextRange> {
         let parse = self.file(db).parse(db);
         let root = parse.tree().syntax().clone();
-
-        let name_node = match self.kind(db) {
-            DefinitionKind::Assignment(ptr) | DefinitionKind::SuperAssignment(ptr) => {
-                let node = ptr.to_node(&root);
-                // Right-assign (`rhs -> x`, `rhs ->> x`) puts the target on
-                // the right, every other form (`x <- rhs`, `x <<- rhs`,
-                // `x = rhs`) puts it on the left.
-                let target = if is_right_assignment(&node) {
-                    node.right().ok()?
-                } else {
-                    node.left().ok()?
-                };
-                target.into_syntax()
-            },
-            DefinitionKind::Parameter(ptr) => {
-                let node = ptr.to_node(&root);
-                node.name().ok()?.into_syntax()
-            },
-            DefinitionKind::ForVariable(ptr) => {
-                let node = ptr.to_node(&root);
-                node.variable().ok()?.into_syntax()
-            },
-            DefinitionKind::Import { .. } => return None,
-            DefinitionKind::Assign { name, .. } => name.to_node(&root).into_syntax(),
-        };
-        Some(name_node.text_trimmed_range())
+        Some(name_node(self.kind(db), &root)?.text_trimmed_range())
     }
 }
 
+/// The syntax node of the bound name for `kind`.
+fn name_node(kind: &DefinitionKind, root: &RSyntaxNode) -> Option<RSyntaxNode> {
+    let node = match kind {
+        DefinitionKind::Assignment(ptr) | DefinitionKind::SuperAssignment(ptr) => {
+            let node = ptr.to_node(root);
+            // Right-assign (`rhs -> x`, `rhs ->> x`) puts the target on
+            // the right, every other form (`x <- rhs`, `x <<- rhs`,
+            // `x = rhs`) puts it on the left.
+            let target = if is_right_assignment(&node) {
+                node.right().ok()?
+            } else {
+                node.left().ok()?
+            };
+            target.into_syntax()
+        },
+        DefinitionKind::Parameter(ptr) => {
+            let node = ptr.to_node(root);
+            node.name().ok()?.into_syntax()
+        },
+        DefinitionKind::ForVariable(ptr) => {
+            let node = ptr.to_node(root);
+            node.variable().ok()?.into_syntax()
+        },
+        DefinitionKind::Assign { name, .. } => name.to_node(root).into_syntax(),
+        // No name token at the binding site
+        DefinitionKind::Import { .. } => return None,
+    };
+    Some(node)
+}
+
 fn is_right_assignment(node: &RBinaryExpression) -> bool {
     node.operator().is_ok_and(|op| {
         matches!(

crates/oak_db/src/file.rs

@@ -4,6 +4,7 @@ use std::sync::Arc;
 use aether_path::FilePath;
 use biome_line_index::LineIndex;
 use biome_rowan::TextRange;
+use oak_semantic::semantic_index::AmbiguityReason;
 use oak_semantic::semantic_index::ScopeId;
 use oak_semantic::semantic_index::SemanticDiagnostic;
 use oak_semantic::semantic_index::SemanticIndex;
@@ -362,20 +363,54 @@ fn build_semantic_index_inner(file: File, db: &dyn Db) -> SemanticIndex {
         let line_index = file.line_index(db);
 
         for diagnostic in diagnostics {
-            match diagnostic {
-                SemanticDiagnostic::LazyShadowAmbiguity {
-                    name,
-                    call_range,
-                    overwrite_range,
-                } => {
-                    let call = format_line_col(line_index, *call_range);
+            if let SemanticDiagnostic::AmbiguousAttachOrder { packages, range } = diagnostic {
+                let at = format_line_col(line_index, *range);
+                log::warn!(
+                    "Ambiguous attach order in {path}:{at}: the branches attach {packages} in \
+                     different orders.",
+                    packages = packages.join(", ")
+                );
+                continue;
+            }
+
+            let SemanticDiagnostic::EffectAmbiguity {
+                name,
+                call_range,
+                reason,
+            } = diagnostic
+            else {
+                continue;
+            };
+            let call = format_line_col(line_index, *call_range);
+
+            match reason {
+                AmbiguityReason::LazyShadow { overwrite_range } => {
                     let overwrite = format_line_col(line_index, *overwrite_range);
                     log::warn!(
                         "Lazy-shadow ambiguity in {path}:{call}: callee `{name}` is recognized \
                          as effectful, but a lazy-crossed ancestor binds it at {overwrite} with \
                          undetermined timing"
                     )
                 },
+                AmbiguityReason::ConditionalShadow { binding_range } => {
+                    let binding = format_line_col(line_index, *binding_range);
+                    log::warn!(
+                        "Conditional-shadow ambiguity in {path}:{call}: callee `{name}` is \
+                         recognized as effectful, but a conditional local binding at {binding} \
+                         could shadow it on some path"
+                    )
+                },
+                AmbiguityReason::ConditionalAttach {
+                    package,
+                    attach_range,
+                } => {
+                    let attach = format_line_col(line_index, *attach_range);
+                    log::warn!(
+                        "Conditional-attach ambiguity in {path}:{call}: callee `{name}` is read as \
+                         plain because `{package}`, attached at {attach}, dropped at a branch or \
+                         loop join. It would be effectful on the path where that attach ran"
+                    )
+                },
             }
         }
     }

crates/oak_db/src/file_imports.rs

@@ -4,8 +4,11 @@ use std::collections::HashMap;
 use biome_rowan::TextSize;
 use camino::Utf8Path;
 use oak_package_metadata::namespace::Namespace;
+use oak_semantic::semantic_index::AttachRegion;
+use oak_semantic::semantic_index::ScopeId;
+use oak_semantic::semantic_index::SemanticCall;
 use oak_semantic::semantic_index::SemanticCallKind;
-use oak_semantic::ScopeId;
+use oak_semantic::semantic_index::SemanticIndex;
 
 use crate::Db;
 use crate::File;
@@ -67,6 +70,72 @@ impl CrossFileLayers {
     }
 }
 
+/// Which of a file's own `library()` attaches a caller sees.
+#[derive(Clone, Copy)]
+enum AttachView {
+    /// Every attach in the file.
+    ///
+    /// Over-approximates on two axes. An attach in a function body counts even
+    /// though the body may never run, and a conditional one counts even though
+    /// its branch may not have been taken.
+    Anywhere,
+    /// Attaches visible at `offset` in lazy `scope`.
+    ///
+    /// An attach in `scope_id` or an enclosing lazy body applies only after its
+    /// `library()` call. The lazy view treats an unconditional top-level attach
+    /// as visible regardless of position, which over-approximates this case:
+    ///
+    /// ```r
+    /// f <- function() {
+    ///     cli_alert("x")
+    /// }
+    ///
+    /// f()
+    /// library(cli)
+    /// ```
+    ///
+    /// `f()` runs before `library(cli)`, so the attach is unavailable at
+    /// `cli_alert()`. In the future, call analysis could detect that order.
+    ///
+    /// Conditional attaches remain limited to their arm, and child or sibling
+    /// bodies do not reach `scope_id`.
+    Lazy { offset: TextSize, scope_id: ScopeId },
+    /// The attaches that have run and still hold at `offset` in an eagerly
+    /// evaluated scope. Calls reached only by running a lazy body are dropped,
+    /// as are calls after the offset.
+    Eager(TextSize),
+}
+
+impl AttachView {
+    fn sees(&self, index: &SemanticIndex, call: &SemanticCall, region: &AttachRegion) -> bool {
+        match *self {
+            AttachView::Anywhere => true,
+            AttachView::Lazy {
+                offset,
+                scope_id: scope,
+            } => match index.enclosing_lazy_scope(call.scope()) {
+                // The lazy view treats unconditional top-level attaches as
+                // preceding every body. Conditional attaches stay in their arm.
+                None => match region {
+                    AttachRegion::Unconditional => true,
+                    AttachRegion::Conditional { .. } => region.contains(call, offset),
+                },
+                // A lazy body's attach reaches only that body and its descendants,
+                // and only after the call returns.
+                Some(unit) => {
+                    index
+                        .ancestor_scope_ids(scope)
+                        .any(|ancestor| ancestor == unit) &&
+                        region.contains(call, offset)
+                },
+            },
+            AttachView::Eager(offset) => {
+                index.scope_is_eager(call.scope()) && region.contains(call, offset)
+            },
+        }
+    }
+}
+
 /// The point in a package's load at which a file views its collation siblings.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub(crate) enum CollationView {
@@ -80,10 +149,12 @@ pub(crate) enum CollationView {
 
 #[salsa::tracked]
 impl File {
-    /// The import layers visible to this file at end-of-file, in R's lookup
-    /// (LIFO) priority order. Symbols that don't have local bindings (are
-    /// unbound in the file's semantic index) can be resolved against these
-    /// imports.
+    /// Every import layer this file could see, in R's lookup (LIFO) priority
+    /// order. Symbols that don't have local bindings (are unbound in the file's
+    /// semantic index) can be resolved against these imports.
+    ///
+    /// Over-approximates: every attach in the file contributes a layer,
+    /// including ones in a function body or in a branch that wasn't taken.
     ///
     /// `library()` calls further down the file come earlier in the returned
     /// `Vec`, and collation files later in the package come earlier too. The
@@ -97,17 +168,16 @@ impl File {
     #[salsa::tracked(returns(ref))]
     pub fn imports(self, db: &dyn Db) -> Vec<ImportLayer> {
         let layers = self.cross_file_layers(db, CollationView::Lazy);
-        let own = self.attach_layers(db, None);
+        let own = self.attach_layers(db, AttachView::Anywhere);
         layers.splice_own_attaches(own)
     }
 
     /// Import layers visible at an `offset` in a file:
     ///
-    /// - **Cursor in lazy context**: returns the full lazy view. Lazy
-    ///   contexts like functions are treated as if they run after the
-    ///   file is fully sourced (over-approximation). Any `library()` /
-    ///   collation entry is potentially visible regardless of where it
-    ///   appears relative to the cursor.
+    /// - **Cursor in lazy context**: every collation sibling and unconditional
+    ///   top-level attach is visible. Attaches from the current or enclosing lazy
+    ///   body must precede the cursor, and conditional attaches remain limited to
+    ///   the arm that attaches them.
     ///
     /// - **Top-level cursor (script)**: only `library()` calls that
     ///   have occurred before `offset`. Most recently attached comes
@@ -120,54 +190,51 @@ impl File {
     /// Plain method rather than `#[salsa::tracked]`. Tracking would key the
     /// cache on `(self, offset)`, creating one entry per cursor position.
     /// Skipping the cache is fine because the body just reads already-cached
-    /// subqueries (`imports`, `semantic_index`) and applies an O(n) filter.
+    /// subqueries (`cross_file_layers`, `semantic_index`) and applies an O(n)
+    /// filter.
     pub fn imports_at(self, db: &dyn Db, offset: TextSize) -> Vec<ImportLayer> {
         let index = self.semantic_index(db);
-        let file_scope = ScopeId::from(0);
         let (cursor_scope, _) = index.scope_at(offset);
 
-        // Cursor in lazy context. EOF view, same as `imports()`.
-        if cursor_scope != file_scope {
-            return self.imports(db).clone();
-        }
+        // An eager scope runs during the file's own top-level execution, so a
+        // cursor in a `local()` block sees the search path as of that point,
+        // the same as one at file scope.
+        let (collation, attaches) = if index.scope_is_eager(cursor_scope) {
+            // Predecessors only, and own attaches narrowed to the calls that
+            // have run by `offset`.
+            (CollationView::Eager, AttachView::Eager(offset))
+        } else {
+            (CollationView::Lazy, AttachView::Lazy {
+                offset,
+                scope_id: cursor_scope,
+            })
+        };
 
-        // Top-level cursor: predecessors only, and own attaches narrowed to the
-        // calls that have run by `offset`.
-        let layers = self.cross_file_layers(db, CollationView::Eager);
-        let own = self.attach_layers(db, Some(offset));
+        let layers = self.cross_file_layers(db, collation);
+        let own = self.attach_layers(db, attaches);
         layers.splice_own_attaches(own)
     }
 
     /// This file's own `library()` / `require()` attaches as `Package` layers,
-    /// in LIFO order (latest-attached first). Reads the file's own semantic
-    /// index. `before` selects which calls to include:
-    ///
-    /// - `None`: every attach. The end-of-file view, used for lazy contexts.
-    /// - `Some(offset)`: only top-level (file-scope) calls that have run by
-    ///   `offset`. Calls nested in a block (e.g. inside `test_that({})`) are
-    ///   dropped, as are calls after the offset.
+    /// in LIFO order (latest-attached first), narrowed to what `view` admits.
+    /// Reads the file's own semantic index.
     ///
     /// An attach to a package absent from every root is dropped (no entity).
-    fn attach_layers(self, db: &dyn Db, before: Option<TextSize>) -> Vec<ImportLayer> {
+    fn attach_layers(self, db: &dyn Db, view: AttachView) -> Vec<ImportLayer> {
         let index = self.semantic_index(db);
-        let file_scope = ScopeId::from(0);
         index
             .semantic_calls()
             .iter()
             .rev()
-            .filter(|call| match before {
-                Some(offset) => call.scope() == file_scope && call.offset() < offset,
-                None => true,
-            })
             .filter_map(|call| match call.kind() {
-                SemanticCallKind::Attach { package } => {
-                    db.package_by_name(package).map(ImportLayer::Package)
-                },
+                SemanticCallKind::Attach { package, region } => Some((call, package, region)),
                 // A `library()` inside the sourced file is forwarded separately
                 // by the semantic index builder as its own `Attach`, scoped to
                 // this `source()`.
                 SemanticCallKind::Source { .. } => None,
             })
+            .filter(|(call, _, region)| view.sees(index, call, region))
+            .filter_map(|(_, package, _)| db.package_by_name(package).map(ImportLayer::Package))
             .collect()
     }