diff --git a/crates/ty_python_semantic/src/module_resolver/mod.rs b/crates/ty_python_semantic/src/module_resolver/mod.rs index f894d618ae..901adbf4ef 100644 --- a/crates/ty_python_semantic/src/module_resolver/mod.rs +++ b/crates/ty_python_semantic/src/module_resolver/mod.rs @@ -3,7 +3,7 @@ use std::iter::FusedIterator; pub use list::list_modules; pub(crate) use module::KnownModule; pub use module::Module; -pub use path::SearchPathValidationError; +pub use path::{SearchPath, SearchPathValidationError}; pub use resolver::SearchPaths; pub(crate) use resolver::file_to_module; pub use resolver::{resolve_module, resolve_real_module}; diff --git a/crates/ty_python_semantic/src/module_resolver/module.rs b/crates/ty_python_semantic/src/module_resolver/module.rs index e61717364e..a7d50829b0 100644 --- a/crates/ty_python_semantic/src/module_resolver/module.rs +++ b/crates/ty_python_semantic/src/module_resolver/module.rs @@ -59,7 +59,7 @@ impl<'db> Module<'db> { } /// Is this a module that we special-case somehow? If so, which one? - pub(crate) fn known(self, db: &'db dyn Database) -> Option { + pub fn known(self, db: &'db dyn Database) -> Option { match self { Module::File(module) => module.known(db), Module::Namespace(_) => None, @@ -75,7 +75,7 @@ impl<'db> Module<'db> { /// /// It is guaranteed that if `None` is returned, then this is a namespace /// package. Otherwise, this is a regular package or file module. - pub(crate) fn search_path(self, db: &'db dyn Database) -> Option<&'db SearchPath> { + pub fn search_path(self, db: &'db dyn Database) -> Option<&'db SearchPath> { match self { Module::File(module) => Some(module.search_path(db)), Module::Namespace(_) => None, @@ -214,7 +214,7 @@ fn all_submodule_names_for_package(db: &dyn Db, file: File) -> Option> } /// A module that resolves to a file (`lib.py` or `package/__init__.py`) -#[salsa::tracked(debug, heap_size=ruff_memory_usage::heap_size)] +#[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] pub struct FileModule<'db> { #[returns(ref)] pub(super) name: ModuleName, @@ -229,7 +229,7 @@ pub struct FileModule<'db> { /// /// Namespace packages are special because there are /// multiple possible paths and they have no corresponding code file. -#[salsa::tracked(debug, heap_size=ruff_memory_usage::heap_size)] +#[salsa::interned(debug, heap_size=ruff_memory_usage::heap_size)] pub struct NamespacePackage<'db> { #[returns(ref)] pub(super) name: ModuleName, diff --git a/crates/ty_test/src/lib.rs b/crates/ty_test/src/lib.rs index 10dc4a7ce1..877954addb 100644 --- a/crates/ty_test/src/lib.rs +++ b/crates/ty_test/src/lib.rs @@ -18,8 +18,9 @@ use std::fmt::Write; use ty_python_semantic::pull_types::pull_types; use ty_python_semantic::types::check_types; use ty_python_semantic::{ - Program, ProgramSettings, PythonEnvironment, PythonPlatform, PythonVersionSource, - PythonVersionWithSource, SearchPathSettings, SysPrefixPathOrigin, + Module, Program, ProgramSettings, PythonEnvironment, PythonPlatform, PythonVersionSource, + PythonVersionWithSource, SearchPath, SearchPathSettings, SysPrefixPathOrigin, list_modules, + resolve_module, }; mod assertion; @@ -68,13 +69,16 @@ pub fn run( Log::Filter(filter) => setup_logging_with_filter(filter), }); + let failures = run_test(&mut db, relative_fixture_path, snapshot_path, &test); + let inconsistencies = run_module_resolution_consistency_test(&db); + let this_test_failed = failures.is_err() || inconsistencies.is_err(); + any_failures = any_failures || this_test_failed; + + if this_test_failed && output_format.is_cli() { + println!("\n{}\n", test.name().bold().underline()); + } + if let Err(failures) = run_test(&mut db, relative_fixture_path, snapshot_path, &test) { - any_failures = true; - - if output_format.is_cli() { - println!("\n{}\n", test.name().bold().underline()); - } - let md_index = LineIndex::from_source_text(&source); for test_failures in failures { @@ -100,20 +104,33 @@ pub fn run( } } } - - let escaped_test_name = test.name().replace('\'', "\\'"); - - if output_format.is_cli() { - println!( - "\nTo rerun this specific test, set the environment variable: {}='{escaped_test_name}'", - EnvVars::MDTEST_TEST_FILTER, - ); - println!( - "{}='{escaped_test_name}' cargo test -p ty_python_semantic --test mdtest -- {test_name}", - EnvVars::MDTEST_TEST_FILTER, - ); + } + if let Err(inconsistencies) = run_module_resolution_consistency_test(&db) { + any_failures = true; + for inconsistency in inconsistencies { + match output_format { + OutputFormat::Cli => { + let info = relative_fixture_path.to_string().cyan(); + println!(" {info} {inconsistency}"); + } + OutputFormat::GitHub => { + println!("::error file={absolute_fixture_path}::{inconsistency}"); + } + } } } + + if this_test_failed && output_format.is_cli() { + let escaped_test_name = test.name().replace('\'', "\\'"); + println!( + "\nTo rerun this specific test, set the environment variable: {}='{escaped_test_name}'", + EnvVars::MDTEST_TEST_FILTER, + ); + println!( + "{}='{escaped_test_name}' cargo test -p ty_python_semantic --test mdtest -- {test_name}", + EnvVars::MDTEST_TEST_FILTER, + ); + } } println!("\n{}\n", "-".repeat(50)); @@ -406,6 +423,92 @@ fn run_test( } } +/// Reports an inconsistency between "list modules" and "resolve module." +/// +/// Values of this type are only constructed when `from_list` and +/// `from_resolve` are not equivalent. +struct ModuleInconsistency<'db> { + db: &'db db::Db, + /// The module returned from `list_module`. + from_list: Module<'db>, + /// The module returned, if any, from `resolve_module`. + from_resolve: Option>, +} + +/// Tests that "list modules" is consistent with "resolve module." +/// +/// This only checks that everything returned by `list_module` is the +/// identical module we get back from `resolve_module`. It does not +/// check that all possible outputs of `resolve_module` are captured by +/// `list_module`. +fn run_module_resolution_consistency_test(db: &db::Db) -> Result<(), Vec>> { + let mut errs = vec![]; + for from_list in list_modules(db) { + errs.push(match resolve_module(db, from_list.name(db)) { + None => ModuleInconsistency { + db, + from_list, + from_resolve: None, + }, + Some(from_resolve) if from_list != from_resolve => ModuleInconsistency { + db, + from_list, + from_resolve: Some(from_resolve), + }, + _ => continue, + }); + } + if errs.is_empty() { Ok(()) } else { Err(errs) } +} + +impl std::fmt::Display for ModuleInconsistency<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + fn fmt_module( + db: &db::Db, + f: &mut std::fmt::Formatter, + module: &Module<'_>, + ) -> std::fmt::Result { + let name = module.name(db); + let path = module + .file(db) + .map(|file| file.path(db).to_string()) + .unwrap_or_else(|| "N/A".to_string()); + let search_path = module + .search_path(db) + .map(SearchPath::to_string) + .unwrap_or_else(|| "N/A".to_string()); + let known = module + .known(db) + .map(|known| known.to_string()) + .unwrap_or_else(|| "N/A".to_string()); + write!( + f, + "Module(\ + name={name}, \ + file={path}, \ + kind={kind:?}, \ + search_path={search_path}, \ + known={known}\ + )", + kind = module.kind(db), + ) + } + write!(f, "Found ")?; + fmt_module(self.db, f, &self.from_list)?; + match self.from_resolve { + None => write!( + f, + " when listing modules, but `resolve_module` returned `None`", + )?, + Some(ref got) => { + write!(f, " when listing modules, but `resolve_module` returned ",)?; + fmt_module(self.db, f, got)?; + } + } + Ok(()) + } +} + type Failures = Vec; /// The failures for a single file in a test by line number.