executor: a lone $ is a literal, not the shell pid (#672) - #748
Merged
Conversation
`$$` is the process ID. A single `$` introduces nothing -- there is no
parameter named "" -- so the only coherent reading is the character itself.
An explicit special case returned the PID instead:
echo $ -> 71681 bash, zsh, dash: $
v=$ -> stores the pid
Every OTHER position was already correct -- `a$`, `"$"`, `'$'`, `5$`, `$%`
and `cost: 5$` all produced the literal -- which is why this only showed when
the `$` stood alone as an unquoted word, and why it survived so long.
bash, zsh and dash all print the literal and lush's own model gives the same
answer independently, so the fix is mode-invariant and gates nothing. Asked
explicitly whether lush can do better than the three here: no. A lone `$` has
one sensible meaning and this is it.
Both expansion paths were verified to agree afterwards (default CST route,
LUSH_WORD_CST=0, and under LUSH_WORD_CST_AUDIT), since the issue noted the
audit could not see this one: legacy and CST were equally wrong, so nothing
diverged for it to catch.
tests/integration/test_dangling_dollar.c: 24 checks -- the lone `$` in eight
positions (word, before another word, assigned, doubled, through a pipe,
inside a substitution, as a printf argument, in a for list), `$$` still a pid
by shape, `$?`/`$#` untouched, the six positions that were already correct,
and six where a `$` sits next to a REAL expansion that must not be disturbed.
9 fail against the parent build.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #672.
The defect
$$is the process ID. A single$introduces nothing — there is no parameter named""— so the only coherent reading is the character itself. An explicit special case returned the PID:Every other position was already correct —
a$,"$",'$',5$,$%,cost: 5$all produced the literal. That is why it only showed when the$stood alone as an unquoted word, and why it survived so long.Curation
$has one sensible meaning and this is it. Recorded rather than left implicit; mode-invariant, gates nothing.On the audit blind spot
The issue noted the CST audit could not flag this, because the legacy and CST paths were equally wrong — there was no divergence to catch. Both were verified to agree after the fix: default CST route,
LUSH_WORD_CST=0, and underLUSH_WORD_CST_AUDIT=1(no abort).Verification
tests/integration/test_dangling_dollar.c— 24 checks: the lone$in eight positions (whole word, before another word, assigned, doubled, through a pipe, inside a substitution, as aprintfargument, in aforlist);$$still a pid, asserted by shape since it has no fixed value;$?and$#untouched; the six positions that were already correct; and six where a$sits adjacent to a real expansion ($v$,$(printf x)$,$((1+1))$,${HOME:+x}$) that must not be disturbed.