Shellcheck: [SC2244] Countless false positives in countless scripts. (Regression)

[orbea]

For bugs

• Rule Id: SC2244
• My shellcheck version: online
• [x] The rule's wiki page does not already cover this (e.g. https://shellcheck.net/wiki/SC2086)
• [x] I tried on shellcheck.net and verified that this is still a problem on the latest commit

Here's a snippet or screenshot that shows the problem:

#!/bin/sh

foo=
if [ "${foo}" ]; then echo yea; fi

Here's what shellcheck currently says:

Line 4:
if [ "${foo}" ]; then echo yea; fi
     ^-- SC2244: Prefer explicit -n to check non-empty string (or use =/-ne to check boolean/integer).

Here's what I wanted or expected to see:

Nothing.

These warnings are garbage and the only thing they will accomplish is for experienced users to not use or contribute to shellcheck. With test -n is well defined as the default and this does not teach inexperienced users from writing better code, instead it misinforms them.

This is a regression after commit https://github.com/koalaman/shellcheck/commit/278ce56650b895a84a828966d0c2ed48189d313d.

[orbea]

One potential compromise is to only warn with unquoted strings, anyone that properly quotes these should hopefully know what they are doing. However shellcheck already warns in many such cases and it would still not be very useful...

[orbea]

From the wiki page:

If you are familiar with the semantics of [, you can ignore this suggestion with no ill effects.

Ignoring this suggestion is not a valid option, this is too prevalent to add ignore rules to every script and greatly reduces the usefulness of the online shellcheck.net when 90% of the warnings are erroneous. Its also not practical to instruct every downstream to revert this or for all users to change their config.

The only practical choices are for this to be reverted, greatly reduced in scope or for users to stop using shellcheck...

[ngzhian]

Thanks for the feedback, please note that this is not released yet, so it looks like you're building from master? That's great because we get quick feedback.
I'm thinking we can do some smarter variable flow here (like how we do it for SC2086). Or like you said we can just warn when unquoted strings are used.

[orbea]

I am using the online shellcheck.net which I understand uses the master.

[koalaman]

There's another option: autofixing! TTY output on master already makes the suggestion:

In - line 1:
if [ "${foo}" ]; then echo yea; fi
     ^------^ SC2244: Prefer explicit -n to check non-empty string (or use =/-ne to check boolean/integer).

Did you mean:
if [ -n "${foo}" ]; then echo yea; fi

ShellCheck.net does not yet have this ability, as the functionality is just a few weeks old. The intent is to be able to fix all these instances in a simple, safe and automatic way.

[orbea]

Personally I do not want to fix them in my scripts since they are they working as intended and I prefer the stylistic choice of using the default behavior for test. Autofixing could have other stylistic consequences such as breaking the 80-col limit.

[matthewpersico]

On a related note, where can we read about how autofix (I’m not using the word autocorrect because it sucks on phones) and do you think it will be 0.7.0 when we see it?

[koalaman]

I do believe that this check is generally useful as a default for the reasons highlighted in the wiki, so I'm leaving it in.

Being well defined is not the same as being simple or predictable: take [ 1 > 2 ] for example. I also don't see anything misinformative about this stylistic warning or its rationale.

It's true that it's likely to be annoying for anyone who's been scripting for years with no illusions about what [ "$var" ] does. I would encourage anyone to disable any suggestions they find unnecessarily hand-holdy with e.g. export SHELLCHECK_OPTS='-e SC2244', or -S info to disable all stylistic suggestions.

@matthewpersico It's still very early, but I pushed an initial front-end hookup to ShellCheck.net this morning so you can try it out on the few select issues (like quoting) that currently have fixes available. The intent is that the various editor plugins or CI systems will be able to offer to apply fixes according to their normal UI/workflow. I'll write some integration docs at some point, and there's an example implementation used by shellcheck.net that anyone's free to copy.

[orbea]

@koalaman Can you fix this please? This issue is unresolved and the choice of not using shellcheck is much more appealing than any other workarouds suggested so far.

[koalaman]

I'm sorry I wasn't clear. No, this won't be fixed. It's working as intended.

If there's anything I can do to improve your experience in this matter, such as adding a persistent exclude option to shellcheck.net, don't hesitate to ask. If the issue is purely that taking some minimal one-time responsibility for your own configuration is an absolute deal breaker, so be it.

[orbea]

The issue is that this is an obvious regression and I'm not going to jump through hoops for blatantly erroneous warnings,

If you can't see that you are misinforming people rather than helping them write better scripts than I'm not sure what else to say...