anordal / shellharden

The corrective bash syntax highlighter
Mozilla Public License 2.0
4.63k stars 130 forks source link

Quoting not needed inside double brackets #8

Closed xyproto closed 6 years ago

xyproto commented 6 years ago

Quoting is generally not needed when variables are used between double brackets ([[ and ]]).

Here is a test case that does not expand:

x='*'; if [[ $x == '*' ]]; then echo not expanded; fi

While with single brackets, this is the result:

x='*'; if [ $x == '*' ]; then echo not expanded; fi
bash: [: too many arguments

shellharden wants to add double quotes around $x in the first case, but this is not really needed.

anordal commented 6 years ago

I'm torn on this.

Reasons for not sanctioning unquoted stuff in double brackets:

But safety is not everything. The usage is legit, and supporting existing scripts as much as possible is a goal. I've done similar things, like accepting certain unquoted variables of numeric content ($? and $#) and unquoted variables in arithmetic expansions.

I guess it will have to be supported eventually, but it will take some work, and is low on my list.

xyproto commented 6 years ago

Just an observation:

The argument for using quotes consistently, also when they are not needed within double brackets, is the same type of argument that is presented for using curly brackets consistently, also when they are not needed.

alok commented 6 years ago

I would argue that pointless braces around variables are really noisy, since scripts are full of variables, whereas conditionals are a small part of the overall script.

(un)quoting correctly is an admirable goal, but I suspect this is one of those rules that's just hard to remember when it applies, sort of like how most people do not really bother to learn the difference between "${a[*]}" and "${a[@]}". It's probably fine to just leave the quoting here to avoid confusing people.

One integration that may be useful is a heuristic for when a script relies on word splitting: If quoting causes a loop to run only once (shellcheck has an error code for this and it could probably be parsed by shellharden), then it probably relies on word splitting, and shellharden should issue a warning.

xyproto commented 6 years ago

:+1:

anordal commented 6 years ago

It's probably fine to just leave the quoting here to avoid confusing people.

Indeed, that's the approach taken so far, but accepting good scripts is also a virtue. I don't think Shellharden will ever remove quotes (I see no reason to do that).