Skip to content

Commit 87f6f08

Browse files
authored
[ty] Make check_file a salsa query (astral-sh#19255)
## Summary We noticed that all files get reparsed when workspace diagnostics are enabled. I realised that this is because `check_file_impl` access the parsed module but itself isn't a salsa query. This pr makes `check_file_impl` a salsa query, so that we only access the `parsed_module` when the file actually changed. I decided to remove the salsa query from `check_types` because most functions it calls are salsa queries itself and having both `check_types` and `check_file` as salsa querise has the downside that we double cache the diagnostics. ## Test Plan **Before** ``` 2025-07-10 12:54:16.620766000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0c))}: File `/Users/micha/astral/test/yaml/yaml-stubs/__init__.pyi` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.621942000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c13))}: File `/Users/micha/astral/test/ignore2 2/nested-repository/main.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.622107000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c09))}: File `/Users/micha/astral/test/notebook.ipynb` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.622357000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c04))}: File `/Users/micha/astral/test/no-trailing.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.622634000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c02))}: File `/Users/micha/astral/test/simple.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.623056000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c07))}: File `/Users/micha/astral/test/open/more.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.623254000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c11))}: File `/Users/micha/astral/test/ignore-bug/backend/src/subdir/log/some_logging_lib.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.623450000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0f))}: File `/Users/micha/astral/test/yaml/tomllib/__init__.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.624599000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c05))}: File `/Users/micha/astral/test/create.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.624784000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c00))}: File `/Users/micha/astral/test/lib.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.624911000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0a))}: File `/Users/micha/astral/test/sub/test.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.625032000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c12))}: File `/Users/micha/astral/test/ignore2/nested-repository/main.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.625101000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c08))}: File `/Users/micha/astral/test/open/test.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.625227000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c03))}: File `/Users/micha/astral/test/pseudocode_with_bom.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.625353000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0b))}: File `/Users/micha/astral/test/yaml/yaml-stubs/loader.pyi` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.625543000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c01))}: File `/Users/micha/astral/test/test_trailing.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.625616000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0d))}: File `/Users/micha/astral/test/yaml/tomllib/_re.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.625667000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c06))}: File `/Users/micha/astral/test/yaml/main.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.625779000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c10))}: File `/Users/micha/astral/test/yaml/tomllib/_types.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.627526000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0e))}: File `/Users/micha/astral/test/yaml/tomllib/_parser.py` was reparsed after being collected in the current Salsa revision 2025-07-10 12:54:16.627959000 DEBUG request{id=19 method="workspace/diagnostic"}:Project::check: Checking all files took 0.007s ``` Now, no more logs regarding reparsing
1 parent 59114d0 commit 87f6f08

File tree

5 files changed

+77
-71
lines changed

5 files changed

+77
-71
lines changed

crates/ty_project/src/lib.rs

Lines changed: 68 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -257,8 +257,11 @@ impl Project {
257257
tracing::debug_span!(parent: project_span, "check_file", ?file);
258258
let _entered = check_file_span.entered();
259259

260-
let result = self.check_file_impl(&db, file);
261-
file_diagnostics.lock().unwrap().extend(result);
260+
let result = check_file_impl(&db, file);
261+
file_diagnostics
262+
.lock()
263+
.unwrap()
264+
.extend(result.iter().map(Clone::clone));
262265

263266
reporter.report_file(&file);
264267
});
@@ -285,7 +288,7 @@ impl Project {
285288
return Vec::new();
286289
}
287290

288-
self.check_file_impl(db, file)
291+
check_file_impl(db, file).iter().map(Clone::clone).collect()
289292
}
290293

291294
/// Opens a file in the project.
@@ -466,71 +469,73 @@ impl Project {
466469
self.set_file_set(db).to(IndexedFiles::lazy());
467470
}
468471
}
472+
}
469473

470-
fn check_file_impl(self, db: &dyn Db, file: File) -> Vec<Diagnostic> {
471-
let mut diagnostics: Vec<Diagnostic> = Vec::new();
472-
473-
// Abort checking if there are IO errors.
474-
let source = source_text(db, file);
475-
476-
if let Some(read_error) = source.read_error() {
477-
diagnostics.push(
478-
IOErrorDiagnostic {
479-
file: Some(file),
480-
error: read_error.clone().into(),
481-
}
482-
.to_diagnostic(),
483-
);
484-
return diagnostics;
485-
}
474+
#[salsa::tracked(returns(deref), heap_size=get_size2::GetSize::get_heap_size)]
475+
pub(crate) fn check_file_impl(db: &dyn Db, file: File) -> Box<[Diagnostic]> {
476+
let mut diagnostics: Vec<Diagnostic> = Vec::new();
486477

487-
let parsed = parsed_module(db, file);
478+
// Abort checking if there are IO errors.
479+
let source = source_text(db, file);
488480

489-
let parsed_ref = parsed.load(db);
490-
diagnostics.extend(
491-
parsed_ref
492-
.errors()
493-
.iter()
494-
.map(|error| Diagnostic::invalid_syntax(file, &error.error, error)),
481+
if let Some(read_error) = source.read_error() {
482+
diagnostics.push(
483+
IOErrorDiagnostic {
484+
file: Some(file),
485+
error: read_error.clone().into(),
486+
}
487+
.to_diagnostic(),
495488
);
489+
return diagnostics.into_boxed_slice();
490+
}
496491

497-
diagnostics.extend(parsed_ref.unsupported_syntax_errors().iter().map(|error| {
498-
let mut error = Diagnostic::invalid_syntax(file, error, error);
499-
add_inferred_python_version_hint_to_diagnostic(db, &mut error, "parsing syntax");
500-
error
501-
}));
502-
503-
{
504-
let db = AssertUnwindSafe(db);
505-
match catch(&**db, file, || check_types(*db, file)) {
506-
Ok(Some(type_check_diagnostics)) => {
507-
diagnostics.extend(type_check_diagnostics.into_iter().cloned());
508-
}
509-
Ok(None) => {}
510-
Err(diagnostic) => diagnostics.push(diagnostic),
492+
let parsed = parsed_module(db, file);
493+
494+
let parsed_ref = parsed.load(db);
495+
diagnostics.extend(
496+
parsed_ref
497+
.errors()
498+
.iter()
499+
.map(|error| Diagnostic::invalid_syntax(file, &error.error, error)),
500+
);
501+
502+
diagnostics.extend(parsed_ref.unsupported_syntax_errors().iter().map(|error| {
503+
let mut error = Diagnostic::invalid_syntax(file, error, error);
504+
add_inferred_python_version_hint_to_diagnostic(db, &mut error, "parsing syntax");
505+
error
506+
}));
507+
508+
{
509+
let db = AssertUnwindSafe(db);
510+
match catch(&**db, file, || check_types(*db, file)) {
511+
Ok(Some(type_check_diagnostics)) => {
512+
diagnostics.extend(type_check_diagnostics);
511513
}
514+
Ok(None) => {}
515+
Err(diagnostic) => diagnostics.push(diagnostic),
512516
}
517+
}
513518

514-
if self
515-
.open_fileset(db)
516-
.is_none_or(|files| !files.contains(&file))
517-
{
518-
// Drop the AST now that we are done checking this file. It is not currently open,
519-
// so it is unlikely to be accessed again soon. If any queries need to access the AST
520-
// from across files, it will be re-parsed.
521-
parsed.clear();
522-
}
519+
if db
520+
.project()
521+
.open_fileset(db)
522+
.is_none_or(|files| !files.contains(&file))
523+
{
524+
// Drop the AST now that we are done checking this file. It is not currently open,
525+
// so it is unlikely to be accessed again soon. If any queries need to access the AST
526+
// from across files, it will be re-parsed.
527+
parsed.clear();
528+
}
523529

524-
diagnostics.sort_unstable_by_key(|diagnostic| {
525-
diagnostic
526-
.primary_span()
527-
.and_then(|span| span.range())
528-
.unwrap_or_default()
529-
.start()
530-
});
530+
diagnostics.sort_unstable_by_key(|diagnostic| {
531+
diagnostic
532+
.primary_span()
533+
.and_then(|span| span.range())
534+
.unwrap_or_default()
535+
.start()
536+
});
531537

532-
diagnostics
533-
}
538+
diagnostics.into_boxed_slice()
534539
}
535540

536541
#[derive(Debug)]
@@ -701,8 +706,8 @@ where
701706

702707
#[cfg(test)]
703708
mod tests {
704-
use crate::Db;
705709
use crate::ProjectMetadata;
710+
use crate::check_file_impl;
706711
use crate::db::tests::TestDb;
707712
use ruff_db::Db as _;
708713
use ruff_db::files::system_path_to_file;
@@ -741,9 +746,8 @@ mod tests {
741746

742747
assert_eq!(source_text(&db, file).as_str(), "");
743748
assert_eq!(
744-
db.project()
745-
.check_file_impl(&db, file)
746-
.into_iter()
749+
check_file_impl(&db, file)
750+
.iter()
747751
.map(|diagnostic| diagnostic.primary_message().to_string())
748752
.collect::<Vec<_>>(),
749753
vec!["Failed to read file: No such file or directory".to_string()]
@@ -758,9 +762,8 @@ mod tests {
758762

759763
assert_eq!(source_text(&db, file).as_str(), "");
760764
assert_eq!(
761-
db.project()
762-
.check_file_impl(&db, file)
763-
.into_iter()
765+
check_file_impl(&db, file)
766+
.iter()
764767
.map(|diagnostic| diagnostic.primary_message().to_string())
765768
.collect::<Vec<_>>(),
766769
vec![] as Vec<String>

crates/ty_python_semantic/src/types.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,7 @@ mod definition;
8888
#[cfg(test)]
8989
mod property_tests;
9090

91-
#[salsa::tracked(returns(ref), heap_size=get_size2::GetSize::get_heap_size)]
92-
pub fn check_types(db: &dyn Db, file: File) -> TypeCheckDiagnostics {
91+
pub fn check_types(db: &dyn Db, file: File) -> Vec<Diagnostic> {
9392
let _span = tracing::trace_span!("check_types", ?file).entered();
9493

9594
tracing::debug!("Checking file '{path}'", path = file.path(db));
@@ -111,7 +110,7 @@ pub fn check_types(db: &dyn Db, file: File) -> TypeCheckDiagnostics {
111110

112111
check_suppressions(db, file, &mut diagnostics);
113112

114-
diagnostics
113+
diagnostics.into_vec()
115114
}
116115

117116
/// Infer the type of a binding.

crates/ty_python_semantic/src/types/diagnostic.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1598,6 +1598,10 @@ impl TypeCheckDiagnostics {
15981598
self.diagnostics.shrink_to_fit();
15991599
}
16001600

1601+
pub(crate) fn into_vec(self) -> Vec<Diagnostic> {
1602+
self.diagnostics
1603+
}
1604+
16011605
pub fn iter(&self) -> std::slice::Iter<'_, Diagnostic> {
16021606
self.diagnostics.iter()
16031607
}

crates/ty_python_semantic/src/types/infer.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10035,7 +10035,7 @@ mod tests {
1003510035
}
1003610036

1003710037
#[track_caller]
10038-
fn assert_diagnostic_messages(diagnostics: &TypeCheckDiagnostics, expected: &[&str]) {
10038+
fn assert_diagnostic_messages(diagnostics: &[Diagnostic], expected: &[&str]) {
1003910039
let messages: Vec<&str> = diagnostics
1004010040
.iter()
1004110041
.map(Diagnostic::primary_message)
@@ -10048,7 +10048,7 @@ mod tests {
1004810048
let file = system_path_to_file(db, filename).unwrap();
1004910049
let diagnostics = check_types(db, file);
1005010050

10051-
assert_diagnostic_messages(diagnostics, expected);
10051+
assert_diagnostic_messages(&diagnostics, expected);
1005210052
}
1005310053

1005410054
#[test]

crates/ty_test/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ fn run_test(
340340
Err(failures) => return Some(failures),
341341
};
342342

343-
diagnostics.extend(type_diagnostics.into_iter().cloned());
343+
diagnostics.extend(type_diagnostics);
344344
diagnostics.sort_by(|left, right| {
345345
left.rendering_sort_key(db)
346346
.cmp(&right.rendering_sort_key(db))

0 commit comments

Comments
 (0)