koalaman / shellcheck

ShellCheck, a static analysis tool for shell scripts
https://www.shellcheck.net
GNU General Public License v3.0
36.2k stars 1.77k forks source link

SC2064 is questionable #1945

Open mcandre opened 4 years ago

mcandre commented 4 years ago

There are many reasons to use double quoted, immediate evaluation strings for traps. Basically, to parameterize them.

mnuccioarpae commented 4 years ago

Isn't it covered by the Exceptions?

If you don't care that the trap code is expanded early because the commands/variables won't change during execution of the script, or because you want to use the current and not the future values, then you can ignore this message.

mcandre commented 4 years ago

hen you can ignore this message.

That's what I'm griping about! I think the exceptions outnumber the utility of this rule.

kins-dev commented 3 years ago

Well for bash at least, I think the heuristic should be, if the variable(s) are readonly (constant) then it shouldn't trigger.

mcandre commented 2 years ago

That's great for bash. But I strive to write pure POSIX sh for portability.

kins-dev commented 2 years ago

POSIX supports read only: https://pubs.opengroup.org/onlinepubs/009695299/utilities/readonly.html

So that should be reasonable as well. It just is mutable results that might not do what users expect.

glennpratt commented 11 months ago

Just seconding that this rule is inverted to my expectations as well.

Lazy evaluation feels like the more surprising mistake. Double quotes evaluating now is not surprising in shell.

3.3k disables https://github.com/search?q=SC2064&type=code

pmhahn commented 1 week ago

SC2064 contains a really good "bad" example under "Problematic code", which shows why you must take extra care when ignoring this rule and evaluating early instead of the recommended late:

trap "rm -fr '$testdir1' '$testdir2'" $TRAP_SIGNALS  # WRONG

Consider what happens if you define one variable as testdir1=1\ 2\"3\'4:

bash: error trap: line 5: unexpected EOF while looking for matching `''

Correct would be

trap "rm -rf ${testdir1@Q} ${testdir2@Q}" $TRAP_SIGNALS  # CORRECT

(I have done this wrong for years myself until today.)

One common case where I would like an exception for is using a temporary directory:

tmp="$(mktemp -d)"
trap "rm -rf ${tmp@Q}" EXIT
# more code using $tmp/
tmp=42  # reuse $tmp for something completely different

In that case I really want to delete the temporary directory I just created on exit, even when some later code does stupid things with the variable naming the directory.