search

koalaman / shellcheck

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

If/Boolean as a potential beginner error #816

Open rugk opened 7 years ago

rugk commented 7 years ago

For new checks and feature suggestions

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

Many beginners may be familiar with other languages where you can just do if ( false ) { #code here not executed } and thus e.g. use a variable if ( $featureIsEnabled ) { #execute awesome feature } to conditionally do or not do things. These users may expect that the code is not executed when $featureIsEnabled is e.g. set to false or (in case of variable casting) to 0.

However Linux/POSIX shells seem to do it differently and threat everything as true (even a boolean "false") and only evaluate to false when nothing (an empty string) is given. The example below shows this in code form.


#!/bin/sh
#

vars="0 1 true false"

for var in $vars; do
    # echo "testing $var"
    if [ "$var" ]; then
        echo "var $var is true"
    fi
    if [ ! "$var" ]; then
        echo "var $var is false"
    fi
done

# test boolean: true
myBoolOption=true
if [ "$myBoolOption" ]; then
    echo "var <true> is true"
fi
if [ ! "$myBoolOption" ]; then
    echo "var <true> is false"
fi

# test boolean: false
myBoolOption=false
if [ "$myBoolOption" ]; then
    echo "var <true> is true"
fi
if [ ! "$myBoolOption" ]; then
    echo "var <true> is false"
fi

# test empty string
emptyString=""
if [ "$emptyString" ]; then
    echo "var <> is true"
fi
if [ ! "$emptyString" ]; then
    echo "var <> is false"
fi

Output:

$ ./test.sh
var 0 is true
var 1 is true
var true is true
var false is true
var <true> is true
var <true> is true
var <> is false

Shellcheck does not complain about this code at all.

So for beginners things such as var false is true can be a horrible error cause.

That's why I'd like to see a message from shellcheck in this cases as I think it could help many devs to prevent unnecessary debugging of such an issue, which is not apparent.

I don't know exactly when/how it should complain, but maybe it could show a warning for if [ "$myBoolOption" ]; then and e.g. suggest to use if [ "$myBoolOption"=true ]; then if it detects that $myBoolOption is a boolean or a 0/1 value or so.

kurahaupo commented 7 years ago

Variables don't have "types" so detecting when a variable should be considered a "boolean" is problematic; "false" is a perfectly good string value that has other uses.

Of course, the real problem is the inappropriate pervasive use of [ ... ] (the string-test command) in conjunction with if ... then when they are actually separate constructs and do not have to be used together.

As for the appropriate idiom, just writing

var=false
if $var ; then ...

works fine. As does

readonly true=1 false=  # necessary preamble, just once
...
var=false
if ((var)) ; then ...

And just for the record, your suggested fix would always result in "true", because "=true" is still a non-empty string.

On 5 January 2017 at 08:09, rugk notifications@github.com wrote:

For new checks and feature suggestions

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

Many beginners may be familiar with other languages where you can just do if ( false ) { #code here not executed } and thus e.g. use a variable if ( $featureIsEnabled ) { #execute awesome feature } to conditionally do or not do things. These users may expect that the code is not executed when $featureIsEnabled is e.g. set to false or (in case of variable casting) to 0.

However Linux/POSIX shells seem to do it differently and threat everything as true (even a boolean "false") and only evaluate to false when nothing (an empty string) is given. The example below shows this in code form.

!/bin/sh

vars="0 1 true false" for var in $vars; do

echo "testing $var"

if [ "$var" ]; then
    echo "var $var is true"
fi
if [ ! "$var" ]; then
    echo "var $var is false"
fidone

test boolean: true

myBoolOption=trueif [ "$myBoolOption" ]; then echo "var is true"fiif [ ! "$myBoolOption" ]; then echo "var is false"fi

test boolean: false

myBoolOption=falseif [ "$myBoolOption" ]; then echo "var is true"fiif [ ! "$myBoolOption" ]; then echo "var is false"fi

test empty string

emptyString=""if [ "$emptyString" ]; then echo "var <> is true"fiif [ ! "$emptyString" ]; then echo "var <> is false"fi

Output:

$ ./test.shvar 0 is truevar 1 is truevar true is truevar false is truevar is truevar is truevar <> is false

Shellcheck does not complain about this code at all.

So for beginners things such as var false is true can be a horrible error cause.

That's why I'd like to see a message from shellcheck in this cases as I think it could help many devs to prevent unnecessary debugging of such an issue, which is not apparent.

I don't know exactly when/how it should complain, but maybe it could show a warning for if [ "$myBoolOption" ]; then and e.g. suggest to use if [ "$myBoolOption"=true ]; then if it detects that $myBoolOption is a boolean or a 0/1 value or so.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/koalaman/shellcheck/issues/816, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMChXl61-Ryg6KzTK_hCUDrnXczw-xaks5rPAqJgaJpZM4LbEdH .

rugk commented 7 years ago

Nice, indeed a good idea to just use if $var ; then. Maybe suggest this instead.

cdavie-artium commented 3 months ago

I would also like some sort of shellcheck rule that would point this potential gotcha out. Suggesting if $var ; then would be good, or if [ "$var" = "true" ] ; then (or if [[ "$var" == "true" ]] ; then. (Depending on the context, sometimes I prefer the shorter version; other times I prefer to use the strings "true" and "false" but make it more obvious that they are just strings.

kurahaupo commented 3 months ago

@rugk Please don't write:

if $var ; then

It's not totally "wrong", but it has problems.

  1. it's not quoted, so
    • if the value of var contains space, tab, or newline, then it will be subject to word splitting;
    • after that, if it contains *, ?, or [...], then it will subject to filename globbing. The speed of the latter will depend on how many files are in the current directory; if you're in a network filesystem, that can take many seconds.
  2. It interprets the content of var as a command and runs it; this would be bad if it contained, say, rm -rf /*
  3. If you don't set a value, it will inherit whatever is in the environment, which could provided by an attacker (see point 3).

Please use this instead:

   if (( var )) ; then

(It's not perfect, but it's much faster, and (in newer shells) more secure; it's certainly no worse, even in older shells.)

rugk commented 3 months ago

Is that POSIX compatible though? Anyway one needs to come up with a proper suggestion of what is actually the best practise/recommend way and then a shellcheck rule could be implemented.