From b3966136d304c560ea636555245d4f35ca1e4e5b Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 19 Jul 2024 17:29:08 +0200 Subject: [PATCH 1/2] [red-knot] Watch search paths --- Cargo.lock | 1 + crates/red_knot/src/main.rs | 18 +- crates/red_knot/src/watch.rs | 2 + .../red_knot/src/watch/workspace_watcher.rs | 112 ++++++++++++ crates/red_knot/src/workspace.rs | 12 ++ crates/red_knot/tests/file_watching.rs | 170 ++++++++++++++++-- crates/red_knot_module_resolver/src/lib.rs | 46 ++++- .../red_knot_module_resolver/src/resolver.rs | 4 +- crates/red_knot_python_semantic/Cargo.toml | 1 + .../src/semantic_index/builder.rs | 12 +- .../src/semantic_index/definition.rs | 3 + .../src/semantic_index/expression.rs | 3 + .../src/semantic_index/symbol.rs | 3 + crates/ruff_db/src/program.rs | 2 +- crates/ruff_db/src/system.rs | 4 +- crates/ruff_db/src/system/path.rs | 55 ++++++ 16 files changed, 411 insertions(+), 37 deletions(-) create mode 100644 crates/red_knot/src/watch/workspace_watcher.rs diff --git a/Cargo.lock b/Cargo.lock index 21a0e64998c5a..341656eed2cb5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1898,6 +1898,7 @@ version = "0.0.0" dependencies = [ "anyhow", "bitflags 2.6.0", + "countme", "hashbrown", "ordermap", "red_knot_module_resolver", diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index dac3e6fe6a2dd..b8ad8022f8911 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -12,7 +12,7 @@ use tracing_tree::time::Uptime; use red_knot::db::RootDatabase; use red_knot::watch; -use red_knot::watch::Watcher; +use red_knot::watch::WorkspaceWatcher; use red_knot::workspace::WorkspaceMetadata; use ruff_db::program::{ProgramSettings, SearchPathSettings}; use ruff_db::system::{OsSystem, System, SystemPathBuf}; @@ -142,7 +142,7 @@ struct MainLoop { receiver: crossbeam_channel::Receiver, /// The file system watcher, if running in watch mode. - watcher: Option, + watcher: Option, verbosity: Option, } @@ -164,26 +164,23 @@ impl MainLoop { fn watch(mut self, db: &mut RootDatabase) -> anyhow::Result<()> { let sender = self.sender.clone(); - let mut watcher = watch::directory_watcher(move |event| { + let watcher = watch::directory_watcher(move |event| { sender.send(MainLoopMessage::ApplyChanges(event)).unwrap(); })?; - watcher.watch(db.workspace().root(db))?; - - self.watcher = Some(watcher); - + self.watcher = Some(WorkspaceWatcher::new(watcher, db)); self.run(db); Ok(()) } #[allow(clippy::print_stderr)] - fn run(self, db: &mut RootDatabase) { + fn run(mut self, db: &mut RootDatabase) { // Schedule the first check. self.sender.send(MainLoopMessage::CheckWorkspace).unwrap(); let mut revision = 0usize; - for message in &self.receiver { + while let Ok(message) = self.receiver.recv() { tracing::trace!("Main Loop: Tick"); match message { @@ -224,6 +221,9 @@ impl MainLoop { revision += 1; // Automatically cancels any pending queries and waits for them to complete. db.apply_changes(changes); + if let Some(watcher) = self.watcher.as_mut() { + watcher.update(db); + } self.sender.send(MainLoopMessage::CheckWorkspace).unwrap(); } MainLoopMessage::Exit => { diff --git a/crates/red_knot/src/watch.rs b/crates/red_knot/src/watch.rs index f68da053389e9..f59c0a81be51d 100644 --- a/crates/red_knot/src/watch.rs +++ b/crates/red_knot/src/watch.rs @@ -1,7 +1,9 @@ use ruff_db::system::{SystemPath, SystemPathBuf}; pub use watcher::{directory_watcher, EventHandler, Watcher}; +pub use workspace_watcher::WorkspaceWatcher; mod watcher; +mod workspace_watcher; /// Classification of a file system change event. /// diff --git a/crates/red_knot/src/watch/workspace_watcher.rs b/crates/red_knot/src/watch/workspace_watcher.rs new file mode 100644 index 0000000000000..d549e4822c0c0 --- /dev/null +++ b/crates/red_knot/src/watch/workspace_watcher.rs @@ -0,0 +1,112 @@ +use crate::db::RootDatabase; +use crate::watch::Watcher; +use ruff_db::system::SystemPathBuf; +use rustc_hash::FxHashSet; +use std::fmt::{Formatter, Write}; +use tracing::info; + +/// Wrapper around a [`Watcher`] that watches the relevant paths of a workspace. +pub struct WorkspaceWatcher { + watcher: Watcher, + + /// The paths that need to be watched. This includes paths for which setting up file watching failed. + watched_paths: FxHashSet, + + /// Paths that should be watched but setting up the watcher failed for some reason. + /// This should be rare. + errored_paths: Vec, +} + +impl WorkspaceWatcher { + /// Create a new workspace watcher. + pub fn new(watcher: Watcher, db: &RootDatabase) -> Self { + let mut watcher = Self { + watcher, + watched_paths: FxHashSet::default(), + errored_paths: Vec::new(), + }; + + watcher.update(db); + + watcher + } + + pub fn update(&mut self, db: &RootDatabase) { + let new_watch_paths = db.workspace().watch_paths(db); + + let mut added_folders = new_watch_paths.difference(&self.watched_paths).peekable(); + let mut removed_folders = self.watched_paths.difference(&new_watch_paths).peekable(); + + if added_folders.peek().is_none() && removed_folders.peek().is_none() { + return; + } + + for added_folder in added_folders { + // Log a warning. It's not worth aborting if registering a single folder fails because + // Ruff otherwise stills works as expected. + if let Err(error) = self.watcher.watch(added_folder) { + // TODO: Log a user-facing warning. + tracing::warn!("Failed to setup watcher for path '{added_folder}': {error}. You have to restart Ruff after making changes to files under this path or you might see stale results."); + self.errored_paths.push(added_folder.clone()); + } + } + + for removed_path in removed_folders { + if let Some(index) = self + .errored_paths + .iter() + .position(|path| path == removed_path) + { + self.errored_paths.swap_remove(index); + continue; + } + + if let Err(error) = self.watcher.unwatch(removed_path) { + info!("Failed to remove the file watcher for the path '{removed_path}: {error}."); + } + } + + info!( + "Set up file watchers for {}", + DisplayWatchedPaths { + paths: &new_watch_paths + } + ); + + self.watched_paths = new_watch_paths; + } + + /// Returns `true` if setting up watching for any path failed. + pub fn has_errored_paths(&self) -> bool { + !self.errored_paths.is_empty() + } + + pub fn flush(&self) { + self.watcher.flush(); + } + + pub fn stop(self) { + self.watcher.stop(); + } +} + +struct DisplayWatchedPaths<'a> { + paths: &'a FxHashSet, +} + +impl std::fmt::Display for DisplayWatchedPaths<'_> { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_char('[')?; + + let mut iter = self.paths.iter(); + if let Some(first) = iter.next() { + write!(f, "\"{first}\"")?; + + for path in iter { + write!(f, ", \"{path}\"")?; + } + } + + f.write_char(']') + } +} diff --git a/crates/red_knot/src/workspace.rs b/crates/red_knot/src/workspace.rs index 0069cffd9e558..1c0cd743dd2ba 100644 --- a/crates/red_knot/src/workspace.rs +++ b/crates/red_knot/src/workspace.rs @@ -6,6 +6,7 @@ use std::{collections::BTreeMap, sync::Arc}; use rustc_hash::{FxBuildHasher, FxHashSet}; pub use metadata::{PackageMetadata, WorkspaceMetadata}; +use red_knot_module_resolver::system_module_search_paths; use ruff_db::{ files::{system_path_to_file, File}, system::{walk_directory::WalkState, SystemPath, SystemPathBuf}, @@ -240,6 +241,17 @@ impl Workspace { FxHashSet::default() } } + + /// Returns the paths that should be watched. + /// + /// The paths that require watching might change with every revision. + pub fn watch_paths(self, db: &dyn Db) -> FxHashSet { + ruff_db::system::deduplicate_nested_paths( + std::iter::once(self.root(db)).chain(system_module_search_paths(db.upcast())), + ) + .map(SystemPath::to_path_buf) + .collect() + } } #[salsa::tracked] diff --git a/crates/red_knot/tests/file_watching.rs b/crates/red_knot/tests/file_watching.rs index e0458e060ca0f..82b45c0e29e09 100644 --- a/crates/red_knot/tests/file_watching.rs +++ b/crates/red_knot/tests/file_watching.rs @@ -6,18 +6,18 @@ use anyhow::{anyhow, Context}; use red_knot::db::RootDatabase; use red_knot::watch; -use red_knot::watch::{directory_watcher, Watcher}; +use red_knot::watch::{directory_watcher, WorkspaceWatcher}; use red_knot::workspace::WorkspaceMetadata; use red_knot_module_resolver::{resolve_module, ModuleName}; use ruff_db::files::{system_path_to_file, File}; -use ruff_db::program::{ProgramSettings, SearchPathSettings, TargetVersion}; +use ruff_db::program::{Program, ProgramSettings, SearchPathSettings, TargetVersion}; use ruff_db::source::source_text; use ruff_db::system::{OsSystem, SystemPath, SystemPathBuf}; use ruff_db::Upcast; struct TestCase { db: RootDatabase, - watcher: Option, + watcher: Option, changes_receiver: crossbeam::channel::Receiver>, temp_dir: tempfile::TempDir, } @@ -70,12 +70,37 @@ impl TestCase { } fn setup(workspace_files: I) -> anyhow::Result +where + I: IntoIterator, + P: AsRef, +{ + setup_with_search_paths(workspace_files, |_root, workspace_path| { + SearchPathSettings { + extra_paths: vec![], + workspace_root: workspace_path.to_path_buf(), + custom_typeshed: None, + site_packages: None, + } + }) +} + +fn setup_with_search_paths( + workspace_files: I, + create_search_paths: impl FnOnce(&SystemPath, &SystemPath) -> SearchPathSettings, +) -> anyhow::Result where I: IntoIterator, P: AsRef, { let temp_dir = tempfile::tempdir()?; + let root_path = SystemPath::from_std_path(temp_dir.path()).ok_or_else(|| { + anyhow!( + "Temp directory '{}' is not a valid UTF-8 path.", + temp_dir.path().display() + ) + })?; + let workspace_path = temp_dir.path().join("workspace"); std::fs::create_dir_all(&workspace_path).with_context(|| { @@ -96,7 +121,7 @@ where workspace_path .as_utf8_path() .canonicalize_utf8() - .with_context(|| "Failed to canonzialize workspace path.")?, + .with_context(|| "Failed to canonicalize workspace path.")?, ); for (relative_path, content) in workspace_files { @@ -115,25 +140,31 @@ where let system = OsSystem::new(&workspace_path); let workspace = WorkspaceMetadata::from_path(&workspace_path, &system)?; + let search_paths = create_search_paths(root_path, workspace.root()); + + for path in search_paths + .extra_paths + .iter() + .chain(search_paths.site_packages.iter()) + .chain(search_paths.custom_typeshed.iter()) + { + std::fs::create_dir_all(path.as_std_path()) + .with_context(|| format!("Failed to create search path '{path}'"))?; + } + let settings = ProgramSettings { target_version: TargetVersion::default(), - search_paths: SearchPathSettings { - extra_paths: vec![], - workspace_root: workspace.root().to_path_buf(), - custom_typeshed: None, - site_packages: None, - }, + search_paths, }; let db = RootDatabase::new(workspace, settings, system); let (sender, receiver) = crossbeam::channel::unbounded(); - let mut watcher = directory_watcher(move |events| sender.send(events).unwrap()) + let watcher = directory_watcher(move |events| sender.send(events).unwrap()) .with_context(|| "Failed to create directory watcher")?; - watcher - .watch(&workspace_path) - .with_context(|| "Failed to set up watcher for workspace directory.")?; + let watcher = WorkspaceWatcher::new(watcher, &db); + assert!(!watcher.has_errored_paths()); let test_case = TestCase { db, @@ -597,3 +628,114 @@ fn directory_deleted() -> anyhow::Result<()> { Ok(()) } + +#[test] +fn search_path() -> anyhow::Result<()> { + let mut case = + setup_with_search_paths([("bar.py", "import sub.a")], |root_path, workspace_path| { + SearchPathSettings { + extra_paths: vec![], + workspace_root: workspace_path.to_path_buf(), + custom_typeshed: None, + site_packages: Some(root_path.join("site_packages")), + } + })?; + + let site_packages = case.root_path().join("site_packages"); + + assert_eq!( + resolve_module(case.db(), ModuleName::new("a").unwrap()), + None + ); + + std::fs::write(site_packages.join("a.py").as_std_path(), "class A: ...")?; + std::fs::write(site_packages.join("__init__.py").as_std_path(), "")?; + + let changes = case.stop_watch(); + + case.db_mut().apply_changes(changes); + + assert!(resolve_module(case.db().upcast(), ModuleName::new_static("a").unwrap()).is_some()); + assert_eq!( + case.collect_package_files(&case.workspace_path("bar.py")), + &[case.system_file(case.workspace_path("bar.py")).unwrap()] + ); + + Ok(()) +} + +#[test] +fn add_search_path() -> anyhow::Result<()> { + let mut case = setup([("bar.py", "import sub.a")])?; + + let site_packages = case.workspace_path("site_packages"); + std::fs::create_dir_all(site_packages.as_std_path())?; + + assert!(resolve_module(case.db().upcast(), ModuleName::new_static("a").unwrap()).is_none()); + + let program = Program::get(case.db()); + let search_path_settings = program.search_paths(case.db()).clone(); + + // Register site-packages as a search path. + program + .set_search_paths(&mut case.db) + .to(SearchPathSettings { + site_packages: Some(site_packages.clone()), + ..search_path_settings + }); + + // Start observing the site-packages folder + let watcher = case.watcher.as_mut().unwrap(); + watcher.update(&case.db); + + assert!(!watcher.has_errored_paths()); + + std::fs::write(site_packages.join("a.py").as_std_path(), "class A: ...")?; + std::fs::write(site_packages.join("__init__.py").as_std_path(), "")?; + + let changes = case.stop_watch(); + + case.db_mut().apply_changes(changes); + + assert!(resolve_module(case.db().upcast(), ModuleName::new_static("a").unwrap()).is_some()); + + Ok(()) +} + +#[test] +fn remove_search_path() -> anyhow::Result<()> { + let mut case = + setup_with_search_paths([("bar.py", "import sub.a")], |root_path, workspace_path| { + SearchPathSettings { + extra_paths: vec![], + workspace_root: workspace_path.to_path_buf(), + custom_typeshed: None, + site_packages: Some(root_path.join("site_packages")), + } + })?; + + let site_packages = case.root_path().join("site_packages"); + + let program = Program::get(case.db()); + let search_path_settings = program.search_paths(case.db()).clone(); + + // Register site-packages from the search paths. + program + .set_search_paths(&mut case.db) + .to(SearchPathSettings { + site_packages: None, + ..search_path_settings + }); + + let watcher = case.watcher.as_mut().unwrap(); + watcher.update(&case.db); + assert!(!watcher.has_errored_paths()); + + std::fs::write(site_packages.join("a.py").as_std_path(), "class A: ...")?; + + let changes = case.stop_watch(); + + assert_eq!(changes, &[]); + + Ok(()) +} diff --git a/crates/red_knot_module_resolver/src/lib.rs b/crates/red_knot_module_resolver/src/lib.rs index bb145efbbd8dd..efc9cd2c6195a 100644 --- a/crates/red_knot_module_resolver/src/lib.rs +++ b/crates/red_knot_module_resolver/src/lib.rs @@ -1,3 +1,16 @@ +use std::iter::FusedIterator; + +pub use db::{Db, Jar}; +pub use module::{Module, ModuleKind}; +pub use module_name::ModuleName; +pub use resolver::resolve_module; +use ruff_db::system::SystemPath; +pub use typeshed::{ + vendored_typeshed_stubs, TypeshedVersionsParseError, TypeshedVersionsParseErrorKind, +}; + +use crate::resolver::{module_resolution_settings, SearchPathIterator}; + mod db; mod module; mod module_name; @@ -9,10 +22,29 @@ mod typeshed; #[cfg(test)] mod testing; -pub use db::{Db, Jar}; -pub use module::{Module, ModuleKind}; -pub use module_name::ModuleName; -pub use resolver::resolve_module; -pub use typeshed::{ - vendored_typeshed_stubs, TypeshedVersionsParseError, TypeshedVersionsParseErrorKind, -}; +/// Returns an iterator over all search paths pointing to a system path +pub fn system_module_search_paths(db: &dyn Db) -> SystemModuleSearchPathsIter { + SystemModuleSearchPathsIter { + inner: module_resolution_settings(db).search_paths(db), + } +} + +pub struct SystemModuleSearchPathsIter<'db> { + inner: SearchPathIterator<'db>, +} + +impl<'db> Iterator for SystemModuleSearchPathsIter<'db> { + type Item = &'db SystemPath; + + fn next(&mut self) -> Option { + loop { + let next = self.inner.next()?; + + if let Some(system_path) = next.as_system_path() { + return Some(system_path); + } + } + } +} + +impl FusedIterator for SystemModuleSearchPathsIter<'_> {} diff --git a/crates/red_knot_module_resolver/src/resolver.rs b/crates/red_knot_module_resolver/src/resolver.rs index 8849b73ee3144..9172f4f532840 100644 --- a/crates/red_knot_module_resolver/src/resolver.rs +++ b/crates/red_knot_module_resolver/src/resolver.rs @@ -258,7 +258,7 @@ pub(crate) fn editable_install_resolution_paths(db: &dyn Db) -> Vec { +pub(crate) struct SearchPathIterator<'db> { db: &'db dyn Db, static_paths: std::slice::Iter<'db, ModuleSearchPath>, dynamic_paths: Option>, @@ -399,7 +399,7 @@ impl ModuleResolutionSettings { self.target_version } - fn search_paths<'db>(&'db self, db: &'db dyn Db) -> SearchPathIterator<'db> { + pub(crate) fn search_paths<'db>(&'db self, db: &'db dyn Db) -> SearchPathIterator<'db> { SearchPathIterator { db, static_paths: self.static_search_paths.iter(), diff --git a/crates/red_knot_python_semantic/Cargo.toml b/crates/red_knot_python_semantic/Cargo.toml index b314905d7aa64..f6d1951add322 100644 --- a/crates/red_knot_python_semantic/Cargo.toml +++ b/crates/red_knot_python_semantic/Cargo.toml @@ -18,6 +18,7 @@ ruff_python_ast = { workspace = true } ruff_text_size = { workspace = true } bitflags = { workspace = true } +countme = { workspace = true } ordermap = { workspace = true } salsa = { workspace = true } tracing = { workspace = true } diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs index ea9b518b5db5b..855c8d8f76d87 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -103,9 +103,13 @@ impl<'db> SemanticIndexBuilder<'db> { #[allow(unsafe_code)] // SAFETY: `node` is guaranteed to be a child of `self.module` - let scope_id = ScopeId::new(self.db, self.file, file_scope_id, unsafe { - node.to_kind(self.module.clone()) - }); + let scope_id = ScopeId::new( + self.db, + self.file, + file_scope_id, + unsafe { node.to_kind(self.module.clone()) }, + countme::Count::default(), + ); self.scope_ids_by_scope.push(scope_id); self.scopes_by_node.insert(node.node_key(), file_scope_id); @@ -180,6 +184,7 @@ impl<'db> SemanticIndexBuilder<'db> { unsafe { definition_node.into_owned(self.module.clone()) }, + countme::Count::default(), ); self.definitions_by_node @@ -201,6 +206,7 @@ impl<'db> SemanticIndexBuilder<'db> { unsafe { AstNodeRef::new(self.module.clone(), expression_node) }, + countme::Count::default(), ); self.expressions_by_node .insert(expression_node.into(), expression); diff --git a/crates/red_knot_python_semantic/src/semantic_index/definition.rs b/crates/red_knot_python_semantic/src/semantic_index/definition.rs index ff114a5856858..f7be3f84c7df9 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/definition.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/definition.rs @@ -24,6 +24,9 @@ pub struct Definition<'db> { #[no_eq] #[return_ref] pub(crate) node: DefinitionKind, + + #[no_eq] + count: countme::Count>, } impl<'db> Definition<'db> { diff --git a/crates/red_knot_python_semantic/src/semantic_index/expression.rs b/crates/red_knot_python_semantic/src/semantic_index/expression.rs index 23f48ca416fdf..8dcbc44e28667 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/expression.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/expression.rs @@ -22,6 +22,9 @@ pub(crate) struct Expression<'db> { #[no_eq] #[return_ref] pub(crate) node: AstNodeRef, + + #[no_eq] + count: countme::Count>, } impl<'db> Expression<'db> { diff --git a/crates/red_knot_python_semantic/src/semantic_index/symbol.rs b/crates/red_knot_python_semantic/src/semantic_index/symbol.rs index a04331199c2ba..a0519d5c6cf94 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/symbol.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/symbol.rs @@ -100,6 +100,9 @@ pub struct ScopeId<'db> { #[no_eq] #[return_ref] pub node: NodeWithScopeKind, + + #[no_eq] + count: countme::Count>, } impl<'db> ScopeId<'db> { diff --git a/crates/ruff_db/src/program.rs b/crates/ruff_db/src/program.rs index 98298d1bdf583..01ac910d4e0af 100644 --- a/crates/ruff_db/src/program.rs +++ b/crates/ruff_db/src/program.rs @@ -65,7 +65,7 @@ impl std::fmt::Debug for TargetVersion { } /// Configures the search paths for module resolution. -#[derive(Eq, PartialEq, Debug)] +#[derive(Eq, PartialEq, Debug, Clone)] pub struct SearchPathSettings { /// List of user-provided paths that should take first priority in the module resolution. /// Examples in other type checkers are mypy's MYPYPATH environment variable, diff --git a/crates/ruff_db/src/system.rs b/crates/ruff_db/src/system.rs index b346ffb346baa..ae3544af22690 100644 --- a/crates/ruff_db/src/system.rs +++ b/crates/ruff_db/src/system.rs @@ -9,7 +9,9 @@ use walk_directory::WalkDirectoryBuilder; use crate::file_revision::FileRevision; -pub use self::path::{SystemPath, SystemPathBuf}; +pub use self::path::{ + deduplicate_nested_paths, DeduplicatedNestedPathsIter, SystemPath, SystemPathBuf, +}; mod memory_fs; #[cfg(feature = "os")] diff --git a/crates/ruff_db/src/system/path.rs b/crates/ruff_db/src/system/path.rs index 114b7d08d41a9..3c4142af61771 100644 --- a/crates/ruff_db/src/system/path.rs +++ b/crates/ruff_db/src/system/path.rs @@ -563,3 +563,58 @@ impl ruff_cache::CacheKey for SystemPathBuf { self.as_path().cache_key(hasher); } } + +/// Deduplicates identical paths and removes nested paths. +/// +/// # Examples +/// ```rust +/// use ruff_db::system::{SystemPath, deduplicate_nested_paths};/// +/// +/// let paths = vec![SystemPath::new("/a/b/c"), SystemPath::new("/a/b"), SystemPath::new("/a/beta"), SystemPath::new("/a/b/c")]; +/// assert_eq!(deduplicate_nested_paths(paths).collect::>(), &[SystemPath::new("/a/b"), SystemPath::new("/a/beta")]); +/// ``` +pub fn deduplicate_nested_paths<'a, I>(paths: I) -> DeduplicatedNestedPathsIter<'a> +where + I: IntoIterator, +{ + DeduplicatedNestedPathsIter::new(paths) +} + +pub struct DeduplicatedNestedPathsIter<'a> { + inner: std::vec::IntoIter<&'a SystemPath>, + next: Option<&'a SystemPath>, +} + +impl<'a> DeduplicatedNestedPathsIter<'a> { + fn new(paths: I) -> Self + where + I: IntoIterator, + { + let mut paths = paths.into_iter().collect::>(); + paths.sort_unstable(); + let mut iter = paths.into_iter(); + + Self { + next: iter.next(), + inner: iter, + } + } +} + +impl<'a> Iterator for DeduplicatedNestedPathsIter<'a> { + type Item = &'a SystemPath; + + fn next(&mut self) -> Option { + let current = self.next.take()?; + + for next in self.inner.by_ref() { + // Skip all paths that have the same prefix as the current path + if !next.starts_with(current) { + self.next = Some(next); + break; + } + } + + Some(current) + } +} From 0dcb095fba8de983b3d706c730adcd3de91b92a5 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 24 Jul 2024 09:34:14 +0200 Subject: [PATCH 2/2] Address code review feedback --- .../red_knot/src/watch/workspace_watcher.rs | 2 +- crates/red_knot/src/workspace.rs | 2 +- crates/red_knot/tests/file_watching.rs | 56 +++++++++---------- crates/ruff_db/src/system/path.rs | 2 + 4 files changed, 30 insertions(+), 32 deletions(-) diff --git a/crates/red_knot/src/watch/workspace_watcher.rs b/crates/red_knot/src/watch/workspace_watcher.rs index d549e4822c0c0..7853c11201d3e 100644 --- a/crates/red_knot/src/watch/workspace_watcher.rs +++ b/crates/red_knot/src/watch/workspace_watcher.rs @@ -32,7 +32,7 @@ impl WorkspaceWatcher { } pub fn update(&mut self, db: &RootDatabase) { - let new_watch_paths = db.workspace().watch_paths(db); + let new_watch_paths = db.workspace().paths_to_watch(db); let mut added_folders = new_watch_paths.difference(&self.watched_paths).peekable(); let mut removed_folders = self.watched_paths.difference(&new_watch_paths).peekable(); diff --git a/crates/red_knot/src/workspace.rs b/crates/red_knot/src/workspace.rs index 1c0cd743dd2ba..cfd9f7a91ba9c 100644 --- a/crates/red_knot/src/workspace.rs +++ b/crates/red_knot/src/workspace.rs @@ -245,7 +245,7 @@ impl Workspace { /// Returns the paths that should be watched. /// /// The paths that require watching might change with every revision. - pub fn watch_paths(self, db: &dyn Db) -> FxHashSet { + pub fn paths_to_watch(self, db: &dyn Db) -> FxHashSet { ruff_db::system::deduplicate_nested_paths( std::iter::once(self.root(db)).chain(system_module_search_paths(db.upcast())), ) diff --git a/crates/red_knot/tests/file_watching.rs b/crates/red_knot/tests/file_watching.rs index 82b45c0e29e09..26ade56707e47 100644 --- a/crates/red_knot/tests/file_watching.rs +++ b/crates/red_knot/tests/file_watching.rs @@ -55,6 +55,23 @@ impl TestCase { all_events } + fn update_search_path_settings( + &mut self, + f: impl FnOnce(&SearchPathSettings) -> SearchPathSettings, + ) { + let program = Program::get(self.db()); + let search_path_settings = program.search_paths(self.db()); + + let new_settings = f(search_path_settings); + + program.set_search_paths(&mut self.db).to(new_settings); + + if let Some(watcher) = &mut self.watcher { + watcher.update(&self.db); + assert!(!watcher.has_errored_paths()); + } + } + fn collect_package_files(&self, path: &SystemPath) -> Vec { let package = self.db().workspace().package(self.db(), path).unwrap(); let files = package.files(self.db()); @@ -673,22 +690,11 @@ fn add_search_path() -> anyhow::Result<()> { assert!(resolve_module(case.db().upcast(), ModuleName::new_static("a").unwrap()).is_none()); - let program = Program::get(case.db()); - let search_path_settings = program.search_paths(case.db()).clone(); - // Register site-packages as a search path. - program - .set_search_paths(&mut case.db) - .to(SearchPathSettings { - site_packages: Some(site_packages.clone()), - ..search_path_settings - }); - - // Start observing the site-packages folder - let watcher = case.watcher.as_mut().unwrap(); - watcher.update(&case.db); - - assert!(!watcher.has_errored_paths()); + case.update_search_path_settings(|settings| SearchPathSettings { + site_packages: Some(site_packages.clone()), + ..settings.clone() + }); std::fs::write(site_packages.join("a.py").as_std_path(), "class A: ...")?; std::fs::write(site_packages.join("__init__.py").as_std_path(), "")?; @@ -714,22 +720,12 @@ fn remove_search_path() -> anyhow::Result<()> { } })?; + // Remove site packages from the search path settings. let site_packages = case.root_path().join("site_packages"); - - let program = Program::get(case.db()); - let search_path_settings = program.search_paths(case.db()).clone(); - - // Register site-packages from the search paths. - program - .set_search_paths(&mut case.db) - .to(SearchPathSettings { - site_packages: None, - ..search_path_settings - }); - - let watcher = case.watcher.as_mut().unwrap(); - watcher.update(&case.db); - assert!(!watcher.has_errored_paths()); + case.update_search_path_settings(|settings| SearchPathSettings { + site_packages: None, + ..settings.clone() + }); std::fs::write(site_packages.join("a.py").as_std_path(), "class A: ...")?; diff --git a/crates/ruff_db/src/system/path.rs b/crates/ruff_db/src/system/path.rs index 3c4142af61771..6c5cf873cadb9 100644 --- a/crates/ruff_db/src/system/path.rs +++ b/crates/ruff_db/src/system/path.rs @@ -591,7 +591,9 @@ impl<'a> DeduplicatedNestedPathsIter<'a> { I: IntoIterator, { let mut paths = paths.into_iter().collect::>(); + // Sort the path to ensure that e.g. `/a/b/c`, comes right after `/a/b`. paths.sort_unstable(); + let mut iter = paths.into_iter(); Self {