mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-30 13:51:16 +00:00
[ty] Test "list modules" versus "resolve module" in every mdtest
This ensures there is some level of consistency between the APIs. This did require exposing a couple more things on `Module` for good error messages. This also motivated a switch to an interned struct instead of a tracked struct. This ensures that `list_modules` and `resolve_modules` reuse the same `Module` values when the inputs are the same. Ref https://github.com/astral-sh/ruff/pull/19883#discussion_r2272520194
This commit is contained in:
parent
2e9c241d7e
commit
468eb37d75
3 changed files with 128 additions and 25 deletions
|
@ -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};
|
||||
|
|
|
@ -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<KnownModule> {
|
||||
pub fn known(self, db: &'db dyn Database) -> Option<KnownModule> {
|
||||
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<Vec<Name>>
|
|||
}
|
||||
|
||||
/// 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,
|
||||
|
|
|
@ -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),
|
||||
});
|
||||
|
||||
if let Err(failures) = run_test(&mut db, relative_fixture_path, snapshot_path, &test) {
|
||||
any_failures = true;
|
||||
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 output_format.is_cli() {
|
||||
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) {
|
||||
let md_index = LineIndex::from_source_text(&source);
|
||||
|
||||
for test_failures in failures {
|
||||
|
@ -100,10 +104,24 @@ pub fn run(
|
|||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
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('\'', "\\'");
|
||||
|
||||
if output_format.is_cli() {
|
||||
println!(
|
||||
"\nTo rerun this specific test, set the environment variable: {}='{escaped_test_name}'",
|
||||
EnvVars::MDTEST_TEST_FILTER,
|
||||
|
@ -114,7 +132,6 @@ pub fn run(
|
|||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
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<Module<'db>>,
|
||||
}
|
||||
|
||||
/// 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<ModuleInconsistency<'_>>> {
|
||||
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<FileFailures>;
|
||||
|
||||
/// The failures for a single file in a test by line number.
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue