mirror of
https://github.com/astral-sh/ruff.git
synced 2026-10-28 10:45:37 +08:00
[ty] Synchronize saved script metadata on open (#27873)
## Summary ty supports in-memory changes, but uv always reads the script's content from disk. This discrepancy could lead to ty skipping a sync when, between opening a file and saving it, the script metadata block wasn't changed, but it changed compared to what was saved on disk when the file was opened. The root cause of this issue is that ty computes the script-fingerprint from the current file content (which may include in-memory changes). Instead, it should compute the fingerprint as it is seen on disk. I considered a few different approaches: * Support reading the persistent content in `ruff_db`: Add a new `persisted_source_text` query, track the persisted `File` revision and the in-memory revision separately. `ScriptsEnvironments` can then compute the fingerprint based on the persisted source text. * Use `system.read_text` in `ScriptsEnvironments`. I disregarded that quickly because we proactively trigger the synchronization of all scripts in a few places and we rely on this bailing quickly if the files are unchanged (we don't want to read each script to bail). * Introduce a new `Force` sync mode and use it in `didSave`. I'd pick the first approach if we had multiple frontends that supported in-memory changes. But, given that it's only the LSP, I started with the `Force` sync mode, and I almost opened a PR for it. During the final cleanups, I realized that there's a much simpler LSP-only fix (the CLI doesn't have this problem). We don't need a `persisted_source_text` query if we schedule the script synchronization before we set the Script's in-memory content in `didOpen`. That is, first trigger the synchronization, then open the file and load its in-memory content. That's all we need. Fixes astral-sh/ty#4250. ## Test Plan **Before** Reload VS Code with unsaved changes, hit save. Note how the script block isn't synced (because we didn't make any changes to it) https://github.com/user-attachments/assets/cd050b94-08a3-4c61-ba14-4a3f1b7a1e48 **This PR** Same sequence, but the sync runs. https://github.com/user-attachments/assets/c8acc477-7666-4411-a9b4-df33328ccb7e
This commit is contained in:
1 parent
c7fe174832
commit
5b1b3fb240
11 files changed
+230
-78
No files matched your search
@@ -123,20 +123,24 @@ impl ProjectDatabase {
|
||||
}
|
||||
|
||||
match change {
|
||||
ChangeEvent::Changed { path, kind: _ } | ChangeEvent::Opened(path) => {
|
||||
ChangeEvent::Changed { path, kind: _ } => {
|
||||
if synced_files.insert(path.to_path_buf()) {
|
||||
File::sync_path_only(self, path);
|
||||
}
|
||||
}
|
||||
|
||||
ChangeEvent::Created { kind, path } => {
|
||||
match kind {
|
||||
CreatedKind::File => {
|
||||
ChangeEvent::Opened(path) | ChangeEvent::Created { path, .. } => {
|
||||
match change {
|
||||
ChangeEvent::Opened(_)
|
||||
| ChangeEvent::Created {
|
||||
kind: CreatedKind::File,
|
||||
..
|
||||
} => {
|
||||
if synced_files.insert(path.to_path_buf()) {
|
||||
File::sync_path(self, path);
|
||||
}
|
||||
}
|
||||
CreatedKind::Directory | CreatedKind::Any => {
|
||||
_ => {
|
||||
sync_recursively.insert(path.clone());
|
||||
}
|
||||
}
|
||||
@@ -160,7 +164,8 @@ impl ProjectDatabase {
|
||||
{
|
||||
project.add_file(self, file);
|
||||
}
|
||||
} else if project.is_directory_included(self, path)
|
||||
} else if change.is_created()
|
||||
&& project.is_directory_included(self, path)
|
||||
&& ignore_files
|
||||
.as_mut()
|
||||
.is_none_or(|ignore_files| !ignore_files.is_ignored(path, true))
|
||||
|
||||
@@ -123,8 +123,9 @@ impl ScriptEnvironments {
|
||||
/// the change, increasing CLI and language-server latency.
|
||||
///
|
||||
/// Instead, project checks initialize virtual environments lazily as they encounter scripts.
|
||||
/// If no [`ScriptEnvironment`] exists, this method synchronizes the virtual environment and
|
||||
/// waits for uv before creating the input. An existing [`ScriptEnvironment`] is reused.
|
||||
/// For a closed file without a [`ScriptEnvironment`], this method synchronizes the virtual
|
||||
/// environment and waits for uv before creating the input. An existing environment is reused.
|
||||
/// Open files without an environment use the default environment until they are saved.
|
||||
///
|
||||
/// Background synchronization works differently: it creates the [`ScriptEnvironment`] input
|
||||
/// before invoking uv so other operations can use it while synchronization runs. Applying the
|
||||
@@ -173,6 +174,13 @@ impl ScriptEnvironments {
|
||||
state = entry.wait_until_initialized(db, state);
|
||||
}
|
||||
|
||||
ScriptEnvironmentState::Vacant if db.is_open_file(file) => {
|
||||
// An unsaved edit added a script metadata block to an ordinary file.
|
||||
// Keep diagnostics available with the default environment, as for other
|
||||
// unsaved edits. Synchronize on save, when uv can read the metadata from disk.
|
||||
return ScriptEnvironmentAvailability::Available;
|
||||
}
|
||||
|
||||
ScriptEnvironmentState::Vacant => {
|
||||
let cache_key = task.request.cache_key();
|
||||
let claim = InitializationClaim::new(&entry, state, cache_key);
|
||||
|
||||
@@ -22,7 +22,10 @@ mod watcher;
|
||||
/// event instead of emitting an event for each file or subdirectory in that path.
|
||||
#[derive(Debug, PartialEq, Eq)]
|
||||
pub enum ChangeEvent {
|
||||
/// The file corresponding to the given path was opened in an editor.
|
||||
/// A new or existing file was opened in an editor.
|
||||
///
|
||||
/// Refresh its metadata and add it to an existing project index if it is included.
|
||||
/// The editor may open a file before its filesystem creation notification arrives.
|
||||
Opened(SystemPathBuf),
|
||||
|
||||
/// A new path was created
|
||||
@@ -88,7 +91,7 @@ impl ChangeEvent {
|
||||
matches!(self, ChangeEvent::Rescan)
|
||||
}
|
||||
|
||||
pub const fn is_created(&self) -> bool {
|
||||
pub(crate) const fn is_created(&self) -> bool {
|
||||
matches!(self, ChangeEvent::Created { .. })
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
use lsp_types::{DidOpenTextDocumentNotification, DidOpenTextDocumentParams, TextDocumentItem};
|
||||
use ty_project::ScriptEnvironmentAvailability;
|
||||
|
||||
use crate::TextDocument;
|
||||
use crate::server::Result;
|
||||
@@ -31,13 +30,7 @@ impl SyncNotificationHandler for DidOpenTextDocumentHandler {
|
||||
} = params;
|
||||
|
||||
let text_doc = TextDocument::new(uri, text, version, language_id);
|
||||
let document = session.open_text_document(text_doc);
|
||||
// Defer diagnostics until synchronization provides the script's declared dependencies.
|
||||
session.synchronize_script(
|
||||
client,
|
||||
document.notebook_or_file_path(),
|
||||
ScriptEnvironmentAvailability::Pending,
|
||||
);
|
||||
let document = session.open_text_document(client, text_doc);
|
||||
publish_diagnostics_if_needed(&document, session, client);
|
||||
|
||||
Ok(())
|
||||
|
||||
@@ -41,7 +41,7 @@ impl SyncNotificationHandler for DidOpenNotebookHandler {
|
||||
let cell_document =
|
||||
TextDocument::new(cell.uri, cell.text, cell.version, cell.language_id)
|
||||
.with_notebook(notebook_path.clone());
|
||||
session.open_text_document(cell_document);
|
||||
session.open_text_document(client, cell_document);
|
||||
}
|
||||
|
||||
// Always publish diagnostics because notebooks only support publish diagnostics.
|
||||
|
||||
@@ -21,11 +21,7 @@ impl SyncNotificationHandler for DidSaveTextDocumentHandler {
|
||||
) -> Result<()> {
|
||||
if let Ok(document) = session.document_handle(¶ms.text_document.uri) {
|
||||
// Keep diagnostics visible if unsaved edits first turned this file into a script.
|
||||
session.synchronize_script(
|
||||
client,
|
||||
document.notebook_or_file_path(),
|
||||
ScriptEnvironmentAvailability::Available,
|
||||
);
|
||||
document.synchronize_script(session, client, ScriptEnvironmentAvailability::Available);
|
||||
}
|
||||
|
||||
for document in session.file_document_handles() {
|
||||
|
||||
@@ -21,9 +21,10 @@ use ruff_db::Db;
|
||||
use ruff_db::files::{File, system_path_to_file};
|
||||
use ruff_db::system::{System, SystemPath, SystemPathBuf};
|
||||
use ruff_python_ast::PySourceType;
|
||||
use salsa::Database as _;
|
||||
use ty_combine::Combine;
|
||||
use ty_project::metadata::Options;
|
||||
use ty_project::watch::{ChangeEvent, CreatedKind};
|
||||
use ty_project::watch::ChangeEvent;
|
||||
use ty_project::{
|
||||
ChangeResult, Db as _, ProjectDatabase, ProjectMetadata, ScriptEnvironmentAvailability,
|
||||
SemanticDb as _, UseUv,
|
||||
@@ -264,24 +265,6 @@ impl Session {
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Synchronizes an open script, using `availability` until its first synchronization completes.
|
||||
pub(crate) fn synchronize_script(
|
||||
&mut self,
|
||||
client: &Client,
|
||||
path: &AnySystemPath,
|
||||
availability: ScriptEnvironmentAvailability,
|
||||
) {
|
||||
let Some(system_path) = path.as_system() else {
|
||||
return;
|
||||
};
|
||||
let capabilities = self.resolved_client_capabilities;
|
||||
let db = self.project_db_mut(path);
|
||||
let Some(file) = db.files().try_system(db, system_path) else {
|
||||
return;
|
||||
};
|
||||
Self::request_script_sync(db, file, client, capabilities, availability);
|
||||
}
|
||||
|
||||
/// Resynchronizes closed scripts whose metadata changed after filesystem events.
|
||||
pub(crate) fn synchronize_closed_scripts(&mut self, client: &Client) {
|
||||
let capabilities = self.resolved_client_capabilities;
|
||||
@@ -1356,9 +1339,44 @@ impl Session {
|
||||
/// Registers a text document at the provided `path`.
|
||||
/// If a document is already open here, it will be overwritten.
|
||||
///
|
||||
/// Starts script synchronization from the backing file before installing the editor contents.
|
||||
///
|
||||
/// Returns a handle to the opened document.
|
||||
pub(crate) fn open_text_document(&mut self, document: TextDocument) -> DocumentHandle {
|
||||
pub(crate) fn open_text_document(
|
||||
&mut self,
|
||||
client: &Client,
|
||||
document: TextDocument,
|
||||
) -> DocumentHandle {
|
||||
let language_id = document.language_id();
|
||||
|
||||
// Request synchronization before installing the editor contents because uv reads the
|
||||
// script from disk. This ensures both use the saved metadata, so saving changed metadata
|
||||
// requests another synchronization.
|
||||
if self.use_uv != UseUv::Off
|
||||
&& language_id == LanguageId::Python
|
||||
&& document.notebook().is_none()
|
||||
&& let DocumentKey::File(system_path) = DocumentKey::from_uri(document.uri())
|
||||
{
|
||||
let capabilities = self.resolved_client_capabilities;
|
||||
let db = self.project_db_mut(&AnySystemPath::System(system_path.clone()));
|
||||
|
||||
// Refresh any cached disk revision before reading the script tag. A filesystem
|
||||
// change may not have reached the watcher yet, and the later open event will
|
||||
// refresh the file from the editor contents instead.
|
||||
File::sync_path(db, &system_path);
|
||||
if let Ok(file) = system_path_to_file(db, &system_path) {
|
||||
// Keep the synchronization alive when the editor buffer is installed. Cancel any
|
||||
// blocking initialization now instead of reusing one that index_mut would cancel.
|
||||
db.trigger_cancellation();
|
||||
Self::request_script_sync(
|
||||
db,
|
||||
file,
|
||||
client,
|
||||
capabilities,
|
||||
ScriptEnvironmentAvailability::Pending,
|
||||
);
|
||||
}
|
||||
}
|
||||
let handle = self.index_mut().open_text_document(document);
|
||||
self.open_document_in_db(&handle, Some(language_id));
|
||||
handle
|
||||
@@ -1367,16 +1385,6 @@ impl Session {
|
||||
fn open_document_in_db(&mut self, document: &DocumentHandle, language_id: Option<LanguageId>) {
|
||||
let path = document.notebook_or_file_path();
|
||||
|
||||
// This is a "maybe" because the `File` might've not been interned yet i.e., the
|
||||
// `try_system` call will return `None` which doesn't mean that the file is new, it's just
|
||||
// that the server didn't need the file yet.
|
||||
let is_maybe_new_system_file = path.as_system().is_some_and(|system_path| {
|
||||
let db = self.project_db(path);
|
||||
db.files()
|
||||
.try_system(db, system_path)
|
||||
.is_none_or(|file| !file.exists(db))
|
||||
});
|
||||
|
||||
// When we know the document isn't a Python source file
|
||||
// then we'll avoid adding it to the project. (But we
|
||||
// still track it as part of the index.)
|
||||
@@ -1384,15 +1392,7 @@ impl Session {
|
||||
|
||||
match path {
|
||||
AnySystemPath::System(system_path) => {
|
||||
let event = if is_maybe_new_system_file {
|
||||
ChangeEvent::Created {
|
||||
path: system_path.clone(),
|
||||
kind: CreatedKind::File,
|
||||
}
|
||||
} else {
|
||||
ChangeEvent::Opened(system_path.clone())
|
||||
};
|
||||
self.apply_changes(path, &[event]);
|
||||
self.apply_changes(path, &[ChangeEvent::Opened(system_path.clone())]);
|
||||
|
||||
if is_not_python {
|
||||
return;
|
||||
@@ -1962,6 +1962,25 @@ impl DocumentHandle {
|
||||
matches!(self, Self::Cell { .. } | Self::Notebook { .. })
|
||||
}
|
||||
|
||||
/// Synchronizes an open script, using `availability` until its first synchronization completes.
|
||||
pub(crate) fn synchronize_script(
|
||||
&self,
|
||||
session: &mut Session,
|
||||
client: &Client,
|
||||
availability: ScriptEnvironmentAvailability,
|
||||
) {
|
||||
let path = self.notebook_or_file_path();
|
||||
let Some(system_path) = path.as_system() else {
|
||||
return;
|
||||
};
|
||||
let capabilities = session.resolved_client_capabilities;
|
||||
let db = session.project_db_mut(path);
|
||||
let Some(file) = db.files().try_system(db, system_path) else {
|
||||
return;
|
||||
};
|
||||
Session::request_script_sync(db, file, client, capabilities, availability);
|
||||
}
|
||||
|
||||
pub(crate) fn update_text_document(
|
||||
&mut self,
|
||||
session: &mut Session,
|
||||
|
||||
@@ -61,11 +61,13 @@ from dependency import script_only
|
||||
mod uv_metadata {
|
||||
use anyhow::{Context, Result, anyhow};
|
||||
use lsp_types::{
|
||||
Definition, DefinitionResponse, FileChangeType, FileEvent, Position,
|
||||
Code, Definition, DefinitionResponse, FileChangeType, FileEvent, Position,
|
||||
TextDocumentContentChangeEvent, TextDocumentContentChangeWholeDocument,
|
||||
WorkspaceDocumentDiagnosticReport,
|
||||
};
|
||||
use ruff_db::system::{OsSystem, System as _, SystemPath, SystemPathBuf};
|
||||
use ty_project::UseUv;
|
||||
use ty_server::{ClientOptions, DiagnosticMode};
|
||||
|
||||
use crate::TestServerBuilder;
|
||||
|
||||
@@ -290,6 +292,109 @@ from idna import encode
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn saving_unsaved_open_metadata_repeats_synchronization() -> Result<()> {
|
||||
let script = SystemPath::new("src/script.py");
|
||||
let initial = r#"# /// script
|
||||
# requires-python = '>=3.12'
|
||||
# dependencies = []
|
||||
# ///
|
||||
"#;
|
||||
let updated = r#"# /// script
|
||||
# requires-python = '>=3.12'
|
||||
# dependencies = ['attrs==25.4.0']
|
||||
# ///
|
||||
from attrs import define
|
||||
"#;
|
||||
|
||||
let mut server = TestServerBuilder::new()?
|
||||
.with_workspace(SystemPath::new("src"), None)?
|
||||
.with_file(script, initial)?
|
||||
.with_use_uv(UseUv::Scripts)
|
||||
.with_env_var("UV", uv_executable()?)
|
||||
.enable_workspace_diagnostic_refresh(true)
|
||||
.build()
|
||||
.wait_until_workspaces_are_initialized();
|
||||
|
||||
// Opening synchronizes the backing file, not the unsaved metadata.
|
||||
server.open_text_document(script, updated, 1);
|
||||
server.await_diagnostic_refresh();
|
||||
assert!(
|
||||
server
|
||||
.goto_definition_request(script, Position::new(4, 18))
|
||||
.is_none(),
|
||||
"unsaved dependencies must not be installed"
|
||||
);
|
||||
|
||||
server.write_file(script, updated)?;
|
||||
server.save_text_document(script);
|
||||
server.await_diagnostic_refresh();
|
||||
assert!(
|
||||
server
|
||||
.goto_definition_request(script, Position::new(4, 18))
|
||||
.is_some(),
|
||||
"saving must install attrs even though the editor's metadata is unchanged"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn workspace_check_does_not_synchronize_unsaved_script_metadata() -> Result<()> {
|
||||
let script = SystemPath::new("src/script.py");
|
||||
let ordinary = "from attrs import define\nprint(define)\n";
|
||||
let source = r#"# /// script
|
||||
# requires-python = '>=3.12'
|
||||
# dependencies = ['attrs==25.4.0']
|
||||
# ///
|
||||
from attrs import define
|
||||
print(define)
|
||||
"#;
|
||||
|
||||
let mut server = TestServerBuilder::new()?
|
||||
.with_workspace(
|
||||
SystemPath::new("src"),
|
||||
Some(ClientOptions::default().with_diagnostic_mode(DiagnosticMode::Workspace)),
|
||||
)?
|
||||
.with_file(script, ordinary)?
|
||||
.with_use_uv(UseUv::Scripts)
|
||||
.with_env_var("UV", uv_executable()?)
|
||||
.enable_workspace_diagnostic_refresh(true)
|
||||
.build()
|
||||
.wait_until_workspaces_are_initialized();
|
||||
|
||||
server.open_text_document(script, source, 1);
|
||||
|
||||
// A workspace check must not initialize uv from metadata that exists only in the editor.
|
||||
let diagnostics = server.workspace_diagnostic_request(None, None);
|
||||
let [WorkspaceDocumentDiagnosticReport::WorkspaceFullDocumentDiagnosticReport(report)] =
|
||||
diagnostics.items.as_slice()
|
||||
else {
|
||||
return Err(anyhow!("expected diagnostics for the unsaved script"));
|
||||
};
|
||||
let [diagnostic] = report.full_document_diagnostic_report.items.as_slice() else {
|
||||
return Err(anyhow!(
|
||||
"expected only the unresolved attrs import: {report:?}"
|
||||
));
|
||||
};
|
||||
assert_eq!(
|
||||
diagnostic.code,
|
||||
Some(Code::String("unresolved-import".to_string()))
|
||||
);
|
||||
|
||||
server.write_file(script, source)?;
|
||||
server.save_text_document(script);
|
||||
server.await_diagnostic_refresh();
|
||||
assert!(
|
||||
server
|
||||
.goto_definition_request(script, Position::new(4, 18))
|
||||
.is_some(),
|
||||
"the first save must synchronize the script's dependencies"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn dependencies_resolve_after_invalid_metadata_is_corrected() -> Result<()> {
|
||||
let workspace_root = SystemPath::new("src");
|
||||
|
||||
@@ -1487,7 +1487,41 @@ mod uv_metadata {
|
||||
};
|
||||
use ty_project::UseUv;
|
||||
|
||||
use super::{DocumentDiagnosticReport, Result, SystemPath, TestServerBuilder};
|
||||
use super::{
|
||||
ClientOptions, DiagnosticMode, DocumentDiagnosticReport, Result, SystemPath,
|
||||
TestServerBuilder, WorkspaceDocumentDiagnosticReport,
|
||||
};
|
||||
|
||||
#[test]
|
||||
fn opening_new_file_updates_workspace_index() -> Result<()> {
|
||||
let added = SystemPath::new("src/added.py");
|
||||
let source = "missing\n";
|
||||
|
||||
let mut server = TestServerBuilder::new()?
|
||||
.with_workspace(
|
||||
SystemPath::new("src"),
|
||||
Some(ClientOptions::default().with_diagnostic_mode(DiagnosticMode::Workspace)),
|
||||
)?
|
||||
.with_file(SystemPath::new("src/initial.py"), source)?
|
||||
.with_use_uv(UseUv::Scripts)
|
||||
.build()
|
||||
.wait_until_workspaces_are_initialized();
|
||||
|
||||
// Build the index before creating the file, without sending a watcher event.
|
||||
let _ = server.workspace_diagnostic_request(None, None);
|
||||
server.write_file(added, source)?;
|
||||
server.open_text_document(added, source, 1);
|
||||
|
||||
let uri = server.file_uri(added);
|
||||
let diagnostics = server.workspace_diagnostic_request(None, None);
|
||||
assert!(diagnostics.items.iter().any(|report| matches!(
|
||||
report,
|
||||
WorkspaceDocumentDiagnosticReport::WorkspaceFullDocumentDiagnosticReport(report)
|
||||
if report.uri == uri
|
||||
)));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn synchronization_failure_highlights_script_metadata() -> Result<()> {
|
||||
|
||||
@@ -24,7 +24,7 @@ use ty_ide::{
|
||||
};
|
||||
use ty_ide::{NavigationTarget, NavigationTargets, hints, signature_help};
|
||||
use ty_project::metadata::options::Options;
|
||||
use ty_project::watch::{ChangeEvent, ChangedKind, CreatedKind, DeletedKind};
|
||||
use ty_project::watch::{ChangeEvent, ChangedKind, DeletedKind};
|
||||
use ty_project::{CheckMode, ProjectMetadata};
|
||||
use ty_project::{Db, ProjectDatabase, SemanticDb as _};
|
||||
use ty_python_core::program::FallibleStrategy;
|
||||
@@ -199,10 +199,7 @@ impl Workspace {
|
||||
.write_file_all(&path, contents)
|
||||
.map_err(into_error)?;
|
||||
|
||||
self.db.apply_changes(&[ChangeEvent::Created {
|
||||
path: path.clone(),
|
||||
kind: CreatedKind::File,
|
||||
}]);
|
||||
self.db.apply_changes(&[ChangeEvent::Opened(path.clone())]);
|
||||
|
||||
let file = system_path_to_file(&self.db, &path).expect("File to exist");
|
||||
|
||||
|
||||
@@ -574,14 +574,6 @@ kind = "inherent_method"
|
||||
level = "expect"
|
||||
reason = "change results expose both project and custom-stdlib invalidation state"
|
||||
|
||||
[[override]]
|
||||
lint = "hawk::dead_public"
|
||||
crate = "ty_project"
|
||||
item = "watch::ChangeEvent::is_created"
|
||||
kind = "inherent_method"
|
||||
level = "expect"
|
||||
reason = "change event predicates intentionally cover created, changed, deleted, and rescan events"
|
||||
|
||||
[[override]]
|
||||
lint = "hawk::dead_public"
|
||||
crate = "ruff_python_ast"
|
||||
|
||||
Reference in new issue
Block a user