cmd/shfmt: change -ln default to "auto"

[mvdan]

This way, when one does shfmt -l -w ., files with #!/bin/sh shebangs would be formatted as if -ln posix had been specified. The tool already has to parse shebangs when detecting if extension-less files are actually shell scripts, so this wouldn't be extra overhead.

The logic for -ln=auto is as follows:

• If an input file has a shell shebang, we parse+format it accordingly (posix/bash/mksh)
• Otherwise, we fall back to the current default, bash

The big advantage is that this will flag shell scripts using #!/bin/sh and bashisms at the same time, which is a bug. A *.sh script without a shebang that also uses bashisms won't be caught, but I think doing that would be far too prone to false positives; calling such a script like bash foo.sh is common, and not all that bad. The logic would ignore filename extensions altogether.

On the flip side, the -ln flag will become a bit more complex. I think I can explain the new logic with an extra line or two in the usage text.

Thoughts?

[mvdan]

An alternative approach is to make -ln=auto do the same as -ln=bash when formatting stdin or any files directly (without directory walking). This would keep the simple uses of shfmt the way they are, and a bit simpler, but it would make the overall tool more inconsistent.

[kaey]

I don't think you will ever find a project which mixes posix/bash scripts together.
If there is even a single bash script, chances are all scripts, even if they set /bin/sh, are only tested in bash and will not work with other shells anyway (due to small incompatibilities, unintended use of extensions or simply forgot to set /bin/bash).

Libraries (which are included with source) never include shebang and parsing them differently to other files will lead to inconsistencies.

The big advantage is that this will flag shell scripts using #!/bin/sh and bashisms at the same time, which is a bug.

It should be possible to do without implementing auto.

TLDR: choice of a shell is a project wide decision and implementing this flag will bring more harm than good.

[mvdan]

I disagree that projects stick to one shell. At work we have repos that mix bash and posix. Bash for development scripts that are simpler to write with bashisms, and posix for scripts that need to run inside deployed containers which only include busybox.

are only tested in bash ... or simply forgot to set /bin/bash

In that case I think they should be forced to change the shebang to bash, and I think that's a good thing. They might be mildly annoyed, but shfmt v3.0 will bring other small incompatible change that might annoy them anyway.

never include shebang and parsing them differently to other files will lead to inconsistencies.

They'll default to bash, which will parse pretty much any shell script out there; I'm not sure that this would be a problem in practice. I get your point about the inconsistency, but if they want explicit consistency, they can use shebangs or -ln=<whatever> for the entire project.

It should be possible to do without implementing auto.

I'm not convinced; see my example above about our work repos. How would we catch those? Using shfmt -ln=posix for the entire repo won't work, as some scripts are Bash. And we can't make the default be posix instead of bash, because that's just not the world we live in unfortunately.

The only alternative for us to catch those errors is to manually implement this shell language detection logic ourselves, or to use a separate tool to "detect incorrect bashisms". Both seem like worse options to me.

[mvdan]

Another option is to add a new "lint" tool in this repo that would catch this error. But of course, practically noone would run this tool, and existing shfmt users wouldn't benefit at all.

[kaey]

Actually shfmt -w . doesn't format files without shebangs at all (for example APKBUILD in alpine)

Another option is to add a new "lint" tool

This actually makes a lot of sense, especially if you port some good checks from https://github.com/koalaman/shellcheck
I think shfmt shouldn't have lang flag at all and make it a job for a linter.

[mvdan]

shfmt does need a language flag, because the parser has the option. Parsing everything as Bash wouldn't be enough, because some of Bash's features are backwards-incompatible with POSIX.

I am aware of shellcheck, but it implements a lot of checks, and I'm just too lazy to implement something as complex as that :)

[mvdan]

It just occurred to me that this has a huge overlap with #393. At the very least, I shouldn't add -ln=auto until I've figured out language support as part of editorconfig files. So I'm closing this for now.

[mvdan]

I've resurrected this idea in #622, by the way.