[ty] Split-off Command execution from System (#27608)

crates/ruff_db/src/system.rs

@@ -1,3 +1,4 @@
+pub use command::{Command, CommandExecutor};
 pub use memory_fs::MemoryFileSystem;
 
 #[cfg(all(feature = "testing", feature = "os"))]
@@ -22,6 +23,7 @@ pub use self::path::{
 };
 use crate::file_revision::FileRevision;
 
+mod command;
 mod memory_fs;
 #[cfg(feature = "os")]
 mod os;
@@ -101,18 +103,21 @@ pub trait System: Debug + Sync + Send {
     /// Find an executable binary's path by name.
     fn which(&self, binary_name: &str) -> WhichResult;
 
-    /// Runs a command in the given working directory, returning its output.
-    fn run_command(
-        &self,
-        program: &str,
-        args: &[&str],
-        current_directory: &SystemPath,
-    ) -> Result<Output> {
-        let _ = (program, args, current_directory);
-        Err(std::io::Error::new(
-            std::io::ErrorKind::Unsupported,
-            "running commands is not supported by this system",
-        ))
+    /// Runs `command` and captures its standard output and standard error.
+    fn run_command(&self, command: Command) -> Result<Output> {
+        let Some(executor) = self.command_executor() else {
+            return Err(std::io::Error::new(
+                std::io::ErrorKind::Unsupported,
+                "running commands is not supported by this system",
+            ));
+        };
+
+        executor.execute(command)
+    }
+
+    /// Returns the system's command executor, if it supports running commands.
+    fn command_executor(&self) -> Option<&dyn CommandExecutor> {
+        None
     }
 
     /// Reads the content of the file at `path` into a [`String`].

crates/ruff_db/src/system/command.rs

@@ -0,0 +1,68 @@
+use std::process::Output;
+
+use super::{Result, SystemPath, SystemPathBuf};
+
+/// An owned description of a command to execute with a [`CommandExecutor`].
+#[derive(Debug)]
+pub struct Command {
+    executable: String,
+    arguments: Vec<String>,
+    current_directory: Option<SystemPathBuf>,
+}
+
+impl Command {
+    /// Creates a command for the given executable.
+    pub fn new(executable: impl Into<String>) -> Self {
+        Self {
+            executable: executable.into(),
+            arguments: Vec::new(),
+            current_directory: None,
+        }
+    }
+
+    /// Adds an argument to the command.
+    pub fn arg(&mut self, argument: impl Into<String>) -> &mut Self {
+        self.arguments.push(argument.into());
+        self
+    }
+
+    /// Adds multiple arguments to the command.
+    pub fn args<I, S>(&mut self, arguments: I) -> &mut Self
+    where
+        I: IntoIterator<Item = S>,
+        S: Into<String>,
+    {
+        self.arguments.extend(arguments.into_iter().map(Into::into));
+        self
+    }
+
+    /// Sets the working directory for the command.
+    pub fn current_dir(&mut self, directory: impl AsRef<SystemPath>) -> &mut Self {
+        self.current_directory = Some(directory.as_ref().to_path_buf());
+        self
+    }
+
+    /// Returns the executable to invoke.
+    pub fn get_executable(&self) -> &str {
+        &self.executable
+    }
+
+    /// Returns the arguments passed to the executable.
+    pub fn get_args(&self) -> &[String] {
+        &self.arguments
+    }
+
+    /// Returns the command's working directory, if explicitly configured.
+    pub fn get_current_dir(&self) -> Option<&SystemPath> {
+        self.current_directory.as_deref()
+    }
+}
+
+/// Executes [`Command`]s.
+pub trait CommandExecutor: Send + Sync {
+    /// Runs a command and captures its standard output and standard error.
+    fn execute(&self, command: Command) -> Result<Output>;
+
+    /// Creates an owned executor that can be moved to another thread.
+    fn dyn_clone(&self) -> Box<dyn CommandExecutor>;
+}

crates/ruff_db/src/system/os.rs

@@ -9,13 +9,13 @@ use super::walk_directory::{
 };
 use crate::max_parallelism;
 use crate::system::{
-    DirectoryEntry, FileType, Metadata, Result, System, SystemPath, SystemPathBuf,
-    SystemVirtualPath, WhichError, WhichResult, WritableSystem,
+    Command, CommandExecutor, DirectoryEntry, FileType, Metadata, Result, System, SystemPath,
+    SystemPathBuf, SystemVirtualPath, WhichError, WhichResult, WritableSystem,
 };
 use filetime::FileTime;
 use ruff_notebook::{Notebook, NotebookError};
 use std::num::NonZeroUsize;
-use std::process::{Command, Output};
+use std::process::Output;
 use std::sync::Arc;
 use std::{any::Any, path::PathBuf};
 
@@ -130,16 +130,8 @@ impl System for OsSystem {
         }
     }
 
-    fn run_command(
-        &self,
-        program: &str,
-        args: &[&str],
-        current_directory: &SystemPath,
-    ) -> Result<Output> {
-        Command::new(program)
-            .args(args)
-            .current_dir(current_directory.as_std_path())
-            .output()
+    fn command_executor(&self) -> Option<&dyn CommandExecutor> {
+        Some(self)
     }
 
     fn current_directory(&self) -> &SystemPath {
@@ -236,6 +228,23 @@ impl System for OsSystem {
     }
 }
 
+impl CommandExecutor for OsSystem {
+    fn execute(&self, command: Command) -> Result<Output> {
+        let directory = command
+            .get_current_dir()
+            .unwrap_or_else(|| self.current_directory());
+
+        std::process::Command::new(command.get_executable())
+            .args(command.get_args())
+            .current_dir(directory.as_std_path())
+            .output()
+    }
+
+    fn dyn_clone(&self) -> Box<dyn CommandExecutor> {
+        Box::new(self.clone())
+    }
+}
+
 impl WritableSystem for OsSystem {
     fn create_new_file(&self, path: &SystemPath) -> Result<()> {
         std::fs::File::create_new(path).map(drop)

crates/ruff_db/src/system/test.rs

@@ -1,14 +1,13 @@
 use ruff_notebook::{Notebook, NotebookError};
 use rustc_hash::FxHashMap;
 use std::panic::RefUnwindSafe;
-use std::process::Output;
 use std::sync::{Arc, Mutex};
 
 use crate::Db;
 use crate::files::File;
 use crate::system::{
-    DirectoryEntry, MemoryFileSystem, Metadata, Result, System, SystemPath, SystemPathBuf,
-    SystemVirtualPath, WhichError, WhichResult,
+    CommandExecutor, DirectoryEntry, MemoryFileSystem, Metadata, Result, System, SystemPath,
+    SystemPathBuf, SystemVirtualPath, WhichError, WhichResult,
 };
 
 use super::WritableSystem;
@@ -141,13 +140,8 @@ impl System for TestSystem {
         Err(WhichError::CannotFindBinaryPath)
     }
 
-    fn run_command(
-        &self,
-        program: &str,
-        args: &[&str],
-        current_directory: &SystemPath,
-    ) -> Result<Output> {
-        self.system().run_command(program, args, current_directory)
+    fn command_executor(&self) -> Option<&dyn CommandExecutor> {
+        self.system().command_executor()
     }
 
     fn read_directory<'a>(

crates/ty_project/src/metadata/uv.rs

@@ -1,7 +1,7 @@
 use std::path::PathBuf;
 
 use pep440_rs::Version;
-use ruff_db::system::{System, SystemPath, SystemPathBuf, WhichError};
+use ruff_db::system::{Command, System, SystemPath, SystemPathBuf, WhichError};
 use ruff_ranged_value::{RangedValue, ValueSource};
 use serde::Deserialize;
 use thiserror::Error;
@@ -32,12 +32,12 @@ impl UvWorkspace {
 
         // `uv check` has already selected and synchronized the environment. Keep this query
         // read-only so package selection and `--isolated` aren't overwritten by a second sync.
+        let mut command = Command::new(uv);
+        command
+            .args(["workspace", "metadata", "--frozen", "--active"])
+            .current_dir(path);
         let output = system
-            .run_command(
-                &uv,
-                &["workspace", "metadata", "--frozen", "--active"],
-                path,
-            )
+            .run_command(command)
             .map_err(UvWorkspaceError::Invocation)?;
 
         if !output.status.success() {

crates/ty_server/src/session.rs

@@ -1992,3 +1992,29 @@ pub(super) fn warn_about_unknown_options(
     tracing::warn!("{message}");
     client.show_warning_message(message);
 }
+
+#[cfg(test)]
+mod tests {
+    use std::sync::Arc;
+
+    use ruff_db::system::{CommandExecutor, OsSystem, System as _};
+
+    use super::Index;
+    use crate::system::LSPSystem;
+
+    /// Mutating the document index requires exclusive ownership after Salsa cancels the current
+    /// database snapshots. A background command executor must not retain an `LSPSystem`, because
+    /// that would keep the index alive and prevent the server from applying document changes.
+    #[test]
+    fn detached_command_executor_does_not_retain_document_index() {
+        let index = Arc::new(Index::new());
+        let system = LSPSystem::new(index.clone(), Arc::new(OsSystem::default()));
+        let executor = system.command_executor().map(CommandExecutor::dyn_clone);
+        assert!(executor.is_some());
+        drop(system);
+
+        assert_eq!(Arc::strong_count(&index), 1);
+
+        drop(executor);
+    }
+}

crates/ty_server/src/system.rs

@@ -3,7 +3,6 @@ use std::fmt;
 use std::fmt::Display;
 use std::hash::{DefaultHasher, Hash, Hasher as _};
 use std::panic::RefUnwindSafe;
-use std::process::Output;
 use std::sync::Arc;
 
 use crate::Db;
@@ -14,7 +13,7 @@ use ruff_db::file_revision::FileRevision;
 use ruff_db::files::{File, FilePath};
 use ruff_db::system::walk_directory::WalkDirectoryBuilder;
 use ruff_db::system::{
-    DirectoryEntry, FileType, Metadata, Result, System, SystemPath, SystemPathBuf,
+    CommandExecutor, DirectoryEntry, FileType, Metadata, Result, System, SystemPath, SystemPathBuf,
     SystemVirtualPath, SystemVirtualPathBuf, WhichResult, WritableSystem,
 };
 use ruff_notebook::{Notebook, NotebookError};
@@ -182,16 +181,6 @@ impl System for LSPSystem {
         self.native_system.is_same_file(first, second)
     }
 
-    fn run_command(
-        &self,
-        program: &str,
-        args: &[&str],
-        current_directory: &SystemPath,
-    ) -> Result<Output> {
-        self.native_system
-            .run_command(program, args, current_directory)
-    }
-
     fn source_type(&self, path: &SystemPath) -> Option<PySourceType> {
         let document = self.system_path_to_document(path)?;
         Self::source_type_from_document(document, path.extension())
@@ -290,6 +279,10 @@ impl System for LSPSystem {
         self.native_system.env_var(name)
     }
 
+    fn command_executor(&self) -> Option<&dyn CommandExecutor> {
+        self.native_system.command_executor()
+    }
+
     fn dyn_clone(&self) -> Box<dyn System> {
         Box::new(self.clone())
     }