Shellcheck: 'exit' called in a function is probably not what you want

[matthewpersico]

For bugs

• Rule Id (if any, e.g. SC1000):
• My shellcheck version (shellcheck --version or 'online'): online
• [x] I tried on shellcheck.net and verified that this is still a problem on the latest commit

For new checks and feature suggestions

• [x] 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/bash
foo ()
{
echo this is a func
exit 0
}

Here's what shellcheck currently says:

Here's what I wanted or expected to see:

Warning: 'exit' called in function. Are you sure you don't want 'return' instead?

[matthewpersico]

Similarly, return from a script should trigger;

Warning: 'return' called outside a function. Are you sure you don't want 'exit' instead? For sourced in scripts, you can disable this check at the top, under shebang.

[jleffler]

I routinely use a function usage() to report usage messages for scripts, which intentionally exits after reporting the message. And I use a function error() to report error messages and exit too. I'd find the warning exasperating.

[harleypig]

I am currently refactoring a fairly large code base that mixes sourcing and just executing files. Sometimes a file is sourced, sometimes it's just executed. It can get confusing as to whether or not I'm in a function or not.

Having shellcheck point out these exits in functions for me would be a huge time saver. I do the same thing as @jleffler does in regards to help and error functions, but it would not be a big deal for me to have to add a '#shellcheck disable ' line.

[matthewpersico]

Hmmm. Maybe it’s time for a new directive: enable. As in you have to explicitly enable this directive. That way those of us that need it don’t interfere with those who don’t need it. And maybe when we’re done converting, we can remove it from our code. And maybe shellcheck gets a new option to set these —enable on the command line. What do you think?

[Dmole]

usage() works with 'return'.

error() would could work with 'return' if you use a state var which would permit cleanup without catching... maybe only warn if not in a sub shell so sourcing scrips are not limited.

[Thaodan]

@matthewpersico: Rpmlint has something like this. Or a #warning like in c. I use preprocessor and with way you can add #warnings to your code on "compile" time. Maybe shellcheck could have something like that, that adds custom rules.

like this

error()
#\\shellcheck warning "please use trap before using this" if trap not fond
{
     stub
}

[Thaodan]

@Dmole sure you can use return to do this. But setting a trap once and just use exit after is cleaner.

[matthewpersico]

This issue is over a year old. @koalaman, if we have not reached a consensus as to what kind of change to make, I suggest we close it.

[koalaman]

I had an empirical look. It's a very common thing to do: in 1000 randomly selected scripts there were 232 instances of exit in functions. I didn't systematically classify each, but from going through the first several dozen, I'd estimate that maybe 20% are good uses (log_fatal, die, abort type functions that wrap exit), and that about 80% are intentional and working uses that I personally would have preferred to rewrite as returns (typical example).

It's also intentional in this function from Microsoft/ChakraCore, even if the rest of the function and its invocations are not. Someone should probably tell them, but I guess they're killing Chakra anyways.

There are a couple of cases like this one from PHP that might warrant a separate warning: exit in a function involved in a subshell with nothing else following it. I filed #1558 for it.

Overall, I wasn't able to find many cases where exit appears to have been accidentally used instead of return, so as posted it seems too noisy.