Repository navigation
shell: expand $? and $$
#45490
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
base: main
Are you sure you want to change the base?
shell: expand $? and $$
#45490
Changes from all commits
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 |
|---|---|---|
|
|
@@ -1233,6 +1233,60 @@ booga" | |
| expect(str).toEqual("bar baz\n"); | ||
| }); | ||
|
|
||
| describe("$?", () => { | ||
| TestBuilder.command`echo $?`.stdout("0\n").runAsTest("is 0 at the start"); | ||
| TestBuilder.command`false; echo $?`.stdout("1\n").runAsTest("after a builtin"); | ||
| TestBuilder.command`false; echo "$?" a$?b`.stdout("1 a1b\n").runAsTest("in double quotes and in a word"); | ||
| TestBuilder.command`${BUN} -e "process.exit(42)"; echo $?`.stdout("42\n").runAsTest("after a command"); | ||
| TestBuilder.command`false; true; echo $?`.stdout("0\n").runAsTest("is the status of the last statement"); | ||
| TestBuilder.command`false || echo $?`.stdout("1\n").runAsTest("right of ||"); | ||
| TestBuilder.command`true && false; echo $?`.stdout("1\n").runAsTest("after &&"); | ||
| TestBuilder.command`true | false; echo $?; false | true; echo $?`.stdout("1\n0\n").runAsTest("after a pipeline"); | ||
| TestBuilder.command`if false; then echo no; else echo $?; fi`.stdout("1\n").runAsTest("in an else branch"); | ||
| TestBuilder.command`false; if false; then echo no; fi; echo $?` | ||
| .stdout("0\n") | ||
| .runAsTest("after an if that takes no branch"); | ||
| TestBuilder.command`(false); echo $?; false; (echo $?)`.stdout("1\n1\n").runAsTest("out of and into a subshell"); | ||
| TestBuilder.command`false; echo $(echo $?)`.stdout("1\n").runAsTest("into a command substitution"); | ||
| // dash prints 1 here and bash prints 0. | ||
| TestBuilder.command`false; echo $(true)$?` | ||
| .stdout("1\n") | ||
| .runAsTest("a $(...) in the same command does not set it"); | ||
| TestBuilder.command`false; STATUS=$?; true; echo $STATUS`.stdout("1\n").runAsTest("as an assignment value"); | ||
| TestBuilder.command`false; [[ $? == 1 ]] && echo yes`.stdout("yes\n").runAsTest("in [[ ]]"); | ||
| TestBuilder.command`false; echo hi > out_$?.txt` | ||
| .ensureTempDir() | ||
| .fileEquals("out_1.txt", "hi\n") | ||
| .runAsTest("in a redirect target"); | ||
| TestBuilder.command`${{ raw: "false; echo é $?" }}` | ||
| .stdout("é 1\n") | ||
| .runAsTest("in a script with a non-ASCII character"); | ||
| TestBuilder.command`false; echo '$?' \$? $\?`.stdout("$? $? $?\n").runAsTest("quoted or escaped stays text"); | ||
| TestBuilder.command`false; echo $${"?"} ${"$?"}`.stdout("$? $?\n").runAsTest("an interpolated value stays text"); | ||
|
|
||
| // `$?` does not see the status of an assignment from `$(...)` today. #45074 and #37839 change this row to 1. | ||
| TestBuilder.command`X=$(false); echo $?`.stdout("0\n").runAsTest("after an assignment from a $(...) that fails"); | ||
|
Comment on lines
+1267
to
+1268
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. 🟣 pre-existing, not blocking: Users who check a command substitution's status with Why this was flaggedTrigger: any Bun shell script (Bun.$, Verification: src/runtime/shell/states/Expansion.rs:656-657 sets |
||
| TestBuilder.command`shell_test_no_such_command; echo $?` | ||
| .stdout("1\n") | ||
| .stderr("bun: command not found: shell_test_no_such_command\n") | ||
| .runAsTest("after a command that is not found"); | ||
|
Comment on lines
+1267
to
+1272
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. 🟡 nit (optional): Two new tests lock in values the PR itself calls wrong, so the follow-up PRs must flip assertions instead of un-todoing them. bunshell.test.ts:1268 asserts Why this was flaggedThe describe block at bunshell.test.ts:1236 adds Verification: nit; acknowledged in diff: /home/claude/bun/test/js/bun/shell/bunshell.test.ts:1268 pins the divergent value: |
||
| }); | ||
|
|
||
| describe("$$", () => { | ||
| TestBuilder.command`echo $$ "$$" $$x` | ||
| .stdout(`${process.pid} ${process.pid} ${process.pid}x\n`) | ||
| .runAsTest("is the process ID of bun"); | ||
| TestBuilder.command`echo $(echo $$); (echo $$)` | ||
| .stdout(`${process.pid}\n${process.pid}\n`) | ||
| .runAsTest("is the same in $(...) and (...)"); | ||
| TestBuilder.command`${{ raw: "echo é $$" }}` | ||
| .stdout(`é ${process.pid}\n`) | ||
| .runAsTest("in a script with a non-ASCII character"); | ||
| TestBuilder.command`echo $$(echo hi)`.stdout("$hi\n").runAsTest("$$( is still a $ and a $(...)"); | ||
| TestBuilder.command`echo '$$' \$\$ $\$`.stdout("$$ $$ $$\n").runAsTest("quoted or escaped stays text"); | ||
| TestBuilder.command`echo $${"$"} ${"$$"}`.stdout("$$ $$\n").runAsTest("an interpolated value stays text"); | ||
| }); | ||
|
|
||
| test("shell var", async () => { | ||
| const { stdout } = await $`FOO=bar BAR=baz && BAZ=1 ${BUN} -e "console.log(JSON.stringify(process.env))"`; | ||
| const str = stdout.toString(); | ||
|
|
||
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.
🟣 pre-existing, not blocking: pre-existing, nit: a script that ends with a bare
exitstill exits 0 after a failed command, whileexit $?now works, so the two spellings disagree. POSIXexitwith no operand uses the last exit status, which this PR now stores inShellExecEnv.last_exit_code(Expansion.rs:508) butexit.rs:22still maps zero arguments to 0. Fix: make the no-argumentexitbuiltin readbase.shell().last_exit_codesofalse; exit,exitandexit $?agree, and add afalse; exit; echo $?style test next to the new$?cases.A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A Bun shell script such as
some-check; exitrun through Bun.$,bun exec,bun run --shell=bunor any package.json script on Windows reaches theexitbuiltin with zero arguments. src/runtime/shell/builtin/exit.rs:22 maps0 => 0, so the Cmd completes with exit code 0 regardless of the previous command's status. On the base branch the same script also exits 0 (the state did not exist), so this is pre-existing; but the diff now records the last status inShellExecEnv.last_exit_code(src/runtime/shell/interpreter.rs:1806, written at src/runtime/shell/states/Stmt.rs:81 and src/runtime/shell/states/Binary.rs:84) and expands it for$?at src/runtime/shell/states/Expansion.rs:508, sofalse; exit $?yields 1 whilefalse; exitstill yields 0.shyields 1 for both. No guard prevents it:exitnever consults the env. The PR text listsX=$(false)and command-not-found as the only$?divergences and does not mention bareexit.Verification: pre-existing (nit). Trigger: any Bun shell script that runs a bare
exitafter a failing command, e.g.false; exit.src/runtime/shell/builtin/exit.rs:21-22still mapsmatch bltn.args_slice().len() { 0 => 0, ... }, so a bareexitalways completes the Cmd with code 0. The base branch behaves identically for bareexit, so merging makes nothing worse.