From 5b21922420271a4af817059b4134b546bd156134 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 10 Jul 2024 15:34:06 +0100 Subject: [PATCH] [red-knot] Add more stress tests for module resolver invalidation (#12272) --- .../red_knot_module_resolver/src/resolver.rs | 135 +++++++++++++++++- crates/red_knot_python_semantic/src/db.rs | 107 -------------- crates/red_knot_python_semantic/src/types.rs | 11 +- crates/ruff_db/src/lib.rs | 1 + crates/ruff_db/src/testing.rs | 116 +++++++++++++++ 5 files changed, 255 insertions(+), 115 deletions(-) create mode 100644 crates/ruff_db/src/testing.rs diff --git a/crates/red_knot_module_resolver/src/resolver.rs b/crates/red_knot_module_resolver/src/resolver.rs index 2e6916f1b6584..71b80787115a7 100644 --- a/crates/red_knot_module_resolver/src/resolver.rs +++ b/crates/red_knot_module_resolver/src/resolver.rs @@ -384,8 +384,10 @@ impl PackageKind { #[cfg(test)] mod tests { + use internal::ModuleNameIngredient; use ruff_db::files::{system_path_to_file, File, FilePath}; use ruff_db::system::{DbWithTestSystem, OsSystem, SystemPath}; + use ruff_db::testing::assert_function_query_was_not_run; use crate::db::tests::TestDb; use crate::module::ModuleKind; @@ -966,7 +968,7 @@ mod tests { } #[test] - fn adding_a_file_on_which_the_module_resolution_depends_on_invalidates_the_query( + fn adding_file_on_which_module_resolution_depends_invalidates_previously_failing_query_that_now_succeeds( ) -> anyhow::Result<()> { let TestCase { mut db, src, .. } = TestCaseBuilder::new().build(); let foo_path = src.join("foo.py"); @@ -986,7 +988,7 @@ mod tests { } #[test] - fn removing_a_file_that_the_module_resolution_depends_on_invalidates_the_query( + fn removing_file_on_which_module_resolution_depends_invalidates_previously_successful_query_that_now_fails( ) -> anyhow::Result<()> { const SRC: &[FileSpec] = &[("foo.py", "x = 1"), ("foo/__init__.py", "x = 2")]; @@ -1009,4 +1011,133 @@ mod tests { Ok(()) } + + #[test] + fn adding_file_to_search_path_with_lower_priority_does_not_invalidate_query() { + const TYPESHED: MockedTypeshed = MockedTypeshed { + versions: "functools: 3.8-", + stdlib_files: &[("functools.pyi", "def update_wrapper(): ...")], + }; + + let TestCase { + mut db, + stdlib, + site_packages, + .. + } = TestCaseBuilder::new() + .with_custom_typeshed(TYPESHED) + .with_target_version(TargetVersion::Py38) + .build(); + + let functools_module_name = ModuleName::new_static("functools").unwrap(); + let stdlib_functools_path = stdlib.join("functools.pyi"); + + let functools_module = resolve_module(&db, functools_module_name.clone()).unwrap(); + assert_eq!(functools_module.search_path(), stdlib); + assert_eq!( + Some(functools_module.file()), + system_path_to_file(&db, &stdlib_functools_path) + ); + + // Adding a file to site-packages does not invalidate the query, + // since site-packages takes lower priority in the module resolution + db.clear_salsa_events(); + let site_packages_functools_path = site_packages.join("functools.py"); + db.write_file(&site_packages_functools_path, "f: int") + .unwrap(); + let functools_module = resolve_module(&db, functools_module_name.clone()).unwrap(); + let events = db.take_salsa_events(); + assert_function_query_was_not_run::( + &db, + |res| &res.function, + &ModuleNameIngredient::new(&db, functools_module_name.clone()), + &events, + ); + assert_eq!(functools_module.search_path(), stdlib); + assert_eq!( + Some(functools_module.file()), + system_path_to_file(&db, &stdlib_functools_path) + ); + } + + #[test] + fn adding_file_to_search_path_with_higher_priority_invalidates_the_query() { + const TYPESHED: MockedTypeshed = MockedTypeshed { + versions: "functools: 3.8-", + stdlib_files: &[("functools.pyi", "def update_wrapper(): ...")], + }; + + let TestCase { + mut db, + stdlib, + src, + .. + } = TestCaseBuilder::new() + .with_custom_typeshed(TYPESHED) + .with_target_version(TargetVersion::Py38) + .build(); + + let functools_module_name = ModuleName::new_static("functools").unwrap(); + let functools_module = resolve_module(&db, functools_module_name.clone()).unwrap(); + assert_eq!(functools_module.search_path(), stdlib); + assert_eq!( + Some(functools_module.file()), + system_path_to_file(&db, stdlib.join("functools.pyi")) + ); + + // Adding a first-party file invalidates the query, + // since first-party files take higher priority in module resolution: + let src_functools_path = src.join("functools.py"); + db.write_file(&src_functools_path, "FOO: int").unwrap(); + let functools_module = resolve_module(&db, functools_module_name.clone()).unwrap(); + assert_eq!(functools_module.search_path(), src); + assert_eq!( + Some(functools_module.file()), + system_path_to_file(&db, &src_functools_path) + ); + } + + #[test] + fn deleting_file_from_higher_priority_search_path_invalidates_the_query() { + const SRC: &[FileSpec] = &[("functools.py", "FOO: int")]; + + const TYPESHED: MockedTypeshed = MockedTypeshed { + versions: "functools: 3.8-", + stdlib_files: &[("functools.pyi", "def update_wrapper(): ...")], + }; + + let TestCase { + mut db, + stdlib, + src, + .. + } = TestCaseBuilder::new() + .with_src_files(SRC) + .with_custom_typeshed(TYPESHED) + .with_target_version(TargetVersion::Py38) + .build(); + + let functools_module_name = ModuleName::new_static("functools").unwrap(); + let src_functools_path = src.join("functools.py"); + + let functools_module = resolve_module(&db, functools_module_name.clone()).unwrap(); + assert_eq!(functools_module.search_path(), src); + assert_eq!( + Some(functools_module.file()), + system_path_to_file(&db, &src_functools_path) + ); + + // If we now delete the first-party file, + // it should resolve to the stdlib: + db.memory_file_system() + .remove_file(&src_functools_path) + .unwrap(); + File::touch_path(&mut db, &src_functools_path); + let functools_module = resolve_module(&db, functools_module_name.clone()).unwrap(); + assert_eq!(functools_module.search_path(), stdlib); + assert_eq!( + Some(functools_module.file()), + system_path_to_file(&db, stdlib.join("functools.pyi")) + ); + } } diff --git a/crates/red_knot_python_semantic/src/db.rs b/crates/red_knot_python_semantic/src/db.rs index 9a543f74c5a72..5d375ad86f56c 100644 --- a/crates/red_knot_python_semantic/src/db.rs +++ b/crates/red_knot_python_semantic/src/db.rs @@ -35,13 +35,8 @@ pub trait Db: #[cfg(test)] pub(crate) mod tests { - use std::fmt::Formatter; - use std::marker::PhantomData; use std::sync::Arc; - use salsa::id::AsId; - use salsa::ingredient::Ingredient; - use salsa::storage::HasIngredientsFor; use salsa::DebugWithDb; use red_knot_module_resolver::{vendored_typeshed_stubs, Db as ResolverDb, Jar as ResolverJar}; @@ -150,106 +145,4 @@ pub(crate) mod tests { }) } } - - pub(crate) fn assert_will_run_function_query<'db, C, Db, Jar>( - db: &'db Db, - to_function: impl FnOnce(&C) -> &salsa::function::FunctionIngredient, - input: &C::Input<'db>, - events: &[salsa::Event], - ) where - C: salsa::function::Configuration - + salsa::storage::IngredientsFor, - Jar: HasIngredientsFor, - Db: salsa::DbWithJar, - C::Input<'db>: AsId, - { - will_run_function_query(db, to_function, input, events, true); - } - - pub(crate) fn assert_will_not_run_function_query<'db, C, Db, Jar>( - db: &'db Db, - to_function: impl FnOnce(&C) -> &salsa::function::FunctionIngredient, - input: &C::Input<'db>, - events: &[salsa::Event], - ) where - C: salsa::function::Configuration - + salsa::storage::IngredientsFor, - Jar: HasIngredientsFor, - Db: salsa::DbWithJar, - C::Input<'db>: AsId, - { - will_run_function_query(db, to_function, input, events, false); - } - - fn will_run_function_query<'db, C, Db, Jar>( - db: &'db Db, - to_function: impl FnOnce(&C) -> &salsa::function::FunctionIngredient, - input: &C::Input<'db>, - events: &[salsa::Event], - should_run: bool, - ) where - C: salsa::function::Configuration - + salsa::storage::IngredientsFor, - Jar: HasIngredientsFor, - Db: salsa::DbWithJar, - C::Input<'db>: AsId, - { - let (jar, _) = - <_ as salsa::storage::HasJar<::Jar>>::jar(db); - let ingredient = jar.ingredient(); - - let function_ingredient = to_function(ingredient); - - let ingredient_index = - as Ingredient>::ingredient_index( - function_ingredient, - ); - - let did_run = events.iter().any(|event| { - if let salsa::EventKind::WillExecute { database_key } = event.kind { - database_key.ingredient_index() == ingredient_index - && database_key.key_index() == input.as_id() - } else { - false - } - }); - - if should_run && !did_run { - panic!( - "Expected query {:?} to run but it didn't", - DebugIdx { - db: PhantomData::, - value_id: input.as_id(), - ingredient: function_ingredient, - } - ); - } else if !should_run && did_run { - panic!( - "Expected query {:?} not to run but it did", - DebugIdx { - db: PhantomData::, - value_id: input.as_id(), - ingredient: function_ingredient, - } - ); - } - } - - struct DebugIdx<'a, I, Db> - where - I: Ingredient, - { - value_id: salsa::Id, - ingredient: &'a I, - db: PhantomData, - } - - impl<'a, I, Db> std::fmt::Debug for DebugIdx<'a, I, Db> - where - I: Ingredient, - { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - self.ingredient.fmt_index(Some(self.value_id), f) - } - } } diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 535123e3ca1cc..517fb52a76e87 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -277,10 +277,9 @@ mod tests { use ruff_db::files::system_path_to_file; use ruff_db::parsed::parsed_module; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; + use ruff_db::testing::{assert_function_query_was_not_run, assert_function_query_was_run}; - use crate::db::tests::{ - assert_will_not_run_function_query, assert_will_run_function_query, TestDb, - }; + use crate::db::tests::TestDb; use crate::semantic_index::root_scope; use crate::types::{infer_types, public_symbol_ty_by_name}; use crate::{HasTy, SemanticModel}; @@ -347,7 +346,7 @@ mod tests { let events = db.take_salsa_events(); let a_root_scope = root_scope(&db, a); - assert_will_run_function_query::( + assert_function_query_was_run::( &db, |ty| &ty.function, &a_root_scope, @@ -385,7 +384,7 @@ mod tests { let a_root_scope = root_scope(&db, a); - assert_will_not_run_function_query::( + assert_function_query_was_not_run::( &db, |ty| &ty.function, &a_root_scope, @@ -422,7 +421,7 @@ mod tests { let events = db.take_salsa_events(); let a_root_scope = root_scope(&db, a); - assert_will_not_run_function_query::( + assert_function_query_was_not_run::( &db, |ty| &ty.function, &a_root_scope, diff --git a/crates/ruff_db/src/lib.rs b/crates/ruff_db/src/lib.rs index cb8469315c51b..5a240e5e54474 100644 --- a/crates/ruff_db/src/lib.rs +++ b/crates/ruff_db/src/lib.rs @@ -14,6 +14,7 @@ pub mod files; pub mod parsed; pub mod source; pub mod system; +pub mod testing; pub mod vendored; pub(crate) type FxDashMap = dashmap::DashMap>; diff --git a/crates/ruff_db/src/testing.rs b/crates/ruff_db/src/testing.rs new file mode 100644 index 0000000000000..06f4f96713463 --- /dev/null +++ b/crates/ruff_db/src/testing.rs @@ -0,0 +1,116 @@ +//! Test helpers for working with Salsa databases + +use std::fmt; +use std::marker::PhantomData; + +use salsa::id::AsId; +use salsa::ingredient::Ingredient; +use salsa::storage::HasIngredientsFor; + +/// Assert that the Salsa query described by the generic parameter `C` +/// was executed at least once with the input `input` +/// in the history span represented by `events`. +pub fn assert_function_query_was_run<'db, C, Db, Jar>( + db: &'db Db, + to_function: impl FnOnce(&C) -> &salsa::function::FunctionIngredient, + input: &C::Input<'db>, + events: &[salsa::Event], +) where + C: salsa::function::Configuration + + salsa::storage::IngredientsFor, + Jar: HasIngredientsFor, + Db: salsa::DbWithJar, + C::Input<'db>: AsId, +{ + function_query_was_run(db, to_function, input, events, true); +} + +/// Assert that there were no executions with the input `input` +/// of the Salsa query described by the generic parameter `C` +/// in the history span represented by `events`. +pub fn assert_function_query_was_not_run<'db, C, Db, Jar>( + db: &'db Db, + to_function: impl FnOnce(&C) -> &salsa::function::FunctionIngredient, + input: &C::Input<'db>, + events: &[salsa::Event], +) where + C: salsa::function::Configuration + + salsa::storage::IngredientsFor, + Jar: HasIngredientsFor, + Db: salsa::DbWithJar, + C::Input<'db>: AsId, +{ + function_query_was_run(db, to_function, input, events, false); +} + +fn function_query_was_run<'db, C, Db, Jar>( + db: &'db Db, + to_function: impl FnOnce(&C) -> &salsa::function::FunctionIngredient, + input: &C::Input<'db>, + events: &[salsa::Event], + should_have_run: bool, +) where + C: salsa::function::Configuration + + salsa::storage::IngredientsFor, + Jar: HasIngredientsFor, + Db: salsa::DbWithJar, + C::Input<'db>: AsId, +{ + let (jar, _) = + <_ as salsa::storage::HasJar<::Jar>>::jar(db); + let ingredient = jar.ingredient(); + + let function_ingredient = to_function(ingredient); + + let ingredient_index = + as Ingredient>::ingredient_index( + function_ingredient, + ); + + let did_run = events.iter().any(|event| { + if let salsa::EventKind::WillExecute { database_key } = event.kind { + database_key.ingredient_index() == ingredient_index + && database_key.key_index() == input.as_id() + } else { + false + } + }); + + if should_have_run && !did_run { + panic!( + "Expected query {:?} to have run but it didn't", + DebugIdx { + db: PhantomData::, + value_id: input.as_id(), + ingredient: function_ingredient, + } + ); + } else if !should_have_run && did_run { + panic!( + "Expected query {:?} not to have run but it did", + DebugIdx { + db: PhantomData::, + value_id: input.as_id(), + ingredient: function_ingredient, + } + ); + } +} + +struct DebugIdx<'a, I, Db> +where + I: Ingredient, +{ + value_id: salsa::Id, + ingredient: &'a I, + db: PhantomData, +} + +impl<'a, I, Db> fmt::Debug for DebugIdx<'a, I, Db> +where + I: Ingredient, +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result { + self.ingredient.fmt_index(Some(self.value_id), f) + } +}