koalaman / shellcheck

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

[SC2116] exception: $(echo $var) can be used to expand glob patterns in $var #807

Open devernay opened 7 years ago

devernay commented 7 years ago

For new checks and feature suggestions

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

The following is used to axpand glob patterns present in a given var (here, LIBADD):


        LIBADD="$(echo $LIBADD)"
                ^-- SC2116: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'.
                       ^-- SC2086: Double quote to prevent globbing and word splitting.

Does that make it an exception to SC2116, or is there an alternative?

kurahaupo commented 7 years ago

On Fri, 23 Dec 2016, Frédéric Devernay wrote:

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

The following is used to axpand glob patterns present in a given var (here, LIBADD):


        LIBADD="$(echo $LIBADD)"
                ^-- SC2116: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'.
                       ^-- SC2086: Double quote to prevent globbing and word splitting.

Does that make it an exception to SC2116, or is there an alternative?

That looks suspiciously like you're later on going to expand $LIBADD without enclosing quotes, because you want the names generated by the wildcard to be separate parameters.

So the appropriate alternative would be to use an array, like:

LIBADD=( $LIBPATTERN )

or even better, avoid having the wildcard in a variable entirely:

LIBADD=( "$LIBADD_PREFIX"*"$LIBADD_SUFFIX" )

Then either way, "${LIBADD[@]}" will correctly expand the names matched by the wildcard as separate parameters.

Also for preference, use lower-case variable names to avoid conflicting with shell built-in and OS-provided variables.

devernay commented 7 years ago

Actually, the original code was:

LIBADD=`echo $LIBADD`

and it was a Bourne shell (I was in the middle of converting it to bash). Do you agree that in this case (Bourne shell construct), the echo is not useless, contrary to what shellcheck suggests, and this is an exception to SC2116? OK, you can write it in lowercase if you prefer, but frankly this is just a matter of taste: I use uppercase for whatever is exported, and I don't think this is bad practice.

devernay commented 7 years ago

I forgot to mention that since then I have already switched to using arrays as you suggest, but arrays are not Bourne shell. And I forgot to thank all the shellcheck developers for that wonderful tool.

andlrc commented 7 years ago

Well echo $LIBADD is not the same as echo "$LIBADD".

Then a=$(echo $b) is different from a=$b, as the first will undergo word splitting and pathname expansion.

Considering that there are so many different implementations of how echo should interpreted flags it can become a huge mess, i.e, consider this:

$ a='-e \n'
$ echo $a

$ echo "$a"
-e \n

So if anything, shellcheck should warn about unquoted variable expansion. One could consider warning about a useless use of echo if the variable is quoted.

But this would potential still give different results i.e:

$ a=-e
$ echo "$a" 

$ { NOTHING BUT THE NEWLINE IS PRINTED IN BASH }
koalaman commented 7 years ago

This is indeed an interesting case. It's not a useless use of echo, but more similar to concatenation of arrays (like var=${array[@]}).

I don't have any great ideas for making shellcheck more helpful in this case, but I started by mentioning this case on the wiki page for this warning.

domenic commented 7 years ago

Note: I tried the supposed better code at https://github.com/koalaman/shellcheck/issues/807#issuecomment-268979570, i.e.

#!/bin/bash
array=( $FOO )
echo "${array[@]}"

and shellcheck gave me SC2206

mloskot commented 2 years ago

Another useful case for $(echo $var) is trimming by taking advantage of echo ignoring spaces and collapsing multiple spaces into a single one.

kurahaupo commented 2 years ago

Any useful linting tool will necessarily err on the side of false positives, so it's entirely normal and appropriate to put "ignore" directives in the code to silence excessive warnings.

Rather than worry when one sees such directives in a script, one should feel reassured that someone has thought through the edge cases that might apply and concluded that the code is still safe. It helps if one also adds an explanation and signature:

# shellcheck ignore=SC2206 -- globbing intended; var cannot contain spaces - Kurahaupo 20220730
mloskot commented 2 years ago

Actually, the $(echo $var) case is already documented with ignoring recommendation in https://github.com/koalaman/shellcheck/wiki/Ignore#ignoring-one-specific-instance-in-a-file

image