Repository navigation
shell: keep variables in one table with an export attribute, and prefix assignments on their command #45371
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
Open
robobun
wants to merge
10
commits into
main
Choose a base branch
from
robobun/06b9ce56/shell-export-attribute
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+604
−175
Open
shell: keep variables in one table with an export attribute, and prefix assignments on their command #45371
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
b110471
shell: keep variables in one table with an export attribute
robobun 5277ee9
[autofix.ci] apply automated fixes
autofix-ci[bot] c6f88fe
shell: shorten the variable table comments, make the cd test independ…
robobun be4b33c
shell: keep the export state in the variable table's own entry, not i…
robobun 07bcb8a
shell: keep a command's NAME=value prefixes on the command
robobun f51d531
shell: find a replaced exported name by name compare, look up export …
robobun 08e204f
shell: test the child view of cd, the Windows name case, later lookup…
robobun c4b24d0
Merge remote-tracking branch 'origin/main' into robobun/06b9ce56/shel…
robobun b7013ad
shell: test that export PATH with no value keeps the PATH for which a…
robobun 3ad5807
shell: match the PATH key for the command lookup by the name rule of …
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🔴 Windows users whose script assigns a lowercase shell variable such as
path=dist,os=$(uname)ortmp=...now hand that value to every later child process under the inheritedPath/OS/TMPname, breaking those children; on the base branch the child kept the inherited value.VarTable::setat src/runtime/shell/EnvMap.rs:158 looks the name up with the ASCII-case-insensitiveEnvMapContext, finds the inheritedExportedentry and overwrites its value in place, sochild_envemits it. Fix: a plain assignment whose spelling differs from an inherited name must not change what children receive for that name, with the same rule atexport(EnvMap.rs:182) and thecdwrites ofPWD/OLDPWD(interpreter.rs:1981).Why this was flagged
A Bun Shell script on Windows runs
path=./dist; node build.js. The default env is process.env, so the table holdsPath,OS,TMPasVar::Exported(src/runtime/shell/ParsedShellScript.rs:112).Assigns::child_donecallsshell.vars.set(name, value_ref)(src/runtime/shell/states/Assigns.rs:144);setcallsself.map.get_or_put(name)at src/runtime/shell/EnvMap.rs:158, andEnvMapContext::eql(EnvMap.rs:35) compares case-insensitively on Windows, sofound_existingis true and theVar::Exported(old)arm at EnvMap.rs:170 replaces the inherited value.child_env(EnvMap.rs:224) then yieldsPath=./disttofill_envandnodeis spawned with a one-entry Path, so anything it spawns (npm,cmd,git) fails with not-found. On the base branch the assignment went to the separateshell_envmap (assign_var, old interpreter.rs:2008) and the child was built fromexport_envonly, so it kept the inheritedPath/OS. The test "a name in another case is the same variable on Windows only" (test/js/bun/shell/bunshell.test.ts) pins the new behaviour.Verification:
VarTable::set(EnvMap.rs:157-176) doesself.map.get_or_put(name)at line 158, sopathfinds the existingPathentry, and lines 170-173 replace it in place withVar::Exported(value).child_env(EnvMap.rs:224-229) then emitsPath=./distto every later child. On the base,AssignCtx::Shellwrote onlyshell_env.insert(label, value), soshell_enventries never reached the child.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.
Kept for now, and it is a maintainer decision (question 2 in #45371 (comment)).
The shell's variable names are ASCII-case-insensitive on Windows on main too:
EnvMapContextis the key rule of all three maps. So on mainpath=distalready changes what$PATHand$Pathexpand to in the shell. Only the child disagreed with the shell, and that disagreement is the bug this PR fixes.A rule by spelling cannot keep the fix. The inherited key is
Path, soPATH=C:\tools;$PATHis also "a spelling that differs from an inherited name". To keeppath=distlocal and letPATH=...reach the child, the shell needs another model: names upper-cased at startup and case-sensitive after that, as Cygwin does for a fixed list. That changes the names a child sees.You are right about the cost, and after 3ad5807 it is larger: on Windows
path=dist; node build.jsnow fails withbun: command not found: node, because the lookup reads the same variable. The description lists this under Downsides, the docs state the rule, and the testa name in another case is the same variable on Windows onlyrecords it. I leave this thread open for the maintainer.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.
One precedent for the other model, for whoever decides. deno_task_shell upper-cases environment names on Windows and keeps shell variables case-sensitive (
src/shell/types.rs:apply_env_varupper-cases the name, andEnvChange::SetShellVarupdates an environment variable only when the name as written is a key ofenv_vars). TherePATH=xupdates the environment andpath=diststays a shell variable. The cost of that model is that a child gets the upper-cased names (PATH, notPath), and thatPath=xis a shell variable too.This PR keeps the rule that main already has for the shell on Windows (one name for every spelling), so the thread stays open as a design question, not as a defect I missed.