Skip to content

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
380 changes: 370 additions & 10 deletions crates/ark/src/lsp/diagnostics.rs

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion crates/ark/src/lsp/diagnostics_syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,10 +309,12 @@ mod tests {
use crate::lsp::diagnostics::DiagnosticContext;
use crate::lsp::diagnostics_syntax::syntax_diagnostics;
use crate::lsp::documents::Document;
use crate::lsp::inputs::library::Library;

fn text_diagnostics(text: &str) -> Vec<Diagnostic> {
let document = Document::new(text, None);
let context = DiagnosticContext::new(&document.contents);
let library = Library::default();
let context = DiagnosticContext::new(&document.contents, &library);
let diagnostics = syntax_diagnostics(document.ast.root_node(), &context).unwrap();
diagnostics
}
Expand Down
148 changes: 148 additions & 0 deletions crates/ark/src/lsp/inputs/library.rs
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>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub library_paths: Arc<Vec<PathBuf>>,
pub library_paths: Vec<PathBuf>,

Is it overkill to Arc this? Probably like 1 or 2 items right?


packages: Arc<RwLock<HashMap<String, Option<Arc<Package>>>>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
packages: Arc<RwLock<HashMap<String, Option<Arc<Package>>>>>,
packages: Arc<RwLock<HashMap<String, Option<Arc<Package>>>>>,

Why is Package itself Arced?

I was assuming that get() would return an immutable reference to a Package in the package list

i.e. what self.packages.read().unwrap().get(name) already returns

And if it's not cached yet, you write it to the cache but still return a get() reference

Then it feels like you don't need to Arc the Package because packages would be the sole owner

}

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Err case, we should probably bail with the error right?

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 library_paths element when that occurs

Copy link
Contributor

Choose a reason for hiding this comment

The 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 anyhow::Result<> here, as you never propagate an error right now

}

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"]);
}
}
12 changes: 12 additions & 0 deletions crates/ark/src/lsp/inputs/mod.rs
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;
59 changes: 59 additions & 0 deletions crates/ark/src/lsp/inputs/package.rs
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Path to the directory that contains `DESCRIPTION`. Could be an installed
/// package, or a package source.
/// Path to the directory that contains `DESCRIPTION` and `NAMESPACE`.
/// Could be an installed package, or a package source.

It looks like you need both, from the load() impl

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
path: package_path.to_path_buf(),
path: package_path,

Already a PathBuf

description,
namespace,
}))
}
}
Loading
Loading