-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: main
Are you sure you want to change the base?
Changes from all commits
292ba8a
32194b4
d2effa5
7fa77b8
042636a
81dfabc
1f9af53
ced4252
ad600e4
6fd8e7f
fa9a13c
b9b757e
4ea50a3
391138e
0d485d9
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,148 @@ | ||||||
// | ||||||
// 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 |
||||||
} | ||||||
|
||||||
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) { | ||||||
Ok(Some(pkg)) => return Ok(Some(pkg)), | ||||||
Ok(None) => (), | ||||||
Err(err) => lsp::log_warn!("Can't load package: {err:?}"), | ||||||
} | ||||||
Comment on lines
+70
to
+75
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. In the Right now I think this says "I found the package, but failed to load it" And IMO we should not continue on to the next 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. If you decide not to do that, note that you don't need the return value to be |
||||||
} | ||||||
|
||||||
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,59 @@ | ||||||||||
// | ||||||||||
// 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`. Could be an installed | ||||||||||
/// package, or a package source. | ||||||||||
Comment on lines
+17
to
+18
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
It looks like you need both, from the |
||||||||||
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? |
||||||||||
|
||||||||||
// 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_contents = fs::read_to_string(&namespace_path)?; | ||||||||||
let namespace = Namespace::parse(&namespace_contents)?; | ||||||||||
|
||||||||||
Ok(Some(Package { | ||||||||||
path: package_path.to_path_buf(), | ||||||||||
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
Already a PathBuf |
||||||||||
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?