refactor(builtins): migrate sort, uniq, tr, tar, rg to ArgParser (#898)

Replace manual `while i < args.len()` index management with ArgParser in 5 builtins: sort, uniq, tr (cuttr.rs), tar (archive.rs), and rg.\n\nAdd `bool_flags()` method to ArgParser for combined short boolean flags like `-rnuf`.\n\nCloses #880

Author: chaliy

crates/bashkit/src/builtins/archive.rs

@@ -91,57 +91,54 @@ impl Builtin for Tar {
         let mut files: Vec<String> = Vec::new();
 
         // Parse arguments
-        let mut i = 0;
-        while i < ctx.args.len() {
-            let arg = &ctx.args[i];
-            if arg.starts_with('-') && !arg.starts_with("--") {
-                // Track whether 'f' or 'C' consumed the next arg
-                let mut consumed_next = false;
-                for c in arg[1..].chars() {
+        let mut p = super::arg_parser::ArgParser::new(ctx.args);
+        while !p.is_done() {
+            if let Some(val) = p.flag_value_opt("-f") {
+                archive_file = Some(val.to_string());
+            } else if let Some(val) = p.flag_value_opt("-C") {
+                change_dir = Some(val.to_string());
+            } else if p.is_flag() {
+                // Combined short flags like -czf where f/C take the next arg
+                let arg = p.current().unwrap();
+                let chars: Vec<char> = arg[1..].chars().collect();
+                p.advance();
+                for c in &chars {
                     match c {
                         'c' => create = true,
                         'x' => extract = true,
                         't' => list = true,
                         'v' => verbose = true,
                         'z' => gzip = true,
                         'O' => to_stdout = true,
-                        'f' => {
-                            i += 1;
-                            consumed_next = true;
-                            if i >= ctx.args.len() {
+                        'f' => match p.positional() {
+                            Some(val) => archive_file = Some(val.to_string()),
+                            None => {
                                 return Ok(ExecResult::err(
                                     "tar: option requires an argument -- 'f'\n".to_string(),
                                     2,
                                 ));
                             }
-                            archive_file = Some(ctx.args[i].clone());
-                        }
-                        'C' => {
-                            i += 1;
-                            consumed_next = true;
-                            if i >= ctx.args.len() {
+                        },
+                        'C' => match p.positional() {
+                            Some(val) => change_dir = Some(val.to_string()),
+                            None => {
                                 return Ok(ExecResult::err(
                                     "tar: option requires an argument -- 'C'\n".to_string(),
                                     2,
                                 ));
                             }
-                            change_dir = Some(ctx.args[i].clone());
-                        }
+                        },
                         _ => {
                             return Ok(ExecResult::err(
                                 format!("tar: invalid option -- '{}'\n", c),
                                 2,
                             ));
                         }
                     }
-                    if consumed_next {
-                        break;
-                    }
                 }
-            } else {
-                files.push(arg.clone());
+            } else if let Some(arg) = p.positional() {
+                files.push(arg.to_string());
             }
-            i += 1;
         }
 
         // Check for exactly one of -c, -x, -t

crates/bashkit/src/builtins/arg_parser.rs

@@ -192,6 +192,26 @@ impl<'a> ArgParser<'a> {
             .map(|s| s.starts_with('-') && s.len() > 1)
             .unwrap_or(false)
     }
+
+    /// Try to consume combined boolean short flags (e.g., `-rnuf`).
+    ///
+    /// If the current arg starts with `-` (not `--`), has length > 1, and
+    /// every character after `-` is in `allowed`, advances and returns the
+    /// matched chars. Otherwise returns an empty vec without advancing.
+    pub fn bool_flags(&mut self, allowed: &str) -> Vec<char> {
+        if let Some(arg) = self.current()
+            && arg.starts_with('-')
+            && !arg.starts_with("--")
+            && arg.len() > 1
+        {
+            let chars: Vec<char> = arg[1..].chars().collect();
+            if chars.iter().all(|c| allowed.contains(*c)) {
+                self.advance();
+                return chars;
+            }
+        }
+        Vec::new()
+    }
 }
 
 #[cfg(test)]
@@ -331,6 +351,50 @@ mod tests {
         assert_eq!(p.rest().len(), 2);
     }
 
+    #[test]
+    fn test_bool_flags() {
+        let a = args(&["-rnuf", "file"]);
+        let mut p = ArgParser::new(&a);
+        let flags = p.bool_flags("rnufsz");
+        assert_eq!(flags, vec!['r', 'n', 'u', 'f']);
+        assert_eq!(p.current(), Some("file"));
+    }
+
+    #[test]
+    fn test_bool_flags_no_match_unknown_char() {
+        let a = args(&["-rxn", "file"]);
+        let mut p = ArgParser::new(&a);
+        let flags = p.bool_flags("rn"); // 'x' not allowed
+        assert!(flags.is_empty());
+        assert_eq!(p.current(), Some("-rxn")); // not advanced
+    }
+
+    #[test]
+    fn test_bool_flags_long_flag_ignored() {
+        let a = args(&["--verbose"]);
+        let mut p = ArgParser::new(&a);
+        let flags = p.bool_flags("verbose");
+        assert!(flags.is_empty());
+    }
+
+    #[test]
+    fn test_bool_flags_single_dash_ignored() {
+        let a = args(&["-"]);
+        let mut p = ArgParser::new(&a);
+        let flags = p.bool_flags("abc");
+        assert!(flags.is_empty());
+        assert_eq!(p.current(), Some("-")); // not advanced
+    }
+
+    #[test]
+    fn test_bool_flags_single_char() {
+        let a = args(&["-v"]);
+        let mut p = ArgParser::new(&a);
+        let flags = p.bool_flags("v");
+        assert_eq!(flags, vec!['v']);
+        assert!(p.is_done());
+    }
+
     #[test]
     fn test_is_flag() {
         let a = args(&["-v", "-", "file", "--long"]);

crates/bashkit/src/builtins/cuttr.rs

@@ -281,30 +281,29 @@ impl Builtin for Tr {
         let mut complement = false;
 
         // Parse flags (can be combined like -ds, -cd)
-        let mut non_flag_args: Vec<&String> = Vec::new();
-        for arg in ctx.args.iter() {
-            if arg.starts_with('-')
-                && arg.len() > 1
-                && arg.chars().skip(1).all(|ch| "dscC".contains(ch))
-            {
-                for ch in arg.chars().skip(1) {
+        let mut non_flag_args: Vec<String> = Vec::new();
+        let mut p = super::arg_parser::ArgParser::new(ctx.args);
+        while !p.is_done() {
+            let flags = p.bool_flags("dscC");
+            if !flags.is_empty() {
+                for ch in flags {
                     match ch {
                         'd' => delete = true,
                         's' => squeeze = true,
                         'c' | 'C' => complement = true,
                         _ => {}
                     }
                 }
-            } else {
-                non_flag_args.push(arg);
+            } else if let Some(arg) = p.positional() {
+                non_flag_args.push(arg.to_string());
             }
         }
 
         if non_flag_args.is_empty() {
             return Ok(ExecResult::err("tr: missing operand\n".to_string(), 1));
         }
 
-        let mut set1 = expand_char_set(non_flag_args[0]);
+        let mut set1 = expand_char_set(&non_flag_args[0]);
         if complement {
             // Complement: use all byte-range chars (0-255) NOT in set1.
             // Covers full Latin-1 range so binary data from /dev/urandom
@@ -321,7 +320,7 @@ impl Builtin for Tr {
         let result = if delete && squeeze {
             // -ds: delete SET1 chars, then squeeze SET2 chars
             let set2 = if non_flag_args.len() >= 2 {
-                expand_char_set(non_flag_args[1])
+                expand_char_set(&non_flag_args[1])
             } else {
                 set1.clone()
             };
@@ -343,7 +342,7 @@ impl Builtin for Tr {
                 ));
             }
 
-            let set2 = expand_char_set(non_flag_args[1]);
+            let set2 = expand_char_set(&non_flag_args[1]);
 
             let translated: String = stdin
                 .chars()

crates/bashkit/src/builtins/rg.rs

@@ -18,7 +18,7 @@
 use async_trait::async_trait;
 use regex::Regex;
 
-use super::search_common::{build_search_regex, collect_files_recursive, parse_numeric_flag_arg};
+use super::search_common::{build_search_regex, collect_files_recursive};
 use super::{Builtin, Context, read_text_file, resolve_path};
 use crate::error::{Error, Result};
 use crate::interpreter::ExecResult;
@@ -57,15 +57,38 @@ impl RgOptions {
         };
 
         let mut positional = Vec::new();
-        let mut i = 0;
-
-        while i < args.len() {
-            let arg = &args[i];
-            if arg.starts_with('-') && arg.len() > 1 && !arg.starts_with("--") {
+        let mut p = super::arg_parser::ArgParser::new(args);
+
+        while !p.is_done() {
+            if let Ok(Some(val)) = p.flag_value("-m", "rg") {
+                opts.max_count = Some(
+                    val.parse()
+                        .map_err(|_| Error::Execution(format!("rg: invalid -m value: {val}")))?,
+                );
+            } else if p.flag("--no-filename") {
+                opts.no_filename = true;
+            } else if p.flag("--no-line-number") {
+                opts.line_numbers = false;
+            } else if p.flag("--line-number") {
+                opts.line_numbers = true;
+            } else if p.flag("--color") {
+                // no-op (may have separate value arg like "never", skip it)
+            } else if p.current().is_some_and(|s| s.starts_with("--color=")) {
+                // --color=VALUE is a no-op
+                p.advance();
+            } else if p.is_flag() {
+                // Combined short flags like -inFw
+                // Safe: is_flag() guarantees current() is Some
+                let arg = p.current().expect("is_flag guarantees Some");
+                if arg.starts_with("--") {
+                    // Unknown long option, skip
+                    p.advance();
+                    continue;
+                }
                 let chars: Vec<char> = arg[1..].chars().collect();
-                let mut j = 0;
-                while j < chars.len() {
-                    match chars[j] {
+                p.advance();
+                for (j, &c) in chars.iter().enumerate() {
+                    match c {
                         'i' => opts.ignore_case = true,
                         'n' => opts.line_numbers = true,
                         'N' => opts.line_numbers = false,
@@ -75,29 +98,31 @@ impl RgOptions {
                         'w' => opts.word_boundary = true,
                         'F' => opts.fixed_strings = true,
                         'm' => {
-                            opts.max_count =
-                                Some(parse_numeric_flag_arg(&chars, j, &mut i, args, "rg", "-m")?);
+                            // Rest of this flag group or next arg is the value
+                            let rest: String = chars[j + 1..].iter().collect();
+                            let num_str = if !rest.is_empty() {
+                                rest
+                            } else {
+                                match p.positional() {
+                                    Some(v) => v.to_string(),
+                                    None => {
+                                        return Err(Error::Execution(
+                                            "rg: -m requires an argument".to_string(),
+                                        ));
+                                    }
+                                }
+                            };
+                            opts.max_count = Some(num_str.parse().map_err(|_| {
+                                Error::Execution(format!("rg: invalid -m value: {num_str}"))
+                            })?);
                             break;
                         }
                         _ => {} // ignore unknown
                     }
-                    j += 1;
                 }
-            } else if let Some(opt) = arg.strip_prefix("--") {
-                if opt == "no-filename" {
-                    opts.no_filename = true;
-                } else if opt == "color" || opt.starts_with("color=") {
-                    // no-op
-                } else if opt == "no-line-number" {
-                    opts.line_numbers = false;
-                } else if opt == "line-number" {
-                    opts.line_numbers = true;
-                }
-                // ignore other long options
-            } else {
-                positional.push(arg.clone());
+            } else if let Some(arg) = p.positional() {
+                positional.push(arg.to_string());
             }
-            i += 1;
         }
 
         if positional.is_empty() {