Add `oak_scan` crate with initial library scanning by lionel- · Pull Request #1243 · posit-dev/ark

lionel- · 2026-05-29T13:24:37Z

Branched from #1226
Progress towards #1212

Introduces oak_scan with a library scanner. The LSP layer calls (in the next PR) the scanner on startup with .libPaths(). Each libpath is set as a library root in oak_db.

oak_scan is the only writer of Salsa inputs in oak_db, and is in charge of maintaining invariants, such as making sure backpointers are up-to-date.

Library roots are static for now. Watching them for changes (package removed or installed) would require our own filesystem watcher. Since we don't need one for the workspace (we'll use LSP file events), the machinery would be a bit too heavy for what we would gain for it at this stage. For now the LSP can be restarted to register changes to libpaths.

DavisVaughan · 2026-05-29T17:36:48Z

+fn test_scan_multiple_library_paths_preserve_order() {
+    let tmp1 = tempfile::tempdir().unwrap();
+    let tmp2 = tempfile::tempdir().unwrap();
+    write_package(&tmp1.path().join("pkg1"), "pkg1", &[]);
+    write_package(&tmp2.path().join("pkg2"), "pkg2", &[]);
+    let mut db = OakDatabase::new();
+
+    let paths: Vec<PathBuf> = vec![tmp1.path().to_path_buf(), tmp2.path().to_path_buf()];
+    db.set_library_paths(&paths);
+
+    let roots = db.library_roots().roots(&db).clone();
+    assert_eq!(roots.len(), 2);
+    assert_eq!(roots[0].packages(&db)[0].name(&db), "pkg1");


Random thought

We have quite a few things in oak that are ordering dependent, but I always forget which is and is not order dependent

It feels like it would be cool to have SortedVec be returned here, or something kind of like it, where its clear that the result is sorted (then, of course, the question becomes "sorted by what"?).

But maybe LibraryRoots suffices, since its docs state that the order invariant is upheld there

The roots are not sorted lexicographically though, or against a well defined sort key that can be easily used from elsewhere, so SortedVec would be a bit weird. It's more like they are ordered than sorted. I agree LibraryRoots suffices as the upholder of the invariant.

DavisVaughan · 2026-05-29T17:38:15Z

Lets teach claude to always do tests/integration/ for all integration tests!

DavisVaughan · 2026-05-29T17:52:48Z

+/// Description of one R file the scanner wants to register.
+///
+/// `contents` is the on-disk snapshot at scan time. It's used as the
+/// initial content whenever the helper mints a new `File` entity, i.e.
+/// the first time a URL is seen, whether at the initial scan or on a
+/// later rescan that discovers a newly-created file.
+///
+/// If a `File` already exists at this URL (scanner-created from an
+/// earlier scan, or VFS-created via `didOpen`), the helpers reuse that
+/// entity and leave its content alone. `set_contents` (driven by the
+/// VFS) is the authoritative way to update content.
+#[derive(Clone, Debug)]
+pub struct FileEntry {


I had the thought "is it always an R file?"

It seems to be, so it might be nice if the name reflected this, maybe just RFile? Or RFileEntry.

But the generic FileEntry left me wondering if it was also used for other things besides just R files

On the other hand there is nothing constraining the type to an R file, it's just that we only use it for R ones.

DavisVaughan · 2026-05-29T17:54:53Z

+
+/// Extension methods on the database for scanner orchestration and
+/// placement-aware updates that don't have a natural `Root` receiver.
+pub trait DbExt: Db + DbInputs {


I would like to advocate for a more informative trait name than DbExt

Perhaps DbLibraryPaths or DbScan

Good idea, I like DbScan after the crate name.

DavisVaughan · 2026-06-08T15:02:01Z

+    for &pkg in &packages {
+        pkg.set_files(db).to(Vec::new());


Suggested change

for &pkg in &packages {

pkg.set_files(db).to(Vec::new());

for &package in &packages {

package.set_files(db).to(Vec::new());

The naming inconsistency hurts me 😆

yep I know, sorry. Using short names in small scopes and longer names in larger ones is my writing style (https://www.lysator.liu.se/c/pikestyle.html). Not sure how to resolve our difference on this :-)

DavisVaughan · 2026-06-08T15:09:21Z

+    /// dropped on a previous `set_*_paths` call, so the entity gets
+    /// reused on re-add. Analysis paths should not call this — they use
+    /// [`Db::package_by_name`] which is stale-blind.
+    fn package_by_url(&self, url: &UrlId) -> Option<Package>;


I have no idea if I should use this vs package_by_name based on the names alone 😢

I wonder if it can be package_by_url_including_stale() or something to really discourage usage of it

Or separate into package_by_url() and stale_package_by_url() for use by the scanner

I wonder if we are going to have to expose some ugly internals like this by having the scanner and db in separate crates :/

hmm good point.

I moved the by-url lookup to oak_scan, which is the only one that should be concerned with stale roots.

If an analysis crate has a URL for a package, they can look up the name on disk and use package_by_name(), which resolves the effective package for that name.

DavisVaughan · 2026-06-08T15:14:46Z

+    /// Stale roots are not included. Salsa-cached and invalidates when one of
+    /// `workspace_roots` / `library_roots` / `orphan_root` changes.
+    fn live_roots(&self) -> &[LiveRoot];


I would also accept Dead root instead of Stale root since we have terminology like:

live roots

"resurrect"ing of dead/stale roots

That'd be fine, just a bit grim to my taste 😄

DavisVaughan · 2026-06-08T15:23:15Z

+pub fn package_by_url_query(db: &dyn Db, url: &UrlId) -> Option<Package> {
+    for &root in db.live_roots() {
+        if let LiveRoot::Workspace(r) | LiveRoot::Library(r) = root {
+            if let Some(&pkg) = root_package_url_index(db, r).get(url) {
+                return Some(pkg);
+            }
        }
    }
-    for root in db.library_roots().roots(db) {
-        if let Some(&pkg) = root_package_index(db, *root).get(name) {
-            return Some(pkg);
+    stale_package_url_index(db).get(url).copied()


yea i would be very happy to see a pair of functions like

package_by_url_query() / stale_package_by_url_query()

live_package_by_url_query() / dead_package_by_url_query()

that feels less confusing to me in the long run, i feel like its a bit hard to tell exactly which set of roots are checked just from the function name. like, is the stale package set checked or not?

I think moving the lookup to oak_scan mostly resolves the confusion.

…:root`

lionel- mentioned this pull request May 29, 2026

Add workspace scanning to oak_scan and wire it to the LSP #1244

Open

DavisVaughan approved these changes Jun 8, 2026

View reviewed changes

lionel- added 11 commits June 10, 2026 12:05

Add oak_scan crate with initial library scanning

cf42831

Use setter approach and handle root eviction

f0e520b

Add db.live_roots() and db.root_by_package() and remove `Package:…

fa4e43d

…:root`

Fix root_depth() on Windows

b8405e8

Move tests to integration/

d38a1c2

Rename DbExt to DbScan

fd6b4bb

Improve comments

9bbab62

Reuse oak_core's is_r_file()

a8166f7

Address code review

88ddd72

Consolidate stale-related utils in oak_scan

79914f6

Make some queries pub(crate)

76a82b0

lionel- force-pushed the oak/salsa-11-scan-library branch from d50f230 to 76a82b0 Compare June 10, 2026 10:07

lionel- merged commit bb1e1a8 into main Jun 10, 2026
17 checks passed

lionel- deleted the oak/salsa-11-scan-library branch June 10, 2026 10:11

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

Conversation

lionel- commented May 29, 2026

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!

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants