Remove `check-extra-masked-returns` "harmless commands" list?

[scop]

Wouldn't we be better off without the "harmless commands" list in check-extra-masked-returns, and give people who want to enable the optional check what they ask for, all the way? I was surprised to find dirname didn't trigger this warning in my script while other commands did.

All the commands in the current list can fail due to a number of reasons, including but not limited to usage errors (different options accepted between shells and OS's), and all of them cannot be even guaranteed to be always available (shopt being the most likely suspect in the current list).

I think these kinds of errors are in the same category that lead to rejection of the cat <<EOF addition to the list in #2359 (comment)

[koalaman]

@DoxasticFox What are your thoughts from implementing this check and collecting the list?

[DoxasticFox]

I added dirname to the harmless commands list because the monorepo at work has SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" (or some variation) in lots of scripts. Without making it a "harmless" command, dirname might be the most common source of errors in our repo, which would make the check very noisy for us. basename "$0" was another common source of noise. In one iteration of #2320, I'd written the code so that dirname "$0" and dirname "${BASH_SOURCE[0]}" were considered harmless, but not other invocations of dirname. What do you think of that, @scop? The reason I removed it was because that way of suppressing harmless command invocations is more complicated than the current behaviour, so it's harder for the user to understand why ShellCheck may not have produced an error.

I'd need to re-compile ShellCheck without the harmless commands list to re-collect examples for the other commands---I'll do that when I have some time---But I think it's obvious why, say, echo is there.

[scop]

I'm not sure if such a tweak to examine command args (dirname in the mentioned example) or commands in general when processing would be much of an improvement, or at all. While doing so would eliminate some messages that are conceivably seen as noise, it would also raise eyebrows on consistency (it'd arguably be worse than currently -- currently the treatment is consistent at least per command), and would in any case address just one instance of multiple possible problems anyway.

I'd like to stress that this is not at all just about dirname, it's just what made me notice.

Even echo isn't strictly speaking completely harmless. Granted, cases where it would bite are rare, but they do exist. For example, it might not be a shell builtin, and the system might be out of file descriptors. If we're out of file descriptors we'll have a problem with the command substitution already anyway (but it can hit just at the time echo would be invoked within it, and the situation might resolve before the faulty empty value we got as result of that would be used for something by a script that didn't exit because of the masked exit status). Anyway this would bring it very close to the reasoning why cat <<EOF was rejected as non-harmless. I'm sure one could come up with more failure scenarios. I'm not sure if I'd personally want to see these messages for echo usage always, but I can't say for certain nobody would. The fix isn't that ugly it couldn't be done everywhere or the check disabled where desired, perhaps by refactoring a bit to eliminate unnecessary command substitutions.

If there's no interest in providing per check configurability, having a generic filtering mechanism in shellcheck -- not just one for enabling/disabling entire checks -- would help with letting users decide. It could come with some filters enabled by default (e.g. these things that are generally considered as harmless), and a mechanism for overriding the entire list or removing/adding specific entries in it. Can't say if I'd be a big fan of that though, would likely be rather hard to use. Currently one can do something like ! shellcheck -f gcc /some/script | grep -v something (wrap in sh -c "..." where necessary) but that's quite ugly. Personally I wouldn't really mind, but that won't help with getting shellcheck to emit something it unconditionally filters out internally.

Judgement calls, etc etc and not that strong opinions here especially when we're reaching the level of discussion whether echo should be there. But I wouldn't personally mind removing the harmless list altogether, considering again that the fix isn't that troublesome in any way, and (to lesser extent) that this is an opt-in check in shellcheck.