shellcheck icon indicating copy to clipboard operation
shellcheck copied to clipboard

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

Open devernay opened this issue 8 years ago • 9 comments

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:

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?

devernay avatar Dec 23 '16 08:12 devernay

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.

kurahaupo avatar Dec 23 '16 11:12 kurahaupo

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 avatar Dec 23 '16 15:12 devernay

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.

devernay avatar Dec 23 '16 15:12 devernay

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 }

andlrc avatar Dec 23 '16 19:12 andlrc

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.

koalaman avatar Dec 26 '16 22:12 koalaman

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

domenic avatar Aug 29 '17 05:08 domenic

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

mloskot avatar Jul 21 '22 21:07 mloskot

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

kurahaupo avatar Jul 30 '22 03:07 kurahaupo

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

mloskot avatar Jul 30 '22 12:07 mloskot