Show info about set -e suppression during function calls

[DoxasticFox]

This PR provides an enhancement very similar to the one described in #2054. The following example is copied from that issue:

#!/bin/bash
set -e

foo() {
    set -e
    false
    echo "output after failed command even with set -e"
}

if foo; then
    echo
fi

The current version of shellcheck provides no notice, although this PR produces the following:

if foo; then
   ^-^ SC2304: This function invocation will run with set -e disabled.

For more information:
  https://www.shellcheck.net/wiki/SC2304 -- This function invocation will run...

It also handles all the other cases (that I know of) where set -e can be disabled during a function call. The new notice can be suppressed by inserting set -e into the correct context. For example this program produces an error in shellcheck:

#!/bin/bash
set -e
foo() { :; }
bar=$(foo)

But this one doesn't:

#!/bin/bash
set -e
foo() { :; }
bar=$(set -e; foo)

Note that Bash doesn't allow set -e to be "re-enabled" in most contexts. For example, this wouldn't re-enable set -e:

#!/bin/bash
set -e
foo() { :; }
if
    set -e
    foo
then
    :
fi

As a result, shellcheck would still display the error. Another interesting edge case is this:

#!/bin/bash
set -e
foo() { :; }
bar=$(set -e; foo) || true

Although you can ordinarily re-enable set -e in command substitutions, the || true overrides that. So a shellcheck notice is still produced in this case.

It's also worth highlighting that this PR only deals provides notices about function invocations, so shellcheck wouldn't produce any output for the program set -e; x=$(git). Related issues and PRs exist (#1484, #2237) to address more general cases.

[uri-canva]

Given that it's very idiomatic to have functions that are meant to be called in conditionals even in scripts where set -e is used, it's best to make this an optional check by adding it to optionalTreeChecks.

[DoxasticFox]

I ran this on the monorepo at work and generated 318 new shellcheck notices! Most of them look useful to me, but there's some useless ones too.

I'd say the most common useless class of error is for library functions which are clearly safe. It'd be nice if we could add a shellcheck directive to the function to mark it as being safe for use in set -e-suppressed contexts. Unfortunately, although I haven't investigated yet, I assume you either need to add the directive near where the error occurs, or make significant changes to shellcheck.

There's also a few instances where function invocations end in || true. My initial thought was that it was useless because the author clearly knew what they were doing in that case. But on second thought, maybe they assumed || true only suppressed the function's return code, as opposed to potentially changing the code path it follows.

There's also a few cases I missed. For example, it doesn't notify about subshells invoked with parentheses. (Edit: This isn't actually a problem. Thanks, @uri-canva for pointing that out.) Also, it does notify about functions which are passed as arguments. I got a few false positives that way. Finally, shopt -s inherit_errexit allows command substitutions to inherit set -e, so that should be used as a heuristic to avoid some notices.

[uri-canva]

For example, it doesn't notify about subshells invoked with parentheses.

Those always inherit set -e no?

bash-4.4$ (set -e; (a() { echo hello; false; echo world; } ; a))
hello
bash-4.4$ (set -e; echo $(a() { echo hello; false; echo world; } ; a))
hello world

[DoxasticFox]

@koalaman I think I'm done making changes. PTAL when you're ready.

There's a few things I'd like to talk about, centred around the phrasing of the new check (SC2304). I've noticed that, unlike this new check, the vast majority of existing ones don't say what's wrong with a code snippet. Rather, they say how it can be improved. I didn't do that, because I think there's a lot of ambiguity around the best course of action. All the cases covered by SC2304 can be fixed by turning the affected functions into scripts, but I imagine that many people would consider that excessive. We can provide more nuanced feedback for the cases where the Bash programmer can simply re-enable set -e, i.e. in dollar or backtick command substitutions. In those cases, we can provide an error message like Add 'set -e' here or 'shopt -s inherit_errexit' to the top of your script to ensure this function runs with 'set -e' enabled. But in all the other cases, I think the programmer will have to decide what's best for their situation. And actually, I'm not totally sold on telling the user what to do for command substitutions, because I wrote SC2304 to be enabled when hasSetE is True. But it'll be True if set -e is present anywhere in the script, which risks false positives. AFAIK, there's no way to know if set -e was set in a function Token's parent context until the Bash script is executed. So I think it might be better for shellcheck to point out the potential mistake rather than advocate a potentially misguided solution. Strategies for dealing with SC2304 can be described in the wiki. What do you think?

[koalaman]

Nice!

While the end result is similar, there's two very different root causes at play with different fixes:

1. errexit is disabled because the command is used as a condition
2. errexit is disabled because Bash has some special behavior related to certain subshells

I think it's worth separating these into two different warnings, for both message and wiki purposes:

1. "This function is invoked as a condition so set -e will be disabled. Invoke separately if failures should cause the script to exit."
2. "Bash implicitly disabled set -e for this invocation. Add set -e; before it or enable inherit_errexit."

This might also simplify the logic a bit as mentioned in one of the comments

[koalaman]

inherit_errexit is Bash specific. Dash and Sh already behave this way. Ksh does not support it afaik. You could set it per shell like hasLastPipe above

[koalaman]

I'm guessing this won't be used anywhere, so you could move it under checkDisabledSetEInIf

[koalaman]

With if hasSetE params then runNodeAnalysis checkNode params t else [] you could skip this entire check when set -e is not in effect, instead of iterating through the nodes and checking if it's set for each.

[koalaman]

This could be something like

if isCondition $ getPath (parentMap params) cmd
then return $ info ... "This function is invoked as a condition so ..."
else
  if not hasInheritErrExit && isPlaceWhereBashDisablesSetE
  then return $ info ... "Bash implicitly disabled `set -e` for this invocation"
  else Nothing
fi

This way, it would only start inspecting the context when it finds a function, instead of doing that work for every cat and ls that won't lead anywhere, as well as avoid reimplementing most of the logic of the existing function isCondition (which could be probably be updated to count T_Banged with no adverse effects)

[DoxasticFox]

I don't think using isCondition would work properly for conditions which execute multiple functions in the conditional part. For example, in the following, the f1 and f2 functions would both run without set -e, but isCondition would only return true for f2:

if
    f1
    f2
then
    :
fi

Also, now that we're entertaining the idea of splitting the warnings, something else which occurred to me is that set -e can be disabled for multiple reasons at the same time. Examples:

x=$(foo && echo bar)

cat <(
  (
    x=$(foo)
    echo "$x"
  ) || true"
)

Now I'm leaning towards giving a different error message for each case listed in getSuppressedChildren. I'll give the ones which share the same behaviour (i.e. conditions, command substitutions and process substitutions) the same SC2xxx code, but any language construct (e.g. if, while, &&, etc) which can be responsible for an error will be given its own message. Does that sound okay?

[DoxasticFox]

Your efficiency suggestion SGTM though. Thanks!

[koalaman]

This is also one of the Bash specific set -e re-enablable ones apparently

[DoxasticFox]

I actually just realised that the behaviour of this is different than in the other cases. In the other cases, Bash disables set -e globally, causing the function and script to continue if a command within them fail.

But the function called in a T_ProcSub inherits set -e, even without inherit_errexit enabled. So the function might terminate early, as it would with set -e. But if that happens, it'll only cause the process spawned by the T_ProcSub to exit early, not the parent script! D: More special cases than my brain has room for!

[DoxasticFox]

Well, at least that's what happens on my machine. bash --version gives me: GNU bash, version 5.0.3(1)-release (x86_64-pc-linux-gnu).

[DoxasticFox]

@koalaman I just implemented all the requested changes except for these (1, 2), as described in the comments. PTAL when you have time. Now when I run shellcheck on this...

#!/bin/bash
set -e

bar() { :; }

if
  bar && true
  echo
then
  :
fi

cat <(
  (
    x=$(bar)
    echo "$x"
  ) || true
)

...I get this output:

In /tmp/a.sh line 7:
  bar && true
  ^-^ SC2304: This function is invoked in an && condition so set -e will be disabled. Invoke separately if failures should cause the script to exit.
  ^-^ SC2304: This function is invoked in an 'if' condition so set -e will be disabled. Invoke separately if failures should cause the script to exit.


In /tmp/a.sh line 15:
    x=$(bar)
        ^-^ SC2304: This function is invoked in an || condition so set -e will be disabled. Invoke separately if failures should cause the script to exit.
        ^-^ SC2305: set -e will not cause the script to exit if this function exits unsuccessfully because it appears in a process substitution. Capture its output with $(...) instead.
        ^-^ SC2306: Bash implicitly disabled set -e for this function invocation because it's inside a command substitution. Add set -e; before it or enable inherit_errexit.

[koalaman]

You were right that these do inherit set -e. The reason why it doesn't exit the script is because it runs in a subshell whose exit code is ignored. This is true for a lot of other cases as well, like f &, echo "$(set -e; f)" and f | cat.

Warning about such things is not necessarily a bad feature, but it's an entirely different issue. You can just ignore this case here.

[DoxasticFox]

That's a good point. It's not set -e being suppressed, but the exit code.

I'd still like to raise a separate PR for the process substitution case, later on. I should give it an SCxxxx number separate from the cases you mention, right? After all, SC2155 already exists for one related case.

[koalaman]

I think it would be more correct/expected to treat an alias invocation as a single command because that's almost universally what they are. This would happen if you just ignore aliases.

Sure it would basically mean copy-pasting findFunctions, but A. they're small, B. they do slightly different things (trying to find shell-only definitions vs find functions), and C. you could optionally add (name, containsSetE t) as the value to easily and efficiently avoid triggering on f() { set -e; false; echo foo; }; x=$(f);

[DoxasticFox]

Ignoring aliases SGTM, but I didn't do the optional bit because I worry it'll be brittle.

[koalaman]

If I'm reading this right, this might all be easier as:

checkCmd = go . getPath (parentMap params)
 where
  go (child:parent:rest) = do
    case parent of
      T_AndIf _ condition _ | child `isIn` condition -> informConditional "an && condition" t
      T_DollarExpansion {} | not $ errExitEnabled parent -> informUninherited t
      [...]
      _ -> return () 
    go (parent:rest) 
  go _ -> return ()

  errExitEnabled t = hasInheritErrexit params || containsSetE t
  isIn t cmds = getId t `elem` map getId cmds

[DoxasticFox]

Thanks!

[koalaman]

Do you think this warning should trigger for things like { a; b; } || c and $(a; b) for the same reason why it triggers for f() { a; b; }; f || c and $(f)?

It's just a thought and I don't expect it of this PR.

[DoxasticFox]

When I ran this PR on the monorepo at work, I found that set -e was mostly suppressed in functions which were used in command substitutions and if statements, not || or && conditions. (I think that's because people don't want to branch with && and || in production code.) So, admittedly, I don't have much empirical evidence to suggest it'll be that useful.

I also think set -e's behaviour is more insidious for functions than the cases you mention, because, one might reasonably expect functions and commands to respond similarly to set -e. i.e. You wouldn't expect the behaviour of the command/function to change, you'd expect the script's response to the command/function's exit code to change.

Nonetheless, I still think the cases you mention are unintuitive enough to warrant their own warnings, because nobody could know errexit is disabled in those contexts without being told. I don't really want to raise a PR though because I haven't seen those pop up much in the wild and I'm lazy 😂

[koalaman]

Looks good to me! Please squash into one neat commit for merge.

The checks are helpful for all shells, but the optional description is bash specific. I'll probably switch it out for something broader that also highlights the problem, maybe:

    ,(newCheckDescription {
        cdName = "check-set-e-suppressed",
        cdDescription = "Notify when set -e is suppressed during function invocation",
        cdPositive = "set -e; func() { cp *.txt ~/backup; rm *.txt; }; func && echo ok",
        cdNegative = "set -e; func() { cp *.txt ~/backup; rm *.txt; }; func; echo ok"
    }, checkSetESuppressed)

I might potentially also add support for using # shellcheck disable on the function instead of having to do it on every call site when you know the function is safe.

[DoxasticFox]

Thanks, @koalaman!

In addition to squashing the current commits, I made two more changes:

• Added the description you mentioned.
• Bumped the SCxxxx numbers because they collide with some existing checks now. Might be worthwhile triple-checking them before you merge.

I might potentially also add support for using # shellcheck disable on the function

You read my mind! (Edit: No, wait, I mentioned that here, a fortnight ago.) Our codebase at work has some functions which are obviously safe to use with errexit suppressed. I mentioned to @uri-canva while writing this PR that suppressing errors like this would be nice, but I didn't implement it because I didn't realise it was that easy. Still, I'm enough of a Haskell n00b that I'd expect a few too many iterations on the PR before getting it right 😂. Better leave it to the experts for now 😄

It'd still like to raise a PR for #2303 (comment) though, because it seems pretty simple and I think it's a common mistake.

[uri-canva]

Nice! Thanks @DoxasticFox and @koalaman for the quick review!