diff --git a/clippy.toml b/clippy.toml index 777fbb8c92ea9..12052f24fd26e 100644 --- a/clippy.toml +++ b/clippy.toml @@ -10,12 +10,12 @@ doc-valid-idents = [ "SCREAMING_SNAKE_CASE", "SQLAlchemy", "StackOverflow", + "PyCharm", ] ignore-interior-mutability = [ # Interned is read-only. The wrapped `Rc` never gets updated. "ruff_formatter::format_element::Interned", - - # The expression is read-only. + # The expression is read-only. "ruff_python_ast::hashable::HashableExpr", ] diff --git a/crates/red_knot/Cargo.toml b/crates/red_knot/Cargo.toml index 5434852061d00..ad59355a18642 100644 --- a/crates/red_knot/Cargo.toml +++ b/crates/red_knot/Cargo.toml @@ -27,12 +27,12 @@ notify = { workspace = true } rayon = { workspace = true } rustc-hash = { workspace = true } salsa = { workspace = true } -filetime = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true } tracing-tree = { workspace = true } [dev-dependencies] +filetime = { workspace = true } tempfile = { workspace = true } diff --git a/crates/red_knot/src/db.rs b/crates/red_knot/src/db.rs index 5dbc44a90f938..f2bbe5087eed3 100644 --- a/crates/red_knot/src/db.rs +++ b/crates/red_knot/src/db.rs @@ -1,7 +1,4 @@ use std::panic::{AssertUnwindSafe, RefUnwindSafe}; -use std::sync::Arc; - -use salsa::Cancelled; use red_knot_module_resolver::{vendored_typeshed_stubs, Db as ResolverDb}; use red_knot_python_semantic::Db as SemanticDb; @@ -10,6 +7,7 @@ use ruff_db::program::{Program, ProgramSettings}; use ruff_db::system::System; use ruff_db::vendored::VendoredFileSystem; use ruff_db::{Db as SourceDb, Upcast}; +use salsa::Cancelled; use crate::lint::Diagnostics; use crate::workspace::{check_file, Workspace, WorkspaceMetadata}; @@ -24,7 +22,7 @@ pub struct RootDatabase { workspace: Option, storage: salsa::Storage, files: Files, - system: Arc, + system: Box, } impl RootDatabase { @@ -36,7 +34,7 @@ impl RootDatabase { workspace: None, storage: salsa::Storage::default(), files: Files::default(), - system: Arc::new(system), + system: Box::new(system), }; let workspace = Workspace::from_metadata(&db, workspace); diff --git a/crates/red_knot/tests/file_watching.rs b/crates/red_knot/tests/file_watching.rs index 6d0cac87cfd0d..bcbaf9507f2a9 100644 --- a/crates/red_knot/tests/file_watching.rs +++ b/crates/red_knot/tests/file_watching.rs @@ -1,9 +1,9 @@ #![allow(clippy::disallowed_names)] +use std::io::Write; use std::time::Duration; use anyhow::{anyhow, Context}; -use filetime::FileTime; use salsa::Setter; use red_knot::db::RootDatabase; @@ -21,7 +21,10 @@ struct TestCase { db: RootDatabase, watcher: Option, changes_receiver: crossbeam::channel::Receiver>, - temp_dir: tempfile::TempDir, + /// The temporary directory that contains the test files. + /// We need to hold on to it in the test case or the temp files get deleted. + _temp_dir: tempfile::TempDir, + root_dir: SystemPathBuf, } impl TestCase { @@ -30,7 +33,7 @@ impl TestCase { } fn root_path(&self) -> &SystemPath { - SystemPath::from_std_path(self.temp_dir.path()).unwrap() + &self.root_dir } fn db(&self) -> &RootDatabase { @@ -42,19 +45,63 @@ impl TestCase { } fn stop_watch(&mut self) -> Vec { - if let Some(watcher) = self.watcher.take() { - // Give the watcher some time to catch up. - std::thread::sleep(Duration::from_millis(10)); - watcher.flush(); - watcher.stop(); + self.try_stop_watch(Duration::from_secs(10)) + .expect("Expected watch changes but observed none.") + } + + fn try_stop_watch(&mut self, timeout: Duration) -> Option> { + let watcher = self + .watcher + .take() + .expect("Cannot call `stop_watch` more than once."); + + let mut all_events = self + .changes_receiver + .recv_timeout(timeout) + .unwrap_or_default(); + watcher.flush(); + watcher.stop(); + + for event in &self.changes_receiver { + all_events.extend(event); } - let mut all_events = Vec::new(); - for events in &self.changes_receiver { - all_events.extend(events); + if all_events.is_empty() { + return None; } - all_events + Some(all_events) + } + + #[cfg(unix)] + fn take_watch_changes(&self) -> Vec { + self.try_take_watch_changes(Duration::from_secs(10)) + .expect("Expected watch changes but observed none.") + } + + fn try_take_watch_changes(&self, timeout: Duration) -> Option> { + let Some(watcher) = &self.watcher else { + return None; + }; + + let mut all_events = self + .changes_receiver + .recv_timeout(timeout) + .unwrap_or_default(); + watcher.flush(); + + while let Ok(event) = self + .changes_receiver + .recv_timeout(Duration::from_millis(10)) + { + all_events.extend(event); + watcher.flush(); + } + + if all_events.is_empty() { + return None; + } + Some(all_events) } fn update_search_path_settings( @@ -88,28 +135,62 @@ impl TestCase { } } -fn setup(workspace_files: I) -> anyhow::Result +trait SetupFiles { + fn setup(self, root_path: &SystemPath, workspace_path: &SystemPath) -> anyhow::Result<()>; +} + +impl SetupFiles for [(P, &'static str); N] 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(self, _root_path: &SystemPath, workspace_path: &SystemPath) -> anyhow::Result<()> { + for (relative_path, content) in self { + let relative_path = relative_path.as_ref(); + let absolute_path = workspace_path.join(relative_path); + if let Some(parent) = absolute_path.parent() { + std::fs::create_dir_all(parent).with_context(|| { + format!("Failed to create parent directory for file '{relative_path}'.",) + })?; + } + + let mut file = std::fs::File::create(absolute_path.as_std_path()) + .with_context(|| format!("Failed to open file '{relative_path}'"))?; + file.write_all(content.as_bytes()) + .with_context(|| format!("Failed to write to file '{relative_path}'"))?; + file.sync_data()?; } + + Ok(()) + } +} + +impl SetupFiles for F +where + F: FnOnce(&SystemPath, &SystemPath) -> anyhow::Result<()>, +{ + fn setup(self, root_path: &SystemPath, workspace_path: &SystemPath) -> anyhow::Result<()> { + self(root_path, workspace_path) + } +} + +fn setup(setup_files: F) -> anyhow::Result +where + F: SetupFiles, +{ + setup_with_search_paths(setup_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, +fn setup_with_search_paths( + setup_files: F, create_search_paths: impl FnOnce(&SystemPath, &SystemPath) -> SearchPathSettings, ) -> anyhow::Result where - I: IntoIterator, - P: AsRef, + F: SetupFiles, { let temp_dir = tempfile::tempdir()?; @@ -132,18 +213,9 @@ where std::fs::create_dir_all(workspace_path.as_std_path()) .with_context(|| format!("Failed to create workspace directory '{workspace_path}'",))?; - for (relative_path, content) in workspace_files { - let relative_path = relative_path.as_ref(); - let absolute_path = workspace_path.join(relative_path); - if let Some(parent) = absolute_path.parent() { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directory for file '{relative_path}'.",) - })?; - } - - std::fs::write(absolute_path.as_std_path(), content) - .with_context(|| format!("Failed to write file '{relative_path}'"))?; - } + setup_files + .setup(&root_path, &workspace_path) + .context("Failed to setup test files")?; let system = OsSystem::new(&workspace_path); @@ -178,30 +250,43 @@ where db, changes_receiver: receiver, watcher: Some(watcher), - temp_dir, + _temp_dir: temp_dir, + root_dir: root_path, }; + // Sometimes the file watcher reports changes for events that happened before the watcher was started. + // Do a best effort at dropping these events. + test_case.try_take_watch_changes(Duration::from_millis(100)); + Ok(test_case) } -/// The precision of the last modified time is platform dependent and not arbitrarily precise. -/// This method sleeps until the last modified time of a newly created file changes. This guarantees -/// that the last modified time of any file written **after** this method completes should be different. -fn next_io_tick() { - let temp = tempfile::tempfile().unwrap(); +/// Updates the content of a file and ensures that the last modified file time is updated. +fn update_file(path: impl AsRef, content: &str) -> anyhow::Result<()> { + let path = path.as_ref().as_std_path(); + + let metadata = path.metadata()?; + let last_modified_time = filetime::FileTime::from_last_modification_time(&metadata); - let last_modified = FileTime::from_last_modification_time(&temp.metadata().unwrap()); + let mut file = std::fs::OpenOptions::new() + .create(false) + .write(true) + .truncate(true) + .open(path)?; + file.write_all(content.as_bytes())?; loop { - filetime::set_file_handle_times(&temp, None, Some(FileTime::now())).unwrap(); + file.sync_all()?; - let new_last_modified = FileTime::from_last_modification_time(&temp.metadata().unwrap()); + let modified_time = filetime::FileTime::from_last_modification_time(&path.metadata()?); - if new_last_modified != last_modified { - break; + if modified_time != last_modified_time { + break Ok(()); } - std::thread::sleep(Duration::from_nanos(100)); + std::thread::sleep(Duration::from_nanos(10)); + + filetime::set_file_handle_times(&file, None, Some(filetime::FileTime::now()))?; } } @@ -260,8 +345,7 @@ fn changed_file() -> anyhow::Result<()> { assert_eq!(source_text(case.db(), foo).as_str(), foo_source); assert_eq!(&case.collect_package_files(&foo_path), &[foo]); - next_io_tick(); - std::fs::write(foo_path.as_std_path(), "print('Version 2')")?; + update_file(&foo_path, "print('Version 2')")?; let changes = case.stop_watch(); @@ -275,49 +359,6 @@ fn changed_file() -> anyhow::Result<()> { Ok(()) } -#[cfg(unix)] -#[test] -fn changed_metadata() -> anyhow::Result<()> { - use std::os::unix::fs::PermissionsExt; - - let mut case = setup([("foo.py", "")])?; - let foo_path = case.workspace_path("foo.py"); - - let foo = case.system_file(&foo_path)?; - assert_eq!( - foo.permissions(case.db()), - Some( - std::fs::metadata(foo_path.as_std_path()) - .unwrap() - .permissions() - .mode() - ) - ); - - next_io_tick(); - std::fs::set_permissions( - foo_path.as_std_path(), - std::fs::Permissions::from_mode(0o777), - ) - .with_context(|| "Failed to set file permissions.")?; - - let changes = case.stop_watch(); - - case.db_mut().apply_changes(changes); - - assert_eq!( - foo.permissions(case.db()), - Some( - std::fs::metadata(foo_path.as_std_path()) - .unwrap() - .permissions() - .mode() - ) - ); - - Ok(()) -} - #[test] fn deleted_file() -> anyhow::Result<()> { let foo_source = "print('Hello, world!')"; @@ -495,7 +536,7 @@ fn directory_moved_to_trash() -> anyhow::Result<()> { ])?; let bar = case.system_file(case.workspace_path("bar.py")).unwrap(); - assert!(resolve_module(case.db().upcast(), ModuleName::new_static("sub.a").unwrap()).is_some(),); + assert!(resolve_module(case.db().upcast(), ModuleName::new_static("sub.a").unwrap()).is_some()); let sub_path = case.workspace_path("sub"); let init_file = case @@ -729,9 +770,464 @@ fn remove_search_path() -> anyhow::Result<()> { std::fs::write(site_packages.join("a.py").as_std_path(), "class A: ...")?; + let changes = case.try_stop_watch(Duration::from_millis(100)); + + assert_eq!(changes, None); + + Ok(()) +} + +/// Watch a workspace that contains two files where one file is a hardlink to another. +/// +/// Setup: +/// ```text +/// - workspace +/// |- foo.py +/// |- bar.py (hard link to foo.py) +/// ``` +/// +/// # Linux +/// `inotify` only emits a single change event for the file that was changed. +/// Other files that point to the same inode (hardlinks) won't get updated. +/// +/// For reference: VS Code and PyCharm have the same behavior where the results for one of the +/// files are stale. +/// +/// # Windows +/// I haven't found any documentation that states the notification behavior on Windows but what +/// we're seeing is that Windows only emits a single event, similar to Linux. +#[test] +fn hard_links_in_workspace() -> anyhow::Result<()> { + let mut case = setup(|_root: &SystemPath, workspace: &SystemPath| { + let foo_path = workspace.join("foo.py"); + std::fs::write(foo_path.as_std_path(), "print('Version 1')")?; + + // Create a hardlink to `foo` + let bar_path = workspace.join("bar.py"); + std::fs::hard_link(foo_path.as_std_path(), bar_path.as_std_path()) + .context("Failed to create hard link from foo.py -> bar.py")?; + + Ok(()) + })?; + + let foo_path = case.workspace_path("foo.py"); + let foo = case.system_file(&foo_path).unwrap(); + let bar_path = case.workspace_path("bar.py"); + let bar = case.system_file(&bar_path).unwrap(); + + assert_eq!(source_text(case.db(), foo).as_str(), "print('Version 1')"); + assert_eq!(source_text(case.db(), bar).as_str(), "print('Version 1')"); + + // Write to the hard link target. + update_file(foo_path, "print('Version 2')").context("Failed to update foo.py")?; + + let changes = case.stop_watch(); + + case.db_mut().apply_changes(changes); + + assert_eq!(source_text(case.db(), foo).as_str(), "print('Version 2')"); + + // macOS is the only platform that emits events for every hardlink. + if cfg!(target_os = "macos") { + assert_eq!(source_text(case.db(), bar).as_str(), "print('Version 2')"); + } + + Ok(()) +} + +/// Watch a workspace that contains one file that is a hardlink to a file outside the workspace. +/// +/// Setup: +/// ```text +/// - foo.py +/// - workspace +/// |- bar.py (hard link to /foo.py) +/// ``` +/// +/// # Linux +/// inotiyf doesn't support observing changes to hard linked files. +/// +/// > Note: when monitoring a directory, events are not generated for +/// > the files inside the directory when the events are performed via +/// > a pathname (i.e., a link) that lies outside the monitored +/// > directory. [source](https://man7.org/linux/man-pages/man7/inotify.7.html) +/// +/// # Windows +/// > Retrieves information that describes the changes within the specified directory. +/// +/// [source](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-readdirectorychangesw) +/// +/// My interpretation of this is that Windows doesn't support observing changes made to +/// hard linked files outside the workspace. +#[test] +#[cfg_attr( + target_os = "linux", + ignore = "inotify doesn't support observing changes to hard linked files." +)] +#[cfg_attr( + target_os = "windows", + ignore = "windows doesn't support observing changes to hard linked files." +)] +fn hard_links_to_target_outside_workspace() -> anyhow::Result<()> { + let mut case = setup(|root: &SystemPath, workspace: &SystemPath| { + let foo_path = root.join("foo.py"); + std::fs::write(foo_path.as_std_path(), "print('Version 1')")?; + + // Create a hardlink to `foo` + let bar_path = workspace.join("bar.py"); + std::fs::hard_link(foo_path.as_std_path(), bar_path.as_std_path()) + .context("Failed to create hard link from foo.py -> bar.py")?; + + Ok(()) + })?; + + let foo_path = case.root_path().join("foo.py"); + let foo = case.system_file(&foo_path).unwrap(); + let bar_path = case.workspace_path("bar.py"); + let bar = case.system_file(&bar_path).unwrap(); + + assert_eq!(source_text(case.db(), foo).as_str(), "print('Version 1')"); + assert_eq!(source_text(case.db(), bar).as_str(), "print('Version 1')"); + + // Write to the hard link target. + update_file(foo_path, "print('Version 2')").context("Failed to update foo.py")?; + let changes = case.stop_watch(); - assert_eq!(changes, &[]); + case.db_mut().apply_changes(changes); + + assert_eq!(source_text(case.db(), bar).as_str(), "print('Version 2')"); Ok(()) } + +#[cfg(unix)] +mod unix { + //! Tests that make use of unix specific file-system features. + use super::*; + + /// Changes the metadata of the only file in the workspace. + #[test] + fn changed_metadata() -> anyhow::Result<()> { + use std::os::unix::fs::PermissionsExt; + + let mut case = setup([("foo.py", "")])?; + let foo_path = case.workspace_path("foo.py"); + + let foo = case.system_file(&foo_path)?; + assert_eq!( + foo.permissions(case.db()), + Some( + std::fs::metadata(foo_path.as_std_path()) + .unwrap() + .permissions() + .mode() + ) + ); + + std::fs::set_permissions( + foo_path.as_std_path(), + std::fs::Permissions::from_mode(0o777), + ) + .with_context(|| "Failed to set file permissions.")?; + + let changes = case.stop_watch(); + + case.db_mut().apply_changes(changes); + + assert_eq!( + foo.permissions(case.db()), + Some( + std::fs::metadata(foo_path.as_std_path()) + .unwrap() + .permissions() + .mode() + ) + ); + + Ok(()) + } + + /// A workspace path is a symlink to a file outside the workspace. + /// + /// Setup: + /// ```text + /// - bar + /// |- baz.py + /// + /// - workspace + /// |- bar -> /bar + /// ``` + /// + /// # macOS + /// This test case isn't supported on macOS. + /// macOS uses `FSEvents` and `FSEvents` doesn't emit an event if a file in a symlinked directory is changed. + /// + /// > Generally speaking, when working with file system event notifications, you will probably want to use lstat, + /// > because changes to the underlying file will not result in a change notification for the directory containing + /// > the symbolic link to that file. However, if you are working with a controlled file structure in + /// > which symbolic links always point within your watched tree, you might have reason to use stat. + /// + /// [source](https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/FSEvents_ProgGuide/UsingtheFSEventsFramework/UsingtheFSEventsFramework.html#//apple_ref/doc/uid/TP40005289-CH4-SW4) + /// + /// Pyright also does not support this case. + #[test] + #[cfg_attr( + target_os = "macos", + ignore = "FSEvents doesn't emit change events for symlinked directories outside of the watched paths." + )] + fn symlink_target_outside_watched_paths() -> anyhow::Result<()> { + let mut case = setup(|root: &SystemPath, workspace: &SystemPath| { + // Set up the symlink target. + let link_target = root.join("bar"); + std::fs::create_dir_all(link_target.as_std_path()) + .context("Failed to create link target directory")?; + let baz_original = link_target.join("baz.py"); + std::fs::write(baz_original.as_std_path(), "def baz(): ...") + .context("Failed to write link target file")?; + + // Create a symlink inside the workspace + let bar = workspace.join("bar"); + std::os::unix::fs::symlink(link_target.as_std_path(), bar.as_std_path()) + .context("Failed to create symlink to bar package")?; + + Ok(()) + })?; + + let baz = resolve_module( + case.db().upcast(), + ModuleName::new_static("bar.baz").unwrap(), + ) + .expect("Expected bar.baz to exist in site-packages."); + let baz_workspace = case.workspace_path("bar/baz.py"); + + assert_eq!( + source_text(case.db(), baz.file()).as_str(), + "def baz(): ..." + ); + assert_eq!( + baz.file().path(case.db()).as_system_path(), + Some(&*baz_workspace) + ); + + let baz_original = case.root_path().join("bar/baz.py"); + + // Write to the symlink target. + update_file(baz_original, "def baz(): print('Version 2')") + .context("Failed to update bar/baz.py")?; + + let changes = case.take_watch_changes(); + + case.db_mut().apply_changes(changes); + + assert_eq!( + source_text(case.db(), baz.file()).as_str(), + "def baz(): print('Version 2')" + ); + + // Write to the symlink source. + update_file(baz_workspace, "def baz(): print('Version 3')") + .context("Failed to update bar/baz.py")?; + + let changes = case.stop_watch(); + + case.db_mut().apply_changes(changes); + + assert_eq!( + source_text(case.db(), baz.file()).as_str(), + "def baz(): print('Version 3')" + ); + + Ok(()) + } + + /// Workspace contains a symlink to another directory inside the workspace. + /// Changes to files in the symlinked directory should be reflected + /// to all files. + /// + /// Setup: + /// ```text + /// - workspace + /// | - bar -> /workspace/patched/bar + /// | + /// | - patched + /// | |-- bar + /// | | |- baz.py + /// | + /// |-- foo.py + /// ``` + #[test] + fn symlink_inside_workspace() -> anyhow::Result<()> { + let mut case = setup(|_root: &SystemPath, workspace: &SystemPath| { + // Set up the symlink target. + let link_target = workspace.join("patched/bar"); + std::fs::create_dir_all(link_target.as_std_path()) + .context("Failed to create link target directory")?; + let baz_original = link_target.join("baz.py"); + std::fs::write(baz_original.as_std_path(), "def baz(): ...") + .context("Failed to write link target file")?; + + // Create a symlink inside site-packages + let bar_in_workspace = workspace.join("bar"); + std::os::unix::fs::symlink(link_target.as_std_path(), bar_in_workspace.as_std_path()) + .context("Failed to create symlink to bar package")?; + + Ok(()) + })?; + + let baz = resolve_module( + case.db().upcast(), + ModuleName::new_static("bar.baz").unwrap(), + ) + .expect("Expected bar.baz to exist in site-packages."); + let bar_baz = case.workspace_path("bar/baz.py"); + + let patched_bar_baz = case.workspace_path("patched/bar/baz.py"); + let patched_bar_baz_file = case.system_file(&patched_bar_baz).unwrap(); + + assert_eq!( + source_text(case.db(), patched_bar_baz_file).as_str(), + "def baz(): ..." + ); + + assert_eq!( + source_text(case.db(), baz.file()).as_str(), + "def baz(): ..." + ); + assert_eq!(baz.file().path(case.db()).as_system_path(), Some(&*bar_baz)); + + // Write to the symlink target. + update_file(&patched_bar_baz, "def baz(): print('Version 2')") + .context("Failed to update bar/baz.py")?; + + let changes = case.stop_watch(); + + case.db_mut().apply_changes(changes); + + // The file watcher is guaranteed to emit one event for the changed file, but it isn't specified + // if the event is emitted for the "original" or linked path because both paths are watched. + // The best we can assert here is that one of the files should have been updated. + // + // In a perfect world, the file watcher would emit two events, one for the original file and + // one for the symlink. I tried parcel/watcher, node's `fs.watch` and `chokidar` and + // only `chokidar seems to support it (used by Pyright). + // + // I further tested how good editor support is for symlinked files and it is not good ;) + // * VS Code doesn't update the file content if a file gets changed through a symlink + // * PyCharm doesn't update diagnostics if a symlinked module is changed (same as red knot). + // + // That's why I think it's fine to not support this case for now. + + let patched_baz_text = source_text(case.db(), patched_bar_baz_file); + let did_update_patched_baz = patched_baz_text.as_str() == "def baz(): print('Version 2')"; + + let bar_baz_text = source_text(case.db(), baz.file()); + let did_update_bar_baz = bar_baz_text.as_str() == "def baz(): print('Version 2')"; + + assert!( + did_update_patched_baz || did_update_bar_baz, + "Expected one of the files to be updated but neither file was updated.\nOriginal: {patched_baz_text}\nSymlinked: {bar_baz_text}", + patched_baz_text = patched_baz_text.as_str(), + bar_baz_text = bar_baz_text.as_str() + ); + + Ok(()) + } + + /// A module search path is a symlink. + /// + /// Setup: + /// ```text + /// - site-packages + /// | - bar/baz.py + /// + /// - workspace + /// |-- .venv/lib/python3.12/site-packages -> /site-packages + /// | + /// |-- foo.py + /// ``` + #[test] + fn symlinked_module_search_path() -> anyhow::Result<()> { + let mut case = setup_with_search_paths( + |root: &SystemPath, workspace: &SystemPath| { + // Set up the symlink target. + let site_packages = root.join("site-packages"); + let bar = site_packages.join("bar"); + std::fs::create_dir_all(bar.as_std_path()) + .context("Failed to create bar directory")?; + let baz_original = bar.join("baz.py"); + std::fs::write(baz_original.as_std_path(), "def baz(): ...") + .context("Failed to write baz.py")?; + + // Symlink the site packages in the venv to the global site packages + let venv_site_packages = workspace.join(".venv/lib/python3.12/site-packages"); + std::fs::create_dir_all(venv_site_packages.parent().unwrap()) + .context("Failed to create .venv directory")?; + std::os::unix::fs::symlink( + site_packages.as_std_path(), + venv_site_packages.as_std_path(), + ) + .context("Failed to create symlink to site-packages")?; + + Ok(()) + }, + |_root, workspace| SearchPathSettings { + extra_paths: vec![], + workspace_root: workspace.to_path_buf(), + custom_typeshed: None, + site_packages: Some(workspace.join(".venv/lib/python3.12/site-packages")), + }, + )?; + + let baz = resolve_module( + case.db().upcast(), + ModuleName::new_static("bar.baz").unwrap(), + ) + .expect("Expected bar.baz to exist in site-packages."); + let baz_site_packages = + case.workspace_path(".venv/lib/python3.12/site-packages/bar/baz.py"); + let baz_original = case.root_path().join("site-packages/bar/baz.py"); + let baz_original_file = case.system_file(&baz_original).unwrap(); + + assert_eq!( + source_text(case.db(), baz_original_file).as_str(), + "def baz(): ..." + ); + + assert_eq!( + source_text(case.db(), baz.file()).as_str(), + "def baz(): ..." + ); + assert_eq!( + baz.file().path(case.db()).as_system_path(), + Some(&*baz_site_packages) + ); + + // Write to the symlink target. + update_file(&baz_original, "def baz(): print('Version 2')") + .context("Failed to update bar/baz.py")?; + + let changes = case.stop_watch(); + + case.db_mut().apply_changes(changes); + + assert_eq!( + source_text(case.db(), baz.file()).as_str(), + "def baz(): print('Version 2')" + ); + + // It would be nice if this is supported but the underlying file system watchers + // only emit a single event. For reference + // * VS Code doesn't update the file content if a file gets changed through a symlink + // * PyCharm doesn't update diagnostics if a symlinked module is changed (same as red knot). + // We could add support for it by keeping a reverse map from `real_path` to symlinked path but + // it doesn't seem worth doing considering that as prominent tools like PyCharm don't support it. + // Pyright does support it, thanks to chokidar. + assert_ne!( + source_text(case.db(), baz_original_file).as_str(), + "def baz(): print('Version 2')" + ); + + Ok(()) + } +} diff --git a/crates/red_knot_module_resolver/src/resolver.rs b/crates/red_knot_module_resolver/src/resolver.rs index 8587cbe8193b8..a1c5f46a6bc8d 100644 --- a/crates/red_knot_module_resolver/src/resolver.rs +++ b/crates/red_knot_module_resolver/src/resolver.rs @@ -2,12 +2,11 @@ use std::borrow::Cow; use std::iter::FusedIterator; use once_cell::sync::Lazy; -use rustc_hash::{FxBuildHasher, FxHashSet}; - use ruff_db::files::{File, FilePath, FileRootKind}; use ruff_db::program::{Program, SearchPathSettings, TargetVersion}; use ruff_db::system::{DirectoryEntry, System, SystemPath, SystemPathBuf}; use ruff_db::vendored::VendoredPath; +use rustc_hash::{FxBuildHasher, FxHashSet}; use crate::db::Db; use crate::module::{Module, ModuleKind}; @@ -625,13 +624,12 @@ impl PackageKind { #[cfg(test)] mod tests { use ruff_db::files::{system_path_to_file, File, FilePath}; - use ruff_db::system::{DbWithTestSystem, OsSystem, SystemPath}; + use ruff_db::system::DbWithTestSystem; use ruff_db::testing::{ assert_const_function_query_was_not_run, assert_function_query_was_not_run, }; use ruff_db::Db; - use crate::db::tests::TestDb; use crate::module::ModuleKind; use crate::module_name::ModuleName; use crate::testing::{FileSpec, MockedTypeshed, TestCase, TestCaseBuilder}; @@ -1153,7 +1151,9 @@ mod tests { #[test] #[cfg(target_family = "unix")] fn symlink() -> anyhow::Result<()> { + use crate::db::tests::TestDb; use ruff_db::program::Program; + use ruff_db::system::{OsSystem, SystemPath}; let mut db = TestDb::new(); diff --git a/crates/ruff_db/src/system.rs b/crates/ruff_db/src/system.rs index ca7d4cb74805a..ed1cea552d7ab 100644 --- a/crates/ruff_db/src/system.rs +++ b/crates/ruff_db/src/system.rs @@ -39,6 +39,8 @@ pub type Result = std::io::Result; /// Abstracting the system also enables tests to use a more efficient in-memory file system. pub trait System: Debug { /// Reads the metadata of the file or directory at `path`. + /// + /// This function will traverse symbolic links to query information about the destination file. fn path_metadata(&self, path: &SystemPath) -> Result; /// Reads the content of the file at `path` into a [`String`].