diff --git a/Cargo.lock b/Cargo.lock index ab8ead5b80cb55..2d1cdb63652f4a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1963,6 +1963,7 @@ dependencies = [ "anyhow", "camino", "compact_str", + "indexmap", "insta", "once_cell", "path-slash", diff --git a/Cargo.toml b/Cargo.toml index a6bfb5d5e79d44..2c52a2362d67dd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,6 +71,7 @@ hashbrown = "0.14.3" ignore = { version = "0.4.22" } imara-diff = { version = "0.1.5" } imperative = { version = "1.0.4" } +indexmap = { version = "2.2.6" } indicatif = { version = "0.17.8" } indoc = { version = "2.0.4" } insta = { version = "1.35.1" } diff --git a/crates/red_knot_module_resolver/Cargo.toml b/crates/red_knot_module_resolver/Cargo.toml index 2681630e3f0511..abe9254b79dae3 100644 --- a/crates/red_knot_module_resolver/Cargo.toml +++ b/crates/red_knot_module_resolver/Cargo.toml @@ -16,6 +16,7 @@ ruff_python_stdlib = { workspace = true } compact_str = { workspace = true } camino = { workspace = true } +indexmap = { workspace = true } once_cell = { workspace = true } rustc-hash = { workspace = true } salsa = { workspace = true } diff --git a/crates/red_knot_module_resolver/src/path.rs b/crates/red_knot_module_resolver/src/path.rs index a649dd078be459..232ee9d55b2173 100644 --- a/crates/red_knot_module_resolver/src/path.rs +++ b/crates/red_knot_module_resolver/src/path.rs @@ -477,12 +477,6 @@ impl SearchPath { ) } - /// Does this search path point to the `site-packages` directory? - #[must_use] - pub(crate) fn is_site_packages(&self) -> bool { - matches!(&*self.0, SearchPathInner::SitePackages(_)) - } - fn is_valid_extension(&self, extension: &str) -> bool { if self.is_standard_library() { extension == "pyi" diff --git a/crates/red_knot_module_resolver/src/resolver.rs b/crates/red_knot_module_resolver/src/resolver.rs index b66d706566abe9..de348dcf178ba0 100644 --- a/crates/red_knot_module_resolver/src/resolver.rs +++ b/crates/red_knot_module_resolver/src/resolver.rs @@ -1,6 +1,7 @@ use std::borrow::Cow; use std::iter::FusedIterator; +use indexmap::IndexSet; use once_cell::sync::Lazy; use ruff_db::files::{File, FilePath, FileRootKind}; use ruff_db::program::{Program, SearchPathSettings, TargetVersion}; @@ -160,19 +161,6 @@ fn try_resolve_module_resolution_settings( SearchPath::vendored_stdlib() }); - for site_packages_dir in site_packages { - files.try_add_root( - db.upcast(), - site_packages_dir, - FileRootKind::LibrarySearchPath, - ); - - static_search_paths.push(SearchPath::site_packages( - system, - site_packages_dir.clone(), - )?); - } - // TODO vendor typeshed's third-party stubs as well as the stdlib and fallback to them as a final step let target_version = program.target_version(db.upcast()); @@ -198,6 +186,7 @@ fn try_resolve_module_resolution_settings( Ok(ModuleResolutionSettings { target_version, static_search_paths, + site_packages_paths: site_packages.iter().cloned().collect(), }) } @@ -207,52 +196,27 @@ pub(crate) fn module_resolution_settings(db: &dyn Db) -> ModuleResolutionSetting try_resolve_module_resolution_settings(db).unwrap() } -/// Collect all dynamic search paths: -/// search paths listed in `.pth` files in the `site-packages` directory -/// due to editable installations of third-party packages. +/// Collect all dynamic search paths. For each `site-packages` path: +/// - Collect that `site-packages` path +/// - Collect any search paths listed in `.pth` files in that `site-packages` directory +/// due to editable installations of third-party packages. +/// +/// The editable-install search paths for the first `site-packages` directory +/// should come between the two `site-packages` directories when it comes to +/// module-resolution priority. #[salsa::tracked(return_ref)] -pub(crate) fn editable_install_resolution_paths(db: &dyn Db) -> Vec { - let settings = module_resolution_settings(db); - let static_search_paths = &settings.static_search_paths; - - let site_packages = static_search_paths - .iter() - .find(|path| path.is_site_packages()); - - let Some(site_packages) = site_packages else { - return Vec::new(); - }; - - let site_packages = site_packages - .as_system_path() - .expect("Expected site-packages never to be a VendoredPath!"); - - let mut dynamic_paths = Vec::default(); +pub(crate) fn dynamic_resolution_paths(db: &dyn Db) -> Vec { + let ModuleResolutionSettings { + target_version: _, + static_search_paths, + site_packages_paths, + } = module_resolution_settings(db); - // This query needs to be re-executed each time a `.pth` file - // is added, modified or removed from the `site-packages` directory. - // However, we don't use Salsa queries to read the source text of `.pth` files; - // we use the APIs on the `System` trait directly. As such, add a dependency on the - // site-package directory's revision. - if let Some(site_packages_root) = db.files().root(db.upcast(), site_packages) { - let _ = site_packages_root.revision(db.upcast()); - } + let mut dynamic_paths = Vec::new(); - // As well as modules installed directly into `site-packages`, - // the directory may also contain `.pth` files. - // Each `.pth` file in `site-packages` may contain one or more lines - // containing a (relative or absolute) path. - // Each of these paths may point to an editable install of a package, - // so should be considered an additional search path. - let Ok(pth_file_iterator) = PthFileIterator::new(db, site_packages) else { + if site_packages_paths.is_empty() { return dynamic_paths; - }; - - // The Python documentation specifies that `.pth` files in `site-packages` - // are processed in alphabetical order, so collecting and then sorting is necessary. - // https://docs.python.org/3/library/site.html#module-site - let mut all_pth_files: Vec = pth_file_iterator.collect(); - all_pth_files.sort_by(|a, b| a.path.cmp(&b.path)); + } let mut existing_paths: FxHashSet<_> = static_search_paths .iter() @@ -260,14 +224,50 @@ pub(crate) fn editable_install_resolution_paths(db: &dyn Db) -> Vec .map(Cow::Borrowed) .collect(); - dynamic_paths.reserve(all_pth_files.len()); + let files = db.files(); + let system = db.system(); - for pth_file in &all_pth_files { - for installation in pth_file.editable_installations() { - if existing_paths.insert(Cow::Owned( - installation.as_system_path().unwrap().to_path_buf(), - )) { - dynamic_paths.push(installation); + for site_packages_dir in site_packages_paths { + files.try_add_root( + db.upcast(), + site_packages_dir, + FileRootKind::LibrarySearchPath, + ); + dynamic_paths + .push(SearchPath::site_packages(system, site_packages_dir.to_owned()).unwrap()); + + // This query needs to be re-executed each time a `.pth` file + // is added, modified or removed from the `site-packages` directory. + // However, we don't use Salsa queries to read the source text of `.pth` files; + // we use the APIs on the `System` trait directly. As such, add a dependency on the + // site-package directory's revision. + if let Some(site_packages_root) = db.files().root(db.upcast(), site_packages_dir) { + let _ = site_packages_root.revision(db.upcast()); + } + + // As well as modules installed directly into `site-packages`, + // the directory may also contain `.pth` files. + // Each `.pth` file in `site-packages` may contain one or more lines + // containing a (relative or absolute) path. + // Each of these paths may point to an editable install of a package, + // so should be considered an additional search path. + let Ok(pth_file_iterator) = PthFileIterator::new(db, site_packages_dir) else { + continue; + }; + + // The Python documentation specifies that `.pth` files in `site-packages` + // are processed in alphabetical order, so collecting and then sorting is necessary. + // https://docs.python.org/3/library/site.html#module-site + let mut all_pth_files: Vec = pth_file_iterator.collect(); + all_pth_files.sort_by(|a, b| a.path.cmp(&b.path)); + + for pth_file in &all_pth_files { + for installation in pth_file.editable_installations() { + if existing_paths.insert(Cow::Owned( + installation.as_system_path().unwrap().to_path_buf(), + )) { + dynamic_paths.push(installation); + } } } } @@ -300,7 +300,7 @@ impl<'db> Iterator for SearchPathIterator<'db> { static_paths.next().or_else(|| { dynamic_paths - .get_or_insert_with(|| editable_install_resolution_paths(*db).iter()) + .get_or_insert_with(|| dynamic_resolution_paths(*db).iter()) .next() }) } @@ -410,9 +410,18 @@ impl<'db> Iterator for PthFileIterator<'db> { #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct ModuleResolutionSettings { target_version: TargetVersion, + /// Search paths that have been statically determined purely from reading Ruff's configuration settings. /// These shouldn't ever change unless the config settings themselves change. static_search_paths: Vec, + + /// site-packages paths are not included in the above field: + /// if there are multiple site-packages paths, editable installations can appear + /// *between* the site-packages paths on `sys.path` at runtime. + /// That means we can't know where a second or third `site-packages` path should sit + /// in terms of module-resolution priority until we've discovered the editable installs + /// for the first `site-packages` path + site_packages_paths: IndexSet, } impl ModuleResolutionSettings { @@ -1585,7 +1594,7 @@ not_a_directory &FilePath::system("/y/src/bar.py") ); let events = db.take_salsa_events(); - assert_const_function_query_was_not_run(&db, editable_install_resolution_paths, &events); + assert_const_function_query_was_not_run(&db, dynamic_resolution_paths, &events); } #[test]