Feature flag the use of `oak_package_metadata` and `oak_sources` in `oak_index`?

[etiennebacher]

Hi, I'm experimenting with Claude to use the new Oak infrastructure in Jarl to get diagnostics on unused objects and unused function arguments (etiennebacher/jarl#478, etiennebacher/jarl#479). I noticed that pulling oak_index also adds a lot of dependencies from oak_package_metadata and oak_sources. I'm wondering if it is possible to put these imports behind a feature flag so that I have the option to not pull all those dependencies.

I don't think this requires large changes in oak_index, see below a patch generated by Claude. Using this patch locally removed 69 dependencies from Jarl (mostly because rustls isn't imported anymore) while keeping the key features of oak_index. I know this is still early stage and WIP on your side, but I wanted to raise this early in case it influences future development. Maybe you were already planning to address this in some way.

Thanks, and I'm really curious to see the future capabilities of Oak!

Claude patch

diff --git a/crates/oak_index/Cargo.toml b/crates/oak_index/Cargo.toml
index 817e9546..d5a1399a 100644
--- a/crates/oak_index/Cargo.toml
+++ b/crates/oak_index/Cargo.toml
@@ -12,6 +12,12 @@ license.workspace = true
 rust-version.workspace = true
 
 [features]
+default = ["package_metadata"]
+# Enables the `library`, `package`, `package_definitions`, `scope_layer`, and
+# `external` modules, which model installed R packages and cross-file scope
+# resolution. Disable for lighter consumers (e.g. linters running per-file
+# semantic analysis) that don't need installed-package lookups.
+package_metadata = ["dep:oak_package_metadata", "dep:oak_sources"]
 testing = ["dep:tempfile"]
 
 [lints]
@@ -27,13 +33,19 @@ itertools.workspace = true
 log.workspace = true
 oak_core.workspace = true
 oak_index_vec.workspace = true
-oak_package_metadata.workspace = true
-oak_sources.workspace = true
 rustc-hash.workspace = true
 smallvec.workspace = true
 stdext.workspace = true
 url.workspace = true
 
+[dependencies.oak_package_metadata]
+workspace = true
+optional = true
+
+[dependencies.oak_sources]
+workspace = true
+optional = true
+
 [dependencies.tempfile]
 workspace = true
 optional = true
diff --git a/crates/oak_index/src/lib.rs b/crates/oak_index/src/lib.rs
index 94301573..9f857358 100644
--- a/crates/oak_index/src/lib.rs
+++ b/crates/oak_index/src/lib.rs
@@ -1,8 +1,13 @@
 pub mod builder;
+#[cfg(feature = "package_metadata")]
 pub mod external;
+#[cfg(feature = "package_metadata")]
 pub mod library;
+#[cfg(feature = "package_metadata")]
 pub mod package;
+#[cfg(feature = "package_metadata")]
 pub mod package_definitions;
+#[cfg(feature = "package_metadata")]
 pub mod scope_layer;
 pub mod semantic_index;
 pub mod use_def_map;

[lionel-]

Just a heads up, things are changing fast. Making the oak_sources dependency behing a feature flag seems reasonable, but we'd probably like to see the infrastructure settle a bit before committing to that. Things are expected to slow down a lot around July. Can you maintain that patch on your side until then?

Regarding oak_package_metadata, that's a lightweight crate so I don't think we would add a feature flag for it.

I'm guessing you need cross-file resolution (from imported packages and sourced scripts) too for jarl? That will pull in the whole Salsa database (see recent PRs and incoming ones), which is an awkward fit for the batch linter, but would make sense in the LSP integration. Would still work transparently in batch mode though.

[lionel-]

but would make sense in the LSP integration

although in IDE context Oak and Jarl would then both try to emit the same unused/undefined lints, which would be awkward ;-)

[lionel-]

oh by the way getting the sources is not really optional to get accurate lints.

For instance if someone uses exportPattern() in NAMESPACE, we need the source code to determine objects defined in the namespace.

[lionel-]

oh or do you only need unused symbol diagnostics? In that case you don't need the imports / cross-file machinery and a light semantic index should be sufficient.

[etiennebacher]

Things are expected to slow down a lot around July. Can you maintain that patch on your side until then?

Sure, no problem about that, I was just experimenting and I understand it's quite unstable for now. I can just wait before merging this PR instead of maintaining a patch myself.

I'm guessing you need cross-file resolution (from imported packages and sourced scripts) too for jarl? That will pull in the whole Salsa database (see recent PRs and incoming ones)

I'm not sure about imported packages (mostly because I don't really know yet what package information is made available via Oak) but I do want the ability to get symbols in sourced scripts so that I don't flag objects as unused while they are used in sourced files. But in the experimental branch I have, I just get the content of source() and parse the corresponding file to import its symbols. I guess in this case I don't need to import the cross file resolution? It's probably better that I wait until it's more stable to determine whether I need this feature from Oak. My dependency concern was more related to web-oriented dependencies that I clearly don't need in jarl, like rustls.

although in IDE context Oak and Jarl would then both try to emit the same unused/undefined lints, which would be awkward ;-)

Yeah that would be duplicated, I don't think it's a big deal though. It's also useful to have this is in Jarl for cases where Ark isn't used or in CI.