Add extra checks for masked return codes

[DoxasticFox]

Adds the check mentioned here, plus related ones: #2303 (comment) Example:

/tmp/a.sh:

#!/usr/bin/env bash
set -e
readarray -t files < <(ls)
echo "${files[@]}"

% ./quickrun /tmp/a.sh --enable=check-extra-masked-returns
Up to date

In /tmp/a.sh line 3:
readarray -t files < <(ls)
                       ^-- SC2312: Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).

For more information:
  https://www.shellcheck.net/wiki/SC2312 -- Consider invoking this command se...

[DoxasticFox]

CC @uri-canva

[koalaman]

I think this would need to cover quite a bit more to live up to its name, such as false | true, true $(false), and x=$(false)$(true), and a check specifically for process substitutions in scripts containing set -e seems a bit narrow on its own.

[DoxasticFox]

@koalaman I think it highlights another issue with my chosen name: It's that the check isn't related to set -e so much as it is to masking return values (e.g. as in SC2155). So although set -e makes Bash's treatment of the masked exit codes even more unexpected, I'd still say it's not the core issue. How about I rename check-set-e-ignored to check-masked-exit-codes, or check-extra-masked-exit-codes (seeing as shellcheck already deals with one case of masking in SC2155). I'll also add checks for the cases you mention, being careful to avoid the cases already handled by SC2155. SGTY?

[koalaman]

SGTM. In this case it should recognize || true so that you have an easy escape hatch for things you're willing to assume will succeed, such as x $(y || true) and x < <(y || true).

[DoxasticFox]

@koalaman PTAL

[koalaman]

oversimplify wasn't as clever or useful as I thought it would be 10 years ago. It would be better to use getCommandBasename :: Token -> Maybe String which is smarter and less heavy handed.

[koalaman]

You could move declaringCommands into ShellCheck.Data instead to avoid this dependency and hopefully allow better build parallelization.

[koalaman]

Maybe add " (or use || true to ignore)."

Invoking separately is not necessarily the best advice for pipes but the wiki can go into detail.

[koalaman]

You could instead formulate this as something like

isMasked t = hasParent masker t
  where
    masker t =
      isHereString t
      || hasRightSibblingInPipe t
      || ..

to avoid iterating the same path repeatedly and instead do it in one pass.

[koalaman]

So far so good! Here are some cases it misses:

False negative:

x="$(false)$(false)"      # Check out getWordParts :: Token -> [Token]
x="$(false)""$(false)"    # It takes a T_NormalWord and flattens it into a list of all the quoted and unquoted elements in the string
x=( $(false) $(false) )
x=( $(false) [4]=$(false) )

# These may be less relevant:
cat << foo
 $(false)
foo
x=$(( $(date +%s) ))
[[ $(false) ]]

False positive:

x=$(false) y=z         # T_SimpleCommand has two assignments in its list in this case
x=$(false) y=$(false)  # Only the first one is masked

These are already correctly handled but untested:

echo $(( $(date +%s) ))
x=`false``false`
x=( $(false) )

[DoxasticFox]

Thanks for all these test cases. Any suggestion as to how I should approach this?

x=$(false) y=$(false)  # Only the first one is masked

I was thinking that once I find a T_SimpleCommand, I'd traverse the parse tree upwards, looking for a T_Assignment. Once I find that, I'd look at the assignments on the the left of the T_SimpleCommand and check to see if their descendants contain any T_SimpleCommand nodes. The trouble is that it's not obvious to me how I can iterate over the descendants of a node without having a huge case ... of statement which essentially handles each rule in Bash's grammar.

[koalaman]

This big case...of statement already exists. There's a doAnalysis which invokes each node in a tree with a monadic action. The obvious monad is Cont, but to avoid importing that, perhaps Maybe:

containsSimpleCommand rootnode = isNothing $ doAnalysis (\c -> case c of T_SimpleCommand {} -> fail ""; _ -> return ()) rootnode

Currently the code finds the potentially masked T_SimpleCommand, and tries to determine if it is masked. This has some issues like echo $(t=$(false); echo $t) emitting two warnings even though only one is actually masked, and echo $([[ x ]]) emits nothing since it's a compound command instead of a simple command.

An alternative approach is finding the potential masker, e.g. a T_ProcSub, or a T_SimpleCommand _ [] (cmd:args) where any isCommandSubstitution $ concatMap getWordParts args. Since it seems like this is basically required to recognize the context anyways, maybe this is easier.

[DoxasticFox]

@koalaman How am I going?

[DoxasticFox]

Looks like this PR will address #1733. Not sure if that's a good thing though; is it okay for it to be an optional check?

[koalaman]

Looks pretty good and feature complete to me!

Did you run this on a larger codebase? What was your experience?

[koalaman]

You could also include a check for { false || true; } | true to make sure you can suppress individual stages in the pipeline too (it already works as intended)

[koalaman]

It should probably support ":" as well since people will likely be writing this a lot :P

[koalaman]

Turns out I was wrong about this one, sorry about that:

$ x=$(( $(echo 1; exit 42) )); echo $?
42

[koalaman]

It's not as pretty but you could limit these extra checks to relevant nodes with something like

isRelevant t = not (isHarmlessCommand t || isCheckedElsewhere t || isMaskDeliberate t)
findMaskedNodes t@(T_SimpleCommand _ _ (_:_)) = when (isRelevant t) $ inform t
findMaskedNodes t@T_Condition {} | isRelevant t= when (isRelevant t) $ inform t

This drops total ShellCheck-wide CPU usage by about 5%

[DoxasticFox]

Did you run this on a larger codebase? What was your experience?

@koalaman Yeah, that's why I added these tests:

shellcheck/src/ShellCheck/Analytics.hs

Lines 4772 to 4775 in 56ddb5f

	prop_checkExtraMaskedReturns29 = verifyNotTree checkExtraMaskedReturns "false < <(set -e)"
	prop_checkExtraMaskedReturns30 = verifyNotTree checkExtraMaskedReturns "false < <(shopt -s cdspell)"
	prop_checkExtraMaskedReturns31 = verifyNotTree checkExtraMaskedReturns "false < <(dirname \"${BASH_SOURCE[0]}\")"
	prop_checkExtraMaskedReturns32 = verifyNotTree checkExtraMaskedReturns "SCRIPT_DIR=\"$( cd \"$( dirname \"$0\" )\" && pwd )\""

But actually I should run it once more and eyeball the results again now that I've made those tweaks.

Also, to make the test SCRIPT_DIR=\"$( cd \"$( dirname \"$0\" )\" && pwd )\" pass, I decided to make dirname a "harmless" command. What I really would've liked was to make it harmless only when invoked on $0 or ${BASH_SOURCE[0]}, but I couldn't see an easy and obvious way to do that. Got any pithy ways to check for that, oh Haskell master? (Edit 2: I gave it a shot in 77f393e. Please let me know if there's a better way.)

Edit 1: I ran it again on our monorepo. I'm seeing lots of errors about basename "$0" in our code base. That's another place where it'd be nice to automatically suppress errors for some_command "$0", but not for other invocations of some_command. The output seems okay to me otherwise.

[koalaman]

What I really would've liked was to make it harmless only when invoked on $0 or ${BASH_SOURCE[0]}

I would avoid this kind of overfitting TBH. It's often better for the test to be predictable, even if it makes it slightly more inaccurate. "dirname and basename are ignored since they're rarely sources of failure" is simple and easy, while it's harder to build a mental model given observations like:

• No warning: basename ${BASH_SOURCE[0]}
• Warning: basename ${BASH_SOURCE[0]?}
• No warning: basename $0
• Warning: current_exe=$0; basename $current_exe
• No warning: dirname ${BASH_SOURCE[0]}
• Warning: dirname ${BASH_SOURCE[-1]}

Other than that it looks good to me. Squash for merge?

PS: Any thoughts on a more convincing doc example? Maybe something like

    cdPositive = "rm -r \"$(get_chroot_dir)/home\"",
    cdNegative = "set -e; dir=\"$(get_chroot_dir)\"; rm -r \"$dir/home\""

[DoxasticFox]

Squashed

It's often better for the test to be predictable, even if it makes it slightly more inaccurate.

Makes sense. I reverted 77f393e (the commit to specifically silence dirname $0 and basename $0) and added basename to the list of harmless commands instead.

PS: Any thoughts on a more convincing doc example? Maybe something like [...]

👍

[koalaman]

Thanks! I've merged it and added a wiki page and changelog entry.

[DoxasticFox]

You might also want to consider closing #1733

[DoxasticFox]

Hi again, @koalaman! Thanks for your responsiveness with this and #2303. @Canva is considering using these new checks in our CI pipelines, but we'd rather not go through the rigmarole of compiling our own version. Is it much trouble to ask for a release?

CC @joscha

[koalaman]

@DoxasticFox Sure. There are a few minor issues I'd like to squeeze in first, including #2362 to extend this warning to local -r x=$(false) which you might be interested in, but I can probably get a new version out next weekend if that works.

[koalaman]

@DoxasticFox v0.8.0 is now out