Resolve symbols from attached and imported packages#1258
Conversation
31b65be to
2fcc1d2
Compare
1b9493a to
d9c64fe
Compare
2fcc1d2 to
9b0c99e
Compare
d9c64fe to
c08723b
Compare
| /// 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. |
There was a problem hiding this comment.
I question whether we should include them at all as standalone scripts vs just logging a diagnostic!
| // `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")); |
There was a problem hiding this comment.
cool!
maybe a snapshot test for the error message?
| // `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)), | ||
| ]); |
There was a problem hiding this comment.
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.
| assert_eq!(targets.len(), 1); | ||
| let target = &targets[0]; | ||
|
|
||
| // Jumps into the package file, at the `foo` binding's coordinates. |
| 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)), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
maybe also include include_declarations: false to prove that the pkg_file reference isn't found in that case?
| /// 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 `<<-`: |
There was a problem hiding this comment.
But overwriting via <<- would never result in a 2nd top level binding, so how can it come back as a candidate?
There was a problem hiding this comment.
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?
|
|
||
| let mut results = Vec::new(); | ||
| for &file in self.files(db) { | ||
| results.extend(file.resolve_export(db, name)); |
There was a problem hiding this comment.
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
| .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() | ||
| { |
There was a problem hiding this comment.
These use next() and only take the first hit, should this function return Vec<Definition>?
There was a problem hiding this comment.
And if so, should there be corresponding tests somewhere that ensure that multiple definitions work?
Branched from #1257
Progress towards #1212
File::resolve()andresolve_at()now handle thePackage(NAMESPACEimport()) andFrom(importFrom()) import layers (previously onlyFilelayers 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::filesare now stored in collation order, which simplifies downstream calls that need to respect R's load order.File::imports()andPackage::resolve()are both used inFile::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.