mirror of
https://github.com/astral-sh/ruff.git
synced 2026-10-28 10:45:37 +08:00
[ty] Fix Goto definition in script dependencies (#27747)
## Summary Each script with inline metadata (PEP 723) gets its own virtual environment with its own dependencies. This is to guarantee isolation. Before for this PR, go to definition only worked when resolving an import directly in a script. E.g. `from mypyp_primer.projects import get_projects`, clicking on projects resolved successfully. However, all go to definition requests within `mypy_primer.projects` failed (unless the main project had `mypy_primer` installed as a dependency). The root cause of this was in `db.program_file(db, file)`. The method's responsibility is to resolve the most likely program to use for `file`. Today, `db.program_file(db, file)` only handled two cases: * It's a first-party and it has inline metadata (PEP 723). Use the script's `Program`. This is why clicking on `mypy_primer.projects` worked from within the script * Otherwise, use the `Project`'s program Because of that, any script dependency used the `Project`'s `Program` instead of the script's. This PR extends `db.program_file(db, file)` with a heuristic for third-party files (it's important that `db.program_file(db, file)` is accurate for first-party files, for third-party files, the best we can do is "guess"). That is, iterate over all known `Program`'s and use the `Program` for which file appears on the search paths. Use `Project` as the fallback (still the best guess if everything else fails). This operation isn't expensive but it also isn't cheap. However, we only call `db.program_file` at API boundaries. That is, we mainly call it once for each LSP request. There are a few where the method is called more than once (2 or three times, constant factor), but these are all first-party files, for which `db.program_file` keeps using the fast-path. That's why I don't think it's justified to Salsa cache this resolution. But we can if this turns out to be a performance bottleneck in the future. Closes https://github.com/astral-sh/ty/issues/4253 ## Test Plan https://github.com/user-attachments/assets/5cff9f33-cb54-46d9-9f33-82f1ee63999b Added language-server integration coverage and ran the affected test suites and Clippy.
This commit is contained in:
1 parent
754d633bd7
commit
c7fe174832
2 files changed
+79
-18
No files matched your search
+40
-12
@@ -15,6 +15,7 @@ use ruff_db::files::{File, Files};
|
||||
use ruff_db::system::System;
|
||||
use ruff_db::vendored::VendoredFileSystem;
|
||||
use salsa::{Database, Event, Setter};
|
||||
use ty_module_resolver::system_module_search_paths;
|
||||
use ty_python_core::ProgramFile;
|
||||
use ty_python_core::program::{FallibleStrategy, MisconfigurationStrategy, UseDefaultStrategy};
|
||||
use ty_python_semantic::lint::{LintRegistry, RuleSelection};
|
||||
@@ -31,6 +32,43 @@ pub trait Db: SemanticDb {
|
||||
fn dyn_clone(&self) -> Box<dyn Db>;
|
||||
}
|
||||
|
||||
/// Returns the program to use for `file`.
|
||||
///
|
||||
/// Scripts use their own program, and project files use the project program. For third-party files,
|
||||
/// this chooses the most likely program.
|
||||
fn program_file(db: &dyn Db, file: File) -> ProgramFile<'_> {
|
||||
if let Some(script) = Script::for_file(db, file) {
|
||||
return script.program(db).program_file(db, file);
|
||||
}
|
||||
|
||||
let project = db.project();
|
||||
let project_program = project.program(db);
|
||||
let Some(path) = file.path(db).as_system_path() else {
|
||||
return project_program.program_file(db, file);
|
||||
};
|
||||
|
||||
if project.is_file_included(db, path).is_included()
|
||||
|| system_module_search_paths(db, project_program.resolver_environment(db))
|
||||
.any(|search_path| path.starts_with(search_path))
|
||||
{
|
||||
return project_program.program_file(db, file);
|
||||
}
|
||||
|
||||
let program = db
|
||||
.script_environments()
|
||||
.files()
|
||||
.into_iter()
|
||||
.filter_map(|script| Script::for_file(db, script))
|
||||
.map(|script| script.program(db))
|
||||
.find(|program| {
|
||||
system_module_search_paths(db, program.resolver_environment(db))
|
||||
.any(|search_path| path.starts_with(search_path))
|
||||
})
|
||||
.unwrap_or(project_program);
|
||||
|
||||
program.program_file(db, file)
|
||||
}
|
||||
|
||||
/// Tracked so that a change to the open-file set only invalidates queries
|
||||
/// for files whose open state actually changed.
|
||||
#[salsa::tracked(heap_size=ruff_memory_usage::heap_size, returns(copy))]
|
||||
@@ -533,12 +571,7 @@ impl SemanticDb for ProjectDatabase {
|
||||
}
|
||||
|
||||
fn program_file(&self, file: File) -> ProgramFile<'_> {
|
||||
let program = match Script::for_file(self, file) {
|
||||
None => self.project().program(self),
|
||||
Some(script) => script.program(self),
|
||||
};
|
||||
|
||||
program.program_file(self, file)
|
||||
program_file(self, file)
|
||||
}
|
||||
|
||||
fn python_version_with_source(&self, file: File) -> &PythonVersionWithSource {
|
||||
@@ -796,12 +829,7 @@ pub(crate) mod testing {
|
||||
#[salsa::db]
|
||||
impl ty_python_semantic::Db for TestDb {
|
||||
fn program_file(&self, file: File) -> ProgramFile<'_> {
|
||||
let program = match Script::for_file(self, file) {
|
||||
None => self.project().program(self),
|
||||
Some(script) => script.program(self),
|
||||
};
|
||||
|
||||
program.program_file(self, file)
|
||||
super::program_file(self, file)
|
||||
}
|
||||
|
||||
fn python_version_with_source(&self, file: File) -> &PythonVersionWithSource {
|
||||
|
||||
@@ -59,12 +59,12 @@ from dependency import script_only
|
||||
|
||||
#[cfg(feature = "test-uv")]
|
||||
mod uv_metadata {
|
||||
use anyhow::Result;
|
||||
use anyhow::{Context, Result, anyhow};
|
||||
use lsp_types::{
|
||||
FileChangeType, FileEvent, Position, TextDocumentContentChangeEvent,
|
||||
TextDocumentContentChangeWholeDocument,
|
||||
Definition, DefinitionResponse, FileChangeType, FileEvent, Position,
|
||||
TextDocumentContentChangeEvent, TextDocumentContentChangeWholeDocument,
|
||||
};
|
||||
use ruff_db::system::{OsSystem, System as _, SystemPath};
|
||||
use ruff_db::system::{OsSystem, System as _, SystemPath, SystemPathBuf};
|
||||
use ty_project::UseUv;
|
||||
|
||||
use crate::TestServerBuilder;
|
||||
@@ -107,8 +107,41 @@ from attrs import define
|
||||
assert_eq!(end.token, progress.token);
|
||||
let _: lsp_types::WorkDoneProgressEnd = serde_json::from_value(end.value)?;
|
||||
|
||||
let definition = server.goto_definition_request(script, Position::new(4, 18));
|
||||
assert!(definition.is_some(), "expected attrs.define to resolve");
|
||||
let definition = server
|
||||
.goto_definition_request(script, Position::new(4, 18))
|
||||
.context("expected attrs.define to resolve")?;
|
||||
let DefinitionResponse::Definition(Definition::LocationList(locations)) = definition else {
|
||||
return Err(anyhow!("expected dependency definition locations"));
|
||||
};
|
||||
let location = locations
|
||||
.first()
|
||||
.context("expected attrs.define definition location")?;
|
||||
let path = location
|
||||
.uri
|
||||
.to_file_path()
|
||||
.map_err(|()| anyhow!("expected dependency definition to have a file URI"))?;
|
||||
let dependency = SystemPathBuf::from_path_buf(path)
|
||||
.map_err(|path| anyhow!("dependency path is not valid UTF-8: {}", path.display()))?;
|
||||
let source = std::fs::read_to_string(dependency.as_std_path())?;
|
||||
let (line, import) = source
|
||||
.lines()
|
||||
.enumerate()
|
||||
.find(|(_, line)| line.starts_with("from attr import Attribute "))
|
||||
.with_context(|| {
|
||||
format!("expected attrs to import attr.Attribute in `{dependency}`")
|
||||
})?;
|
||||
let character = import
|
||||
.find("Attribute")
|
||||
.context("expected attrs to import Attribute")?;
|
||||
let position = Position::new(u32::try_from(line)?, u32::try_from(character)?);
|
||||
|
||||
server.open_text_document(&dependency, &source, 1);
|
||||
assert!(
|
||||
server
|
||||
.goto_definition_request(&dependency, position)
|
||||
.is_some(),
|
||||
"expected imports inside the dependency to use the script's environment"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user