search

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.99k stars 333 forks source link

cmd/shfmt: shfmt can change the interpreter by removing a leading newline #936

Closed MaeIsBad closed 1 year ago

MaeIsBad commented 1 year ago

A file like:


#!/usr/bin/env bash
echo $SHELL

will be executed in the current shell, due to the leading newline. Running shfmt on it will discard the newline, leading to the interpreter being changed to bash.

I've tested this on the latest master commit(6ba49e2c622e3f56330f4de6238a390f395db2d8)

theclapp commented 1 year ago

For those of you reading along at home, the issue is clearer at Github. The file in question is

--- start ---
\n
#!/usr/bin/env bash\n
echo $SHELL\n
--- end ---

@MaeIsBad I'm not @mvdan, and I'm not saying this isn't a bug. Is this something you just happened to notice, or is this an idiom you actually use? If the latter, I'd be interested to hear why. It seems like it'd be rife for bugs or at least confusion.

mvdan commented 1 year ago

Yeah, I'm a bit puzzled by this idiom as well. Many editors will complain or remove trailing empty lines in a file, for example, so I wouldn't be surprised if a leading empty line was similarly fragile - it seems rather easy to remove by accident.

This is technically a bug in the formatter, because it is changing the meaning of the script - it used to not have a shebang, and now it does. However, should we also leave a shebang with a leading space untouched? All of these whitespace edge cases feel a tad out of place for a properly formatted shell script.

Perhaps you could consider an alternative like turning the first line into a comment, which could also explain why you don't want the script to have an actual shebang.

MaeIsBad commented 1 year ago

The issue I had was that another script was generating files with a leading newline, which depending on other factors only sometimes got formatted with shfmt, which led to me missing the buggy behavior when shfmt wasn't being executed.

That's a very specific edge case that's not very likely to occur, so it's understandable if you'd like to close this issue as won't fix

mvdan commented 1 year ago

Thanks for the extra context. We seem to be in agreement that noone would keep a leading empty line in a shell file on purpose, so I'm going to close this issue as "not a supported format" for now. Happy to reconsider that in the future if someone brings forward an argument for its support.