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!

I'm not a fan of rust-analyzer's behaviour where you get absolutely no LSP features when a file is not attached to a crate (generally due to a bug or latency when editing crate/module relationships). Classifying as standalone scripts strands the file, but at least you can still use internal navigation features etc.

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?

We're asserting strings rather than snapshot them

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.

hmm I'm not convinced by the argument that you need to reinstall. The LSP has a strictly static view of the world with the workspace package taking precedence over what's installed, that's its normal way of operating.

That said, it does feel a little safer to require first jumping to the definition of the symbol at point to rename, just as an extra safety measure. But also there's a convenience to just being able to rename functions from your scratch files, when you're working on your utility package (less so for a public CRAN package).

I'm on the fence too, just leaning towards that extra flexibility which is kind of cool.

I think my main point comes down to the fact that if I see library(mypkg), then I was expecting all LSP related features for mypkg to come from a version of it that lives in my .libPaths(), not one that comes from what is in my workspace.

(That would include go-to definition, which would jump to cached sources even though you are in the mypkg workspace. And rename, which would not be renamable on fn() of mypkg::fn())

I think my brain wants to trade "this is a cool feature if you're also in this package's workspace" for "i can always predict what this does really easily"

I think the current approach is really nice for the development of monorepos.

Another viewpoint is that the LSP should be consistent with what you can observe in the Console, but that becomes hard quickly (load_all, outdated package loaded, etc).

In comparison I submit that the "workspace entity has precedence" is a very predictable rule, even if surprising at first if it doesn't match your initial expectation.

A toml/yaml dependency file could clear it up, but it brings heavier machinery that is not guaranteed to pay off for the R audience.

Okay, as-is is fine with me

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: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?

Right <<- is additive, not overwriting, like when two arms of an if/else condition define the same name.

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

They did not! Now done, good catch

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- · 2026-06-16T08:37:54Z

Fix TODO(namespace-refs) notes from #1257 (comment)

lionel- mentioned this pull request Jun 5, 2026

Resolve names to all their candidate definitions #1259

Merged

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- force-pushed the oak/salsa-19-find-refs-rename-migration branch from 2fcc1d2 to 9b0c99e Compare June 11, 2026 13:15

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

lionel- force-pushed the oak/salsa-19-find-refs-rename-migration branch from 9b0c99e to 8363250 Compare June 15, 2026 14:30

lionel- force-pushed the oak/salsa-20-package-layers branch from c08723b to 92d4505 Compare June 16, 2026 07:25

lionel- mentioned this pull request Jun 16, 2026

Use Salsa infrastructure for find-references and rename #1257

Merged

lionel- force-pushed the oak/salsa-20-package-layers branch from 313d381 to ff149f2 Compare June 16, 2026 11:41

lionel- force-pushed the oak/salsa-19-find-refs-rename-migration branch from 1ab24cd to b09b25b Compare June 19, 2026 13:54

Base automatically changed from oak/salsa-19-find-refs-rename-migration to main June 19, 2026 13:54

lionel- added 12 commits June 19, 2026 15:55

Resolve symbols exported by attached packages

b8e49c9

Resolve symbols from imported and attached packages

6ae5496

Add workspace tests

560713a

Fix rebase conflicts

1abab5e

Test full error messages

e7425f2

Add missing tests

277ade9

Resolve package reexports

44c0075

Reformat

27ac5d4

Fix rebase conflicts

b2eabe0

Finish collecting qualified references

d95d493

Filter out installed packages from references

67a0e8b

Format

3209b47

lionel- force-pushed the oak/salsa-20-package-layers branch from 8055225 to 3209b47 Compare June 19, 2026 13:55

lionel- merged commit 24fff01 into main Jun 19, 2026
17 checks passed

lionel- deleted the oak/salsa-20-package-layers branch June 19, 2026 13:56

github-actions Bot locked and limited conversation to collaborators Jun 19, 2026

Conversation

lionel- commented Jun 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lionel- Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

DavisVaughan Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

lionel- commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lionel- Jun 16, 2026 •

edited

Loading

DavisVaughan Jun 16, 2026 •

edited

Loading