Repository navigation
feat(shell): support multiple redirections and fix dup redirect for builtins #25668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d6ca5d9
26844e0
6e17dfd
1cfc388
d286180
a8ff796
d928ba1
21f2b49
acf5db8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| /workspace/bun | ||
| /workspace/bun | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| /workspace/bun | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # Search for pwd_output.txt and append_output.txt
find . -name "pwd_output.txt" -o -name "append_output.txt" 2>/dev/null | head -20Repository: oven-sh/bun Length of output: 91 🏁 Script executed: # Also search for file-io.test.ts
find . -name "file-io.test.ts" 2>/dev/nullRepository: oven-sh/bun Length of output: 90 🏁 Script executed: # Search for references to these files in the codebase
rg "pwd_output|append_output" --type ts --type js -l 2>/dev/nullRepository: oven-sh/bun Length of output: 88 🏁 Script executed: # Examine the file-io.test.ts to understand how these files are used
cat -n test/js/bun/shell/file-io.test.tsRepository: oven-sh/bun Length of output: 12375 🏁 Script executed: # Check the content of both files
echo "=== pwd_output.txt ===" && cat pwd_output.txt && echo -e "\n=== append_output.txt ===" && cat append_output.txtRepository: oven-sh/bun Length of output: 134 🏁 Script executed: # Check if these files are mentioned in git status or gitignore
git ls-files | grep -E "pwd_output|append_output"Repository: oven-sh/bun Length of output: 87 Remove test artifacts from version control and add to .gitignore. Both
🤖 Prompt for AI Agents |
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -805,8 +805,7 @@ pub const AST = struct { | |
| pub const Cmd = struct { | ||
| assigns: []Assign, | ||
| name_and_args: []Atom, | ||
| redirect: RedirectFlags = .{}, | ||
| redirect_file: ?Redirect = null, | ||
| redirects: Redirects = .{}, | ||
|
|
||
| pub fn memoryCost(this: *const @This()) usize { | ||
| var cost: usize = @sizeOf(Cmd); | ||
|
|
@@ -816,12 +815,56 @@ pub const AST = struct { | |
| for (this.name_and_args) |*atom| { | ||
| cost += atom.memoryCost(); | ||
| } | ||
| cost += this.redirects.memoryCost(); | ||
| return cost; | ||
| } | ||
| }; | ||
|
|
||
| if (this.redirect_file) |*redirect_file| { | ||
| cost += redirect_file.memoryCost(); | ||
| } | ||
| /// Redirect destinations for stdin, stdout, and stderr | ||
| pub const Redirects = struct { | ||
| stdin: RedirectTarget = .none, | ||
| stdout: RedirectTarget = .none, | ||
| stderr: RedirectTarget = .none, | ||
|
|
||
| /// Type for specifying which IO stream | ||
| pub const IoKind = enum { stdin, stdout, stderr }; | ||
|
|
||
| pub fn memoryCost(this: *const @This()) usize { | ||
| var cost: usize = @sizeOf(Redirects); | ||
| cost += this.stdin.memoryCost(); | ||
| cost += this.stdout.memoryCost(); | ||
| cost += this.stderr.memoryCost(); | ||
| return cost; | ||
| } | ||
|
|
||
| /// Check if a given io stream is redirected elsewhere | ||
| pub fn redirectsElsewhere(this: *const Redirects, comptime io_kind: IoKind) bool { | ||
| return @field(this, @tagName(io_kind)) != .none; | ||
| } | ||
| }; | ||
|
|
||
| /// Where a stream is redirected to | ||
| pub const RedirectTarget = union(enum) { | ||
| /// No redirect - use the default for this stream | ||
| none: void, | ||
| /// Redirect to a file path (from shell text) | ||
| atom: struct { | ||
| atom: Atom, | ||
| append: bool = false, | ||
| }, | ||
| /// Redirect to a JS object (ArrayBuffer, Blob, etc.) | ||
| jsbuf: JSBuf, | ||
| /// Duplicate to another fd (e.g., 2>&1 means stderr duplicates to stdout's destination) | ||
| dup: enum { stdin, stdout, stderr }, | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove ?RedirectKind and use none: void instead so we avoid the struct size increase unnecessarily |
||
| pub fn memoryCost(this: *const @This()) usize { | ||
| return switch (this.*) { | ||
| .none => 0, | ||
| .atom => |a| @sizeOf(RedirectTarget) + a.atom.memoryCost(), | ||
| .jsbuf => @sizeOf(RedirectTarget), | ||
| .dup => @sizeOf(RedirectTarget), | ||
| }; | ||
| } | ||
| }; | ||
|
|
||
| /// Bit flags for redirects: | ||
|
|
@@ -833,8 +876,6 @@ pub const AST = struct { | |
| /// - `1>>` = Redirect.Append | Redirect.Stdout | ||
| /// - `2>>` = Redirect.Append | Redirect.Stderr | ||
| /// - `&>>` = Redirect.Append | Redirect.Stdout | Redirect.Stderr | ||
| /// | ||
| /// Multiple redirects and redirecting stdin is not supported yet. | ||
| pub const RedirectFlags = packed struct(u8) { | ||
| stdin: bool = false, | ||
| stdout: bool = false, | ||
|
|
@@ -849,7 +890,9 @@ pub const AST = struct { | |
| return @as(u8, @bitCast(this)) == 0; | ||
| } | ||
|
|
||
| pub fn redirectsElsewhere(this: RedirectFlags, io_kind: enum { stdin, stdout, stderr }) bool { | ||
| pub const IoKind = enum { stdin, stdout, stderr }; | ||
|
|
||
| pub fn redirectsElsewhere(this: RedirectFlags, io_kind: IoKind) bool { | ||
| return switch (io_kind) { | ||
| .stdin => this.stdin, | ||
| .stdout => if (this.duplicate_out) !this.stdout else this.stdout, | ||
|
|
@@ -1636,16 +1679,76 @@ pub const Parser = struct { | |
| while (try self.parse_atom()) |arg| { | ||
| try name_and_args.append(arg); | ||
| } | ||
| const parsed_redirect = try self.parse_redirect(); | ||
| const redirects = try self.parse_redirects(); | ||
|
|
||
| return .{ .cmd = .{ | ||
| .assigns = assigns.items[0..], | ||
| .name_and_args = name_and_args.items[0..], | ||
| .redirect_file = parsed_redirect.redirect, | ||
| .redirect = parsed_redirect.flags, | ||
| .redirects = redirects, | ||
| } }; | ||
| } | ||
|
|
||
| /// Parse zero or more redirections and build a Redirects struct. | ||
| /// Each redirect updates the appropriate field (stdin, stdout, stderr). | ||
| /// For duplicate redirects like 2>&1, we set the target to .dup. | ||
| fn parse_redirects(self: *Parser) !AST.Redirects { | ||
| var redirects: AST.Redirects = .{}; | ||
|
|
||
| while (self.match(.Redirect)) { | ||
| const flags = self.prev().Redirect; | ||
|
|
||
| // Empty flags means a no-op redirect (e.g., >&1 which redirects stdout to stdout) | ||
| if (flags.isEmpty()) { | ||
| continue; | ||
| } | ||
|
|
||
| // Handle fd duplication (2>&1, 1>&2, >&0, >&2) | ||
| if (flags.duplicate_out) { | ||
| // For "2>&1": flags.stdout=true means stderr goes to stdout | ||
| // For "1>&2": flags.stderr=true means stdout goes to stderr | ||
| // For ">&0": flags.stdin=true means stdout goes to stdin | ||
| // For ">&2": flags.stderr=true means stdout goes to stderr | ||
| if (flags.stdin) { | ||
| // >&0 - redirect stdout to stdin | ||
| redirects.stdout = .{ .dup = .stdin }; | ||
| } else if (flags.stdout) { | ||
| redirects.stderr = .{ .dup = .stdout }; | ||
| } else if (flags.stderr) { | ||
| redirects.stdout = .{ .dup = .stderr }; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| // Parse the redirect target (file path or JS object) | ||
| const target: AST.RedirectTarget = target: { | ||
| if (self.match(.JSObjRef)) { | ||
| const obj_ref = self.prev().JSObjRef; | ||
| break :target .{ .jsbuf = AST.JSBuf.new(obj_ref) }; | ||
| } | ||
|
|
||
| const atom = try self.parse_atom() orelse { | ||
| try self.add_error("Redirection with no file", .{}); | ||
| return ParseError.Expected; | ||
| }; | ||
| break :target .{ .atom = .{ .atom = atom, .append = flags.append } }; | ||
| }; | ||
|
|
||
| // Set the appropriate redirect field based on which fd is being redirected | ||
| if (flags.stdin) { | ||
| redirects.stdin = target; | ||
| } | ||
| if (flags.stdout) { | ||
| redirects.stdout = target; | ||
| } | ||
| if (flags.stderr) { | ||
| redirects.stderr = target; | ||
| } | ||
| } | ||
|
|
||
| return redirects; | ||
| } | ||
|
|
||
| /// Parse a single redirect (used for subshells which only support one redirect) | ||
| fn parse_redirect(self: *Parser) !ParsedRedirect { | ||
| const has_redirect = self.match(.Redirect); | ||
| const redirect = if (has_redirect) self.prev().Redirect else AST.RedirectFlags{}; | ||
|
|
@@ -1656,16 +1759,15 @@ pub const Parser = struct { | |
| break :redirect_file .{ .jsbuf = AST.JSBuf.new(obj_ref) }; | ||
| } | ||
|
|
||
| const redirect_file = try self.parse_atom() orelse { | ||
| const redirect_target = try self.parse_atom() orelse { | ||
| if (redirect.duplicate_out) break :redirect_file null; | ||
| try self.add_error("Redirection with no file", .{}); | ||
| return ParseError.Expected; | ||
| }; | ||
| break :redirect_file .{ .atom = redirect_file }; | ||
| break :redirect_file .{ .atom = redirect_target }; | ||
| } | ||
| break :redirect_file null; | ||
| }; | ||
| // TODO check for multiple redirects and error | ||
| return .{ .flags = redirect, .redirect = redirect_file }; | ||
| } | ||
|
|
||
|
|
@@ -2964,6 +3066,52 @@ pub fn NewLexer(comptime encoding: StringEncoding) type { | |
| }; | ||
| } | ||
|
|
||
| // Check for >&0, >&1, or >&2 (fd duplication without explicit source fd) | ||
| // >&0 means redirect stdout to stdin (unusual but valid in POSIX) | ||
| // >&1 means stdout goes to stdout (no-op) | ||
| // >&2 means stdout goes to stderr | ||
| if (dir == .out) { | ||
| if (self.peek()) |peeked| { | ||
| if (!peeked.escaped and peeked.char == '&') { | ||
| _ = self.eat(); | ||
| if (self.peek()) |peeked2| { | ||
| switch (peeked2.char) { | ||
| '0' => { | ||
| // >&0 means redirect stdout to stdin (unusual but valid) | ||
| _ = self.eat(); | ||
| return .{ | ||
| .stdin = true, | ||
| .duplicate_out = true, | ||
| }; | ||
| }, | ||
| '1' => { | ||
| // >&1 means stdout to stdout (no-op) | ||
| // Just consume the token and return a plain > redirect | ||
| // with no effect - the parser will not set any redirect | ||
| _ = self.eat(); | ||
| // Return empty flags - this is effectively a no-op | ||
| return .{}; | ||
| }, | ||
| '2' => { | ||
| // >&2 means stdout goes to stderr | ||
| _ = self.eat(); | ||
| return .{ | ||
| .stdout = false, // We're redirecting stdout | ||
| .stderr = true, // to stderr | ||
| .duplicate_out = true, | ||
| }; | ||
| }, | ||
| else => { | ||
| // Not a valid fd dup, backtrack the '&' | ||
| // Actually we can't easily backtrack, so this case will | ||
| // be handled as an error later. For now just return > redirect. | ||
| }, | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return switch (dir) { | ||
| .out => AST.RedirectFlags.@">"(), | ||
| .in => AST.RedirectFlags.@"<"(), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 776
Move
append_output.txtto the test directory or add it to.gitignore.This file is referenced in
test/js/bun/shell/file-io.test.ts:252as an output target for shell redirect testing (&>> append_output.txt). However, it's currently at the repository root and tracked by git. Test artifacts should either:test/js/bun/shell/per repository conventions, or.gitignore(like other test artifacts such asfailing-tests.txt)The current content (
/workspace/bunrepeated twice) is leftover test output from previous runs, not intentional file content. Ensure the file is properly cleaned up after tests execute or excluded from version control.🤖 Prompt for AI Agents