shellcheck, and Reviewing Shell Code

Part I · Module 21 · Chapter 21.7 · shellcheck, and Reviewing Shell Code

Twenty-one modules of defects, and look at what they have in common: an unquoted $f, for f in $(ls), local v=$(cmd), cmd | while read, rm -rf "$dir/", read without -r, ${a[@]} without quotes. Every one is visible in the source. None of them needs the script to run.

That is the definition of something a machine should check.

1. A Note on This Chapter

As in Chapter 19.6, the honest thing is to say what could and could not be run.

$ command -v shellcheck
$ echo "rc=$?"
rc=1

shellcheck is not installed on this machine, and installing it is outside the scope of this work. Nor is bats or shunit2, which Chapter 21.8 will come to.

So this chapter splits:

• Section 2 is derived from shellcheck's documentation, not from running it. The behaviour described is well documented and stable, and you should treat it as reference rather than as something this course verified.
• Sections 3 to 6 are measured. They include a small checker written for this chapter, every one of whose rules is demonstrated firing on a bad specimen and staying quiet on a clean one.

That split is not a consolation prize. Writing the checker turned out to teach something shellcheck would have hidden, which §4 is about.

2. What shellcheck Does — Reference

shellcheck is a static analyser for shell scripts. You point it at a file and it reports defects with a code, a line, and an explanation:

$ shellcheck bad.sh
In bad.sh line 2:
cat $file
    ^---^ SC2086: Double quote to prevent globbing and word splitting.

In bad.sh line 3:
for f in $(ls); do echo "$f"; done
         ^---^ SC2045: Iterating over ls output is fragile. Use globs.

The codes are stable and searchable, which is the main practical benefit — SC2086 is a URL you can paste into a review comment. A sample of the ones that correspond directly to chapters of this course:

Three things about using it that are worth knowing regardless of whether you can run it today.

Run it on everything, in CI. It is fast, it needs no configuration, and its default findings are nearly all real. A shellcheck ./*.sh step in a pipeline is the cheapest quality gate shell code can have.

Silence a finding at the line, with a reason. When a finding is genuinely wrong for your case:

# shellcheck disable=SC2086  # deliberate: $seeds must split into words
for s in $seeds; do ...

The comment applies to the next line only. A blanket disable at the top of a file is how a checker becomes decoration — and the reason matters more than the code, because the next reader needs to know it was a decision.

And # shellcheck shell=bash at the top tells it which dialect to assume when the shebang is absent or the file is meant to be sourced — which matters given Chapter 21.6 §2's point that a sourced file's shebang means nothing.

3. A Substitute, and Why One Is Worth Writing

Since the real tool is unavailable here, this chapter builds a small one. Eight rules, each corresponding to a defect this course measured:

#!/usr/bin/env bash
# shlint — a small static checker for the defects this course taught.
# NOT a shellcheck replacement; a demonstration of what one does.
#
# usage: shlint <script>...
# exit: 0 clean · 1 findings · 2 bad usage

set -o nounset

[ $# -ge 1 ] || { echo "usage: ${0##*/} <script>..." >&2; exit 2; }

findings=0
report() { printf '%s:%s: %s: %s\n' "$1" "$2" "$3" "$4"; findings=$(( findings + 1 )); }

for f in "$@"; do
  # NOTE: this line has a deliberate bug — see §4's callout
  [ -r "$f" ] || { printf '%s: cannot read\n' "$f" >&2; continue; }
  n=0
  while IFS= read -r line || [ -n "$line" ]; do
    n=$(( n + 1 ))
    case $line in \#*) continue ;; esac
    s=${line%%#*}

    # L001 — an unquoted variable passed to a command (9.2, 19.3)
    printf '%s' "$s" | grep -Eq \
      '(^|[[:space:]])(rm|cp|mv|cat|wc|grep|mkdir|chmod|chown|tar)([[:space:]]+-[^[:space:]]+)*[[:space:]]+\$[A-Za-z_]' \
      && report "$f" "$n" L001 'unquoted variable passed to a command'

    # L002 — for over $(ls) (17.2)
    printf '%s' "$s" | grep -Eq 'for[[:space:]]+[A-Za-z_]+[[:space:]]+in[[:space:]]+\$\(ls' \
      && report "$f" "$n" L002 'for over $(ls) — use a glob'

    # L003 — local/declare with a substitution masks the status (18.4, 20.2)
    printf '%s' "$s" | grep -Eq '(local|declare|export|readonly)[[:space:]]+[A-Za-z_][A-Za-z0-9_]*=\$\(' \
      && report "$f" "$n" L003 'local/declare with $( ) discards the status'

    # L004 — rm on a variable path with no guard (20.4)
    printf '%s' "$s" | grep -Eq 'rm[[:space:]]+(-[a-zA-Z]+[[:space:]]+)*"?\$[A-Za-z_][A-Za-z0-9_]*"?/' \
      && ! printf '%s' "$s" | grep -q ':?' \
      && report "$f" "$n" L004 'rm on a variable path with no ${var:?} guard'

    # L005 — an array expanded unquoted (19.3)
    printf '%s' "$s" | grep -Eq '[^"]\$\{[A-Za-z_][A-Za-z0-9_]*\[[@*]\]\}' \
      && report "$f" "$n" L005 'array expanded unquoted — use "${a[@]}"'

    # L006 — a pipe into a while-read loop (17.7)
    printf '%s' "$s" | grep -Eq '\|[[:space:]]*while[[:space:]]+(IFS=[^[:space:]]*[[:space:]]+)?read' \
      && report "$f" "$n" L006 'pipe into while read — variables will not survive'

    # L007 — read without -r (17.5)
    printf '%s' "$s" | grep -Eq '(^|[[:space:]|])read[[:space:]]+(-[^r[:space:]]+[[:space:]]+)*[A-Za-z_]' \
      && ! printf '%s' "$s" | grep -Eq 'read[[:space:]]+(-[a-zA-Z]*r[a-zA-Z]*[[:space:]])' \
      && report "$f" "$n" L007 'read without -r'

    # L008 — eval (19.5)
    printf '%s' "$s" | grep -Eq '(^|[[:space:]])eval([[:space:]]|$)' \
      && report "$f" "$n" L008 'eval — build an array instead'
  done < "$f"
done

printf -- '-- %d finding(s)\n' "$findings"
[ "$findings" -eq 0 ]

A specimen with exactly one defect per rule:

#!/usr/bin/env bash
cat $file
for f in $(ls); do echo "$f"; done
g() { local v=$(wc -l < x); }
rm -rf "$dir"/*
printf '%s\n' ${a[@]}
cat x | while read -r l; do n=1; done
while read l; do :; done < y
eval "$cmd"

$ ./shlint bad.sh
bad.sh:2: L001: unquoted variable passed to a command
bad.sh:3: L002: for over $(ls) — use a glob
bad.sh:4: L003: local/declare with $( ) discards the status
bad.sh:5: L004: rm on a variable path with no ${var:?} guard
bad.sh:6: L005: array expanded unquoted — use "${a[@]}"
bad.sh:7: L006: pipe into while read — variables will not survive
bad.sh:8: L007: read without -r
bad.sh:9: L008: eval — build an array instead
-- 8 finding(s)
$ echo $?
1

Eight rules, eight findings, one per line of the specimen. And the corrected version of the same script:

#!/usr/bin/env bash
set -euo pipefail
cat -- "$file"
for f in ./*.log; do echo "$f"; done
g() { local v; v=$(wc -l < x); }
rm -rf -- "${dir:?}"/*
printf '%s\n' "${a[@]}"
while IFS= read -r l; do n=1; done < <(cat x)
while IFS= read -r l; do :; done < y
opts=(-o out); tool "${opts[@]}"

$ ./shlint good.sh
-- 0 finding(s)
$ echo $?
0

Zero findings on the fixed version, which is the other half of the proof — a checker that fires on everything is as useless as one that fires on nothing.

4. A Check That Cannot Fail Is Worse Than No Check

This is what writing the tool taught, and it is the part worth keeping.

L001's first version was this:

printf '%s' "$s" | grep -Eq \
  '(^|[[:space:]])(rm|cp|mv|cat|wc|grep)[[:space:]]+[^|]*[^"'"'"'$]\$[A-Za-z_][A-Za-z0-9_]*([[:space:]]|$)'

Plausible, carefully constructed, and on the first run against bad.sh the output was:

bad.sh:3: L002: for over $(ls) — use a glob
...
-- 7 finding(s)

Seven findings, not eight. L001 did not fire on cat $file — the regex required a character before the $ that was neither a quote nor a $, and the space in cat satisfied a different part of the pattern than intended. The rule existed, was documented, ran on every line, and could never match anything.

A rule like that is worse than not having it, because it produces confidence. A clean run tells you the file has no unquoted expansions, and the reason it is clean is that nothing is looking.

The fix was a simpler pattern:

'(^|[[:space:]])(rm|cp|mv|cat|wc|grep|mkdir|chmod|chown|tar)([[:space:]]+-[^[:space:]]+)*[[:space:]]+\$[A-Za-z_]'

And the method, which generalises past linters:

Every check needs a specimen that makes it fail. Not "I believe this would catch X" — an actual input, kept next to the checker, that produces the finding. bad.sh above is that specimen set: one line per rule, and a run that does not report all eight is a broken checker rather than a clean file.

That is the same discipline Chapter 21.8 applies to tests, and it is why both chapters build their specimens before trusting their tools.

5. The Limits of Any Static Checker

Worth being clear about, because a green shellcheck run is not a correct script.

It is a reader, not a prover. shellcheck cannot know that $dir is always non-empty because you validated it forty lines earlier, so SC2115 fires anyway; and it cannot know that your $seeds is a list of integers that should split, so SC2086 fires on the one line where unquoted is right (Chapter 17.1 §3). Both are correct findings about the code in isolation, and both need a disable comment with a reason.

It cannot see values. Every defect in Chapter 20.1's catalogue that was not a syntax matter — an empty variable, an unmatched glob, a wrong path — is invisible to it. That is the half Chapter 21.8's tests cover.

It cannot see across processes. A script that calls another script with the wrong arguments, a library whose contract changed, a tool that exits 0 after a fatal error — none of that is in the file being checked.

And it has no opinion about your interface. Everything in Chapters 21.1 to 21.5 — a usage message, an exit-code contract, idempotence, a dry run — is invisible to a static checker and is most of what makes a script reusable.

So the honest ordering of effort is: bash -n and a checker catch the mechanical defects for free; tests catch the behavioural ones; and a human reviewer catches the interface ones. Each layer sees what the others cannot.

6. Reviewing Shell Code by Hand

Which brings us to the part no tool does. A checklist that has earned its place over this course, in rough order of how often it finds something:

Quoting and lists

• Is every expansion quoted except the ones that must split? (9.2, 19.3)
• Is every array expanded as "${a[@]}"? (19.3)
• Does anything iterate $(ls) or $(find)? (17.2, 19.4)
• Is a command line being built in a string? (19.5)

Failure handling

• set -euo pipefail at the top, and not in a sourced library? (20.2, 18.6)
• Does any function called in a condition rely on set -e? (20.2 §4)
• Is local x=$(cmd) split into two lines? (18.4 §5)
• Does any pipeline have a consumer that exits early? (20.2 §5)

Destruction and state

• Is every rm on a variable path guarded with ${var:?} and --? (20.4)
• Is there a trap … EXIT for anything created, registered on the next line? (20.5)
• Does the interrupt handler re-raise? (20.5 §5)
• Is scratch space created with mktemp? (20.6)

Interface

• Does --help go to stdout and exit 0? (21.1 §3)
• Are the exit statuses documented, and does the script use its own range? (21.4)
• Is it safe to run twice, and is that stated? (21.5)
• Is there a -n, and is it really dry? (21.5 §4)

Output

• Is the answer on stdout and everything else on stderr? (20.7)
• Is >&2 inside the logging helper rather than at the call sites? (20.7 §3)

Two notes on using it.

Read the trace, not just the source. Chapter 20.8's point: a reviewer cannot see a split value in the source either. bash -x on the reviewer's own test input finds in seconds what reading finds in minutes.

And review the interface first. A script with perfect quoting and no usage message is less useful than the reverse — the quoting can be fixed by a tool, and the interface decisions are the expensive ones to change later.

7. A Complete Gate

The three layers, wired into one script you can put in CI:

#!/usr/bin/env bash
# check-scripts — the mechanical quality gate for shell code
#
# usage: check-scripts <script>...
#
# Runs, in increasing cost:
#   1. bash -n          syntax (20.8)
#   2. shellcheck       if installed (this chapter)
#   3. shlint           the local rules (this chapter)
#
# exit: 0 all clean · 1 findings · 2 bad usage

set -o nounset

note()  { printf '%s: %s\n' "${0##*/}" "$*" >&2; }
usage() { printf 'usage: %s <script>...\n' "${0##*/}" >&2; }

[ $# -ge 1 ] || { usage; exit 2; }

here=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)
failed=0
checked=0

for f in "$@"; do
  if ! [ -r "$f" ]; then
    note "cannot read: $f"
    failed=$(( failed + 1 ))          # a file not checked is a finding
    continue
  fi
  checked=$(( checked + 1 ))
  printf '== %s\n' "$f"

  # 1. syntax — free, and nothing else is meaningful until it passes
  if ! bash -n "$f"; then
    note "$f: syntax error — skipping the rest"
    failed=$(( failed + 1 ))
    continue
  fi
  printf '   bash -n     ok\n'

  # 2. shellcheck, if it is here
  if command -v shellcheck > /dev/null 2>&1; then
    if shellcheck "$f"; then
      printf '   shellcheck  ok\n'
    else
      failed=$(( failed + 1 ))
    fi
  else
    printf '   shellcheck  SKIPPED (not installed)\n'
  fi

  # 3. the local rules.
  # the pipe means an `if` here would test sed, not shlint — see below
  "$here/shlint" "$f" | sed 's/^/   /'
  if [ "${PIPESTATUS[0]}" -eq 0 ]; then
    printf '   shlint      ok\n'
  else
    failed=$(( failed + 1 ))
  fi
done

printf -- '-- %d file(s) checked, %d with findings\n' "$checked" "$failed"
[ "$failed" -eq 0 ]

$ ./check-scripts good.sh bad.sh
== good.sh
   bash -n     ok
   shellcheck  SKIPPED (not installed)
   -- 0 finding(s)
   shlint      ok
== bad.sh
   bash -n     ok
   shellcheck  SKIPPED (not installed)
   bad.sh:2: L001: unquoted variable passed to a command
   bad.sh:3: L002: for over $(ls) — use a glob
   bad.sh:4: L003: local/declare with $( ) discards the status
   bad.sh:5: L004: rm on a variable path with no ${var:?} guard
   bad.sh:6: L005: array expanded unquoted — use "${a[@]}"
   bad.sh:7: L006: pipe into while read — variables will not survive
   bad.sh:8: L007: read without -r
   bad.sh:9: L008: eval — build an array instead
   -- 8 finding(s)
-- 2 file(s) checked, 1 with findings
$ echo $?
1

$ printf '#!/bin/bash\nif true; then\n  echo hi\n' > broken.sh
$ ./check-scripts broken.sh
== broken.sh
broken.sh: line 4: syntax error: unexpected end of file
check-scripts: broken.sh: syntax error — skipping the rest
-- 1 file(s) checked, 1 with findings
$ echo $?
1

$ ./check-scripts /nonexistent.sh
check-scripts: cannot read: /nonexistent.sh
-- 0 file(s) checked, 1 with findings
$ echo $?
1

$ ./check-scripts
usage: check-scripts <script>...
$ echo $?
2

Six decisions:

Three layers in increasing cost, and syntax first. A file that does not parse cannot usefully be linted, so a bash -n failure skips the rest and says so rather than producing a hundred spurious findings.

shellcheck is optional and the skip is printed. SKIPPED (not installed) in the output is the difference between a gate that is weaker than you think and a gate that tells you so. A silent skip is Chapter 20.1's shape seven.

An unreadable file is a finding in both layers. §4's callout left that bug in shlint deliberately to make the point; the version used by this gate has the three-character fix applied, so shlint /nonexistent.sh now reports -- 1 finding(s) and exits 1 as well.

$here via $BASH_SOURCE so shlint is found next to the gate regardless of the caller's directory. Chapter 18.6 §4.

${PIPESTATUS[0]} rather than if … | sed …, and this one is not a stylistic nicety. The first version of this gate was written as:

if "$here/shlint" "$f" | sed 's/^/   /'; then
  printf '   shlint      ok\n'
else
  failed=$(( failed + 1 ))
fi

Which produced, on the same two files:

$ ./check-scripts good.sh bad.sh
== bad.sh
   ...
   bad.sh:9: L008: eval — build an array instead
   -- 8 finding(s)
   shlint      ok
-- 2 file(s) checked, 0 with findings
$ echo $?
0

Eight findings printed, shlint ok underneath them, and the gate exited 0. The if tested the last stage of the pipeline — sed, which succeeded — and never saw shlint's 1. Chapter 12.8, in a quality gate, producing exactly the false green that Chapter 20.1 is about.

$ bash -c 'false | sed "s/^/x/"; echo "if would see: $?  PIPESTATUS=${PIPESTATUS[*]}"'
if would see: 0  PIPESTATUS=1 0

So the gate runs the pipeline as a statement and then tests ${PIPESTATUS[0]}. It is worth noticing how this one escaped: the output was visibly wrong — eight findings above a line claiming ok — and it still took running the thing to see it. Reading would not have caught it, which is §5's point about layers.

And the summary counts files, not findings. "1 file with findings" is what a CI log needs on one line; the findings themselves are above it.

Practice

1. Check whether shellcheck is installed for you. If it is, run it on the longest script you have written in this course and count the findings by code.
2. Write shlint and the bad.sh specimen. Confirm all eight rules fire, then fix the specimen one line at a time and watch the count drop to zero.
3. Replace L001's pattern with a plainly broken one — grep -q 'NEVERMATCHES' — and run it on bad.sh. Note that the output looks like a better result. Explain the danger in one sentence.
4. Add a ninth rule to shlint for a defect this course taught. Write its specimen line first, confirm it fails, then write the rule.
5. Run shlint on a file that does not exist. Note the status, then apply the three-character fix from §4's callout.
6. Take the §6 checklist and review a script of your own against it. Record which section found the most.
7. Review the same script by running bash -x on a test input instead of reading it. Which defects did the trace find that reading did not?
8. Write check-scripts with the broken if … | sed … form and run it on bad.sh. Confirm it prints eight findings and then exits 0. Then fix it with ${PIPESTATUS[0]} and re-run.
9. Remove the SKIPPED (not installed) line so the shellcheck step is silently absent. Argue in two sentences why that is a worse gate.
10. Add check-scripts to a git pre-commit hook for a directory of your own scripts. Then deliberately commit a script with an unquoted expansion.

Check Your Understanding

1. What do the defects of Modules 17 to 20 have in common that makes a static checker the right tool?
2. What is SC2086, and which chapters does it correspond to?
3. How do you silence one finding, and what must the comment include besides the code?
4. L001's first version never matched anything. Why is that worse than not having the rule?
5. What is the method that prevents it?
6. shlint exits 0 on a file it could not read. Why is that the same class of bug as Chapter 20.1's?
7. Name three things a static checker cannot see.

8. Remember

• The defects of the last five modules are all visible in the source, which makes a static checker the cheapest quality gate shell code can have.
• shellcheck reports a code, a line and an explanation. The codes are stable and searchable, which makes them good review currency.
• Run it on everything, in CI. It needs no configuration and its defaults are nearly all real findings.
• Silence one finding at the line, with a reason. A blanket disable makes the checker decoration.
• A check that cannot fire is worse than no check, because it produces confidence in work nothing did.
• So every check needs a specimen that makes it fail, kept next to the checker.
• "I could not check this" is a finding, not zero findings. A skipped file must not be a green gate.
• Print the skip when an optional tool is absent. A silent skip is a gate weaker than you think.
• A checker cannot see values, process boundaries, or your interface — tests and a human cover those.
• Order the effort: bash -n and a checker for free, tests for behaviour, a human for the interface.
• Review the interface first. Quoting is fixable by a tool; interface decisions are expensive to change.
• Read the trace, not only the source. bash -x finds a split value that reading does not.

Chapter 21.8 closes Part I with the layer a checker cannot reach: proving the script behaves correctly — and proving that the proof itself is capable of failing.

• A review needs a specimen, not a checklist. Chapter 35.7 reviews thirty lines that are clean under bash -n and report three passes on a regression where one test does not compile.