Surface diagnostics for ambiguous effects

Cargo.toml

@@ -31,6 +31,7 @@ aether_parser = { git = "https://github.com/posit-dev/air", package = "air_r_par
 aether_syntax = { git = "https://github.com/posit-dev/air", package = "air_r_syntax", rev = "d2659d5b158374bf486b594625ca50abbd0ac879" }
 aether_path = { path = "crates/aether_path" }
 amalthea = { path = "crates/amalthea" }
+annotate-snippets = "0.12.16"
 anyhow = "1.0.102"
 ark = { path = "crates/ark" }
 ark_macros = { path = "crates/ark_macros" }

crates/ark/src/lsp/analysis.rs

@@ -22,7 +22,6 @@ pub(crate) use pool::AnalysisPool;
 pub(crate) use refresh::DiagnosticsReady;
 pub(crate) use refresh::DiagnosticsState;
 pub(crate) use snapshot::WorldStateSnapshot;
-pub(crate) use warmup::warm_semantic_indexes;
 pub(crate) use warmup::warm_workspace_index;
 
 /// Run `f`, swallowing a salsa cancellation as `None`. Any other panic propagates.

crates/ark/src/lsp/analysis/refresh.rs

@@ -124,7 +124,7 @@ fn refresh_diagnostics(
     let now = std::time::Instant::now();
     lsp::log_info!("Generating diagnostics for file: {}", uri.as_str());
 
-    let diagnostics = generate_diagnostics(file.file(), state, testthat);
+    let diagnostics = generate_diagnostics(file.file(), state, testthat, &uri);
 
     lsp::log_info!(
         "Finished diagnostics for file: {} in {:.0?}",

crates/ark/src/lsp/analysis/warmup.rs

@@ -5,9 +5,6 @@
 //
 //
 
-use oak_db::all_used_files;
-use oak_db::warm_file;
-
 use super::pool::AnalysisPool;
 use crate::lsp;
 use crate::lsp::indexer;
@@ -33,21 +30,3 @@ pub(crate) fn warm_workspace_index(state: &WorldState, pool: &AnalysisPool) {
         lsp::log_info!("Finished workspace index warmup ({:.0?})", now.elapsed());
     })
 }
-
-/// Warm the oak `semantic_index` of every file the workspace depends on, on a
-/// background thread.
-///
-/// Idempotent, so re-running on every revision is cheap once a file's index is
-/// already warm (salsa cache hit). A concurrent write just cancels the
-/// in-flight warm; the next revision re-runs it, which is what carries warmup
-/// through the startup write-storm and warms a freshly-typed `pkg::`
-/// dependency as soon as its sources land.
-pub(crate) fn warm_semantic_indexes(state: &WorldState, pool: &AnalysisPool) {
-    pool.spawn(state.snapshot(), |snapshot| {
-        let now = std::time::Instant::now();
-        for &file in all_used_files(snapshot.db()) {
-            warm_file(snapshot.db(), file);
-        }
-        lsp::log_info!("Warmed semantic indexes ({:.0?})", now.elapsed());
-    })
-}

crates/ark/src/lsp/config.rs

@@ -27,6 +27,14 @@ pub static GLOBAL_SETTINGS: &[Setting<LspConfig>] = &[
                 .unwrap_or_else(|| DiagnosticsConfig::default().enable)
         },
     },
+    Setting {
+        key: "oak.diagnostics.experimental.enabled",
+        set: |cfg, v| {
+            cfg.diagnostics.experimental = v
+                .as_bool()
+                .unwrap_or_else(|| DiagnosticsConfig::default().experimental)
+        },
+    },
     Setting {
         key: "positron.r.symbols.includeAssignmentsInBlocks",
         set: |cfg, v| {

crates/ark/src/lsp/diagnostics.rs

@@ -9,16 +9,22 @@ use std::collections::BTreeMap;
 use std::collections::HashMap;
 use std::collections::HashSet;
 
+use aether_lsp_utils::proto::to_proto;
 use aether_lsp_utils::proto::PositionEncoding;
 use anyhow::bail;
 use anyhow::Result;
 use harp::syntax::is_valid_symbol;
 use harp::syntax::sym_quote_invalid;
 use oak_db::File;
 use oak_db::RootKind;
+use oak_db::Severity as OakSeverity;
 use stdext::*;
 use tower_lsp_server::ls_types::Diagnostic;
+use tower_lsp_server::ls_types::DiagnosticRelatedInformation;
 use tower_lsp_server::ls_types::DiagnosticSeverity;
+use tower_lsp_server::ls_types::Location;
+use tower_lsp_server::ls_types::NumberOrString;
+use tower_lsp_server::ls_types::Uri;
 use tree_sitter::Node;
 use tree_sitter::Point;
 use tree_sitter::Range;
@@ -41,13 +47,18 @@ use crate::treesitter::UnaryOperatorType;
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct DiagnosticsConfig {
     pub enable: bool,
+
+    /// Whether to publish diagnostics whose `DiagnosticKind::is_experimental()`
+    /// is `true`.
+    pub experimental: bool,
 }
 
 #[derive(Clone)]
 pub struct DiagnosticContext<'a> {
     pub(crate) db: &'a dyn ArkDb,
     pub(crate) file: File,
     pub(crate) encoding: PositionEncoding,
+    pub(crate) uri: &'a Uri,
 
     /// The symbols currently defined and available in the session.
     pub session_symbols: HashSet<String>,
@@ -75,7 +86,10 @@ pub struct DiagnosticContext<'a> {
 
 impl Default for DiagnosticsConfig {
     fn default() -> Self {
-        Self { enable: true }
+        Self {
+            enable: true,
+            experimental: false,
+        }
     }
 }
 
@@ -84,11 +98,17 @@ impl<'a> DiagnosticContext<'a> {
         self.file.source_text(self.db).as_str()
     }
 
-    pub(crate) fn new(db: &'a dyn ArkDb, file: File, encoding: PositionEncoding) -> Self {
+    pub(crate) fn new(
+        db: &'a dyn ArkDb,
+        file: File,
+        encoding: PositionEncoding,
+        uri: &'a Uri,
+    ) -> Self {
         Self {
             file,
             encoding,
             db,
+            uri,
             document_symbols: Vec::new(),
             session_symbols: HashSet::new(),
             workspace_symbols: HashSet::new(),
@@ -136,6 +156,7 @@ pub(crate) fn generate_diagnostics(
     file: File,
     state: WorldStateSnapshot,
     testthat: bool,
+    uri: &Uri,
 ) -> Vec<Diagnostic> {
     let mut diagnostics = Vec::new();
 
@@ -165,7 +186,7 @@ pub(crate) fn generate_diagnostics(
     }
 
     let encoding = state.config.position_encoding;
-    let mut context = DiagnosticContext::new(db, file, encoding);
+    let mut context = DiagnosticContext::new(db, file, encoding, uri);
 
     // Add a 'root' context for the document.
     context.document_symbols.push(HashMap::new());
@@ -251,9 +272,83 @@ pub(crate) fn generate_diagnostics(
         Err(err) => log::error!("Error while generating semantic diagnostics: {err:?}"),
     }
 
+    // Collect diagnostics from `oak_db`'s semantic index
+    match oak_diagnostics(&context, state.config.diagnostics.experimental) {
+        Ok(mut oak_diagnostics) => diagnostics.append(&mut oak_diagnostics),
+        Err(err) => log::error!("Error while generating oak diagnostics: {err:?}"),
+    }
+
     diagnostics
 }
 
+/// Convert `oak_db`'s file-level diagnostics into LSP diagnostics.
+fn oak_diagnostics(
+    context: &DiagnosticContext,
+    experimental: bool,
+) -> anyhow::Result<Vec<Diagnostic>> {
+    let mut diagnostics = Vec::new();
+
+    for diagnostic in context.file.diagnostics(context.db) {
+        if diagnostic.kind().is_experimental() && !experimental {
+            continue;
+        }
+
+        let range = to_proto::range(
+            diagnostic.range(),
+            context.file.line_index(context.db),
+            context.encoding,
+        )?;
+
+        let related_information = if diagnostic.annotations().is_empty() {
+            None
+        } else {
+            Some(
+                diagnostic
+                    .annotations()
+                    .iter()
+                    .map(|annotation| {
+                        let range = to_proto::range(
+                            annotation.range,
+                            context.file.line_index(context.db),
+                            context.encoding,
+                        )?;
+                        Ok(DiagnosticRelatedInformation {
+                            location: Location {
+                                uri: context.uri.clone(),
+                                range,
+                            },
+                            message: annotation.message.clone(),
+                        })
+                    })
+                    .collect::<anyhow::Result<Vec<_>>>()?,
+            )
+        };
+
+        diagnostics.push(Diagnostic {
+            range,
+            severity: Some(oak_severity_to_lsp(diagnostic.kind().severity())),
+            code: Some(NumberOrString::String(
+                diagnostic.kind().as_str().to_string(),
+            )),
+            source: Some("ark".to_string()),
+            message: diagnostic.message().to_string(),
+            related_information,
+            ..Default::default()
+        });
+    }
+
+    Ok(diagnostics)
+}
+
+fn oak_severity_to_lsp(severity: OakSeverity) -> DiagnosticSeverity {
+    match severity {
+        OakSeverity::Error => DiagnosticSeverity::ERROR,
+        OakSeverity::Warning => DiagnosticSeverity::WARNING,
+        OakSeverity::Info => DiagnosticSeverity::INFORMATION,
+        OakSeverity::Hint => DiagnosticSeverity::HINT,
+    }
+}
+
 fn semantic_diagnostics(
     root: Node,
     context: &mut DiagnosticContext,
@@ -1192,18 +1287,20 @@ mod tests {
 
     use crate::console::console_inputs;
     use crate::lsp::state::WorldState;
+    use crate::lsp::traits::url::UrlExt;
     use crate::r_task;
 
     fn generate_diagnostics(code: &str, state: &WorldState) -> Vec<lsp_types::Diagnostic> {
         let url = url::Url::parse("file:///test.R").unwrap();
+        let uri = url.to_uri().unwrap();
         let file = oak_db::File::new(
             &state.db,
             FilePath::from_url(&url),
             oak_db::FileRevision::zero(),
             Some(code.to_string()),
             None,
         );
-        super::generate_diagnostics(file, state.snapshot(), false)
+        super::generate_diagnostics(file, state.snapshot(), false, &uri)
     }
 
     fn current_state() -> WorldState {
@@ -1826,7 +1923,59 @@ foo
             ..Default::default()
         };
 
-        let diagnostics = super::generate_diagnostics(file, state.snapshot(), false);
+        let url = url::Url::parse("file:///a.R").unwrap();
+        let uri = url.to_uri().unwrap();
+        let diagnostics = super::generate_diagnostics(file, state.snapshot(), false, &uri);
         assert!(diagnostics.is_empty());
     }
+
+    #[test]
+    fn test_oak_diagnostics_ambiguous_effect_experimental() {
+        r_task(|| {
+            // `local`'s NSE reading could be shadowed by the later
+            // `local <- identity` at file scope, with undetermined timing
+            // relative to `f`'s call.
+            let text = "f <- function() local({ x <- 1 })\nlocal <- identity\n";
+
+            let mut state = current_state();
+            state.config.diagnostics.experimental = true;
+            let diagnostics = generate_diagnostics(text, &state);
+
+            // This snippet doesn't trip any of the legacy tree-sitter checks:
+            // `local` and `identity` both resolve to base R.
+            assert_eq!(diagnostics.len(), 1);
+            let diagnostic = &diagnostics[0];
+
+            assert_eq!(
+                diagnostic.code,
+                Some(lsp_types::NumberOrString::String(
+                    "ambiguous-effect".to_string()
+                ))
+            );
+            assert_eq!(diagnostic.range.start, Position::new(0, 16));
+            assert_eq!(diagnostic.range.end, Position::new(0, 33));
+            assert_eq!(diagnostic.source, Some("ark".to_string()));
+            assert_eq!(
+                diagnostic.severity,
+                Some(lsp_types::DiagnosticSeverity::INFORMATION)
+            );
+
+            let related = diagnostic.related_information.as_ref().unwrap();
+            assert_eq!(related.len(), 1);
+            assert_eq!(related[0].message, "could run before the call");
+            assert_eq!(related[0].location.range.start, Position::new(1, 0));
+            assert_eq!(related[0].location.range.end, Position::new(1, 5));
+        })
+    }
+
+    #[test]
+    fn test_oak_diagnostics_ambiguous_effect_disabled_by_default() {
+        r_task(|| {
+            let text = "f <- function() local({ x <- 1 })\nlocal <- identity\n";
+
+            let diagnostics = generate_diagnostics(text, &current_state());
+
+            assert!(diagnostics.is_empty());
+        })
+    }
 }

crates/ark/src/lsp/diagnostics_syntax.rs

@@ -445,7 +445,7 @@ mod tests {
 
     fn text_diagnostics(text: &str) -> Vec<Diagnostic> {
         let (db, open_file) = test_open_file(text);
-        let context = DiagnosticContext::new(&db, open_file.file(), ENCODING);
+        let context = DiagnosticContext::new(&db, open_file.file(), ENCODING, open_file.wire_uri());
         let diagnostics =
             syntax_diagnostics(open_file.tree_sitter(&db).root_node(), &context).unwrap();
         diagnostics

crates/ark/src/lsp/main_loop.rs

@@ -630,12 +630,6 @@ impl GlobalState {
                 &self.lsp_state.source_pool,
                 &self.events_tx,
             );
-
-            // Re-warm the oak semantic indexes on every revision, counting on
-            // idempotence (warm files are salsa cache hits). Takes care of
-            // warming up the initial workspace as well as any new dependency
-            // introduced by user edits.
-            analysis::warm_semantic_indexes(&self.world, &self.lsp_state.analysis_pool);
         }
 
         Ok(())