Shellcheck: SC2181 after conditionals

[arth1]

For SC2181, recommending not checking $? but instead test the command directly, it does not take into account that the return code may be based on one of several execution branches.

Consider this:

if some_test; then command1; else command2; fi
if [ $? -ne 0 ]; then
  # regardless of which command ran, it failed
fi

I can't see that this is better written as

if ! if some_test; then command1; else command2; fi; fi; then
  # something
fi

Perhaps SC2181 should not kick in directly after a conditional close like fi or esac?

[kurahaupo]

On Wed, 1 Mar 2017, arth1 wrote:

Consider this:

if some_test; then command1; else command2; fi
if [ $? -ne 0 ]; then
  # regardless of which command ran, it failed
fi

I can't see that this is better written as

if ! if some_test; then command1; else command2; fi; fi; then
  # something
fi

The latter definitely looks like an improvement to me (after fixing it to
remove the extra "fi", and perhaps reformatting it for better legibility:

if ! if some_test ; then
        command1
     else
        command2
     fi
then
  # something
fi

Or perhaps more succinctly:

if some_test
then command1
else command2
fi || handle_failure

Or a lot of ills can be cured by wrapping things in functions:

do_this_or_that() {
    if some_test
    then command1
    else command2
    fi
}


if ! do_this_or_that ; then
    handle_failure
fi

[kurahaupo]

(I suspect half the reason that these types of constructs are regarded as awkward is because people insist -- sometimes via "style guides" -- on putting "then" on the end of the last line of the test, rather than allowing it to be on its own line when that adds clarity.)

[arth1]

The ";then" is irrelevant here, and the concatenation into a single line was for brevity.
The "if ! if" or "if if" construct is awkward, no matter what kind of formatting, and even more so if the test gets split up into several lines. Using $? is quite useful in avoiding such constructs and make the code more readable.

[kurahaupo]

The "if ! if" or "if if" construct is awkward

That is a matter of personal taste, and I disagree.

To me, the nested ifs are perfectly reasonable readable code. Whereas
inspecting $? is just ugly.

So since both ways are considered awful by someone, let's take an
alternative approach: wrap the condition block in a funcion, then we can
both be happy.

Whether that merits a different wording in the warning is an open question.

The ";then" is irrelevant here, and the concatenation into a single line
was for brevity.

Ok, fair enough. But the lack of an obvious termination is often cited
as a reason to discourage multi-line tests, which is silly when putting the
"then" on its own line makes the end of the test block very obvious.

IMO people waste far too much effort trying to make Shell look like some other
language, where -- for example -- putting "then" or its equivalent on the same
line as "if" is the natural and preferred way to do things. The net effect of
such effort is to give people a false mental model of how the shell works, and
that eventually leads to bugs.

The shell is not C, not Java, not Python, not Haskell, not Prolog, and not
Perl. Get over it.

The shell grammar is quite plain:

if LIST ; then LIST ; [ elif LIST ; then LIST ; ]... [ else LIST ; ] fi

All of those LISTs have exactly the same grammatical structure, and all the
semicolons are all "semicolon or newline".

So this is equivalent:

if LIST
then LIST
[ elif LIST
then LIST
]... [ else LIST
] fi

Note that it does NOT say a newline is required after "then" or "else", and
semicolon is actually forbidden there. Instead there's a secondary rule saying that
newlines are permitted but ignored after most keywords.

(An example of a bad mental model of the shell: thinking that newline and
semicolon are interchangeable. They're not; newlines can usually be replaced by
newlines, but the reverse does not apply.)

So IMO it is NOT reasonable to say that some of those LISTs MUST be on one line
and some MUST not, nor that some of those semicolons MUST actually be newlines
while others MUST not. Or that some optional newlines are actually compulsory.

Instead I prefer to allow whichever style makes the most readable code in the
circumstances. Sometimes that will be

if foo ; then
    bar
else
    zot
fi

Other times that will be

if  case $x in
    [aeiou]) true ;;
    [a-z]) false ;;
    ??) are_digraphs_allowed ;;
    *) are_words_allowed ;;
    esac
then
    process_vowel_or_digraph
else
    process_other
fi

And other times that will be

if   use_bw    ; then load_bw_film    ; take_photo ; process_bw_film     ; print_slides
elif use_color ; then load_color_film ; take_photo ; process_colour_film ; print_slides
elif use_audio ; then load_cassette   ; record_sound                     ; burn_cd
else ask_user_what_to_do
fi

Horses for courses.

[koalaman]

This is indeed a bit of an odd case.

My thought would be that you either would or would not take advantage of the exit code of compound commands. If you do, if ! if .. is the logical conclusion. If you don't, there's the coworker-friendly C-style approach:

if some_test
then 
  command1
  fail=$?
else
  command2
  fail=$?
fi

if [ "$fail" -ne 0 ]; then
  true
fi

Though as always, there's no substitute for case-by-case human judgement.

[antofthy]

Another practical example of use of the exit from conditionals... From a script that has been used on a LOT of different machines.

case "$(sh -c 'ping -\? 2>&1')" in
*-w*) # we can give a wait deadline       (Linux/FreeBSD)
         ping -c 1 -w 5 $Vamp >/dev/null 2>&1 ;;
*-c*) # yes so give it a -c option
        ping -c 1 $Vamp >/dev/null 2>&1 ;;
*)    # No -c option, so don't provide it (very old style - Solaris)
       ping $Vamp 5 >/dev/null 2>&1 ;;
esac
if [ $? -ne 0 ]; then
  echo >&2 "ERROR: Client Host \"$Vamp\" does not seem to be online!"
  exit 0;
fi

[qneill]

"IMO people waste far too much effort trying to make Shell look like some other language"

Sometimes style choices are driven by primary consumers of your Shell program - the _users_ who will read, copy, and even improve and maintain the program after it is finished. In such cases the time is not wasted in fact it is a strategic investment.

[qneill]

I am wondering how this particular idiom should be handled:
( set -e; cmd1; cmd2; if...; fi; case... esac; cmd3; cmdN; ); [ $? -ne 0 ] && exit 1

I'm aware there are many ways to refactor this particular code, but I'm more interested in discussing whether SC2181 is too narrowly defined (or how to use a directive to avoid it).

[kurahaupo]

@qneill It's precisely those primary consumers I'm thinking about when I say that making Shell _look_ like a different language is, in some cases, counter-productive. It leads to the naïve future editor introducing bugs when they try to implement new features without understanding how very limited is the mimicry they're looking at.

_Some_ mimicry can be helpful, but I've seen thousand-line libraries trying to implement look-alike features, and falling short, when there are Shell idioms for achieving the same goals more reliably and _much_ more simply.

[kurahaupo]

If I was going to try to mimic some other language, I would opt for something like:

(   set -e  # TRY
    cmd1
    if cmd2
    then
        cmd3
    else
        cmd4
    fi
    ...
) ||        # CATCH
    exit 1