Add new check for error ignoring on command substitutions

[felipecrs]

For new checks and feature suggestions

• https://www.shellcheck.net/ (i.e. the latest commit) currently gives no useful warnings about this
• I searched through https://github.com/koalaman/shellcheck/issues and didn't find anything related

Suggestion

Given the following script:

#!/bin/bash

set -e

basename "$(curl -fsSL -w '%{url_effective}' -o /dev/null https://github.com/docker/compose/releases/latest)"

It would normally work, but curl could actually fail (such as with command not found). So, for the sake of the example, I will force it to fail by using curl2.

Examples:

$ bash <<'EOF'; echo $?
#!/bin/bash

set -e

basename "$(curl -fsSL -w '%{url_effective}' -o /dev/null https://github.com/docker/compose/releases/latest)"
EOF
v2.0.0
0

With curl2:

$ bash <<'EOF'; echo $?
#!/bin/bash

set -e

basename "$(curl2 -fsSL -w '%{url_effective}' -o /dev/null https://github.com/docker/compose/releases/latest)"
EOF
bash: line 5: curl2: command not found

0

The status code of the latter example is 0 even if the command failed. This is misleading. ShellCheck could advise to be written like so instead:

$ bash <<'EOF'; echo $?
#!/bin/bash

set -e

curl_output="$(curl2 -fsSL -w '%{url_effective}' -o /dev/null https://github.com/docker/compose/releases/latest)"
basename "$curl_output"
EOF
bash: line 5: curl2: command not found
127

References twpayne/chezmoi#1466 (comment)

[kurahaupo]

Yet another strike against set -e for creating expectations that directly conflict with how the shell actually works and how correct, idiomatic shell code is normally written.

If such a warning were given, I would like it to be gated on the presence of set -e, and for the wording of the warning to point out that set -e makes promises that cannot be met.

[felipecrs]

I couldn't agree more with @kurahaupo.

[koalaman]

Thanks to @DoxasticFox you can now enable an optional check for this:

#!/bin/bash
# shellcheck enable=check-extra-masked-returns
set -e
basename "$(curl -fsSL -w '%{url_effective}' -o /dev/null https://github.com/docker/compose/releases/latest)"

results in

basename "$(curl -fsSL -w '%{url_effective}' -o /dev/null https://github.com/docker/compose/releases/latest)"
            ^-- SC2312 (info): Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).

[felipecrs]

This is awesome!

[felipecrs]

For reference, this is @DoxasticFox's PR: #2320

[felipecrs]

Using this feature, I realized there are:

[[ $(command -v curl) ]]

and

my_function "$(cat <<EOF
my
multiline
parameter
EOF
)"

Which are two cases very known, and return code on them is very likely not to mean anything. Should this rule have exceptions like these above?

[DoxasticFox]

@felipecrs I think this:

command -v curl

and

my_function "my
multiline
parameter
"

should have similar effects. Why not these?

[felipecrs]

For the first, you are right. Btw the drop-in replacement would be command -v curl &>/dev/null, as my first does not print to stdout too.

For the second, there are multiple benefits of using here-docs, such as 1. better readability in the code, 2. allow to be indented with <<-EOF and 3. no need to escape quotes inside of it.

[DoxasticFox]

Ah, I didn't understand the heredoc was the important part. Well, #2320 assumes that certain commands' exit codes can be safely masked. We could add cat to that list of commands. But that seems overly permissive to me, because cat file/that/might/not/exist is a common use of cat and it can fail. Also, based on @koalaman's comment #2320 (comment), he might need some convincing to accept a PR dealing with special cases, like where cat takes a heredoc. It might be worth raising an issue to discuss it.

In the meanwhile, another alternative is this:

arg="$(cat <<EOF
my
multiline
parameter
EOF
)"
my_function "$arg"

It's a bit of shame to have to introduce a variable to deal with this case though, I'll admit. But it wouldn't be the only case where using set -e would force you to do that.

[felipecrs]

That's right. Thanks for the rationale.

Perhaps an alternative solution would be to allow # shellcheck disable=check-extra-masked-returns instead of having to use || true, as it would be just a comment rather than being an actual command in the code which bash will have to test whether the command failed or not at runtime.

[DoxasticFox]

You can use # shellcheck disable=SC2312.

[felipecrs]

Btw I created the issue for discussing: #2359

[felipecrs]

You can use # shellcheck disable=SC2312.

Thanks a lot, it works!

[GhostLyrics]

Is enabling this optional rule necessary if you run your (bash) script with -o pipefail? I think the wiki page for this check should contain a hint about whether this makes sense.

[felipecrs]

I don't get it. What is the relationship between this and -o pipefail?

[GhostLyrics]

I don't get it. What is the relationship between this and -o pipefail?

For context - I run my scripts with set -euo pipefail.

For example, I scanned my existing code:

example 1

In resources/scripts/helper/REDACTED_lib.sh line 269:
            unprocessed_jobs_count=$(echo "$unprocessed_jobs" | tail --lines +2 | wc --lines)
                                                                ^-------------^ SC2312 (info): Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).

In my understanding enabling pipefail will not cause the errors to be swallowed because the entire "pipeline" will fail as soon as the first step fails.

example 2

Similarly here - if one step fails, the pipeline will fail and it would use the else branch of my code:

In resources/scripts/helper/REDACTED_lib.sh line 275:
        if ! jobs_in_queue=$(REDACTED status | tail --lines +2 | grep --count --invert-match "$excluded_strings"); then
                             ^-------------^ SC2312 (info): Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).
                                               ^-------------^ SC2312 (info): Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).

Scanned section in question for example 2:

# grep returns code 0 with no results
if ! jobs_in_queue=$(REDACTED status | tail --lines +2 | grep --count --invert-match "$excluded_strings"); then
    if [ "$jobs_in_queue" == 0 ]; then
        REDACTED "Processed all pending jobs."
        break
    fi

    REDACTED "REDACTED: could not retrieve pending jobs." "$jobs_in_queue"
fi

[DoxasticFox]

@GhostLyrics SC2312 will be suppressed in pipelines if you use set -o pipefail, but there's other ways a command's exit code can be masked. The unit tests give examples.

[kurahaupo]

[[ $(command -v curl) ]]

This is sadly a badly over-used idiom.

1. In many cases (including command -v) the exit status will tell you what you want to know, and the output can be entirely ignored: command -v curl >/dev/null 2>&1
2. If the objective is truly to test whether the output of a command is not empty, it's not necessary to wait for the entire output of the command to finish. Once it outputs a single byte, you have the answer to that question: somecmd | read will tell you whether the output of somecmd is non-empty, without waiting for somecmd to finish. (Normally somecmd would be terminated almost immediately with SIGPIPE, and that would be a good thing.)

my_function "$(cat <<EOF
my
multiline
parameter
EOF
)"

Multiline strings are valid, so if it's simple just use:

my_function 'my
multiline
parameter'

If you have complex embedded quoting, it would probably make the code clearer if the value is in a variable anyway:

sql_expression=$(cat) <<\EOF
SELECT count(*) as `Num`,
       'String with "Quotes"' as `Label`
FROM bananas
WHERE ripeness like '%firm%'
AND price = '$14'
EOF
my_sql_cmd -e "$sql_expression"

Either way, the relevant exit status is more obvious to ShellCheck.