diff --git a/Cargo.lock b/Cargo.lock index b7eaa2dad..d9b6293af 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -332,6 +332,17 @@ dependencies = [ "libc", ] +[[package]] +name = "annotate-snippets" +version = "0.12.16" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f211a51805bc641f3ad5b7664c77d2547af685cc33b4cd8d31964027a46f13f1" +dependencies = [ + "anstyle", + "memchr", + "unicode-width 0.2.2", +] + [[package]] name = "anstream" version = "1.0.0" @@ -2549,11 +2560,13 @@ dependencies = [ "aether_path", "air_r_parser", "air_r_syntax", + "annotate-snippets", "biome_line_index", "biome_rowan", "camino", "compact_str", "filetime", + "insta", "log", "oak_core", "oak_package_metadata", diff --git a/Cargo.toml b/Cargo.toml index ecd27cf3d..a1e7b55b9 100644 --- a/Cargo.toml +++ b/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" } diff --git a/crates/ark/src/lsp/analysis.rs b/crates/ark/src/lsp/analysis.rs index 2e2877acf..6a372852e 100644 --- a/crates/ark/src/lsp/analysis.rs +++ b/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. diff --git a/crates/ark/src/lsp/analysis/refresh.rs b/crates/ark/src/lsp/analysis/refresh.rs index cfab9b3fb..cdd68af11 100644 --- a/crates/ark/src/lsp/analysis/refresh.rs +++ b/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?}", diff --git a/crates/ark/src/lsp/analysis/warmup.rs b/crates/ark/src/lsp/analysis/warmup.rs index 7772bfd4f..38466b415 100644 --- a/crates/ark/src/lsp/analysis/warmup.rs +++ b/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()); - }) -} diff --git a/crates/ark/src/lsp/config.rs b/crates/ark/src/lsp/config.rs index dfba452be..d6c6ca424 100644 --- a/crates/ark/src/lsp/config.rs +++ b/crates/ark/src/lsp/config.rs @@ -27,6 +27,14 @@ pub static GLOBAL_SETTINGS: &[Setting] = &[ .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| { diff --git a/crates/ark/src/lsp/diagnostics.rs b/crates/ark/src/lsp/diagnostics.rs index c73e22373..a66653477 100644 --- a/crates/ark/src/lsp/diagnostics.rs +++ b/crates/ark/src/lsp/diagnostics.rs @@ -9,6 +9,7 @@ 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; @@ -16,9 +17,14 @@ 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,6 +47,10 @@ 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)] @@ -48,6 +58,7 @@ 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, @@ -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 { 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> { + 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::>>()?, + ) + }; + + 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,10 +1287,12 @@ 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 { 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), @@ -1203,7 +1300,7 @@ mod tests { 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, ¤t_state()); + + assert!(diagnostics.is_empty()); + }) + } } diff --git a/crates/ark/src/lsp/diagnostics_syntax.rs b/crates/ark/src/lsp/diagnostics_syntax.rs index 2754367d8..db656298e 100644 --- a/crates/ark/src/lsp/diagnostics_syntax.rs +++ b/crates/ark/src/lsp/diagnostics_syntax.rs @@ -445,7 +445,7 @@ mod tests { fn text_diagnostics(text: &str) -> Vec { 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 diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index 110fce4d4..2ecd35ba4 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/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(()) diff --git a/crates/ark/src/lsp/tests/diagnostics.rs b/crates/ark/src/lsp/tests/diagnostics.rs index 77c04ace0..fe30e6c89 100644 --- a/crates/ark/src/lsp/tests/diagnostics.rs +++ b/crates/ark/src/lsp/tests/diagnostics.rs @@ -17,23 +17,24 @@ fn test_diagnostics_published_through_refresh_snapshot() { // Open an editor file with an undefined symbol, mirroring `did_open`. // `upsert_editor` pushes the contents into the oak and returns the // matching `File`, which `insert_open_file` stores as an `OpenFile`. - let uri = Url::parse("file:///test.R").unwrap(); + let url = Url::parse("file:///test.R").unwrap(); + let uri = url.to_uri().unwrap(); let code = "foo"; let file = state .db - .upsert_editor(FilePath::from_url(&uri), code.to_string()); - state.insert_open_file(uri.to_uri().unwrap(), FilePath::from_url(&uri), file, None); + .upsert_editor(FilePath::from_url(&url), code.to_string()); + state.insert_open_file(uri.clone(), FilePath::from_url(&url), file, None); // Mirror `DiagnosticsState::refresh_all`: fetch the `File` from the // live state, then hand the worker a snapshot. The snapshot's oak must // still serve that file. let file = state - .open_file(&FilePath::from_url(&uri)) + .open_file(&FilePath::from_url(&url)) .expect("file is open in live state") .file(); let snapshot = state.snapshot(); - generate_diagnostics(file, snapshot, false) + generate_diagnostics(file, snapshot, false, &uri) }); assert!(!diagnostics.is_empty()); diff --git a/crates/oak_db/Cargo.toml b/crates/oak_db/Cargo.toml index 15d2dfd6c..82f90f64b 100644 --- a/crates/oak_db/Cargo.toml +++ b/crates/oak_db/Cargo.toml @@ -34,5 +34,7 @@ stdext.workspace = true url.workspace = true [dev-dependencies] +annotate-snippets.workspace = true +insta.workspace = true oak_semantic = { workspace = true, features = ["salsa", "testing"] } tempfile.workspace = true diff --git a/crates/oak_db/src/diagnostic.rs b/crates/oak_db/src/diagnostic.rs new file mode 100644 index 000000000..a4506d47d --- /dev/null +++ b/crates/oak_db/src/diagnostic.rs @@ -0,0 +1,190 @@ +use biome_rowan::TextRange; +use oak_semantic::semantic_index::AmbiguityReason; +use oak_semantic::semantic_index::SemanticDiagnostic; + +/// A diagnostic derived from a file's semantic analysis. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Diagnostic { + kind: DiagnosticKind, + message: String, + range: TextRange, + annotations: Vec, +} + +impl Diagnostic { + pub(crate) fn new( + kind: DiagnosticKind, + message: String, + range: TextRange, + annotations: Vec, + ) -> Self { + Self { + kind, + message, + range, + annotations, + } + } + + pub fn kind(&self) -> DiagnosticKind { + self.kind + } + + pub fn message(&self) -> &str { + &self.message + } + + pub fn range(&self) -> TextRange { + self.range + } + + pub fn annotations(&self) -> &[Annotation] { + &self.annotations + } +} + +/// A secondary site that gives context for a diagnostic. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Annotation { + pub range: TextRange, + pub message: String, +} + +/// Identifies a diagnostic's kind. Drives its LSP `code`, severity, and +/// experimental status. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DiagnosticKind { + AmbiguousEffect, + AmbiguousAttachOrder, + UninstalledPackage, +} + +impl DiagnosticKind { + /// The stable string an LSP consumer reports as the diagnostic `code`. + pub fn as_str(&self) -> &'static str { + match self { + DiagnosticKind::AmbiguousEffect => "ambiguous-effect", + DiagnosticKind::AmbiguousAttachOrder => "ambiguous-attach-order", + DiagnosticKind::UninstalledPackage => "uninstalled-package", + } + } + + pub fn severity(&self) -> Severity { + match self { + DiagnosticKind::AmbiguousEffect => Severity::Info, + DiagnosticKind::AmbiguousAttachOrder => Severity::Info, + DiagnosticKind::UninstalledPackage => Severity::Warning, + } + } + + pub fn is_experimental(&self) -> bool { + match self { + DiagnosticKind::AmbiguousEffect => true, + DiagnosticKind::AmbiguousAttachOrder => true, + DiagnosticKind::UninstalledPackage => true, + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Severity { + Error, + Warning, + Info, + Hint, +} + +/// Lower one of `oak_semantic`'s raw diagnostic records into a `Diagnostic`. +pub(crate) fn lower_semantic_diagnostic(diagnostic: &SemanticDiagnostic) -> Diagnostic { + match diagnostic { + SemanticDiagnostic::AmbiguousEffect { + name, + call_range, + reason, + } => lower_ambiguous_effect(name, *call_range, reason), + SemanticDiagnostic::AmbiguousAttachOrder { packages, range } => { + lower_ambiguous_attach_order(packages, *range) + }, + SemanticDiagnostic::UninstalledPackage { package, range } => { + lower_uninstalled_package(package, *range) + }, + } +} + +/// The primary range is always the call site. The reason's competing site +/// becomes a single annotation. +fn lower_ambiguous_effect( + name: &str, + call_range: TextRange, + reason: &AmbiguityReason, +) -> Diagnostic { + let (message, annotation) = match reason { + AmbiguityReason::LazyShadow { overwrite_range } => ( + format!( + "Ambiguous reading of effectful `{name}()`. An assignment to `{name}` in an enclosing \ + scope could run before this call and change its effect." + ), + Annotation { + range: *overwrite_range, + message: "could run before the call".to_string(), + }, + ), + AmbiguityReason::ConditionalShadow { binding_range } => ( + format!( + "Ambiguous reading of effectful `{name}()`. A conditional assignment could shadow `{name}` \ + on some paths and change its effect." + ), + Annotation { + range: *binding_range, + message: format!("conditional assignment to `{name}`"), + }, + ), + AmbiguityReason::ConditionalAttach { + package, + attach_range, + } => ( + format!( + "Ambiguous reading of `{name}()`. The package `{package}` is conditionally attached and does not import `{name}` \ + across all paths." + ), + Annotation { + range: *attach_range, + message: format!("`{package}` attached only here"), + }, + ), + }; + + Diagnostic::new(DiagnosticKind::AmbiguousEffect, message, call_range, vec![ + annotation, + ]) +} + +/// The primary range covers the `if`. Both arms are inside it, so there is no +/// competing site to annotate. +fn lower_ambiguous_attach_order(packages: &[String], range: TextRange) -> Diagnostic { + let message = format!( + "Ambiguous attach order. The branches attach {packages} in different orders, so which \ + package masks the other depends on the branch taken.", + packages = packages + .iter() + .map(|package| format!("`{package}`")) + .collect::>() + .join(", ") + ); + + Diagnostic::new( + DiagnosticKind::AmbiguousAttachOrder, + message, + range, + Vec::new(), + ) +} + +fn lower_uninstalled_package(package: &str, range: TextRange) -> Diagnostic { + Diagnostic::new( + DiagnosticKind::UninstalledPackage, + format!("Package `{package}` is not installed. Language analysis will be incomplete."), + range, + Vec::new(), + ) +} diff --git a/crates/oak_db/src/file.rs b/crates/oak_db/src/file.rs index cccea2dba..1473b9c05 100644 --- a/crates/oak_db/src/file.rs +++ b/crates/oak_db/src/file.rs @@ -1,13 +1,11 @@ use std::fs; use aether_path::FilePath; -use biome_line_index::LineIndex; -use biome_rowan::TextRange; -use oak_semantic::semantic_index::AmbiguityReason; -use oak_semantic::semantic_index::SemanticDiagnostic; use oak_semantic::semantic_index::SemanticIndex; use crate::db::root_by_file; +use crate::diagnostic::lower_semantic_diagnostic; +use crate::diagnostic::Diagnostic; use crate::file_revision::report_untracked_if_zero; use crate::imports::SalsaImportsResolver; use crate::parse::OakParse; @@ -254,6 +252,23 @@ impl File { names } + /// Diagnostics derived from this file's semantic index. + /// + /// Not keyed on user configuration, so the memo isn't duplicated per + /// setting combination. Consumers filter on severity and + /// [`crate::DiagnosticId::is_experimental`] instead. + /// + /// Ranges shift whenever text above them changes, so unlike + /// `attached_packages()` and friends this query can't backdate. + #[salsa::tracked(returns(ref))] + pub fn diagnostics(self, db: &dyn Db) -> Vec { + self.semantic_index(db) + .diagnostics() + .iter() + .map(lower_semantic_diagnostic) + .collect() + } + /// The root containing this file, if any. /// /// Packaged files ask the db which live root holds the package via @@ -301,17 +316,6 @@ fn root_by_path(db: &dyn Db, path: &FilePath) -> Option { .map(|(_, r)| r) } -/// Warm the tracked queries an LSP request reads on `file`, so the first -/// request after a scan doesn't pay the cold build. -/// -/// Computing `imports()` builds the file's `semantic_index` and its cross-file -/// import view in one go; the file's collation predecessors get pulled in (and -/// primed shallow) as a side effect. Best-effort, meant to run off the request -/// thread once a scan settles. -pub fn warm_file(db: &dyn Db, file: File) { - file.imports(db); -} - /// Guard against stack overflow when `semantic_index` recurses across files. const STACK_RED_ZONE: usize = 1024 * 1024; const STACK_GROW_BY: usize = 8 * 1024 * 1024; @@ -335,79 +339,7 @@ fn build_semantic_index(file: File, db: &dyn Db) -> SemanticIndex { fn build_semantic_index_inner(file: File, db: &dyn Db) -> SemanticIndex { let parsed = file.parse(db); let resolver = SalsaImportsResolver::new(db, file); - let index = oak_semantic::build_index(&parsed.tree(), resolver); - - // TODO(diagnostics): Diagnostics are not surfaced yet, so log them for now. - // The builder is file-agnostic, so it carries them on the index and leaves - // the file reference to us. - let diagnostics = index.diagnostics(); - if !diagnostics.is_empty() { - let path = file.path(db); - let line_index = file.line_index(db); - - for diagnostic in diagnostics { - if let SemanticDiagnostic::AmbiguousAttachOrder { packages, range } = diagnostic { - let at = format_line_col(line_index, *range); - log::warn!( - "Ambiguous attach order in {path}:{at}: the branches attach {packages} in \ - different orders.", - packages = packages.join(", ") - ); - continue; - } - - let SemanticDiagnostic::EffectAmbiguity { - name, - call_range, - reason, - } = diagnostic - else { - continue; - }; - let call = format_line_col(line_index, *call_range); - - match reason { - AmbiguityReason::LazyShadow { overwrite_range } => { - let overwrite = format_line_col(line_index, *overwrite_range); - log::warn!( - "Lazy-shadow ambiguity in {path}:{call}: callee `{name}` is recognized \ - as effectful, but a lazy-crossed ancestor binds it at {overwrite} with \ - undetermined timing" - ) - }, - AmbiguityReason::ConditionalShadow { binding_range } => { - let binding = format_line_col(line_index, *binding_range); - log::warn!( - "Conditional-shadow ambiguity in {path}:{call}: callee `{name}` is \ - recognized as effectful, but a conditional local binding at {binding} \ - could shadow it on some path" - ) - }, - AmbiguityReason::ConditionalAttach { - package, - attach_range, - } => { - let attach = format_line_col(line_index, *attach_range); - log::warn!( - "Conditional-attach ambiguity in {path}:{call}: callee `{name}` is read as \ - plain because `{package}`, attached at {attach}, dropped at a branch or \ - loop join. It would be effectful on the path where that attach ran" - ) - }, - } - } - } - - index -} - -/// Render a byte range as `line:col` (1-based), anchored at its start, for a log -/// message. Falls back to the raw byte range if the offset can't be mapped. -fn format_line_col(line_index: &LineIndex, range: TextRange) -> String { - match line_index.line_col(range.start()) { - Some(pos) => format!("{}:{}", pos.line + 1, pos.col + 1), - None => format!("{range:?}"), - } + oak_semantic::build_index(&parsed.tree(), resolver) } fn semantic_index_cycle_result(db: &dyn Db, _id: salsa::Id, file: File) -> SemanticIndex { diff --git a/crates/oak_db/src/imports.rs b/crates/oak_db/src/imports.rs index d597f5264..05f53a638 100644 --- a/crates/oak_db/src/imports.rs +++ b/crates/oak_db/src/imports.rs @@ -126,6 +126,10 @@ impl<'db> ImportsResolver for SalsaImportsResolver<'db> { self.cache.insert(name, attached, effects); effects } + + fn package_exists(&mut self, package: &str) -> bool { + self.db.package_by_name(package).is_some() + } } /// What a package layer contributes for `name` as the walk reaches it. diff --git a/crates/oak_db/src/lib.rs b/crates/oak_db/src/lib.rs index 2d147593c..947637660 100644 --- a/crates/oak_db/src/lib.rs +++ b/crates/oak_db/src/lib.rs @@ -1,5 +1,6 @@ mod db; mod definition; +mod diagnostic; mod file; mod file_exports; mod file_imports; @@ -25,7 +26,10 @@ pub use db::workspace_files; pub use db::Db; pub use db::DbInputs; pub use definition::Definition; -pub use file::warm_file; +pub use diagnostic::Annotation; +pub use diagnostic::Diagnostic; +pub use diagnostic::DiagnosticKind; +pub use diagnostic::Severity; pub use file::File; pub use file_exports::ExportEntry; pub use file_exports::FileExports; diff --git a/crates/oak_db/src/tests.rs b/crates/oak_db/src/tests.rs index 2e13f93e7..989946dc9 100644 --- a/crates/oak_db/src/tests.rs +++ b/crates/oak_db/src/tests.rs @@ -1,5 +1,7 @@ mod db; +mod diagnostic_render; mod file; +mod file_diagnostics; mod file_exports; mod file_imports; mod file_imports_at; diff --git a/crates/oak_db/src/tests/diagnostic_render.rs b/crates/oak_db/src/tests/diagnostic_render.rs new file mode 100644 index 000000000..30e5733d9 --- /dev/null +++ b/crates/oak_db/src/tests/diagnostic_render.rs @@ -0,0 +1,67 @@ +//! Renders `Diagnostic`s alongside the source they annotate, for use in +//! `insta` snapshot tests. Built on `annotate-snippets`, the same rustc-style +//! renderer ty and ruff use. + +use std::ops::Range; + +use annotate_snippets::AnnotationKind; +use annotate_snippets::Group; +use annotate_snippets::Level; +use annotate_snippets::Renderer; +use annotate_snippets::Snippet; +use biome_rowan::TextRange; + +use crate::Diagnostic; +use crate::Severity; + +/// Render each of `diagnostics` as its own rustc-style report against +/// `source`, joined by blank lines. `name` is the file name shown in each +/// report's `-->` locus (typically `"a.R"`). +pub(super) fn render(name: &str, source: &str, diagnostics: &[Diagnostic]) -> String { + if diagnostics.is_empty() { + return format!("{name}\n(no diagnostics)"); + } + + let renderer = Renderer::plain(); + diagnostics + .iter() + .map(|diagnostic| renderer.render(&[group_for(name, source, diagnostic)])) + .collect::>() + .join("\n") +} + +/// Build one report `Group`: a title carrying the diagnostic's kind and +/// message, and a snippet with the call site as the primary annotation and +/// each `Annotation` as a labeled secondary one. +fn group_for<'a>(name: &'a str, source: &'a str, diagnostic: &'a Diagnostic) -> Group<'a> { + let title = level_for(diagnostic.kind().severity()) + .primary_title(diagnostic.message()) + .id(diagnostic.kind().as_str()); + + let mut snippet = Snippet::source(source) + .path(name) + .annotation(AnnotationKind::Primary.span(byte_range(diagnostic.range()))); + + for annotation in diagnostic.annotations() { + snippet = snippet.annotation( + AnnotationKind::Context + .span(byte_range(annotation.range)) + .label(annotation.message.as_str()), + ); + } + + title.element(snippet) +} + +fn level_for(severity: Severity) -> Level<'static> { + match severity { + Severity::Error => Level::ERROR, + Severity::Warning => Level::WARNING, + Severity::Info => Level::INFO, + Severity::Hint => Level::HELP, + } +} + +fn byte_range(range: TextRange) -> Range { + usize::from(range.start())..usize::from(range.end()) +} diff --git a/crates/oak_db/src/tests/file_diagnostics.rs b/crates/oak_db/src/tests/file_diagnostics.rs new file mode 100644 index 000000000..ead811abd --- /dev/null +++ b/crates/oak_db/src/tests/file_diagnostics.rs @@ -0,0 +1,320 @@ +//! Snapshot tests for diagnostics rendering. Each test is one case, with a +//! comment explaining whether it's correct-by-design or a known gap. + +use crate::tests::diagnostic_render::render; +use crate::tests::resolver::install_packages; +use crate::tests::test_db::file_path; +use crate::tests::test_db::TestDb; +use crate::File; +use crate::FileRevision; + +fn new_file(db: &TestDb, name: &str, contents: &str) -> File { + File::new( + db, + file_path(name), + FileRevision::zero(), + Some(contents.to_string()), + None, + ) +} + +#[test] +fn test_diagnostic_ambiguous_attach_order() { + // The arms attach the same two packages in opposite orders, so which one + // masks the other after the `if` depends on the branch taken. Flagged + // even though `cli` and `rlang` share no names today. + let mut db = TestDb::new(); + install_packages(&mut db, &["cli", "rlang"]); + let source = "\ +if (cond) { + library(cli) + library(rlang) +} else { + library(rlang) + library(cli) +} +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_argument_matching_no_scope_silent() { + // Correct by design, silent. `test_that(1)` has no second argument at + // all, so there's no `code` block to scope on either path. The + // conditional `test_that <- identity` has nothing to shadow. + let mut db = TestDb::new(); + install_packages(&mut db, &["testthat"]); + let source = "\ +library(testthat) +if (cond) { + test_that <- identity +} +test_that(1) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_conditional_attach_branch_join() { + // `library(shiny)` attaches on only the `if` path, so the attach drops at + // the join and `reactive()` reads as plain. `reactive`'s NSE annotation + // comes from the static effect registry keyed on the package name + // `shiny`, which needs `shiny` to resolve as an installed package. + // Unlike base functions, non-base packages have no hardcoded fallback. + let mut db = TestDb::new(); + install_packages(&mut db, &["shiny"]); + let source = "\ +if (cond) library(shiny) +reactive({ + x <- 1 +}) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_conditional_attach_wins_over_shadow() { + // The `if` branch attaches shiny and the `else` branch conditionally + // shadows `reactive`, so both branches create a join-scope ambiguity. + // Only the attach diagnostic fires for this call. The shadow diagnostic + // doesn't also pile on. + let mut db = TestDb::new(); + install_packages(&mut db, &["shiny"]); + let source = "\ +if (sample(0:1, 1)) { + library(shiny) +} else { + reactive <- identity +} +reactive(1) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_conditional_shadow_definite_silent() { + // Correct by design, silent. `local` is reassigned before the call on + // the only path there is, so by the time `local({ x <- 1 })` runs, + // `local` is plain `identity()`. There's no NSE effect left to be + // ambiguous about. + let db = TestDb::new(); + let source = "\ +local <- identity +local({ x <- 1 }) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_conditional_shadow_eager() { + // The inner `local({ y <- 1 })` is flagged because `local` is + // conditionally reassigned earlier in the same outer scope, which is an + // eager NSE scope. That conditional binding could shadow the inner + // call, so it's ambiguous. This uses base `local()`, so no package + // fixture is needed. + let db = TestDb::new(); + let source = "\ +local({ + if (cond) local <- identity + local({ + y <- 1 + }) +}) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_conditional_shadow_package_call() { + // A conditional reassignment of `test_that` earlier in the same eager + // scope makes the later `test_that(...)` call ambiguous, the same shape + // as the base `local()` case, but through a real package effect + // (`testthat::test_that()`) instead of the hardcoded base fallback. + let mut db = TestDb::new(); + install_packages(&mut db, &["testthat"]); + let source = "\ +library(testthat) +if (cond) { + test_that <- identity +} +test_that(\"d\", { x <- 1 }) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_gap_conditional_shadow_enclosing_scope() { + // Known gap, silent. `local` is conditionally shadowed at file scope, + // and the inner `local({ y <- 1 })` runs inside `with()`'s eager nested + // scope, so the shadow really can reach it. But + // `record_conditional_shadow_ambiguity()` in + // `crates/oak_semantic/src/builder/scan.rs` only checks the current + // scan scope, which is `with()`'s body, not the file scope where the + // conditional assignment actually lives, so it misses this case. + let db = TestDb::new(); + let source = "\ +if (cond) local <- identity +with(d, { + local({ y <- 1 }) +}) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_gap_lazy_sibling_attach() { + // Known gap, silent. `g`'s `library(shiny)` never runs, since nothing + // calls `g`. Even if it did, `record_conditional_attach_ambiguity()`'s + // call-site probe only sees attaches reachable from its own scan, so it + // can't tell that `f`'s `reactive()` might one day run after `g`. + // Catching this needs a whole-file post-pass over lazy contexts, not a + // call-site probe (see the doc comment on + // `record_conditional_attach_ambiguity()` in + // `crates/oak_semantic/src/builder/effects.rs`). + let mut db = TestDb::new(); + install_packages(&mut db, &["shiny"]); + let source = "\ +g <- function() library(shiny) +f <- function() reactive({ x <- 1 }) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_gap_named_arg_before_block() { + // R matches named arguments first, so `desc = "d"` binds to the `desc` + // formal, and the unnamed block then fills the remaining `code` formal, + // which is formal position 1, even though the block sits at call + // position 0. `match_positional()` in `crates/oak_semantic/src/effects.rs` + // only matches a positional argument to a formal declared at that exact + // call position, so it never finds `code` here and no scope gets pushed + // for `x <- 1`. Confirmed by direct comparison: this source yields one + // scope, versus two for the same call with `code` in its normal + // position, so `x` resolves at file scope instead of inside + // `test_that()`. + let mut db = TestDb::new(); + install_packages(&mut db, &["testthat"]); + let source = "\ +library(testthat) +if (cond) { + test_that <- identity +} +test_that({ x <- 1 }, desc = \"d\") +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_installed_package_silent() { + // Correct by design, silent. `shiny` is registered as installed, so the + // attach resolves and there's nothing to report about it. + let mut db = TestDb::new(); + install_packages(&mut db, &["shiny"]); + let source = "library(shiny)\n"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_lazy_shadow_interleaved() { + // `f`'s body calls both `with()` and a nested `local()`, and both + // symbols are reassigned afterwards at file scope, so each call is + // flagged for the same lazy-timing reason: an assignment at file scope + // that might run before the call. The two calls share a line, so this + // also checks marker ordering: the outer `with()` call starts before + // the nested `local()` call, and both marker rows must appear in that + // order. + let db = TestDb::new(); + let source = "\ +f <- function() with(local({ x <- 1 }), { y <- 2 }) +local <- identity +with <- identity +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_lazy_shadow_reassignment() { + // `f` is never called, so there's no way to know whether + // `local <- identity` at file scope would run before or after `f`'s + // eventual call. That undetermined timing is why the inner + // `local({ x <- 1 })` is flagged. This exercises base's `local()` NSE + // annotation, which resolves without any package fixture: + // `SalsaImportsResolver` falls back to a static base registry when + // nothing else applies. + let db = TestDb::new(); + let source = "\ +f <- function() local({ x <- 1 }) +local <- identity +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_unconditional_attach_silent() { + // Correct by design, silent. `library(shiny)` attaches on every path, + // so `reactive()` is unambiguously NSE. Nothing competes with it. + let mut db = TestDb::new(); + install_packages(&mut db, &["shiny"]); + let source = "\ +library(shiny) +reactive({ x <- 1 }) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_uninstalled_package_conditional() { + // Same source as the conditional-attach-at-a-branch-join case, but this + // time `shiny` is never registered as an installed package, deliberately. + // Without a resolvable package, `reactive` never gets an NSE annotation in + // the first place, so there's no attach to be conditional about, and the + // ambiguity diagnostic can't fire. We report the uninstalled package + // instead, so the user still gets a signal that analysis is degraded here. + let db = TestDb::new(); + let source = "\ +if (cond) library(shiny) +reactive({ + x <- 1 +}) +"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} + +#[test] +fn test_diagnostic_uninstalled_package_unconditional() { + let db = TestDb::new(); + let source = "library(shiny)\n"; + let file = new_file(&db, "a.R", source); + + insta::assert_snapshot!(render("a.R", source, file.diagnostics(&db))); +} diff --git a/crates/oak_db/src/tests/resolver.rs b/crates/oak_db/src/tests/resolver.rs index b204d3698..6833a09b3 100644 --- a/crates/oak_db/src/tests/resolver.rs +++ b/crates/oak_db/src/tests/resolver.rs @@ -115,7 +115,7 @@ fn setup_testthat(db: &mut TestDb, scripts: &[(&str, &str)]) -> Vec { /// Register bare installed packages (empty namespace) on `LibraryRoots`, one /// root each, so `package_by_name` finds them. Their NSE effects come from the /// static registry keyed on the name, so no namespace is needed here. -fn install_packages(db: &mut TestDb, names: &[&str]) { +pub(super) fn install_packages(db: &mut TestDb, names: &[&str]) { let roots: Vec = names .iter() .map(|&name| { diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_ambiguous_attach_order.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_ambiguous_attach_order.snap new file mode 100644 index 000000000..1741c790a --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_ambiguous_attach_order.snap @@ -0,0 +1,15 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +info[ambiguous-attach-order]: Ambiguous attach order. The branches attach `rlang`, `cli` in different orders, so which package masks the other depends on the branch taken. + --> a.R:1:1 + | +1 | / if (cond) { +2 | | library(cli) +3 | | library(rlang) +4 | | } else { +5 | | library(rlang) +6 | | library(cli) +7 | | } + | |_^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_argument_matching_no_scope_silent.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_argument_matching_no_scope_silent.snap new file mode 100644 index 000000000..879ba7dc2 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_argument_matching_no_scope_silent.snap @@ -0,0 +1,6 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +a.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_attach_branch_join.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_attach_branch_join.snap new file mode 100644 index 000000000..75981c4fa --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_attach_branch_join.snap @@ -0,0 +1,13 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +info[ambiguous-effect]: Ambiguous reading of `reactive()`. The package `shiny` is conditionally attached and does not import `reactive` across all paths. + --> a.R:2:1 + | +1 | if (cond) library(shiny) + | -------------- `shiny` attached only here +2 | / reactive({ +3 | | x <- 1 +4 | | }) + | |__^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_attach_wins_over_shadow.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_attach_wins_over_shadow.snap new file mode 100644 index 000000000..ee2ed8fdc --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_attach_wins_over_shadow.snap @@ -0,0 +1,12 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +info[ambiguous-effect]: Ambiguous reading of `reactive()`. The package `shiny` is conditionally attached and does not import `reactive` across all paths. + --> a.R:6:1 + | +2 | library(shiny) + | -------------- `shiny` attached only here +... +6 | reactive(1) + | ^^^^^^^^^^^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_shadow_definite_silent.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_shadow_definite_silent.snap new file mode 100644 index 000000000..879ba7dc2 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_shadow_definite_silent.snap @@ -0,0 +1,6 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +a.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_shadow_eager.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_shadow_eager.snap new file mode 100644 index 000000000..ac4749a9f --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_shadow_eager.snap @@ -0,0 +1,13 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +info[ambiguous-effect]: Ambiguous reading of effectful `local()`. A conditional assignment could shadow `local` on some paths and change its effect. + --> a.R:3:5 + | +2 | if (cond) local <- identity + | ----- conditional assignment to `local` +3 | / local({ +4 | | y <- 1 +5 | | }) + | |______^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_shadow_package_call.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_shadow_package_call.snap new file mode 100644 index 000000000..42b040f18 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_conditional_shadow_package_call.snap @@ -0,0 +1,12 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +info[ambiguous-effect]: Ambiguous reading of effectful `test_that()`. A conditional assignment could shadow `test_that` on some paths and change its effect. + --> a.R:5:1 + | +3 | test_that <- identity + | --------- conditional assignment to `test_that` +4 | } +5 | test_that("d", { x <- 1 }) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_conditional_shadow_enclosing_scope.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_conditional_shadow_enclosing_scope.snap new file mode 100644 index 000000000..879ba7dc2 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_conditional_shadow_enclosing_scope.snap @@ -0,0 +1,6 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +a.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_lazy_sibling_attach.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_lazy_sibling_attach.snap new file mode 100644 index 000000000..879ba7dc2 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_lazy_sibling_attach.snap @@ -0,0 +1,6 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +a.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_named_arg_before_block.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_named_arg_before_block.snap new file mode 100644 index 000000000..879ba7dc2 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_gap_named_arg_before_block.snap @@ -0,0 +1,6 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +a.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_installed_package_silent.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_installed_package_silent.snap new file mode 100644 index 000000000..879ba7dc2 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_installed_package_silent.snap @@ -0,0 +1,6 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +a.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_lazy_shadow_interleaved.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_lazy_shadow_interleaved.snap new file mode 100644 index 000000000..4869d043e --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_lazy_shadow_interleaved.snap @@ -0,0 +1,19 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +info[ambiguous-effect]: Ambiguous reading of effectful `with()`. An assignment to `with` in an enclosing scope could run before this call and change its effect. + --> a.R:1:17 + | +1 | f <- function() with(local({ x <- 1 }), { y <- 2 }) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +2 | local <- identity +3 | with <- identity + | ---- could run before the call +info[ambiguous-effect]: Ambiguous reading of effectful `local()`. An assignment to `local` in an enclosing scope could run before this call and change its effect. + --> a.R:1:22 + | +1 | f <- function() with(local({ x <- 1 }), { y <- 2 }) + | ^^^^^^^^^^^^^^^^^ +2 | local <- identity + | ----- could run before the call diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_lazy_shadow_reassignment.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_lazy_shadow_reassignment.snap new file mode 100644 index 000000000..5673f2ffc --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_lazy_shadow_reassignment.snap @@ -0,0 +1,11 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +info[ambiguous-effect]: Ambiguous reading of effectful `local()`. An assignment to `local` in an enclosing scope could run before this call and change its effect. + --> a.R:1:17 + | +1 | f <- function() local({ x <- 1 }) + | ^^^^^^^^^^^^^^^^^ +2 | local <- identity + | ----- could run before the call diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_unconditional_attach_silent.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_unconditional_attach_silent.snap new file mode 100644 index 000000000..879ba7dc2 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_unconditional_attach_silent.snap @@ -0,0 +1,6 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +a.R +(no diagnostics) diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_uninstalled_package_conditional.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_uninstalled_package_conditional.snap new file mode 100644 index 000000000..a3ce3a256 --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_uninstalled_package_conditional.snap @@ -0,0 +1,9 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +warning[uninstalled-package]: Package `shiny` is not installed. Language analysis will be incomplete. + --> a.R:1:11 + | +1 | if (cond) library(shiny) + | ^^^^^^^^^^^^^^ diff --git a/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_uninstalled_package_unconditional.snap b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_uninstalled_package_unconditional.snap new file mode 100644 index 000000000..bfe0b33eb --- /dev/null +++ b/crates/oak_db/src/tests/snapshots/oak_db__tests__file_diagnostics__diagnostic_uninstalled_package_unconditional.snap @@ -0,0 +1,9 @@ +--- +source: crates/oak_db/src/tests/file_diagnostics.rs +expression: "render(\"a.R\", source, file.diagnostics(&db))" +--- +warning[uninstalled-package]: Package `shiny` is not installed. Language analysis will be incomplete. + --> a.R:1:1 + | +1 | library(shiny) + | ^^^^^^^^^^^^^^ diff --git a/crates/oak_semantic/src/builder/effects.rs b/crates/oak_semantic/src/builder/effects.rs index 9bbae3d3b..b6235f56c 100644 --- a/crates/oak_semantic/src/builder/effects.rs +++ b/crates/oak_semantic/src/builder/effects.rs @@ -259,7 +259,7 @@ impl SemanticIndexBuilder { call_range: TextRange, overwrite_range: TextRange, ) { - self.diagnostics.push(SemanticDiagnostic::EffectAmbiguity { + self.diagnostics.push(SemanticDiagnostic::AmbiguousEffect { name, call_range, reason: AmbiguityReason::LazyShadow { overwrite_range }, @@ -301,7 +301,7 @@ impl SemanticIndexBuilder { continue; } - self.diagnostics.push(SemanticDiagnostic::EffectAmbiguity { + self.diagnostics.push(SemanticDiagnostic::AmbiguousEffect { name: sym.to_string(), call_range, reason: AmbiguityReason::ConditionalAttach { diff --git a/crates/oak_semantic/src/builder/scan.rs b/crates/oak_semantic/src/builder/scan.rs index ec108817f..ce8b59b8d 100644 --- a/crates/oak_semantic/src/builder/scan.rs +++ b/crates/oak_semantic/src/builder/scan.rs @@ -416,6 +416,9 @@ impl SemanticIndexBuilder { if let Some(package) = attach { let call_range = call.syntax().text_trimmed_range(); + if !self.resolver.package_exists(&package) { + self.record_uninstalled_package(package.clone(), call_range); + } self.scan .call_resolutions .entry(call_range) @@ -578,6 +581,18 @@ impl SemanticIndexBuilder { .arguments = Some(arg_effects); } + /// Flag a `library()`/`require()` attach whose package doesn't resolve. + /// Its NSE annotations, if it has any, can't be looked up, so effect + /// ambiguity diagnostics for calls it would have annotated go silent + /// instead of firing; this stands in for that missing signal. + fn record_uninstalled_package(&mut self, package: String, call_range: TextRange) { + self.diagnostics + .push(SemanticDiagnostic::UninstalledPackage { + package, + range: call_range, + }); + } + /// Flag an NSE call whose scope only exists on some branches, because a /// *conditional* binding earlier in this scope shadows its callee. /// @@ -647,7 +662,7 @@ impl SemanticIndexBuilder { } let call_range = call.syntax().text_trimmed_range(); - self.diagnostics.push(SemanticDiagnostic::EffectAmbiguity { + self.diagnostics.push(SemanticDiagnostic::AmbiguousEffect { name, call_range, reason: AmbiguityReason::ConditionalShadow { binding_range }, diff --git a/crates/oak_semantic/src/resolver.rs b/crates/oak_semantic/src/resolver.rs index 0abae5b9b..a3b044208 100644 --- a/crates/oak_semantic/src/resolver.rs +++ b/crates/oak_semantic/src/resolver.rs @@ -31,7 +31,7 @@ pub struct SourceResolution { /// for isolated indexing (CLI tools, unit tests). /// - `oak_db::SalsaImportsResolver`: salsa-backed lookup against the source graph. /// -/// The trait has three queries: +/// The trait has four queries: /// /// - [`resolve_source`](ImportsResolver::resolve_source) is the bulk /// query, "enumerate every name this `source("path")` brings in," used @@ -40,6 +40,8 @@ pub struct SourceResolution { /// callee against imports, e.g. the search path, and returns known effects. /// - [`resolve_qualified_effects`](ImportsResolver::resolve_qualified_effects) /// resolves the effects of a `pkg::fn` (or `:::) callee against a named package. +/// - [`package_exists`](ImportsResolver::package_exists) tells whether a +/// `library()`/`require()` target resolves to an installed package. pub trait ImportsResolver { /// Resolve a `source("path")` call to the target file's exported names /// and transitive `library()` attachments. The path is the literal @@ -62,6 +64,14 @@ pub trait ImportsResolver { fn resolve_qualified_effects(&mut self, package: &str, name: &str) -> Option { effects::lookup(package, name).copied() } + + /// Whether `package` resolves to an installed package. Defaults to + /// `true`: a resolver that can't answer this must not produce false + /// "not installed" diagnostics. + fn package_exists(&mut self, package: &str) -> bool { + let _ = package; + true + } } /// Resolver that returns nothing. The builder skips all cross-file diff --git a/crates/oak_semantic/src/semantic_index.rs b/crates/oak_semantic/src/semantic_index.rs index a3b21e2db..8b6a6d38a 100644 --- a/crates/oak_semantic/src/semantic_index.rs +++ b/crates/oak_semantic/src/semantic_index.rs @@ -923,7 +923,7 @@ pub enum SemanticDiagnostic { /// reading even though another reading was possible. `call_range` points at /// the call we decided about, which may or may not have come out effectful; /// `reason` says what made it ambiguous and where the competing site is. - EffectAmbiguity { + AmbiguousEffect { name: String, call_range: TextRange, reason: AmbiguityReason, @@ -938,10 +938,13 @@ pub enum SemanticDiagnostic { packages: Vec, range: TextRange, }, + /// A `library()`/`require()` attach whose package doesn't resolve. `range` + /// points at the attach call. + UninstalledPackage { package: String, range: TextRange }, } -/// What made an [`EffectAmbiguity`](SemanticDiagnostic::EffectAmbiguity) -/// ambiguous, and where the competing site is. +/// Why an [`AmbiguousEffect`](SemanticDiagnostic::AmbiguousEffect) could have +/// read the other way, and where the competing site is. #[derive(Debug, Clone, PartialEq, Eq)] pub enum AmbiguityReason { /// The callee is bound by a lazy-crossed ancestor with undetermined diff --git a/crates/oak_semantic/tests/integration/common.rs b/crates/oak_semantic/tests/integration/common.rs index a030f1652..4e60e5583 100644 --- a/crates/oak_semantic/tests/integration/common.rs +++ b/crates/oak_semantic/tests/integration/common.rs @@ -14,41 +14,37 @@ use oak_semantic::semantic_index::DefinitionKind; use oak_semantic::semantic_index::ScopeId; use oak_semantic::semantic_index::SemanticCallKind; use oak_semantic::semantic_index::SemanticIndex; +use oak_semantic::ImportsResolver; use oak_semantic::NoopImportsResolver; use crate::resolvers::TestImportsResolver; pub(crate) fn index(source: &str) -> SemanticIndex { - let parsed = parse(source, RParserOptions::default()); - - if parsed.has_error() { - panic!("source has syntax errors: {source}"); - } - - build_index(&parsed.tree(), NoopImportsResolver) + build_with(source, NoopImportsResolver) } /// Build with base attached. Attach recognition (`library()`/`require()`) runs /// on the resolve path now, so it needs a resolver that resolves base, unlike /// the resolver-independent `source()` recognition the `index()` helper covers. pub(crate) fn index_with_base(source: &str) -> SemanticIndex { - let parsed = parse(source, RParserOptions::default()); - - if parsed.has_error() { - panic!("source has syntax errors: {source}"); - } - - build_index(&parsed.tree(), TestImportsResolver::with_base()) + build_with(source, TestImportsResolver::with_base()) } /// Build with `packages` attached (plus base), for package-contributed effects /// like magrittr's `%<>%` operator. pub(crate) fn index_with_attached(source: &str, packages: &[&str]) -> SemanticIndex { + build_with(source, TestImportsResolver::with_attached(packages)) +} + +/// Build with an arbitrary resolver, for cases the helpers above don't cover. +pub(crate) fn build_with(source: &str, resolver: impl ImportsResolver) -> SemanticIndex { let parsed = parse(source, RParserOptions::default()); + if parsed.has_error() { panic!("source has syntax errors: {source}"); } - build_index(&parsed.tree(), TestImportsResolver::with_attached(packages)) + + build_index(&parsed.tree(), resolver) } pub(crate) fn semantic_call_kinds(index: &SemanticIndex) -> Vec<&SemanticCallKind> { diff --git a/crates/oak_semantic/tests/integration/contrib/base.rs b/crates/oak_semantic/tests/integration/contrib/base.rs index 526080e71..782150efe 100644 --- a/crates/oak_semantic/tests/integration/contrib/base.rs +++ b/crates/oak_semantic/tests/integration/contrib/base.rs @@ -1432,7 +1432,7 @@ y let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::EffectAmbiguity { + SemanticDiagnostic::AmbiguousEffect { name, call_range, reason: AmbiguityReason::ConditionalShadow { .. }, @@ -1503,7 +1503,7 @@ f <- function() local({ let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::EffectAmbiguity { + SemanticDiagnostic::AmbiguousEffect { name, call_range, reason: AmbiguityReason::LazyShadow { overwrite_range }, @@ -1554,7 +1554,7 @@ local({ let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::EffectAmbiguity { + SemanticDiagnostic::AmbiguousEffect { name, call_range, reason: AmbiguityReason::ConditionalShadow { .. }, @@ -1604,7 +1604,7 @@ with(d, { let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::EffectAmbiguity { + SemanticDiagnostic::AmbiguousEffect { name, call_range, reason: AmbiguityReason::ConditionalShadow { .. }, @@ -2394,7 +2394,7 @@ local <- identity let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::EffectAmbiguity { + SemanticDiagnostic::AmbiguousEffect { name, call_range, reason: AmbiguityReason::LazyShadow { overwrite_range }, diff --git a/crates/oak_semantic/tests/integration/contrib/rlang.rs b/crates/oak_semantic/tests/integration/contrib/rlang.rs index ac0268889..d3a1a808b 100644 --- a/crates/oak_semantic/tests/integration/contrib/rlang.rs +++ b/crates/oak_semantic/tests/integration/contrib/rlang.rs @@ -240,7 +240,7 @@ rlang::on_load({ local <- identity }) let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::EffectAmbiguity { + SemanticDiagnostic::AmbiguousEffect { name, reason: AmbiguityReason::LazyShadow { .. }, .. diff --git a/crates/oak_semantic/tests/integration/contrib/shiny.rs b/crates/oak_semantic/tests/integration/contrib/shiny.rs index 9e235c1e5..23c83fb90 100644 --- a/crates/oak_semantic/tests/integration/contrib/shiny.rs +++ b/crates/oak_semantic/tests/integration/contrib/shiny.rs @@ -387,7 +387,7 @@ reactive({ let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::EffectAmbiguity { + SemanticDiagnostic::AmbiguousEffect { name, call_range, reason: @@ -428,7 +428,7 @@ reactive({ let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::EffectAmbiguity { + SemanticDiagnostic::AmbiguousEffect { reason: AmbiguityReason::ConditionalAttach { package, @@ -445,6 +445,50 @@ reactive({ } } +#[test] +fn test_nse_conditional_attach_wins_over_sibling_conditional_shadow() { + // The `else` branch conditionally binds the callee, so both ambiguity + // reasons look applicable. Only the attach one fires: the conditional + // binding drops at the join, so `resolve_symbol_effects()` still probes the + // search path, and that probe fails, which is what suppresses the + // conditional-shadow check downstream (it only runs on a resolved effect). + let source = "\ +if (sample(0:1, 1)) { + library(shiny) +} else { + reactive <- identity +} +reactive(1) +"; + let index = index(source); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::AmbiguousEffect { + name, + call_range, + reason: + AmbiguityReason::ConditionalAttach { + package, + attach_range, + }, + } => { + assert_eq!(name, "reactive"); + assert_eq!(package, "shiny"); + + let call_start = u32::from(call_range.start()) as usize; + let call_end = u32::from(call_range.end()) as usize; + assert_eq!(&source[call_start..call_end], "reactive(1)"); + + let attach_start = u32::from(attach_range.start()) as usize; + let attach_end = u32::from(attach_range.end()) as usize; + assert_eq!(&source[attach_start..attach_end], "library(shiny)"); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} + #[test] fn test_nse_coguarded_attach_no_ambiguity() { // Attach and use are co-guarded (see `test_nse_coguarded_attach_reaches_lazy_body`): @@ -510,7 +554,7 @@ reactive({ let diagnostics = index.diagnostics(); assert_eq!(diagnostics.len(), 1); match &diagnostics[0] { - SemanticDiagnostic::EffectAmbiguity { + SemanticDiagnostic::AmbiguousEffect { name, reason: AmbiguityReason::ConditionalAttach { package, .. }, .. diff --git a/crates/oak_semantic/tests/integration/diagnostics.rs b/crates/oak_semantic/tests/integration/diagnostics.rs new file mode 100644 index 000000000..59a04d73c --- /dev/null +++ b/crates/oak_semantic/tests/integration/diagnostics.rs @@ -0,0 +1,28 @@ +//! Diagnostics the builder records regardless of which package contributed the +//! effect. Package-specific diagnostic coverage lives under `contrib/`. + +use oak_semantic::semantic_index::SemanticDiagnostic; + +use crate::common::build_with; +use crate::resolvers::MissingPackageResolver; + +#[test] +fn test_attach_records_uninstalled_package() { + // `MissingPackageResolver` never resolves a package, so the attach + // records an `UninstalledPackage` diagnostic pointing at the whole + // `library()` call. + let source = "library(shiny)\n"; + let index = build_with(source, MissingPackageResolver); + + let diagnostics = index.diagnostics(); + assert_eq!(diagnostics.len(), 1); + match &diagnostics[0] { + SemanticDiagnostic::UninstalledPackage { package, range } => { + assert_eq!(package, "shiny"); + let start = u32::from(range.start()) as usize; + let end = u32::from(range.end()) as usize; + assert_eq!(&source[start..end], "library(shiny)"); + }, + other => panic!("unexpected diagnostic: {other:?}"), + } +} diff --git a/crates/oak_semantic/tests/integration/main.rs b/crates/oak_semantic/tests/integration/main.rs index 48dd13efe..2564d137d 100644 --- a/crates/oak_semantic/tests/integration/main.rs +++ b/crates/oak_semantic/tests/integration/main.rs @@ -1,5 +1,6 @@ mod builder; mod common; mod contrib; +mod diagnostics; mod resolvers; mod use_def_map; diff --git a/crates/oak_semantic/tests/integration/resolvers.rs b/crates/oak_semantic/tests/integration/resolvers.rs index 7cb50788a..d4e4929e4 100644 --- a/crates/oak_semantic/tests/integration/resolvers.rs +++ b/crates/oak_semantic/tests/integration/resolvers.rs @@ -78,3 +78,22 @@ impl ImportsResolver for TestImportsResolver { .find_map(|pkg| effects::lookup(pkg, name).copied()) } } + +/// Resolves base effects (so `library()` itself is recognized as an attach +/// call) but reports every package as not installed. For asserting the +/// builder's `UninstalledPackage` diagnostic at an attach site. +pub struct MissingPackageResolver; + +impl ImportsResolver for MissingPackageResolver { + fn resolve_source(&mut self, _path: &str) -> Option { + None + } + + fn resolve_effects(&mut self, name: &str, _: &[String]) -> Option { + effects::lookup("base", name).copied() + } + + fn package_exists(&mut self, _package: &str) -> bool { + false + } +} diff --git a/justfile b/justfile index 12c4a8b99..3beb88a38 100644 --- a/justfile +++ b/justfile @@ -12,6 +12,10 @@ test-verbose: test-insta: cargo insta test --test-runner nextest +# Rewrite the diagnostic snapshots in place +test-insta-diagnostics: + INSTA_UPDATE=always cargo nextest run -p oak_db test_diagnostic_ + # Run clippy clippy: cargo clippy --workspace --all-targets --all-features -- -D warnings