Fix search path order

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" }

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 }

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"

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,67 +196,78 @@ 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<SearchPath> {
-    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<SearchPath> {
+    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<PthFile> = 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()
         .filter_map(|path| path.as_system_path())
         .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<PthFile> = 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<SearchPath>,
+
+    /// 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<SystemPathBuf>,
 }
 
 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]