Maintainability Practices for Legacy Perl

Part II · Module 27 · Chapter 27.7 · Maintainability Practices for Legacy Perl

1. Safe to Edit, Not Rewritten

When an inherited script is hard to change, the tempting answer is to rewrite it. A rewrite throws away the one thing the old script has that a new one does not: years of running on real inputs. Every special case it handles, including the ones nobody remembers, would have to be rediscovered — usually in production.

The alternative is to make the script safe to edit, in small steps, each proven by Chapter 27.6's baseline comparison. The set of changes is short and well known:

• use strict and use warnings, so Perl reports the mistakes it can see.
• Variables declared in the smallest scope that needs them, with my.
• Named subroutines for each distinct job, so each can be read, tested and replaced on its own.
• Each pattern defined once, with a name, so that it can be found, reviewed and changed in one place.
• A usage message and a header saying what the script takes, produces and returns.

Chapter 27.6 already did the first two to old_summary.pl and stopped at step3_lexical.pl, having found a bug it deliberately did not fix. This chapter continues from there.

2. The Cheapest Check: perl -wc

Before changing anything, one command finds a surprising amount: perl -wc compiles the script with warnings on, without running it. It changes no file and needs no inputs.

#!/bin/bash
# sorted: Perl reports these in hash order, which changes from run to run
perl -wc old_summary.pl 2>&1 | sort | sed 's/^/  /'

  Name "main::nunk" used only once: possible typo at old_summary.pl line 14.
  Name "main::nunknown" used only once: possible typo at old_summary.pl line 23.
  old_summary.pl syntax OK

Both halves of the typo, named, with their line numbers. The driver sorts the two warnings because Perl emits them in hash order: across four runs on this machine, one printed them the other way round. The warning "used only once: possible typo" fires when a package variable appears exactly once in the whole program — which is what a misspelled variable looks like. Here the counter $nunk is only ever incremented, and the summary prints $nunknown, which is never set: two variables, each used once. That is the bug Chapter 27.6 found by adding strict, and that has printed a blank count in every summary since the script was written.

perl -wc is safe to run on any script, at any time, without changing it. It is the first thing to do with an inherited script and the cheapest gate to add to a flow's checks. It does not see everything — it only reports what can be known at compile time — but what it reports is almost always real.

3. strict Finds It Too, Buried

Adding use strict finds the same typo, but differently. Under strict, every variable must be declared, so an undeclared one is an error — and in an inherited script, every variable is undeclared:

#!/bin/bash
{ echo 'use strict;'; cat old_summary.pl; } > strict_try.pl
perl -c strict_try.pl > strict.out 2>&1
echo "  'Global symbol' errors: $(grep -c 'Global symbol' strict.out)"
echo "  distinct variables named: $(grep -o 'Global symbol "[^"]*"' strict.out | sort -u | wc -l | tr -d ' ')"
echo "  of which the typo pair:"
grep -o 'Global symbol "\$nunk[a-z]*"' strict.out | sort -u | sed 's/^/    /'
echo "  last line reported: $(grep -o 'line [0-9]*' strict.out | tail -n 1)"
echo "  \$nunknown is used on: line $(grep -n 'nunknown' strict_try.pl | cut -d: -f1)"
tail -n 1 strict.out | sed 's/^/  /'

  'Global symbol' errors: 31
  distinct variables named: 10
  of which the typo pair:
    Global symbol "$nunk"
  last line reported: line 20
  $nunknown is used on: line 24
  strict_try.pl had compilation errors.

Thirty-one errors about ten variables — and only one half of the typo is among them. Perl stopped reporting at line 20 and gave up with "had compilation errors"; the misspelled $nunknown is on line 24, so strict never mentions it on this first pass. It would appear only after the earlier variables had been declared and the script compiled again. Among nine other undeclared but perfectly correct variables, $nunk alone is not a clue at all. This is why strict is added as a step of its own — declaring each variable with my in the smallest scope that needs it (Chapter 12.7) — rather than as a quick check. While doing that step, the engineer meets each variable once and has to decide where it belongs. That is when $nunknown stands out: a variable that is printed and never set has nowhere sensible to be declared.

So on the same script, perl -wc named both halves of the typo in two lines, while strict named one half in thirty-one. perl -wc is the better detector; strict is the better prevention. Once a script is under strict, a new typo becomes a compile error the moment it is written.

4. Warnings at Run Time

use warnings adds checks that can only happen while the script runs. Adding it to Chapter 27.6's step 3 — and changing nothing else — and running it on the real inputs:

#!/bin/bash
{ sed -n '1,2p' step3_lexical.pl; echo 'use warnings;'; sed -n '3,$p' step3_lexical.pl; } > with_warnings.pl
perl with_warnings.pl inputs/*.log > /dev/null 2> warnings.txt
sed 's/^/  /' warnings.txt

  Use of uninitialized value $nunknown in concatenation (.) or string at with_warnings.pl line 31.

At run time, the same bug surfaces a third way: the printed variable is undefined. Chapter 14.7 decoded the classic warnings; this one, "Use of uninitialized value", is the most common in inherited Perl and almost always marks a real defect.

Notice what adding use warnings does to the baseline comparison from Chapter 27.6: it adds a line to standard error. On these inputs that is a change a caller can see — a wrapper that treats any stderr output as a problem would now complain. So turning on warnings is itself a behaviour change, and the honest way to make it is together with the fix that silences the warning, as one deliberate, reviewed change.

5. Fix the Typo, Alone

Step 5 turns on warnings, fixes the typo, and starts every counter at zero — because a counter that is never incremented, such as $npass on a night when nothing passed, would print as a blank and warn, the same bug waiting for a different input:

#!/usr/bin/perl
# step 5: warnings on, and the unknown count fixed -- a BEHAVIOUR change, made alone.
use strict;
use warnings;

my ($total, $npass, $nfail, $nunk) = (0, 0, 0, 0);   # every counter starts at 0
my (%status, %first);

foreach my $f (@ARGV) {
    open(my $fh, "<", $f) || die "cannot open $f";
    my $text = "";
    while (<$fh>) { $text .= $_; }
    close($fh);
    my $name = $f;
    $name =~ s/.*\///;
    $name =~ s/\.log$//;
    $status{$name} = classify($text);
    if    ($status{$name} eq "FAIL") { $nfail++ }
    elsif ($status{$name} eq "PASS") { $npass++ }
    else                             { $nunk++ }
    if ($text =~ /^ERROR: (.*)$/m) { $first{$name} = $1; }
    $total++;
}
foreach my $t (sort keys %status) {
    print "$t: $status{$t}";
    print "  first error: $first{$t}" if $first{$t};
    print "\n";
}
print "total $total, pass $npass, fail $nfail, unknown $nunk\n";

sub classify {
    my ($text) = @_;
    return "FAIL" if $text =~ /ERROR/ || $text =~ /FATAL/;
    return "PASS" if $text =~ /finish/;
    return "UNKNOWN";
}

#!/bin/bash
perl -pe 's/^foreach \$t \(keys %status\)/foreach \$t (sort keys %status)/' old_summary.pl > step1_sorted.pl
bash snapshot.sh step1_sorted.pl baseline
bash compare.sh step5_counts_fixed.pl

step5_counts_fixed.pl: CHANGED
    @@ -8,4 +8,4 @@
     icarus_sv_error: FAIL  first error: tb/sv_error.v:3: data mismatch exp=5 got=4
     icarus_sv_fatal: FAIL
     iverilog_compile_error: UNKNOWN
    -total 10, pass 2, fail 4, unknown 
    +total 10, pass 2, fail 4, unknown 4

Exactly one line differs, and it is exactly the fix: unknown became unknown 4. Standard error is still empty, so warnings found nothing else. That diff is the whole review: anyone reading it can see what changed and agree it is right. After the review, the baseline is regenerated from step 5, and the next steps are compared against that.

6. Named Subroutines, Named Patterns, a Usage Line

Step 6 is pure structure. Reading a file and turning a path into a log name each become a named subroutine (Chapter 10.12). Every pattern moves to the top of the file, defined once, as a named qr// (Chapter 11.18) with a comment saying what it is for. The script gains a header saying what it takes, produces and returns, and a usage message that exits 2 (Chapters 15.3 and 15.4).

#!/usr/bin/perl
# summary.pl -- summarise simulation logs given on the command line.
#   usage:  summary.pl LOG...
#   output: one "name: VERDICT" line per log, sorted, then a totals line (stdout)
#   exit:   0; 2 for a usage error; non-zero (die) if a log cannot be opened
use strict;
use warnings;

# Every pattern the script uses, defined once, with what it is for.
my $ERROR_WORD  = qr/ERROR/;              # classification: the word, anywhere
my $ERROR_LINE  = qr/^ERROR: (.*)$/m;     # first error: a line that starts ERROR:
my $FATAL_WORD  = qr/FATAL/;
my $FINISH_WORD = qr/finish/;

unless (@ARGV) { print STDERR "usage: summary.pl LOG...\n"; exit 2 }

my ($total, $npass, $nfail, $nunk) = (0, 0, 0, 0);
my (%status, %first);

foreach my $f (@ARGV) {
    my $text = read_file($f);
    my $name = log_name($f);
    $status{$name} = classify($text);
    if    ($status{$name} eq "FAIL") { $nfail++ }
    elsif ($status{$name} eq "PASS") { $npass++ }
    else                             { $nunk++ }
    $first{$name} = $1 if $text =~ $ERROR_LINE;
    $total++;
}
foreach my $t (sort keys %status) {
    print "$t: $status{$t}";
    print "  first error: $first{$t}" if $first{$t};
    print "\n";
}
print "total $total, pass $npass, fail $nfail, unknown $nunk\n";

sub read_file {
    my ($f) = @_;
    open(my $fh, "<", $f) || die "cannot open $f";
    local $/;
    my $text = <$fh> // "";
    close($fh);
    return $text;
}

sub log_name {
    my ($f) = @_;
    (my $name = $f) =~ s{.*/}{};
    $name =~ s/\.log$//;
    return $name;
}

sub classify {
    my ($text) = @_;
    return "FAIL" if $text =~ $ERROR_WORD || $text =~ $FATAL_WORD;
    return "PASS" if $text =~ $FINISH_WORD;
    return "UNKNOWN";
}

#!/bin/bash
bash snapshot.sh step5_counts_fixed.pl baseline
bash compare.sh step6_named_patterns.pl
echo "--- and called with no arguments"
perl step6_named_patterns.pl 2>&1
echo "exit $?"

step6_named_patterns.pl: IDENTICAL to the baseline (stdout, stderr, exit status)
--- and called with no arguments
usage: summary.pl LOG...
exit 2

Identical, and the usage message works.

Look at what naming the patterns did. In the original, the two definitions of an error were written inline, in two different statements doing two different jobs, and nobody noticed they differed — that difference was the bug Chapter 27.6's step 4 tripped over. In step 6 they sit on adjacent lines:

my $ERROR_WORD  = qr/ERROR/;              # classification: the word, anywhere
my $ERROR_LINE  = qr/^ERROR: (.*)$/m;     # first error: a line that starts ERROR:

The inconsistency is now impossible to miss. Step 6 did not fix it — that would be a behaviour change — but it made it visible to the next reviewer, which is most of what maintainability means. A named pattern is also a pattern that can be tested on its own, with Chapter 27.5's table.

7. The Payoff: The Shared Module

With the script under strict and warnings, structured into named pieces and fully covered by a baseline, the change everyone wanted at the start is now small and safe: replace the script's own classification with parse_log from the shared module in Chapter 27.1.

#!/usr/bin/perl
# summary.pl -- summarise simulation logs given on the command line.
#   usage:  summary.pl LOG...
#   output: one "name: VERDICT" line per log, sorted, then a totals line (stdout)
#   exit:   0; 2 for a usage error; non-zero if a log cannot be opened (croak)
use strict;
use warnings;
use FindBin qw($Bin);
use lib "$Bin/lib";
use VLSI::Report qw(parse_log);

my $ERROR_LINE = qr/^ERROR: (.*)$/m;      # first error: a line that starts ERROR:

unless (@ARGV) { print STDERR "usage: summary.pl LOG...\n"; exit 2 }

my (%status, %first, %count);
foreach my $f (@ARGV) {
    my $name = log_name($f);
    $status{$name} = parse_log($f)->{verdict};     # one definition, shared (27.1)
    $count{ $status{$name} }++;
    my $text = do { open my $fh, "<", $f or die "cannot open $f"; local $/; <$fh> // "" };
    $first{$name} = $1 if $text =~ $ERROR_LINE;
}
foreach my $t (sort keys %status) {
    print "$t: $status{$t}";
    print "  first error: $first{$t}" if $first{$t};
    print "\n";
}
print "total ", scalar(keys %status), ", ",
      join(", ", map { lc($_) . " $count{$_}" } sort keys %count), "\n";

sub log_name {
    my ($f) = @_;
    (my $name = $f) =~ s{.*/}{};
    $name =~ s/\.log$//;
    return $name;
}

This is a behaviour change by design — the module's verdicts are not the old script's — so the baseline comparison is expected to report differences. The question is whether they are the right differences. The check below lists every verdict that changed and compares the new one with what a person reading the log concludes (the truth tables from Chapters 27.1 and 27.4):

use strict;
use warnings;

# What a person reading each log concludes (27.1 and 27.4).
my %TRUTH = (
    icarus_pass => "PASS", icarus_mismatch => "FAIL", icarus_sv_error => "FAIL",
    icarus_finish_1 => "FAIL", icarus_sv_fatal => "FAIL", icarus_killed => "NO_FINISH",
    icarus_empty => "EMPTY_LOG", icarus_pass_then_killed => "NO_FINISH",
    icarus_info_mentions_error => "PASS",
    iverilog_compile_error => "not PASS",
);

sub verdicts {
    my ($script) = @_;
    my %v;
    for (`perl $script inputs/*.log`) { $v{$1} = $2 if /^(\w+): (\w+)/ }
    return \%v;
}
my ($old, $new) = (verdicts("step6_named_patterns.pl"), verdicts("step7_shared_module.pl"));

sub agrees { my ($got, $want) = @_; $want eq "not PASS" ? $got ne "PASS" : $got eq $want }

printf "%-28s %-9s %-10s %-10s %s\n", "log", "old", "new", "truth", "new agrees?";
my ($changed, $fixed) = (0, 0);
for my $log (sort keys %TRUTH) {
    next if $old->{$log} eq $new->{$log};
    $changed++;
    my $ok = agrees($new->{$log}, $TRUTH{$log});
    $fixed++ if $ok;
    printf "%-28s %-9s %-10s %-10s %s\n", $log, $old->{$log}, $new->{$log}, $TRUTH{$log}, $ok ? "yes" : "NO";
}
my $old_right = grep { agrees($old->{$_}, $TRUTH{$_}) } keys %TRUTH;
my $new_right = grep { agrees($new->{$_}, $TRUTH{$_}) } keys %TRUTH;
print "\n$changed verdicts changed; $fixed of them now agree with the truth.\n";
print "agreeing with the truth: old script $old_right of 10, new script $new_right of 10\n";

#!/bin/bash
bash snapshot.sh step6_named_patterns.pl baseline
bash compare.sh step7_shared_module.pl > /dev/null
echo "compare.sh says the output changed (exit $?); checking each change against the truth:"
echo
perl check_truth.pl

compare.sh says the output changed (exit 1); checking each change against the truth:

log                          old       new        truth      new agrees?
icarus_empty                 UNKNOWN   EMPTY_LOG  EMPTY_LOG  yes
icarus_finish_1              PASS      FAIL       FAIL       yes
icarus_info_mentions_error   FAIL      PASS       PASS       yes
icarus_killed                UNKNOWN   NO_FINISH  NO_FINISH  yes
icarus_pass_then_killed      UNKNOWN   NO_FINISH  NO_FINISH  yes
iverilog_compile_error       UNKNOWN   NO_FINISH  not PASS   yes

6 verdicts changed; 6 of them now agree with the truth.
agreeing with the truth: old script 5 of 10, new script 10 of 10

Six verdicts changed, and all six now agree with the truth. The old script called a failed run a pass, a passing run a failure, and four different kinds of trouble "UNKNOWN". It agreed with a careful reader on 5 of 10 logs; the new script agrees on 10 of 10. The diff that a baseline comparison produced is, read the right way, the list of everything the old script had been getting wrong — which is the strongest possible review description for this change.

The script is also simpler: the classification and its counters are gone, replaced by one call. The parts that remain are the parts that are genuinely this script's job — naming the logs, printing the first error, formatting the summary.

8. What Not To Do

The practices above work because each step is small and proven. Four common habits break that:

Rewriting "while you are in there". A step that also restructures something else, renames variables or "cleans up" produces a diff nobody can review. One idea per step.

Reformatting the whole file in the same change. Re-indenting every line makes the real change invisible in the diff. If formatting is wanted, do it as its own step, prove it identical, and only then change anything. (Perl::Tidy, the usual formatter, is a CPAN module and is not installed on the machine these examples were run on.)

Turning on warnings and silencing them. no warnings around a warning, or 2>/dev/null on the script, hides exactly the evidence section 4 used. Fix the cause.

Fixing bugs inside a refactoring step. Chapter 27.6's rule. The typo here was found in one step and fixed in another, and each diff says exactly one thing.

9. Exercises

1. Run perl -wc on your flow. Run it over every Perl script in a flow you have access to and count the "used only once" warnings. For each one, decide whether it is a typo, a variable used by another file (our), or harmless. How many real bugs did one command find?

2. Make the inconsistency a decision. Step 6 left $ERROR_WORD and $ERROR_LINE side by side. Write the change that settles which one the classification should use, as its own step, with its compare.sh diff and a one-paragraph review description. (Chapter 27.6 already measured one answer.)

3. A counter that is never incremented. Construct a set of real inputs on which step 3 — counters not initialised — prints a blank count, and show the warning use warnings gives. Then confirm that step 5 prints 0 for the same inputs.

4. Name a subroutine for testing. Extract step 6's classify into a module and write a Test::More file for it using the golden logs from Chapter 27.2. Which of Chapter 27.4's negative inputs make its tests fail, and does that tell you to fix classify or to switch to parse_log?

5. Measure before you rewrite. Take a script someone wants to rewrite. Before agreeing, run check_truth.pl's idea on it: build a truth table for a few dozen real inputs and measure how often the current script is right. Write down what number would justify a rewrite instead of the steps in this chapter.

10. What To Remember

• Make inherited scripts safe to edit; do not rewrite them. A rewrite throws away years of real inputs.
• perl -wc first. Measured: it named both halves of a years-old typo — $nunk and $nunknown, each "used only once" — without editing anything.
• strict reported 31 errors and only one half of the typo — it stopped before reaching the other. Add it as its own step, declaring each variable in the smallest scope (12.7).
• use warnings reported the bug at run time — and turning it on changes stderr, so it belongs with the fix.
• Fix bugs as separate changes. Measured: step 5's diff was exactly one line, unknown → unknown 4.
• Initialise every counter, or a quiet night prints a blank.
• Named subroutines and named patterns are pure structure. Measured: identical — and two different error patterns now sit on adjacent lines.
• A usage message, a header, and a deliberate exit status (15.3, 15.4).
• With the script under control, the shared module is one line. Measured: 6 verdicts changed, 6 of 6 toward the truth; right on 5 of 10 logs before, 10 of 10 after.
• One idea per step; never reformat, rewrite or fix inside a refactor.

Chapters 27.8 to 27.11 turn this module into four review checklists — for patterns, for parsers, for maintainability and for automation — each one measured against defective specimens, so that a reviewer can catch these problems before a script is merged rather than years later.