Skip to content

Commit 4f753f6

Browse files
committed
answer for the imports a renamed module leaves behind
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.
1 parent 4753a20 commit 4f753f6

13 files changed

Lines changed: 1266 additions & 0 deletions

File tree

crates/ty_ide/src/lib.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ mod hover;
2525
mod importer;
2626
mod inlay_hints;
2727
mod markup;
28+
mod module_rename;
2829
mod references;
2930
mod rename;
3031
mod selection_range;
@@ -69,6 +70,9 @@ pub use inlay_hints::{
6970
InlayHintKind, InlayHintLabel, InlayHintSettings, InlayHintTextEdit, inlay_hints,
7071
};
7172
pub use markup::MarkupKind;
73+
pub use module_rename::{
74+
FileMove, ModuleRenameEdits, SkipReason, SkippedImport, module_rename_edits,
75+
};
7276
pub use references::ReferencesMode;
7377
pub use rename::{can_rename, rename};
7478
pub use selection_range::selection_range;

crates/ty_ide/src/module_rename.rs

Lines changed: 800 additions & 0 deletions
Large diffs are not rendered by default.

crates/ty_module_resolver/src/lib.rs

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,52 @@ type FxOrderMap<K, V> = ordermap::map::OrderMap<K, V, BuildHasherDefault<FxHashe
4545
#[cfg(test)]
4646
mod testing;
4747

48+
/// The name a module at `path` would have, whether or not anything is there yet.
49+
///
50+
/// [`file_to_module`] answers this for a file the system already knows about, and it does more than
51+
/// convert a path: it resolves the name it derived back to a file and checks that the answer is the
52+
/// same file, so that a `src/foo.py` sitting beside a `src/foo/__init__.py` is correctly reported as
53+
/// *not* being the module `foo`.
54+
///
55+
/// That check is exactly what cannot be done for a path nothing is at. This answers the narrower,
56+
/// purely path-shaped question — which search path covers it, and what does the rest of the path
57+
/// spell — which is what an editor asks when it is about to *move* a file: at that moment the old
58+
/// path still holds the file and the new path holds nothing, and both names are needed to work out
59+
/// what the move costs.
60+
///
61+
/// Works for a directory as well as a file, because a package is a directory: `src/foo/bar` and
62+
/// `src/foo/bar.py` both give `foo.bar`.
63+
///
64+
/// # Which search path names it
65+
///
66+
/// The **deepest** one that contains it, not the first one consulted. Search paths nest all the
67+
/// time — a project root with a `src/` layout under it, and, in a uv workspace, one editable entry
68+
/// per member pointing at that member's own `src` — so a member's package is inside two of them at
69+
/// once. Taking the first would name `packages/alpha/src/alpha` after the project root, as
70+
/// `packages.alpha.src.alpha`, which is not a module anything can import and not the name any
71+
/// `import alpha` in the project resolves to. The deepest entry is the one whose name resolves back
72+
/// to that path, which is what [`file_to_module`] verifies for a file that exists.
73+
pub fn path_to_module_name<'db>(
74+
db: &'db dyn Db,
75+
resolver_environment: ResolverEnvironment<'db>,
76+
path: &SystemPath,
77+
) -> Option<ModuleName> {
78+
// `Typing` mode for the reason `system_module_search_paths` gives below: the question is which
79+
// paths belong to the project at all, not which of two stdlib variants a name resolves to.
80+
search_paths(db, resolver_environment, ModuleResolveMode::Typing)
81+
.filter_map(|search_path| {
82+
let name = search_path.relativize_system_path(path)?.to_module_name()?;
83+
// How much of `path` this search path accounts for. A vendored path accounts for none of
84+
// it and sorts last, which is right: nothing under the project is named by the stdlib.
85+
let depth = search_path
86+
.as_system_path()
87+
.map_or(0, |root| root.as_str().len());
88+
Some((depth, name))
89+
})
90+
.max_by_key(|(depth, _)| *depth)
91+
.map(|(_, name)| name)
92+
}
93+
4894
/// Returns an iterator over all search paths pointing to a system path
4995
pub fn system_module_search_paths<'db>(
5096
db: &'db dyn Db,
@@ -76,3 +122,92 @@ impl<'db> Iterator for SystemModuleSearchPathsIter<'db> {
76122
}
77123

78124
impl FusedIterator for SystemModuleSearchPathsIter<'_> {}
125+
126+
#[cfg(test)]
127+
mod tests {
128+
use ruff_db::Db as _;
129+
use ruff_db::system::{DbWithWritableSystem as _, SystemPathBuf};
130+
131+
use crate::db::tests::TestDb;
132+
use crate::settings::SearchPathSettings;
133+
use crate::strategy::FallibleStrategy;
134+
135+
use super::*;
136+
137+
/// A project whose search paths nest, which is what a uv workspace's editable installs produce:
138+
/// the project root, and one entry per member pointing at that member's own `src`.
139+
fn workspace() -> TestDb {
140+
let project = SystemPathBuf::from("/project");
141+
let member_src = project.join("packages/alpha/src");
142+
143+
let mut db = TestDb::new();
144+
db.write_file(member_src.join("alpha/__init__.py"), "")
145+
.unwrap();
146+
147+
let search_paths = SearchPathSettings {
148+
src_roots: vec![project],
149+
extra_paths: vec![member_src],
150+
..SearchPathSettings::empty()
151+
}
152+
.to_search_paths(db.system(), db.vendored(), &FallibleStrategy)
153+
.expect("valid search path settings");
154+
db.set_search_paths(search_paths);
155+
db
156+
}
157+
158+
/// The bug this rule exists for: the project root also contains the member's package, and naming
159+
/// it from there gives `packages.alpha.src.alpha` — a name nothing imports and nothing resolves.
160+
#[test]
161+
fn a_path_is_named_by_the_deepest_search_path_that_contains_it() {
162+
let db = workspace();
163+
assert_eq!(
164+
path_to_module_name(
165+
&db,
166+
db.resolver_environment(),
167+
SystemPath::new("/project/packages/alpha/src/alpha"),
168+
),
169+
ModuleName::new("alpha"),
170+
);
171+
}
172+
173+
#[test]
174+
fn a_module_inside_a_package_is_named_under_it() {
175+
let db = workspace();
176+
assert_eq!(
177+
path_to_module_name(
178+
&db,
179+
db.resolver_environment(),
180+
SystemPath::new("/project/packages/alpha/src/alpha/util.py"),
181+
),
182+
ModuleName::new("alpha.util"),
183+
);
184+
}
185+
186+
/// The point of asking about a path rather than a file: at the moment an editor asks, the answer
187+
/// is about somewhere nothing has been written yet.
188+
#[test]
189+
fn a_path_nothing_is_at_still_has_a_name() {
190+
let db = workspace();
191+
assert_eq!(
192+
path_to_module_name(
193+
&db,
194+
db.resolver_environment(),
195+
SystemPath::new("/project/packages/alpha/src/gamma"),
196+
),
197+
ModuleName::new("gamma"),
198+
);
199+
}
200+
201+
#[test]
202+
fn a_path_no_search_path_covers_is_not_a_module() {
203+
let db = workspace();
204+
assert_eq!(
205+
path_to_module_name(
206+
&db,
207+
db.resolver_environment(),
208+
SystemPath::new("/elsewhere/thing.py")
209+
),
210+
None,
211+
);
212+
}
213+
}

crates/ty_server/src/capabilities.rs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,10 @@ pub(crate) fn server_capabilities(
572572
supported: Some(true),
573573
change_notifications: Some(true.into()),
574574
}),
575+
file_operations: Some(lsp_types::FileOperationOptions {
576+
will_rename: Some(will_rename_registration()),
577+
..Default::default()
578+
}),
575579
..Default::default()
576580
}),
577581
type_hierarchy_provider: Some(true.into()),
@@ -580,6 +584,44 @@ pub(crate) fn server_capabilities(
580584
}
581585
}
582586

587+
/// Which renames the client should ask about before it carries them out.
588+
///
589+
/// Two filters, and both are needed for the same feature. A **file** rename is a module renamed —
590+
/// every module lives in one file, and the extensions listed are the ones the module resolver
591+
/// accepts. A **folder** rename is a *package* renamed, which renames every module inside it, and
592+
/// the client sends only the folder (never its contents), so a server that asked for files alone
593+
/// would be told nothing at all about the rename that changes the most names.
594+
///
595+
/// The folder pattern cannot be narrowed the way the file one is: a directory has no extension to
596+
/// match on, and whether it is a package is a question about the search paths rather than about its
597+
/// name. So every folder rename is asked about, and the ones that turn out not to be packages cost
598+
/// one request that answers with no edits.
599+
fn will_rename_registration() -> lsp_types::FileOperationRegistrationOptions {
600+
fn filter(
601+
glob: &str,
602+
kind: lsp_types::FileOperationPatternKind,
603+
) -> lsp_types::FileOperationFilter {
604+
lsp_types::FileOperationFilter {
605+
scheme: Some("file".to_string()),
606+
pattern: lsp_types::FileOperationPattern {
607+
glob: glob.to_string(),
608+
matches: Some(kind),
609+
options: None,
610+
},
611+
}
612+
}
613+
614+
lsp_types::FileOperationRegistrationOptions {
615+
filters: vec![
616+
filter(
617+
"**/*.{py,pyi,by,byi}",
618+
lsp_types::FileOperationPatternKind::File,
619+
),
620+
filter("**", lsp_types::FileOperationPatternKind::Folder),
621+
],
622+
}
623+
}
624+
583625
/// Creates the default [`DiagnosticOptions`] for the server.
584626
pub(crate) fn server_diagnostic_options(workspace_diagnostics: bool) -> DiagnosticOptions {
585627
DiagnosticOptions {

crates/ty_server/src/server/api.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,14 @@ pub(super) fn request(req: server::Request) -> Task {
136136
>(
137137
req, BackgroundSchedule::Worker
138138
),
139+
// The client is holding a file move open waiting for this, so it is scheduled like the
140+
// other things a person is watching for rather than as background work.
141+
requests::WillRenameFilesRequestHandler::METHOD => {
142+
background_request_task::<requests::WillRenameFilesRequestHandler>(
143+
req,
144+
BackgroundSchedule::LatencySensitive,
145+
)
146+
}
139147
requests::PrepareTypeHierarchyRequestHandler::METHOD => background_document_request_task::<
140148
requests::PrepareTypeHierarchyRequestHandler,
141149
>(

crates/ty_server/src/server/api/requests.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ mod signature_help;
4444
mod transpile;
4545
mod type_hierarchy_subtypes;
4646
mod type_hierarchy_supertypes;
47+
mod will_rename_files;
4748
mod workspace_diagnostic;
4849
mod workspace_symbols;
4950

@@ -79,5 +80,6 @@ pub(super) use signature_help::SignatureHelpRequestHandler;
7980
pub(super) use transpile::TranspileRequestHandler;
8081
pub(super) use type_hierarchy_subtypes::TypeHierarchySubtypesRequestHandler;
8182
pub(super) use type_hierarchy_supertypes::TypeHierarchySupertypesRequestHandler;
83+
pub(super) use will_rename_files::WillRenameFilesRequestHandler;
8284
pub(super) use workspace_diagnostic::WorkspaceDiagnosticRequestHandler;
8385
pub(super) use workspace_symbols::WorkspaceSymbolRequestHandler;
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
use lsp_types::{RenameFilesParams, TextEdit, Uri, WillRenameFilesRequest, WorkspaceEdit};
2+
use ruff_db::files::FileRange;
3+
use ruff_db::system::SystemPathBuf;
4+
use ruff_text_size::Ranged;
5+
use rustc_hash::FxHashMap;
6+
use ty_ide::{FileMove, module_rename_edits};
7+
8+
use crate::document::FileRangeExt;
9+
use crate::server::api::traits::{
10+
BackgroundRequestHandler, RequestHandler, RetriableRequestHandler,
11+
};
12+
use crate::session::SessionSnapshot;
13+
use crate::session::client::Client;
14+
15+
/// `workspace/willRenameFiles` — the edits that keep imports working across a rename the editor is
16+
/// about to perform.
17+
///
18+
/// The client asks *before* it moves anything, applies whatever comes back, and only then does the
19+
/// move. That ordering is what makes the answer computable at all: the old path still holds the
20+
/// file, so the module it is today can be resolved, while the new path is just a path — see
21+
/// [`ty_module_resolver::path_to_module_name`].
22+
///
23+
/// A rename that changes no module's name — a `README.md`, a directory no search path covers —
24+
/// answers `None` rather than an empty edit, which is what tells the client to get on with the move
25+
/// without showing the user an empty preview.
26+
pub(crate) struct WillRenameFilesRequestHandler;
27+
28+
impl RequestHandler for WillRenameFilesRequestHandler {
29+
type RequestType = WillRenameFilesRequest;
30+
}
31+
32+
impl BackgroundRequestHandler for WillRenameFilesRequestHandler {
33+
fn run(
34+
snapshot: &SessionSnapshot,
35+
_client: &Client,
36+
params: RenameFilesParams,
37+
) -> crate::server::Result<Option<WorkspaceEdit>> {
38+
let moves: Vec<FileMove> = params
39+
.files
40+
.iter()
41+
.filter_map(|rename| {
42+
Some(FileMove {
43+
old_path: system_path(&rename.old_uri)?,
44+
new_path: system_path(&rename.new_uri)?,
45+
})
46+
})
47+
.collect();
48+
49+
if moves.is_empty() {
50+
return Ok(None);
51+
}
52+
53+
let mut changes: FxHashMap<Uri, Vec<TextEdit>> = FxHashMap::default();
54+
55+
// Every project, because a rename in one workspace folder can move a module that another
56+
// folder imports; a project the paths have nothing to do with contributes nothing, since
57+
// the paths resolve to no module of its.
58+
for db in snapshot.projects() {
59+
let result = module_rename_edits(db, &moves);
60+
61+
for skipped in &result.skipped {
62+
// Not an error: the rename can still go ahead, and this is the one import the user
63+
// will have to look at themselves. Logged with its location so that "which line?"
64+
// has an answer that does not involve searching the project.
65+
tracing::info!(
66+
"willRenameFiles: leaving an import in {} alone ({:?}); it would need a \
67+
different statement to name the module's new home",
68+
skipped.file.path(db),
69+
skipped.reason,
70+
);
71+
}
72+
73+
for file_edit in result.edits {
74+
let range = FileRange::new(file_edit.file, file_edit.edit.range());
75+
let Some(location) = range
76+
.to_lsp_range(db, snapshot.position_encoding())
77+
.and_then(|range| range.to_location())
78+
else {
79+
continue;
80+
};
81+
changes.entry(location.uri).or_default().push(TextEdit {
82+
range: location.range,
83+
new_text: file_edit.edit.content().unwrap_or_default().to_string(),
84+
});
85+
}
86+
}
87+
88+
if changes.is_empty() {
89+
return Ok(None);
90+
}
91+
92+
Ok(Some(WorkspaceEdit {
93+
changes: Some(changes.into_iter().collect()),
94+
document_changes: None,
95+
change_annotations: None,
96+
}))
97+
}
98+
}
99+
100+
/// The path a `file:` URI names, or nothing for a URI that is not one.
101+
///
102+
/// A client may send `untitled:` for a buffer that has never been saved, which cannot be a module
103+
/// and cannot be moved; those are dropped rather than refused, so a mixed rename still gets the
104+
/// edits for the files that do exist.
105+
fn system_path(uri: &Uri) -> Option<SystemPathBuf> {
106+
SystemPathBuf::from_path_buf(uri.to_file_path().ok()?).ok()
107+
}
108+
109+
impl RetriableRequestHandler for WillRenameFilesRequestHandler {
110+
/// A rename is a one-shot gesture the user is waiting on, and the client is holding the file
111+
/// move until it answers. Retrying on a database change is the right trade here for the same
112+
/// reason it is for the other whole-project requests: the alternative is telling the editor to
113+
/// go ahead with a rename this never got to check.
114+
const RETRY_ON_CANCELLATION: bool = true;
115+
}

crates/ty_server/tests/e2e/main.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ mod rename;
4646
mod semantic_tokens;
4747
mod signature_help;
4848
mod type_hierarchy;
49+
mod will_rename_files;
4950
mod workspace_folders;
5051

5152
use std::collections::{BTreeMap, HashMap, VecDeque};

0 commit comments

Comments
 (0)