shellcheck

shellcheck copied to clipboard

For new checks and feature suggestions

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

Here's a snippet or screenshot that shows the problem:

#!/bin/sh

foo()
{
    set -x
    false
}

bar()
{
    set -x
    false
    set +x
}

if foo; then echo success; else echo fail; fi
if bar; then echo success; else echo fail; fi

Output:

+ false
+ echo fail
fail
+ bar
+ set -x
+ false
+ set +x
success

Here's what shellcheck currently says:

Nothing

Here's what I wanted or expected to see:

Warning: Shell builtins mask return codes

With any normal command, I would consider this expected behavior. However, since set isn't really a "normal" command, it is unexpected that it could mask return codes and break the conditional. It's kind of a corner case, but I just ran into it in the field :) There may also be other areas where this could happen outside my sample here, so I'm not sure about the scope of the issue.

It is not masking the return code. The last command is just successful and you are missing set -eou pipefail.

You are clearly not understanding the issue here. Please read the entire post before commenting.

You think that bash handles set any different to any other command which is not the case. It might be technically a builtin but as far as I know that has no influence on return codes. It would be unexpected to handle it any different to any other command in a function.

I do not "think" that bash handles set any differently. Again, you did not read the issue.

Why should shellcheck flag a script that is doing what you tell it to do? How could this case be detected generally? set +x is a perfectly valid thing to do.

Finally, you're being unhelpful by saying "you did not read the issue". If you think that the issue is being misunderstood, take the time to explain why.

Why should shellcheck flag a script that is doing what you tell it to do?

This kind of question is so rhetorical as to be completely unconstructive. There are many "gotchas" and footguns in many languages, especially so in shell scripts. There are already many, many warnings in shellcheck that are of the form "I see what you are trying to do here, but you actually are telling the shell to do X, so here is a better way, or here is the 'correct' way". I am asking for this check to be added, as one of these gotcha warnings.

How could this case be detected generally?

In exactly the way I described in the OP. If someone is checking a return code right after using set, they are 99.9999% trying to check the return code of the command right before set, and not whether set succeeded.

A more trivial example might be:

set -x
a
long
series
of
commands
set +x
if [ "$?" = "0" ]...

I think anyone seeing that might find it obvious that whoever wrote it made a mistake. Of course, this more trivial example would already sort of be caught be shellcheck, since it warns about checking "$?" in an if statement as opposed to just checking the command directly.

This is why I posted the more complex version, which is a simplified version of what I ran into in the field when debugging an archaic script that was misbehaving. I usually turn to shellcheck to look at very old scripts, since it is very good at finding little gotchas that probably are creating the bug I'm investigating.

Luckily, since shellcheck is fairly advanced, I feel like it might be able to see a bit deeper into how scripts run to detect this more complex case. Here is the broad logic:

Say, if the last command in a function/alias being checked by the user is a shell builtin, then assume it is a mistake and offer a warning.

The broad logic is very straightforward, but of course things usually wind up being more difficult than they appear, as I said in OP.

if [ "$?" = "0" ]...

shellcheck won't like that either ..

Thanks for the detailed explanation. @SuperSandro2000 will appreciate it as well. I'm glad I didn't have to debug a failing script due to set +x being the last statement in a function. This is a useful cautionary tale. It might also be helpful to remind people watching this issue that set -x / set +x enables and disables tracing of commands and thus the return value is next-to meaningless (like you point out).

Now I can see how detecting this case would be useful. I'm not sure if it should be extended to all POSIX sh builtins. I will say I've never (knowingly) checked the return value of any.

Indeed, it can get quite hairy. I would think some builtins are fair game, exec comes to mind, since if it returns at all theres a problem, so it would be fair to check that one in a script. Also the most obvious one being return, which really should be checked.

This is why I mentioned concerns about scope at the end of OP, because it could easily turn into having some semi-arbitrary subset of shell builtins being checked, or different builtin lists for different shells (many shells have true and false as builtins for example).

To me though, set is one command that really stands out as never being worth checking, as I'm sure the buggy script author also felt.