diff --git a/lsp/package.json b/lsp/package.json index 154fd77a85..877f84ad33 100644 --- a/lsp/package.json +++ b/lsp/package.json @@ -88,6 +88,11 @@ "title": "Run Test", "category": "pyrefly", "command": "pyrefly.runTest" + }, + { + "title": "Find File Usages", + "category": "pyrefly", + "command": "pyrefly.findFileUsages" } ], "semanticTokenScopes": [ diff --git a/lsp/src/extension.ts b/lsp/src/extension.ts index be948979ac..b3456861a4 100644 --- a/lsp/src/extension.ts +++ b/lsp/src/extension.ts @@ -285,6 +285,7 @@ export async function activate( ...((rawInitialisationOptions as any).pyrefly ?? {}), typeErrorDisplayStatusVersion: TYPE_ERROR_DISPLAY_STATUS_VERSION, customHoverProvider: supportsHoverVerbosity, + findFileUsages: true, pushTypeErrorDisplayStatus: true, }, }; @@ -510,6 +511,63 @@ export async function activate( ); registerCodeLensCommands(context, pythonEnv); + context.subscriptions.push( + vscode.commands.registerCommand( + 'pyrefly.findFileUsages', + async (...args: unknown[]) => { + const payload = args[0] as + | { + uri?: string; + locations?: Array<{ + uri: string; + range: { + start: {line: number; character: number}; + end: {line: number; character: number}; + }; + }>; + } + | undefined; + if (payload == null || typeof payload.uri !== 'string') { + return; + } + if (!Array.isArray(payload.locations)) { + return; + } + + const targetUri = vscode.Uri.parse(payload.uri); + const locations = payload.locations + .map(location => { + if ( + location == null || + typeof location.uri !== 'string' || + location.range == null + ) { + return null; + } + const range = new vscode.Range( + location.range.start.line, + location.range.start.character, + location.range.end.line, + location.range.end.character, + ); + return new vscode.Location(vscode.Uri.parse(location.uri), range); + }) + .filter( + (location): location is vscode.Location => location != null, + ); + if (locations.length === 0) { + return; + } + await vscode.commands.executeCommand( + 'editor.action.showReferences', + targetUri, + new vscode.Position(0, 0), + locations, + ); + }, + ), + ); + // When our extension is activated, make sure ms-python knows // TODO(kylei): remove this hack once ms-python has this behavior await triggerMsPythonRefreshLanguageServersIfInstalled(); diff --git a/pyrefly/lib/lsp/non_wasm/safe_delete_file.rs b/pyrefly/lib/lsp/non_wasm/safe_delete_file.rs index c70cbd412d..cedb914ad0 100644 --- a/pyrefly/lib/lsp/non_wasm/safe_delete_file.rs +++ b/pyrefly/lib/lsp/non_wasm/safe_delete_file.rs @@ -5,21 +5,40 @@ * LICENSE file in the root directory of this source tree. */ +use std::collections::HashSet; +use std::path::Path; +use std::sync::Arc; + use lsp_types::ClientCapabilities; use lsp_types::CodeAction; use lsp_types::CodeActionKind; use lsp_types::CodeActionResponse; +use lsp_types::Command; use lsp_types::DeleteFile; use lsp_types::DeleteFileOptions; use lsp_types::DocumentChange; +use lsp_types::Location; use lsp_types::ResourceOperationKind; use lsp_types::Uri; use lsp_types::WorkspaceEdit; +use pyrefly_build::handle::Handle; use pyrefly_python::PYTHON_EXTENSIONS; +use pyrefly_python::ast::Ast; +use pyrefly_python::module::TextRangeWithModule; use pyrefly_python::module_name::ModuleName; use pyrefly_python::module_path::ModulePath; +use pyrefly_python::module_path::ModulePathDetails; +use ruff_python_ast::Stmt; +use ruff_python_ast::StmtImport; +use ruff_python_ast::StmtImportFrom; +use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::visitor::walk_stmt; +use ruff_text_size::Ranged; +use ruff_text_size::TextRange; +use serde_json::json; use crate::lsp::non_wasm::module_helpers::handle_from_module_path; +use crate::state::lsp::FindPreference; use crate::state::state::State; use crate::state::state::Transaction; @@ -46,13 +65,16 @@ fn supports_workspace_edit_resource_ops( .all(|kind| supported.is_some_and(|ops| ops.contains(kind))) } -/// Builds a safe-delete refactor action for a file that nothing imports. -pub(crate) fn safe_delete_file_code_action( +/// Builds safe-delete refactor actions for a file. +pub(crate) fn safe_delete_file_code_actions( capabilities: &ClientCapabilities, + supports_find_usages: bool, state: &State, transaction: &Transaction<'_>, uri: &Uri, -) -> Option { + get_open_handle: impl Fn(&Path) -> Option, + to_lsp_location: impl Fn(&TextRangeWithModule) -> Option, +) -> Option> { if !supports_workspace_edit_document_changes(capabilities) { return None; } @@ -75,9 +97,29 @@ pub(crate) fn safe_delete_file_code_action( if module_name == ModuleName::unknown() { return None; } - if transaction.is_depended_on_by_anything(&handle) { - return None; + let rdeps = transaction.get_rdeps(&handle); + if rdeps.is_empty() { + return Some(vec![delete_file_code_action( + format!("Safe delete file `{file_name}`"), + uri, + )]); + } + let mut actions = Vec::new(); + if supports_find_usages { + let usages = collect_import_usages(transaction, rdeps, module_name, get_open_handle); + let locations: Vec<_> = usages.iter().filter_map(to_lsp_location).collect(); + if !locations.is_empty() { + actions.push(find_usages_code_action(&file_name, uri, locations)); + } } + actions.push(delete_file_code_action( + format!("Delete file `{file_name}` anyway"), + uri, + )); + Some(actions) +} + +fn delete_file_code_action(title: String, uri: &Uri) -> CodeActionResponse { let operation = DocumentChange::DeleteFile(DeleteFile { uri: uri.clone(), options: Some(DeleteFileOptions { @@ -86,13 +128,214 @@ pub(crate) fn safe_delete_file_code_action( }), annotation_id: None, }); - Some(CodeActionResponse::CodeAction(CodeAction { - title: format!("Safe delete file `{file_name}`"), + CodeActionResponse::CodeAction(CodeAction { + title, kind: Some(CodeActionKind::Custom("refactor.delete".into())), edit: Some(WorkspaceEdit { document_changes: Some(vec![operation]), ..Default::default() }), ..Default::default() - })) + }) +} + +fn find_usages_code_action( + file_name: &str, + uri: &Uri, + locations: Vec, +) -> CodeActionResponse { + let title = format!("Find usages of file `{file_name}`"); + CodeActionResponse::CodeAction(CodeAction { + title: title.clone(), + kind: Some(CodeActionKind::Custom("refactor.delete".into())), + command: Some(Command { + title, + tooltip: None, + command: "pyrefly.findFileUsages".to_owned(), + arguments: Some(vec![json!({ + "uri": uri, + "locations": locations, + })]), + }), + ..Default::default() + }) +} + +fn collect_import_usages( + transaction: &Transaction<'_>, + rdeps: HashSet, + target_module: ModuleName, + get_open_handle: impl Fn(&Path) -> Option, +) -> Vec { + let mut seen = HashSet::new(); + let mut usages = Vec::new(); + for rdep_handle in rdeps { + // An open buffer can remove its dependency while the indexed disk file retains it. + let rdep_handle = get_open_handle(rdep_handle.path().as_path()).unwrap_or(rdep_handle); + if !seen.insert(rdep_handle.path().as_path().to_owned()) { + continue; + } + let Some(module_info) = transaction.get_module_info(&rdep_handle) else { + continue; + }; + if !matches!( + module_info.path().details(), + ModulePathDetails::FileSystem(_) | ModulePathDetails::Memory(_) + ) { + continue; + } + // Only indexed closed files without a retained AST need parsing. + let ast = transaction.get_ast(&rdep_handle).unwrap_or_else(|| { + Arc::new(Ast::parse(module_info.contents(), module_info.source_type()).0) + }); + let mut visitor = ImportUsageVisitor { + transaction, + importer: &rdep_handle, + target_module, + current_module: module_info.name(), + is_init: module_info.path().is_init(), + ranges: Vec::new(), + }; + for stmt in &ast.body { + visitor.visit_stmt(stmt); + } + if visitor.ranges.is_empty() { + continue; + } + for range in visitor.ranges { + usages.push(TextRangeWithModule::new(module_info.clone(), range)); + } + } + usages +} + +struct ImportUsageVisitor<'a> { + transaction: &'a Transaction<'a>, + importer: &'a Handle, + target_module: ModuleName, + current_module: ModuleName, + is_init: bool, + ranges: Vec, +} + +impl ImportUsageVisitor<'_> { + fn record_import(&mut self, imported_module: ModuleName, range: TextRange) -> bool { + if module_matches_target(imported_module, self.target_module) { + self.ranges.push(range); + return true; + } + false + } + + fn visit_import(&mut self, import: &StmtImport) { + for alias in &import.names { + let imported_module = ModuleName::from_name(&alias.name.id); + if self.record_import(imported_module, import.range()) { + return; + } + } + } + + fn visit_import_from(&mut self, import_from: &StmtImportFrom) { + let base = self.current_module.new_maybe_relative( + self.is_init, + import_from.level, + import_from.module.as_ref().map(|id| &id.id), + ); + let Some(base) = base else { + return; + }; + if self.record_import(base, import_from.range()) { + return; + } + for alias in &import_from.names { + let refers_to_target = self + .transaction + .resolve_named_import( + self.importer, + base, + alias.name.id.clone(), + FindPreference::default(), + ) + .is_some_and(|(handle, _, _)| { + module_matches_target(handle.module(), self.target_module) + }); + if refers_to_target { + self.ranges.push(import_from.range()); + return; + } + } + } +} + +impl Visitor<'_> for ImportUsageVisitor<'_> { + fn visit_stmt(&mut self, stmt: &Stmt) { + match stmt { + Stmt::Import(import) => self.visit_import(import), + Stmt::ImportFrom(import_from) => self.visit_import_from(import_from), + _ => walk_stmt(self, stmt), + } + } +} + +fn module_matches_target(module: ModuleName, target: ModuleName) -> bool { + if module == target { + return true; + } + let module_str = module.as_str(); + let target_str = target.as_str(); + module_str.starts_with(target_str) + && module_str.len() > target_str.len() + && module_str.as_bytes().get(target_str.len()) == Some(&b'.') +} + +#[cfg(test)] +mod tests { + use std::fs; + + use lsp_types::ClientCapabilities; + use lsp_types::CodeActionResponse; + use lsp_types::Uri; + use serde_json::json; + use tempfile::tempdir; + + use super::safe_delete_file_code_actions; + use crate::test::util::TestEnv; + + #[test] + fn test_safe_delete_file_skips_locations_for_unsupported_clients() { + let root = tempdir().unwrap(); + let target_path = root.path().join("target.py"); + let consumer_path = root.path().join("consumer.py"); + fs::write(&target_path, "value = 1\n").unwrap(); + fs::write(&consumer_path, "import target\n").unwrap(); + let mut env = TestEnv::new(); + env.add_real_path("target", target_path.clone()); + env.add_real_path("consumer", consumer_path); + let (state, handle) = env.to_state(); + let transaction = state.transaction(); + assert!(!transaction.get_rdeps(&handle("target")).is_empty()); + let capabilities: ClientCapabilities = serde_json::from_value(json!({ + "workspace": { + "workspaceEdit": { + "documentChanges": true, + "resourceOperations": ["delete"] + } + } + })) + .unwrap(); + let actions = safe_delete_file_code_actions( + &capabilities, + false, + &state, + &transaction, + &Uri::from_file_path(target_path).unwrap(), + |_| panic!("unsupported clients must not inspect importer buffers"), + |_| panic!("unsupported clients must not convert locations"), + ) + .unwrap(); + assert_eq!(actions.len(), 1); + assert!(matches!(&actions[0], CodeActionResponse::CodeAction(action) + if action.title == "Delete file `target.py` anyway")); + } } diff --git a/pyrefly/lib/lsp/non_wasm/server.rs b/pyrefly/lib/lsp/non_wasm/server.rs index 934958e5fc..0708c609ba 100644 --- a/pyrefly/lib/lsp/non_wasm/server.rs +++ b/pyrefly/lib/lsp/non_wasm/server.rs @@ -324,7 +324,7 @@ use crate::lsp::non_wasm::queue::HeavyTaskQueue; use crate::lsp::non_wasm::queue::LspEvent; use crate::lsp::non_wasm::queue::LspQueue; use crate::lsp::non_wasm::queue::QueuedEvent; -use crate::lsp::non_wasm::safe_delete_file::safe_delete_file_code_action; +use crate::lsp::non_wasm::safe_delete_file::safe_delete_file_code_actions; use crate::lsp::non_wasm::stdlib::should_show_stdlib_error; use crate::lsp::non_wasm::transaction_manager::TransactionManager; use crate::lsp::non_wasm::type_error_display_status::BuildSystemStatus; @@ -5480,13 +5480,29 @@ impl Server { } record_code_action_telemetry("move_symbol_new_file", start); let start = Instant::now(); - if let Some(action) = safe_delete_file_code_action( + let supports_find_usages = self + .initialize_params + .initialization_options + .as_ref() + .and_then(|options| options.get("pyrefly")) + .and_then(|options| options.get("findFileUsages")) + .and_then(Value::as_bool) + .unwrap_or(false); + if let Some(mut file_actions) = safe_delete_file_code_actions( &self.initialize_params.capabilities, + supports_find_usages, &self.state, transaction, uri, + |path| { + self.open_files + .read() + .contains_key(path) + .then(|| make_open_handle(&self.state, path)) + }, + |location| self.to_lsp_location(location), ) { - actions.push(action); + actions.append(&mut file_actions); } record_code_action_telemetry("safe_delete_file", start); } diff --git a/pyrefly/lib/state/lsp.rs b/pyrefly/lib/state/lsp.rs index 1da1f71bfd..822fe0e7d7 100644 --- a/pyrefly/lib/state/lsp.rs +++ b/pyrefly/lib/state/lsp.rs @@ -1734,7 +1734,8 @@ impl<'a> Transaction<'a> { } } - fn resolve_named_import( + /// Resolve a named import through re-exports and package submodules. + pub(crate) fn resolve_named_import( &self, handle: &Handle, module_name: ModuleName, diff --git a/pyrefly/lib/state/state.rs b/pyrefly/lib/state/state.rs index 96ffed10cd..d2f83691f1 100644 --- a/pyrefly/lib/state/state.rs +++ b/pyrefly/lib/state/state.rs @@ -1113,7 +1113,7 @@ impl<'a> Transaction<'a> { self.get_load(handle).map(|x| x.module_info.dupe()) } - /// Whether any other module depends on this one. + /// Return the modules that directly depend on this one. /// /// A dependency edge means the depending module's result was computed from /// this file's contents, which is exactly the question safe deletion asks, @@ -1128,13 +1128,15 @@ impl<'a> Transaction<'a> { /// filesystem handle: as with `get_transitive_rdeps`, the rdeps of an /// in-memory handle contain only itself, which would make any file an IDE /// has open look unused. - pub fn is_depended_on_by_anything(&self, handle: &Handle) -> bool { + pub fn get_rdeps(&self, handle: &Handle) -> HashSet { let path = handle.path().as_path(); self.get_module(handle) .rdeps .lock() .iter() - .any(|rdep| rdep.path().as_path() != path) + .filter(|rdep| rdep.path().as_path() != path) + .cloned() + .collect() } /// Compute transitive dependency closure for the given handle. diff --git a/pyrefly/lib/test/lsp/lsp_interaction/safe_delete_file.rs b/pyrefly/lib/test/lsp/lsp_interaction/safe_delete_file.rs index 451e03165a..518839c902 100644 --- a/pyrefly/lib/test/lsp/lsp_interaction/safe_delete_file.rs +++ b/pyrefly/lib/test/lsp/lsp_interaction/safe_delete_file.rs @@ -5,19 +5,31 @@ * LICENSE file in the root directory of this source tree. */ +use std::fs; + +use lsp_types::CodeActionKind; use lsp_types::CodeActionRequest; use lsp_types::CodeActionResponse; use lsp_types::DocumentChange; +use lsp_types::Location; use lsp_types::Uri; +use pyrefly_lsp_test::IndexingMode; use pyrefly_lsp_test::object_model::InitializeSettings; use pyrefly_lsp_test::object_model::LspInteraction; +use pyrefly_lsp_test::object_model::LspInteractionArgs; use serde_json::json; use crate::test::lsp::lsp_interaction::util::get_test_files_root; -fn init_with_delete_support(root_path: &std::path::Path) -> (LspInteraction, Uri) { +fn init_with_delete_support( + root_path: &std::path::Path, + supports_find_usages: bool, + indexing_mode: IndexingMode, +) -> (LspInteraction, Uri) { let scope_uri = Uri::from_file_path(root_path).unwrap(); - let mut interaction = LspInteraction::new(); + let mut args = LspInteractionArgs::default(); + args.args.indexing_mode = indexing_mode; + let mut interaction = LspInteraction::new_with_args(args); interaction.set_root(root_path.to_path_buf()); interaction .initialize(InitializeSettings { @@ -30,22 +42,49 @@ fn init_with_delete_support(root_path: &std::path::Path) -> (LspInteraction, Uri } } })), + initialization_options: supports_find_usages + .then(|| json!({"pyrefly": {"findFileUsages": true}})), ..Default::default() }) .unwrap(); (interaction, scope_uri) } +fn find_usage_locations( + actions: &[CodeActionResponse], + file_name: &str, + target_uri: &Uri, +) -> Option> { + let title = format!("Find usages of file `{file_name}`"); + let code_action = actions.iter().find_map(|action| { + let CodeActionResponse::CodeAction(code_action) = action else { + return None; + }; + (code_action.title == title).then_some(code_action) + })?; + if code_action.kind.as_ref()? != &CodeActionKind::new("refactor.delete") { + return None; + } + let command = code_action.command.as_ref()?; + if command.command != "pyrefly.findFileUsages" { + return None; + } + let payload = command.arguments.as_ref()?.first()?.as_object()?; + if payload.get("uri")? != &json!(target_uri) { + return None; + } + serde_json::from_value(payload.get("locations")?.clone()).ok() +} + #[test] fn test_safe_delete_file_unused() { let root = get_test_files_root(); let root_path = root.path().join("safe_delete_file"); - let (interaction, _scope_uri) = init_with_delete_support(&root_path); + let (interaction, _scope_uri) = init_with_delete_support(&root_path, true, IndexingMode::None); let file = "unused.py"; let file_path = root_path.join(file); let uri = Uri::from_file_path(&file_path).unwrap(); - interaction.client.did_open(file); interaction @@ -90,17 +129,93 @@ fn test_safe_delete_file_unused() { } #[test] -fn test_safe_delete_file_rejects_usages() { +fn test_safe_delete_file_finds_usage_in_indexed_closed_file() { let root = get_test_files_root(); let root_path = root.path().join("safe_delete_file"); - let (interaction, _scope_uri) = init_with_delete_support(&root_path); + let (interaction, _scope_uri) = + init_with_delete_support(&root_path, true, IndexingMode::LazyBlocking); let file = "target.py"; let file_path = root_path.join(file); let uri = Uri::from_file_path(&file_path).unwrap(); + let consumer_uri = Uri::from_file_path(root_path.join("consumer.py")).unwrap(); interaction.client.did_open(file); + + interaction + .client + .send_request::(json!({ + "textDocument": { "uri": uri }, + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } + }, + "context": { "diagnostics": [] } + })) + .expect_response_with(|response: Option>| { + let Some(actions) = response else { + return false; + }; + let locations = find_usage_locations(&actions, file, &uri).unwrap_or_default(); + let has_consumer = locations + .iter() + .any(|location| location.uri == consumer_uri); + let has_delete = actions.iter().any(|action| { + matches!( + action, + CodeActionResponse::CodeAction(action) + if action.title == "Delete file `target.py` anyway" + ) + }); + let has_safe = actions.iter().any(|action| { + matches!( + action, + CodeActionResponse::CodeAction(action) + if action.title == "Safe delete file `target.py`" + ) + }); + has_consumer && has_delete && !has_safe + }) + .unwrap(); + + interaction.shutdown().unwrap(); +} + +#[test] +fn test_safe_delete_file_prefers_open_importer() { + let root = get_test_files_root(); + let root_path = root.path().join("safe_delete_file"); + let (interaction, _scope_uri) = + init_with_delete_support(&root_path, true, IndexingMode::LazyBlocking); + + let file = "target.py"; + let uri = Uri::from_file_path(root_path.join(file)).unwrap(); + let consumer_uri = Uri::from_file_path(root_path.join("consumer.py")).unwrap(); + interaction.client.did_open(file); + // Complete indexing before opening a buffer for the indexed importer. + interaction + .client + .send_request::(json!({ + "textDocument": { "uri": uri }, + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } + }, + "context": { "diagnostics": [] } + })) + .expect_response_with(|response: Option>| { + let actions = response.expect("expected file actions after indexing"); + let locations = find_usage_locations(&actions, file, &uri) + .expect("expected usages from the indexed importer"); + assert_eq!(locations.len(), 1); + assert_eq!(locations[0].uri, consumer_uri); + true + }) + .unwrap(); interaction.client.did_open("consumer.py"); + interaction + .client + .did_change("consumer.py", "\n\nimport target\n"); interaction .client @@ -114,13 +229,208 @@ fn test_safe_delete_file_rejects_usages() { })) .expect_response_with(|response: Option>| { let Some(actions) = response else { - return true; + return false; }; - actions.iter().all(|action| { - let CodeActionResponse::CodeAction(code_action) = action else { - return true; - }; - code_action.title != "Safe delete file `target.py`" + let locations = find_usage_locations(&actions, file, &uri) + .expect("expected usages from the open importer"); + assert_eq!(locations.len(), 1); + assert_eq!(locations[0].uri, consumer_uri); + assert_eq!(locations[0].range.start.line, 2); + true + }) + .unwrap(); + + interaction + .client + .did_change("consumer.py", "answer = 123\n"); + interaction + .client + .send_request::(json!({ + "textDocument": { "uri": uri }, + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } + }, + "context": { "diagnostics": [] } + })) + .expect_response_with(|response: Option>| { + let actions = + response.expect("expected a delete-anyway action for indexed dependencies"); + assert!(find_usage_locations(&actions, file, &uri).is_none()); + assert!(actions.iter().any(|action| { + matches!(action, CodeActionResponse::CodeAction(action) + if action.title == "Delete file `target.py` anyway") + })); + assert!(actions.iter().all(|action| { + !matches!(action, CodeActionResponse::CodeAction(action) + if action.title == "Safe delete file `target.py`") + })); + true + }) + .unwrap(); + + interaction.shutdown().unwrap(); +} + +#[test] +fn test_safe_delete_file_finds_package_reexports() { + for chained in [false, true] { + let root = get_test_files_root(); + let root_path = root.path().join("safe_delete_file_relative"); + fs::write(root_path.join("pkg/__init__.py"), "from . import target\n").unwrap(); + let consumer_source = if chained { + fs::write( + root_path.join("bridge.py"), + "from pkg import target as exported\n", + ) + .unwrap(); + "from bridge import exported as target\ntarget.func()\n" + } else { + "from pkg import target\ntarget.func()\n" + }; + fs::write(root_path.join("consumer.py"), consumer_source).unwrap(); + let (interaction, _scope_uri) = + init_with_delete_support(&root_path, true, IndexingMode::LazyBlocking); + let uri = Uri::from_file_path(root_path.join("pkg/target.py")).unwrap(); + let package_uri = Uri::from_file_path(root_path.join("pkg/__init__.py")).unwrap(); + let consumer_uri = Uri::from_file_path(root_path.join("consumer.py")).unwrap(); + interaction.client.did_open("pkg/target.py"); + interaction + .client + .send_request::(json!({ + "textDocument": { "uri": uri }, + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } + }, + "context": { "diagnostics": [] } + })) + .expect_response_with(|response: Option>| { + let actions = response.expect("expected actions for a re-exported submodule"); + let locations = find_usage_locations(&actions, "target.py", &uri) + .expect("expected references through package re-exports"); + assert!(locations.iter().any(|location| location.uri == package_uri)); + assert!( + locations + .iter() + .any(|location| location.uri == consumer_uri) + ); + assert!(actions.iter().any(|action| { + matches!(action, CodeActionResponse::CodeAction(action) + if action.title == "Delete file `target.py` anyway") + })); + true + }) + .unwrap(); + interaction.shutdown().unwrap(); + } +} + +#[test] +fn test_safe_delete_file_negotiates_find_usages_command() { + let root = get_test_files_root(); + let root_path = root.path().join("safe_delete_file"); + let (interaction, _scope_uri) = init_with_delete_support(&root_path, false, IndexingMode::None); + + let file = "target.py"; + let uri = Uri::from_file_path(root_path.join(file)).unwrap(); + interaction.client.did_open(file); + interaction.client.did_open("consumer.py"); + + interaction + .client + .send_request::(json!({ + "textDocument": { "uri": uri }, + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } + }, + "context": { "diagnostics": [] } + })) + .expect_response_with(|response: Option>| { + let Some(actions) = response else { + return false; + }; + let titles = actions.iter().filter_map(|action| match action { + CodeActionResponse::CodeAction(action) => Some(action.title.as_str()), + CodeActionResponse::Command(_) => None, + }); + let titles = titles.collect::>(); + titles.contains(&"Delete file `target.py` anyway") + && !titles.contains(&"Find usages of file `target.py`") + }) + .unwrap(); + + interaction.shutdown().unwrap(); +} + +#[test] +fn test_safe_delete_file_maps_notebook_usage_to_cell() { + let root = get_test_files_root(); + let root_path = root.path().join("safe_delete_file"); + let (interaction, _scope_uri) = init_with_delete_support(&root_path, true, IndexingMode::None); + + let file = "target.py"; + let uri = Uri::from_file_path(root_path.join(file)).unwrap(); + interaction.client.did_open(file); + interaction.open_notebook("consumer.ipynb", vec!["import target"]); + let cell_uri = interaction.cell_uri("consumer.ipynb", "cell1"); + + interaction + .client + .send_request::(json!({ + "textDocument": { "uri": uri }, + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } + }, + "context": { "diagnostics": [] } + })) + .expect_response_with(|response: Option>| { + let Some(actions) = response else { + return false; + }; + find_usage_locations(&actions, file, &uri) + .is_some_and(|locations| locations.iter().any(|location| location.uri == cell_uri)) + }) + .unwrap(); + + interaction.shutdown().unwrap(); +} + +#[test] +fn test_safe_delete_file_does_not_report_attribute_import_as_submodule() { + let root = get_test_files_root(); + let root_path = root.path().join("safe_delete_file_attribute_import"); + let (interaction, _scope_uri) = init_with_delete_support(&root_path, true, IndexingMode::None); + + let file = "package/target.py"; + let uri = Uri::from_file_path(root_path.join(file)).unwrap(); + let package_uri = Uri::from_file_path(root_path.join("package/__init__.py")).unwrap(); + let consumer_uri = Uri::from_file_path(root_path.join("consumer.py")).unwrap(); + interaction.client.did_open(file); + interaction.client.did_open("package/__init__.py"); + interaction.client.did_open("consumer.py"); + + interaction + .client + .send_request::(json!({ + "textDocument": { "uri": uri }, + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } + }, + "context": { "diagnostics": [] } + })) + .expect_response_with(|response: Option>| { + let Some(actions) = response else { + return false; + }; + find_usage_locations(&actions, "target.py", &uri).is_some_and(|locations| { + locations.iter().any(|location| location.uri == package_uri) + && locations + .iter() + .all(|location| location.uri != consumer_uri) }) }) .unwrap(); @@ -132,7 +442,7 @@ fn test_safe_delete_file_rejects_usages() { fn test_safe_delete_file_rejects_from_import() { let root = get_test_files_root(); let root_path = root.path().join("safe_delete_file_from_import"); - let (interaction, _scope_uri) = init_with_delete_support(&root_path); + let (interaction, _scope_uri) = init_with_delete_support(&root_path, true, IndexingMode::None); let file = "target.py"; let file_path = root_path.join(file); @@ -153,14 +463,23 @@ fn test_safe_delete_file_rejects_from_import() { })) .expect_response_with(|response| { let Some(actions) = response else { - return true; + return false; }; - actions.iter().all(|action| { + let mut saw_find = false; + let mut saw_delete = false; + let mut saw_safe = false; + for action in actions { let CodeActionResponse::CodeAction(code_action) = action else { - return true; + continue; }; - code_action.title != "Safe delete file `target.py`" - }) + match code_action.title.as_str() { + "Find usages of file `target.py`" => saw_find = true, + "Delete file `target.py` anyway" => saw_delete = true, + "Safe delete file `target.py`" => saw_safe = true, + _ => {} + } + } + saw_find && saw_delete && !saw_safe }) .unwrap(); @@ -171,7 +490,7 @@ fn test_safe_delete_file_rejects_from_import() { fn test_safe_delete_file_from_import_unused() { let root = get_test_files_root(); let root_path = root.path().join("safe_delete_file_from_import"); - let (interaction, _scope_uri) = init_with_delete_support(&root_path); + let (interaction, _scope_uri) = init_with_delete_support(&root_path, true, IndexingMode::None); let file = "unused.py"; let file_path = root_path.join(file); @@ -224,7 +543,7 @@ fn test_safe_delete_file_from_import_unused() { fn test_safe_delete_file_rejects_relative_import() { let root = get_test_files_root(); let root_path = root.path().join("safe_delete_file_relative"); - let (interaction, _scope_uri) = init_with_delete_support(&root_path); + let (interaction, _scope_uri) = init_with_delete_support(&root_path, true, IndexingMode::None); let file = "pkg/target.py"; let file_path = root_path.join(file); @@ -245,14 +564,23 @@ fn test_safe_delete_file_rejects_relative_import() { })) .expect_response_with(|response| { let Some(actions) = response else { - return true; + return false; }; - actions.iter().all(|action| { + let mut saw_find = false; + let mut saw_delete = false; + let mut saw_safe = false; + for action in actions { let CodeActionResponse::CodeAction(code_action) = action else { - return true; + continue; }; - code_action.title != "Safe delete file `target.py`" - }) + match code_action.title.as_str() { + "Find usages of file `target.py`" => saw_find = true, + "Delete file `target.py` anyway" => saw_delete = true, + "Safe delete file `target.py`" => saw_safe = true, + _ => {} + } + } + saw_find && saw_delete && !saw_safe }) .unwrap(); @@ -262,12 +590,12 @@ fn test_safe_delete_file_rejects_relative_import() { /// A Configerator `.cinc` file reaches its Thrift stub through an /// `import_thrift(...)` call rather than an `import` statement. The dependency /// is real -- deleting the stub breaks the config -- and the dependency graph -/// records it, so safe delete must not offer to remove it. +/// records it, so deletion must carry the "anyway" warning. #[test] fn test_safe_delete_file_rejects_special_import() { let root = get_test_files_root(); let root_path = root.path().join("safe_delete_special_import"); - let (interaction, _scope_uri) = init_with_delete_support(&root_path); + let (interaction, _scope_uri) = init_with_delete_support(&root_path, true, IndexingMode::None); interaction.client.did_open("config.cinc"); @@ -283,7 +611,12 @@ fn test_safe_delete_file_rejects_special_import() { "context": { "diagnostics": [] } })) .expect_response_with(|response: Option>| { - response.unwrap_or_default().iter().all(|action| { + let actions = response.expect("expected a delete-anyway action for a special import"); + assert!(actions.iter().any(|action| { + matches!(action, CodeActionResponse::CodeAction(action) + if action.title == "Delete file `types.thrift.pyi` anyway") + })); + actions.iter().all(|action| { let CodeActionResponse::CodeAction(code_action) = action else { return true; }; diff --git a/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/consumer.py b/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/consumer.py new file mode 100644 index 0000000000..982ca1967d --- /dev/null +++ b/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/consumer.py @@ -0,0 +1,8 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the MIT license found in the +# LICENSE file in the root directory of this source tree. + +from package import target + +print(target) diff --git a/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/package/__init__.py b/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/package/__init__.py new file mode 100644 index 0000000000..8d5e4d9667 --- /dev/null +++ b/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/package/__init__.py @@ -0,0 +1,8 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the MIT license found in the +# LICENSE file in the root directory of this source tree. + +from .target import value + +target = value diff --git a/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/package/target.py b/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/package/target.py new file mode 100644 index 0000000000..ce611ceaae --- /dev/null +++ b/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/package/target.py @@ -0,0 +1,6 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the MIT license found in the +# LICENSE file in the root directory of this source tree. + +value = 1 diff --git a/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/pyrefly.toml b/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/pyrefly.toml new file mode 100644 index 0000000000..54029c46ff --- /dev/null +++ b/pyrefly/lib/test/lsp/lsp_interaction/test_files/safe_delete_file_attribute_import/pyrefly.toml @@ -0,0 +1 @@ +search_path = ["."]