From 4f753f686b503ee953ca4bef15bb8fd4e779083d Mon Sep 17 00:00:00 2001 From: KotlinIsland <65446343+kotlinisland@users.noreply.github.com> Date: Mon, 17 Aug 2026 17:32:48 +1000 Subject: [PATCH] answer for the imports a renamed module leaves behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit renaming `util.by` to `helpers.by` renames the module `alpha.util`, and every `from alpha.util import thing` in the project is now naming a module that is not there. an editor cannot find those on its own: it would have to resolve every import against the same search paths the checker uses. so the protocol has it ask first, and the server had no answer — `workspace/willRenameFiles` was never advertised, and `rename` is a symbol rename that refuses an import's module component outright. the request is now handled. it arrives before the file moves, which is what makes it answerable at all: the old path still holds the file, so the module it is today resolves, while the new path is a path to read a name out of. that is the one thing `file_to_module` cannot do — it resolves the name it derives back to a file and checks the answer is the same file, which nothing at the new path can satisfy — so `path_to_module_name` answers the narrower, purely path-shaped question, for directories as well as files. what is rewritten is the module paths in import statements, at any depth in the file (an `if TYPE_CHECKING:` import is exactly the one written carefully), and the *uses* of a name an import binds when that name changes: `import alpha.util` binds `alpha`, so `alpha.util.thing()` moves too. those uses are found by their text and confirmed by their type — an expression is only rewritten when the checker says it is the module that moved, so a local called `util` in a file that also imports a module of that name is left alone. a relative import inside a package that is being renamed as a whole comes out unchanged, which is the truth: the dots go on meaning the file's own package, and that package is moving with it. two things are deliberately not rewritten, and both are reported rather than half-done: a module named as a string, and an import that would have to change shape — moving `alpha.util` to `beta.util` leaves `from alpha import util` needing a different statement, not a different word. folders are asked about as well as files, because renaming a directory renames every module under it and the client sends only the directory. a folder pattern cannot be narrowed the way the file one is — a directory has no extension, and whether it is a package is a question about the search paths — so every folder rename costs one request that usually answers with no edits. name a path by the deepest search path that contains it caught by driving the server against a real uv workspace. the member's package sits inside two search paths at once — the project root, and the editable entry uv writes for the member itself, pointing at its own `src` — and taking the first one consulted named `packages/alpha/src/alpha` as `packages.alpha.src.alpha`. that is not a module anything imports and not the name any `import alpha` resolves to, so a rename of it found nothing to rewrite and answered no edits at all. the deepest search path is the one whose name resolves back to the path, which is what `file_to_module` verifies for a file that exists. with the rule fixed, the same workspace answers with all three edits: the `from alpha import thing`, the `import alpha`, and the `alpha.thing()` in the body. --- crates/ty_ide/src/lib.rs | 4 + crates/ty_ide/src/module_rename.rs | 800 ++++++++++++++++++ crates/ty_module_resolver/src/lib.rs | 135 +++ crates/ty_server/src/capabilities.rs | 42 + crates/ty_server/src/server/api.rs | 8 + crates/ty_server/src/server/api/requests.rs | 2 + .../server/api/requests/will_rename_files.rs | 115 +++ crates/ty_server/tests/e2e/main.rs | 1 + .../e2e__initialize__initialization.snap | 20 + ...ialize__initialization_with_workspace.snap | 20 + ...enaming_a_module_rewrites_the_imports.snap | 23 + .../ty_server/tests/e2e/will_rename_files.rs | 48 ++ docs/basedpython/features/editor.md | 48 ++ 13 files changed, 1266 insertions(+) create mode 100644 crates/ty_ide/src/module_rename.rs create mode 100644 crates/ty_server/src/server/api/requests/will_rename_files.rs create mode 100644 crates/ty_server/tests/e2e/snapshots/e2e__will_rename_files__renaming_a_module_rewrites_the_imports.snap create mode 100644 crates/ty_server/tests/e2e/will_rename_files.rs diff --git a/crates/ty_ide/src/lib.rs b/crates/ty_ide/src/lib.rs index ae6845a7ba..e73c31a8c7 100644 --- a/crates/ty_ide/src/lib.rs +++ b/crates/ty_ide/src/lib.rs @@ -25,6 +25,7 @@ mod hover; mod importer; mod inlay_hints; mod markup; +mod module_rename; mod references; mod rename; mod selection_range; @@ -69,6 +70,9 @@ pub use inlay_hints::{ InlayHintKind, InlayHintLabel, InlayHintSettings, InlayHintTextEdit, inlay_hints, }; pub use markup::MarkupKind; +pub use module_rename::{ + FileMove, ModuleRenameEdits, SkipReason, SkippedImport, module_rename_edits, +}; pub use references::ReferencesMode; pub use rename::{can_rename, rename}; pub use selection_range::selection_range; diff --git a/crates/ty_ide/src/module_rename.rs b/crates/ty_ide/src/module_rename.rs new file mode 100644 index 0000000000..36933b9693 --- /dev/null +++ b/crates/ty_ide/src/module_rename.rs @@ -0,0 +1,800 @@ +//! Keeping a project's imports pointing at a module that is about to move. +//! +//! An editor that renames `src/alpha/util.py` to `src/alpha/helpers.py` has renamed the module +//! `alpha.util`, and every `from alpha.util import thing` in the project now names a module that +//! is not there. Finding those is not something an editor can do for itself — it would have to +//! resolve every import in the project against the same search paths the type checker uses — so +//! LSP has the client ask before it moves anything (`workspace/willRenameFiles`) and the server +//! answers with the edits that keep the project working. +//! +//! # What is edited +//! +//! The module paths written in import statements, in every file of the project: +//! +//! ```py +//! import alpha.util # the dotted name +//! import alpha.util as util # ... with an alias, which is unaffected +//! from alpha.util import thing # the module a symbol is imported from +//! from alpha import util # the module imported as a name +//! from .util import thing # a relative import, when the new name can still be written +//! # relative to the importing file's own package +//! ``` +//! +//! and, when one of those imports *binds* a name that changes — `import alpha.util` binds `alpha`, +//! and `from alpha import util` binds `util` — the uses of that name in the same file. Those are +//! found by asking the type checker what each expression is rather than by matching text: `util` in +//! one file may be the module and in another a local variable that happens to share its name, and +//! only one of them is a reference to what moved. +//! +//! # What is not +//! +//! - **Module names written as strings** — `importlib.import_module("alpha.util")`, a Django +//! `INSTALLED_APPS` entry, a `pyproject.toml` entry point. The Django ones have their own answer +//! in [`crate::django_template`]; the rest are not distinguishable from any other string. +//! - **An import that would have to change shape.** `from alpha import util` can be rewritten while +//! `util` is still a submodule of `alpha`; a move that puts it under a different package needs a +//! different statement, and rewriting one import statement into another is a refactor rather than +//! a repair. Those are left alone rather than half-done — see [`ModuleRenameEdits::skipped`]. +//! - **Relative imports whose target leaves the importing file's package.** Same reason: the dots +//! no longer reach it, and only a different statement would. +//! +//! Everything left alone is reported, so the client can tell the user which files it could not fix +//! rather than leaving them to find out from a stack trace. + +use ruff_db::files::File; +use ruff_db::parsed::parsed_module; +use ruff_db::system::SystemPathBuf; +use ruff_diagnostics::Edit; +use ruff_python_ast::visitor::source_order::{SourceOrderVisitor, walk_stmt}; +use ruff_python_ast::{self as ast}; +use ruff_text_size::{Ranged, TextRange}; +use ty_module_resolver::{ImportingFile, ModuleName, path_to_module_name}; +use ty_project::Db; +use ty_python_core::ProgramFile; +use ty_python_semantic::types::Type; +use ty_python_semantic::{HasType, SemanticModel}; + +use crate::code_action::FileEdit; + +/// A file or directory the client is about to move, and where it is going. +/// +/// Both paths are absolute. The old one is where the thing still is when this is asked — the +/// request arrives *before* the move — and the new one is where nothing is yet. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct FileMove { + pub old_path: SystemPathBuf, + pub new_path: SystemPathBuf, +} + +/// What a set of moves costs the project. +#[derive(Debug, Default)] +pub struct ModuleRenameEdits { + /// The edits to apply, at most one per range, grouped by nothing in particular. + pub edits: Vec, + + /// Imports that name something being moved and that this could not rewrite. + /// + /// Reported rather than dropped: an import left pointing at a module that has gone is a broken + /// file, and the difference between "the rename fixed everything" and "the rename fixed + /// everything except these two lines" is the difference between a working project and half an + /// hour of confusion. + pub skipped: Vec, +} + +/// An import naming something that moved, and why it was left alone. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkippedImport { + pub file: File, + pub range: TextRange, + pub reason: SkipReason, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SkipReason { + /// The module ends up under a different parent, so `from import ` no longer + /// reaches it and only a different statement would. + NeedsDifferentStatement, + /// A relative import whose target is no longer reachable from the importing file's package. + NoLongerRelative, +} + +/// The edits that keep the project's imports working when `moves` happen. +/// +/// Returns nothing when no move changes a module's name — moving a file the search paths do not +/// cover, or renaming a directory that is not a package, is not a rename of anything importable. +pub fn module_rename_edits(db: &dyn Db, moves: &[FileMove]) -> ModuleRenameEdits { + let environment = db.project().program(db).resolver_environment(db); + + let renamings: Vec = moves + .iter() + .filter_map(|file_move| { + let old = path_to_module_name(db, environment, &file_move.old_path)?; + let new = path_to_module_name(db, environment, &file_move.new_path)?; + (old != new).then_some(Renaming { old, new }) + }) + .collect(); + + if renamings.is_empty() { + return ModuleRenameEdits::default(); + } + + let mut result = ModuleRenameEdits::default(); + // Serial rather than the parallel walk `workspace_symbols` uses: this runs once, from a + // deliberate gesture, and it has to produce a stable order — the client applies these as one + // edit and a set of edits that arrives in a different order on every run is one nobody can + // review or test. + let mut files: Vec = db.project().files(db).iter().copied().collect(); + files.sort_by_key(|file| file.path(db).as_str().to_string()); + + for file in files { + collect_edits_for_file(db, file, &renamings, &mut result); + } + + result +} + +/// One module moving to another name, and everything under it moving with it. +struct Renaming { + old: ModuleName, + new: ModuleName, +} + +impl Renaming { + /// What `name` becomes, or `None` when this renaming does not cover it. + /// + /// A package takes its submodules with it: renaming `alpha` to `beta` renames `alpha.util` to + /// `beta.util` without anybody saying so, which is why this is a prefix rewrite rather than an + /// equality check. + fn apply(&self, name: &ModuleName) -> Option { + if name == &self.old { + return Some(self.new.clone()); + } + let rest = name.relative_to(&self.old)?; + let mut renamed = self.new.clone(); + renamed.extend(&rest); + Some(renamed) + } +} + +/// The new name for `name` under any of `renamings`. +fn renamed(renamings: &[Renaming], name: &ModuleName) -> Option { + renamings.iter().find_map(|renaming| renaming.apply(name)) +} + +fn collect_edits_for_file( + db: &dyn Db, + file: File, + renamings: &[Renaming], + result: &mut ModuleRenameEdits, +) { + let program_file = db.program_file(file); + let parsed = parsed_module(db, program_file.python_file(db)); + let module = parsed.load(db); + + let mut imports = Imports::default(); + imports.visit_body(&module.syntax().body); + if imports.is_empty() { + return; + } + + let importing_file = + ImportingFile::File(file, db.project().program(db).resolver_environment(db)); + + // The names this file binds to a module whose name changes. Collected while the import + // statements are rewritten and used afterwards, because a use of `alpha` in the body can only + // be judged once it is known that this file's `import alpha` was one of the imports rewritten. + let mut rebound: Vec = Vec::new(); + + for import in &imports.plain { + for alias in &import.names { + let Some(name) = ModuleName::new(alias.name.as_str()) else { + continue; + }; + let Some(new_name) = renamed(renamings, &name) else { + continue; + }; + result.edits.push(FileEdit { + file, + edit: Edit::range_replacement(new_name.as_str().to_string(), alias.name.range()), + }); + // `import alpha.util` binds `alpha`, and every use of it in the body is written + // `alpha.util.thing`; with an `as` the binding is the alias and nothing else changes. + if alias.asname.is_none() { + rebound.push(Rebinding { + spelling: name.clone(), + module: name, + replacement: new_name.as_str().to_string(), + }); + } + } + } + + for import in &imports.from { + collect_edits_for_import_from( + db, + file, + importing_file, + renamings, + import, + result, + &mut rebound, + ); + } + + if !rebound.is_empty() { + collect_edits_for_uses(db, file, program_file, &module, &rebound, result); + } +} + +/// A name this file binds that is about to mean something else. +/// +/// Three separate facts, because the name as *written* and the module it *is* are not the same +/// string: `from alpha import util` writes `util` and means `alpha.util`. The written form is what +/// the body spells and what has to be matched there; the module is what the type checker will say +/// the expression is; and the replacement is the written form's new spelling, which for that +/// statement is one component and for `import alpha.util` is the whole dotted name. +struct Rebinding { + /// How the body spells it — `util`, or `alpha.util`. + spelling: ModuleName, + /// The module it refers to, absolute, which is what its type will name. + module: ModuleName, + /// What the body should spell instead. + replacement: String, +} + +fn collect_edits_for_import_from( + db: &dyn Db, + file: File, + importing_file: ImportingFile<'_>, + renamings: &[Renaming], + import: &ast::StmtImportFrom, + result: &mut ModuleRenameEdits, + rebound: &mut Vec, +) { + // The module the statement imports *from*, as an absolute name. Relative imports are resolved + // against the importing file, which is the whole reason this needs the resolver rather than the + // text of the statement. + let Ok(from) = ModuleName::from_import_statement(db, importing_file, import) else { + return; + }; + + if let Some(new_from) = renamed(renamings, &from) { + match rewritten_module_reference(db, importing_file, renamings, import, &new_from) { + Ok(Some(edit)) => result.edits.push(edit), + // Nothing to rewrite: either the statement has no module text at all (`from . import x`) + // or the text it has still spells the right thing, which is the ordinary outcome for a + // relative import inside a package that is moving as a whole. + Ok(None) => {} + Err(reason) => result.skipped.push(SkippedImport { + file, + range: import.range(), + reason, + }), + } + } + + // `from alpha import util` names a module in its *alias* rather than in its module path, and + // that is the form the rename of a leaf module usually meets. + for alias in &import.names { + let Some(alias_name) = ModuleName::new(alias.name.as_str()) else { + continue; + }; + let mut imported = from.clone(); + imported.extend(&alias_name); + let Some(new_imported) = renamed(renamings, &imported) else { + continue; + }; + // Only the last component may change here: the rest of the name is written in the `from`, + // which the loop above has already dealt with if it moved too. + let Some(new_parent) = new_imported.parent() else { + continue; + }; + let new_from = renamed(renamings, &from).unwrap_or_else(|| from.clone()); + if new_parent != new_from { + result.skipped.push(SkippedImport { + file, + range: alias.range(), + reason: SkipReason::NeedsDifferentStatement, + }); + continue; + } + // The last component is often untouched — renaming the package `alpha` to `beta` leaves + // `from beta import util` spelling `util` exactly as it did — and an edit that replaces a + // name with itself is noise in a diff the user is about to be shown. + if new_imported.last_component() == alias.name.as_str() { + continue; + } + result.edits.push(FileEdit { + file, + edit: Edit::range_replacement( + new_imported.last_component().to_string(), + alias.name.range(), + ), + }); + if alias.asname.is_none() { + rebound.push(Rebinding { + spelling: alias_name, + module: imported, + replacement: new_imported.last_component().to_string(), + }); + } + } +} + +/// The edit that makes `import` name `new_from`, or the reason it cannot. +/// +/// Absolute imports are a straight replacement of the module text. A relative one is only +/// rewritable while the new name is still under the package the dots reach: `from .util import x` +/// in `alpha/main.py` can become `from .helpers import x`, but nothing that starts with a dot can +/// name a module that has left `alpha`. +fn rewritten_module_reference( + db: &dyn Db, + importing_file: ImportingFile<'_>, + renamings: &[Renaming], + import: &ast::StmtImportFrom, + new_from: &ModuleName, +) -> Result, SkipReason> { + let file = importing_file.file(db); + let Some(module) = import.module.as_ref() else { + // `from . import x` / `from .. import x`: the dots are the whole reference. + return Ok(None); + }; + + if import.level == 0 { + return Ok(Some(FileEdit { + file, + edit: Edit::range_replacement(new_from.as_str().to_string(), module.range()), + })); + } + + // What the dots resolve to, which is what the text after them is relative to. + let Ok(base) = ModuleName::from_identifier_parts(db, importing_file, None, import.level) else { + return Err(SkipReason::NoLongerRelative); + }; + // The dots go on meaning "this file's own package", and that package moves when the file moves + // with it. So a relative import inside a package that is being renamed as a whole is measured + // against where the package is going, and comes out unchanged — which is the truth: nothing + // about `from .util import thing` stops working because its package was renamed around it. + let new_base = renamed(renamings, &base).unwrap_or(base); + match new_from.relative_to(&new_base) { + Some(tail) if tail.as_str() == module.as_str() => Ok(None), + Some(tail) => Ok(Some(FileEdit { + file, + edit: Edit::range_replacement(tail.as_str().to_string(), module.range()), + })), + None => Err(SkipReason::NoLongerRelative), + } +} + +/// The uses, in this file, of a name that an import statement bound to a module that moved. +/// +/// `import alpha.util` binds `alpha`, so `alpha.util.thing()` in the body has to become +/// `beta.util.thing()` when `alpha` moves. The candidates are found by their text and confirmed by +/// their type: an expression is only rewritten when the type checker says it *is* the module that +/// moved, which is what keeps a local variable called `util` from being rewritten in a file that +/// also imports a module of that name. +fn collect_edits_for_uses( + db: &dyn Db, + file: File, + program_file: ProgramFile<'_>, + module: &ruff_db::parsed::ParsedModuleRef, + rebound: &[Rebinding], + result: &mut ModuleRenameEdits, +) { + let model = SemanticModel::new(db, program_file); + + let mut uses = ModuleUses { + rebound, + candidates: Vec::new(), + }; + uses.visit_body(&module.syntax().body); + + for (expression, rebinding) in uses.candidates { + let Some(Type::ModuleLiteral(literal)) = expression.inferred_type(&model) else { + continue; + }; + // The expression's own text says which module it *reads* as; the type says which module it + // is. Both have to agree, or this is a use of something else that happens to be spelled the + // same — a package whose `__init__` re-exports a submodule of another name, most obviously. + if literal.module(db).name(db) != &rebinding.module { + continue; + } + result.edits.push(FileEdit { + file, + edit: Edit::range_replacement(rebinding.replacement.clone(), expression.range()), + }); + } +} + +/// Every import statement in a file, at any depth. +/// +/// Imports are not only at the top of a file: they sit inside `if TYPE_CHECKING:`, inside `try:` +/// blocks that fall back to another package, and inside functions that defer an expensive one. A +/// rename that only rewrote the top-level ones would leave exactly the imports that were written +/// carefully. +#[derive(Default)] +struct Imports<'a> { + plain: Vec<&'a ast::StmtImport>, + from: Vec<&'a ast::StmtImportFrom>, +} + +impl Imports<'_> { + fn is_empty(&self) -> bool { + self.plain.is_empty() && self.from.is_empty() + } +} + +impl<'a> SourceOrderVisitor<'a> for Imports<'a> { + fn visit_stmt(&mut self, stmt: &'a ast::Stmt) { + match stmt { + ast::Stmt::Import(import) => self.plain.push(import), + ast::Stmt::ImportFrom(import) => self.from.push(import), + _ => {} + } + walk_stmt(self, stmt); + } +} + +/// The expressions that spell one of the rebound module names. +/// +/// Text first, type second: asking the type checker about every expression in a file would type the +/// whole file to answer a question about the handful of expressions that could possibly be affected. +struct ModuleUses<'a, 'r> { + rebound: &'r [Rebinding], + candidates: Vec<(ModuleUse<'a>, &'r Rebinding)>, +} + +/// An expression that reads as a dotted module name. +#[derive(Debug, Clone, Copy)] +enum ModuleUse<'a> { + Name(&'a ast::ExprName), + Attribute(&'a ast::ExprAttribute), +} + +impl ModuleUse<'_> { + fn range(self) -> TextRange { + match self { + ModuleUse::Name(name) => name.range(), + ModuleUse::Attribute(attribute) => attribute.range(), + } + } + + fn inferred_type<'db>(self, model: &SemanticModel<'db>) -> Option> { + match self { + ModuleUse::Name(name) => name.inferred_type(model), + ModuleUse::Attribute(attribute) => attribute.inferred_type(model), + } + } +} + +impl<'a> SourceOrderVisitor<'a> for ModuleUses<'a, '_> { + fn visit_expr(&mut self, expr: &'a ast::Expr) { + // The longest dotted prefix wins, and its subexpressions are not visited: rewriting both + // `alpha` and `alpha.util` inside `alpha.util.thing` would produce two overlapping edits of + // the same text. + if let Some((candidate, rebinding)) = self.candidate(expr) { + self.candidates.push((candidate, rebinding)); + return; + } + ruff_python_ast::visitor::source_order::walk_expr(self, expr); + } +} + +impl<'a, 'r> ModuleUses<'a, 'r> { + fn candidate(&self, expr: &'a ast::Expr) -> Option<(ModuleUse<'a>, &'r Rebinding)> { + let (use_, spelling) = match expr { + ast::Expr::Name(name) => (ModuleUse::Name(name), name.id.to_string()), + ast::Expr::Attribute(attribute) => (ModuleUse::Attribute(attribute), dotted(expr)?), + _ => return None, + }; + let spelled = ModuleName::new(&spelling)?; + let rebinding = self + .rebound + .iter() + .find(|rebinding| rebinding.spelling == spelled)?; + Some((use_, rebinding)) + } +} + +/// The dotted name an expression spells, when it is one: `alpha.util` from `alpha.util`, and +/// nothing at all from `f().util` or `alpha[0].util`. +fn dotted(expr: &ast::Expr) -> Option { + match expr { + ast::Expr::Name(name) => Some(name.id.to_string()), + ast::Expr::Attribute(attribute) => { + let mut base = dotted(&attribute.value)?; + base.push('.'); + base.push_str(attribute.attr.as_str()); + Some(base) + } + _ => None, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tests::{CursorTest, cursor_test}; + use insta::assert_snapshot; + use ruff_db::source::source_text; + use std::collections::BTreeMap; + use std::fmt::Write; + + impl CursorTest { + /// The project after `old` is renamed to `new`, showing only the files that changed. + /// + /// The edits are applied rather than listed, because what matters about a rename is the + /// import line it leaves behind — a list of ranges and replacement strings is a puzzle the + /// reader has to solve before they can see whether the answer is right. + fn rename_module(&self, old: &str, new: &str) -> String { + let result = salsa::attach(&self.db, || { + module_rename_edits( + &self.db, + &[FileMove { + old_path: SystemPathBuf::from(old), + new_path: SystemPathBuf::from(new), + }], + ) + }); + + let mut by_file: BTreeMap)> = BTreeMap::new(); + for file_edit in &result.edits { + by_file + .entry(file_edit.file.path(&self.db).as_str().to_string()) + .or_insert_with(|| (file_edit.file, Vec::new())) + .1 + .push(&file_edit.edit); + } + + let mut rendered = String::new(); + for (path, (file, mut edits)) in by_file { + let mut text = source_text(&self.db, file).as_str().to_string(); + // Back to front, so an earlier edit's replacement cannot move a later one's range. + edits.sort_by_key(|edit| std::cmp::Reverse(edit.start())); + for edit in edits { + text.replace_range( + usize::from(edit.start())..usize::from(edit.end()), + edit.content().unwrap_or_default(), + ); + } + let _ = writeln!(rendered, "--- {path}\n{}", text.trim_end()); + } + + for skipped in &result.skipped { + let _ = writeln!( + rendered, + "!!! {} {:?} {:?}", + skipped.file.path(&self.db), + skipped.range, + skipped.reason + ); + } + + if rendered.is_empty() { + "no edits".to_string() + } else { + rendered + } + } + } + + /// The commonest form by far, and the one with nothing else to think about: the module is named + /// in the `from`, and the names the statement binds are symbols rather than the module. + #[test] + fn from_import_follows_a_renamed_module() { + let test = CursorTest::builder() + .source("alpha/__init__.py", "") + .source("alpha/util.py", "def thing(): ...") + .source( + "main.py", + "\ +from alpha.util import thing + +thing() +", + ) + .build(); + + assert_snapshot!(test.rename_module("/alpha/util.py", "/alpha/helpers.py"), @" + --- /main.py + from alpha.helpers import thing + + thing() + "); + } + + /// `import alpha.util` binds `alpha`, not `alpha.util`, so the statement is only half the job: + /// every use in the body spells the module out again and has to move with it. + #[test] + fn plain_import_and_its_uses_follow_a_renamed_module() { + let test = CursorTest::builder() + .source("alpha/__init__.py", "") + .source("alpha/util.py", "def thing(): ...") + .source( + "main.py", + "\ +import alpha.util + +alpha.util.thing() +print(alpha.util) +", + ) + .build(); + + assert_snapshot!(test.rename_module("/alpha/util.py", "/alpha/helpers.py"), @" + --- /main.py + import alpha.helpers + + alpha.helpers.thing() + print(alpha.helpers) + "); + } + + /// A renamed package takes everything under it, without any of those modules being named. + #[test] + fn renaming_a_package_renames_the_modules_inside_it() { + let test = CursorTest::builder() + .source("alpha/__init__.py", "") + .source("alpha/util.py", "def thing(): ...") + .source("alpha/deep/__init__.py", "") + .source("alpha/deep/inner.py", "value = 1") + .source( + "main.py", + "\ +from alpha.util import thing +from alpha.deep.inner import value +from alpha import util +import alpha.util + +thing() +print(value, util, alpha.util) +", + ) + .build(); + + assert_snapshot!(test.rename_module("/alpha", "/beta"), @" + --- /main.py + from beta.util import thing + from beta.deep.inner import value + from beta import util + import beta.util + + thing() + print(value, util, beta.util) + "); + } + + /// With an alias, the name the body uses is the alias, which the move does not touch. + #[test] + fn an_alias_is_left_alone() { + let test = CursorTest::builder() + .source("alpha/__init__.py", "") + .source("alpha/util.py", "def thing(): ...") + .source( + "main.py", + "\ +import alpha.util as u + +u.thing() +", + ) + .build(); + + assert_snapshot!(test.rename_module("/alpha/util.py", "/alpha/helpers.py"), @" + --- /main.py + import alpha.helpers as u + + u.thing() + "); + } + + /// A relative import keeps its dots and changes only the part after them, so long as the module + /// is still inside the package they reach. + #[test] + fn a_relative_import_is_rewritten_after_the_dots() { + let test = CursorTest::builder() + .source("alpha/__init__.py", "") + .source("alpha/util.py", "def thing(): ...") + .source( + "alpha/main.py", + "\ +from .util import thing +from . import util + +thing() +util.thing() +", + ) + .build(); + + assert_snapshot!(test.rename_module("/alpha/util.py", "/alpha/helpers.py"), @" + --- /alpha/main.py + from .helpers import thing + from . import helpers + + thing() + helpers.thing() + "); + } + + /// The check that stops this being a search and replace: the local `util` is a string that + /// happens to be spelled like the module, and renaming it would change what the function means. + #[test] + fn a_local_that_shadows_the_module_is_not_touched() { + let test = CursorTest::builder() + .source("alpha/__init__.py", "") + .source("alpha/util.py", "def thing(): ...") + .source( + "main.py", + "\ +from alpha import util + +def shadowed(): + util = \"not the module\" + return util.upper() + +util.thing() +", + ) + .build(); + + assert_snapshot!(test.rename_module("/alpha/util.py", "/alpha/helpers.py"), @r#" + --- /main.py + from alpha import helpers + + def shadowed(): + util = "not the module" + return util.upper() + + helpers.thing() + "#); + } + + /// Moving a module to another package is not a rename any single import statement can express: + /// `from alpha import util` would have to become a different statement. Reported rather than + /// rewritten into something that does not mean the same thing. + #[test] + fn an_import_that_would_need_a_different_statement_is_reported() { + let test = CursorTest::builder() + .source("alpha/__init__.py", "") + .source("alpha/util.py", "def thing(): ...") + .source("beta/__init__.py", "") + .source( + "main.py", + "\ +from alpha import util + +util.thing() +", + ) + .build(); + + assert_snapshot!(test.rename_module("/alpha/util.py", "/beta/util.py"), @"!!! /main.py 18..22 NeedsDifferentStatement"); + } + + /// Nothing to do for a file the search paths do not cover: it is not a module, so no import can + /// be naming it. + #[test] + fn moving_something_that_is_not_a_module_costs_nothing() { + let test = cursor_test( + "\ +x = 1 +", + ); + + assert_snapshot!(test.rename_module("/notes.md", "/notes-old.md"), @"no edits"); + } + + /// A move that leaves the name alone — a directory renamed to itself, a file moved between two + /// paths that spell the same module — is not a rename of anything. + #[test] + fn a_move_that_does_not_change_the_name_costs_nothing() { + let test = cursor_test( + "\ +x = 1 +", + ); + + assert_snapshot!(test.rename_module("/alpha/util.py", "/alpha/util.py"), @"no edits"); + } +} diff --git a/crates/ty_module_resolver/src/lib.rs b/crates/ty_module_resolver/src/lib.rs index efe1de84df..9ba9185b4c 100644 --- a/crates/ty_module_resolver/src/lib.rs +++ b/crates/ty_module_resolver/src/lib.rs @@ -45,6 +45,52 @@ type FxOrderMap = ordermap::map::OrderMap( + db: &'db dyn Db, + resolver_environment: ResolverEnvironment<'db>, + path: &SystemPath, +) -> Option { + // `Typing` mode for the reason `system_module_search_paths` gives below: the question is which + // paths belong to the project at all, not which of two stdlib variants a name resolves to. + search_paths(db, resolver_environment, ModuleResolveMode::Typing) + .filter_map(|search_path| { + let name = search_path.relativize_system_path(path)?.to_module_name()?; + // How much of `path` this search path accounts for. A vendored path accounts for none of + // it and sorts last, which is right: nothing under the project is named by the stdlib. + let depth = search_path + .as_system_path() + .map_or(0, |root| root.as_str().len()); + Some((depth, name)) + }) + .max_by_key(|(depth, _)| *depth) + .map(|(_, name)| name) +} + /// Returns an iterator over all search paths pointing to a system path pub fn system_module_search_paths<'db>( db: &'db dyn Db, @@ -76,3 +122,92 @@ impl<'db> Iterator for SystemModuleSearchPathsIter<'db> { } impl FusedIterator for SystemModuleSearchPathsIter<'_> {} + +#[cfg(test)] +mod tests { + use ruff_db::Db as _; + use ruff_db::system::{DbWithWritableSystem as _, SystemPathBuf}; + + use crate::db::tests::TestDb; + use crate::settings::SearchPathSettings; + use crate::strategy::FallibleStrategy; + + use super::*; + + /// A project whose search paths nest, which is what a uv workspace's editable installs produce: + /// the project root, and one entry per member pointing at that member's own `src`. + fn workspace() -> TestDb { + let project = SystemPathBuf::from("/project"); + let member_src = project.join("packages/alpha/src"); + + let mut db = TestDb::new(); + db.write_file(member_src.join("alpha/__init__.py"), "") + .unwrap(); + + let search_paths = SearchPathSettings { + src_roots: vec![project], + extra_paths: vec![member_src], + ..SearchPathSettings::empty() + } + .to_search_paths(db.system(), db.vendored(), &FallibleStrategy) + .expect("valid search path settings"); + db.set_search_paths(search_paths); + db + } + + /// The bug this rule exists for: the project root also contains the member's package, and naming + /// it from there gives `packages.alpha.src.alpha` — a name nothing imports and nothing resolves. + #[test] + fn a_path_is_named_by_the_deepest_search_path_that_contains_it() { + let db = workspace(); + assert_eq!( + path_to_module_name( + &db, + db.resolver_environment(), + SystemPath::new("/project/packages/alpha/src/alpha"), + ), + ModuleName::new("alpha"), + ); + } + + #[test] + fn a_module_inside_a_package_is_named_under_it() { + let db = workspace(); + assert_eq!( + path_to_module_name( + &db, + db.resolver_environment(), + SystemPath::new("/project/packages/alpha/src/alpha/util.py"), + ), + ModuleName::new("alpha.util"), + ); + } + + /// The point of asking about a path rather than a file: at the moment an editor asks, the answer + /// is about somewhere nothing has been written yet. + #[test] + fn a_path_nothing_is_at_still_has_a_name() { + let db = workspace(); + assert_eq!( + path_to_module_name( + &db, + db.resolver_environment(), + SystemPath::new("/project/packages/alpha/src/gamma"), + ), + ModuleName::new("gamma"), + ); + } + + #[test] + fn a_path_no_search_path_covers_is_not_a_module() { + let db = workspace(); + assert_eq!( + path_to_module_name( + &db, + db.resolver_environment(), + SystemPath::new("/elsewhere/thing.py") + ), + None, + ); + } +} diff --git a/crates/ty_server/src/capabilities.rs b/crates/ty_server/src/capabilities.rs index 70f14a1517..7bc6fb50f2 100644 --- a/crates/ty_server/src/capabilities.rs +++ b/crates/ty_server/src/capabilities.rs @@ -572,6 +572,10 @@ pub(crate) fn server_capabilities( supported: Some(true), change_notifications: Some(true.into()), }), + file_operations: Some(lsp_types::FileOperationOptions { + will_rename: Some(will_rename_registration()), + ..Default::default() + }), ..Default::default() }), type_hierarchy_provider: Some(true.into()), @@ -580,6 +584,44 @@ pub(crate) fn server_capabilities( } } +/// Which renames the client should ask about before it carries them out. +/// +/// Two filters, and both are needed for the same feature. A **file** rename is a module renamed — +/// every module lives in one file, and the extensions listed are the ones the module resolver +/// accepts. A **folder** rename is a *package* renamed, which renames every module inside it, and +/// the client sends only the folder (never its contents), so a server that asked for files alone +/// would be told nothing at all about the rename that changes the most names. +/// +/// The folder pattern cannot be narrowed the way the file one is: a directory has no extension to +/// match on, and whether it is a package is a question about the search paths rather than about its +/// name. So every folder rename is asked about, and the ones that turn out not to be packages cost +/// one request that answers with no edits. +fn will_rename_registration() -> lsp_types::FileOperationRegistrationOptions { + fn filter( + glob: &str, + kind: lsp_types::FileOperationPatternKind, + ) -> lsp_types::FileOperationFilter { + lsp_types::FileOperationFilter { + scheme: Some("file".to_string()), + pattern: lsp_types::FileOperationPattern { + glob: glob.to_string(), + matches: Some(kind), + options: None, + }, + } + } + + lsp_types::FileOperationRegistrationOptions { + filters: vec![ + filter( + "**/*.{py,pyi,by,byi}", + lsp_types::FileOperationPatternKind::File, + ), + filter("**", lsp_types::FileOperationPatternKind::Folder), + ], + } +} + /// Creates the default [`DiagnosticOptions`] for the server. pub(crate) fn server_diagnostic_options(workspace_diagnostics: bool) -> DiagnosticOptions { DiagnosticOptions { diff --git a/crates/ty_server/src/server/api.rs b/crates/ty_server/src/server/api.rs index 692a56b94c..e72129ffb8 100644 --- a/crates/ty_server/src/server/api.rs +++ b/crates/ty_server/src/server/api.rs @@ -136,6 +136,14 @@ pub(super) fn request(req: server::Request) -> Task { >( req, BackgroundSchedule::Worker ), + // The client is holding a file move open waiting for this, so it is scheduled like the + // other things a person is watching for rather than as background work. + requests::WillRenameFilesRequestHandler::METHOD => { + background_request_task::( + req, + BackgroundSchedule::LatencySensitive, + ) + } requests::PrepareTypeHierarchyRequestHandler::METHOD => background_document_request_task::< requests::PrepareTypeHierarchyRequestHandler, >( diff --git a/crates/ty_server/src/server/api/requests.rs b/crates/ty_server/src/server/api/requests.rs index b87ba79bd4..f9867ddb1a 100644 --- a/crates/ty_server/src/server/api/requests.rs +++ b/crates/ty_server/src/server/api/requests.rs @@ -44,6 +44,7 @@ mod signature_help; mod transpile; mod type_hierarchy_subtypes; mod type_hierarchy_supertypes; +mod will_rename_files; mod workspace_diagnostic; mod workspace_symbols; @@ -79,5 +80,6 @@ pub(super) use signature_help::SignatureHelpRequestHandler; pub(super) use transpile::TranspileRequestHandler; pub(super) use type_hierarchy_subtypes::TypeHierarchySubtypesRequestHandler; pub(super) use type_hierarchy_supertypes::TypeHierarchySupertypesRequestHandler; +pub(super) use will_rename_files::WillRenameFilesRequestHandler; pub(super) use workspace_diagnostic::WorkspaceDiagnosticRequestHandler; pub(super) use workspace_symbols::WorkspaceSymbolRequestHandler; diff --git a/crates/ty_server/src/server/api/requests/will_rename_files.rs b/crates/ty_server/src/server/api/requests/will_rename_files.rs new file mode 100644 index 0000000000..e1590ea167 --- /dev/null +++ b/crates/ty_server/src/server/api/requests/will_rename_files.rs @@ -0,0 +1,115 @@ +use lsp_types::{RenameFilesParams, TextEdit, Uri, WillRenameFilesRequest, WorkspaceEdit}; +use ruff_db::files::FileRange; +use ruff_db::system::SystemPathBuf; +use ruff_text_size::Ranged; +use rustc_hash::FxHashMap; +use ty_ide::{FileMove, module_rename_edits}; + +use crate::document::FileRangeExt; +use crate::server::api::traits::{ + BackgroundRequestHandler, RequestHandler, RetriableRequestHandler, +}; +use crate::session::SessionSnapshot; +use crate::session::client::Client; + +/// `workspace/willRenameFiles` — the edits that keep imports working across a rename the editor is +/// about to perform. +/// +/// The client asks *before* it moves anything, applies whatever comes back, and only then does the +/// move. That ordering is what makes the answer computable at all: the old path still holds the +/// file, so the module it is today can be resolved, while the new path is just a path — see +/// [`ty_module_resolver::path_to_module_name`]. +/// +/// A rename that changes no module's name — a `README.md`, a directory no search path covers — +/// answers `None` rather than an empty edit, which is what tells the client to get on with the move +/// without showing the user an empty preview. +pub(crate) struct WillRenameFilesRequestHandler; + +impl RequestHandler for WillRenameFilesRequestHandler { + type RequestType = WillRenameFilesRequest; +} + +impl BackgroundRequestHandler for WillRenameFilesRequestHandler { + fn run( + snapshot: &SessionSnapshot, + _client: &Client, + params: RenameFilesParams, + ) -> crate::server::Result> { + let moves: Vec = params + .files + .iter() + .filter_map(|rename| { + Some(FileMove { + old_path: system_path(&rename.old_uri)?, + new_path: system_path(&rename.new_uri)?, + }) + }) + .collect(); + + if moves.is_empty() { + return Ok(None); + } + + let mut changes: FxHashMap> = FxHashMap::default(); + + // Every project, because a rename in one workspace folder can move a module that another + // folder imports; a project the paths have nothing to do with contributes nothing, since + // the paths resolve to no module of its. + for db in snapshot.projects() { + let result = module_rename_edits(db, &moves); + + for skipped in &result.skipped { + // Not an error: the rename can still go ahead, and this is the one import the user + // will have to look at themselves. Logged with its location so that "which line?" + // has an answer that does not involve searching the project. + tracing::info!( + "willRenameFiles: leaving an import in {} alone ({:?}); it would need a \ + different statement to name the module's new home", + skipped.file.path(db), + skipped.reason, + ); + } + + for file_edit in result.edits { + let range = FileRange::new(file_edit.file, file_edit.edit.range()); + let Some(location) = range + .to_lsp_range(db, snapshot.position_encoding()) + .and_then(|range| range.to_location()) + else { + continue; + }; + changes.entry(location.uri).or_default().push(TextEdit { + range: location.range, + new_text: file_edit.edit.content().unwrap_or_default().to_string(), + }); + } + } + + if changes.is_empty() { + return Ok(None); + } + + Ok(Some(WorkspaceEdit { + changes: Some(changes.into_iter().collect()), + document_changes: None, + change_annotations: None, + })) + } +} + +/// The path a `file:` URI names, or nothing for a URI that is not one. +/// +/// A client may send `untitled:` for a buffer that has never been saved, which cannot be a module +/// and cannot be moved; those are dropped rather than refused, so a mixed rename still gets the +/// edits for the files that do exist. +fn system_path(uri: &Uri) -> Option { + SystemPathBuf::from_path_buf(uri.to_file_path().ok()?).ok() +} + +impl RetriableRequestHandler for WillRenameFilesRequestHandler { + /// A rename is a one-shot gesture the user is waiting on, and the client is holding the file + /// move until it answers. Retrying on a database change is the right trade here for the same + /// reason it is for the other whole-project requests: the alternative is telling the editor to + /// go ahead with a rename this never got to check. + const RETRY_ON_CANCELLATION: bool = true; +} diff --git a/crates/ty_server/tests/e2e/main.rs b/crates/ty_server/tests/e2e/main.rs index 899084af3a..23ac98ea15 100644 --- a/crates/ty_server/tests/e2e/main.rs +++ b/crates/ty_server/tests/e2e/main.rs @@ -46,6 +46,7 @@ mod rename; mod semantic_tokens; mod signature_help; mod type_hierarchy; +mod will_rename_files; mod workspace_folders; use std::collections::{BTreeMap, HashMap, VecDeque}; diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization.snap b/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization.snap index ce2381cfc5..d80895c5f1 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization.snap @@ -119,6 +119,26 @@ expression: initialization_result "workspaceFolders": { "supported": true, "changeNotifications": true + }, + "fileOperations": { + "willRename": { + "filters": [ + { + "scheme": "file", + "pattern": { + "glob": "**/*.{py,pyi,by,byi}", + "matches": "file" + } + }, + { + "scheme": "file", + "pattern": { + "glob": "**", + "matches": "folder" + } + } + ] + } } } }, diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization_with_workspace.snap b/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization_with_workspace.snap index ce2381cfc5..d80895c5f1 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization_with_workspace.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization_with_workspace.snap @@ -119,6 +119,26 @@ expression: initialization_result "workspaceFolders": { "supported": true, "changeNotifications": true + }, + "fileOperations": { + "willRename": { + "filters": [ + { + "scheme": "file", + "pattern": { + "glob": "**/*.{py,pyi,by,byi}", + "matches": "file" + } + }, + { + "scheme": "file", + "pattern": { + "glob": "**", + "matches": "folder" + } + } + ] + } } } }, diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__will_rename_files__renaming_a_module_rewrites_the_imports.snap b/crates/ty_server/tests/e2e/snapshots/e2e__will_rename_files__renaming_a_module_rewrites_the_imports.snap new file mode 100644 index 0000000000..53118985ab --- /dev/null +++ b/crates/ty_server/tests/e2e/snapshots/e2e__will_rename_files__renaming_a_module_rewrites_the_imports.snap @@ -0,0 +1,23 @@ +--- +source: crates/ty_server/tests/e2e/will_rename_files.rs +expression: edits +--- +{ + "changes": { + "file:///main.py": [ + { + "range": { + "start": { + "line": 0, + "character": 5 + }, + "end": { + "line": 0, + "character": 15 + } + }, + "newText": "alpha.helpers" + } + ] + } +} diff --git a/crates/ty_server/tests/e2e/will_rename_files.rs b/crates/ty_server/tests/e2e/will_rename_files.rs new file mode 100644 index 0000000000..ecbe66063c --- /dev/null +++ b/crates/ty_server/tests/e2e/will_rename_files.rs @@ -0,0 +1,48 @@ +use crate::TestServerBuilder; +use insta::assert_json_snapshot; +use lsp_types::{RenameFilesParams, WillRenameFilesRequest}; + +/// Renaming a module's file, end to end: the client asks before it moves anything and gets back the +/// edit that keeps the import naming a module that exists. +#[test] +fn renaming_a_module_rewrites_the_imports() -> anyhow::Result<()> { + let mut server = TestServerBuilder::new()? + .with_file("alpha/__init__.py", "")? + .with_file("alpha/util.py", "def thing(): ...\n")? + .with_file("main.py", "from alpha.util import thing\n\nthing()\n")? + .build() + .wait_until_workspaces_are_initialized(); + + let edits = server.send_request_await::(RenameFilesParams { + files: vec![lsp_types::FileRename { + old_uri: server.file_uri("alpha/util.py"), + new_uri: server.file_uri("alpha/helpers.py"), + }], + }); + + assert_json_snapshot!(edits); + + Ok(()) +} + +/// A file no import can be naming: the answer is nothing at all, so the editor carries on with the +/// move rather than showing a preview of an empty change. +#[test] +fn renaming_something_that_is_not_a_module_answers_nothing() -> anyhow::Result<()> { + let mut server = TestServerBuilder::new()? + .with_file("main.py", "x = 1\n")? + .with_file("notes.md", "hello\n")? + .build() + .wait_until_workspaces_are_initialized(); + + let edits = server.send_request_await::(RenameFilesParams { + files: vec![lsp_types::FileRename { + old_uri: server.file_uri("notes.md"), + new_uri: server.file_uri("notes-old.md"), + }], + }); + + assert!(edits.is_none(), "expected no edits, got {edits:?}"); + + Ok(()) +} diff --git a/docs/basedpython/features/editor.md b/docs/basedpython/features/editor.md index 879b58de70..367ae35ce3 100644 --- a/docs/basedpython/features/editor.md +++ b/docs/basedpython/features/editor.md @@ -228,3 +228,51 @@ between: one frame's `limit` is not another's, and a name that scope does not itself bind — a global it only reads, an attribute it never assigns — is a value nothing in that scope can vouch for + +## renaming a module + +renaming `util.by` to `helpers.by` renames the module `alpha.util`, and every +`from alpha.util import thing` in the project now names a module that is not +there. an editor cannot find those on its own — it would have to resolve every +import against the same search paths the checker uses — so it asks first + +the request is the protocol's own `workspace/willRenameFiles`, sent *before* the +file moves, and the answer is the edits that keep the project working. the +ordering is what makes the answer computable: the old path still holds the file, +so the module it is today can be resolved, and the new path is a path to read a +name out of + +both a file and a *directory* are asked about, because renaming a directory +renames every module under it — the client sends only the directory, never its +contents + +what gets rewritten: + +```by +import alpha.util # the dotted name +import alpha.util as util # ... with an alias, which is left alone +from alpha.util import thing # the module a symbol comes from +from alpha import util # the module imported as a name +from .util import thing # a relative import, after the dots +``` + +and the *uses* of a name an import binds, when that name changes. +`import alpha.util` binds `alpha`, so `alpha.util.thing()` in the body is part of +the rename too. those are found by asking what each expression is rather than by +matching text — a local called `util` in a file that also imports a module of +that name is not a reference to the module, and is not touched + +a relative import inside a package that is being renamed as a whole comes out +unchanged, which is the truth: nothing about `from .util import thing` stops +working because its package was renamed around it + +two things are deliberately not rewritten: + +- **a module named as a string** — `importlib.import_module("alpha.util")`, an + `INSTALLED_APPS` entry, an entry point in `pyproject.toml`. django's own + module strings are handled by the django rename, and the rest are not + distinguishable from any other string +- **an import that would have to change shape** — moving `alpha.util` to + `beta.util` leaves `from alpha import util` needing a different statement, + not a different word. those are reported in the server log rather than + rewritten into something that does not mean the same thing