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

syntax: breaks some substring expansions #982

Closed CS-Account closed 1 year ago

CS-Account commented 1 year ago

Summary of issue

In the case of bash shell shell parameter expansion, substring expansion containing a parameter, offset and length is incorrectly formatted, losing syntactically important whitespace in the process, and making bash incorrectly interpret it as ‘:-’ expansion. Further information regarding this process can be found in the official Bash manual under the "3.5.3 Shell Parameter Expansion (linked here)" heading.

You can ctrl-f to [‘:-’ expansion.] (brackets excluded) to find the relevant section of the manual.

Relevant Information

Examples

Below are some reproducible examples with a few variations tested.

Variations where formatting does not work correctly

Before:

test_var="${test_var:0: -2}"
test_var=${test_var:0: -2}
test_var="${test_var: -2: -1}"
test_var=${test_var: -2: -1}
test_var="${test_var: -2: 1}"
test_var=${test_var: -2: 1}
test_var="${test_var: -2}"
test_var=${test_var: -2}

After:

test_var="${test_var:0:-2}"
test_var=${test_var:0:-2}
test_var="${test_var: -2:-1}"
test_var=${test_var: -2:-1}
test_var="${test_var: -2:1}"
test_var=${test_var: -2:1}

Variations where formatting does work correctly

Before:

test_var="${test_var: -2}"
test_var=${test_var

After:

test_var="${test_var: -2}"
test_var=${test_var: -2}

If there is more information you need to reproduce / get context about this issue please ask.

Sitenote

Unless I am missing it, I don't notice any functionality to just ignore specific lines. This would be a really useful feature for times like this where a bug is currently out and there isn't another way to deal with it.

Edit: After searching through the codebase, issues and discussions for a while, I believe there is some hesitancy towards adding this feature, so I just wanted to be another person to voice support for such a feature. No matter how strict, opinionated or well tuned a formatter is, languages change, bugs show up, etc. Not including a method to offer temporary relief for such an issue leaves very few solutions as the only solutions I can think of are to either temporarily go without formatting, change your code to the whims of a formatter even if syntactically and logically correct, or alternatively, wrapping shfmt in a script, with that script implementing a self built line ignore feature which is more complexity than if it was natively built in.

mvdan commented 1 year ago

Perhaps I'm missing something, but I don't think shfmt is currently breaking parameter expansions. We avoid turning the first : - into :-, because that would change the meaning, as you point out. In the case of the second colon, we can remove the whitespace as that doesn't change anything.

For example:

$ cat f.bash
foo=bar
echo ${foo: 1: -1}

foo=bar
echo ${foo: -1: 1}
$ bash f.bash
a
r
$ shfmt f.bash > f2.bash
$ cat f2.bash
foo=bar
echo ${foo:1:-1}

foo=bar
echo ${foo: -1:1}
$ bash f2.bash
a
r

So, as far as I can tell, there isn't a bug here.

mvdan commented 1 year ago

Closing per the above for now.