Record the command's id in CFSetExitCode for built-in handlers - #3516
Open
Eljees wants to merge 1 commit into
Open
Record the command's id in CFSetExitCode for built-in handlers#3516Eljees wants to merge 1 commit into
Eljees wants to merge 1 commit into
Conversation
handleCommand receives the whole T_SimpleCommand as `cmd`, and the ordinary path registers the exit code against it through `handleOthers (getId cmd) ...`. regularExpansionWithStatus, used by the built-in table, shadowed that name with the command's first word, so printf, unset, wait, mapfile, readarray, read and the four DEFINE_* commands recorded their exit code under the id of a T_NormalWord instead. Consumers that resolve the id through idMap then get a word where they expect a command. checkOverwrittenExitCode is one of them: getCommandBasename is Nothing for a T_NormalWord, so isPrinting never matched and SC2320 stayed silent for printf while firing for echo, which is not in the table and goes through handleOthers. Drop the shadowing pattern so cmd again refers to the command. Fixes koalaman#3490
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.
Fixes #3490.
Symptom
SC2320 fires for
echobut not forprintf:A four-way probe puts the discriminator on the command name rather than on quoting:
printf '...%d' $?printf '...%d' "$?"echo "...$?"echo $?Cause
handleCommand cmd vars args literalCmdreceives the wholeT_SimpleCommandascmd, and the ordinary path registers the exit code against it:regular = handleOthers (getId cmd) vars args literalCmdregularExpansionWithStatus, which the built-in table uses, shadows that name with the command's first word:So
printf,unset,wait,mapfile,readarray,readand the fourDEFINE_*commands all register their exit code under the id of aT_NormalWord. A consumer that resolves that id throughidMapthen gets a word where it expects a command, andcheckOverwrittenExitCodeis exactly such a consumer:getCommandBasenameisNothingfor aT_NormalWord, soisPrintingnever matches.echois not in the table, goes throughhandleOthers, and therefore works.Fix
Drop the shadowing pattern so
cmdrefers to the command again, matchinghandleOthers. One token; the body of the helper is unchanged.Tests
Two
prop_cases next to the existing ones:The second pins the intended narrowness:
readalso goes through the helper and also gets a corrected id, but it is neither a condition nor a printing command, so nothing new is reported for it.Verified on GHC 9.8.4:
cabal testfails (*** Failed! Falsified (after 1 test),Test suite test-shellcheck: FAIL);cabal testpasses in full;CLAUDE.md: the reporter's script now reports SC2320,echois unchanged, andmycommand; unset foo; [ $? -eq 0 ]stays silent apart from the unrelated SC2181.AI usage
I used Claude to help trace the CFG path and to draft the patch and the two tests, following the workflow in this repository's
.claude/CLAUDE.md. I read every line of the change, ran the four-way probe to establish the cause rather than assume it, ran the new tests against unpatchedmasterfirst to confirm they fail without the fix, and ran the fullcabal testand the end-to-end checks myself.