shellcheck icon indicating copy to clipboard operation
shellcheck copied to clipboard

Feature: When using "set -u" and testing if a variable is set, don't forget "${:-}"

Open sbr-nrubinstein opened this issue 8 years ago • 4 comments

For new checks and feature suggestions

  • [x] shellcheck.net (i.e. the latest commit) currently gives no useful warnings about this
  • [x] I searched through https://github.com/koalaman/shellcheck/issues and didn't find anything related

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


#!/bin/bash
set -u

if [ -z "$FOOBAR" ]; then
  echo "FOOBAR is empty"
fi


#!/bin/bash
set -u

if [ -n "$FOOBAR" ]; then
  echo "FOOBAR is not empty"
fi

If "-n" or "-z" is used, the developer probably expects that the variable may be unset. But because "set -u" will abort the program, the test is ineffective.

Here's what shellcheck currently says:

Nothing

Here's what I wanted or expected to see:

SC....: If $FOOBAR is unset, this test will abort the script. Use "${FOOBAR:-}" instead.

sbr-nrubinstein avatar Nov 15 '17 14:11 sbr-nrubinstein

Caveat: it is possible that the programmer knows that the variable is set, but still wants to know if it is empty. In this case, using "${FOOBAR:-}" doesn't hurt, but forfeits the error checking of "set -u"

sbr-nrubinstein avatar Nov 15 '17 14:11 sbr-nrubinstein

If this change were implemented, I'd have to add a bunch more # shellcheck disable lines in my scripts.

I think it's relatively common to have a variable that should be assigned to something, but you want to test whether or not it's the empty string - for example, if the variable was previously assigned to the output of some command which may or may not have returned any output. In this case, as you mention, using ${foo:-} gives up the error checking.

The whole point of set -u is to abort the script if a variable is undefined, so it seems strange to warn the programmer that this command will work as intended. Given the possibility of variables being defined but empty, the -z and -n tests are no different from any other place in the script where a variable could appear.

automorphism88 avatar Nov 16 '17 03:11 automorphism88

If you want to know if a variable is empty, then test for it explicitly.

#!/bin/sh

set -u

case "${foo:-}" in
  '') echo "Empty variable." ;;
  *) echo "Set variable." ;;
esac

My understanding set -u should exit when it encounters an unset variable so the script's maintainer can fix it with the goal of there being no unset variables.

orbea avatar Nov 19 '17 00:11 orbea

Old issue, but just became current for me.

A script that was working in a Home Assistant add-on for users not defining "DISPLAY" just broke for them, probably because the Home Assistant Team (member) decided to add more security by requiring that all variables are defined.

So before, the "set +u" was the default and "echo "DISPLAY:'$DISPLAY'" worked even if DISPLAY was not set, now this fails the script.

DISPLAY is conditionnaly set depending on a configuration parameter set by the user.

I checked how to enable checking this in shellscript. I even tried adding 'set -u' to the script explicitally.

So I think that this would be a good check to (optionnaly) add - and automatically add or disable when set -u/set +u is detected.

mdeweerd avatar Nov 02 '23 20:11 mdeweerd