From b93dd92a5c54ce1379876d3d31e0d7aeeed9fd54 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 21:59:29 +0000 Subject: [PATCH 1/5] shell: name the rejected flag in the builtins' "illegal option" errors cat, touch and mkdir sliced the short-flag cluster at 1 + i, one past the byte they rejected, so `mkdir -z` printed "illegal option -- " and `mkdir -zp` printed "illegal option -- p". ls always named the first flag of the cluster, mv always printed "-", and cp and rm printed the rest of the cluster. Every builtin now names the single byte it rejected, as getopt(3) does. An unknown --long option is rejected at its second dash and reported as "-", which rm, ls and mv already did. --- src/runtime/shell/builtin/cat.rs | 4 +--- src/runtime/shell/builtin/cp.rs | 2 +- src/runtime/shell/builtin/ls.rs | 2 +- src/runtime/shell/builtin/mkdir.rs | 4 +--- src/runtime/shell/builtin/mv.rs | 14 ++++++------ src/runtime/shell/builtin/rm.rs | 15 ++++++------- src/runtime/shell/builtin/touch.rs | 4 +--- src/runtime/shell/interpreter.rs | 5 +++++ test/js/bun/shell/commands/ls.test.ts | 10 ++------- test/js/bun/shell/exec.test.ts | 31 +++++++++++++++++++++++++-- 10 files changed, 56 insertions(+), 35 deletions(-) diff --git a/src/runtime/shell/builtin/cat.rs b/src/runtime/shell/builtin/cat.rs index bd510becc71d..12b08dcd77c1 100644 --- a/src/runtime/shell/builtin/cat.rs +++ b/src/runtime/shell/builtin/cat.rs @@ -419,9 +419,7 @@ impl FlagParser for Opts { b't' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-t"))), b'u' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-u"))), b'v' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-v"))), - _ => Some(ParseFlagResult::IllegalOption( - &raw const smallflags[1 + i..], - )), + _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..=i])), } } } diff --git a/src/runtime/shell/builtin/cp.rs b/src/runtime/shell/builtin/cp.rs index 8a7a39d19a51..c321291a5d1d 100644 --- a/src/runtime/shell/builtin/cp.rs +++ b/src/runtime/shell/builtin/cp.rs @@ -809,7 +809,7 @@ impl FlagParser for Opts { Some(ParseFlagResult::ContinueParsing) } b'n' => Some(ParseFlagResult::ContinueParsing), - _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..])), + _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..=i])), } } } diff --git a/src/runtime/shell/builtin/ls.rs b/src/runtime/shell/builtin/ls.rs index 8f6213a5dcad..293f46088d80 100644 --- a/src/runtime/shell/builtin/ls.rs +++ b/src/runtime/shell/builtin/ls.rs @@ -259,7 +259,7 @@ impl Ls { | b'h' | b'H' | b'i' | b'I' | b'k' | b'L' | b'm' | b'n' | b'N' | b'o' | b'p' | b'q' | b'Q' | b's' | b'S' | b't' | b'T' | b'u' | b'U' | b'v' | b'w' | b'x' | b'X' | b'Z' => {} - _ => return ParseFlag::IllegalOption(Box::from(&flag[1..2])), + _ => return ParseFlag::IllegalOption(Box::from([ch])), } } ParseFlag::ContinueParsing diff --git a/src/runtime/shell/builtin/mkdir.rs b/src/runtime/shell/builtin/mkdir.rs index a4695d61c6b2..ed8d60ff7ab0 100644 --- a/src/runtime/shell/builtin/mkdir.rs +++ b/src/runtime/shell/builtin/mkdir.rs @@ -454,9 +454,7 @@ impl FlagParser for Opts { self.verbose = true; None } - _ => Some(ParseFlagResult::IllegalOption( - &raw const smallflags[1 + i..], - )), + _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..=i])), } } } diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index 915d34c3bfaf..a4decf0d7b32 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -46,7 +46,8 @@ pub enum MvState { /// mv uses its own simpler parser. enum MvParseError { - IllegalOption(&'static [u8]), + /// The rejected option byte. + IllegalOption(u8), ShowUsage, } @@ -94,11 +95,11 @@ impl Mv { Tag::Idle => { if let Err(e) = Self::parse_opts(interp, cmd) { let buf: Vec = match e { - MvParseError::IllegalOption(s) => Builtin::fmt_error_arena( + MvParseError::IllegalOption(ch) => Builtin::fmt_error_arena( interp, cmd, Some(Kind::Mv), - format_args!("illegal option -- {}\n", bstr::BStr::new(s)), + format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), ) .to_vec(), MvParseError::ShowUsage => Kind::Mv.usage_string().to_vec(), @@ -360,7 +361,7 @@ impl Mv { return Ok(()); } MvFlag::ContinueParsing => {} - MvFlag::IllegalOption(s) => return Err(MvParseError::IllegalOption(s)), + MvFlag::IllegalOption(ch) => return Err(MvParseError::IllegalOption(ch)), } idx += 1; } @@ -374,7 +375,7 @@ impl Mv { for &ch in &flag[1..] { match ch { b'f' | b'h' | b'i' | b'n' | b'v' => {} - _ => return MvFlag::IllegalOption(b"-"), + _ => return MvFlag::IllegalOption(ch), } } MvFlag::ContinueParsing @@ -396,7 +397,8 @@ impl Drop for Mv { enum MvFlag { ContinueParsing, Done, - IllegalOption(&'static [u8]), + /// The rejected option byte. + IllegalOption(u8), } /// `openat(target, O_RDONLY|O_DIRECTORY)` diff --git a/src/runtime/shell/builtin/rm.rs b/src/runtime/shell/builtin/rm.rs index 8c4cde84e18d..b1306bdb50f4 100644 --- a/src/runtime/shell/builtin/rm.rs +++ b/src/runtime/shell/builtin/rm.rs @@ -92,8 +92,10 @@ pub enum PromptBehaviour { enum RmParseFlag { ContinueParsing, Done, + /// Unknown `--long` option. IllegalOption, - IllegalOptionWithFlag, + /// Unknown short option: the rejected byte. + IllegalOptionWithFlag(u8), } impl Rm { @@ -248,7 +250,7 @@ impl Rm { b"rm: illegal option -- -\n", ); } - RmParseFlag::IllegalOptionWithFlag => { + RmParseFlag::IllegalOptionWithFlag(ch) => { if let Some(safeguard) = Builtin::of(interp, cmd).stderr.needs_io() { Self::state_mut(interp, cmd).state = RmState::ParseOpts { idx, @@ -258,10 +260,7 @@ impl Rm { return Builtin::of_mut(interp, cmd).stderr.enqueue_fmt( child, Some(Kind::Rm), - format_args!( - "illegal option -- {}\n", - bstr::BStr::new(&arg[1..]) - ), + format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), safeguard, ); } @@ -269,7 +268,7 @@ impl Rm { interp, cmd, Some(Kind::Rm), - format_args!("illegal option -- {}\n", bstr::BStr::new(&arg[1..])), + format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), ) .to_vec(); let _ = Builtin::write_no_io(interp, cmd, IoKind::Stderr, &buf); @@ -532,7 +531,7 @@ impl Rm { b'd' => opts.remove_empty_dirs = true, b'i' => opts.prompt_behaviour = PromptBehaviour::Once { removed_count: 0 }, b'I' => opts.prompt_behaviour = PromptBehaviour::Always, - _ => return RmParseFlag::IllegalOptionWithFlag, + _ => return RmParseFlag::IllegalOptionWithFlag(ch), } } RmParseFlag::ContinueParsing diff --git a/src/runtime/shell/builtin/touch.rs b/src/runtime/shell/builtin/touch.rs index 95ae1398deee..87ac1f57549a 100644 --- a/src/runtime/shell/builtin/touch.rs +++ b/src/runtime/shell/builtin/touch.rs @@ -373,9 +373,7 @@ impl FlagParser for Opts { b'm' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-m"))), b'r' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-r"))), b't' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-t"))), - _ => Some(ParseFlagResult::IllegalOption( - &raw const smallflags[1 + i..], - )), + _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..=i])), } } } diff --git a/src/runtime/shell/interpreter.rs b/src/runtime/shell/interpreter.rs index f0e6da780e48..15077619e75d 100644 --- a/src/runtime/shell/interpreter.rs +++ b/src/runtime/shell/interpreter.rs @@ -2348,6 +2348,11 @@ pub trait FlagParser { /// Handle a `--long` flag. Return `None` to fall through to short parsing. fn parse_long(&mut self, flag: &[u8]) -> Option; /// Handle one byte of a `-abc` cluster. Return `None` to keep iterating. + /// + /// `smallflags` is the argument minus its leading `-` and `ch == smallflags[i]`. + /// An unknown byte is reported as `IllegalOption(&raw const smallflags[i..=i])`: + /// the message names that one byte, as getopt(3) does, and the slice borrows + /// argv, which outlives the error (see [`ParseError::opt`]). fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option; } diff --git a/test/js/bun/shell/commands/ls.test.ts b/test/js/bun/shell/commands/ls.test.ts index 3c7ea5176cc3..07d11d4dae7e 100644 --- a/test/js/bun/shell/commands/ls.test.ts +++ b/test/js/bun/shell/commands/ls.test.ts @@ -288,17 +288,11 @@ describe.concurrent("bunshell ls", () => { }); test("invalid flag", async () => { - await TestBuilder.command`ls -z` - .exitCode(1) - .stderr(s => expect(s).toContain("illegal option")) - .run(); + await TestBuilder.command`ls -z`.exitCode(1).stderr("ls: illegal option -- z\n").run(); }); test("invalid combined flags", async () => { - await TestBuilder.command`ls -az` - .exitCode(1) - .stderr(s => expect(s).toContain("illegal option")) - .run(); + await TestBuilder.command`ls -az`.exitCode(1).stderr("ls: illegal option -- z\n").run(); }); test.if(isPosix)("permission denied directory", async () => { diff --git a/test/js/bun/shell/exec.test.ts b/test/js/bun/shell/exec.test.ts index b7c272197124..40b647dcdc9e 100644 --- a/test/js/bun/shell/exec.test.ts +++ b/test/js/bun/shell/exec.test.ts @@ -46,8 +46,8 @@ describe("bun exec", () => { // prettier-ignore const programs = [ // ["cat", 1, "", ""], - ["touch", 1, "touch: illegal option -- help\n", ""], - ["mkdir", 1, "mkdir: illegal option -- help\n", ""], + ["touch", 1, "touch: illegal option -- -\n", ""], + ["mkdir", 1, "mkdir: illegal option -- -\n", ""], // ["cd", 1, "cd: no such file or directory: --help\n", ""], ["echo", 0, "", "--help\n"], ["pwd", 1, "pwd: too many arguments\n", ""], @@ -71,6 +71,33 @@ describe("bun exec", () => { } }); + // Like getopt(3), the message names the one byte that was rejected, wherever + // it sits in a cluster of short flags. An unknown --long option is rejected + // at its second `-`, so it is reported as `-` (what BSD getopt prints too). + describe("illegal option names the rejected flag", () => { + // prettier-ignore + const programs: [program: string, cases: [args: string, rejected: string][]][] = [ + ["cat", [["-z", "z"], ["-zb", "z"], ["--bogus", "-"]]], + ["touch", [["-z", "z"], ["-za", "z"], ["--bogus", "-"]]], + ["mkdir", [["-z", "z"], ["-pz", "z"], ["-zp", "z"], ["--bogus", "-"]]], + ["cp", [["-z", "z"], ["-zR", "z"], ["--bogus", "-"]]], + ["ls", [["-z", "z"], ["-az", "z"], ["-za", "z"], ["--bogus", "-"]]], + ["rm", [["-z", "z"], ["-rz", "z"], ["-zr", "z"], ["--bogus", "-"]]], + ["mv", [["-z", "z"], ["-fz", "z"], ["-zf", "z"], ["--bogus", "-"]]], + ]; + for (const [program, cases] of programs) { + const script = cases.map(([args]) => `${program} ${args}`).join("; "); + const stderr = cases.map(([, rejected]) => `${program}: illegal option -- ${rejected}\n`).join(""); + TestBuilder.command`${BUN} exec ${script}` + // cat and cp are builtins only on Windows unless this flag is set. + .env({ ...bunEnv, BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS: "1" }) + .exitCode(1) + .stderr(stderr) + .stdout("") + .runAsTest(program); + } + }); + TestBuilder.command`${BUN} exec cd` .env(bunEnv) .exitCode(0) From 950982c06322f06fb133a976574c5799bdfbd429 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 22:08:24 +0000 Subject: [PATCH 2/5] test: run each illegal option case as its own bun exec so every exit code is asserted --- test/js/bun/shell/exec.test.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/js/bun/shell/exec.test.ts b/test/js/bun/shell/exec.test.ts index 40b647dcdc9e..2d40747e7acd 100644 --- a/test/js/bun/shell/exec.test.ts +++ b/test/js/bun/shell/exec.test.ts @@ -74,7 +74,7 @@ describe("bun exec", () => { // Like getopt(3), the message names the one byte that was rejected, wherever // it sits in a cluster of short flags. An unknown --long option is rejected // at its second `-`, so it is reported as `-` (what BSD getopt prints too). - describe("illegal option names the rejected flag", () => { + describe.concurrent("illegal option names the rejected flag", () => { // prettier-ignore const programs: [program: string, cases: [args: string, rejected: string][]][] = [ ["cat", [["-z", "z"], ["-zb", "z"], ["--bogus", "-"]]], @@ -86,15 +86,15 @@ describe("bun exec", () => { ["mv", [["-z", "z"], ["-fz", "z"], ["-zf", "z"], ["--bogus", "-"]]], ]; for (const [program, cases] of programs) { - const script = cases.map(([args]) => `${program} ${args}`).join("; "); - const stderr = cases.map(([, rejected]) => `${program}: illegal option -- ${rejected}\n`).join(""); - TestBuilder.command`${BUN} exec ${script}` - // cat and cp are builtins only on Windows unless this flag is set. - .env({ ...bunEnv, BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS: "1" }) - .exitCode(1) - .stderr(stderr) - .stdout("") - .runAsTest(program); + for (const [args, rejected] of cases) { + TestBuilder.command`${BUN} exec ${`${program} ${args}`}` + // cat and cp are builtins only on Windows unless this flag is set. + .env({ ...bunEnv, BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS: "1" }) + .exitCode(1) + .stderr(`${program}: illegal option -- ${rejected}\n`) + .stdout("") + .runAsTest(`${program} ${args}`); + } } }); From ac1632dee6a2d61f22f153ea81beb663a1d437ea Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 22:12:25 +0000 Subject: [PATCH 3/5] shell: build the illegal option payload in one place --- src/runtime/shell/builtin/cat.rs | 5 +++-- src/runtime/shell/builtin/cp.rs | 4 ++-- src/runtime/shell/builtin/mkdir.rs | 4 ++-- src/runtime/shell/builtin/touch.rs | 4 ++-- src/runtime/shell/interpreter.rs | 15 +++++++++------ 5 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/runtime/shell/builtin/cat.rs b/src/runtime/shell/builtin/cat.rs index 12b08dcd77c1..f9df69275a7c 100644 --- a/src/runtime/shell/builtin/cat.rs +++ b/src/runtime/shell/builtin/cat.rs @@ -3,7 +3,8 @@ use std::sync::Arc; use crate::shell::ExitCode; use crate::shell::builtin::{Builtin, BuiltinIO, BuiltinInput, BuiltinState, IoKind, Kind}; use crate::shell::interpreter::{ - FlagParser, Interpreter, NodeId, ParseFlagResult, parse_flags, shell_openat, unsupported_flag, + FlagParser, Interpreter, NodeId, ParseFlagResult, illegal_flag, parse_flags, shell_openat, + unsupported_flag, }; use crate::shell::io_reader::{ChildPtr as ReaderChildPtr, IOReader, ReaderTag}; use crate::shell::io_writer::{ChildPtr, WriterTag}; @@ -419,7 +420,7 @@ impl FlagParser for Opts { b't' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-t"))), b'u' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-u"))), b'v' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-v"))), - _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..=i])), + _ => Some(ParseFlagResult::IllegalOption(illegal_flag(smallflags, i))), } } } diff --git a/src/runtime/shell/builtin/cp.rs b/src/runtime/shell/builtin/cp.rs index c321291a5d1d..4cfd2c64ffdb 100644 --- a/src/runtime/shell/builtin/cp.rs +++ b/src/runtime/shell/builtin/cp.rs @@ -3,7 +3,7 @@ use bun_paths::resolve_path; use crate::shell::builtin::{Builtin, BuiltinState, IoKind, Kind}; use crate::shell::interpreter::{ EventLoopHandle, FlagParser, Interpreter, NodeId, OutputSrc, OutputTask, OutputTaskVTable, - ParseFlagResult, ShellTask, parse_flags, unsupported_flag, + ParseFlagResult, ShellTask, illegal_flag, parse_flags, unsupported_flag, }; use crate::shell::io_writer::{ChildPtr, WriterTag}; use crate::shell::yield_::Yield; @@ -809,7 +809,7 @@ impl FlagParser for Opts { Some(ParseFlagResult::ContinueParsing) } b'n' => Some(ParseFlagResult::ContinueParsing), - _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..=i])), + _ => Some(ParseFlagResult::IllegalOption(illegal_flag(smallflags, i))), } } } diff --git a/src/runtime/shell/builtin/mkdir.rs b/src/runtime/shell/builtin/mkdir.rs index ed8d60ff7ab0..79d30b828775 100644 --- a/src/runtime/shell/builtin/mkdir.rs +++ b/src/runtime/shell/builtin/mkdir.rs @@ -4,7 +4,7 @@ use crate::shell::ExitCode; use crate::shell::builtin::{Builtin, BuiltinState, IoKind, Kind}; use crate::shell::interpreter::{ EventLoopHandle, FlagParser, Interpreter, NodeId, OutputSrc, OutputTask, OutputTaskVTable, - ParseFlagResult, ShellTask, parse_flags, unsupported_flag, + ParseFlagResult, ShellTask, illegal_flag, parse_flags, unsupported_flag, }; use crate::shell::io_writer::{ChildPtr, WriterTag}; use crate::shell::yield_::Yield; @@ -454,7 +454,7 @@ impl FlagParser for Opts { self.verbose = true; None } - _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..=i])), + _ => Some(ParseFlagResult::IllegalOption(illegal_flag(smallflags, i))), } } } diff --git a/src/runtime/shell/builtin/touch.rs b/src/runtime/shell/builtin/touch.rs index 87ac1f57549a..9f1a4283880c 100644 --- a/src/runtime/shell/builtin/touch.rs +++ b/src/runtime/shell/builtin/touch.rs @@ -2,7 +2,7 @@ use crate::shell::ExitCode; use crate::shell::builtin::{Builtin, BuiltinState, IoKind, Kind}; use crate::shell::interpreter::{ EventLoopHandle, FlagParser, Interpreter, NodeId, OutputSrc, OutputTask, OutputTaskVTable, - ParseFlagResult, ShellTask, parse_flags, unsupported_flag, + ParseFlagResult, ShellTask, illegal_flag, parse_flags, unsupported_flag, }; use crate::shell::io_writer::{ChildPtr, WriterTag}; use crate::shell::yield_::Yield; @@ -373,7 +373,7 @@ impl FlagParser for Opts { b'm' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-m"))), b'r' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-r"))), b't' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-t"))), - _ => Some(ParseFlagResult::IllegalOption(&raw const smallflags[i..=i])), + _ => Some(ParseFlagResult::IllegalOption(illegal_flag(smallflags, i))), } } } diff --git a/src/runtime/shell/interpreter.rs b/src/runtime/shell/interpreter.rs index 15077619e75d..66b1ef1050cf 100644 --- a/src/runtime/shell/interpreter.rs +++ b/src/runtime/shell/interpreter.rs @@ -2343,16 +2343,19 @@ pub(crate) const fn unsupported_flag(name: &'static [u8]) -> *const [u8] { std::ptr::from_ref::<[u8]>(name) } +/// `IllegalOption` payload for the byte `FlagParser::parse_short` is rejecting: +/// that one byte (all getopt(3) names either), borrowed from argv. +#[inline] +pub(crate) fn illegal_flag(smallflags: &[u8], i: usize) -> *const [u8] { + &raw const smallflags[i..=i] +} + /// Per-builtin opts type implements this to plug into `FlagParser::parse_flags`. pub trait FlagParser { /// Handle a `--long` flag. Return `None` to fall through to short parsing. fn parse_long(&mut self, flag: &[u8]) -> Option; - /// Handle one byte of a `-abc` cluster. Return `None` to keep iterating. - /// - /// `smallflags` is the argument minus its leading `-` and `ch == smallflags[i]`. - /// An unknown byte is reported as `IllegalOption(&raw const smallflags[i..=i])`: - /// the message names that one byte, as getopt(3) does, and the slice borrows - /// argv, which outlives the error (see [`ParseError::opt`]). + /// Handle one byte of a `-abc` cluster (`ch == smallflags[i]`). Return `None` + /// to keep iterating; reject an unknown byte with [`illegal_flag`]. fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option; } From 7229364570c8cf5455de2a548c4490cfd643d2bd Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 22:15:21 +0000 Subject: [PATCH 4/5] shell: shorten the flag parser docs --- src/runtime/shell/interpreter.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/runtime/shell/interpreter.rs b/src/runtime/shell/interpreter.rs index 66b1ef1050cf..625443a5260d 100644 --- a/src/runtime/shell/interpreter.rs +++ b/src/runtime/shell/interpreter.rs @@ -2343,8 +2343,7 @@ pub(crate) const fn unsupported_flag(name: &'static [u8]) -> *const [u8] { std::ptr::from_ref::<[u8]>(name) } -/// `IllegalOption` payload for the byte `FlagParser::parse_short` is rejecting: -/// that one byte (all getopt(3) names either), borrowed from argv. +/// `IllegalOption` payload for `parse_short`: just the rejected byte, as getopt(3) reports it. #[inline] pub(crate) fn illegal_flag(smallflags: &[u8], i: usize) -> *const [u8] { &raw const smallflags[i..=i] @@ -2354,8 +2353,7 @@ pub(crate) fn illegal_flag(smallflags: &[u8], i: usize) -> *const [u8] { pub trait FlagParser { /// Handle a `--long` flag. Return `None` to fall through to short parsing. fn parse_long(&mut self, flag: &[u8]) -> Option; - /// Handle one byte of a `-abc` cluster (`ch == smallflags[i]`). Return `None` - /// to keep iterating; reject an unknown byte with [`illegal_flag`]. + /// Handle one byte of a `-abc` cluster: `None` keeps iterating, [`illegal_flag`] rejects it. fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option; } From dbb0c108bc92dd0153181c174276f44ff90d2710 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 16 Aug 2026 02:45:17 +0000 Subject: [PATCH 5/5] shell: carry the rejected option byte as a u8 through the flag parsers parse_short only ever used smallflags and i to build the IllegalOption payload, and three of the four impls sliced it wrong. The shared loop now hands each impl just the byte, and IllegalOption carries that byte, so the raw-pointer payload, ParseError::opt and its unsafe go away. ls and rm carry the byte the same way; rm's two illegal-option variants and its second format site collapse into one. --- src/runtime/shell/Builtin.rs | 10 +++--- src/runtime/shell/builtin/cat.rs | 7 ++-- src/runtime/shell/builtin/cp.rs | 6 ++-- src/runtime/shell/builtin/ls.rs | 15 ++++---- src/runtime/shell/builtin/mkdir.rs | 6 ++-- src/runtime/shell/builtin/rm.rs | 36 ++++--------------- src/runtime/shell/builtin/touch.rs | 6 ++-- src/runtime/shell/interpreter.rs | 51 +++++++-------------------- test/js/bun/shell/commands/rm.test.ts | 7 ++++ test/js/bun/shell/exec.test.ts | 16 +++++++++ 10 files changed, 67 insertions(+), 93 deletions(-) diff --git a/src/runtime/shell/Builtin.rs b/src/runtime/shell/Builtin.rs index 6ce494deb4ed..6927194ab6a3 100644 --- a/src/runtime/shell/Builtin.rs +++ b/src/runtime/shell/Builtin.rs @@ -1077,22 +1077,22 @@ impl Builtin { e: &ParseError, set_wait_err: impl FnOnce(), ) -> Yield { - let buf: Vec = match e { - ParseError::IllegalOption(_) => Self::fmt_error_arena( + let buf: Vec = match *e { + ParseError::IllegalOption(ch) => Self::fmt_error_arena( interp, cmd, Some(kind), - format_args!("illegal option -- {}\n", bstr::BStr::new(e.opt())), + format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), ) .to_vec(), ParseError::ShowUsage => kind.usage_string().to_vec(), - ParseError::Unsupported(_) => Self::fmt_error_arena( + ParseError::Unsupported(name) => Self::fmt_error_arena( interp, cmd, Some(kind), format_args!( "unsupported option, please open a GitHub issue -- {}\n", - bstr::BStr::new(e.opt()) + bstr::BStr::new(name) ), ) .to_vec(), diff --git a/src/runtime/shell/builtin/cat.rs b/src/runtime/shell/builtin/cat.rs index f9df69275a7c..c242a918e6e2 100644 --- a/src/runtime/shell/builtin/cat.rs +++ b/src/runtime/shell/builtin/cat.rs @@ -3,8 +3,7 @@ use std::sync::Arc; use crate::shell::ExitCode; use crate::shell::builtin::{Builtin, BuiltinIO, BuiltinInput, BuiltinState, IoKind, Kind}; use crate::shell::interpreter::{ - FlagParser, Interpreter, NodeId, ParseFlagResult, illegal_flag, parse_flags, shell_openat, - unsupported_flag, + FlagParser, Interpreter, NodeId, ParseFlagResult, parse_flags, shell_openat, unsupported_flag, }; use crate::shell::io_reader::{ChildPtr as ReaderChildPtr, IOReader, ReaderTag}; use crate::shell::io_writer::{ChildPtr, WriterTag}; @@ -411,7 +410,7 @@ impl FlagParser for Opts { None } - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option { + fn parse_short(&mut self, ch: u8) -> Option { match ch { b'b' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-b"))), b'e' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-e"))), @@ -420,7 +419,7 @@ impl FlagParser for Opts { b't' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-t"))), b'u' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-u"))), b'v' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-v"))), - _ => Some(ParseFlagResult::IllegalOption(illegal_flag(smallflags, i))), + _ => Some(ParseFlagResult::IllegalOption(ch)), } } } diff --git a/src/runtime/shell/builtin/cp.rs b/src/runtime/shell/builtin/cp.rs index 4cfd2c64ffdb..475af8b22ced 100644 --- a/src/runtime/shell/builtin/cp.rs +++ b/src/runtime/shell/builtin/cp.rs @@ -3,7 +3,7 @@ use bun_paths::resolve_path; use crate::shell::builtin::{Builtin, BuiltinState, IoKind, Kind}; use crate::shell::interpreter::{ EventLoopHandle, FlagParser, Interpreter, NodeId, OutputSrc, OutputTask, OutputTaskVTable, - ParseFlagResult, ShellTask, illegal_flag, parse_flags, unsupported_flag, + ParseFlagResult, ShellTask, parse_flags, unsupported_flag, }; use crate::shell::io_writer::{ChildPtr, WriterTag}; use crate::shell::yield_::Yield; @@ -792,7 +792,7 @@ impl FlagParser for Opts { None } - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option { + fn parse_short(&mut self, ch: u8) -> Option { match ch { b'f' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-f"))), b'H' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-H"))), @@ -809,7 +809,7 @@ impl FlagParser for Opts { Some(ParseFlagResult::ContinueParsing) } b'n' => Some(ParseFlagResult::ContinueParsing), - _ => Some(ParseFlagResult::IllegalOption(illegal_flag(smallflags, i))), + _ => Some(ParseFlagResult::IllegalOption(ch)), } } } diff --git a/src/runtime/shell/builtin/ls.rs b/src/runtime/shell/builtin/ls.rs index 293f46088d80..71502210be60 100644 --- a/src/runtime/shell/builtin/ls.rs +++ b/src/runtime/shell/builtin/ls.rs @@ -44,7 +44,8 @@ pub struct ExecState { enum ParseFlag { ContinueParsing, Done, - IllegalOption(Box<[u8]>), + /// The rejected option byte, as getopt(3) reports it. + IllegalOption(u8), } impl Ls { @@ -72,12 +73,12 @@ impl Ls { // which case we run once with ".". let paths_start = match Self::parse_opts(interp, cmd) { Ok(p) => p, - Err(opt) => { + Err(ch) => { let buf: Vec = Builtin::fmt_error_arena( interp, cmd, Some(Kind::Ls), - format_args!("illegal option -- {}\n", bstr::BStr::new(&opt[..])), + format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), ) .to_vec(); Self::state_mut(interp, cmd).state = State::WaitingWriteErr; @@ -221,7 +222,7 @@ impl Ls { /// Returns the index of the first non-flag arg, or `None` if there are no /// positional args. `Err` carries the offending flag byte. - fn parse_opts(interp: &Interpreter, cmd: NodeId) -> Result, Box<[u8]>> { + fn parse_opts(interp: &Interpreter, cmd: NodeId) -> Result, u8> { let argc = Builtin::of(interp, cmd).args_slice().len(); if argc == 0 { return Ok(None); @@ -232,7 +233,7 @@ impl Ls { match Self::parse_flag(&mut Self::state_mut(interp, cmd).opts, flag) { ParseFlag::Done => return Ok(Some(idx)), ParseFlag::ContinueParsing => {} - ParseFlag::IllegalOption(s) => return Err(s), + ParseFlag::IllegalOption(ch) => return Err(ch), } idx += 1; } @@ -245,7 +246,7 @@ impl Ls { } // FIXME windows if flag.len() == 1 { - return ParseFlag::IllegalOption(Box::from(&b"-"[..])); + return ParseFlag::IllegalOption(b'-'); } for &ch in &flag[1..] { match ch { @@ -259,7 +260,7 @@ impl Ls { | b'h' | b'H' | b'i' | b'I' | b'k' | b'L' | b'm' | b'n' | b'N' | b'o' | b'p' | b'q' | b'Q' | b's' | b'S' | b't' | b'T' | b'u' | b'U' | b'v' | b'w' | b'x' | b'X' | b'Z' => {} - _ => return ParseFlag::IllegalOption(Box::from([ch])), + _ => return ParseFlag::IllegalOption(ch), } } ParseFlag::ContinueParsing diff --git a/src/runtime/shell/builtin/mkdir.rs b/src/runtime/shell/builtin/mkdir.rs index 79d30b828775..8c882d9cbdd8 100644 --- a/src/runtime/shell/builtin/mkdir.rs +++ b/src/runtime/shell/builtin/mkdir.rs @@ -4,7 +4,7 @@ use crate::shell::ExitCode; use crate::shell::builtin::{Builtin, BuiltinState, IoKind, Kind}; use crate::shell::interpreter::{ EventLoopHandle, FlagParser, Interpreter, NodeId, OutputSrc, OutputTask, OutputTaskVTable, - ParseFlagResult, ShellTask, illegal_flag, parse_flags, unsupported_flag, + ParseFlagResult, ShellTask, parse_flags, unsupported_flag, }; use crate::shell::io_writer::{ChildPtr, WriterTag}; use crate::shell::yield_::Yield; @@ -443,7 +443,7 @@ impl FlagParser for Opts { None } - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option { + fn parse_short(&mut self, ch: u8) -> Option { match ch { b'm' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-m "))), b'p' => { @@ -454,7 +454,7 @@ impl FlagParser for Opts { self.verbose = true; None } - _ => Some(ParseFlagResult::IllegalOption(illegal_flag(smallflags, i))), + _ => Some(ParseFlagResult::IllegalOption(ch)), } } } diff --git a/src/runtime/shell/builtin/rm.rs b/src/runtime/shell/builtin/rm.rs index b1306bdb50f4..6c27a343fcd3 100644 --- a/src/runtime/shell/builtin/rm.rs +++ b/src/runtime/shell/builtin/rm.rs @@ -92,10 +92,8 @@ pub enum PromptBehaviour { enum RmParseFlag { ContinueParsing, Done, - /// Unknown `--long` option. - IllegalOption, - /// Unknown short option: the rejected byte. - IllegalOptionWithFlag(u8), + /// The rejected option byte, as getopt(3) reports it (`-` for an unknown `--long` option). + IllegalOption(u8), } impl Rm { @@ -242,28 +240,7 @@ impl Rm { }); continue; } - RmParseFlag::IllegalOption => { - return Self::write_err_literal( - interp, - cmd, - idx, - b"rm: illegal option -- -\n", - ); - } - RmParseFlag::IllegalOptionWithFlag(ch) => { - if let Some(safeguard) = Builtin::of(interp, cmd).stderr.needs_io() { - Self::state_mut(interp, cmd).state = RmState::ParseOpts { - idx, - wait_write_err: true, - }; - let child = ChildPtr::new(cmd, WriterTag::Builtin); - return Builtin::of_mut(interp, cmd).stderr.enqueue_fmt( - child, - Some(Kind::Rm), - format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), - safeguard, - ); - } + RmParseFlag::IllegalOption(ch) => { let buf = Builtin::fmt_error_arena( interp, cmd, @@ -271,8 +248,7 @@ impl Rm { format_args!("illegal option -- {}\n", bstr::BStr::new(&[ch])), ) .to_vec(); - let _ = Builtin::write_no_io(interp, cmd, IoKind::Stderr, &buf); - return Builtin::done(interp, cmd, 1); + return Self::write_err_literal(interp, cmd, idx, &buf); } } } @@ -517,7 +493,7 @@ impl Rm { opts.prompt_behaviour = PromptBehaviour::Always; RmParseFlag::ContinueParsing } - _ => RmParseFlag::IllegalOption, + _ => RmParseFlag::IllegalOption(b'-'), }; } for &ch in &flag[1..] { @@ -531,7 +507,7 @@ impl Rm { b'd' => opts.remove_empty_dirs = true, b'i' => opts.prompt_behaviour = PromptBehaviour::Once { removed_count: 0 }, b'I' => opts.prompt_behaviour = PromptBehaviour::Always, - _ => return RmParseFlag::IllegalOptionWithFlag(ch), + _ => return RmParseFlag::IllegalOption(ch), } } RmParseFlag::ContinueParsing diff --git a/src/runtime/shell/builtin/touch.rs b/src/runtime/shell/builtin/touch.rs index 9f1a4283880c..65ccc33eb09a 100644 --- a/src/runtime/shell/builtin/touch.rs +++ b/src/runtime/shell/builtin/touch.rs @@ -2,7 +2,7 @@ use crate::shell::ExitCode; use crate::shell::builtin::{Builtin, BuiltinState, IoKind, Kind}; use crate::shell::interpreter::{ EventLoopHandle, FlagParser, Interpreter, NodeId, OutputSrc, OutputTask, OutputTaskVTable, - ParseFlagResult, ShellTask, illegal_flag, parse_flags, unsupported_flag, + ParseFlagResult, ShellTask, parse_flags, unsupported_flag, }; use crate::shell::io_writer::{ChildPtr, WriterTag}; use crate::shell::yield_::Yield; @@ -364,7 +364,7 @@ impl FlagParser for Opts { } } - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option { + fn parse_short(&mut self, ch: u8) -> Option { match ch { b'a' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-a"))), b'c' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-c"))), @@ -373,7 +373,7 @@ impl FlagParser for Opts { b'm' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-m"))), b'r' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-r"))), b't' => Some(ParseFlagResult::Unsupported(unsupported_flag(b"-t"))), - _ => Some(ParseFlagResult::IllegalOption(illegal_flag(smallflags, i))), + _ => Some(ParseFlagResult::IllegalOption(ch)), } } } diff --git a/src/runtime/shell/interpreter.rs b/src/runtime/shell/interpreter.rs index 625443a5260d..985fa650919f 100644 --- a/src/runtime/shell/interpreter.rs +++ b/src/runtime/shell/interpreter.rs @@ -2303,58 +2303,34 @@ pub(crate) fn shell_openat( // ──────────────────────────────────────────────────────────────────────────── /// Custom parse error for invalid options. -/// -/// Payload slices borrow from the builtin's argv (NUL-terminated arena strings) -/// or are `'static` literals; the builtin formats them into an error message -/// before the next argv mutation, so a raw fat pointer is safe. pub(crate) enum ParseError { - IllegalOption(*const [u8]), - Unsupported(*const [u8]), + /// The rejected option byte, as getopt(3) reports it. + IllegalOption(u8), + Unsupported(&'static [u8]), ShowUsage, } -impl ParseError { - /// Borrow the option-name payload. The pointer borrows either a `'static` - /// literal (e.g. `b"-"`) or the owning `Builtin`'s argv storage - /// (NUL-terminated `Vec` in `Cmd::args`, live for the `Builtin`'s - /// lifetime — see [`Builtin::arg_bytes`](crate::shell::builtin::Builtin::arg_bytes)). - /// Builtins format the error before any argv mutation. - #[inline] - pub(crate) fn opt(&self) -> &[u8] { - match self { - // SAFETY: see doc comment. - ParseError::IllegalOption(s) | ParseError::Unsupported(s) => unsafe { &**s }, - ParseError::ShowUsage => b"", - } - } -} - pub enum ParseFlagResult { ContinueParsing, Done, - IllegalOption(*const [u8]), - Unsupported(*const [u8]), + /// The rejected option byte, as getopt(3) reports it. + IllegalOption(u8), + Unsupported(&'static [u8]), } /// Returns just `name` and lets the caller's `fmt_error_arena` add the /// "unsupported option" prefix once. #[inline] -pub(crate) const fn unsupported_flag(name: &'static [u8]) -> *const [u8] { - std::ptr::from_ref::<[u8]>(name) -} - -/// `IllegalOption` payload for `parse_short`: just the rejected byte, as getopt(3) reports it. -#[inline] -pub(crate) fn illegal_flag(smallflags: &[u8], i: usize) -> *const [u8] { - &raw const smallflags[i..=i] +pub(crate) const fn unsupported_flag(name: &'static [u8]) -> &'static [u8] { + name } /// Per-builtin opts type implements this to plug into `FlagParser::parse_flags`. pub trait FlagParser { /// Handle a `--long` flag. Return `None` to fall through to short parsing. fn parse_long(&mut self, flag: &[u8]) -> Option; - /// Handle one byte of a `-abc` cluster: `None` keeps iterating, [`illegal_flag`] rejects it. - fn parse_short(&mut self, ch: u8, smallflags: &[u8], i: usize) -> Option; + /// Handle one byte of a `-abc` cluster. Return `None` to keep iterating. + fn parse_short(&mut self, ch: u8) -> Option; } /// Returns the trailing non-flag args (`args[idx..]`) on success. @@ -2385,16 +2361,15 @@ fn parse_one_flag(opts: &mut O, flag: &[u8]) -> ParseFlagResult { return ParseFlagResult::Done; } if flag.len() == 1 { - return ParseFlagResult::IllegalOption(std::ptr::from_ref::<[u8]>(b"-")); + return ParseFlagResult::IllegalOption(b'-'); } if flag.len() > 2 && flag[1] == b'-' { if let Some(r) = opts.parse_long(flag) { return r; } } - let small_flags = &flag[1..]; - for (i, &ch) in small_flags.iter().enumerate() { - if let Some(r) = opts.parse_short(ch, small_flags, i) { + for &ch in &flag[1..] { + if let Some(r) = opts.parse_short(ch) { return r; } } diff --git a/test/js/bun/shell/commands/rm.test.ts b/test/js/bun/shell/commands/rm.test.ts index d442eb197773..92f14a2a31fd 100644 --- a/test/js/bun/shell/commands/rm.test.ts +++ b/test/js/bun/shell/commands/rm.test.ts @@ -30,6 +30,13 @@ describe.concurrent("bunshell rm", () => { .doesNotExist("node_modules") .runAsTest("node_modules"); + // With .quiet() stderr is a buffer rather than an fd, the other way a parse error gets written. + test("illegal option in a cluster", async () => { + const { stderr, exitCode } = await $`rm -rz`.quiet(); + expect(stderr.toString()).toBe("rm: illegal option -- z\n"); + expect(exitCode).toBe(1); + }); + test("force", async () => { const files = { "existent.txt": "", diff --git a/test/js/bun/shell/exec.test.ts b/test/js/bun/shell/exec.test.ts index 2d40747e7acd..d85a08bb1d91 100644 --- a/test/js/bun/shell/exec.test.ts +++ b/test/js/bun/shell/exec.test.ts @@ -98,6 +98,22 @@ describe("bun exec", () => { } }); + // Recognised but unimplemented options take the other branch of the same parser result. + describe.concurrent("unsupported option names the option", () => { + for (const [program, option] of [ + ["cat", "-n"], + ["touch", "--no-create"], + ["cp", "-i"], + ]) { + TestBuilder.command`${BUN} exec ${`${program} ${option}`}` + .env({ ...bunEnv, BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS: "1" }) + .exitCode(1) + .stderr(`${program}: unsupported option, please open a GitHub issue -- ${option}\n`) + .stdout("") + .runAsTest(`${program} ${option}`); + } + }); + TestBuilder.command`${BUN} exec cd` .env(bunEnv) .exitCode(0)