mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
6.98k stars 332 forks source link

Bash compound conditional expressions require spacing between the outer brackets and the inner expression #1025

Closed TyIsI closed 10 months ago

TyIsI commented 10 months ago

The following example generates an error: ./test.sh: line 5: [[-f: command not found

#!/bin/bash

LOCKFILE=test.lock

if [-f "${LOCKFILE}"]; then
    echo "locked"
fi

This fails on both double and single square bracket notation.

The correct syntax would be:

#!/bin/bash

LOCKFILE=test.lock

if [ -f "${LOCKFILE}" ]; then
    echo "locked"
fi

I went over all the documentation and issues and I did not find anything relating to this. And I wasn't sure if this was unintended or by design. Apologies if I missed anything.

mvdan commented 10 months ago

Yes, your original snippet is not valid shell, since [ is not a special token like [[ is.

bash: [-f: command not found

What do you mean by this issue? As far as I can tell, there's no bug in our parser, as it follows the spec.

TyIsI commented 10 months ago

From the bash man page1.

In the Conditional Expressions section it states:

Conditional expressions are used by the [[ compound command and the test and [ builtin commands to test file attributes and perform string and arithmetic comparisons.

Then in the Shell Builtin Commands section:

test expr [ expr ] Return a status of 0 or 1 depending on the evaluation of the conditional expression expr. Each operator and operand must be a separate argument. Expressions are composed of the primaries described above under CONDITIONAL EXPRESSIONS. test does not accept any options, nor does it accept and ignore an argument of -- as signifying the end of options. Expressions may be combined using the following operators, listed in decreasing order of precedence. The evaluation depends on the number of arguments; see below. ...

In the Compound Commands section they describe it as:

[[ expression ]] Return a status of 0 or 1 depending on the evaluation of the conditional expression expression. Expressions are composed of the primaries described below under CONDITIONAL EXPRESSIONS. Word splitting and pathname expansion are not performed on the words between the [[ and ]]; tilde expansion, parameter and variable expansion, arithmetic expansion, command substitution, process substitution, and quote removal are performed. Conditional operators such as -f must be unquoted to be recognized as primaries.

This seems to line up with testing various shells:

================================================================================
Test:                           no-space-double-bracket.sh
Expression:                     [[-f "${LOCKFILE}"]]
================================================================================
no-space-double-bracket.sh      ash     ERROR: [[-f: not found
no-space-double-bracket.sh      bash    ERROR: [[-f: command not found
no-space-double-bracket.sh      dash    ERROR: [[-f: not found
no-space-double-bracket.sh      ksh     ERROR: not found
no-space-double-bracket.sh      zsh     ERROR: bad pattern: [[-f
================================================================================
Test:                           no-space-single-bracket.sh
Expression:                     [-f "${LOCKFILE}"]
================================================================================
no-space-single-bracket.sh      ash     ERROR: [-f: not found
no-space-single-bracket.sh      bash    ERROR: [-f: command not found
no-space-single-bracket.sh      dash    ERROR: [-f: not found
no-space-single-bracket.sh      ksh     ERROR: not found
no-space-single-bracket.sh      zsh     ERROR: bad pattern: [-f
================================================================================
Test:                           with-space-double-bracket.sh
Expression:                     [[ -f "${LOCKFILE}" ]]
================================================================================
with-space-double-bracket.sh    ash     ERROR: [[: not found
with-space-double-bracket.sh    bash    OK
with-space-double-bracket.sh    dash    ERROR: [[: not found
with-space-double-bracket.sh    ksh     OK
with-space-double-bracket.sh    zsh     OK
================================================================================
Test:                           with-space-single-bracket.sh
Expression:                     [ -f "${LOCKFILE}" ]
================================================================================
with-space-single-bracket.sh    ash     OK
with-space-single-bracket.sh    bash    OK
with-space-single-bracket.sh    dash    OK
with-space-single-bracket.sh    ksh     OK
with-space-single-bracket.sh    zsh     OK
================================================================================

The current posix standard2 states the following:

2.9.4 Compound Commands
The shell has several programming constructs that are "compound commands", which provide control flow for commands. Each of these compound commands has a reserved word or control operator at the beginning, and a corresponding terminator reserved word or operator at the end. In addition, each can be followed by redirections on the same line as the terminator. Each redirection shall apply to all the commands within the compound command that do not explicitly override that redirection.

Grouping Commands
The format for grouping commands is as follows:

( compound-list )
Execute compound-list in a subshell environment; see [Shell Execution Environment](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_12). Variable assignments and built-in commands that affect the environment shall not remain in effect after the list finishes.
If a character sequence beginning with "((" would be parsed by the shell as an arithmetic expansion if preceded by a '$', shells which implement an extension whereby "((expression))" is evaluated as an arithmetic expression may treat the "((" as introducing as an arithmetic evaluation instead of a grouping command. A conforming application shall ensure that it separates the two leading '(' characters with white space to prevent the shell from performing an arithmetic evaluation.

{ compound-list ; }
Execute compound-list in the current process environment. The semicolon shown here is an example of a control operator delimiting the } reserved word. Other delimiters are possible, as shown in [Shell Grammar](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_10); a <newline> is frequently used.
Exit Status
The exit status of a grouping command shall be the exit status of compound-list.

...

The if Conditional Construct
The if command shall execute a compound-list and use its exit status to determine whether to execute another compound-list.

The format for the if construct is as follows:

if compound-list
then
    compound-list
[elif compound-list
then
    compound-list] ...
[else
    compound-list]
fi

The if compound-list shall be executed; if its exit status is zero, the then compound-list shall be executed and the command shall complete. Otherwise, each elif compound-list shall be executed, in turn, and if its exit status is zero, the then compound-list shall be executed and the command shall complete. Otherwise, the else compound-list shall be executed.

This leads me to believe that the correct way of writing a conditional is including the whitespace separators within the wrapping brackets.

mvdan commented 10 months ago

This leads me to believe that the correct way of writing a conditional is including the whitespace separators within the wrapping brackets.

Correct, and shfmt will work if you include the space. If you don't include the space, like in [-f, it will correctly fail.

I feel like I'm missing something here, we seem to be in agreement, and I still don't understand what this issue is about. Do you think there is a bug?

TyIsI commented 10 months ago

Aha! Yes, as a formatter, I expected the formatter to pick up on the lack of whitespace and correct that.

It was the assumption that the formatter would format this.

TyIsI commented 10 months ago

Or is this the awkward overlap between linter and formatter?

mvdan commented 10 months ago

shfmt is a formatter, to make the style and indentation more consistent - its job is not to rewrite the contents of your code to try to fix bugs :) Perhaps a linter would do that, although linters can't fix most bugs automatically either.

Note that this project has the syntax parser and printer as separate APIs, so you could fairly easily write any tool that you fancy on top of them.

TyIsI commented 10 months ago

Okay. My bad. Thanks for your time!