Resolve symbols from attached and imported packages by lionel- · Pull Request #1258 · posit-dev/ark

lionel- · 2026-06-05T09:26:56Z

Branched from #1257
Progress towards #1212

File::resolve() and resolve_at() now handle the Package (NAMESPACE import()) and From (importFrom()) import layers (previously only File layers were walked; the rest were dropped).
Package::resolve() answers "is this name exported by this package?" by walking the package's files and collecting their exports. It provides a lazy / post-load view of package exports. For instance it returns two definitions when a top-level stub is overwritten in the .onLoad() hook via <<-.
Package::files are now stored in collation order, which simplifies downstream calls that need to respect R's load order.

File::imports() and Package::resolve() are both used in File::resolve() / File::resolve_at(), which themselves support goto-def, find-refs, and rename.

With these changes, when a workspace contains both scripts and a package, you can now use goto-def, find-refs, and rename with symbols imported via library().

goto-def will also work with installed packages when oak_sources is wired up again.

DavisVaughan · 2026-06-11T15:38:49Z

+/// Split the package's `R/*.R` files into `(loadable, leftover)` using the
+/// `Collate:` directive: `loadable` is the listed files in that order, and
+/// `leftover` is the R/ files not listed. Logs mismatches in either direction:
+/// `Collate:` entries with no file on disk, and R/ files absent from `Collate:`
+/// (which become standalone scripts, see [`read_workspace_package`]).
+///
+/// TODO(diagnostics): surface these as LSP diagnostics on `DESCRIPTION`
+/// instead of just log lines.


I question whether we should include them at all as standalone scripts vs just logging a diagnostic!

DavisVaughan · 2026-06-11T15:44:35Z

+    // `library(mypkg)` then a use of its exported `foo`. The use now resolves
+    // through the package layer to the installed-package binding, so rename
+    // refuses with the installed-package guard. Before package-layer resolution
+    // this use was unbound and errored with "no binding" instead.
+    let mut db = OakDatabase::new();
+    let _pkg_file =
+        install_library_package(&mut db, "mypkg", &["foo"], "a.R", "foo <- function() 42\n");
+    let script = upsert(&mut db, "script.R", "library(mypkg)\nfoo\n");
+
+    let use_start = "library(mypkg)\n".len() as u32;
+    let err = rename(&db, script, offset(use_start), "bar").unwrap_err();
+    assert!(err.to_string().contains("installed package"));


cool!

maybe a snapshot test for the error message?

DavisVaughan · 2026-06-11T15:46:27Z

+    // `library(mypkg)` where `mypkg` is a *workspace* package. Unlike an
+    // installed package, workspace files are editable, so rename must succeed
+    // and rewrite both the script use and the definition in the package file.
+    let mut db = OakDatabase::new();
+    let pkg_file =
+        install_workspace_package(&mut db, "mypkg", &["foo"], "a.R", "foo <- function() 42\n");
+    let script = upsert(&mut db, "script.R", "library(mypkg)\nfoo\n");
+
+    let use_start = "library(mypkg)\n".len() as u32;
+    let result = rename(&db, script, offset(use_start), "bar").unwrap();
+    assert_eq!(pairs(&result.ranges), vec![
+        (script, range(use_start, use_start + 3)),
+        (pkg_file, range(0, 3)),
+    ]);


ehhhh maybe?

any time i see library(mypkg) in a script, I assume that mypkg is installed and read only, even if im working on mypkg itself

i think the reason i would disagree with this is that a simple load_all() would not get things back in working order after the rename. with a library() call at the top, you really need to build and install and restart R first, then youll be back in working order.

DavisVaughan · 2026-06-11T15:48:45Z

+    assert_eq!(targets.len(), 1);
+    let target = &targets[0];
+
+    // Jumps into the package file, at the `foo` binding's coordinates.


DavisVaughan · 2026-06-11T15:50:14Z

+    let use_start = "library(mypkg)\n".len() as u32;
+    let refs = find_references(&db, script, offset(use_start), true);
+    assert_eq!(pairs(&refs), vec![
+        (script, range(use_start, use_start + 3)),
+        (pkg_file, range(0, 3)),
+    ]);
+}


maybe also include include_declarations: false to prove that the pkg_file reference isn't found in that case?

DavisVaughan · 2026-06-11T15:53:03Z

+    /// Returns a `Vec` because a package's namespace can carry more than one
+    /// binding per name. A common pattern is a top-level stub overridden from
+    /// an `.onLoad` hook via `<<-`:


But overwriting via <<- would never result in a 2nd top level binding, so how can it come back as a candidate?

Oh, or is it that <<- itself is special in the semantic index and registers foo as a top level binding even though its found in the onLoad scope?

DavisVaughan · 2026-06-11T15:56:53Z

+
+        let mut results = Vec::new();
+        for &file in self.files(db) {
+            results.extend(file.resolve_export(db, name));


Do re-exports work? i.e. does export(tibble) work when you just have tibble::tibble in the R file?
https://github.com/tidyverse/dplyr/blob/d5e94e7fa8fd4a5f79c1a707d1842216bb4c691f/R/reexport-tibble.R#L23

DavisVaughan · 2026-06-11T15:59:39Z

+                        .resolve(db, name, PackageVisibility::Exported)
+                        .into_iter()
+                        .next()
+                    {
+                        return Some(def);
+                    }
+                },
+                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()
+                            {


These use next() and only take the first hit, should this function return Vec<Definition>?

And if so, should there be corresponding tests somewhere that ensure that multiple definitions work?

ha, resolved by #1259

lionel- mentioned this pull request Jun 5, 2026

Resolve names to all their candidate definitions #1259

Open

lionel- force-pushed the oak/salsa-19-find-refs-rename-migration branch from 31b65be to 2fcc1d2 Compare June 11, 2026 12:39

lionel- force-pushed the oak/salsa-20-package-layers branch from 1b9493a to d9c64fe Compare June 11, 2026 12:39

lionel- added 2 commits June 11, 2026 15:13

Resolve symbols exported by attached packages

f8ea210

Resolve symbols from imported and attached packages

5ee8dea

lionel- force-pushed the oak/salsa-19-find-refs-rename-migration branch from 2fcc1d2 to 9b0c99e Compare June 11, 2026 13:15

lionel- added 2 commits June 11, 2026 15:18

Add workspace tests

8dd4c25

Fix rebase conflicts

c08723b

lionel- force-pushed the oak/salsa-20-package-layers branch from d9c64fe to c08723b Compare June 11, 2026 13:19

DavisVaughan approved these changes Jun 11, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve symbols from attached and imported packages#1258

Resolve symbols from attached and imported packages#1258
lionel- wants to merge 4 commits into
oak/salsa-19-find-refs-rename-migrationfrom
oak/salsa-20-package-layers

lionel- commented Jun 5, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

DavisVaughan Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lionel- commented Jun 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants