add check for set e and operator

[jchorl]

Add a new rule to check for && usage with set -e set.

Recently I got burned by https://unix.stackexchange.com/questions/312631/bash-script-with-set-e-doesnt-stop-on-command/318412#318412

I'm pretty new to Haskell so I figured I'd start small. In the future, we could expand this to include if, while, ||, etc.

While this rule is very broad-reaching, I believe it to be helpful.

Looking for advice on:

1. Is this a reasonable thing to info on?
2. What else is required to add a new rule (e.g. adding the wiki page)?
3. Is the implementation idiomatic haskell?

For some backstory on why this is useful:

I had a script like:

#!/bin/bash

set -e

(exit 1) && echo "hello"
exit_code="$?"
echo command exited "$exit_code"

This actually prints command exited 1, while I'd expect it to not even make it to the echo.

The following script does not make it to the echo:

#!/bin/bash

set -e

(exit 1)
exit_code="$?"
echo command exited "$exit_code"

To me this was extremely unintuitive and I wish shellcheck would have warned me.

[koalaman]

set -e certainly has many issues.

Part of what it does is to forego exiting when a failing command is used as a condition, such as with if, while and &&. It can certainly be surprising, but it's also one of the primary purposes of the mode, so always warning on all of them could get noisy. There are also some corner cases:

# Probably not expected to exit
if [ -e .bashrc ] && [ -e .kshrc ]; then ...; else ...; fi
cd foo && ./configure || true
[[ $verbose ]] && set -x

# Does exit on failure
script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

Is the expectation that the user will rewrite AND-lists into ; lists or if statements depending on whether they do or don't want the script to exit when the condition fails?

[jchorl]

Thanks for the response! Agreed that -e has issues - my hope is to leverage shellcheck to help users better understand and navigate unexpected behaviour.

Is the expectation that the user will rewrite AND-lists into ; lists or if statements depending on whether they do or don't want the script to exit when the condition fails?

I could think of a couple remediations given this warning:

1. Rewrite the && list into if if they do not want the script to exit upon failure of the condition check (this is way more intuitive to me - you would expect if foo to not error if foo is false). IMO ; lists should also be treated similarly (why not separate onto multiple lines) because they're also subject to this footgun.
2. Add some kind of #shellcheck-ignore above the line to say "yes I'm sure I don't want the script to exit if anything in the pipeline is false"

If we wanted to make this more user friendly, we could not warn if the line ends with || true or other common cases where it's clear the user knows what they're doing.

I'm not particularly tied to any specific implementation - just feel that there is an opportunity here to help users avoid a footgun. However we go about that is 👍

[koalaman]

; lists are fortunately not subject to this, but yes, I think this makes sense.

If the && is:

• not in a context where exit would already be ignored, such as a condition in if/while or left of ||
• not the a context where failure would be immediately followed by exit, such as in the last and/or-list in the script, subshell, command expansion, or process substitution.

then suggest rewriting it via if or ;/linefeed

Obviously this increases the complexity a bit and is not fair to ask of this small PR, but let me know if you'd still like to give it a whack. All the information can be deduced from walking the list of parent elements (getPath (parentMap params) x), and the first point is already done via the function isCondition which could also be adapted into a isFinalCommandInShell or similar.

[jchorl]

I'm on vacation, might as well mess around with haskell. I'll take a stab at it over the next week and see what happens.

[uri-canva]

This check would be quite useful, the more I thought about it the more confused I got, so I tested out the whole truth table of common logical operators:

evaluate_boolean() {
    local expression="$1"
    local output
    output=$(bash -c "set -e; $expression; echo end")
    local rc
    eval "$expression"
    rc=$?
    if [[ "$output" == "end" ]]; then
        echo "$expression returns ${rc} and continues"
    else
        echo "$expression returns ${rc} and exits"
    fi
}

while read -r expression; do
    evaluate_boolean "$expression"
done <<EOF
false && true
false || true
true && false
true || false
true && true
true || true
false && false
false || false
! true
! false
true
false
EOF

false && true returns 1 and continues
false || true returns 0 and continues
true && false returns 1 and exits
true || false returns 0 and continues
true && true returns 0 and continues
true || true returns 0 and continues
false && false returns 1 and continues
false || false returns 1 and exits
! true returns 1 and continues
! false returns 0 and continues
true returns 0 and continues
false returns 1 and exits

With || exit codes and whether set -e will cause the shell to exit or not matches, but && and even ! can have unintuitive behaviours.

My highlight is false && false returns 1 and continues.

These results are very confusing so I'd love my script to have some bug I've missed, but I've tested the most egregious results in a plain shell and could reproduce them.

[uri-canva]

The documentation for set -e https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#set explains it, but it's still very unintuitive.