-
Notifications
You must be signed in to change notification settings - Fork 21
Static diagnostics for library()
and require()
calls
#870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
86605a4
adc048d
1f8b9ed
9eab565
714fa44
3854690
b23af64
3fa816a
8d2131d
128afee
a5395e2
90ea54b
e06e5d1
90e6604
4ca839c
1209357
d5ea6a9
9016fb5
7e3cb8d
144c4dc
56d34f0
1886249
e3c5529
249bc5c
1c5bae4
eb61a0f
dd9d846
541ddb2
72ed267
7f96aae
9f8277e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,147 @@ | ||||||
// | ||||||
// library.rs | ||||||
// | ||||||
// Copyright (C) 2025 by Posit Software, PBC | ||||||
// | ||||||
|
||||||
use std::collections::HashMap; | ||||||
use std::path::PathBuf; | ||||||
use std::sync::Arc; | ||||||
use std::sync::RwLock; | ||||||
|
||||||
use super::package::Package; | ||||||
use crate::lsp; | ||||||
|
||||||
/// Lazily manages a list of known R packages by name | ||||||
#[derive(Default, Clone, Debug)] | ||||||
pub struct Library { | ||||||
/// Paths to library directories, i.e. what `base::libPaths()` returns. | ||||||
pub library_paths: Arc<Vec<PathBuf>>, | ||||||
|
||||||
packages: Arc<RwLock<HashMap<String, Option<Arc<Package>>>>>, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Why is I was assuming that i.e. what And if it's not cached yet, you write it to the cache but still return a Then it feels like you don't need to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Packages are meant to be shared across threads so we can't use |
||||||
} | ||||||
|
||||||
impl Library { | ||||||
pub fn new(library_paths: Vec<PathBuf>) -> Self { | ||||||
Self { | ||||||
packages: Arc::new(RwLock::new(HashMap::new())), | ||||||
library_paths: Arc::new(library_paths), | ||||||
} | ||||||
} | ||||||
|
||||||
/// Get a package by name, loading and caching it if necessary. | ||||||
/// Returns `None` if the package can't be found or loaded. | ||||||
pub fn get(&self, name: &str) -> Option<Arc<Package>> { | ||||||
// Try to get from cache first (could be `None` if we already tried to | ||||||
// load a non-existent or broken package) | ||||||
if let Some(entry) = self.packages.read().unwrap().get(name) { | ||||||
return entry.clone(); | ||||||
} | ||||||
|
||||||
// Not cached, try to load | ||||||
let pkg = match self.load_package(name) { | ||||||
Ok(Some(pkg)) => Some(Arc::new(pkg)), | ||||||
Ok(None) => None, | ||||||
Err(err) => { | ||||||
lsp::log_error!("Can't load R package: {err:?}"); | ||||||
None | ||||||
}, | ||||||
}; | ||||||
|
||||||
self.packages | ||||||
.write() | ||||||
.unwrap() | ||||||
.insert(name.to_string(), pkg.clone()); | ||||||
|
||||||
pkg | ||||||
} | ||||||
|
||||||
/// Insert a package in the library for testing purposes. | ||||||
#[cfg(test)] | ||||||
pub fn insert(self, name: &str, package: Package) -> Self { | ||||||
self.packages | ||||||
.write() | ||||||
.unwrap() | ||||||
.insert(name.to_string(), Some(Arc::new(package))); | ||||||
self | ||||||
} | ||||||
|
||||||
fn load_package(&self, name: &str) -> anyhow::Result<Option<Package>> { | ||||||
for lib_path in self.library_paths.iter() { | ||||||
match Package::load(&lib_path, name)? { | ||||||
Some(pkg) => return Ok(Some(pkg)), | ||||||
None => (), | ||||||
} | ||||||
} | ||||||
|
||||||
Ok(None) | ||||||
} | ||||||
} | ||||||
|
||||||
#[cfg(test)] | ||||||
mod tests { | ||||||
use std::fs::File; | ||||||
use std::fs::{self}; | ||||||
use std::io::Write; | ||||||
|
||||||
use tempfile::TempDir; | ||||||
|
||||||
use super::*; | ||||||
|
||||||
// Helper to create a temporary package directory with DESCRIPTION and NAMESPACE | ||||||
fn create_temp_package( | ||||||
pkg_name: &str, | ||||||
description: &str, | ||||||
namespace: &str, | ||||||
) -> (TempDir, PathBuf) { | ||||||
let temp_dir = TempDir::new().unwrap(); | ||||||
let pkg_dir = temp_dir.path().join(pkg_name); | ||||||
fs::create_dir(&pkg_dir).unwrap(); | ||||||
|
||||||
let desc_path = pkg_dir.join("DESCRIPTION"); | ||||||
let mut desc_file = File::create(&desc_path).unwrap(); | ||||||
desc_file.write_all(description.as_bytes()).unwrap(); | ||||||
|
||||||
let ns_path = pkg_dir.join("NAMESPACE"); | ||||||
let mut ns_file = File::create(&ns_path).unwrap(); | ||||||
ns_file.write_all(namespace.as_bytes()).unwrap(); | ||||||
|
||||||
(temp_dir, pkg_dir) | ||||||
} | ||||||
|
||||||
#[test] | ||||||
fn test_load_and_cache_package() { | ||||||
let pkg_name = "mypkg"; | ||||||
let description = r#" | ||||||
Package: mypkg | ||||||
Version: 1.0 | ||||||
"#; | ||||||
let namespace = r#" | ||||||
export(foo) | ||||||
export(bar) | ||||||
importFrom(pkg, baz) | ||||||
"#; | ||||||
|
||||||
let (temp_dir, _pkg_dir) = create_temp_package(pkg_name, description, namespace); | ||||||
|
||||||
// Library should point to the temp_dir as its only library path | ||||||
let lib = Library::new(vec![temp_dir.path().to_path_buf()]); | ||||||
|
||||||
// First access loads from disk | ||||||
let pkg = lib.get(pkg_name).unwrap(); | ||||||
assert_eq!(pkg.description.name, "mypkg"); | ||||||
|
||||||
// Second access uses cache (note that we aren't testing that we are | ||||||
// indeed caching, just exercising the cache code path) | ||||||
assert!(lib.get(pkg_name).is_some()); | ||||||
|
||||||
// Negative cache: missing package | ||||||
assert!(lib.get("notapkg").is_none()); | ||||||
// Now cached as absent | ||||||
assert!(lib.get("notapkg").is_none()); | ||||||
|
||||||
// Namespace is parsed | ||||||
assert_eq!(pkg.namespace.exports, vec!["bar", "foo"]); | ||||||
assert_eq!(pkg.namespace.imports, vec!["baz"]); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// | ||
// mod.rs | ||
// | ||
// Copyright (C) 2025 by Posit Software, PBC | ||
// | ||
// | ||
|
||
pub mod library; | ||
pub mod package; | ||
pub mod package_description; | ||
pub mod package_namespace; | ||
pub mod source_root; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
// | ||
// package.rs | ||
// | ||
// Copyright (C) 2025 by Posit Software, PBC | ||
// | ||
// | ||
|
||
use std::fs; | ||
use std::path::PathBuf; | ||
|
||
use crate::lsp::inputs::package_description::Description; | ||
use crate::lsp::inputs::package_namespace::Namespace; | ||
|
||
/// Represents an R package and its metadata relevant for static analysis. | ||
#[derive(Clone, Debug)] | ||
pub struct Package { | ||
/// Path to the directory that contains `DESCRIPTION``. Can | ||
/// be an installed package or a package source. | ||
pub path: PathBuf, | ||
|
||
pub description: Description, | ||
pub namespace: Namespace, | ||
} | ||
|
||
impl Package { | ||
/// Attempts to load a package from the given path and name. | ||
pub fn load(lib_path: &std::path::Path, name: &str) -> anyhow::Result<Option<Self>> { | ||
let package_path = lib_path.join(name); | ||
|
||
let description_path = package_path.join("DESCRIPTION"); | ||
let namespace_path = package_path.join("NAMESPACE"); | ||
|
||
// Only consider libraries that have a folder named after the | ||
// requested package and that contains a description file | ||
if !description_path.is_file() { | ||
return Ok(None); | ||
} | ||
Comment on lines
+33
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You seem to also require a NAMESPACE below, so bail early here too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm actually we probably should be robust to missing Edit: I went ahead and made |
||
|
||
// This fails if there is no `Package` field, so we're never loading | ||
// folders like bookdown projects as package | ||
let description_contents = fs::read_to_string(&description_path)?; | ||
let description = Description::parse(&description_contents)?; | ||
|
||
if description.name != name { | ||
return Err(anyhow::anyhow!( | ||
"`Package` field in `DESCRIPTION` doesn't match folder name '{name}'" | ||
)); | ||
} | ||
|
||
let namespace = if namespace_path.is_file() { | ||
let namespace_contents = fs::read_to_string(&namespace_path)?; | ||
Namespace::parse(&namespace_contents)? | ||
} else { | ||
tracing::info!("Package `{name}` doesn't contain a NAMESPACE file, using defaults"); | ||
Namespace::default() | ||
}; | ||
|
||
Ok(Some(Package { | ||
path: package_path, | ||
description, | ||
namespace, | ||
})) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it overkill to Arc this? Probably like 1 or 2 items right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep I didn't Arc this to avoid cloning the data, but to express that this is a view into the worldstate. With an owned vector, the paths could have been modified along the way. Not sure it is that useful to make the distinction but that was the idea.