diff --git a/Cargo.lock b/Cargo.lock index f9b9009fa5..6509245cd6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2544,6 +2544,7 @@ version = "0.1.0" dependencies = [ "air_r_parser", "air_r_syntax", + "anyhow", "assert_matches", "biome_rowan", ] @@ -2583,15 +2584,10 @@ version = "0.1.0" dependencies = [ "air_r_parser", "air_r_syntax", - "assert_matches", + "anyhow", "biome_rowan", - "log", "oak_core", - "oak_db", - "oak_package_metadata", "oak_semantic", - "oak_sources", - "stdext", "url", ] @@ -2633,7 +2629,6 @@ dependencies = [ "air_r_parser", "air_r_syntax", "anyhow", - "assert_matches", "biome_rowan", "biome_text_size", "itertools 0.14.0", @@ -2642,7 +2637,6 @@ dependencies = [ "oak_index_vec", "oak_package_metadata", "oak_semantic", - "oak_sources", "rustc-hash", "salsa", "smallvec", diff --git a/crates/ark/src/lsp.rs b/crates/ark/src/lsp.rs index 0e63279696..05acf233a6 100644 --- a/crates/ark/src/lsp.rs +++ b/crates/ark/src/lsp.rs @@ -19,7 +19,6 @@ pub mod document_context; pub mod events; pub mod folding_range; pub mod goto_definition; -pub mod goto_definition_legacy; pub mod handler; pub mod handlers; pub mod help; @@ -33,6 +32,7 @@ pub mod main_loop; pub mod markdown; pub mod find_references; +pub mod rename; pub mod selection_range; pub mod signature_help; pub mod state; @@ -42,6 +42,9 @@ pub mod symbols; pub mod traits; pub mod util; +#[cfg(test)] +mod tests; + // These send LSP messages in a non-async and non-blocking way. // The LOG level is not timestamped so we're not using it. macro_rules! log_info { diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 9d4feb6417..5d87418982 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -7,7 +7,6 @@ #![allow(deprecated)] -use std::path::PathBuf; use std::sync::atomic::Ordering; use std::sync::Arc; @@ -81,7 +80,10 @@ macro_rules! cast_response { }, RequestResponse::Result(Err(err)) => match err { LspError::JsonRpc(err) => Err(err), - LspError::Anyhow(err) => Err(new_jsonrpc_error(format!("{err:?}"))), + // `{err}` (Display) prints just the message. `{err:?}` (Debug) + // would include the captured backtrace when `RUST_BACKTRACE` + // is set, which then surfaces in the client's error popup. + LspError::Anyhow(err) => Err(new_jsonrpc_error(format!("{err}"))), }, RequestResponse::Crashed(err) => { // Notify user that the LSP has crashed and is no longer active @@ -154,6 +156,8 @@ pub(crate) enum LspRequest { GotoImplementation(GotoImplementationParams), SelectionRange(SelectionRangeParams), References(ReferenceParams), + PrepareRename(TextDocumentPositionParams), + Rename(RenameParams), StatementRange(StatementRangeParams), HelpTopic(HelpTopicParams), OnTypeFormatting(DocumentOnTypeFormattingParams), @@ -178,6 +182,8 @@ pub(crate) enum LspResponse { GotoImplementation(Option), SelectionRange(Option>), References(Option>), + PrepareRename(Option), + Rename(Option), StatementRange(Option), HelpTopic(Option), OnTypeFormatting(Option>), @@ -432,6 +438,25 @@ impl LanguageServer for Backend { ) } + async fn prepare_rename( + &self, + params: TextDocumentPositionParams, + ) -> Result> { + cast_response!( + self, + self.request(LspRequest::PrepareRename(params)).await, + LspResponse::PrepareRename + ) + } + + async fn rename(&self, params: RenameParams) -> Result> { + cast_response!( + self, + self.request(LspRequest::Rename(params)).await, + LspResponse::Rename + ) + } + async fn on_type_formatting( &self, params: DocumentOnTypeFormattingParams, @@ -518,7 +543,6 @@ impl Backend { } pub(crate) fn start_lsp( - r_home: PathBuf, runtime: Arc, server_start: ServerStartMessage, server_started_tx: Sender, @@ -557,7 +581,7 @@ pub(crate) fn start_lsp( let (shutdown_tx, mut shutdown_rx) = tokio::sync::mpsc::channel::<()>(1); let init = |client: Client| { - let state = GlobalState::new(client, r_home, console_notification_tx); + let state = GlobalState::new(client, console_notification_tx); let events_tx = state.events_tx(); // Start main loop and hold onto the handle that keeps it alive diff --git a/crates/ark/src/lsp/diagnostics.rs b/crates/ark/src/lsp/diagnostics.rs index a62c452d37..483ca87d5b 100644 --- a/crates/ark/src/lsp/diagnostics.rs +++ b/crates/ark/src/lsp/diagnostics.rs @@ -1654,7 +1654,7 @@ foo let package = Package::from_parts(PathBuf::from("/mock/path"), description, namespace); // Create a library with `mockpkg` installed - let library = Library::new(vec![], None).insert("mockpkg", package); + let library = Library::new(vec![]).insert("mockpkg", package); // Simulate a search path with `library` in scope let console_scopes = vec![vec!["library".to_string()]]; @@ -1769,7 +1769,7 @@ foo let package2 = Package::from_parts(PathBuf::from("/mock/path2"), description2, namespace2); - let library = Library::new(vec![], None) + let library = Library::new(vec![]) .insert("pkg1", package1) .insert("pkg2", package2); @@ -1827,7 +1827,7 @@ foo }; let package = Package::from_parts(PathBuf::from("/mock/path"), description, namespace); - let library = Library::new(vec![], None).insert("pkg", package); + let library = Library::new(vec![]).insert("pkg", package); let console_scopes = vec![vec!["require".to_string()]]; let state = WorldState { @@ -1858,7 +1858,7 @@ foo let palmerpenguins_pkg = Package::load_from_folder(palmerpenguins_dir.path()) .unwrap() .unwrap(); - let library = Library::new(vec![], None).insert("penguins", palmerpenguins_pkg); + let library = Library::new(vec![]).insert("penguins", palmerpenguins_pkg); // Simulate a world state with the penguins package installed and attached let mut state = DEFAULT_STATE.clone(); diff --git a/crates/ark/src/lsp/find_references.rs b/crates/ark/src/lsp/find_references.rs index 92f6651875..d038ece9ab 100644 --- a/crates/ark/src/lsp/find_references.rs +++ b/crates/ark/src/lsp/find_references.rs @@ -1,16 +1,11 @@ -// -// references.rs -// -// Copyright (C) 2022-2026 Posit Software, PBC. All rights reserved. -// -// - use std::path::Path; +use aether_lsp_utils::proto::from_proto; +use aether_lsp_utils::proto::to_proto; use anyhow::anyhow; use stdext::result::ResultExt; +use stdext::unwrap; use stdext::unwrap::IntoResult; -use stdext::*; use tower_lsp::lsp_types::Location; use tower_lsp::lsp_types::Position; use tower_lsp::lsp_types::Range; @@ -32,6 +27,68 @@ use crate::treesitter::ExtractOperatorType; use crate::treesitter::NodeType; use crate::treesitter::NodeTypeExt; +pub(crate) fn find_references( + params: ReferenceParams, + state: &WorldState, +) -> anyhow::Result> { + let uri = params.text_document_position.text_document.uri; + let position = params.text_document_position.position; + let include_declaration = params.context.include_declaration; + + let document = state.get_document(&uri)?; + + let mut locations: Vec = Vec::new(); + + let index = document.semantic_index(); + let root = document.syntax()?; + + // Intra-file resolution is precise via the semantic index + let offset = from_proto::offset_from_position( + position, + &document.line_index, + document.position_encoding, + )?; + let pos = oak_ide::FileOffset { + file: uri.clone(), + offset, + }; + let intra = oak_ide::find_references(&index, &root, &pos, include_declaration); + + for file_range in intra { + let Some(range) = to_proto::range( + file_range.range, + &document.line_index, + document.position_encoding, + ) + .log_err() else { + continue; + }; + locations.push(Location::new(file_range.file, range)); + } + + if !locations.is_empty() { + // If the intra-file pass resolved cleanly, we have the precise answer. + // The textual cross-file walk can only add unrefined external matches + // which, for a locally-bound symbol, would just be unrelated noise. + return Ok(locations); + } + + // Truly free variable: no within-file binding. Fall back to a textual walk + // over workspace folders (including the current file: intra-file gave + // nothing, so no dedup needed). No semantic refinement yet, so unrelated + // same-name symbols in other files leak through. + if let Ok(context) = build_context(&uri, position, state) { + for folder in state.workspace.folders.iter() { + if let Ok(path) = folder.to_file_path() { + lsp::log_info!("searching references in folder {}", path.display()); + find_references_in_folder(&context, &path, &mut locations, state); + } + } + } + + Ok(locations) +} + #[derive(Debug, PartialEq)] enum ReferenceKind { Symbol, // a regular R symbol @@ -94,21 +151,19 @@ fn found_match(node: &Node, contents: &str, context: &Context) -> bool { if !node.is_identifier() { return false; } - - let symbol = node.node_to_string(contents).unwrap(); + let Ok(symbol) = node.node_to_string(contents) else { + return false; + }; if symbol != context.symbol { return false; } - context.kind == node_reference_kind(node) } fn build_context(uri: &Url, position: Position, state: &WorldState) -> anyhow::Result { - // Unwrap the URL. let path = uri.file_path()?; - // Figure out the identifier we're looking for. - let context = with_document(path.as_path(), state, |document| { + with_document(path.as_path(), state, |document| { let ast = &document.ast; let contents = document.contents.as_str(); let point = document.tree_sitter_point_from_lsp_position(position)?; @@ -118,23 +173,32 @@ fn build_context(uri: &Url, position: Position, state: &WorldState) -> anyhow::R .descendant_for_point_range(point, point) .into_result()?; - // Check and see if we got an identifier. If we didn't, we might need to use - // some heuristics to look around. Unfortunately, it seems like if you double-click - // to select an identifier, and then use Right Click -> Find All References, the - // position received by the LSP maps to the _end_ of the selected range, which - // is technically not part of the associated identifier's range. In addition, we - // can't just subtract 1 from the position column since that would then fail to - // resolve the correct identifier when the cursor is located at the start of the - // identifier. + // Zero-width range queries at an identifier boundary return the + // wrapping node rather than the identifier itself. If the cursor is at + // the trailing edge of a selection (column past the last character), + // retry one column back. If it's at the leading edge (column on the + // first character), retry one column forward. if !node.is_identifier() && point.column > 0 { - let point = Point::new(point.row, point.column - 1); - node = ast + let back = Point::new(point.row, point.column - 1); + if let Some(retry) = ast .root_node() - .descendant_for_point_range(point, point) - .into_result()?; + .descendant_for_point_range(back, back) + .filter(|n| n.is_identifier()) + { + node = retry; + } + } + if !node.is_identifier() { + let fwd = Point::new(point.row, point.column + 1); + if let Some(retry) = ast + .root_node() + .descendant_for_point_range(fwd, fwd) + .filter(|n| n.is_identifier()) + { + node = retry; + } } - // double check that we found an identifier if !node.is_identifier() { return Err(anyhow!( "couldn't find an identifier associated with point {point:?}", @@ -142,14 +206,10 @@ fn build_context(uri: &Url, position: Position, state: &WorldState) -> anyhow::R } let kind = node_reference_kind(&node); - - // return identifier text contents let symbol = node.node_to_string(contents)?; Ok(Context { kind, symbol }) - }); - - context + }) } fn find_references_in_folder( @@ -167,7 +227,6 @@ fn find_references_in_folder( continue; } - lsp::log_info!("found R file {}", path.display()); let result = with_document(path, state, |document| { find_references_in_document(context, path, document, locations) }); @@ -201,30 +260,3 @@ fn find_references_in_document( }); Ok(()) } - -pub(crate) fn find_references( - params: ReferenceParams, - state: &WorldState, -) -> anyhow::Result> { - // Create our locations vector. - let mut locations: Vec = Vec::new(); - - // Extract relevant parameters. - let uri = params.text_document_position.text_document.uri; - let position = params.text_document_position.position; - - // Figure out what we're looking for. - let context = unwrap!(build_context(&uri, position, state), Err(err) => { - return Err(anyhow!("Failed to find build context at position {position:?}: {err:?}")); - }); - - // Now, start searching through workspace folders for references to that identifier. - for folder in state.workspace.folders.iter() { - if let Ok(path) = folder.to_file_path() { - lsp::log_info!("searching references in folder {}", path.display()); - find_references_in_folder(&context, &path, &mut locations, state); - } - } - - Ok(locations) -} diff --git a/crates/ark/src/lsp/goto_definition.rs b/crates/ark/src/lsp/goto_definition.rs index ff65cc14ba..6894fbd246 100644 --- a/crates/ark/src/lsp/goto_definition.rs +++ b/crates/ark/src/lsp/goto_definition.rs @@ -1,19 +1,21 @@ use aether_lsp_utils::proto::from_proto; use aether_lsp_utils::proto::to_proto; -use anyhow::anyhow; use oak_ide::NavigationTarget; use stdext::result::ResultExt; use tower_lsp::lsp_types::GotoDefinitionParams; use tower_lsp::lsp_types::GotoDefinitionResponse; use tower_lsp::lsp_types::LocationLink; +use tower_lsp::lsp_types::Position; +use url::Url; use crate::lsp::document::Document; -use crate::lsp::state::WorldState; +use crate::lsp::indexer; +use crate::lsp::traits::node::NodeExt; +use crate::treesitter::NodeTypeExt; pub(crate) fn goto_definition( document: &Document, params: GotoDefinitionParams, - state: &WorldState, ) -> anyhow::Result> { let uri = params.text_document_position_params.text_document.uri; let position = params.text_document_position_params.position; @@ -24,44 +26,82 @@ pub(crate) fn goto_definition( document.position_encoding, )?; - let (index, scope) = state.file_analysis(&uri, document); + let index = document.semantic_index(); let root = document.syntax()?; - let targets = oak_ide::goto_definition(state, offset, &uri, &root, &index, &scope); - - if targets.is_empty() { - return Ok(None); - } + let pos = oak_ide::FileOffset { + file: uri.clone(), + offset, + }; + let targets = oak_ide::goto_definition(&index, &root, &pos); let links: Vec<_> = targets .into_iter() - .filter_map(|target| nav_target_to_link(target, state).log_err()) + .filter_map(|target| nav_target_to_link(target, document).log_err()) .collect(); - if links.is_empty() { + if !links.is_empty() { + return Ok(Some(GotoDefinitionResponse::Link(links))); + } + + // Within-file resolution found nothing. Fall back to the workspace + // indexer for a best-effort cross-file lookup of the identifier at the + // cursor. Non-identifier cursors (operators, parens, whitespace) return + // `None`. TODO(salsa): Replace the indexer step with a proper cross-file + // (file imports) lookup. + if let Some(link) = fallback_link_at(document, position, &uri)? { + return Ok(Some(GotoDefinitionResponse::Link(vec![link]))); + } + + Ok(None) +} + +/// Look up the identifier at `position` in the workspace indexer (current +/// file first, then other files). Returns `None` when the cursor isn't on +/// an identifier or the symbol isn't in the index. +fn fallback_link_at( + document: &Document, + position: Position, + uri: &Url, +) -> anyhow::Result> { + let point = document.tree_sitter_point_from_lsp_position(position)?; + let Some(node) = document.ast.root_node().find_closest_node_to_point(point) else { + log::warn!("Failed to find the closest node to point {point}."); + return Ok(None); + }; + + if !node.is_identifier() { return Ok(None); } - Ok(Some(GotoDefinitionResponse::Link(links))) + let symbol = node.node_as_str(&document.contents)?; + let Some((file_id, entry)) = + indexer::find_in_file(symbol, uri).or_else(|| indexer::find(symbol)) + else { + return Ok(None); + }; + + Ok(Some(LocationLink { + origin_selection_range: None, + target_uri: file_id.as_uri().clone(), + target_range: entry.range, + target_selection_range: entry.range, + })) } fn nav_target_to_link( target: NavigationTarget, - state: &WorldState, + document: &Document, ) -> anyhow::Result { - let doc = if let Some(open) = state.documents.get(&target.file) { - open - } else { - let path = target - .file - .to_file_path() - .map_err(|_| anyhow!("Can't convert URI to path: {}", target.file))?; - let contents = std::fs::read_to_string(&path)?; - &Document::new(&contents, None) - }; - - let target_range = to_proto::range(target.full_range, &doc.line_index, doc.position_encoding)?; - let target_selection_range = - to_proto::range(target.focus_range, &doc.line_index, doc.position_encoding)?; + let target_range = to_proto::range( + target.full_range, + &document.line_index, + document.position_encoding, + )?; + let target_selection_range = to_proto::range( + target.focus_range, + &document.line_index, + document.position_encoding, + )?; Ok(LocationLink { origin_selection_range: None, @@ -71,8 +111,17 @@ fn nav_target_to_link( }) } -#[cfg(test)] -mod tests { +// Parked cross-file goto-definition tests pending the Salsa Oak wiring. +// +// Snapshot of the old `LegacyDb`-based suite, trimmed to the cross-file +// cases that the within-file handler doesn't cover (collation, `source()`, +// `library()`, NAMESPACE, package scope, base/search path). Intra-file +// tests live in `crates/ark/src/lsp/tests/goto_definition.rs`. +// `#[cfg(any())]` skips the block at compile time so the references to +// deleted types (`LegacyDb`, `ExternalScope`, `ScopeLayer`, ...) stay as +// the behavioural spec to re-implement on top of `oak_db::File::resolve_at`. +#[cfg(any())] +mod parked_salsa_tests { use std::path::PathBuf; use std::process::Command; @@ -119,96 +168,6 @@ mod tests { state } - #[test] - fn test_goto_definition() { - let code = "foo <- 42\nprint(foo)\n"; - let doc = Document::new(code, None); - let uri = test_path("test.R"); - let state = make_state(&uri, &doc); - - let params = make_params(uri, 1, 6); - - assert_matches!( - goto_definition(&doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 3), - } - ); - } - ); - } - - #[test] - fn test_goto_definition_prefers_local_symbol() { - let code = "foo <- 1\nfoo\n"; - let doc = Document::new(code, None); - let uri = test_path("file.R"); - let state = make_state(&uri, &doc); - - let params = make_params(uri.clone(), 1, 0); - - assert_matches!( - goto_definition(&doc, params, &state).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!(links[0].target_uri, uri); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(0, 0), - end: lsp_types::Position::new(0, 3), - } - ); - } - ); - } - - #[test] - fn test_goto_definition_no_use_returns_none() { - let code = "x <- 1\n"; - let doc = Document::new(code, None); - let uri = test_path("test.R"); - let state = make_state(&uri, &doc); - - let params = make_params(uri, 0, 3); - let result = goto_definition(&doc, params, &state).unwrap(); - assert_eq!(result, None); - } - - #[test] - fn test_goto_definition_unresolved_returns_none() { - let code = "foo\n"; - let doc = Document::new(code, None); - let uri = test_path("test.R"); - let state = make_state(&uri, &doc); - - let params = make_params(uri, 0, 0); - let result = goto_definition(&doc, params, &state).unwrap(); - assert_eq!(result, None); - } - - #[test] - fn test_other_file_not_visible_without_scope_chain() { - // file2 uses `foo` but file1's definition is not in the scope chain, - // so it should not resolve. - let doc1 = Document::new("foo <- 1\n", None); - let uri1 = test_path("file1.R"); - - let doc2 = Document::new("foo\n", None); - let uri2 = test_path("file2.R"); - - let mut state = WorldState::default(); - state.documents.insert(uri1, doc1); - state.documents.insert(uri2.clone(), doc2.clone()); - - let params = make_params(uri2, 0, 0); - let result = goto_definition(&doc2, params, &state).unwrap(); - assert_eq!(result, None); - } - #[test] fn test_package_import_from_resolves() { // A package with `importFrom(dplyr, mutate)` should make `mutate` diff --git a/crates/ark/src/lsp/goto_definition_legacy.rs b/crates/ark/src/lsp/goto_definition_legacy.rs deleted file mode 100644 index 5dce1365ca..0000000000 --- a/crates/ark/src/lsp/goto_definition_legacy.rs +++ /dev/null @@ -1,268 +0,0 @@ -// -// definitions.rs -// -// Copyright (C) 2022 Posit Software, PBC. All rights reserved. -// -// - -use anyhow::Result; -use tower_lsp::lsp_types::GotoDefinitionParams; -use tower_lsp::lsp_types::GotoDefinitionResponse; -use tower_lsp::lsp_types::LocationLink; -use tower_lsp::lsp_types::Range; - -use crate::lsp::document::Document; -use crate::lsp::indexer; -use crate::lsp::traits::node::NodeExt; -use crate::treesitter::NodeTypeExt; - -pub fn goto_definition( - document: &Document, - params: GotoDefinitionParams, -) -> Result> { - // get reference to AST - let ast = &document.ast; - - // try to find node at position - let position = params.text_document_position_params.position; - let point = document.tree_sitter_point_from_lsp_position(position)?; - - let Some(node) = ast.root_node().find_closest_node_to_point(point) else { - log::warn!("Failed to find the closest node to point {point}."); - return Ok(None); - }; - - let start = document.lsp_position_from_tree_sitter_point(node.start_position())?; - let end = document.lsp_position_from_tree_sitter_point(node.end_position())?; - let range = Range { start, end }; - - // Search for a reference in the document index - if node.is_identifier() { - let symbol = node.node_as_str(&document.contents)?; - - // First search in current file, then in all files - let uri = ¶ms.text_document_position_params.text_document.uri; - let info = indexer::find_in_file(symbol, uri).or_else(|| indexer::find(symbol)); - - if let Some((file_id, entry)) = info { - let target_uri = file_id.as_uri().clone(); - let link = LocationLink { - origin_selection_range: None, - target_uri, - target_range: entry.range, - target_selection_range: entry.range, - }; - let response = GotoDefinitionResponse::Link(vec![link]); - return Ok(Some(response)); - } - } - - // TODO: We should see if we can find the referenced item in: - // - // 1. The document's current AST, - // 2. The public functions from other documents in the project, - // 3. A definition in the R session (which we could open in a virtual document) - // - // If we can't find a definition, then we can return the referenced item itself, - // which will tell Positron to instead try to look for references for that symbol. - let link = LocationLink { - origin_selection_range: Some(range), - target_uri: params.text_document_position_params.text_document.uri, - target_range: range, - target_selection_range: range, - }; - - let response = GotoDefinitionResponse::Link(vec![link]); - Ok(Some(response)) -} - -#[cfg(test)] -mod tests { - use assert_matches::assert_matches; - use tower_lsp::lsp_types; - - use super::*; - use crate::lsp::document::Document; - use crate::lsp::indexer; - use crate::lsp::util::test_path; - - #[test] - fn test_goto_definition() { - let _guard = indexer::ResetIndexerGuard; - - let code = r#" -foo <- 42 -print(foo) -"#; - let doc = Document::new(code, None); - let uri = test_path("test.R"); - - indexer::update(&doc, &uri).unwrap(); - - let params = GotoDefinitionParams { - text_document_position_params: lsp_types::TextDocumentPositionParams { - text_document: lsp_types::TextDocumentIdentifier { uri }, - position: lsp_types::Position::new(2, 7), - }, - work_done_progress_params: Default::default(), - partial_result_params: Default::default(), - }; - - assert_matches!( - goto_definition(&doc, params).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(1, 0), - end: lsp_types::Position::new(1, 3), - } - ); - } - ); - } - - #[test] - fn test_goto_definition_comment_section() { - let _guard = indexer::ResetIndexerGuard; - - let code = r#" -# foo ---- -foo <- 1 -print(foo) -"#; - let doc = Document::new(code, None); - let uri = test_path("test.R"); - - indexer::update(&doc, &uri).unwrap(); - - let params = lsp_types::GotoDefinitionParams { - text_document_position_params: lsp_types::TextDocumentPositionParams { - text_document: lsp_types::TextDocumentIdentifier { uri }, - position: lsp_types::Position::new(3, 7), - }, - work_done_progress_params: Default::default(), - partial_result_params: Default::default(), - }; - - assert_matches!( - goto_definition(&doc, params).unwrap(), - Some(lsp_types::GotoDefinitionResponse::Link(ref links)) => { - // The section should is not the target, the variable has priority - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(2, 0), - end: lsp_types::Position::new(2, 3), - } - ); - } - ); - } - - #[test] - fn test_goto_definition_prefers_local_symbol() { - let _guard = indexer::ResetIndexerGuard; - - // Both files define the same symbol - let code1 = r#" -foo <- 1 -foo -"#; - let code2 = r#" -foo <- 2 -foo -"#; - - let doc1 = Document::new(code1, None); - let doc2 = Document::new(code2, None); - - let uri1 = test_path("file1.R"); - let uri2 = test_path("file2.R"); - - indexer::update(&doc1, &uri1).unwrap(); - indexer::update(&doc2, &uri2).unwrap(); - - // Go to definition for foo in file1 - let params1 = GotoDefinitionParams { - text_document_position_params: lsp_types::TextDocumentPositionParams { - text_document: lsp_types::TextDocumentIdentifier { uri: uri1.clone() }, - position: lsp_types::Position::new(2, 0), - }, - work_done_progress_params: Default::default(), - partial_result_params: Default::default(), - }; - assert_matches!( - goto_definition(&doc1, params1).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - // Should jump to foo in file1 - assert_eq!(links[0].target_uri, uri1); - } - ); - - // Go to definition for foo in file2 - let params2 = GotoDefinitionParams { - text_document_position_params: lsp_types::TextDocumentPositionParams { - text_document: lsp_types::TextDocumentIdentifier { uri: uri2.clone() }, - position: lsp_types::Position::new(2, 0), - }, - work_done_progress_params: Default::default(), - partial_result_params: Default::default(), - }; - assert_matches!( - goto_definition(&doc2, params2).unwrap(), - Some(GotoDefinitionResponse::Link(ref links)) => { - // Should jump to foo in file2 - assert_eq!(links[0].target_uri, uri2); - } - ); - } - - #[test] - fn test_goto_definition_falls_back_to_other_file() { - let _guard = indexer::ResetIndexerGuard; - - // file1 defines foo, file2 does not - let code1 = r#" -foo <- 1 -"#; - let code2 = r#" -foo -"#; - - let doc1 = Document::new(code1, None); - let doc2 = Document::new(code2, None); - - // Use test_path for cross-platform compatibility - let uri1 = test_path("file1.R"); - let uri2 = test_path("file2.R"); - - indexer::update(&doc1, &uri1).unwrap(); - indexer::update(&doc2, &uri2).unwrap(); - - // Go to definition for foo in file2 (should jump to file1) - let params2 = GotoDefinitionParams { - text_document_position_params: lsp_types::TextDocumentPositionParams { - text_document: lsp_types::TextDocumentIdentifier { uri: uri2.clone() }, - position: lsp_types::Position::new(1, 0), - }, - work_done_progress_params: Default::default(), - partial_result_params: Default::default(), - }; - let result2 = goto_definition(&doc2, params2).unwrap(); - assert_matches!( - result2, - Some(GotoDefinitionResponse::Link(ref links)) => { - // Should jump to foo in file1 - assert_eq!(links[0].target_uri, uri1); - assert_eq!( - links[0].target_range, - lsp_types::Range { - start: lsp_types::Position::new(1, 0), - end: lsp_types::Position::new(1, 3), - } - ); - } - ); - } -} diff --git a/crates/ark/src/lsp/handler.rs b/crates/ark/src/lsp/handler.rs index b4ef46a1d5..38320c20d2 100644 --- a/crates/ark/src/lsp/handler.rs +++ b/crates/ark/src/lsp/handler.rs @@ -5,7 +5,6 @@ // // -use std::path::PathBuf; use std::sync::Arc; use amalthea::comm::server_comm::ServerStartMessage; @@ -24,7 +23,6 @@ use crate::console::ConsoleNotification; use crate::console::KernelInfo; pub(crate) struct Lsp { - r_home: PathBuf, runtime: Arc, kernel_init_rx: BusReader, kernel_initialized: bool, @@ -33,7 +31,6 @@ pub(crate) struct Lsp { impl Lsp { pub(crate) fn new( - r_home: PathBuf, kernel_init_rx: BusReader, console_notification_tx: AsyncUnboundedSender, ) -> Self { @@ -47,7 +44,6 @@ impl Lsp { .unwrap(); Self { - r_home, runtime: Arc::new(rt), kernel_init_rx, kernel_initialized: false, @@ -79,11 +75,9 @@ impl ServerHandler for Lsp { // account for potential reconnects let runtime = self.runtime.clone(); - let r_home = self.r_home.clone(); let console_notification_tx = self.console_notification_tx.clone(); spawn!("ark-lsp", move || { backend::start_lsp( - r_home, runtime, server_start, server_started_tx, diff --git a/crates/ark/src/lsp/handlers.rs b/crates/ark/src/lsp/handlers.rs index c86e08a057..949482dd26 100644 --- a/crates/ark/src/lsp/handlers.rs +++ b/crates/ark/src/lsp/handlers.rs @@ -26,13 +26,16 @@ use tower_lsp::lsp_types::HoverContents; use tower_lsp::lsp_types::HoverParams; use tower_lsp::lsp_types::Location; use tower_lsp::lsp_types::MessageType; +use tower_lsp::lsp_types::PrepareRenameResponse; use tower_lsp::lsp_types::ReferenceParams; use tower_lsp::lsp_types::Registration; +use tower_lsp::lsp_types::RenameParams; use tower_lsp::lsp_types::SelectionRange; use tower_lsp::lsp_types::SelectionRangeParams; use tower_lsp::lsp_types::SignatureHelp; use tower_lsp::lsp_types::SignatureHelpParams; use tower_lsp::lsp_types::SymbolInformation; +use tower_lsp::lsp_types::TextDocumentPositionParams; use tower_lsp::lsp_types::TextEdit; use tower_lsp::lsp_types::WorkspaceEdit; use tower_lsp::lsp_types::WorkspaceSymbolParams; @@ -50,7 +53,6 @@ use crate::lsp::document_context::DocumentContext; use crate::lsp::find_references::find_references; use crate::lsp::folding_range::folding_range; use crate::lsp::goto_definition::goto_definition; -use crate::lsp::goto_definition_legacy; use crate::lsp::help_topic::help_topic; use crate::lsp::help_topic::HelpTopicParams; use crate::lsp::help_topic::HelpTopicResponse; @@ -59,6 +61,7 @@ use crate::lsp::indent::indent_edit; use crate::lsp::input_boundaries::InputBoundariesParams; use crate::lsp::input_boundaries::InputBoundariesResponse; use crate::lsp::main_loop::LspState; +use crate::lsp::rename; use crate::lsp::selection_range::convert_selection_range_from_tree_sitter_to_lsp; use crate::lsp::selection_range::selection_range; use crate::lsp::signature_help::r_signature_help; @@ -279,13 +282,7 @@ pub(crate) fn handle_goto_definition( let uri = ¶ms.text_document_position_params.text_document.uri; let document = state.get_document(uri)?; - if std::env::var("ARK_OAK_GOTO_DEF").is_ok() { - Ok(goto_definition(document, params, state).log_err().flatten()) - } else { - Ok(goto_definition_legacy::goto_definition(document, params) - .log_err() - .flatten()) - } + Ok(goto_definition(document, params).log_err().flatten()) } #[tracing::instrument(level = "info", skip_all)] @@ -334,6 +331,25 @@ pub(crate) fn handle_references( } } +#[tracing::instrument(level = "info", skip_all)] +pub(crate) fn handle_prepare_rename( + params: TextDocumentPositionParams, + state: &WorldState, +) -> LspResult> { + // Don't propagate errors to the frontend + Ok(rename::prepare_rename(params, state).log_err().flatten()) +} + +#[tracing::instrument(level = "info", skip_all)] +pub(crate) fn handle_rename( + params: RenameParams, + state: &WorldState, +) -> LspResult> { + // Propagate error to the frontend (unlike `prepare_rename()`) to give + // actionable feedback to the user + Ok(rename::rename(params, state)?) +} + #[tracing::instrument(level = "info", skip_all)] pub(crate) fn handle_statement_range( params: StatementRangeParams, diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index daf65c0667..0395157729 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -12,7 +12,6 @@ use std::path::PathBuf; use std::pin::Pin; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; -use std::sync::Arc; use std::sync::LazyLock; use std::sync::RwLock; @@ -20,8 +19,6 @@ use anyhow::anyhow; use futures::stream::FuturesUnordered; use futures::StreamExt; use oak_semantic::library::Library; -use oak_sources::PackageCache; -use stdext::result::ResultExt; use tokio::sync::mpsc; use tokio::sync::mpsc::unbounded_channel as tokio_unbounded_channel; use tokio::task; @@ -189,7 +186,6 @@ impl GlobalState { /// and auxiliary loop. pub(crate) fn new( client: Client, - r_home: PathBuf, console_notification_tx: TokioUnboundedSender, ) -> Self { // Transmission channel for the main loop events. Shared with the @@ -219,12 +215,7 @@ impl GlobalState { let library_paths: Vec = library_paths.into_iter().map(PathBuf::from).collect(); - let r = harp::command::r_executable(&r_home); - let package_sources = r - .and_then(|r| PackageCache::new(r, library_paths.clone()).log_err()) - .map(|cache| Arc::new(cache) as Arc); - - let library = Library::new(library_paths, package_sources); + let library = Library::new(library_paths); Self { world: WorldState::new(library), @@ -379,6 +370,12 @@ impl GlobalState { LspRequest::References(params) => { respond(tx, || handlers::handle_references(params, &self.world), LspResponse::References)?; }, + LspRequest::PrepareRename(params) => { + respond(tx, || handlers::handle_prepare_rename(params, &self.world), LspResponse::PrepareRename)?; + }, + LspRequest::Rename(params) => { + respond(tx, || handlers::handle_rename(params, &self.world), LspResponse::Rename)?; + }, LspRequest::StatementRange(params) => { respond(tx, || handlers::handle_statement_range(params, &self.world), LspResponse::StatementRange)?; }, @@ -499,7 +496,12 @@ fn respond( let out = match response { RequestResponse::Result(Ok(_)) => Ok(()), RequestResponse::Result(Err(ref error)) => { - Err(anyhow!("Error while handling request:\n{error:?}")) + // The error has already been sent to the client on `response_tx` + // as a jsonrpc error, so the user sees the popup. Log here at + // info level (with `{:?}` for the full debug format including a + // backtrace) so server logs keep diagnostic context. + lsp::log_info!("Error while handling request:\n{error:?}"); + Ok(()) }, RequestResponse::Crashed(ref error) => { Err(anyhow!("Crashed while handling request:\n{error:?}")) diff --git a/crates/ark/src/lsp/rename.rs b/crates/ark/src/lsp/rename.rs new file mode 100644 index 0000000000..fbf4572189 --- /dev/null +++ b/crates/ark/src/lsp/rename.rs @@ -0,0 +1,83 @@ +use std::collections::HashMap; + +use aether_lsp_utils::proto::from_proto; +use aether_lsp_utils::proto::to_proto; +use tower_lsp::lsp_types; +use tower_lsp::lsp_types::PrepareRenameResponse; +use tower_lsp::lsp_types::RenameParams; +use tower_lsp::lsp_types::TextDocumentPositionParams; +use tower_lsp::lsp_types::TextEdit; +use tower_lsp::lsp_types::WorkspaceEdit; + +use crate::lsp::state::WorldState; + +pub(crate) fn prepare_rename( + params: TextDocumentPositionParams, + state: &WorldState, +) -> anyhow::Result> { + let uri = params.text_document.uri; + let position = params.position; + let document = state.get_document(&uri)?; + + let offset = from_proto::offset_from_position( + position, + &document.line_index, + document.position_encoding, + )?; + let index = document.semantic_index(); + let tree = document.syntax()?; + let pos = oak_ide::FileOffset { file: uri, offset }; + + let Some((range, placeholder)) = oak_ide::prepare_rename(&index, &tree, &pos) else { + return Ok(None); + }; + + let range = to_proto::range(range, &document.line_index, document.position_encoding)?; + Ok(Some(PrepareRenameResponse::RangeWithPlaceholder { + range, + placeholder, + })) +} + +pub(crate) fn rename( + params: RenameParams, + state: &WorldState, +) -> anyhow::Result> { + let uri = params.text_document_position.text_document.uri; + let position = params.text_document_position.position; + let new_name = params.new_name; + let document = state.get_document(&uri)?; + + let offset = from_proto::offset_from_position( + position, + &document.line_index, + document.position_encoding, + )?; + let index = document.semantic_index(); + let root = document.syntax()?; + let pos = oak_ide::FileOffset { + file: uri.clone(), + offset, + }; + + let targets = oak_ide::rename(&index, &root, &pos, &new_name)?; + + // All edits target the current file (intra-file rename). + let mut edits: Vec = Vec::with_capacity(targets.ranges.len()); + for r in targets.ranges { + let range = to_proto::range(r.range, &document.line_index, document.position_encoding)?; + edits.push(TextEdit { + range, + new_text: targets.new_text.clone(), + }); + } + + let mut changes: HashMap> = HashMap::new(); + changes.insert(uri, edits); + + Ok(Some(WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + })) +} diff --git a/crates/ark/src/lsp/state.rs b/crates/ark/src/lsp/state.rs index 7f529caedc..7a728e7f5c 100644 --- a/crates/ark/src/lsp/state.rs +++ b/crates/ark/src/lsp/state.rs @@ -1,48 +1,14 @@ -use std::borrow::Cow; use std::collections::HashMap; -use std::collections::HashSet; use std::path::Path; -use std::path::PathBuf; use anyhow::anyhow; -use oak_core::file::list_r_files; -use oak_db::semantic_index_with_source_resolver; -use oak_db::LegacyDb; -use oak_ide::ExternalScope; use oak_semantic::library::Library; -use oak_semantic::scope_layer::default_search_path; -use oak_semantic::scope_layer::file_layers; -use oak_semantic::scope_layer::package_root_layers; -use oak_semantic::semantic_index::SemanticIndex; -use oak_semantic::SourceResolution; -use stdext::result::ResultExt; use url::Url; use crate::lsp::config::LspConfig; use crate::lsp::document::Document; use crate::lsp::inputs::source_root::SourceRoot; -impl LegacyDb for WorldState { - fn semantic_index(&self, file: &Url) -> Option { - let doc = self.workspace_document(file)?; - let source_root = self.source_root(file); - - let mut stack = HashSet::new(); - stack.insert(file.clone()); - - let index = semantic_index_with_source_resolver(&doc.parse.tree(), |path| { - let dir = source_root.as_ref()?; - self.resolve_source(dir, path, &mut stack) - }); - - Some(index) - } - - fn library(&self) -> &Library { - &self.library - } -} - #[derive(Clone, Default, Debug)] /// The world state, i.e. all the inputs necessary for analysing or refactoring /// code. This is a pure value. There is no interior mutability in this data @@ -122,197 +88,6 @@ impl WorldState { Err(anyhow!("Can't find document for URI {uri}")) } } - - /// Look up a document by URL: returns an open document if available, - /// otherwise reads from disk. - /// - /// TODO(salsa): Replace with a proper VFS so non-opened workspace documents - /// are cached rather than re-read on every query. - pub(crate) fn workspace_document(&self, uri: &Url) -> Option> { - if let Some(doc) = self.documents.get(uri) { - return Some(Cow::Borrowed(doc)); - } - - let path = uri.to_file_path().log_err()?; - let contents = std::fs::read_to_string(&path).log_err()?; - Some(Cow::Owned(Document::new(&contents, None))) - } - - /// Resolve from the workspace root, falling back to the file's directory. - /// This matches RStudio behaviour. Potential improvements: support - /// `setwd()` calls and find the workspace containing the file if there are - /// multiple. - fn source_root(&self, file: &Url) -> Option { - self.workspace - .folders - .first() - .and_then(|url| url.to_file_path().ok()) - .or_else(|| { - file.to_file_path() - .ok() - .and_then(|p| p.parent().map(|d| d.to_path_buf())) - }) - } - - /// Create the semantic index and scope chain for a particular file. - /// - /// For scripts, the index is built with a source resolver so that - /// `source()` directives carry the sourced file's exports. - /// For packages, cross-file visibility comes from NAMESPACE imports and - /// collation ordering. - pub(crate) fn file_analysis( - &self, - file: &Url, - doc: &Document, - ) -> (SemanticIndex, ExternalScope) { - match self.root { - Some(SourceRoot::Package(ref pkg)) => self.package_file_analysis(file, doc, pkg), - _ => self.script_file_analysis(file, doc), - } - } - - // TODO(salsa): Align with the deferred-resolution model used for scripts. Package - // files should produce Import-kind forwarding definitions and resolve - // through `resolve_definition()`, rather than pre-resolving through the - // scope chain. This will happen naturally once Salsa provides cheap - // cross-file index access. - fn package_file_analysis( - &self, - file: &Url, - doc: &Document, - pkg: &oak_semantic::package::Package, - ) -> (SemanticIndex, ExternalScope) { - let root_layers = package_root_layers(pkg.namespace()); - - // FIXME: This only works for source packages. If we do #1168, then the - // `DESCRIPTION` of an installed package won't live alongside its cached `R/` - // sources. - let r_dir = pkg.path().join("R"); - - // If there is a collation field, we use it as an authoritative source - // for the files in the package (and the order in which they are loaded) - let ordered: Vec = pkg - .description() - .collate() - .map(|names| names.into_iter().map(|n| r_dir.join(n)).collect()) - .unwrap_or_else(|| { - // No collation field, list R files and sort in C order - // (R's default collation) - let mut paths = list_r_files(&r_dir); - paths.sort(); - paths - }); - - let Some(file_path) = file.to_file_path().ok() else { - return (doc.semantic_index(), ExternalScope::default()); - }; - - // Iterate in reverse collation order so later files (which shadow - // earlier ones) come first in the chain. Split at the current file - // to separate predecessors from the full set. - let mut top_level = Vec::new(); - let mut lazy = Vec::new(); - let mut past_current = false; - - for path in ordered.iter().rev() { - if *path == file_path { - past_current = true; - continue; - } - - let Some(uri) = Url::from_file_path(path).log_err() else { - continue; - }; - let Some(doc) = self.workspace_document(&uri) else { - continue; - }; - - let layers = file_layers(uri.clone(), &doc.semantic_index()); - lazy.extend(layers.clone()); - if past_current { - top_level.extend(layers); - } - } - - top_level.extend(root_layers.clone()); - lazy.extend(root_layers); - - ( - doc.semantic_index(), - ExternalScope::package(top_level, lazy), - ) - } - - fn script_file_analysis(&self, file: &Url, doc: &Document) -> (SemanticIndex, ExternalScope) { - let source_root = self.source_root(file); - - let mut stack = HashSet::new(); - stack.insert(file.clone()); - - let index = semantic_index_with_source_resolver(&doc.parse.tree(), |path| { - let dir = source_root.as_ref()?; - self.resolve_source(dir, path, &mut stack) - }); - - let semantic_calls = index.semantic_calls().to_vec(); - ( - index, - ExternalScope::search_path(semantic_calls, default_search_path()), - ) - } - - /// Resolve a `source()` call into a [`SourceResolution`] containing the - /// sourced file's exported names and `library()` package attachments. - /// - /// `stack` tracks files currently being resolved (grey set) to break - /// cycles. A file is added when resolution starts and removed when it - /// finishes, so shared dependencies (diamond patterns) are resolved - /// independently for each parent. - fn resolve_source( - &self, - base_dir: &Path, - path: &str, - stack: &mut HashSet, - ) -> Option { - let resolved = base_dir.join(path); - let url = Url::from_file_path(&resolved).log_err()?; - - if !stack.insert(url.clone()) { - return None; - } - - let Some(sourced_doc) = self.workspace_document(&url) else { - stack.remove(&url); - return None; - }; - - // Build the sourced file's index with a nested resolver so that - // transitive `source()` calls are also resolved. The base - // directory stays the same (workspace root) throughout the chain. - let index = semantic_index_with_source_resolver(&sourced_doc.parse.tree(), |nested_path| { - self.resolve_source(base_dir, nested_path, stack) - }); - - let names: Vec = index - .exports() - .keys() - .map(|name| name.to_string()) - .collect(); - - let packages: Vec = index - .attached_packages() - .into_iter() - .map(|s| s.to_string()) - .collect(); - - stack.remove(&url); - - Some(SourceResolution { - url, - names, - packages, - }) - } } pub(crate) fn with_document( @@ -352,75 +127,3 @@ pub(crate) fn workspace_uris(state: &WorldState) -> Vec { let uris: Vec = state.documents.iter().map(|elt| elt.0.clone()).collect(); uris } - -#[cfg(test)] -mod tests { - use biome_rowan::TextSize; - use oak_semantic::scope_layer::ScopeLayer; - use stdext::assert_not; - - use super::*; - use crate::lsp::document::Document; - use crate::lsp::util::test_path; - - fn make_state(uri: &Url, doc: &Document) -> WorldState { - let mut state = WorldState::default(); - state.documents.insert(uri.clone(), doc.clone()); - state - } - - fn has_package(layers: &[ScopeLayer], name: &str) -> bool { - layers - .iter() - .any(|l| matches!(l, ScopeLayer::PackageExports(p) if p == name)) - } - - #[test] - fn test_script_library_directive_position_sensitive() { - // At top-level, `library()` is position-sensitive: code before - // the call should not see the package, code after should. - // The lazy scope (used for completions etc.) sees all directives. - let code = "inform('hi')\nlibrary(rlang)\ninform('hello')\n"; - let doc = Document::new(code, None); - let uri = test_path("script.R"); - let state = make_state(&uri, &doc); - - let (index, scope) = state.file_analysis(&uri, &doc); - - let before = scope.at(&index, TextSize::from(0)); - assert_not!(has_package(&before, "rlang")); - - let after = scope.at(&index, TextSize::from(code.rfind("inform").unwrap() as u32)); - assert!(has_package(&after, "rlang")); - - assert!(has_package(&scope.lazy(), "rlang")); - } - - #[test] - fn test_script_library_directive_visible_in_function_before_call() { - // Function bodies see all directives regardless of position, - // because the function will typically be called after the - // script has been fully sourced. - let code = "f <- function() inform('hello')\nlibrary(rlang)\n"; - let doc = Document::new(code, None); - let uri = test_path("script.R"); - let state = make_state(&uri, &doc); - - let (index, scope) = state.file_analysis(&uri, &doc); - - let in_function = scope.at(&index, TextSize::from(code.find("inform").unwrap() as u32)); - assert!(has_package(&in_function, "rlang")); - } - - #[test] - fn test_script_without_library_no_extra_packages() { - let code = "inform('hello')\n"; - let doc = Document::new(code, None); - let uri = test_path("script.R"); - let state = make_state(&uri, &doc); - - let (_index, scope) = state.file_analysis(&uri, &doc); - let layers = scope.lazy(); - assert_not!(has_package(&layers, "rlang")); - } -} diff --git a/crates/ark/src/lsp/state_handlers.rs b/crates/ark/src/lsp/state_handlers.rs index 52666c7f3d..8d8d6ecf9b 100644 --- a/crates/ark/src/lsp/state_handlers.rs +++ b/crates/ark/src/lsp/state_handlers.rs @@ -32,6 +32,7 @@ use tower_lsp::lsp_types::InitializeParams; use tower_lsp::lsp_types::InitializeResult; use tower_lsp::lsp_types::OneOf; use tower_lsp::lsp_types::RenameFilesParams; +use tower_lsp::lsp_types::RenameOptions; use tower_lsp::lsp_types::SelectionRangeProviderCapability; use tower_lsp::lsp_types::ServerCapabilities; use tower_lsp::lsp_types::ServerInfo; @@ -164,6 +165,12 @@ pub(crate) fn initialize( type_definition_provider: None, implementation_provider: Some(ImplementationProviderCapability::Simple(true)), references_provider: Some(OneOf::Left(true)), + rename_provider: Some(OneOf::Right(RenameOptions { + prepare_provider: Some(true), + work_done_progress_options: WorkDoneProgressOptions { + work_done_progress: None, + }, + })), document_symbol_provider: Some(OneOf::Left(true)), folding_range_provider: Some(FoldingRangeProviderCapability::Simple(true)), workspace_symbol_provider: Some(OneOf::Left(true)), diff --git a/crates/ark/src/lsp/tests.rs b/crates/ark/src/lsp/tests.rs new file mode 100644 index 0000000000..47b8ec5fd6 --- /dev/null +++ b/crates/ark/src/lsp/tests.rs @@ -0,0 +1,4 @@ +mod find_references; +mod goto_definition; +mod rename; +mod utils; diff --git a/crates/ark/src/lsp/tests/find_references.rs b/crates/ark/src/lsp/tests/find_references.rs new file mode 100644 index 0000000000..ad3c4adc52 --- /dev/null +++ b/crates/ark/src/lsp/tests/find_references.rs @@ -0,0 +1,252 @@ +use tower_lsp::lsp_types; +use tower_lsp::lsp_types::Location; +use tower_lsp::lsp_types::ReferenceContext; +use tower_lsp::lsp_types::ReferenceParams; + +use super::utils::make_state; +use super::utils::range; +use crate::lsp::document::Document; +use crate::lsp::find_references::find_references; +use crate::lsp::util::test_path; + +fn make_params( + uri: lsp_types::Url, + line: u32, + character: u32, + include_decl: bool, +) -> ReferenceParams { + ReferenceParams { + text_document_position: lsp_types::TextDocumentPositionParams { + text_document: lsp_types::TextDocumentIdentifier { uri }, + position: lsp_types::Position::new(line, character), + }, + context: ReferenceContext { + include_declaration: include_decl, + }, + work_done_progress_params: Default::default(), + partial_result_params: Default::default(), + } +} + +#[test] +fn test_intra_file_use_and_def() { + let code = "foo <- 1\nfoo + foo\n"; + let doc = Document::new(code, None); + let uri = test_path("test.R"); + let state = make_state(&uri, &doc); + + // Cursor on the first use at (1, 0) + let params = make_params(uri.clone(), 1, 0, true); + let locs = find_references(params, &state).unwrap(); + let expected: Vec = vec![ + Location::new(uri.clone(), range((0, 0), (0, 3))), + Location::new(uri.clone(), range((1, 0), (1, 3))), + Location::new(uri, range((1, 6), (1, 9))), + ]; + assert_eq!(locs, expected); +} + +#[test] +fn test_excludes_declaration() { + let code = "foo <- 1\nfoo\n"; + let doc = Document::new(code, None); + let uri = test_path("test.R"); + let state = make_state(&uri, &doc); + + let params = make_params(uri.clone(), 1, 0, false); + let locs = find_references(params, &state).unwrap(); + assert_eq!(locs, vec![Location::new(uri, range((1, 0), (1, 3)))]); +} + +#[test] +fn test_no_identifier_returns_empty() { + let code = "x <- 1\n"; + let doc = Document::new(code, None); + let uri = test_path("test.R"); + let state = make_state(&uri, &doc); + + // Cursor on the `<-` operator: no identifier, no refs. + let params = make_params(uri, 0, 3, true); + let locs = find_references(params, &state).unwrap(); + assert!(locs.is_empty()); +} + +#[test] +fn test_cursor_past_trailing_edge_resolves_via_fallback() { + // Some LSP clients send the cursor one past the last character of a + // selected identifier (typical for double-click then "find references"). + // We exercise this through the cross-file fallback because `mutate` is + // unbound: the intra-file pass returns nothing, and `build_context`'s + // boundary retry pulls the cursor back one column onto the identifier + // before the textual walk runs. + let dir = tempfile::tempdir().unwrap(); + + let file1_path = dir.path().join("a.R"); + std::fs::write(&file1_path, "mutate\n").unwrap(); + let file1_uri = lsp_types::Url::from_file_path(&file1_path).unwrap(); + + let file2_path = dir.path().join("b.R"); + std::fs::write(&file2_path, "mutate\n").unwrap(); + let file2_uri = lsp_types::Url::from_file_path(&file2_path).unwrap(); + + let doc1 = Document::new("mutate\n", None); + let mut state = make_state(&file1_uri, &doc1); + state.workspace.folders = vec![lsp_types::Url::from_directory_path(dir.path()).unwrap()]; + + // Cursor at column 6: one past the last character of `mutate` (which + // spans columns 0..6). + let params = make_params(file1_uri.clone(), 0, 6, true); + let locs = find_references(params, &state).unwrap(); + + assert!(locs + .iter() + .any(|l| l.uri == file1_uri && l.range == range((0, 0), (0, 6)))); + assert!(locs + .iter() + .any(|l| l.uri == file2_uri && l.range == range((0, 0), (0, 6)))); +} + +#[test] +fn test_cross_file_walks_for_unbound_symbol() { + // file1 uses `mutate` but doesn't define it locally. file2 also has + // `mutate`. Cursor on file1's `mutate` -- intra-file returns nothing + // (unbound), so the cross-file textual walk fires and picks up + // file2's occurrences. This is not ideal behaviour, it's part of the legacy + // Ark fallback handling. + let dir = tempfile::tempdir().unwrap(); + + let file1_path = dir.path().join("a.R"); + std::fs::write(&file1_path, "mutate\n").unwrap(); + let file1_uri = lsp_types::Url::from_file_path(&file1_path).unwrap(); + + let file2_path = dir.path().join("b.R"); + std::fs::write(&file2_path, "mutate\nmutate + 1\n").unwrap(); + let file2_uri = lsp_types::Url::from_file_path(&file2_path).unwrap(); + + let doc1 = Document::new("mutate\n", None); + let mut state = make_state(&file1_uri, &doc1); + state.workspace.folders = vec![lsp_types::Url::from_directory_path(dir.path()).unwrap()]; + + let params = make_params(file1_uri.clone(), 0, 0, true); + let locs = find_references(params, &state).unwrap(); + + // The current file's occurrence (textual walk picks it up too, + // since intra-file resolution gave nothing to dedup against). + assert!(locs + .iter() + .any(|l| l.uri == file1_uri && l.range == range((0, 0), (0, 6)))); + // file2's two `mutate` occurrences. + assert!(locs + .iter() + .any(|l| l.uri == file2_uri && l.range == range((0, 0), (0, 6)))); + assert!(locs + .iter() + .any(|l| l.uri == file2_uri && l.range == range((1, 0), (1, 6)))); +} + +#[test] +fn test_cross_file_walk_respects_dollar_kind() { + // Cursor on `bar` in `foo$bar`. Intra-file gives nothing (member names + // aren't tracked by the semantic index), so the cross-file walk fires. + // The walk's `ReferenceKind` discrimination should match `bar` only + // when it's also the RHS of a `$`, not plain identifier occurrences. + let dir = tempfile::tempdir().unwrap(); + + let file1_path = dir.path().join("a.R"); + std::fs::write(&file1_path, "foo <- list()\nfoo$bar\n").unwrap(); + let file1_uri = lsp_types::Url::from_file_path(&file1_path).unwrap(); + + let file2_path = dir.path().join("b.R"); + std::fs::write(&file2_path, "foo$bar\nbar\n").unwrap(); + let file2_uri = lsp_types::Url::from_file_path(&file2_path).unwrap(); + + let doc1 = Document::new("foo <- list()\nfoo$bar\n", None); + let mut state = make_state(&file1_uri, &doc1); + state.workspace.folders = vec![lsp_types::Url::from_directory_path(dir.path()).unwrap()]; + + // Cursor on `bar` (RHS of `$`) in file1, line 1 col 4. + let params = make_params(file1_uri.clone(), 1, 4, true); + let locs = find_references(params, &state).unwrap(); + + // file1's own `foo$bar` `bar`. + assert!(locs + .iter() + .any(|l| l.uri == file1_uri && l.range == range((1, 4), (1, 7)))); + // file2's `foo$bar` `bar` (Dollar kind, matches). + assert!(locs + .iter() + .any(|l| l.uri == file2_uri && l.range == range((0, 4), (0, 7)))); + // file2's plain `bar` (Symbol kind, does NOT match). + assert!(!locs + .iter() + .any(|l| l.uri == file2_uri && l.range == range((1, 0), (1, 3)))); +} + +#[test] +fn test_fixme_cross_file_walk_on_namespace_access() { + // Cursor on `mutate` in `dplyr::mutate`. Intra-file gives nothing + // (NamespaceAccess), so the cross-file walk fires. The kind is + // `Symbol` (parent is a namespace expression, not an extract + // operator), so the walk matches other plain-identifier occurrences + // of `mutate` but not `foo$mutate`. This is the noisy fallback + // behavior; cross-file resolution will refine it later. + let dir = tempfile::tempdir().unwrap(); + + let file1_path = dir.path().join("a.R"); + std::fs::write(&file1_path, "dplyr::mutate\n").unwrap(); + let file1_uri = lsp_types::Url::from_file_path(&file1_path).unwrap(); + + let file2_path = dir.path().join("b.R"); + std::fs::write(&file2_path, "mutate\nfoo$mutate\n").unwrap(); + let file2_uri = lsp_types::Url::from_file_path(&file2_path).unwrap(); + + let doc1 = Document::new("dplyr::mutate\n", None); + let mut state = make_state(&file1_uri, &doc1); + state.workspace.folders = vec![lsp_types::Url::from_directory_path(dir.path()).unwrap()]; + + // Cursor on `mutate` in `dplyr::mutate`, line 0 col 7. + let params = make_params(file1_uri.clone(), 0, 7, true); + let locs = find_references(params, &state).unwrap(); + + // file1's own `mutate` in `dplyr::mutate`. + assert!(locs + .iter() + .any(|l| l.uri == file1_uri && l.range == range((0, 7), (0, 13)))); + // file2's plain `mutate` (Symbol kind, matches). + assert!(locs + .iter() + .any(|l| l.uri == file2_uri && l.range == range((0, 0), (0, 6)))); + // file2's `foo$mutate` `mutate` (Dollar kind, does NOT match). + assert!(!locs + .iter() + .any(|l| l.uri == file2_uri && l.range == range((1, 4), (1, 10)))); +} + +#[test] +fn test_intra_file_match_skips_cross_file_walk() { + // file1 defines + uses `foo` locally. file2 has unrelated `foo`s. + // Cursor on file1's def -- intra-file resolves cleanly, so the + // textual walk is skipped and file2's `foo`s don't pollute the + // result. + let dir = tempfile::tempdir().unwrap(); + + let file1_path = dir.path().join("a.R"); + std::fs::write(&file1_path, "foo <- 1\nfoo\n").unwrap(); + let file1_uri = lsp_types::Url::from_file_path(&file1_path).unwrap(); + + let file2_path = dir.path().join("b.R"); + std::fs::write(&file2_path, "foo <- 99\nfoo\n").unwrap(); + let file2_uri = lsp_types::Url::from_file_path(&file2_path).unwrap(); + + let doc1 = Document::new("foo <- 1\nfoo\n", None); + let mut state = make_state(&file1_uri, &doc1); + state.workspace.folders = vec![lsp_types::Url::from_directory_path(dir.path()).unwrap()]; + + let params = make_params(file1_uri.clone(), 0, 0, true); + let locs = find_references(params, &state).unwrap(); + + // Exactly the two file1 refs. None from file2. + assert_eq!(locs.len(), 2); + assert!(locs.iter().all(|l| l.uri == file1_uri)); + assert!(locs.iter().all(|l| l.uri != file2_uri)); +} diff --git a/crates/ark/src/lsp/tests/goto_definition.rs b/crates/ark/src/lsp/tests/goto_definition.rs new file mode 100644 index 0000000000..0fe97a83f2 --- /dev/null +++ b/crates/ark/src/lsp/tests/goto_definition.rs @@ -0,0 +1,151 @@ +use assert_matches::assert_matches; +use tower_lsp::lsp_types; +use tower_lsp::lsp_types::GotoDefinitionParams; +use tower_lsp::lsp_types::GotoDefinitionResponse; + +use crate::lsp::document::Document; +use crate::lsp::goto_definition::goto_definition; +use crate::lsp::indexer; +use crate::lsp::util::test_path; + +fn make_params(uri: lsp_types::Url, line: u32, character: u32) -> GotoDefinitionParams { + GotoDefinitionParams { + text_document_position_params: lsp_types::TextDocumentPositionParams { + text_document: lsp_types::TextDocumentIdentifier { uri }, + position: lsp_types::Position::new(line, character), + }, + work_done_progress_params: Default::default(), + partial_result_params: Default::default(), + } +} + +#[test] +fn test_goto_definition() { + let code = "foo <- 42\nprint(foo)\n"; + let doc = Document::new(code, None); + let uri = test_path("test.R"); + + let params = make_params(uri, 1, 6); + + assert_matches!( + goto_definition(&doc, params).unwrap(), + Some(GotoDefinitionResponse::Link(ref links)) => { + assert_eq!( + links[0].target_range, + lsp_types::Range { + start: lsp_types::Position::new(0, 0), + end: lsp_types::Position::new(0, 3), + } + ); + } + ); +} + +#[test] +fn test_goto_definition_prefers_local_symbol() { + let code = "foo <- 1\nfoo\n"; + let doc = Document::new(code, None); + let uri = test_path("file.R"); + + let params = make_params(uri.clone(), 1, 0); + + assert_matches!( + goto_definition(&doc, params).unwrap(), + Some(GotoDefinitionResponse::Link(ref links)) => { + assert_eq!(links[0].target_uri, uri); + assert_eq!( + links[0].target_range, + lsp_types::Range { + start: lsp_types::Position::new(0, 0), + end: lsp_types::Position::new(0, 3), + } + ); + } + ); +} + +#[test] +fn test_unbound_identifier_returns_none() { + // A free identifier with no local def and no indexer entry returns + // `None`, matching how rust-analyzer and ty handle the same case. + let _guard = indexer::ResetIndexerGuard; + + let doc = Document::new("foo\n", None); + let uri = test_path("file.R"); + + let params = make_params(uri, 0, 0); + assert_eq!(goto_definition(&doc, params).unwrap(), None); +} + +#[test] +fn test_cursor_on_operator_returns_none() { + // Cursor on `<-` (a non-identifier token): even though the file has + // an identifier nearby, the operator itself doesn't resolve to + // anything, so the response is `None`. + let _guard = indexer::ResetIndexerGuard; + + let other = Document::new("foo <- function() 'other'\n", None); + let other_uri = test_path("other.R"); + indexer::update(&other, &other_uri).unwrap(); + + let doc = Document::new("foo <- 1\n", None); + let uri = test_path("file.R"); + + // Cursor on the `<` of `<-` at column 4. + let params = make_params(uri, 0, 4); + assert_eq!(goto_definition(&doc, params).unwrap(), None); +} + +#[test] +fn test_fallback_resolves_cross_file() { + // A free variable that the intra-file resolver can't bind falls back + // to the workspace indexer, which finds the definition in another + // indexed file. + let _guard = indexer::ResetIndexerGuard; + + let doc1 = Document::new("foo <- function() 1\n", None); + let uri1 = test_path("defs.R"); + indexer::update(&doc1, &uri1).unwrap(); + + let doc2 = Document::new("foo\n", None); + let uri2 = test_path("uses.R"); + + let params = make_params(uri2, 0, 0); + assert_matches!( + goto_definition(&doc2, params).unwrap(), + Some(GotoDefinitionResponse::Link(ref links)) => { + assert_eq!(links.len(), 1); + assert_eq!(links[0].target_uri, uri1); + } + ); +} + +#[test] +fn test_fallback_skipped_when_local_def_wins() { + // When the symbol has a binding within the file, the within-file + // result is returned and the indexer fallback isn't consulted. + let _guard = indexer::ResetIndexerGuard; + + let other = Document::new("foo <- function() 'other'\n", None); + let other_uri = test_path("other.R"); + indexer::update(&other, &other_uri).unwrap(); + + let code = "foo <- 1\nfoo\n"; + let doc = Document::new(code, None); + let uri = test_path("main.R"); + + let params = make_params(uri.clone(), 1, 0); + assert_matches!( + goto_definition(&doc, params).unwrap(), + Some(GotoDefinitionResponse::Link(ref links)) => { + assert_eq!(links[0].target_uri, uri); + assert_eq!( + links[0].target_range, + lsp_types::Range { + start: lsp_types::Position::new(0, 0), + end: lsp_types::Position::new(0, 3), + } + ); + } + ); +} diff --git a/crates/ark/src/lsp/tests/rename.rs b/crates/ark/src/lsp/tests/rename.rs new file mode 100644 index 0000000000..92fdd9befe --- /dev/null +++ b/crates/ark/src/lsp/tests/rename.rs @@ -0,0 +1,129 @@ +use tower_lsp::lsp_types; +use tower_lsp::lsp_types::PrepareRenameResponse; +use tower_lsp::lsp_types::RenameParams; +use tower_lsp::lsp_types::TextDocumentPositionParams; +use tower_lsp::lsp_types::TextEdit; + +use super::utils::make_state; +use super::utils::range; +use crate::lsp::document::Document; +use crate::lsp::rename::prepare_rename; +use crate::lsp::rename::rename; +use crate::lsp::util::test_path; + +fn make_prepare_params( + uri: lsp_types::Url, + line: u32, + character: u32, +) -> TextDocumentPositionParams { + TextDocumentPositionParams { + text_document: lsp_types::TextDocumentIdentifier { uri }, + position: lsp_types::Position::new(line, character), + } +} + +fn make_rename_params( + uri: lsp_types::Url, + line: u32, + character: u32, + new_name: &str, +) -> RenameParams { + RenameParams { + text_document_position: TextDocumentPositionParams { + text_document: lsp_types::TextDocumentIdentifier { uri }, + position: lsp_types::Position::new(line, character), + }, + new_name: new_name.to_string(), + work_done_progress_params: Default::default(), + } +} + +#[test] +fn test_prepare_rename_returns_range_and_placeholder() { + let code = "foo <- 1\nfoo\n"; + let doc = Document::new(code, None); + let uri = test_path("test.R"); + let state = make_state(&uri, &doc); + + let params = make_prepare_params(uri, 0, 0); + let result = prepare_rename(params, &state).unwrap().unwrap(); + + let PrepareRenameResponse::RangeWithPlaceholder { + range: r, + placeholder, + } = result + else { + panic!("expected RangeWithPlaceholder"); + }; + assert_eq!(r, range((0, 0), (0, 3))); + assert_eq!(placeholder, "foo"); +} + +#[test] +fn test_prepare_rename_on_namespace_access_returns_none() { + let code = "dplyr::mutate\n"; + let doc = Document::new(code, None); + let uri = test_path("test.R"); + let state = make_state(&uri, &doc); + + let params = make_prepare_params(uri, 0, 7); + assert!(prepare_rename(params, &state).unwrap().is_none()); +} + +#[test] +fn test_rename_emits_edits_for_def_and_uses() { + let code = "foo <- 1\nfoo + foo\n"; + let doc = Document::new(code, None); + let uri = test_path("test.R"); + let state = make_state(&uri, &doc); + + let params = make_rename_params(uri.clone(), 0, 0, "bar"); + let edit = rename(params, &state).unwrap().unwrap(); + + let changes = edit.changes.expect("changes map"); + assert_eq!(changes.len(), 1); + let edits = changes.get(&uri).expect("edits for uri"); + let expected: Vec = vec![ + TextEdit { + range: range((0, 0), (0, 3)), + new_text: "bar".to_string(), + }, + TextEdit { + range: range((1, 0), (1, 3)), + new_text: "bar".to_string(), + }, + TextEdit { + range: range((1, 6), (1, 9)), + new_text: "bar".to_string(), + }, + ]; + assert_eq!(edits, &expected); +} + +#[test] +fn test_rename_to_reserved_word_errors() { + // Reserved word as new name. `rename` returns an `Err`, which + // `handle_rename` propagates to the client so the editor can show + // the message inline. + let code = "foo <- 1\n"; + let doc = Document::new(code, None); + let uri = test_path("test.R"); + let state = make_state(&uri, &doc); + + let params = make_rename_params(uri, 0, 0, "if"); + let err = rename(params, &state).unwrap_err(); + assert!(err.to_string().contains("reserved")); +} + +#[test] +fn test_rename_to_name_with_space_wraps_in_backticks() { + let code = "foo <- 1\nfoo\n"; + let doc = Document::new(code, None); + let uri = test_path("test.R"); + let state = make_state(&uri, &doc); + + let params = make_rename_params(uri.clone(), 0, 0, "new name"); + let edit = rename(params, &state).unwrap().unwrap(); + let edits = edit.changes.unwrap().remove(&uri).unwrap(); + assert!(edits.iter().all(|e| e.new_text == "`new name`")); +} diff --git a/crates/ark/src/lsp/tests/utils.rs b/crates/ark/src/lsp/tests/utils.rs new file mode 100644 index 0000000000..94c63b25d3 --- /dev/null +++ b/crates/ark/src/lsp/tests/utils.rs @@ -0,0 +1,17 @@ +use tower_lsp::lsp_types; + +use crate::lsp::document::Document; +use crate::lsp::state::WorldState; + +pub(super) fn make_state(uri: &lsp_types::Url, doc: &Document) -> WorldState { + let mut state = WorldState::default(); + state.documents.insert(uri.clone(), doc.clone()); + state +} + +pub(super) fn range(start: (u32, u32), end: (u32, u32)) -> lsp_types::Range { + lsp_types::Range { + start: lsp_types::Position::new(start.0, start.1), + end: lsp_types::Position::new(end.0, end.1), + } +} diff --git a/crates/ark/src/start.rs b/crates/ark/src/start.rs index f7566037f2..90b35ea6da 100644 --- a/crates/ark/src/start.rs +++ b/crates/ark/src/start.rs @@ -89,7 +89,6 @@ pub fn start_kernel( // Not all Amalthea kernels provide these, but ark does. // They must be able to deliver messages to the shell channel directly. let lsp = Arc::new(Mutex::new(lsp::handler::Lsp::new( - r_home.clone(), kernel_init_tx.add_rx(), console_notification_tx.clone(), ))); diff --git a/crates/ark/tests/lsp.rs b/crates/ark/tests/lsp.rs index 6f4c5568c7..0ac9b25b33 100644 --- a/crates/ark/tests/lsp.rs +++ b/crates/ark/tests/lsp.rs @@ -9,6 +9,7 @@ // kernel, i.e. LSP features that require dynamic access to the R session. use ark_test::DummyArkFrontend; +use serde_json::json; #[test] fn test_lsp_init() { @@ -18,3 +19,46 @@ fn test_lsp_init() { // Verify the server reports completion support assert!(lsp.server_capabilities().completion_provider.is_some()); } + +// The two cases below test errors that don't depend on the rename +// implementation's resolution capabilities. New-name validation always +// applies (R language constraints), so these tests stay valid once +// cross-file rename lands. +// +// They also pin the wire format: an exact `assert_eq!` catches both +// `Anyhow(...)` wrapping and `Stack backtrace:` blocks that anyhow's +// `{:?}` formatting would smuggle into the editor popup. + +#[test] +fn test_rename_to_reserved_word_returns_clean_error() { + let frontend = DummyArkFrontend::lock(); + let mut lsp = frontend.start_lsp(); + + let uri = lsp.open_document("rename_reserved.R", "foo <- 1\n"); + + let params = json!({ + "textDocument": { "uri": uri }, + "position": { "line": 0, "character": 0 }, + "newName": "if", + }); + let message = lsp.send_request_expect_error("textDocument/rename", params); + + assert_eq!(message, "`if` is a reserved word in R"); +} + +#[test] +fn test_rename_to_empty_name_returns_clean_error() { + let frontend = DummyArkFrontend::lock(); + let mut lsp = frontend.start_lsp(); + + let uri = lsp.open_document("rename_empty.R", "foo <- 1\n"); + + let params = json!({ + "textDocument": { "uri": uri }, + "position": { "line": 0, "character": 0 }, + "newName": "", + }); + let message = lsp.send_request_expect_error("textDocument/rename", params); + + assert_eq!(message, "Identifier cannot be empty"); +} diff --git a/crates/ark_test/src/lsp_client.rs b/crates/ark_test/src/lsp_client.rs index b19f5f6bf8..1f9208dce1 100644 --- a/crates/ark_test/src/lsp_client.rs +++ b/crates/ark_test/src/lsp_client.rs @@ -198,6 +198,27 @@ impl LspClient { self.recv_response(method, id) } + /// Send a JSON-RPC request expecting an error response. Returns the + /// error message string. Panics if the response is a success or has no + /// `message` field. + #[track_caller] + pub fn send_request_expect_error(&mut self, method: &str, params: Value) -> String { + self.next_id += 1; + let id = self.next_id; + + let mut message = json!({ + "jsonrpc": "2.0", + "id": id, + "method": method, + }); + if !params.is_null() { + message["params"] = params; + } + + self.send_raw(&message); + self.recv_error_response(method, id) + } + /// Send a JSON-RPC notification (no response expected). pub fn send_notification(&mut self, method: &str, params: Value) { let mut message = json!({ @@ -278,6 +299,46 @@ impl LspClient { } } + /// Receive an expected error response for `id`. Returns the error + /// `message` field. Panics on success responses or malformed errors. + #[track_caller] + fn recv_error_response(&mut self, method: &str, id: i64) -> String { + loop { + match self.recv_any() { + LspMessage::Response { + id: msg_id, + result: _, + error, + } => { + assert_eq!( + msg_id, id, + "Response id mismatch: expected {id}, got {msg_id}" + ); + let error = + error.unwrap_or_else(|| panic!("Expected error response for `{method}`")); + return error["message"] + .as_str() + .unwrap_or_else(|| { + panic!("Error response for `{method}` has no message: {error}") + }) + .to_string(); + }, + LspMessage::ServerRequest { + method: req_method, .. + } => { + panic!( + "Unexpected LSP server request `{req_method}` while waiting for response to `{method}`" + ); + }, + LspMessage::Notification { diagnostics } => { + if let Some(params) = diagnostics { + self.diagnostics.insert(params.uri, params.diagnostics); + } + }, + } + } + } + /// Receive the next server-to-client request, assert its method, and /// auto-reply with an empty success so the server doesn't block. /// diff --git a/crates/oak_core/Cargo.toml b/crates/oak_core/Cargo.toml index 800d7af852..41fb5f1e40 100644 --- a/crates/oak_core/Cargo.toml +++ b/crates/oak_core/Cargo.toml @@ -15,6 +15,7 @@ workspace = true [dependencies] aether_syntax.workspace = true +anyhow.workspace = true biome_rowan.workspace = true [dev-dependencies] diff --git a/crates/oak_core/src/identifier.rs b/crates/oak_core/src/identifier.rs new file mode 100644 index 0000000000..b9257435c2 --- /dev/null +++ b/crates/oak_core/src/identifier.rs @@ -0,0 +1,169 @@ +//! R identifier syntax and reserved words. +//! +//! Language-level facts about what counts as a valid R identifier. Used +//! by rename, diagnostics, completions, and anything else that needs to +//! emit or recognise identifier text. + +use anyhow::anyhow; + +/// Convert a semantic name into its canonical R source form. +/// +/// A name that parses as a plain R identifier (and isn't a reserved +/// word) returns as-is. Anything else gets wrapped in backticks. Empty +/// names, reserved words, and names containing a literal backtick return +/// `Err` (a backtick can't appear inside a backtick-quoted identifier). +pub fn to_identifier_text(name: &str) -> anyhow::Result { + if name.is_empty() { + return Err(anyhow!("Identifier cannot be empty")); + } + if is_reserved(name) { + return Err(anyhow!("`{name}` is a reserved word in R")); + } + if is_valid_identifier(name) { + return Ok(name.to_string()); + } + if name.contains('`') { + return Err(anyhow!("Identifier cannot contain a backtick")); + } + Ok(format!("`{name}`")) +} + +/// Whether `name` is a valid bare R identifier (no backticks needed). +/// +/// R's rule: starts with a letter or `.`, then letters, digits, `.`, or +/// `_`. A leading `.` followed by a digit is a number literal, not an +/// identifier (e.g. `.5`). +pub fn is_valid_identifier(name: &str) -> bool { + let mut chars = name.chars(); + let Some(first) = chars.next() else { + return false; + }; + if !(first.is_ascii_alphabetic() || first == '.') { + return false; + } + if first == '.' { + if let Some(second) = name.chars().nth(1) { + if second.is_ascii_digit() { + return false; + } + } + } + chars.all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '_') +} + +/// R reserved words that cannot be used as identifier names. Source: +/// `?Reserved` in R. Note that `return` is a function, not a reserved +/// word, so it's missing from this list (`return <- 1` is valid R). +/// `_` became reserved in R 4.2 for use as the `|>` pipe placeholder. +pub fn is_reserved(name: &str) -> bool { + matches!( + name, + "if" | "else" | + "for" | + "while" | + "repeat" | + "break" | + "next" | + "function" | + "in" | + "TRUE" | + "FALSE" | + "NULL" | + "NA" | + "NA_integer_" | + "NA_real_" | + "NA_complex_" | + "NA_character_" | + "NaN" | + "Inf" | + "..." | + "_" + ) || is_dot_dot_n(name) +} + +/// `..1`, `..2`, ..., the variadic positional accessors. Listed in +/// `?Reserved` as "..1, ..2 etc.". +fn is_dot_dot_n(name: &str) -> bool { + let Some(rest) = name.strip_prefix("..") else { + return false; + }; + !rest.is_empty() && rest.chars().all(|c| c.is_ascii_digit()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_valid_identifiers() { + assert!(is_valid_identifier("foo")); + assert!(is_valid_identifier(".foo")); + assert!(is_valid_identifier("foo.bar")); + assert!(is_valid_identifier("foo_bar")); + assert!(is_valid_identifier("foo123")); + } + + #[test] + fn test_invalid_identifiers() { + assert!(!is_valid_identifier("")); + assert!(!is_valid_identifier("1foo")); + assert!(!is_valid_identifier("_foo")); + assert!(!is_valid_identifier(".1foo")); + assert!(!is_valid_identifier("foo bar")); + assert!(!is_valid_identifier("foo-bar")); + } + + #[test] + fn test_reserved_words() { + for word in [ + "if", "for", "function", "TRUE", "FALSE", "NULL", "NA", "...", "_", + ] { + assert!(is_reserved(word)); + } + // `return` is a function, not reserved (`return <- 1` is valid R). + assert!(!is_reserved("return")); + // `T` / `F` are reassignable aliases for TRUE/FALSE. + assert!(!is_reserved("T")); + assert!(!is_reserved("F")); + assert!(!is_reserved("foo")); + } + + #[test] + fn test_dot_dot_n_is_reserved() { + assert!(is_reserved("..1")); + assert!(is_reserved("..2")); + assert!(is_reserved("..42")); + // `..` alone is just an identifier. + assert!(!is_reserved("..")); + // `..foo` is not reserved (variadic accessors require digits). + assert!(!is_reserved("..foo")); + // `.1` is a number literal, not even an identifier. + assert!(!is_reserved(".1")); + } + + #[test] + fn test_to_identifier_text_plain() { + assert_eq!(to_identifier_text("foo").unwrap(), "foo"); + } + + #[test] + fn test_to_identifier_text_wraps_non_identifier() { + assert_eq!(to_identifier_text("foo bar").unwrap(), "`foo bar`"); + assert_eq!(to_identifier_text("1foo").unwrap(), "`1foo`"); + } + + #[test] + fn test_to_identifier_text_rejects_empty() { + assert!(to_identifier_text("").is_err()); + } + + #[test] + fn test_to_identifier_text_rejects_reserved() { + assert!(to_identifier_text("if").is_err()); + } + + #[test] + fn test_to_identifier_text_rejects_backtick() { + assert!(to_identifier_text("foo`bar").is_err()); + } +} diff --git a/crates/oak_core/src/lib.rs b/crates/oak_core/src/lib.rs index 8123137526..36dc1bb20f 100644 --- a/crates/oak_core/src/lib.rs +++ b/crates/oak_core/src/lib.rs @@ -1,4 +1,5 @@ pub mod declaration; pub mod file; +pub mod identifier; pub mod range; pub mod syntax_ext; diff --git a/crates/oak_db/src/legacy.rs b/crates/oak_db/src/legacy.rs deleted file mode 100644 index b7c7a47e0b..0000000000 --- a/crates/oak_db/src/legacy.rs +++ /dev/null @@ -1,51 +0,0 @@ -use aether_syntax::RRoot; -use oak_semantic::library::Library; -use oak_semantic::semantic_index::SemanticIndex; -use oak_semantic::ImportsResolver; -use oak_semantic::SourceResolution; -use url::Url; - -/// Legacy database trait used by the tree-sitter backed code path. -/// -/// Exists only during the Salsa migration. -pub trait LegacyDb { - /// `None` means the file disappeared between index-build and query time, - /// which is an edge case, not a normal path. With Salsa inputs this - /// becomes infallible. - fn semantic_index(&self, file: &Url) -> Option; - fn library(&self) -> &Library; -} - -/// Build a [`SemanticIndex`] with cross-file `source()` resolution -/// driven by a `FnMut` callback. -/// -/// Used by `ark::lsp::state::WorldState`'s tree-sitter backed -/// resolver, which closes over LSP state (workspace document map, -/// cycle-detection stack) to satisfy [`ImportsResolver`] without -/// defining a struct. -/// -/// TODO(salsa): retire alongside the LSP migration to oak_db's -/// `File::semantic_index` tracked query. Once `ark::lsp::state` -/// switches to consuming the tracked query (or implements its own -/// [`ImportsResolver`] directly), this function and its private -/// [`CallbackImportsResolver`] adapter can be deleted. -pub fn semantic_index_with_source_resolver( - root: &RRoot, - resolver: impl FnMut(&str) -> Option, -) -> SemanticIndex { - let resolver = CallbackImportsResolver(resolver); - oak_semantic::build_index(root, resolver) -} - -struct CallbackImportsResolver(F) -where - F: FnMut(&str) -> Option; - -impl ImportsResolver for CallbackImportsResolver -where - F: FnMut(&str) -> Option, -{ - fn resolve_source(&mut self, path: &str) -> Option { - (self.0)(path) - } -} diff --git a/crates/oak_db/src/lib.rs b/crates/oak_db/src/lib.rs index 3e0e8a306a..b8879a9f4d 100644 --- a/crates/oak_db/src/lib.rs +++ b/crates/oak_db/src/lib.rs @@ -6,7 +6,6 @@ mod file_imports; mod file_resolve; mod imports; mod inputs; -mod legacy; mod name; mod parse; mod storage; @@ -27,7 +26,5 @@ pub use inputs::Package; pub use inputs::Root; pub use inputs::RootKind; pub use inputs::WorkspaceRoots; -pub use legacy::semantic_index_with_source_resolver; -pub use legacy::LegacyDb; pub use name::Name; pub use storage::OakDatabase; diff --git a/crates/oak_ide/Cargo.toml b/crates/oak_ide/Cargo.toml index c362a4efc9..05820636e8 100644 --- a/crates/oak_ide/Cargo.toml +++ b/crates/oak_ide/Cargo.toml @@ -15,18 +15,12 @@ workspace = true [dependencies] aether_syntax.workspace = true +anyhow.workspace = true biome_rowan.workspace = true -log.workspace = true oak_core.workspace = true -oak_db.workspace = true oak_semantic.workspace = true -oak_sources.workspace = true -stdext.workspace = true url.workspace = true [dev-dependencies] aether_parser.workspace = true -assert_matches.workspace = true -oak_package_metadata.workspace = true oak_semantic = { workspace = true, features = ["testing"] } -oak_sources = { workspace = true, features = ["testing"] } diff --git a/crates/oak_ide/src/external_scope.rs b/crates/oak_ide/src/external_scope.rs deleted file mode 100644 index f3605457ef..0000000000 --- a/crates/oak_ide/src/external_scope.rs +++ /dev/null @@ -1,136 +0,0 @@ -use std::borrow::Cow; - -use biome_rowan::TextSize; -use oak_semantic::scope_layer::ScopeLayer; -use oak_semantic::semantic_index::ScopeKind; -use oak_semantic::semantic_index::SemanticCall; -use oak_semantic::semantic_index::SemanticCallKind; -use oak_semantic::semantic_index::SemanticIndex; -use oak_semantic::ScopeId; - -/// The external scope chain for a file, determined by its project context. -#[derive(Debug)] -pub enum ExternalScope { - /// File inside an R package. The scope chain includes layers from other - /// package files (ordered by collation, later files shadow earlier ones) - /// and from NAMESPACE imports (`importFrom`, `import`), with base - /// at the bottom. Top-level code only sees predecessor files - /// whereas function bodies (lazy scopes) see all files because the - /// namespace is fully populated before any function runs. - Package { - top_level: Vec, - lazy: Vec, - }, - - /// Script or file outside a package. The scope chain is the R - /// search path: `library()` attachments from the file itself, - /// default packages (stats, graphics, etc.), and base. - /// - /// At top-level, only `Attach` semantic calls that appear before - /// the cursor are visible (R executes scripts sequentially). - /// Inside function bodies all visible attachments are exposed - /// because the function will typically be called after the full - /// script has been sourced. - SearchPath { - base: Vec, - semantic_calls: Vec, - }, -} - -impl Default for ExternalScope { - fn default() -> Self { - Self::SearchPath { - base: Vec::new(), - semantic_calls: Vec::new(), - } - } -} - -impl ExternalScope { - pub fn package(top_level: Vec, lazy: Vec) -> Self { - Self::Package { top_level, lazy } - } - - pub fn search_path(semantic_calls: Vec, base: Vec) -> Self { - Self::SearchPath { - base, - semantic_calls, - } - } - - /// Return the scope chain appropriate for the given offset. For - /// packages, top-level scope uses predecessors only while lazy - /// (function) scopes see all files. For scripts, top-level code - /// only sees `library()` calls that precede the cursor while - /// function bodies see all attachments. - pub fn at(&self, index: &SemanticIndex, offset: TextSize) -> Cow<'_, [ScopeLayer]> { - match self { - Self::Package { top_level, lazy } => { - let (_, scope) = index.scope_at(offset); - match scope.kind() { - ScopeKind::File => Cow::Borrowed(top_level), - ScopeKind::Function => Cow::Borrowed(lazy), - } - }, - Self::SearchPath { - base, - semantic_calls, - } => { - let (cursor_scope, _) = index.scope_at(offset); - let file_scope = ScopeId::from(0); - let in_function = cursor_scope != file_scope; - let layers: Vec<_> = semantic_calls - .iter() - .rev() - .filter(|c| { - let call_scope = c.scope(); - // File-scope attachments are always visible inside - // function bodies (the function is typically called - // after the full script has been sourced). - if in_function && call_scope == file_scope { - return true; - } - c.offset() < offset && - index.ancestor_scopes(cursor_scope).any(|s| s == call_scope) - }) - .filter_map(|c| match c.kind() { - SemanticCallKind::Attach { package } => { - Some(ScopeLayer::PackageExports(package.clone())) - }, - SemanticCallKind::Source { .. } => None, - }) - .chain(base.iter().cloned()) - .collect(); - Cow::Owned(layers) - }, - } - } - - /// The full scope for lazy contexts. Useful for features that don't - /// have a cursor position (e.g. completions, workspace symbols). - pub fn lazy(&self) -> Cow<'_, [ScopeLayer]> { - match self { - Self::Package { lazy, .. } => Cow::Borrowed(lazy), - Self::SearchPath { - semantic_calls, - base, - .. - } => { - let file_scope = ScopeId::from(0); - let mut layers: Vec = semantic_calls - .iter() - .rev() - .filter(|c| c.scope() == file_scope) - .filter_map(|c| match c.kind() { - SemanticCallKind::Attach { package } => { - Some(ScopeLayer::PackageExports(package.clone())) - }, - SemanticCallKind::Source { .. } => None, - }) - .collect(); - layers.extend(base.iter().cloned()); - Cow::Owned(layers) - }, - } - } -} diff --git a/crates/oak_ide/src/find_references.rs b/crates/oak_ide/src/find_references.rs new file mode 100644 index 0000000000..4c1a020dd4 --- /dev/null +++ b/crates/oak_ide/src/find_references.rs @@ -0,0 +1,113 @@ +use std::collections::HashSet; + +use aether_syntax::RSyntaxNode; +use oak_semantic::semantic_index::SemanticIndex; +use oak_semantic::DefinitionId; +use oak_semantic::ScopeId; + +use crate::FileOffset; +use crate::FileRange; +use crate::Identifier; + +/// Find all in-file references to the symbol at `pos`. +/// +/// The target is usually a single def. It grows when a use is reached by +/// conditional defs, or when a free variable picks up multiple visible +/// defs from an enclosing scope. +/// +/// Returns an empty vec for: +/// - Non-identifier cursors (no `Identifier::classify` match). +/// - `pkg::sym` namespace access. TODO(salsa). +/// - Truly free variables. These are handled by the ark-layer cross-file +/// fallback, TODO(salsa). +/// +/// TODO(salsa): switch the candidate pool to a textual scan so the same +/// `candidates -> refine via name resolution` path works for intra-file +/// and cross-file uniformly (r-a / ty's approach). +/// +/// TODO(places): `foo$bar` / `foo@bar` member accesses aren't tracked by +/// the semantic index, so cursor on a member name returns empty here. +pub fn find_references( + index: &SemanticIndex, + root: &RSyntaxNode, + pos: &FileOffset, + include_declaration: bool, +) -> Vec { + let Some(ident) = Identifier::classify(index, root, pos.offset) else { + return Vec::new(); + }; + + // Compute the cursor's reaching defs. Same operation we'll run on + // every candidate use below. + let (target_defs, name): (HashSet<(ScopeId, DefinitionId)>, String) = match ident { + Identifier::Definition { + scope_id, + def_id, + name, + .. + } => ( + std::iter::once((scope_id, def_id)).collect(), + name.to_string(), + ), + Identifier::Use { + scope_id, + use_id, + name, + .. + } => ( + index.reaching_definitions(scope_id, use_id).collect(), + name.to_string(), + ), + Identifier::NamespaceAccess { .. } => return Vec::new(), + }; + + if target_defs.is_empty() { + return Vec::new(); + } + + let mut results: Vec = Vec::new(); + + // Definition sites come straight from `target_defs`. + if include_declaration { + for &(scope_id, def_id) in &target_defs { + let def = &index.definitions(scope_id)[def_id]; + results.push(FileRange { + file: pos.file.clone(), + range: def.range(), + }); + } + } + + // Walk all uses in every scope and check for each use of the same name + // whether its binding set intersects the target. + for scope_id in index.scope_ids() { + let symbols = index.symbols(scope_id); + let Some(symbol_id) = symbols.id(&name) else { + // The scope doesn't have any uses for that symbol + continue; + }; + + for (use_id, use_site) in index.uses(scope_id).iter() { + if use_site.symbol() != symbol_id { + continue; + } + let intersects = index + .reaching_definitions(scope_id, use_id) + .any(|d| target_defs.contains(&d)); + if !intersects { + continue; + } + results.push(FileRange { + file: pos.file.clone(), + range: use_site.range(), + }); + } + } + + // Defs are emitted in `target_defs` (HashSet) iteration order, which + // is non-deterministic. Sort by start offset so callers see source + // order regardless of how we collected results. + results.sort_by_key(|r| r.range.start()); + + results +} diff --git a/crates/oak_ide/src/goto_definition.rs b/crates/oak_ide/src/goto_definition.rs index 2797beedb1..086eb586e7 100644 --- a/crates/oak_ide/src/goto_definition.rs +++ b/crates/oak_ide/src/goto_definition.rs @@ -1,223 +1,70 @@ -use std::collections::HashSet; - use aether_syntax::RSyntaxNode; -use biome_rowan::TextSize; -use oak_db::LegacyDb; -use oak_semantic::external::resolve_external_name; -use oak_semantic::external::resolve_in_package; -use oak_semantic::package_definitions::PackageDefinitionVisibility; -use oak_semantic::scope_layer::ScopeLayer; -use oak_semantic::semantic_index::DefinitionKind; use oak_semantic::semantic_index::SemanticIndex; -use oak_semantic::semantic_index::Use; -use oak_semantic::DefinitionId; use oak_semantic::ScopeId; use oak_semantic::UseId; use url::Url; -use crate::ExternalScope; +use crate::FileOffset; use crate::Identifier; use crate::NavigationTarget; /// Resolve the symbol at `offset` in a file. /// -/// Uses `Identifier::classify` to determine what the offset points at: -/// -/// - Definition site (LHS of assignment, parameter, for variable): -/// navigates to itself. We round-trip through the index to get -/// `full_range` and `focus_range` from the `Definition`. Currently both -/// are the name range, but once we distinguish them `full_range` will -/// come from the `DefinitionKind`'s `RSyntaxNode` (the whole assignment / -/// parameter expression). -/// -/// - Use site (name reference): resolves via the use-def map, enclosing -/// scopes, and the external scope chain. -/// -/// - Namespace access (`pkg::sym` or `pkg:::sym`): resolves the symbol -/// directly in the named package. -/// -/// Returns an empty `Vec` if the offset doesn't point at a known -/// identifier, or if the symbol cannot be resolved. -// TODO(salsa): This body is the `LegacyDb`-based implementation. Gets -// rewritten on top of `oak_db::File::resolve_at`, at which point the -// helpers below (`resolve_use`, `resolve_import`, `resolve_external`, -// `resolve_namespace_access`) and the `ExternalScope` parameter all go -// away. +/// TODO(salsa) Within-file only. `pkg::sym` and any unresolved free variable +/// currently return an empty list. Cross-file resolution lives in the +/// salsa-backed path and will be wired in later. pub fn goto_definition( - db: &dyn LegacyDb, - offset: TextSize, - file: &Url, - root: &RSyntaxNode, index: &SemanticIndex, - scope: &ExternalScope, + root: &RSyntaxNode, + pos: &FileOffset, ) -> Vec { - let Some(ident) = Identifier::classify(root, index, offset) else { + let Some(ident) = Identifier::classify(index, root, pos.offset) else { return Vec::new(); }; match ident { - Identifier::Definition { scope_id, def_id } => { - let def = &index.definitions(scope_id)[def_id]; - let name = index.symbols(scope_id).symbol(def.symbol()).name(); - + Identifier::Definition { def, name, .. } => { vec![NavigationTarget { - file: file.clone(), + file: pos.file.clone(), name: name.to_string(), full_range: def.range(), focus_range: def.range(), }] }, - Identifier::Use { scope_id, use_id } => { - let use_site = &index.uses(scope_id)[use_id]; - resolve_use(db, file, index, scope_id, use_id, use_site, offset, scope) - }, - Identifier::NamespaceAccess { - ref package, - ref symbol, - internal, + Identifier::Use { + scope_id, + use_id, + name, .. - } => { - let visibility = if internal { - PackageDefinitionVisibility::Internal - } else { - PackageDefinitionVisibility::Exported - }; - resolve_namespace_access(db, symbol, package, visibility) - }, + } => resolve_use(index, &pos.file, scope_id, use_id, name), + Identifier::NamespaceAccess { .. } => Vec::new(), } } fn resolve_use( - db: &dyn LegacyDb, - file: &Url, index: &SemanticIndex, + file: &Url, scope_id: ScopeId, use_id: UseId, - use_site: &Use, - offset: TextSize, - scope: &ExternalScope, -) -> Vec { - let use_def_map = index.use_def_map(scope_id); - let bindings = use_def_map.bindings_at_use(use_id); - - let symbol_name = index.symbols(scope_id).symbol(use_site.symbol()).name(); - - let definitions = bindings.definitions(); - - let local_targets = |scope, defs: &[DefinitionId]| -> Vec { - defs.iter() - .filter_map(|&def_id| { - let def = &index.definitions(scope)[def_id]; - match def.kind() { - DefinitionKind::Import { file, name, .. } => resolve_import(db, file, name), - _ => Some(NavigationTarget { - file: file.clone(), - name: symbol_name.to_string(), - full_range: def.range(), - focus_range: def.range(), - }), - } - }) - .collect() - }; - - let external_targets = || { - let scope_chain = scope.at(index, offset); - resolve_external(db, symbol_name, &scope_chain) - }; - - if !definitions.is_empty() { - let mut targets = local_targets(scope_id, definitions); - if bindings.may_be_unbound() { - targets.extend(external_targets()); - } - return targets; - } - - // No local definitions. If we're in a nested scope, check enclosing - // bindings (the symbol might be defined in an outer function scope). - if let Some((enclosing_scope, enclosing_bindings)) = - index.enclosing_bindings(scope_id, use_site.symbol()) - { - let enclosing_defs = enclosing_bindings.definitions(); - if !enclosing_defs.is_empty() { - let mut targets = local_targets(enclosing_scope, enclosing_defs); - if enclosing_bindings.may_be_unbound() { - targets.extend(external_targets()); - } - return targets; - } - } - - external_targets() -} - -/// Chase a `DefinitionKind::Import` forwarding binding to the actual -/// definition in the target file. Recurses through chains of Import -/// definitions (e.g., a.R sources b.R sources c.R). -/// -/// TODO(salsa): Move to `oak_semantic` once it depends on `oak_db`. -fn resolve_import(db: &dyn LegacyDb, file: &Url, name: &str) -> Option { - let mut visited = HashSet::new(); - resolve_import_inner(db, file, name, &mut visited) -} - -fn resolve_import_inner( - db: &dyn LegacyDb, - file: &Url, name: &str, - visited: &mut HashSet<(Url, String)>, -) -> Option { - if !visited.insert((file.clone(), name.to_string())) { - return None; - } - - let target_index = db.semantic_index(file)?; - - // Imports always target top-level definitions in the sourced file. - let file_scope = ScopeId::from(0); - let symbols = &target_index.symbols(file_scope); - - let (_def_id, def) = target_index - .definitions(file_scope) - .iter() - .filter(|(_id, def)| symbols.symbol(def.symbol()).name() == name) - .last()?; - - match def.kind() { - DefinitionKind::Import { - file: next_file, - name: next_name, - .. - } => resolve_import_inner(db, next_file, next_name, visited), - _ => Some(NavigationTarget { - file: file.clone(), - name: name.to_string(), - full_range: def.range(), - focus_range: def.range(), - }), - } -} - -fn resolve_namespace_access( - db: &dyn LegacyDb, - symbol: &str, - package: &str, - visibility: PackageDefinitionVisibility, ) -> Vec { - let Some(external) = resolve_in_package(db.library(), package, symbol, visibility) else { - return Vec::new(); - }; - vec![external.into()] -} - -fn resolve_external( - db: &dyn LegacyDb, - symbol: &str, - scope_chain: &[ScopeLayer], -) -> Vec { - let Some(external) = resolve_external_name(db.library(), scope_chain, symbol) else { - return Vec::new(); - }; - vec![external.into()] + // `reaching_definitions` unions the local use-def map with the + // enclosing-scope snapshot when `may_be_unbound` is true. That + // covers the conditional-local-plus-outer-binding case where both + // the inner conditional def and the outer def can reach the use. + // + // TODO(salsa): when the result is empty, fall through to cross-file + // resolution (file imports / package exports). + index + .reaching_definitions(scope_id, use_id) + .map(|(scope, def_id)| { + let def = &index.definitions(scope)[def_id]; + NavigationTarget { + file: file.clone(), + name: name.to_string(), + full_range: def.range(), + focus_range: def.range(), + } + }) + .collect() } diff --git a/crates/oak_ide/src/identifier.rs b/crates/oak_ide/src/identifier.rs index 27508c74d6..2cefb2c994 100644 --- a/crates/oak_ide/src/identifier.rs +++ b/crates/oak_ide/src/identifier.rs @@ -7,21 +7,28 @@ use biome_rowan::TextRange; use biome_rowan::TextSize; use oak_core::syntax_ext::RIdentifierExt; use oak_core::syntax_ext::RStringValueExt; +use oak_semantic::semantic_index::Definition; use oak_semantic::semantic_index::SemanticIndex; +use oak_semantic::semantic_index::Use; use oak_semantic::DefinitionId; use oak_semantic::ScopeId; use oak_semantic::UseId; +/// Semantic identity of identifier at cursor. #[derive(Debug, Clone, PartialEq, Eq)] -pub enum Identifier { +pub enum Identifier<'a> { Definition { scope_id: ScopeId, def_id: DefinitionId, + def: &'a Definition, + name: &'a str, }, Use { scope_id: ScopeId, use_id: UseId, + use_site: &'a Use, + name: &'a str, }, NamespaceAccess { @@ -33,20 +40,37 @@ pub enum Identifier { }, } -impl Identifier { - pub fn classify(root: &RSyntaxNode, index: &SemanticIndex, offset: TextSize) -> Option { +impl<'a> Identifier<'a> { + pub fn classify( + index: &'a SemanticIndex, + root: &RSyntaxNode, + offset: TextSize, + ) -> Option { let (scope_id, _) = index.scope_at(offset); + let symbols = index.symbols(scope_id); // `Import` definitions have empty ranges (no physical text position, // since `source()` injects them) so `contains()` skips them. If the // cursor is on the `source` symbol, the offset classifies instead as a // use of `source` via the check below. - if let Some((def_id, _def)) = index.definitions(scope_id).contains(offset) { - return Some(Identifier::Definition { scope_id, def_id }); + if let Some((def_id, def)) = index.definitions(scope_id).contains(offset) { + let name = symbols.symbol(def.symbol()).name(); + return Some(Identifier::Definition { + scope_id, + def_id, + def, + name, + }); } - if let Some((use_id, _)) = index.uses(scope_id).contains(offset) { - return Some(Identifier::Use { scope_id, use_id }); + if let Some((use_id, use_site)) = index.uses(scope_id).contains(offset) { + let name = symbols.symbol(use_site.symbol()).name(); + return Some(Identifier::Use { + scope_id, + use_id, + use_site, + name, + }); } if let Some(namespace_access) = classify_namespace(root, offset) { @@ -57,7 +81,7 @@ impl Identifier { } } -fn classify_namespace(root: &RSyntaxNode, offset: TextSize) -> Option { +fn classify_namespace<'a>(root: &RSyntaxNode, offset: TextSize) -> Option> { let token = root.token_at_offset(offset).right_biased()?; let ns_expr = token diff --git a/crates/oak_ide/src/lib.rs b/crates/oak_ide/src/lib.rs index 838af1c748..07e8a3ffc8 100644 --- a/crates/oak_ide/src/lib.rs +++ b/crates/oak_ide/src/lib.rs @@ -1,18 +1,39 @@ -mod external_scope; +mod find_references; mod goto_definition; mod identifier; +mod rename; use biome_rowan::TextRange; -pub use external_scope::ExternalScope; +use biome_rowan::TextSize; +pub use find_references::find_references; pub use goto_definition::goto_definition; pub use identifier::Identifier; -use oak_semantic::external::ExternalDefinition; +pub use rename::prepare_rename; +pub use rename::rename; +pub use rename::RenameTargets; use url::Url; +/// A cursor location in the workspace: a file and a byte offset into it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct FileOffset { + pub file: Url, + pub offset: TextSize, +} + +/// A span in the workspace: a file and a byte range within it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct FileRange { + pub file: Url, + pub range: TextRange, +} + /// A location in source code that the editor can navigate to. /// -/// Shared result type for IDE features like goto-definition, find-references, -/// etc. The LSP layer converts these uniformly into `LocationLink`s. +/// Shared result type for IDE features like goto-definition, hover, +/// etc., where the editor distinguishes the full extent of the binding +/// from the focus (the name selection). The LSP layer converts this into +/// `LocationLink`. For features without that distinction (find-refs, +/// document-highlight), use `FileRange` directly. #[derive(Debug, Clone, PartialEq, Eq)] pub struct NavigationTarget { pub file: Url, @@ -20,15 +41,3 @@ pub struct NavigationTarget { pub full_range: TextRange, pub focus_range: TextRange, } - -impl From for NavigationTarget { - fn from(value: ExternalDefinition) -> Self { - let (file, name, range) = value.into_parts(); - Self { - file, - name, - full_range: range, - focus_range: range, - } - } -} diff --git a/crates/oak_ide/src/rename.rs b/crates/oak_ide/src/rename.rs new file mode 100644 index 0000000000..a31d4d15bf --- /dev/null +++ b/crates/oak_ide/src/rename.rs @@ -0,0 +1,79 @@ +use aether_syntax::RSyntaxNode; +use anyhow::anyhow; +use biome_rowan::TextRange; +use oak_core::identifier::to_identifier_text; +use oak_semantic::semantic_index::SemanticIndex; + +use crate::find_references; +use crate::FileOffset; +use crate::FileRange; +use crate::Identifier; + +/// All edits needed to rename the symbol at the cursor. Each range gets +/// replaced by `new_text`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RenameTargets { + pub ranges: Vec, + /// The canonical syntactic form of the new name (backtick-wrapped if + /// needed). Same for every range in `ranges`. + pub new_text: String, +} + +/// Identify the renamable identifier at `pos`, returning its range and +/// current (unquoted) name. Returns `None` when the cursor is on a +/// non-identifier, a `pkg::sym` namespace access, or a `$`/`@` member +/// name (TODO(places)). +/// +/// Equivalent to LSP `textDocument/prepareRename`. Clients use the range +/// to highlight the editable region and the name as the placeholder. +pub fn prepare_rename( + index: &SemanticIndex, + root: &RSyntaxNode, + pos: &FileOffset, +) -> Option<(TextRange, String)> { + let ident = Identifier::classify(index, root, pos.offset)?; + match ident { + Identifier::Definition { def, name, .. } => Some((def.range(), name.to_string())), + Identifier::Use { use_site, name, .. } => Some((use_site.range(), name.to_string())), + Identifier::NamespaceAccess { .. } => None, + } +} + +/// Compute all rename edits within the file. +/// +/// Returns `Err` when: +/// - `new_name` is empty, is an R reserved word, or contains a literal +/// backtick (which can't appear in a backtick-quoted identifier). +/// - The cursor isn't on a renamable identifier (no `prepare_rename` +/// target). +/// - Nothing in the file binds to the cursor's symbol (free variable +/// from outside the file). Rename would only edit local sites without +/// touching the external definition, so we refuse rather than produce +/// a partial result. +/// +/// TODO(places): renaming a `$`/`@` member name returns `Err` because +/// the semantic index doesn't track member names. +/// +/// TODO(salsa): cross-file renames are out of scope until cross-file +/// resolution lands. For now this is intra-file only. +pub fn rename( + index: &SemanticIndex, + root: &RSyntaxNode, + pos: &FileOffset, + new_name: &str, +) -> anyhow::Result { + let new_text = to_identifier_text(new_name)?; + + if prepare_rename(index, root, pos).is_none() { + return Err(anyhow!("No renamable identifier at cursor")); + } + + let ranges = find_references(index, root, pos, true); + if ranges.is_empty() { + return Err(anyhow!( + "Cannot rename: symbol has no local binding in this file" + )); + } + + Ok(RenameTargets { ranges, new_text }) +} diff --git a/crates/oak_ide/tests/find_references.rs b/crates/oak_ide/tests/find_references.rs new file mode 100644 index 0000000000..cde3780eb3 --- /dev/null +++ b/crates/oak_ide/tests/find_references.rs @@ -0,0 +1,315 @@ +use aether_parser::parse; +use aether_parser::RParserOptions; +use aether_syntax::RSyntaxNode; +use biome_rowan::TextRange; +use biome_rowan::TextSize; +use oak_ide::find_references; +use oak_ide::FileOffset; +use oak_ide::FileRange; +use oak_semantic::build_index; +use oak_semantic::semantic_index::SemanticIndex; +use oak_semantic::NoopImportsResolver; +use url::Url; + +fn parse_source(source: &str) -> (RSyntaxNode, SemanticIndex) { + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let index = build_index(&parsed.tree(), NoopImportsResolver); + (root, index) +} + +fn text_range(start: u32, end: u32) -> TextRange { + TextRange::new(TextSize::from(start), TextSize::from(end)) +} + +fn file_url(name: &str) -> Url { + Url::parse(&format!("file:///project/R/{name}")).unwrap() +} + +fn offset(n: u32) -> TextSize { + TextSize::from(n) +} + +fn pos(file: &Url, n: u32) -> FileOffset { + FileOffset { + file: file.clone(), + offset: offset(n), + } +} + +fn ranges(refs: Vec) -> Vec { + refs.into_iter().map(|r| r.range).collect() +} + +// --- Local resolution --- + +#[test] +fn test_local_simple() { + // "x <- 1\nx\n" + // 0123456 78 + let source = "x <- 1\nx\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on the use at offset 7 + let refs = ranges(find_references(&idx, &root, &pos(&file, 7), true)); + assert_eq!(refs, vec![text_range(0, 1), text_range(7, 8)]); +} + +#[test] +fn test_local_excludes_declaration() { + let source = "x <- 1\nx\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // With include_declaration = false, the def at 0..1 is dropped + let refs = ranges(find_references(&idx, &root, &pos(&file, 7), false)); + assert_eq!(refs, vec![text_range(7, 8)]); +} + +#[test] +fn test_from_definition_site() { + // Cursor on the def `x` in `x <- 1` should still return all refs + let source = "x <- 1\nx\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let refs = ranges(find_references(&idx, &root, &pos(&file, 0), true)); + assert_eq!(refs, vec![text_range(0, 1), text_range(7, 8)]); +} + +#[test] +fn test_shadowing_excludes_outer() { + // Outer `x` and inner `x` are different bindings; refs of inner + // shouldn't include outer (and vice versa). + // + // "x <- 1\nf <- function() {\n x <- 2\n x\n}\n" + // 0 7 26 35 + // outer def at 0..1, outer never used + // inner def at 28..29, inner use at 37..38 + let source = "x <- 1\nf <- function() {\n x <- 2\n x\n}\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on the inner use `x` (offset 37). Should return inner pair only. + let inner_def = source.find("x <- 2").unwrap() as u32; + let inner_use = source.rfind('x').unwrap() as u32; + let refs = ranges(find_references(&idx, &root, &pos(&file, inner_use), true)); + assert_eq!(refs, vec![ + text_range(inner_def, inner_def + 1), + text_range(inner_use, inner_use + 1), + ]); + + // Cursor on the outer def `x` (offset 0). Outer has no uses, so just + // the def itself. + let refs = ranges(find_references(&idx, &root, &pos(&file, 0), true)); + assert_eq!(refs, vec![text_range(0, 1)]); +} + +#[test] +fn test_free_variable_in_inner_scope() { + // Free `x` inside `f` resolves to file-scope `x`. Refs should include + // the file-scope def and the inner use. + let source = "x <- 1\nf <- function() {\n x\n}\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let inner_use = source.rfind('x').unwrap() as u32; + let refs = ranges(find_references(&idx, &root, &pos(&file, inner_use), true)); + assert_eq!(refs, vec![ + text_range(0, 1), + text_range(inner_use, inner_use + 1) + ]); +} + +#[test] +fn test_multiple_uses() { + let source = "x <- 1\nx + x + x\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let refs = ranges(find_references(&idx, &root, &pos(&file, 0), true)); + assert_eq!(refs, vec![ + text_range(0, 1), + text_range(7, 8), + text_range(11, 12), + text_range(15, 16), + ]); +} + +#[test] +fn test_parameter_refs() { + let source = "f <- function(x) {\n x + x\n}\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on parameter `x` at offset 14 + let refs = ranges(find_references(&idx, &root, &pos(&file, 14), true)); + assert_eq!(refs.len(), 3); + assert_eq!(refs[0], text_range(14, 15)); +} + +#[test] +fn test_reassignment_separates_refs() { + // After `x <- 2`, the use of `x` binds to the second def. Find-refs on + // the second def returns just the def itself and the use that follows. + let source = "x <- 1\nx <- 2\nx\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on second def `x` at offset 7 + let refs = ranges(find_references(&idx, &root, &pos(&file, 7), true)); + // Use at offset 14 binds to the second def. The first def at offset 0 + // is killed by the second. + assert_eq!(refs, vec![text_range(7, 8), text_range(14, 15)]); + + // Cursor on first def at offset 0. The only ref is the def itself + // (its use is shadowed by the second def). + let refs = ranges(find_references(&idx, &root, &pos(&file, 0), true)); + assert_eq!(refs, vec![text_range(0, 1)]); +} + +#[test] +fn test_conditional_defs_seen_from_enclosing_scope() { + // Outer scope has conditional defs; inner function uses the name as + // a free variable. The use's target def set has BOTH outer defs + // (multiple-element set via the enclosing snapshot). Refs should + // include both defs and the inner use. + // + // "if (TRUE) x <- 1 else x <- 2\nf <- function() x\n" + // 0 10 16 22 45 + let source = "if (TRUE) x <- 1 else x <- 2\nf <- function() x\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let use_offset = source.rfind('x').unwrap() as u32; + let refs = ranges(find_references(&idx, &root, &pos(&file, use_offset), true)); + assert_eq!(refs, vec![ + text_range(10, 11), + text_range(22, 23), + text_range(use_offset, use_offset + 1), + ]); +} + +#[test] +fn test_super_assignment_targets_outer_scope() { + // `x <<- 2` inside `f` defines `x` in the enclosing scope. A use of + // `x` outside `f` sees BOTH the original def and the super-assigned + // def (the use-def map adds `<<-` without clearing). + // + // "x <- 1\nf <- function() x <<- 2\nx\n" + // 0 23 31 + let source = "x <- 1\nf <- function() x <<- 2\nx\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let super_def = source.find("x <<-").unwrap() as u32; + let outer_use = source.rfind('x').unwrap() as u32; + let refs = ranges(find_references(&idx, &root, &pos(&file, outer_use), true)); + // Both file-scope defs (`x <- 1` and the super-assigned `x`) and the + // use are returned. + assert_eq!(refs, vec![ + text_range(0, 1), + text_range(super_def, super_def + 1), + text_range(outer_use, outer_use + 1), + ]); +} + +#[test] +fn test_conditional_binding_includes_both_defs() { + let source = "if (TRUE) x <- 1 else x <- 2\nx\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let use_offset = source.rfind('x').unwrap() as u32; + let refs = ranges(find_references(&idx, &root, &pos(&file, use_offset), true)); + // Both conditional defs reach the use, so both are returned. + assert_eq!(refs, vec![ + text_range(10, 11), + text_range(22, 23), + text_range(use_offset, use_offset + 1), + ]); +} + +// --- No resolution --- + +#[test] +fn test_no_identifier_at_offset() { + let source = "x <- 1\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on `<-` operator + let refs = find_references(&idx, &root, &pos(&file, 3), true); + assert!(refs.is_empty()); +} + +#[test] +fn test_unbound_use_returns_empty() { + // `foo` has no local def: classification gives Use but the target def + // set is empty. Within-file find-refs returns nothing; cross-file + // textual fallback in the ark wrapper handles this case. + let source = "foo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let refs = find_references(&idx, &root, &pos(&file, 0), true); + assert!(refs.is_empty()); +} + +#[test] +fn test_fixme_namespace_access_returns_empty() { + let source = "dplyr::mutate\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on `mutate` - NamespaceAccess, returns empty. + let refs = find_references(&idx, &root, &pos(&file, 7), true); + assert!(refs.is_empty()); +} + +// --- Dollar/at member access (places TODO) --- + +#[test] +fn test_fixme_dollar_lhs_resolves_only_to_variable() { + // `foo` on the LHS of `$` is a real variable use. Find-refs should + // include the def and the LHS use, but NOT the RHS member name. + let source = "foo <- list()\nfoo$foo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on `foo` LHS at offset 14 + let refs = ranges(find_references(&idx, &root, &pos(&file, 14), true)); + // Def at 0..3, LHS use at 14..17. RHS `foo` (offset 18..21) is a + // member name, not tracked by the semantic index. + assert_eq!(refs, vec![text_range(0, 3), text_range(14, 17)]); +} + +#[test] +fn test_fixme_dollar_rhs_returns_empty() { + // Cursor on `bar` in `foo$bar` - member names aren't variable refs. + let source = "foo <- list()\nfoo$bar\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let refs = find_references(&idx, &root, &pos(&file, 18), true); + assert!(refs.is_empty()); +} + +#[test] +fn test_string_def_returns_quoted_range_for_def() { + // `"foo" <- 1` defines `foo`. The semantic index records the def's + // range as covering the whole `"foo"` token (5 chars). Find-refs on + // the use of `foo` returns the def at the quoted range and uses at + // the bare range -- semantically correct, just visually unusual. + let source = "\"foo\" <- 1\nfoo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on the bare `foo` use at offset 11 + let refs = ranges(find_references(&idx, &root, &pos(&file, 11), true)); + assert_eq!(refs, vec![ + text_range(0, 5), // covers `"foo"` (the def site) + text_range(11, 14), // covers `foo` (the use site) + ]); +} diff --git a/crates/oak_ide/tests/goto_definition.rs b/crates/oak_ide/tests/goto_definition.rs index 476b7715b2..72bd5cfdf9 100644 --- a/crates/oak_ide/tests/goto_definition.rs +++ b/crates/oak_ide/tests/goto_definition.rs @@ -1,31 +1,14 @@ -use std::collections::HashMap; -use std::path::PathBuf; -use std::sync::Arc; - use aether_parser::parse; use aether_parser::RParserOptions; use aether_syntax::RSyntaxNode; use biome_rowan::TextRange; use biome_rowan::TextSize; -use oak_db::LegacyDb; use oak_ide::goto_definition; -use oak_ide::ExternalScope; +use oak_ide::FileOffset; use oak_ide::NavigationTarget; -use oak_package_metadata::description::Description; -use oak_package_metadata::namespace::Namespace; use oak_semantic::build_index; -use oak_semantic::library::Library; -use oak_semantic::package::Package; -use oak_semantic::scope_layer::file_layers; -use oak_semantic::scope_layer::ScopeLayer; -use oak_semantic::semantic_index::SemanticCallKind; use oak_semantic::semantic_index::SemanticIndex; -use oak_semantic::ImportsResolver; use oak_semantic::NoopImportsResolver; -use oak_semantic::ScopeId; -use oak_semantic::SourceResolution; -use oak_sources::test::TestPackageCache; -use stdext::SortedVec; use url::Url; fn parse_source(source: &str) -> (RSyntaxNode, SemanticIndex) { @@ -35,110 +18,6 @@ fn parse_source(source: &str) -> (RSyntaxNode, SemanticIndex) { (root, index) } -/// Cross-file resolver that returns the same resolution for any path. Used -/// by the source()-resolution tests below; the path argument is irrelevant -/// because each test wires a single sourced file. -struct ConstResolver(SourceResolution); - -impl ImportsResolver for ConstResolver { - fn resolve_source(&mut self, _path: &str) -> Option { - Some(self.0.clone()) - } -} - -struct TestDb { - library: Library, - sources: HashMap, -} - -impl LegacyDb for TestDb { - fn semantic_index(&self, file: &Url) -> Option { - // Rebuild from source for tests. We store the source instead. - self.sources.get(file).map(|source| { - let parsed = parse(source, RParserOptions::default()); - build_index(&parsed.tree(), NoopImportsResolver) - }) - } - fn library(&self) -> &Library { - &self.library - } -} - -fn empty_library() -> Library { - Library::new(vec![], None) -} - -struct TestPackage { - name: String, - file: PathBuf, - exports: Vec, - internals: Vec, -} - -impl TestPackage { - // Most convenient input types for test construction - fn new(name: &str, file: &str, exports: Vec<&str>, internals: Vec<&str>) -> Self { - Self { - name: String::from(name), - file: PathBuf::from(file), - exports: exports - .into_iter() - .map(|export| export.to_string()) - .collect(), - internals: internals - .into_iter() - .map(|export| export.to_string()) - .collect(), - } - } -} - -fn test_library(packages: Vec) -> Library { - let cache = TestPackageCache::new().unwrap(); - - // Both exports and internals are in the test file - for package in &packages { - let content = test_file(&package.exports, &package.internals); - cache - .add(&package.name, vec![(package.file.as_path(), &content)]) - .expect("Can write to cache"); - } - - let cache = Arc::new(cache); - - let mut library = Library::new(vec![], Some(cache)); - - // Only exports are included in `Namespace` - for package in &packages { - let ns = Namespace { - exports: SortedVec::from_vec(package.exports.clone()), - ..Default::default() - }; - let desc = Description { - name: package.name.clone(), - ..Default::default() - }; - let pkg = Package::from_parts(PathBuf::from("/fake"), desc, ns); - library = library.insert(&package.name, pkg); - } - - library -} - -// Create a file worth of function definitions -fn test_file(exports: &Vec, internals: &Vec) -> String { - let mut out = String::new(); - - for export in exports { - out.push_str(&format!("{export} <- function() {{}}\n\n")); - } - for internal in internals { - out.push_str(&format!("{internal} <- function() {{}}\n\n")); - } - - out -} - fn text_range(start: u32, end: u32) -> TextRange { TextRange::new(TextSize::from(start), TextSize::from(end)) } @@ -151,6 +30,13 @@ fn offset(n: u32) -> TextSize { TextSize::from(n) } +fn pos(file: &Url, n: u32) -> FileOffset { + FileOffset { + file: file.clone(), + offset: offset(n), + } +} + // --- Local resolution --- #[test] @@ -160,19 +46,8 @@ fn test_local_simple() { let source = "x <- 1\nx\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - let targets = goto_definition( - &db, - offset(7), - &file, - &root, - &idx, - &ExternalScope::default(), - ); + let targets = goto_definition(&idx, &root, &pos(&file, 7)); assert_eq!(targets, vec![NavigationTarget { file, name: "x".to_string(), @@ -187,19 +62,8 @@ fn test_local_reassignment_shadows() { let source = "x <- 1\nx <- 2\nx\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - let targets = goto_definition( - &db, - offset(14), - &file, - &root, - &idx, - &ExternalScope::default(), - ); + let targets = goto_definition(&idx, &root, &pos(&file, 14)); assert_eq!(targets, vec![NavigationTarget { file, name: "x".to_string(), @@ -213,21 +77,10 @@ fn test_local_conditional_returns_both() { let source = "if (TRUE) x <- 1 else x <- 2\nx\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &file, - &root, - &idx, - &ExternalScope::default(), - ); + let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); assert_eq!(targets, vec![ NavigationTarget { file: file.clone(), @@ -249,20 +102,9 @@ fn test_local_in_function() { let source = "f <- function() {\n x <- 1\n x\n}\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &file, - &root, - &idx, - &ExternalScope::default(), - ); + let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); assert_eq!(targets, vec![NavigationTarget { file, name: "x".to_string(), @@ -276,20 +118,9 @@ fn test_local_parameter() { let source = "f <- function(x) {\n x\n}\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &file, - &root, - &idx, - &ExternalScope::default(), - ); + let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); assert_eq!(targets, vec![NavigationTarget { file, name: "x".to_string(), @@ -305,20 +136,9 @@ fn test_enclosing_scope() { let source = "x <- 1\nf <- function() {\n x\n}\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &file, - &root, - &idx, - &ExternalScope::default(), - ); + let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); assert_eq!(targets, vec![NavigationTarget { file, name: "x".to_string(), @@ -327,125 +147,93 @@ fn test_enclosing_scope() { }]); } -// --- External resolution: project file --- - #[test] -fn test_external_project_file() { - let source = "foo\n"; - let file = file_url("current.R"); +fn test_conditional_local_falls_through_to_enclosing() { + // Inside `f`, `x` is conditionally defined: bindings = {inner def} + // with `may_be_unbound = true`. At runtime the use of `x` reaches + // either the inner def (when the branch ran) or the outer def (when + // it didn't). Goto-def should return both. + // + // "x <- 1\nf <- function(cond) {\n if (cond) x <- 2\n x\n}\n" + // 0 7 30 42 49 + let source = "x <- 1\nf <- function(cond) {\n if (cond) x <- 2\n x\n}\n"; + let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - let other_url = file_url("other.R"); - let other_source = "foo <- 42\n"; - let (_other_root, other_idx) = parse_source(other_source); - let scope_chain = file_layers(other_url.clone(), &other_idx); + let outer_def = source.find("x <- 1").unwrap() as u32; + let inner_def = source.find("x <- 2").unwrap() as u32; + let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition( - &db, - offset(0), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); + let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); + let ranges: Vec<_> = targets.into_iter().map(|t| t.full_range).collect(); + assert!(ranges.contains(&text_range(outer_def, outer_def + 1))); + assert!(ranges.contains(&text_range(inner_def, inner_def + 1))); + assert_eq!(ranges.len(), 2); +} + +// --- Member access (`$`) --- + +#[test] +fn test_dollar_lhs_resolves() { + // Cursor on `foo` in `foo$bar` resolves to the definition of `foo` + let source = "foo <- list()\nfoo$bar\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // `foo` in `foo$bar` starts at offset 14 + let targets = goto_definition(&idx, &root, &pos(&file, 14)); assert_eq!(targets, vec![NavigationTarget { - file: other_url, + file, name: "foo".to_string(), full_range: text_range(0, 3), focus_range: text_range(0, 3), }]); } -// --- External resolution: package --- - #[test] -fn test_external_package() { - let source = "mutate\n"; +fn test_dollar_rhs_no_resolution() { + // Cursor on `bar` in `foo$bar`: member names are not tracked by the index + let source = "foo <- list()\nfoo$bar\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - let scope_chain = vec![ScopeLayer::PackageExports("dplyr".to_string())]; + // `bar` starts at offset 18 + let targets = goto_definition(&idx, &root, &pos(&file, 18)); + assert!(targets.is_empty()); +} - let targets = goto_definition( - &db, - offset(0), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); +// --- No resolution --- - assert_eq!(targets.len(), 1); +#[test] +fn test_no_use_at_offset() { + let source = "x <- 1\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); - let target = targets.first().unwrap(); - assert!(target.file.path().ends_with("dplyr.R")); - assert_eq!(target.name, "mutate".to_string()); + let targets = goto_definition(&idx, &root, &pos(&file, 3)); + assert!(targets.is_empty()); } -// --- External resolution: importFrom --- - #[test] -fn test_external_import_from() { - let source = "tibble\n"; +fn test_unresolved_symbol() { + let source = "foo\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - - let mut imports = HashMap::new(); - imports.insert("tibble".to_string(), "tibble".to_string()); - let scope_chain = vec![ScopeLayer::PackageImports(imports)]; - let targets = goto_definition( - &db, - offset(0), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); - // importFrom resolves to a package, no file/range to navigate to + let targets = goto_definition(&idx, &root, &pos(&file, 0)); assert!(targets.is_empty()); } -// --- Member access (`$`) --- +// --- Definition site navigation --- #[test] -fn test_dollar_lhs_resolves() { - // Cursor on `foo` in `foo$bar` resolves to the definition of `foo` - let source = "foo <- list()\nfoo$bar\n"; +fn test_definition_site_assignment() { + // Cursor on the `foo` in `foo <- 1` should navigate to itself + let source = "foo <- 1\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - // `foo` in `foo$bar` starts at offset 14 - let targets = goto_definition( - &db, - offset(14), - &file, - &root, - &idx, - &ExternalScope::default(), - ); + let targets = goto_definition(&idx, &root, &pos(&file, 0)); assert_eq!(targets, vec![NavigationTarget { file, name: "foo".to_string(), @@ -455,362 +243,557 @@ fn test_dollar_lhs_resolves() { } #[test] -fn test_dollar_rhs_no_resolution() { - // Cursor on `bar` in `foo$bar`: member names are not tracked by the index - let source = "foo <- list()\nfoo$bar\n"; +fn test_definition_site_parameter() { + let source = "f <- function(x) { x }\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - // `bar` starts at offset 18 - let targets = goto_definition( - &db, - offset(18), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - assert!(targets.is_empty()); + // Cursor on the `x` parameter name (offset 14) + let targets = goto_definition(&idx, &root, &pos(&file, 14)); + assert_eq!(targets, vec![NavigationTarget { + file, + name: "x".to_string(), + full_range: text_range(14, 15), + focus_range: text_range(14, 15), + }]); } -// --- Use inside function body with cross-file definition --- - #[test] -fn test_use_in_function_body_resolves_via_external() { - // Reproduces: `is_null` used inside a function body, defined in another - // file. The use is free in the function scope, so resolution should fall - // through enclosing scopes to the external scope chain. - let source = "f <- function(x) {\n if (is_null(x)) NULL\n}\n"; - let file = file_url("R/cnd-last.R"); +fn test_definition_site_for_variable() { + let source = "for (i in 1:10) i\n"; + let file = file_url("test.R"); let (root, idx) = parse_source(source); - let other_source = "is_null <- is.null\n"; - let (_other_root, other_idx) = parse_source(other_source); - let other_url = file_url("R/types.R"); - let scope_chain = file_layers(other_url.clone(), &other_idx); + // Cursor on the `i` in `for (i in ...)` + let targets = goto_definition(&idx, &root, &pos(&file, 5)); + assert_eq!(targets, vec![NavigationTarget { + file, + name: "i".to_string(), + full_range: text_range(5, 6), + focus_range: text_range(5, 6), + }]); +} - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; +// --- Right assignment --- - // `is_null` starts at offset 24 - let is_null_offset = source.find("is_null").unwrap(); - assert_eq!(is_null_offset, 25); +#[test] +fn test_right_assignment_definition_site() { + // `1 -> x`: cursor on `x` (the definition target) + let source = "1 -> x\nx\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); - let scope = ExternalScope::package(Vec::new(), scope_chain); + let targets = goto_definition(&idx, &root, &pos(&file, 5)); + assert_eq!(targets, vec![NavigationTarget { + file: file.clone(), + name: "x".to_string(), + full_range: text_range(5, 6), + focus_range: text_range(5, 6), + }]); +} - let targets = goto_definition( - &db, - offset(is_null_offset as u32), - &file, - &root, - &idx, - &scope, - ); +#[test] +fn test_right_assignment_use_resolves() { + // `1 -> x` then use `x` + let source = "1 -> x\nx\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let targets = goto_definition(&idx, &root, &pos(&file, 7)); assert_eq!(targets, vec![NavigationTarget { - file: other_url, - name: "is_null".to_string(), - full_range: text_range(0, 7), - focus_range: text_range(0, 7), + file, + name: "x".to_string(), + full_range: text_range(5, 6), + focus_range: text_range(5, 6), }]); } -// --- No resolution --- +// --- Super assignment --- #[test] -fn test_no_use_at_offset() { - let source = "x <- 1\n"; +fn test_super_assignment_resolves_in_enclosing() { + // `x <<- 1` inside a function creates a definition in the file scope. + // A use of `x` in another function should resolve to it. + let source = "f <- function() x <<- 1\ng <- function() x\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - let targets = goto_definition( - &db, - offset(3), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - assert!(targets.is_empty()); + // `x` use in `g` body + let use_offset = source.rfind('x').unwrap() as u32; + let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); + assert_eq!(targets.len(), 1); + assert_eq!(targets[0].name, "x"); + assert_eq!(targets[0].file, file); } #[test] -fn test_unresolved_symbol() { - let source = "foo\n"; +fn test_super_assignment_definition_site() { + // Cursor on `x` in `x <<- 1` + let source = "f <- function() {\n x <<- 1\n}\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - let targets = goto_definition( - &db, - offset(0), - &file, - &root, - &idx, - &ExternalScope::default(), - ); - assert!(targets.is_empty()); + // `x` at offset 20 + let def_offset = source.find("x <<-").unwrap() as u32; + let targets = goto_definition(&idx, &root, &pos(&file, def_offset)); + assert_eq!(targets.len(), 1); + assert_eq!(targets[0].name, "x"); } -// --- Local takes precedence over external --- +// --- String definitions --- #[test] -fn test_local_shadows_external() { - let source = "foo <- 1\nfoo\n"; +fn test_string_definition() { + // `"foo" <- 1` is equivalent to `foo <- 1` in R + let source = "\"foo\" <- 1\nfoo\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let library = test_library(vec![TestPackage::new("pkg", "pkg.R", vec!["foo"], vec![])]); - let db = TestDb { - library, - sources: HashMap::new(), - }; - - let scope_chain = vec![ScopeLayer::PackageExports("pkg".to_string())]; - let use_offset = source.rfind("foo").unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); + // Use of `foo` at offset 11 + let targets = goto_definition(&idx, &root, &pos(&file, 11)); assert_eq!(targets, vec![NavigationTarget { file, name: "foo".to_string(), - full_range: text_range(0, 3), - focus_range: text_range(0, 3), + // The definition range covers the string literal `"foo"` + full_range: text_range(0, 5), + focus_range: text_range(0, 5), }]); } -// --- Conditional definition (may_be_unbound but has local defs) --- +// --- Nested functions --- #[test] -fn test_conditional_definition_includes_external() { - let source = "if (TRUE) x <- 1\nx\n"; +fn test_deeply_nested_function() { + // Free variable `z` resolves through two function scopes to file scope + let source = "z <- 1\nf <- function() {\n g <- function() {\n z\n }\n}\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let other_url = file_url("other.R"); - let other_source = "x <- 99\n"; - let (_other_root, other_idx) = parse_source(other_source); - let scope_chain = file_layers(other_url.clone(), &other_idx); + let use_offset = source.rfind('z').unwrap() as u32; + let targets = goto_definition(&idx, &root, &pos(&file, use_offset)); + assert_eq!(targets, vec![NavigationTarget { + file, + name: "z".to_string(), + full_range: text_range(0, 1), + focus_range: text_range(0, 1), + }]); +} - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - - let use_offset = source.rfind('x').unwrap() as u32; - let targets = goto_definition( - &db, - offset(use_offset), - &file, - &root, - &idx, - &ExternalScope::package(scope_chain.clone(), scope_chain), - ); - assert_eq!(targets, vec![ - NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(10, 11), - focus_range: text_range(10, 11), - }, - NavigationTarget { - file: other_url, - name: "x".to_string(), - full_range: text_range(0, 1), - focus_range: text_range(0, 1), - }, - ]); -} - -// --- Definition site navigation --- +// --- Use on RHS of assignment --- #[test] -fn test_definition_site_assignment() { - // Cursor on the `foo` in `foo <- 1` should navigate to itself - let source = "foo <- 1\n"; +fn test_use_on_rhs_of_assignment() { + // `x <- x + 1`: the `x` on the RHS refers to the previous binding + let source = "x <- 1\nx <- x + 1\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - let targets = goto_definition( - &db, - offset(0), - &file, - &root, - &idx, - &ExternalScope::default(), - ); + // The `x` on the RHS of the second assignment. `x <- x + 1` starts at + // offset 7, the RHS `x` is at offset 12. + let rhs_offset = 7 + "x <- ".len() as u32; + let targets = goto_definition(&idx, &root, &pos(&file, rhs_offset)); assert_eq!(targets, vec![NavigationTarget { file, - name: "foo".to_string(), - full_range: text_range(0, 3), - focus_range: text_range(0, 3), + name: "x".to_string(), + // Resolves to the first definition + full_range: text_range(0, 1), + focus_range: text_range(0, 1), }]); } +// --- Namespace access (`::`, `:::`) returns empty in the within-file handler --- + #[test] -fn test_definition_site_parameter() { - let source = "f <- function(x) { x }\n"; +fn test_namespace_access_returns_empty() { + let source = "dplyr::mutate\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; - // Cursor on the `x` parameter name (offset 14) - let targets = goto_definition( - &db, - offset(14), - &file, - &root, - &idx, - &ExternalScope::default(), + // Cursor on `mutate` (offset 7) + let targets = goto_definition(&idx, &root, &pos(&file, 7)); + assert!(targets.is_empty()); + + // Cursor on `dplyr` (offset 0) + let targets = goto_definition(&idx, &root, &pos(&file, 0)); + assert!(targets.is_empty()); +} + +// --- Identifier::classify keeps recognizing `pkg::sym` --- + +#[test] +fn test_namespace_classify() { + use oak_ide::Identifier; + + let source = "dplyr::mutate\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + // Cursor on `mutate` (offset 7) + let ident = Identifier::classify(&idx, &root, offset(7)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "dplyr".to_string(), + symbol: "mutate".to_string(), + internal: false, + package_range: text_range(0, 5), + symbol_range: text_range(7, 13), + }) + ); + + // Cursor on `dplyr` (offset 2) + let ident = Identifier::classify(&idx, &root, offset(2)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "dplyr".to_string(), + symbol: "mutate".to_string(), + internal: false, + package_range: text_range(0, 5), + symbol_range: text_range(7, 13), + }) ); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(14, 15), - focus_range: text_range(14, 15), - }]); } #[test] -fn test_definition_site_for_variable() { - let source = "for (i in 1:10) i\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; +fn test_namespace_classify_triple_colon() { + use oak_ide::Identifier; - // Cursor on the `i` in `for (i in ...)` - let targets = goto_definition( - &db, - offset(5), - &file, - &root, - &idx, - &ExternalScope::default(), + let source = "pkg:::sym\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + let ident = Identifier::classify(&idx, &root, offset(6)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "pkg".to_string(), + symbol: "sym".to_string(), + internal: true, + package_range: text_range(0, 3), + symbol_range: text_range(6, 9), + }) ); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "i".to_string(), - full_range: text_range(5, 6), - focus_range: text_range(5, 6), - }]); } -// --- Right assignment --- +#[test] +fn test_namespace_classify_in_call() { + use oak_ide::Identifier; + + // foo::bar() + // 0123456789 + let source = "foo::bar()\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + let ident = Identifier::classify(&idx, &root, offset(5)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "foo".to_string(), + symbol: "bar".to_string(), + internal: false, + package_range: text_range(0, 3), + symbol_range: text_range(5, 8), + }) + ); +} #[test] -fn test_right_assignment_definition_site() { - // `1 -> x`: cursor on `x` (the definition target) - let source = "1 -> x\nx\n"; - let file = file_url("test.R"); - let (root, idx) = parse_source(source); - let db = TestDb { - library: empty_library(), - sources: HashMap::new(), - }; +fn test_namespace_classify_in_extract() { + use oak_ide::Identifier; - let targets = goto_definition( - &db, - offset(5), - &file, - &root, - &idx, - &ExternalScope::default(), + // foo::bar$baz + // 0123456789... + let source = "foo::bar$baz\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + // Cursor on `bar` (offset 5) -- inside the RNamespaceExpression + let ident = Identifier::classify(&idx, &root, offset(5)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "foo".to_string(), + symbol: "bar".to_string(), + internal: false, + package_range: text_range(0, 3), + symbol_range: text_range(5, 8), + }) ); - assert_eq!(targets, vec![NavigationTarget { - file: file.clone(), - name: "x".to_string(), - full_range: text_range(5, 6), - focus_range: text_range(5, 6), - }]); + + // Cursor on `baz` (offset 9) -- RHS of $, not a namespace access + let ident = Identifier::classify(&idx, &root, offset(9)); + assert_eq!(ident, None); } #[test] -fn test_right_assignment_use_resolves() { - // `1 -> x` then use `x` - let source = "1 -> x\nx\n"; - let file = file_url("test.R"); +fn test_namespace_classify_string_selectors() { + use oak_ide::Identifier; + + // "foo"::"bar" + // 0123456789... + let source = "\"foo\"::\"bar\"\n"; + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let idx = build_index(&parsed.tree(), NoopImportsResolver); + + let ident = Identifier::classify(&idx, &root, offset(7)); + assert_eq!( + ident, + Some(Identifier::NamespaceAccess { + package: "foo".to_string(), + symbol: "bar".to_string(), + internal: false, + package_range: text_range(0, 5), + symbol_range: text_range(7, 12), + }) + ); +} + +// Parked cross-file goto-definition tests pending the Salsa Oak wiring. +// +// Snapshot of the old `LegacyDb`-based suite, trimmed to the cross-file cases +// that the within-file handler doesn't cover (`source()`, package collation, +// `library()`, NAMESPACE imports, `pkg::sym` resolution). Intra-file tests live +// in the active section above. `#[cfg(any())]` skips this block at compile time +// so the references to deleted types (`LegacyDb`, `ExternalScope`, +// `ScopeLayer`, ...) stay as the behavioural spec to re-implement on top of +// `oak_db::File::resolve_at`. +#[cfg(any())] +#[rustfmt::skip] +mod parked_salsa_tests { +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; + +use aether_parser::parse; +use aether_parser::RParserOptions; +use aether_syntax::RSyntaxNode; +use biome_rowan::TextRange; +use biome_rowan::TextSize; +use oak_db::LegacyDb; +use oak_ide::goto_definition; +use oak_ide::ExternalScope; +use oak_ide::NavigationTarget; +use oak_package_metadata::description::Description; +use oak_package_metadata::namespace::Namespace; +use oak_semantic::build_index; +use oak_semantic::library::Library; +use oak_semantic::package::Package; +use oak_semantic::scope_layer::file_layers; +use oak_semantic::scope_layer::ScopeLayer; +use oak_semantic::semantic_index::SemanticCallKind; +use oak_semantic::semantic_index::SemanticIndex; +use oak_semantic::ImportsResolver; +use oak_semantic::NoopImportsResolver; +use oak_semantic::ScopeId; +use oak_semantic::SourceResolution; +use oak_sources::test::TestPackageCache; +use stdext::SortedVec; +use url::Url; + +fn parse_source(source: &str) -> (RSyntaxNode, SemanticIndex) { + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let index = build_index(&parsed.tree(), NoopImportsResolver); + (root, index) +} + +/// Cross-file resolver that returns the same resolution for any path. Used +/// by the source()-resolution tests below; the path argument is irrelevant +/// because each test wires a single sourced file. +struct ConstResolver(SourceResolution); + +impl ImportsResolver for ConstResolver { + fn resolve_source(&mut self, _path: &str) -> Option { + Some(self.0.clone()) + } +} + +struct TestDb { + library: Library, + sources: HashMap, +} + +impl LegacyDb for TestDb { + fn semantic_index(&self, file: &Url) -> Option { + // Rebuild from source for tests. We store the source instead. + self.sources.get(file).map(|source| { + let parsed = parse(source, RParserOptions::default()); + build_index(&parsed.tree(), NoopImportsResolver) + }) + } + fn library(&self) -> &Library { + &self.library + } +} + +fn empty_library() -> Library { + Library::new(vec![], None) +} + +struct TestPackage { + name: String, + file: PathBuf, + exports: Vec, + internals: Vec, +} + +impl TestPackage { + // Most convenient input types for test construction + fn new(name: &str, file: &str, exports: Vec<&str>, internals: Vec<&str>) -> Self { + Self { + name: String::from(name), + file: PathBuf::from(file), + exports: exports + .into_iter() + .map(|export| export.to_string()) + .collect(), + internals: internals + .into_iter() + .map(|export| export.to_string()) + .collect(), + } + } +} + +fn test_library(packages: Vec) -> Library { + let cache = TestPackageCache::new().unwrap(); + + // Both exports and internals are in the test file + for package in &packages { + let content = test_file(&package.exports, &package.internals); + cache + .add(&package.name, vec![(package.file.as_path(), &content)]) + .expect("Can write to cache"); + } + + let cache = Arc::new(cache); + + let mut library = Library::new(vec![], Some(cache)); + + // Only exports are included in `Namespace` + for package in &packages { + let ns = Namespace { + exports: SortedVec::from_vec(package.exports.clone()), + ..Default::default() + }; + let desc = Description { + name: package.name.clone(), + ..Default::default() + }; + let pkg = Package::from_parts(PathBuf::from("/fake"), desc, ns); + library = library.insert(&package.name, pkg); + } + + library +} + +// Create a file worth of function definitions +fn test_file(exports: &Vec, internals: &Vec) -> String { + let mut out = String::new(); + + for export in exports { + out.push_str(&format!("{export} <- function() {{}}\n\n")); + } + for internal in internals { + out.push_str(&format!("{internal} <- function() {{}}\n\n")); + } + + out +} + +fn text_range(start: u32, end: u32) -> TextRange { + TextRange::new(TextSize::from(start), TextSize::from(end)) +} + +fn file_url(name: &str) -> Url { + Url::parse(&format!("file:///project/R/{name}")).unwrap() +} + +fn offset(n: u32) -> TextSize { + TextSize::from(n) +} + +// --- External resolution: project file --- + +#[test] +fn test_external_project_file() { + let source = "foo\n"; + let file = file_url("current.R"); let (root, idx) = parse_source(source); let db = TestDb { library: empty_library(), sources: HashMap::new(), }; + let other_url = file_url("other.R"); + let other_source = "foo <- 42\n"; + let (_other_root, other_idx) = parse_source(other_source); + let scope_chain = file_layers(other_url.clone(), &other_idx); + let targets = goto_definition( &db, - offset(7), + offset(0), &file, &root, &idx, - &ExternalScope::default(), + &ExternalScope::package(scope_chain.clone(), scope_chain), ); assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - full_range: text_range(5, 6), - focus_range: text_range(5, 6), + file: other_url, + name: "foo".to_string(), + full_range: text_range(0, 3), + focus_range: text_range(0, 3), }]); } -// --- Super assignment --- +// --- External resolution: package --- #[test] -fn test_super_assignment_resolves_in_enclosing() { - // `x <<- 1` inside a function creates a definition in the file scope. - // A use of `x` in another function should resolve to it. - let source = "f <- function() x <<- 1\ng <- function() x\n"; +fn test_external_package() { + let source = "mutate\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); + let library = test_library(vec![TestPackage::new( + "dplyr", + "dplyr.R", + vec!["filter", "mutate", "select"], + vec![], + )]); let db = TestDb { - library: empty_library(), + library, sources: HashMap::new(), }; - // `x` use in `g` body - let use_offset = source.rfind('x').unwrap() as u32; + let scope_chain = vec![ScopeLayer::PackageExports("dplyr".to_string())]; + let targets = goto_definition( &db, - offset(use_offset), + offset(0), &file, &root, &idx, - &ExternalScope::default(), + &ExternalScope::package(scope_chain.clone(), scope_chain), ); + assert_eq!(targets.len(), 1); - assert_eq!(targets[0].name, "x"); - assert_eq!(targets[0].file, file); + + let target = targets.first().unwrap(); + assert!(target.file.path().ends_with("dplyr.R")); + assert_eq!(target.name, "mutate".to_string()); } +// --- External resolution: importFrom --- + #[test] -fn test_super_assignment_definition_site() { - // Cursor on `x` in `x <<- 1` - let source = "f <- function() {\n x <<- 1\n}\n"; +fn test_external_import_from() { + let source = "tibble\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); let db = TestDb { @@ -818,112 +801,138 @@ fn test_super_assignment_definition_site() { sources: HashMap::new(), }; - // `x` at offset 20 - let def_offset = source.find("x <<-").unwrap() as u32; + let mut imports = HashMap::new(); + imports.insert("tibble".to_string(), "tibble".to_string()); + let scope_chain = vec![ScopeLayer::PackageImports(imports)]; + let targets = goto_definition( &db, - offset(def_offset), + offset(0), &file, &root, &idx, - &ExternalScope::default(), + &ExternalScope::package(scope_chain.clone(), scope_chain), ); - assert_eq!(targets.len(), 1); - assert_eq!(targets[0].name, "x"); + // importFrom resolves to a package, no file/range to navigate to + assert!(targets.is_empty()); } -// --- String definitions --- +// --- Use inside function body with cross-file definition --- #[test] -fn test_string_definition() { - // `"foo" <- 1` is equivalent to `foo <- 1` in R - let source = "\"foo\" <- 1\nfoo\n"; - let file = file_url("test.R"); +fn test_use_in_function_body_resolves_via_external() { + // Reproduces: `is_null` used inside a function body, defined in another + // file. The use is free in the function scope, so resolution should fall + // through enclosing scopes to the external scope chain. + let source = "f <- function(x) {\n if (is_null(x)) NULL\n}\n"; + let file = file_url("R/cnd-last.R"); let (root, idx) = parse_source(source); + + let other_source = "is_null <- is.null\n"; + let (_other_root, other_idx) = parse_source(other_source); + let other_url = file_url("R/types.R"); + let scope_chain = file_layers(other_url.clone(), &other_idx); + let db = TestDb { library: empty_library(), sources: HashMap::new(), }; - // Use of `foo` at offset 11 + // `is_null` starts at offset 24 + let is_null_offset = source.find("is_null").unwrap(); + assert_eq!(is_null_offset, 25); + + let scope = ExternalScope::package(Vec::new(), scope_chain); + let targets = goto_definition( &db, - offset(11), + offset(is_null_offset as u32), &file, &root, &idx, - &ExternalScope::default(), + &scope, ); assert_eq!(targets, vec![NavigationTarget { - file, - name: "foo".to_string(), - // The definition range covers the string literal `"foo"` - full_range: text_range(0, 5), - focus_range: text_range(0, 5), + file: other_url, + name: "is_null".to_string(), + full_range: text_range(0, 7), + focus_range: text_range(0, 7), }]); } -// --- Nested functions --- +// --- Local takes precedence over external --- #[test] -fn test_deeply_nested_function() { - // Free variable `z` resolves through two function scopes to file scope - let source = "z <- 1\nf <- function() {\n g <- function() {\n z\n }\n}\n"; +fn test_local_shadows_external() { + let source = "foo <- 1\nfoo\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); + let library = test_library(vec![TestPackage::new("pkg", "pkg.R", vec!["foo"], vec![])]); let db = TestDb { - library: empty_library(), + library, sources: HashMap::new(), }; - let use_offset = source.rfind('z').unwrap() as u32; + let scope_chain = vec![ScopeLayer::PackageExports("pkg".to_string())]; + + let use_offset = source.rfind("foo").unwrap() as u32; let targets = goto_definition( &db, offset(use_offset), &file, &root, &idx, - &ExternalScope::default(), + &ExternalScope::package(scope_chain.clone(), scope_chain), ); assert_eq!(targets, vec![NavigationTarget { file, - name: "z".to_string(), - full_range: text_range(0, 1), - focus_range: text_range(0, 1), + name: "foo".to_string(), + full_range: text_range(0, 3), + focus_range: text_range(0, 3), }]); } -// --- Use on RHS of assignment --- +// --- Conditional definition (may_be_unbound but has local defs) --- #[test] -fn test_use_on_rhs_of_assignment() { - // `x <- x + 1`: the `x` on the RHS refers to the previous binding - let source = "x <- 1\nx <- x + 1\n"; +fn test_conditional_definition_includes_external() { + let source = "if (TRUE) x <- 1\nx\n"; let file = file_url("test.R"); let (root, idx) = parse_source(source); + + let other_url = file_url("other.R"); + let other_source = "x <- 99\n"; + let (_other_root, other_idx) = parse_source(other_source); + let scope_chain = file_layers(other_url.clone(), &other_idx); + let db = TestDb { library: empty_library(), sources: HashMap::new(), }; - // The `x` on the RHS of the second assignment. `x <- x + 1` starts at - // offset 7, the RHS `x` is at offset 12. - let rhs_offset = 7 + "x <- ".len() as u32; + let use_offset = source.rfind('x').unwrap() as u32; let targets = goto_definition( &db, - offset(rhs_offset), + offset(use_offset), &file, &root, &idx, - &ExternalScope::default(), + &ExternalScope::package(scope_chain.clone(), scope_chain), ); - assert_eq!(targets, vec![NavigationTarget { - file, - name: "x".to_string(), - // Resolves to the first definition - full_range: text_range(0, 1), - focus_range: text_range(0, 1), - }]); + assert_eq!(targets, vec![ + NavigationTarget { + file, + name: "x".to_string(), + full_range: text_range(10, 11), + focus_range: text_range(10, 11), + }, + NavigationTarget { + file: other_url, + name: "x".to_string(), + full_range: text_range(0, 1), + focus_range: text_range(0, 1), + }, + ]); } // --- library() directive in predecessor file --- @@ -1167,64 +1176,6 @@ fn test_fixme_namespace_access_cursor_on_operator() { assert_eq!(target.name, "mutate".to_string()); } -#[test] -fn test_namespace_classify() { - use oak_ide::Identifier; - - let source = "dplyr::mutate\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - // Cursor on `mutate` (offset 7) - let ident = Identifier::classify(&root, &idx, offset(7)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "dplyr".to_string(), - symbol: "mutate".to_string(), - internal: false, - package_range: text_range(0, 5), - symbol_range: text_range(7, 13), - }) - ); - - // Cursor on `dplyr` (offset 2) - let ident = Identifier::classify(&root, &idx, offset(2)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "dplyr".to_string(), - symbol: "mutate".to_string(), - internal: false, - package_range: text_range(0, 5), - symbol_range: text_range(7, 13), - }) - ); -} - -#[test] -fn test_namespace_classify_triple_colon() { - use oak_ide::Identifier; - - let source = "pkg:::sym\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - let ident = Identifier::classify(&root, &idx, offset(6)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "pkg".to_string(), - symbol: "sym".to_string(), - internal: true, - package_range: text_range(0, 3), - symbol_range: text_range(6, 9), - }) - ); -} - #[test] fn test_namespace_access_in_call() { // foo::bar() — cursor on `bar` @@ -1281,83 +1232,6 @@ fn test_namespace_access_in_extract() { assert_eq!(target.name, "bar".to_string()); } -#[test] -fn test_namespace_classify_in_call() { - use oak_ide::Identifier; - - // foo::bar() - // 0123456789 - let source = "foo::bar()\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - let ident = Identifier::classify(&root, &idx, offset(5)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "foo".to_string(), - symbol: "bar".to_string(), - internal: false, - package_range: text_range(0, 3), - symbol_range: text_range(5, 8), - }) - ); -} - -#[test] -fn test_namespace_classify_in_extract() { - use oak_ide::Identifier; - - // foo::bar$baz - // 0123456789... - let source = "foo::bar$baz\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - // Cursor on `bar` (offset 5) — inside the RNamespaceExpression - let ident = Identifier::classify(&root, &idx, offset(5)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "foo".to_string(), - symbol: "bar".to_string(), - internal: false, - package_range: text_range(0, 3), - symbol_range: text_range(5, 8), - }) - ); - - // Cursor on `baz` (offset 9) — RHS of $, not a namespace access - let ident = Identifier::classify(&root, &idx, offset(9)); - assert_eq!(ident, None); -} - -#[test] -fn test_namespace_classify_string_selectors() { - use oak_ide::Identifier; - - // "foo"::"bar" - // 0123456789... - let source = "\"foo\"::\"bar\"\n"; - let parsed = parse(source, RParserOptions::default()); - let root = parsed.syntax(); - let idx = build_index(&parsed.tree(), NoopImportsResolver); - - let ident = Identifier::classify(&root, &idx, offset(7)); - assert_eq!( - ident, - Some(Identifier::NamespaceAccess { - package: "foo".to_string(), - symbol: "bar".to_string(), - internal: false, - package_range: text_range(0, 5), - symbol_range: text_range(7, 12), - }) - ); -} - // --- source() directive --- #[test] @@ -1796,3 +1670,4 @@ fn test_resolve_import_last_def_wins() { focus_range: text_range(26, 29), }]); } +} diff --git a/crates/oak_ide/tests/rename.rs b/crates/oak_ide/tests/rename.rs new file mode 100644 index 0000000000..24b1798b33 --- /dev/null +++ b/crates/oak_ide/tests/rename.rs @@ -0,0 +1,225 @@ +use aether_parser::parse; +use aether_parser::RParserOptions; +use aether_syntax::RSyntaxNode; +use biome_rowan::TextRange; +use biome_rowan::TextSize; +use oak_ide::prepare_rename; +use oak_ide::rename; +use oak_ide::FileOffset; +use oak_semantic::build_index; +use oak_semantic::semantic_index::SemanticIndex; +use oak_semantic::NoopImportsResolver; +use url::Url; + +fn parse_source(source: &str) -> (RSyntaxNode, SemanticIndex) { + let parsed = parse(source, RParserOptions::default()); + let root = parsed.syntax(); + let index = build_index(&parsed.tree(), NoopImportsResolver); + (root, index) +} + +fn text_range(start: u32, end: u32) -> TextRange { + TextRange::new(TextSize::from(start), TextSize::from(end)) +} + +fn file_url(name: &str) -> Url { + Url::parse(&format!("file:///project/R/{name}")).unwrap() +} + +fn pos(file: &Url, n: u32) -> FileOffset { + FileOffset { + file: file.clone(), + offset: TextSize::from(n), + } +} + +fn edited_ranges(targets: oak_ide::RenameTargets) -> Vec { + targets.ranges.into_iter().map(|r| r.range).collect() +} + +// --- prepare_rename --- + +#[test] +fn test_prepare_rename_on_def() { + let source = "foo <- 1\nfoo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on the def `foo` at offset 0 + let result = prepare_rename(&idx, &root, &pos(&file, 0)).unwrap(); + assert_eq!(result, (text_range(0, 3), "foo".to_string())); +} + +#[test] +fn test_prepare_rename_on_use() { + let source = "foo <- 1\nfoo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on the use at offset 9 + let result = prepare_rename(&idx, &root, &pos(&file, 9)).unwrap(); + assert_eq!(result, (text_range(9, 12), "foo".to_string())); +} + +#[test] +fn test_prepare_rename_namespace_access_returns_none() { + let source = "dplyr::mutate\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + assert!(prepare_rename(&idx, &root, &pos(&file, 7)).is_none()); +} + +#[test] +fn test_prepare_rename_non_identifier_returns_none() { + let source = "x <- 1\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + // Cursor on `<-` operator + assert!(prepare_rename(&idx, &root, &pos(&file, 3)).is_none()); +} + +// --- rename: basic --- + +#[test] +fn test_rename_def_and_use() { + let source = "foo <- 1\nfoo + foo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let targets = rename(&idx, &root, &pos(&file, 0), "bar").unwrap(); + assert_eq!(targets.new_text, "bar"); + assert_eq!(edited_ranges(targets), vec![ + text_range(0, 3), + text_range(9, 12), + text_range(15, 18), + ]); +} + +#[test] +fn test_rename_excludes_shadowed_outer() { + // Inner `x` is a different binding from outer `x`. Renaming inner + // should not touch outer. + let source = "x <- 1\nf <- function() {\n x <- 2\n x\n}\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let inner_def = source.find("x <- 2").unwrap() as u32; + let targets = rename(&idx, &root, &pos(&file, inner_def), "y").unwrap(); + assert_eq!(edited_ranges(targets), vec![ + text_range(inner_def, inner_def + 1), + text_range( + source.rfind('x').unwrap() as u32, + source.rfind('x').unwrap() as u32 + 1 + ), + ]); +} + +// --- rename: validation --- + +#[test] +fn test_rename_empty_name_errors() { + let source = "foo <- 1\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let err = rename(&idx, &root, &pos(&file, 0), "").unwrap_err(); + assert!(err.to_string().contains("empty")); +} + +#[test] +fn test_rename_reserved_word_errors() { + let source = "foo <- 1\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + for word in ["if", "for", "function", "TRUE", "NULL", "NA"] { + let err = rename(&idx, &root, &pos(&file, 0), word).unwrap_err(); + assert!( + err.to_string().contains("reserved"), + "expected {word} to be rejected" + ); + } +} + +#[test] +fn test_rename_non_renamable_errors() { + // Cursor on `mutate` in `dplyr::mutate` (NamespaceAccess: not a + // renamable identifier). TODO: if package is in the workspace we + // should allow renaming. + let source = "dplyr::mutate\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let err = rename(&idx, &root, &pos(&file, 7), "x").unwrap_err(); + assert!(err.to_string().contains("renamable identifier")); +} + +#[test] +fn test_rename_unbound_use_errors() { + // Free variable: classifies as a Use but has no local binding, so + // rename refuses rather than producing a partial edit. + let source = "foo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let err = rename(&idx, &root, &pos(&file, 0), "bar").unwrap_err(); + assert!(err.to_string().contains("no local binding")); +} + +// --- rename: backtick canonicalization --- + +#[test] +fn test_rename_to_name_with_space_gets_backticked() { + let source = "foo <- 1\nfoo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let targets = rename(&idx, &root, &pos(&file, 0), "foo bar").unwrap(); + assert_eq!(targets.new_text, "`foo bar`"); +} + +#[test] +fn test_rename_to_name_with_starting_digit_gets_backticked() { + let source = "foo <- 1\nfoo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let targets = rename(&idx, &root, &pos(&file, 0), "1foo").unwrap(); + assert_eq!(targets.new_text, "`1foo`"); +} + +#[test] +fn test_rename_to_name_with_backtick_errors() { + let source = "foo <- 1\nfoo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let err = rename(&idx, &root, &pos(&file, 0), "foo`bar").unwrap_err(); + assert!(err.to_string().contains("backtick")); +} + +// --- rename: string definitions --- + +#[test] +fn test_rename_string_def_normalizes_to_identifier_form() { + // `"foo" <- 1` defines `foo` using R's rarely-seen string-literal + // assignment form. The semantic index records the def's range as + // covering the whole `"foo"` token (5 chars), so rename emits an + // edit that replaces `"foo"` with the new name in bare identifier + // form. The user's quoting style is intentionally not preserved: + // `bar <- 1` is valid R, more idiomatic than `"bar" <- 1`, and + // preserving it would require per-site `new_text` plus escaping + // rules for new names that contain quotes or backslashes. + let source = "\"foo\" <- 1\nfoo\n"; + let file = file_url("test.R"); + let (root, idx) = parse_source(source); + + let targets = rename(&idx, &root, &pos(&file, 11), "bar").unwrap(); + assert_eq!(targets.new_text, "bar"); + assert_eq!(edited_ranges(targets), vec![ + text_range(0, 5), // covers `"foo"` (replaced as a whole) + text_range(11, 14), // covers `foo` (the use site) + ]); +} diff --git a/crates/oak_semantic/Cargo.toml b/crates/oak_semantic/Cargo.toml index 3d26667650..28bc9f26ce 100644 --- a/crates/oak_semantic/Cargo.toml +++ b/crates/oak_semantic/Cargo.toml @@ -3,7 +3,7 @@ name = "oak_semantic" version = "0.1.0" description = """ Per-file semantic index for R: scopes, symbols, bindings, and uses. -Also includes the Library cache, package definitions, and scope layers. +Also includes the Library cache. """ authors.workspace = true @@ -29,7 +29,6 @@ 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 @@ -44,7 +43,5 @@ workspace = true optional = true [dev-dependencies] -assert_matches.workspace = true oak_semantic = { workspace = true, features = ["testing"] } -oak_sources = { workspace = true, features = ["testing"] } tempfile.workspace = true diff --git a/crates/oak_semantic/src/external.rs b/crates/oak_semantic/src/external.rs deleted file mode 100644 index 7daafcaf48..0000000000 --- a/crates/oak_semantic/src/external.rs +++ /dev/null @@ -1,108 +0,0 @@ -use biome_rowan::TextRange; -use stdext::result::ResultExt; -use url::Url; - -use crate::library::Library; -use crate::package_definitions::PackageDefinitionVisibility; -use crate::scope_layer::ScopeLayer; - -/// The result of resolving a name against the external scope chain. -/// -/// TODO: Try to fold into `Definition` after switch to Salsa -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct ExternalDefinition { - file: Url, - name: String, - range: TextRange, -} - -impl ExternalDefinition { - pub fn into_parts(self) -> (Url, String, TextRange) { - (self.file, self.name, self.range) - } - - pub fn file(&self) -> &Url { - &self.file - } - - pub fn name(&self) -> &str { - &self.name - } - - pub fn range(&self) -> TextRange { - self.range - } -} - -/// Walk the scope chain front-to-back, returning the first match. -pub fn resolve_external_name( - library: &Library, - scope: &[ScopeLayer], - name: &str, -) -> Option { - for source in scope { - match source { - ScopeLayer::FileExports { file, exports } => { - if let Some(range) = exports.get(name) { - return Some(ExternalDefinition { - file: file.clone(), - name: name.to_string(), - range: *range, - }); - } - }, - - ScopeLayer::PackageImports(names) => { - if let Some(pkg) = names.get(name) { - if let Some(def) = resolve_in_package( - library, - pkg, - name, - PackageDefinitionVisibility::Exported, - ) { - return Some(def); - } - } - }, - - ScopeLayer::PackageExports(pkg) => { - if let Some(def) = - resolve_in_package(library, pkg, name, PackageDefinitionVisibility::Exported) - { - return Some(def); - } - }, - } - } - - None -} - -/// Resolve a name in a specific package's exported symbols. -pub fn resolve_in_package( - library: &Library, - package: &str, - name: &str, - visibility: PackageDefinitionVisibility, -) -> Option { - // FIXME: Slow without salsa! - let package_definitions = library.definitions(package)?; - let definition = package_definitions.get(name)?; - - if visibility == PackageDefinitionVisibility::Exported && - definition.visibility() == PackageDefinitionVisibility::Internal - { - // Requested only exports, but located definition is internal. - // Notably if we requested internal, both exports and internal are returned. - return None; - } - - let file = package_definitions.file(definition.file_id()); - let file = Url::from_file_path(file).log_err()?; - - Some(ExternalDefinition { - file, - name: name.to_string(), - range: definition.range(), - }) -} diff --git a/crates/oak_semantic/src/lib.rs b/crates/oak_semantic/src/lib.rs index 49c05a8731..eeac6409dc 100644 --- a/crates/oak_semantic/src/lib.rs +++ b/crates/oak_semantic/src/lib.rs @@ -1,10 +1,7 @@ pub mod builder; -pub mod external; pub mod library; pub mod package; -pub mod package_definitions; pub mod resolver; -pub mod scope_layer; pub mod semantic_index; pub mod use_def_map; diff --git a/crates/oak_semantic/src/library.rs b/crates/oak_semantic/src/library.rs index 30b9b05ae5..59fd147809 100644 --- a/crates/oak_semantic/src/library.rs +++ b/crates/oak_semantic/src/library.rs @@ -3,11 +3,9 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::RwLock; -use oak_sources::PackageSources; -use stdext::result::ResultExt; - use crate::package::Package; -use crate::package_definitions::PackageDefinitions; + +// TODO(salsa): Replace by oak_db inputs /// Lazily manages a list of known R packages by name #[derive(Default, Clone, Debug)] @@ -15,23 +13,13 @@ pub struct Library { /// Paths to library directories, i.e. what `base::libPaths()` returns. pub library_paths: Arc>, - /// Object for loading package sources - /// - /// Stored as `dyn PackageSources` so we can easily swap in a test cache during - /// LSP feature testing - package_sources: Option>, - packages: Arc>>>>, } impl Library { - pub fn new( - library_paths: Vec, - package_sources: Option>, - ) -> Self { + pub fn new(library_paths: Vec) -> Self { Self { library_paths: Arc::new(library_paths), - package_sources, packages: Arc::new(RwLock::new(HashMap::new())), } } @@ -63,18 +51,6 @@ impl Library { pkg } - /// Collect all top level definitions from a package's sources - /// - /// FIXME: This is currently very expensive as it reparses the package at every call. - /// We expect this to be a tracked salsa function, which should memoize it - /// efficiently. - pub fn definitions(&self, name: &str) -> Option { - let package_sources = self.package_sources.as_ref()?; - let package = self.get(name)?; - let directory = package_sources.get(&package.description().name)?; - PackageDefinitions::load_from_directory(&directory, package.namespace()).log_err() - } - /// Insert a package in the library for testing purposes. #[cfg(any(test, feature = "testing"))] pub fn insert(self, name: &str, package: Package) -> Self { @@ -144,7 +120,7 @@ importFrom(pkg, baz) let (temp_dir, _pkg_dir) = create_temp_package(pkg_name, description, namespace); // Library should point to the temp_dir as its only library path - let lib = Library::new(vec![temp_dir.path().to_path_buf()], None); + let lib = Library::new(vec![temp_dir.path().to_path_buf()]); // First access loads from disk let pkg = lib.get(pkg_name).unwrap(); diff --git a/crates/oak_semantic/src/package_definitions.rs b/crates/oak_semantic/src/package_definitions.rs deleted file mode 100644 index 7b529a0e69..0000000000 --- a/crates/oak_semantic/src/package_definitions.rs +++ /dev/null @@ -1,165 +0,0 @@ -use std::fs::DirEntry; -use std::path::Path; -use std::path::PathBuf; - -use aether_parser::RParserOptions; -use biome_text_size::TextRange; -use oak_index_vec::define_index; -use oak_index_vec::IndexVec; -use oak_package_metadata::namespace::Namespace; -use rustc_hash::FxHashMap; -use stdext::result::ResultExt; - -use crate::build_index; -use crate::resolver::NoopImportsResolver; - -define_index!(FileId); - -/// The top level definitions exposed by a single package -#[derive(Debug)] -pub struct PackageDefinitions { - /// Vector of file names indexable by `FileId` - files: IndexVec, - - /// Map from object name to its definition - definitions: FxHashMap, -} - -impl PackageDefinitions { - /// Given an `R/` directory, load all files recursively within that directory - /// - /// Most R packages have flat `R/` directories, but some base packages have OS - /// specific subdirectories, notably utils and parallel. - pub(crate) fn load_from_directory( - directory: &Path, - namespace: &Namespace, - ) -> anyhow::Result { - let mut files = IndexVec::new(); - let mut definitions = FxHashMap::default(); - - visit_paths( - directory, - namespace, - &mut files, - &mut definitions, - &append_definitions, - )?; - - Ok(Self { files, definitions }) - } - - /// Get a [PackageDefinition] by name - pub fn get(&self, name: &str) -> Option<&PackageDefinition> { - self.definitions.get(name) - } - - /// Get the [Path] for a [FileId] - pub fn file(&self, file_id: FileId) -> &Path { - self.files[file_id].as_path() - } -} - -/// For a given R file: -/// - Read from disk -/// - Parse -/// - Create a semantic index -/// - Extract top level file exports and categorize as exported/internal -fn append_definitions( - entry: &DirEntry, - namespace: &Namespace, - files: &mut IndexVec, - definitions: &mut FxHashMap, -) { - let file = entry.path(); - - if !oak_core::file::is_r_file(&file) { - return; - } - - let Some(content) = std::fs::read_to_string(&file).log_err() else { - return; - }; - - let parsed = aether_parser::parse(&content, RParserOptions::default()); - if parsed.has_error() { - return; - } - - let index = build_index(&parsed.tree(), NoopImportsResolver); - - let file_id = files.push(file); - - for (name, def) in index.exports() { - let visibility = if namespace.exports.contains_str(name) { - PackageDefinitionVisibility::Exported - } else { - PackageDefinitionVisibility::Internal - }; - - let definition = PackageDefinition { - visibility, - file_id, - range: def.range(), - }; - - definitions.insert(name.to_string(), definition); - } -} - -/// Recursively walk a directory -#[expect(clippy::type_complexity)] -fn visit_paths( - directory: &Path, - namespace: &Namespace, - files: &mut IndexVec, - definitions: &mut FxHashMap, - cb: &dyn Fn( - &DirEntry, - &Namespace, - &mut IndexVec, - &mut FxHashMap, - ), -) -> std::io::Result<()> { - if directory.is_dir() { - for entry in std::fs::read_dir(directory)? { - let entry = entry?; - let path = entry.path(); - if path.is_dir() { - visit_paths(&path, namespace, files, definitions, cb)?; - } else { - cb(&entry, namespace, files, definitions); - } - } - } - Ok(()) -} - -#[derive(Debug, Clone, Copy, PartialEq)] -pub enum PackageDefinitionVisibility { - Exported, - Internal, -} - -/// A definition within a package -/// -/// Retrieve its original file name using its [FileId] and [PackageDefinitions::file()]. -#[derive(Debug)] -pub struct PackageDefinition { - visibility: PackageDefinitionVisibility, - file_id: FileId, - range: TextRange, -} - -impl PackageDefinition { - pub fn visibility(&self) -> PackageDefinitionVisibility { - self.visibility - } - - pub fn file_id(&self) -> FileId { - self.file_id - } - - pub fn range(&self) -> TextRange { - self.range - } -} diff --git a/crates/oak_semantic/src/scope_layer.rs b/crates/oak_semantic/src/scope_layer.rs deleted file mode 100644 index 45220ed952..0000000000 --- a/crates/oak_semantic/src/scope_layer.rs +++ /dev/null @@ -1,112 +0,0 @@ -//! TODO(salsa): Pre-salsa scope-chain implementation. The salsa-tracked -//! equivalents live in `oak_db::file_imports` (`File::imports` / -//! `File::imports_at`) and `oak_db::file_resolve` (`File::resolve` / -//! `File::resolve_at`). This module is still used by the `LegacyDb`-based -//! body of `oak_ide::goto_definition`. It goes away once that body is -//! rewritten on top of `File::resolve_at(db, offset)`. At that point -//! `ExternalScope`, the `LegacyDb` helpers in `oak_ide::goto_definition`, -//! `ark::lsp::state::file_analysis`, and this module all go together. - -use std::collections::HashMap; - -use biome_rowan::TextRange; -use oak_package_metadata::namespace::Namespace; -use url::Url; - -use crate::semantic_index::SemanticCallKind; -use crate::semantic_index::SemanticIndex; - -/// A layer in the scope chain. Layers are ordered most-local-first; resolution -/// iterates front-to-back, first match wins. -#[derive(Debug, Clone)] -pub enum ScopeLayer { - /// Bindings from a project file's top-level definitions. - /// When a name is defined multiple times, the last definition wins. - FileExports { - file: Url, - exports: HashMap, - }, - - /// Imports from e.g. `importFrom`. Maps symbol name to package name. - PackageImports(HashMap), - - /// Exports of an attached package (`library()` or NAMESPACE `import()`). - PackageExports(String), -} - -/// Compute the scope layers that a single file contributes to the -/// scope chain: one `FileExports` layer from its top-level definitions, plus -/// one `PackageExports` layer per `library()`/`require()` semantic call. -pub fn file_layers(file: Url, index: &SemanticIndex) -> Vec { - let mut layers = Vec::new(); - - let exports = index - .exports() - .into_iter() - .map(|(name, def)| (name.to_string(), def.range())) - .collect(); - - layers.push(ScopeLayer::FileExports { file, exports }); - - for call in index.semantic_calls() { - match call.kind() { - SemanticCallKind::Attach { package } => { - layers.push(ScopeLayer::PackageExports(package.clone())); - }, - SemanticCallKind::Source { .. } => { - // `source()` injects into local scope, not the search path; - // not a scope-chain layer. - }, - } - } - - layers -} - -/// The default R search path for scripts: the default packages that R -/// attaches on startup, in search order (last attached = searched first). -pub fn default_search_path() -> Vec { - // R's default packages, in reverse attachment order (most recently - // attached first). These are always on the search path unless - // overridden by `R_DEFAULT_PACKAGES`. - let default_packages = [ - "utils", - "stats", - "datasets", - "methods", - "grDevices", - "graphics", - "base", - ]; - default_packages - .into_iter() - .map(|pkg| ScopeLayer::PackageExports(pkg.to_string())) - .collect() -} - -/// Build the root layers for a package from its NAMESPACE. -/// -/// These go at the back of every file's scope chain: -/// - `PackageImports` from `importFrom()` directives (name → package) -/// - `PackageExports` from `import()` directives -/// - `PackageExports` for `base` (always implicitly available) -pub fn package_root_layers(namespace: &Namespace) -> Vec { - let mut layers = Vec::new(); - - if !namespace.imports.is_empty() { - let map = namespace - .imports - .iter() - .map(|imp| (imp.name.clone(), imp.package.clone())) - .collect(); - layers.push(ScopeLayer::PackageImports(map)); - } - - for pkg in &namespace.package_imports { - layers.push(ScopeLayer::PackageExports(pkg.clone())); - } - - layers.push(ScopeLayer::PackageExports("base".to_string())); - - layers -} diff --git a/crates/oak_semantic/src/semantic_index.rs b/crates/oak_semantic/src/semantic_index.rs index 8dc362a6ae..1b830fa17e 100644 --- a/crates/oak_semantic/src/semantic_index.rs +++ b/crates/oak_semantic/src/semantic_index.rs @@ -161,7 +161,7 @@ impl SemanticIndex { // Start at the file scope let mut current = ScopeId::from(0); 'outer: loop { - for child_id in self.child_scopes(current) { + for child_id in self.child_scope_ids(current) { if self.scopes[child_id].range.contains(offset) { current = child_id; continue 'outer; @@ -189,21 +189,26 @@ impl SemanticIndex { } /// Iterate direct child scopes of `scope`. - pub fn child_scopes(&self, scope: ScopeId) -> ChildScopesIter<'_> { - let descendants = &self.scopes[scope].descendants; - ChildScopesIter { + pub fn child_scope_ids(&self, scope_id: ScopeId) -> ChildScopeIdsIter<'_> { + let descendants = &self.scopes[scope_id].descendants; + ChildScopeIdsIter { index: self, current: descendants.start, end: descendants.end, } } + /// Iterate over every scope in the file (source order, file scope first). + pub fn scope_ids(&self) -> impl Iterator + '_ { + self.scopes.iter().map(|(id, _)| id) + } + /// Walk from `scope` up through ancestors to the file root. Note that /// `scope` itself is included in the ancestors. - pub fn ancestor_scopes(&self, scope: ScopeId) -> AncestorsIter<'_> { - AncestorsIter { + pub fn ancestor_scope_ids(&self, scope_id: ScopeId) -> AncestorScopeIdsIter<'_> { + AncestorScopeIdsIter { index: self, - current: Some(scope), + current: Some(scope_id), } } @@ -216,7 +221,7 @@ impl SemanticIndex { name: &str, scope: ScopeId, ) -> Option<(ScopeId, DefinitionId, &Definition)> { - for ancestor in self.ancestor_scopes(scope) { + for ancestor in self.ancestor_scope_ids(scope) { let Some(symbol_id) = self.symbol_tables[ancestor].id(name) else { continue; }; @@ -247,6 +252,37 @@ impl SemanticIndex { None } + /// All definitions that could reach the use at `(scope, use_id)`. + /// + /// The local use-def bindings always count. The enclosing-scope snapshot + /// also counts when `may_be_unbound` is true. That happens when the local + /// binding doesn't cover every control-flow path, so execution can fall + /// through to the outer scope. + /// + /// When `may_be_unbound` is false we deliberately skip the enclosing scope. + /// Otherwise a shadowed inner use would also bind to the outer def of the + /// same name. + pub fn reaching_definitions( + &self, + scope_id: ScopeId, + use_id: UseId, + ) -> impl Iterator + '_ { + let bindings = self.use_def_map(scope_id).bindings_at_use(use_id); + let local = bindings.definitions().iter().map(move |&d| (scope_id, d)); + + let enclosing = if bindings.may_be_unbound() { + let symbol_id = self.uses(scope_id)[use_id].symbol(); + self.enclosing_bindings(scope_id, symbol_id) + } else { + None + }; + let enclosing_iter = enclosing.into_iter().flat_map(|(scope, bindings)| { + bindings.definitions().iter().map(move |&def| (scope, def)) + }); + + local.chain(enclosing_iter) + } + /// Resolve a free variable's bindings from the enclosing scope. /// /// When a use in `scope` may be unbound (`may_be_unbound: true`), some @@ -628,13 +664,13 @@ impl SemanticCall { // --- Iterators --- -pub struct ChildScopesIter<'a> { +pub struct ChildScopeIdsIter<'a> { index: &'a SemanticIndex, current: ScopeId, end: ScopeId, } -impl<'a> Iterator for ChildScopesIter<'a> { +impl<'a> Iterator for ChildScopeIdsIter<'a> { type Item = ScopeId; fn next(&mut self) -> Option { @@ -648,12 +684,12 @@ impl<'a> Iterator for ChildScopesIter<'a> { } } -pub struct AncestorsIter<'a> { +pub struct AncestorScopeIdsIter<'a> { index: &'a SemanticIndex, current: Option, } -impl<'a> Iterator for AncestorsIter<'a> { +impl<'a> Iterator for AncestorScopeIdsIter<'a> { type Item = ScopeId; fn next(&mut self) -> Option { diff --git a/crates/oak_semantic/tests/builder.rs b/crates/oak_semantic/tests/builder.rs index a7133b52b8..ea15780e92 100644 --- a/crates/oak_semantic/tests/builder.rs +++ b/crates/oak_semantic/tests/builder.rs @@ -273,7 +273,7 @@ fn test_child_scopes() { let index = index("f <- function(x) x\ng <- function(y) y"); let file = ScopeId::from(0); - let children: Vec<_> = index.child_scopes(file).collect(); + let children: Vec<_> = index.child_scope_ids(file).collect(); assert_eq!(children.len(), 2); } @@ -284,7 +284,7 @@ fn test_ancestor_scopes() { let outer = ScopeId::from(1); let file = ScopeId::from(0); - let ancestors: Vec<_> = index.ancestor_scopes(inner).collect(); + let ancestors: Vec<_> = index.ancestor_scope_ids(inner).collect(); assert_eq!(ancestors, vec![inner, outer, file]); } diff --git a/crates/oak_semantic/tests/external.rs b/crates/oak_semantic/tests/external.rs deleted file mode 100644 index 9d937969a7..0000000000 --- a/crates/oak_semantic/tests/external.rs +++ /dev/null @@ -1,604 +0,0 @@ -use std::path::PathBuf; -use std::sync::Arc; - -use aether_parser::parse; -use aether_parser::RParserOptions; -use assert_matches::assert_matches; -use biome_rowan::TextRange; -use biome_rowan::TextSize; -use oak_package_metadata::description::Description; -use oak_package_metadata::namespace::Import; -use oak_package_metadata::namespace::Namespace; -use oak_semantic::build_index; -use oak_semantic::external::resolve_external_name; -use oak_semantic::library::Library; -use oak_semantic::package::Package; -use oak_semantic::scope_layer::file_layers; -use oak_semantic::scope_layer::package_root_layers; -use oak_semantic::scope_layer::ScopeLayer; -use oak_semantic::NoopImportsResolver; -use oak_sources::test::TestPackageCache; -use stdext::SortedVec; -use url::Url; - -fn empty_library() -> Library { - Library::new(vec![], None) -} - -struct TestPackage { - name: String, - file: PathBuf, - exports: Vec, - internals: Vec, -} - -impl TestPackage { - // Most convenient input types for test construction - fn new(name: &str, file: &str, exports: Vec<&str>, internals: Vec<&str>) -> Self { - Self { - name: String::from(name), - file: PathBuf::from(file), - exports: exports - .into_iter() - .map(|export| export.to_string()) - .collect(), - internals: internals - .into_iter() - .map(|export| export.to_string()) - .collect(), - } - } -} - -fn test_library(packages: Vec) -> Library { - let cache = TestPackageCache::new().unwrap(); - - // Both exports and internals are in the test file - for package in &packages { - let content = test_file(&package.exports, &package.internals); - cache - .add(&package.name, vec![(package.file.as_path(), &content)]) - .expect("Can write to cache"); - } - - let cache = Arc::new(cache); - - let mut library = Library::new(vec![], Some(cache)); - - // Only exports are included in `Namespace` - for package in &packages { - let ns = Namespace { - exports: SortedVec::from_vec(package.exports.clone()), - ..Default::default() - }; - let desc = Description { - name: package.name.clone(), - ..Default::default() - }; - let pkg = Package::from_parts(PathBuf::from("/fake"), desc, ns); - library = library.insert(&package.name, pkg); - } - - library -} - -// Create a file worth of function definitions -fn test_file(exports: &Vec, internals: &Vec) -> String { - let mut out = String::new(); - - for export in exports { - out.push_str(&format!("{export} <- function() {{}}\n\n")); - } - for internal in internals { - out.push_str(&format!("{internal} <- function() {{}}\n\n")); - } - - out -} - -fn range(start: u32, end: u32) -> TextRange { - TextRange::new(TextSize::from(start), TextSize::from(end)) -} - -fn file_url(name: &str) -> Url { - Url::parse(&format!("file:///project/R/{name}")).unwrap() -} - -fn file_exports(file: &str, entries: Vec<(&str, TextRange)>) -> ScopeLayer { - ScopeLayer::FileExports { - file: file_url(file), - exports: entries - .into_iter() - .map(|(n, r)| (n.to_string(), r)) - .collect(), - } -} - -fn package_imports(entries: Vec<(&str, &str)>) -> ScopeLayer { - ScopeLayer::PackageImports( - entries - .into_iter() - .map(|(n, p)| (n.to_string(), p.to_string())) - .collect(), - ) -} - -// --- resolve_external_name --- - -#[test] -fn test_resolve_file_exports() { - let scope = vec![file_exports("utils.R", vec![("helper", range(0, 6))])]; - - let result = resolve_external_name(&empty_library(), &scope, "helper").unwrap(); - assert_eq!(result.file(), &file_url("utils.R")); - assert_eq!(result.name(), "helper"); - assert_eq!(result.range(), range(0, 6)); -} - -#[test] -fn test_resolve_file_exports_miss() { - let scope = vec![file_exports("utils.R", vec![("helper", range(0, 6))])]; - - let result = resolve_external_name(&empty_library(), &scope, "other"); - assert_eq!(result, None); -} - -#[test] -fn test_resolve_imported_names() { - let library = test_library(vec![TestPackage::new( - "stats", - "stats.R", - vec!["median"], - vec![], - )]); - let scope = vec![package_imports(vec![("median", "stats")])]; - - let result = resolve_external_name(&library, &scope, "median").unwrap(); - assert!(result.file().path().ends_with("stats.R")); - assert_eq!(result.name(), "median") -} - -#[test] -fn test_resolve_package_exports() { - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - - let scope = vec![ScopeLayer::PackageExports("dplyr".to_string())]; - - let result = resolve_external_name(&library, &scope, "filter").unwrap(); - assert!(result.file().path().ends_with("dplyr.R")); - assert_eq!(result.name(), "filter"); -} - -#[test] -fn test_resolve_package_exports_miss() { - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate", "select"], - vec![], - )]); - - let scope = vec![ScopeLayer::PackageExports("dplyr".to_string())]; - - let result = resolve_external_name(&library, &scope, "summarise"); - assert_eq!(result, None); -} - -#[test] -fn test_resolve_unknown_package_skipped() { - let scope = vec![ScopeLayer::PackageExports("nonexistent".to_string())]; - - let result = resolve_external_name(&empty_library(), &scope, "foo"); - assert_eq!(result, None); -} - -#[test] -fn test_resolve_package_shadowing() { - // Both dplyr and stats export `filter`. dplyr was loaded later so it - // appears earlier in the scope and shadows stats's version. - let library = test_library(vec![ - TestPackage::new("stats", "stats.R", vec!["filter", "median"], vec![]), - TestPackage::new("dplyr", "dplyr.R", vec!["filter", "mutate"], vec![]), - ]); - - let scope = vec![ - ScopeLayer::PackageExports("dplyr".to_string()), - ScopeLayer::PackageExports("stats".to_string()), - ]; - - // dplyr's `filter` wins - let result = resolve_external_name(&library, &scope, "filter").unwrap(); - assert!(result.file().path().ends_with("dplyr.R")); - assert_eq!(result.name(), "filter"); - - // `median` only in stats, falls through - let result = resolve_external_name(&library, &scope, "median").unwrap(); - assert!(result.file().path().ends_with("stats.R")); - assert_eq!(result.name(), "median"); -} - -#[test] -fn test_resolve_first_match_wins() { - let library = test_library(vec![TestPackage::new( - "stats", - "stats.R", - vec!["filter"], - vec![], - )]); - - let scope = vec![ - file_exports("utils.R", vec![("filter", range(0, 6))]), - ScopeLayer::PackageExports("stats".to_string()), - ]; - - // File export should win over package export - let result = resolve_external_name(&library, &scope, "filter").unwrap(); - assert_eq!(result.file(), &file_url("utils.R")); - assert_eq!(result.name(), "filter"); - assert_eq!(result.range(), range(0, 6)); -} - -#[test] -fn test_resolve_falls_through_to_later_layer() { - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate"], - vec![], - )]); - - let scope = vec![ - file_exports("utils.R", vec![("helper", range(0, 6))]), - ScopeLayer::PackageExports("dplyr".to_string()), - ]; - - // "filter" is not in file exports, falls through to package - let result = resolve_external_name(&library, &scope, "filter").unwrap(); - assert!(result.file().path().ends_with("dplyr.R")); - assert_eq!(result.name(), "filter"); -} - -#[test] -fn test_resolve_imported_names_shadow_package_exports() { - let library = test_library(vec![ - TestPackage::new("dplyr", "dplyr.R", vec!["filter"], vec![]), - TestPackage::new("stats", "stats.R", vec!["filter"], vec![]), - ]); - - let scope = vec![ - package_imports(vec![("filter", "stats")]), - ScopeLayer::PackageExports("dplyr".to_string()), - ]; - - let result = resolve_external_name(&library, &scope, "filter").unwrap(); - assert!(result.file().path().ends_with("stats.R")); - assert_eq!(result.name(), "filter"); -} - -#[test] -fn test_resolve_empty_scope() { - let result = resolve_external_name(&empty_library(), &[], "anything"); - assert_eq!(result, None); -} - -#[test] -fn test_resolve_file_exports_last_definition_wins() { - // HashMap built from a vec: last insert wins - let scope = vec![file_exports("utils.R", vec![ - ("x", range(0, 1)), - ("x", range(10, 11)), - ])]; - - let result = resolve_external_name(&empty_library(), &scope, "x").unwrap(); - assert_eq!(result.file(), &file_url("utils.R")); - assert_eq!(result.name(), "x"); - assert_eq!(result.range(), range(10, 11)); -} - -// --- file_layers --- - -fn index_source(source: &str) -> oak_semantic::semantic_index::SemanticIndex { - let parsed = parse(source, RParserOptions::default()); - build_index(&parsed.tree(), NoopImportsResolver) -} - -#[test] -fn test_file_layers_exports_only() { - let index = index_source("x <- 1\ny <- 2"); - let layers = file_layers(file_url("foo.R"), &index); - - assert_eq!(layers.len(), 1); - assert_matches!(&layers[0], ScopeLayer::FileExports { file, exports } => { - assert_eq!(file, &file_url("foo.R")); - assert!(exports.contains_key("x")); - assert!(exports.contains_key("y")); - assert_eq!(exports.len(), 2); - }); -} - -#[test] -fn test_file_layers_with_library_directives() { - let index = index_source("library(dplyr)\nlibrary(tidyr)\nx <- 1"); - let layers = file_layers(file_url("script.R"), &index); - - // FileExports + 2 PackageExports - assert_eq!(layers.len(), 3); - - assert_matches!(&layers[0], ScopeLayer::FileExports { exports, .. } => { - assert_eq!(exports.len(), 1); - assert!(exports.contains_key("x")); - }); - assert_matches!(&layers[1], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "dplyr"); - }); - assert_matches!(&layers[2], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "tidyr"); - }); -} - -#[test] -fn test_file_layers_last_def_wins() { - // "x <- 1\nx <- 2" has two definitions of x; the map should keep the last - let index = index_source("x <- 1\nx <- 2"); - let layers = file_layers(file_url("foo.R"), &index); - - assert_matches!(&layers[0], ScopeLayer::FileExports { exports, .. } => { - assert_eq!(exports.len(), 1); - let range = exports.get("x").unwrap(); - assert_eq!(range.start(), TextSize::from(7)); - }); -} - -#[test] -fn test_file_layers_empty_file() { - let index = index_source(""); - let layers = file_layers(file_url("empty.R"), &index); - - assert_eq!(layers.len(), 1); - assert_matches!(&layers[0], ScopeLayer::FileExports { exports, .. } => { - assert!(exports.is_empty()); - }); -} - -#[test] -fn test_file_layers_source_directive_skipped() { - let index = index_source("library(dplyr)\nsource(\"helpers.R\")\nx <- 1"); - let layers = file_layers(file_url("script.R"), &index); - - // FileExports + PackageExports(dplyr), source() is not emitted as a layer - assert_eq!(layers.len(), 2); - assert_matches!(&layers[0], ScopeLayer::FileExports { exports, .. } => { - assert_eq!(exports.len(), 1); - assert!(exports.contains_key("x")); - }); - assert_matches!(&layers[1], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "dplyr"); - }); -} - -// --- Integration: file_layers -> resolve_external_name --- - -#[test] -fn test_file_layers_resolve_roundtrip() { - let library = test_library(vec![TestPackage::new( - "dplyr", - "dplyr.R", - vec!["filter", "mutate"], - vec![], - )]); - - let index = index_source("library(dplyr)\nmy_helper <- function() NULL"); - let layers = file_layers(file_url("script.R"), &index); - - // Resolve a file export - let result = resolve_external_name(&library, &layers, "my_helper").unwrap(); - assert_eq!(result.file(), &file_url("script.R")); - - // Resolve a package export - let result = resolve_external_name(&library, &layers, "filter").unwrap(); - assert!(result.file().path().ends_with("dplyr.R")); - assert_eq!(result.name(), "filter"); - - // Miss - let result = resolve_external_name(&library, &layers, "unknown"); - assert_eq!(result, None); -} - -#[test] -fn test_chained_scope_predecessor_files() { - let library = test_library(vec![TestPackage::new( - "ggplot2", - "ggplot2.R", - vec!["aes", "ggplot"], - vec![], - )]); - - // Simulate two predecessor files, then root layers - let index_a = index_source("helper_a <- 1"); - let index_b = index_source("library(ggplot2)\nhelper_b <- 2"); - - let mut scope = Vec::new(); - - // Later files come first (they shadow earlier ones) - scope.extend(file_layers(file_url("b.R"), &index_b)); - scope.extend(file_layers(file_url("a.R"), &index_a)); - - // Resolve from predecessor file b - // Predecessor file export from b.R - let result = resolve_external_name(&library, &scope, "helper_b").unwrap(); - assert_eq!(result.file(), &file_url("b.R")); - - // Resolve from predecessor file a - let result = resolve_external_name(&library, &scope, "helper_a").unwrap(); - assert_eq!(result.file(), &file_url("a.R")); - - // Resolve from ggplot2 (attached by b.R) - let result = resolve_external_name(&library, &scope, "ggplot").unwrap(); - assert!(result.file().path().ends_with("ggplot2.R")); - assert_eq!(result.name(), "ggplot"); -} - -// --- root_layers --- - -#[test] -fn test_root_layers_from_namespace_imports() { - let ns = Namespace { - package_imports: vec!["rlang".to_string(), "cli".to_string()], - ..Default::default() - }; - let layers = package_root_layers(&ns); - assert_eq!(layers.len(), 3); - assert_matches!(&layers[0], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "rlang"); - }); - assert_matches!(&layers[1], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "cli"); - }); - assert_matches!(&layers[2], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "base"); - }); -} - -#[test] -fn test_root_layers_empty_namespace() { - let ns = Namespace::default(); - let layers = package_root_layers(&ns); - assert_eq!(layers.len(), 1); - assert_matches!(&layers[0], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "base"); - }); -} - -#[test] -fn test_root_layers_includes_importfrom() { - let ns = Namespace { - imports: vec![ - Import { - name: "median".to_string(), - package: "stats".to_string(), - }, - Import { - name: "head".to_string(), - package: "utils".to_string(), - }, - ], - ..Default::default() - }; - let layers = package_root_layers(&ns); - assert_eq!(layers.len(), 2); - assert_matches!(&layers[0], ScopeLayer::PackageImports(map) => { - assert_eq!(map.get("median").unwrap(), "stats"); - assert_eq!(map.get("head").unwrap(), "utils"); - }); - assert_matches!(&layers[1], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "base"); - }); -} - -#[test] -fn test_root_layers_importfrom_before_package_exports() { - let ns = Namespace { - imports: vec![Import { - name: "filter".to_string(), - package: "stats".to_string(), - }], - package_imports: vec!["dplyr".to_string()], - ..Default::default() - }; - let layers = package_root_layers(&ns); - assert_eq!(layers.len(), 3); - assert_matches!(&layers[0], ScopeLayer::PackageImports(_)); - assert_matches!(&layers[1], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "dplyr"); - }); - assert_matches!(&layers[2], ScopeLayer::PackageExports(pkg) => { - assert_eq!(pkg, "base"); - }); -} - -// --- scope chain assembly --- - -#[test] -fn test_scope_chain_combines_predecessors_and_root() { - let library = test_library(vec![ - TestPackage::new("rlang", "rlang.R", vec!["sym", "expr"], vec![]), - TestPackage::new("dplyr", "dplyr.R", vec!["filter", "mutate"], vec![]), - ]); - - let index_a = index_source("helper_a <- 1"); - let index_b = index_source("library(dplyr)\nhelper_b <- 2"); - - let ns = Namespace { - package_imports: vec!["rlang".to_string()], - ..Default::default() - }; - - let mut scope = Vec::new(); - scope.extend(file_layers(file_url("b.R"), &index_b)); - scope.extend(file_layers(file_url("a.R"), &index_a)); - scope.extend(package_root_layers(&ns)); - - // Predecessor file export - let result = resolve_external_name(&library, &scope, "helper_b").unwrap(); - assert_eq!(result.file(), &file_url("b.R")); - assert_eq!(result.name(), "helper_b"); - assert_eq!(result.range(), range(15, 23)); - - // Predecessor library() directive - let result = resolve_external_name(&library, &scope, "filter").unwrap(); - assert!(result.file().path().ends_with("dplyr.R")); - assert_eq!(result.name(), "filter"); - - // Root layer (NAMESPACE import) - let result = resolve_external_name(&library, &scope, "sym").unwrap(); - assert!(result.file().path().ends_with("rlang.R")); - assert_eq!(result.name(), "sym"); - - // Miss - assert_eq!(resolve_external_name(&library, &scope, "unknown"), None); -} - -#[test] -fn test_scope_chain_predecessors_shadow_root() { - let library = test_library(vec![TestPackage::new( - "rlang", - "rlang.R", - vec!["expr"], - vec![], - )]); - - let index = index_source("expr <- function() NULL"); - - let ns = Namespace { - package_imports: vec!["rlang".to_string()], - ..Default::default() - }; - - let mut scope = Vec::new(); - scope.extend(file_layers(file_url("utils.R"), &index)); - scope.extend(package_root_layers(&ns)); - - // File export shadows the rlang root layer - let result = resolve_external_name(&library, &scope, "expr").unwrap(); - assert_eq!(result.file(), &file_url("utils.R")); -} - -#[test] -fn test_scope_chain_later_predecessor_shadows_earlier() { - // b.R is loaded after a.R, so b.R's layers come first in the scope - // and its `helper` should shadow a.R's `helper`. - let index_a = index_source("helper <- 1"); - let index_b = index_source("helper <- 2"); - - let mut scope = Vec::new(); - scope.extend(file_layers(file_url("b.R"), &index_b)); - scope.extend(file_layers(file_url("a.R"), &index_a)); - - let result = resolve_external_name(&empty_library(), &scope, "helper").unwrap(); - assert_eq!(result.file(), &file_url("b.R")); -}