Resolve names to all their candidate definitions

crates/ark/src/lsp/tests/goto_definition.rs

@@ -146,10 +146,10 @@ fn test_local_def_shadows_sourced() {
 }
 
 #[test]
-fn test_last_def_in_sourced_file_wins() {
-    // When the sourced file binds the same name twice, the use navigates to
-    // the last definition. The link range must point at that second def, in
-    // the target file's coordinates.
+fn test_sourced_file_with_repeated_def_offers_both() {
+    // When the sourced file binds the same name twice, goto-def offers both
+    // candidate definitions, in definition order. The last link is the binding
+    // R picks at runtime. Ranges are in the target file's coordinates.
     let script_uri = test_path("script.R");
     let helpers_uri = test_path("helpers.R");
     let state = make_state_with(&[
@@ -164,8 +164,11 @@ fn test_last_def_in_sourced_file_wins() {
     assert_matches!(
         goto_definition(params, &state).unwrap(),
         Some(GotoDefinitionResponse::Link(ref links)) => {
+            assert_eq!(links.len(), 2);
             assert_eq!(links[0].target_uri, helpers_uri);
-            assert_eq!(links[0].target_range, range((1, 0), (1, 2)));
+            assert_eq!(links[0].target_range, range((0, 0), (0, 2)));
+            assert_eq!(links[1].target_uri, helpers_uri);
+            assert_eq!(links[1].target_range, range((1, 0), (1, 2)));
         }
     );
 }

crates/oak_db/src/file_exports.rs

@@ -15,7 +15,7 @@ use crate::File;
 /// internal edits.
 #[derive(Debug, Default, Clone, PartialEq, Eq)]
 pub struct FileExports {
-    entries: HashMap<String, ExportEntry>,
+    entries: HashMap<String, Vec<ExportEntry>>,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -28,12 +28,16 @@ pub enum ExportEntry {
 }
 
 impl FileExports {
-    pub fn get(&self, name: &str) -> Option<&ExportEntry> {
-        self.entries.get(name)
+    /// Every entry bound to `name` in this file, deduplicated. A `Local`
+    /// marker appears at most once even when the name has several top-level
+    /// definitions; `File::resolve_export()` fans those back out by re-reading
+    /// the semantic index. `Import` entries are distinct per `(file, name)`.
+    pub fn get(&self, name: &str) -> Option<&[ExportEntry]> {
+        self.entries.get(name).map(Vec::as_slice)
     }
 
-    pub fn iter(&self) -> impl Iterator<Item = (&str, &ExportEntry)> {
-        self.entries.iter().map(|(k, v)| (k.as_str(), v))
+    pub fn iter(&self) -> impl Iterator<Item = (&str, &[ExportEntry])> {
+        self.entries.iter().map(|(k, v)| (k.as_str(), v.as_slice()))
     }
 
     pub fn len(&self) -> usize {
@@ -60,27 +64,42 @@ impl File {
     /// chains by returning empty exports for every cycle participant.
     #[salsa::tracked(returns(ref), cycle_result = exports_cycle_result)]
     pub fn exports(self, db: &dyn Db) -> FileExports {
-        let mut entries: HashMap<String, ExportEntry> = HashMap::new();
-        for (name, (_def_id, def)) in self.semantic_index(db).exports() {
-            let entry = match def.kind() {
-                DefinitionKind::Import {
-                    file: target_url,
-                    name: target_name,
-                    ..
-                } => {
-                    let target_url_id = FilePath::from_url(target_url);
-                    let Some(target_file) = db.file_by_url(&target_url_id) else {
-                        continue;
-                    };
-                    ExportEntry::Import {
-                        file: target_file,
-                        name: target_name.clone(),
-                    }
-                },
-                _ => ExportEntry::Local,
-            };
-            entries.insert(name.to_string(), entry);
+        let mut entries: HashMap<String, Vec<ExportEntry>> = HashMap::new();
+
+        for (name, defs) in self.semantic_index(db).exports() {
+            let list = entries.entry(name.to_string()).or_default();
+            for (_def_id, def) in defs {
+                let entry = match def.kind() {
+                    DefinitionKind::Import {
+                        file: target_url,
+                        name: target_name,
+                        ..
+                    } => {
+                        let target_url_id = FilePath::from_url(target_url);
+                        let Some(target_file) = db.file_by_url(&target_url_id) else {
+                            continue;
+                        };
+                        ExportEntry::Import {
+                            file: target_file,
+                            name: target_name.clone(),
+                        }
+                    },
+                    _ => ExportEntry::Local,
+                };
+                // Dedup, moving each entry to its last position so the final
+                // entry is the binding R picks at runtime (statements run in
+                // order, last write wins). `Local` carries no `def_id` (that
+                // contentlessness is what keeps `exports()` stable across body
+                // edits), so a name's several top-level definitions collapse to
+                // one `Local`, identical `source()` forwards to one `Import`,
+                // and `resolve_export` re-mints from the marker either way.
+                if let Some(pos) = list.iter().position(|e| e == &entry) {
+                    list.remove(pos);
+                }
+                list.push(entry);
+            }
         }
+
         FileExports { entries }
     }
 }

crates/oak_db/src/file_resolve.rs

@@ -36,7 +36,12 @@ impl<'db> File {
     ///    `Package` and `From` layers call [`Package::resolve`] with
     ///    `Exported` visibility.
     ///
-    /// The returned `Definition` is keyed by `(file, scope, name)`, so
+    /// Returns every definition the name reaches in the first layer that binds
+    /// it, so a name with two top-level bindings yields both. The own-file
+    /// `exports()` chain shadows imports, matching R: if the file binds the
+    /// name at top level we stop there and never fall through to a package.
+    ///
+    /// Each returned `Definition` is keyed by `(file, scope, name)`, so
     /// downstream queries that only depend on identity stay cached across
     /// edits that shift the binding's source position. Consumers that need
     /// a position or the bound expression read `def.kind(db)` and project
@@ -45,9 +50,10 @@ impl<'db> File {
     /// For the offset-aware, sequential-semantics variant, see
     /// [`File::resolve_at`].
     #[salsa::tracked]
-    pub fn resolve(self, db: &'db dyn Db, name: Name<'db>) -> Option<Definition<'db>> {
-        if let Some(def) = self.resolve_export(db, name) {
-            return Some(def);
+    pub fn resolve(self, db: &'db dyn Db, name: Name<'db>) -> Vec<Definition<'db>> {
+        let exported = self.resolve_export(db, name);
+        if !exported.is_empty() {
+            return exported;
         }
 
         // For each sibling `ImportLayer::File`, check the target's exports
@@ -65,37 +71,32 @@ impl<'db> File {
         for layer in self.imports(db) {
             match layer {
                 ImportLayer::File(target) => {
-                    if let Some(def) = target.resolve_export(db, name) {
-                        return Some(def);
+                    let defs = target.resolve_export(db, name);
+                    if !defs.is_empty() {
+                        return defs;
                     }
                 },
                 ImportLayer::Package(pkg) => {
-                    if let Some(def) = pkg
-                        .resolve(db, name, PackageVisibility::Exported)
-                        .into_iter()
-                        .next()
-                    {
-                        return Some(def);
+                    let defs = pkg.resolve(db, name, PackageVisibility::Exported);
+                    if !defs.is_empty() {
+                        return defs;
                     }
                 },
                 ImportLayer::From(map) => {
                     let name_str = name.text(db).as_str();
                     if let Some(pkg_name) = map.get(name_str) {
                         if let Some(pkg) = db.package_by_name(pkg_name) {
-                            if let Some(def) = pkg
-                                .resolve(db, name, PackageVisibility::Exported)
-                                .into_iter()
-                                .next()
-                            {
-                                return Some(def);
+                            let defs = pkg.resolve(db, name, PackageVisibility::Exported);
+                            if !defs.is_empty() {
+                                return defs;
                             }
                         }
                     }
                 },
             }
         }
 
-        None
+        Vec::new()
     }
 
     /// Resolve the name at `offset` to its definition(s).
@@ -136,15 +137,15 @@ impl<'db> File {
         if !reaching.is_empty() {
             return reaching
                 .into_iter()
-                .filter_map(|(scope, def_id)| self.resolve_definition(db, scope, def_id))
+                .flat_map(|(scope, def_id)| self.resolve_definition(db, scope, def_id))
                 .collect();
         }
 
         // Nothing local reaches the use, so resolve across files.
         let file_scope = ScopeId::from(0);
         if use_scope != file_scope {
             // Function body: the lazy / end-of-file view the body sees at run time.
-            return self.resolve(db, interned).into_iter().collect();
+            return self.resolve(db, interned);
         }
 
         // Top level: collation predecessors / other visible files (exports-only
@@ -153,8 +154,9 @@ impl<'db> File {
         for layer in self.imports_at(db, offset) {
             match layer {
                 ImportLayer::File(target) => {
-                    if let Some(def) = target.resolve_export(db, interned) {
-                        return vec![def];
+                    let defs = target.resolve_export(db, interned);
+                    if !defs.is_empty() {
+                        return defs;
                     }
                 },
                 ImportLayer::Package(pkg) => {
@@ -185,61 +187,100 @@ impl<'db> File {
         db: &'db dyn Db,
         scope: ScopeId,
         def_id: DefinitionId,
-    ) -> Option<Definition<'db>> {
+    ) -> Vec<Definition<'db>> {
         let index = self.semantic_index(db);
         if let DefinitionKind::Import {
             file: target_url,
             name: forwarded,
             ..
         } = index.definitions(scope)[def_id].kind()
         {
-            let target = db.file_by_url(&FilePath::from_url(target_url))?;
+            let Some(target) = db.file_by_url(&FilePath::from_url(target_url)) else {
+                return Vec::new();
+            };
             return target.resolve_export(db, Name::new(db, forwarded.as_str()));
         }
-        self.definition(db, scope, def_id)
+        self.definition(db, scope, def_id).into_iter().collect()
     }
 
-    /// Walk this file's exports chain for `name`, chasing `source()`-forwarded
-    /// `Import` entries through target exports until a `Local` is found. Cycles
-    /// resolve to `None` via `exports`'s `cycle_fn`.
+    /// Walk this file's exports for `name`, chasing `source()`-forwarded
+    /// `Import` entries through target exports until they land on `Local`
+    /// definitions. Returns every definition the name reaches, so a name with
+    /// two top-level bindings, or forwards from two different sourced files,
+    /// yields all of them.
+    ///
+    /// Order follows `exports()`: entries are visited in definition order and
+    /// imports are chased in place, so the last element is the binding R picks
+    /// at runtime (statements run in order, last write wins). Several top-level
+    /// locals collapse to one `Local` marker, so they come out grouped at that
+    /// marker's position rather than each at its own offset, but the runtime
+    /// winner is still last.
+    ///
+    /// Cycles resolve to empty via `exports`'s `cycle_fn`.
     #[salsa::tracked]
-    pub(crate) fn resolve_export(
+    pub(crate) fn resolve_export(self, db: &'db dyn Db, name: Name<'db>) -> Vec<Definition<'db>> {
+        let mut results = Vec::new();
+
+        // `visited` keys on `(file, name)` so each binding is minted once even
+        // when several forwards converge on it, and so an `Import` cycle (which
+        // `exports()`'s `cycle_result` already breaks) can't loop here. The
+        // `Rc<str>` is cheap to clone (refcount bump).
+        let mut visited: HashSet<(File, Rc<str>)> = HashSet::new();
+        self.collect_exports(
+            db,
+            Rc::from(name.text(db).as_str()),
+            &mut visited,
+            &mut results,
+        );
+
+        results
+    }
+
+    /// Append every definition `name` reaches in `self`'s exports to `results`,
+    /// in `exports()` order, recursing into `Import` forwards in place. See
+    /// [`File::resolve_export`] for the ordering contract.
+    fn collect_exports(
         self,
         db: &'db dyn Db,
-        name: Name<'db>,
-    ) -> Option<Definition<'db>> {
-        let mut current_file = self;
-        let mut current_name: Rc<str> = Rc::from(name.text(db).as_str());
-
-        // Defensive: cycle through `Import` is prevented upstream by
-        // `exports()`'s `cycle_result` (which returns empty for one cycle
-        // participant). The `Rc<str>` is cheap to clone (refcount bump).
-        let mut visited: HashSet<(File, Rc<str>)> = HashSet::new();
+        name: Rc<str>,
+        visited: &mut HashSet<(File, Rc<str>)>,
+        results: &mut Vec<Definition<'db>>,
+    ) {
+        if !visited.insert((self, name.clone())) {
+            return;
+        }
 
-        loop {
-            if !visited.insert((current_file, current_name.clone())) {
-                log::error!(
-                    "Internal error: Cycle through `Import` forwards while resolving \
-                    `{current_name}` in {url}.",
-                    url = current_file.url(db),
-                );
-                return None;
-            }
+        let Some(entries) = self.exports(db).get(name.as_ref()) else {
+            return;
+        };
 
-            match current_file.exports(db).get(current_name.as_ref())? {
+        for entry in entries {
+            match entry {
                 ExportEntry::Local => {
-                    // Look up the file-scope binding through the semantic index
-                    // to recover its `def_id`, then fetch the interned
-                    // definition. `exports()` returns the last-wins
-                    // definitions, so this is the right binding for the name.
-                    let index = current_file.semantic_index(db);
-                    let def_id = index.exports().get(current_name.as_ref())?.0;
-                    return current_file.definition(db, ScopeId::from(0), def_id);
+                    // The `Local` marker doesn't carry a `def_id`, so recover
+                    // every file-scope `def_id` for the name from the semantic
+                    // index and mint each through the single site. A name bound
+                    // more than once at top level fans out here.
+                    //
+                    // `exports()` also lists the `Import`-kind defs that
+                    // `source()` emits at file scope. Skip them: they're the
+                    // forwards already chased through the `Import` entries, and
+                    // minting one here would add a bogus target at the empty
+                    // `source()` call span.
+                    let index = self.semantic_index(db);
+                    for &(def_id, def) in index.exports().get(name.as_ref()).into_iter().flatten() {
+                        if matches!(def.kind(), DefinitionKind::Import { .. }) {
+                            continue;
+                        }
+                        results.extend(self.definition(db, ScopeId::from(0), def_id));
+                    }
                 },
 
-                ExportEntry::Import { file, name } => {
-                    current_file = *file;
-                    current_name = Rc::from(name.as_str());
+                ExportEntry::Import {
+                    file,
+                    name: forwarded,
+                } => {
+                    file.collect_exports(db, Rc::from(forwarded.as_str()), visited, results);
                 },
             }
         }