vercel · chris-olszewski · Dec 5, 2024 · Nov 14, 2024 · Nov 14, 2024 · Nov 15, 2024
@@ -1,4 +1,10 @@
-use std::{backtrace::Backtrace, env, fmt, fmt::Display, io, mem, process};
+use std::{
+    backtrace::Backtrace,
+    env,
+    ffi::OsString,
+    fmt::{self, Display},
+    io, mem, process,
+};
 
 use biome_deserialize_macros::Deserializable;
 use camino::{Utf8Path, Utf8PathBuf};
@@ -329,52 +335,9 @@ pub enum LinkTarget {
 }
 
 impl Args {
-    pub fn new() -> Self {
-        // We always pass --single-package in from the shim.
-        // We need to omit it, and then add it in for run.
-        let arg_separator_position = env::args_os().position(|input_token| input_token == "--");
-
-        let single_package_position =
-            env::args_os().position(|input_token| input_token == "--single-package");
-
-        let is_single_package = match (arg_separator_position, single_package_position) {
-            (_, None) => false,
-            (None, Some(_)) => true,
-            (Some(arg_separator_position), Some(single_package_position)) => {
-                single_package_position < arg_separator_position
-            }
-        };
-
-        // Clap supports arbitrary iterators as input.
-        // We can remove all instances of --single-package
-        let single_package_free = std::env::args_os()
-            .enumerate()
-            .filter(|(index, input_token)| {
-                arg_separator_position
-                    .is_some_and(|arg_separator_position| index > &arg_separator_position)
-                    || input_token != "--single-package"
-            })
-            .map(|(_, input_token)| input_token);
-
-        let mut clap_args = match Args::try_parse_from(single_package_free) {
-            Ok(mut args) => {
-                // And then only add them back in when we're in `run`.
-                // The value can appear in two places in the struct.
-                // We defensively attempt to set both.
-                if let Some(ref mut execution_args) = args.execution_args {
-                    execution_args.single_package = is_single_package
-                }
-
-                if let Some(Command::Run {
-                    run_args: _,
-                    ref mut execution_args,
-                }) = args.command
-                {
-                    execution_args.single_package = is_single_package;
-                }
-
-                args
-            }
+    pub fn new(os_args: Vec<OsString>) -> Self {
+        let clap_args = match Args::parse(os_args) {
+            Ok(args) => args,
             // Don't use error logger when displaying help text
             Err(e)
                 if matches!(
@@ -409,10 +372,6 @@ impl Args {
             process::exit(0);
         }
 
-        if env::var("TEST_RUN").is_ok() {
-            clap_args.test_run = true;
-        }
-
         if let Some(run_args) = clap_args.run_args() {
             if run_args.no_cache {
                 warn!(
@@ -437,6 +396,33 @@ impl Args {
         clap_args
     }
 
+    fn parse(os_args: Vec<OsString>) -> Result<Self, clap::Error> {
+        let (is_single_package, single_package_free) = Self::remove_single_package(os_args);
+        let mut args = Args::try_parse_from(single_package_free)?;
+        // And then only add them back in when we're in `run`.
+        // The value can appear in two places in the struct.
+        // We defensively attempt to set both.
+        if let Some(ref mut execution_args) = args.execution_args {
+            execution_args.single_package = is_single_package
+        }
+
+        if let Some(Command::Run {
+            run_args: _,
+            ref mut execution_args,
+        }) = args.command
+        {
+            execution_args.single_package = is_single_package;
+        }
+
+        if env::var("TEST_RUN").is_ok() {
+            args.test_run = true;
+        }
+
+        args.validate()?;
+
+        Ok(args)
+    }
+
     pub fn track(&self, tel: &GenericEventBuilder) {
         // track usage only
         track_usage!(tel, self.skip_infer, |val| val);
@@ -493,6 +479,69 @@ impl Args {
             self.execution_args.as_ref()
         }
     }
+
+    fn remove_single_package(args: Vec<OsString>) -> (bool, impl Iterator<Item = OsString>) {
+        // We always pass --single-package in from the shim.
+        // We need to omit it, and then add it in for run.
+        let arg_separator_position = args.iter().position(|input_token| input_token == "--");
+
+        let single_package_position = args
+            .iter()
+            .position(|input_token| input_token == "--single-package");
+
+        let is_single_package = match (arg_separator_position, single_package_position) {
+            (_, None) => false,
+            (None, Some(_)) => true,
+            (Some(arg_separator_position), Some(single_package_position)) => {
+                single_package_position < arg_separator_position
+            }
+        };
+
+        // Clap supports arbitrary iterators as input.
+        // We can remove all instances of --single-package
+        let single_package_free = args
+            .into_iter()
+            .enumerate()
+            .filter(move |(index, input_token)| {
+                arg_separator_position
+                    .is_some_and(|arg_separator_position| index > &arg_separator_position)
+                    || input_token != "--single-package"
+            })
+            .map(|(_, input_token)| input_token);
+
+        (is_single_package, single_package_free)
+    }
+
+    fn validate(&self) -> Result<(), clap::Error> {
+        if self.run_args.is_some()
+            && !matches!(
+                self.command,
+                None | Some(Command::Run { .. }) | Some(Command::Config)
+            )
+        {
+            let mut cmd = Self::command();
+            Err(cmd.error(
+                clap::error::ErrorKind::UnknownArgument,
+                "Cannot use run arguments outside of run command",
+            ))
+        } else if self.execution_args.is_some() && matches!(self.command, Some(Command::Watch(_))) {
+            let mut cmd = Self::command();
+            Err(cmd.error(
+                clap::error::ErrorKind::ArgumentConflict,
+                "Cannot use watch arguments before `watch` subcommand",
+            ))
+        } else if matches!(self.command, Some(Command::Run { .. }))
+            && (self.run_args.is_some() || self.execution_args.is_some())
+        {
+            let mut cmd = Self::command();
+            Err(cmd.error(
+                clap::error::ErrorKind::ArgumentConflict,
+                "Cannot use run arguments before `run` subcommand",
+            ))
+        } else {
+            Ok(())
+        }
+    }
 }
 
 /// Defines the subcommands for CLI
@@ -1071,7 +1120,7 @@ pub async fn run(
     color_config: ColorConfig,
 ) -> Result<i32, Error> {
     // TODO: remove mutability from this function
-    let mut cli_args = Args::new();
+    let mut cli_args = Args::new(env::args_os().collect());
     let version = get_version();
 
     // track telemetry handle to close at the end of the run
@@ -1462,10 +1511,11 @@ pub async fn run(
 
 #[cfg(test)]
 mod test {
-    use std::assert_matches::assert_matches;
+    use std::{assert_matches::assert_matches, ffi::OsString};
 
     use camino::Utf8PathBuf;
     use clap::Parser;
+    use insta::assert_snapshot;
     use itertools::Itertools;
     use pretty_assertions::assert_eq;
 
@@ -2725,4 +2775,126 @@ mod test {
         assert!(Args::try_parse_from(["turbo", "build", "--filter", "foo", "--affected"]).is_err(),);
         assert!(Args::try_parse_from(["turbo", "ls", "--filter", "foo", "--affected"]).is_err(),);
     }
+
+    struct SinglePackageTestCase {
+        args: &'static [&'static str],
+        expected_is_single: bool,
+        expected: &'static [&'static str],
+    }
+
+    impl SinglePackageTestCase {
+        pub const fn new<const N: usize>(args: &'static [&'static str; N]) -> Self {
+            let args = args.as_slice();
+            Self {
+                args,
+                expected_is_single: false,
+                expected: args,
+            }
+        }
+
+        pub const fn expected<const N: usize>(
+            mut self,
+            expected: &'static [&'static str; N],
+        ) -> Self {
+            self.expected_is_single = true;
+            self.expected = expected.as_slice();
+            self
+        }
+
+        pub fn os_args(&self) -> Vec<OsString> {
+            self.args.iter().map(|s| OsString::from(*s)).collect()
+        }
+
+        pub fn assert_actual(&self, actual: (bool, impl Iterator<Item = OsString>)) {
+            let (is_single, args) = actual;
+            assert_eq!(is_single, self.expected_is_single);
+            assert_eq!(
+                args.map(|s| s.to_str().unwrap().to_string())
+                    .collect::<Vec<_>>()
+                    .as_slice(),
+                self.expected
+            );
+        }
+    }
+
+    const NO_SINGLE_PKG: SinglePackageTestCase =
+        SinglePackageTestCase::new(&["turbo", "--version"]);
+    const SINGLE_PKG_AFTER_PASS: SinglePackageTestCase =
+        SinglePackageTestCase::new(&["turbo", "--", "--single-package"]);
+    const SINGLE_PKG: SinglePackageTestCase =
+        SinglePackageTestCase::new(&["turbo", "--single-package", "run", "build"])
+            .expected(&["turbo", "run", "build"]);
+    const SINGLE_PKG_BEFORE_AFTER: SinglePackageTestCase = SinglePackageTestCase::new(&[
+        "turbo",
+        "--single-package",
+        "run",
+        "build",
+        "--",
+        "--single-package",
+    ])
+    .expected(&["turbo", "run", "build", "--", "--single-package"]);
+
+    #[test_case::test_case(NO_SINGLE_PKG)]
+    #[test_case::test_case(SINGLE_PKG_AFTER_PASS)]
+    #[test_case::test_case(SINGLE_PKG)]
+    #[test_case::test_case(SINGLE_PKG_BEFORE_AFTER)]
+    fn test_single_package_removal(test: SinglePackageTestCase) {
+        let os_args = test.os_args();
+        let actual = Args::remove_single_package(os_args);
+        test.assert_actual(actual);
+    }
+
+    #[test]
+    fn test_set_single_package() {
+        let inferred_run = Args::parse(
+            ["turbo", "--single-package", "build"]
+                .iter()
+                .map(|s| OsString::from(*s))
+                .collect(),
+        )
+        .unwrap();
+        let explicit_run = Args::parse(
+            ["turbo", "run", "--single-package", "build"]
+                .iter()
+                .map(|s| OsString::from(*s))
+                .collect(),
+        )
+        .unwrap();
+        assert!(inferred_run
+            .execution_args
+            .as_ref()
+            .map_or(false, |e| e.single_package));
+        assert!(explicit_run
+            .command
+            .as_ref()
+            .and_then(|cmd| if let Command::Run { execution_args, .. } = cmd {
+                Some(execution_args.single_package)
+            } else {
+                None
+            })
+            .unwrap_or(false));
+    }
+
+    #[test_case::test_case(&["turbo", "watch", "build", "--no-daemon"]; "after watch")]
+    #[test_case::test_case(&["turbo", "--no-daemon", "watch", "build"]; "before watch")]
+    fn test_no_run_args_outside_of_run(args: &[&str]) {
+        let os_args = args.iter().map(|s| OsString::from(*s)).collect();
+        let err = Args::parse(os_args).unwrap_err();
+        assert_snapshot!(args.join("-").as_str(), err);
+    }
+
+    #[test_case::test_case(&["turbo", "--filter=foo", "run", "build"], false; "execution args")]
+    #[test_case::test_case(&["turbo", "--no-daemon", "run", "build"], false; "run args")]
+    #[test_case::test_case(&["turbo", "build", "run"], true; "task")]
+    #[test_case::test_case(&["turbo", "--filter=web", "watch", "build"], false; "execution before watch")]
+    fn test_no_run_args_before_run(args: &[&str], is_okay: bool) {
+        let os_args = args.iter().map(|s| OsString::from(*s)).collect();
+        let cli = Args::parse(os_args);
+        if is_okay {
+            cli.unwrap();
+        } else {
+            let err = cli.unwrap_err();
+            assert_snapshot!(args.join("-").as_str(), err);
+        }
+    }
 }
@@ -0,0 +1,9 @@
+---
+source: crates/turborepo-lib/src/cli/mod.rs
+expression: err
+---
+error: Cannot use run arguments before `run` subcommand
+
+Usage: turbo [OPTIONS] [COMMAND]
+
+For more information, try '--help'.
@@ -0,0 +1,9 @@
+---
+source: crates/turborepo-lib/src/cli/mod.rs
+expression: err
+---
+error: Cannot use watch arguments before `watch` subcommand
+
+Usage: turbo [OPTIONS] [COMMAND]
+
+For more information, try '--help'.
@@ -0,0 +1,9 @@
+---
+source: crates/turborepo-lib/src/cli/mod.rs
+expression: err
+---
+error: Cannot use run arguments before `run` subcommand
+
+Usage: turbo [OPTIONS] [COMMAND]
+
+For more information, try '--help'.
@@ -0,0 +1,9 @@
+---
+source: crates/turborepo-lib/src/cli/mod.rs
+expression: err
+---
+error: Cannot use run arguments outside of run command
+
+Usage: turbo [OPTIONS] [COMMAND]
+
+For more information, try '--help'.
@@ -0,0 +1,12 @@
+---
+source: crates/turborepo-lib/src/cli/mod.rs
+expression: err
+---
+error: unexpected argument '--no-daemon' found
+
+  tip: a similar argument exists: '--no-update-notifier'
+  tip: to pass '--no-daemon' as a value, use '-- --no-daemon'
+
+Usage: turbo watch --no-update-notifier <--cache-dir <CACHE_DIR>|--concurrency <CONCURRENCY>|--continue|--single-package|--framework-inference [<BOOL>]|--global-deps <GLOBAL_DEPS>|--env-mode [<ENV_MODE>]|--filter <FILTER>|--affected|--output-logs <OUTPUT_LOGS>|--log-order <LOG_ORDER>|--only|--pkg-inference-root <PKG_INFERENCE_ROOT>|--log-prefix <LOG_PREFIX>|TASKS|PASS_THROUGH_ARGS>
+
+For more information, try '--help'.