Pre-Merge Review: Regex Correctness

Part II · Module 27 · Chapter 27.8 · Pre-Merge Review: Regex Correctness

1. The Checklist

A checklist is useful in review for one reason: a reviewer reading a pattern sees what the pattern is meant to match, and has to make a deliberate effort to think about what it also matches. Each question below forces that effort in one specific direction.

The questions are not new — Chapter 18.4 made the case for anchoring severity tags, Chapter 11.21 for debugging a pattern that lies, Chapter 27.5 for testing patterns line by line. The checklist puts them in one place, in the order a reviewer can work through them, with the evidence for each.

2. The Evidence: One Real Log Full of Traps

The review below runs every pattern on one real log, captured from the simulator. The testbench prints lines that real testbenches print, and each one happens to be a trap for at least one of the five questions:

module tb;
  initial begin
    $display("INFO: next phase injects an ERROR response on purpose");
    $display("INFO: BYPASS mode enabled for this test");
    $display("INFO: wrote \"cov_a.ucdb\" and \"cov_b.ucdb\"");
    $error("data mismatch exp=5 got=4");
    $display("TEST FAILED");
    $finish(1);
  end
endmodule

#!/bin/bash
iverilog -g2012 -o corpus.vvp tb/corpus.v && vvp -n corpus.vvp > corpus.log
sed 's/^/  | /' corpus.log

  | INFO: next phase injects an ERROR response on purpose
  | INFO: BYPASS mode enabled for this test
  | INFO: wrote "cov_a.ucdb" and "cov_b.ucdb"
  | ERROR: tb/corpus.v:6: data mismatch exp=5 got=4
  |        Time: 0  Scope: tb
  | TEST FAILED
  | tb/corpus.v:8: $finish(1) called at 0 (1s)

Seven lines. One real error report. A run that failed — TEST FAILED. And three INFO lines that between them mention ERROR, contain the letters PASS, and quote two file names.

The review script asks each question of a pattern someone might plausibly submit, and of the corrected pattern, on these exact lines:

use strict;
use warnings;

# One real log, captured from the simulator (tb/corpus.v). Every checklist
# question is asked of a pattern someone might plausibly submit, and of the
# corrected pattern, on these exact lines.
open my $fh, "<", "corpus.log" or die "corpus.log: $!\n";
my @lines = <$fh>;
close $fh;
chomp @lines;

sub count { my ($re) = @_; scalar grep { $_ =~ $re } @lines }

print "1. Anchored? -- count the error reports (truth: 1)\n";
printf "   submitted  %-28s -> %d\n", '/ERROR/',         count(qr/ERROR/);
printf "   corrected  %-28s -> %d\n", '/^ERROR: /',      count(qr/^ERROR: /);

print "2. Bounded to whole words? -- did the run pass? (truth: no, TEST FAILED)\n";
printf "   submitted  %-28s -> %s\n", '/PASS/',          count(qr/PASS/)           ? "passed" : "failed";
printf "   corrected  %-28s -> %s\n", '/^TEST PASSED$/', count(qr/^TEST PASSED$/)  ? "passed" : "failed";

print "3. Greedy on purpose? -- the files named in the INFO line (truth: 2 files)\n";
my ($line) = grep { /wrote/ } @lines;
my @greedy = $line =~ /"(.*)"/g;
my @lazy   = $line =~ /"([^"]*)"/g;
printf "   submitted  %-28s -> %d capture: [%s]\n", '/"(.*)"/g',     scalar @greedy, join("] [", @greedy);
printf "   corrected  %-28s -> %d captures: [%s]\n", '/"([^"]*)"/g', scalar @lazy,   join("] [", @lazy);

print "4. Literals escaped? -- find the end-of-run line (truth: 1)\n";
printf "   submitted  %-28s -> %d\n", '/\$finish(1) called/',     count(qr/\$finish(1) called/);
printf "   also wrong %-28s -> %d\n", '/\Q\$finish(1) called\E/', count(qr/\Q\$finish(1) called\E/);
printf "   corrected  %-28s -> %d\n", '/\$finish\(1\) called/',   count(qr/\$finish\(1\) called/);
my $literal = '$finish(1) called';
printf "   corrected  %-28s -> %d\n", '/\Q$literal\E/',           count(qr/\Q$literal\E/);

print "5. What if the field is absent? -- slack per path (synthetic report lines, 26.4)\n";
my @paths = ("  slack (VIOLATED)   -0.043", "  slack (MET)", "  slack (MET)         0.120");
my (@stale, @zero, @honest);
for my $p (@paths) {
    $p =~ /slack \(\w+\)\s+(-?\d+\.\d+)/;
    push @stale, $1;                                   # $1 never checked
    my ($s) = $p =~ /slack \(\w+\)\s+(-?\d+\.\d+)/;
    push @zero, $s // 0;                               # missing becomes 0
    push @honest, defined $s ? $s : "none";            # missing stays missing
}
printf "   submitted  %-28s -> %s\n", 'use $1 after the match',  join("  ", @stale);
printf "   submitted  %-28s -> %s\n", 'capture, then // 0',      join("  ", @zero);
printf "   corrected  %-28s -> %s\n", 'capture, test defined',   join("  ", @honest);

#!/bin/bash
iverilog -g2012 -o corpus.vvp tb/corpus.v && vvp -n corpus.vvp > corpus.log
perl review_regex.pl

1. Anchored? -- count the error reports (truth: 1)
   submitted  /ERROR/                      -> 2
   corrected  /^ERROR: /                   -> 1
2. Bounded to whole words? -- did the run pass? (truth: no, TEST FAILED)
   submitted  /PASS/                       -> passed
   corrected  /^TEST PASSED$/              -> failed
3. Greedy on purpose? -- the files named in the INFO line (truth: 2 files)
   submitted  /"(.*)"/g                    -> 1 capture: [cov_a.ucdb" and "cov_b.ucdb]
   corrected  /"([^"]*)"/g                 -> 2 captures: [cov_a.ucdb] [cov_b.ucdb]
4. Literals escaped? -- find the end-of-run line (truth: 1)
   submitted  /\$finish(1) called/         -> 0
   also wrong /\Q\$finish(1) called\E/     -> 0
   corrected  /\$finish\(1\) called/       -> 1
   corrected  /\Q$literal\E/               -> 1
5. What if the field is absent? -- slack per path (synthetic report lines, 26.4)
   submitted  use $1 after the match       -> -0.043  -0.043  0.120
   submitted  capture, then // 0           -> -0.043  0  0.120
   corrected  capture, test defined        -> -0.043  none  0.120

Every submitted pattern got its question wrong, and one "corrected" pattern did too. The sections below take them one at a time.

3. Question 1: Is It Anchored?

The submitted /ERROR/ counted two error reports; there is one. The second match is the testbench saying it will inject an ERROR response on purpose — an INFO line, describing expected behaviour. Chapter 18.4 measured exactly this class of mistake for severity tags, and Chapter 27.4 caught it as a planted bug.

The simulator puts its error reports at the start of a line, followed by a colon and a space. The corrected /^ERROR: / says so, and counts one.

How to review it: for every pattern, ask where on the line the thing it matches really appears, and whether the pattern says so. Unanchored is right only when the text can legitimately appear anywhere.

4. Question 2: Is It Bounded to Whole Words?

The submitted /PASS/ said the run passed. It failed. The match came from BYPASS mode enabled — the letters PASS inside a longer word. This is the most dangerous answer in the chapter, because the error is in the direction nobody investigates: a failed run reported as a pass.

The corrected pattern does more than add \b. It matches the whole line the testbench prints for a pass, ^TEST PASSED$, because the thing being checked is that line, not the word. A word-boundary version, /\bPASS(ED)?\b/, would avoid BYPASS and still match an INFO line that said "the previous test PASSED". Bounding a pattern means bounding it to the actual unit of meaning — here, a complete line.

How to review it: ask what longer words, or longer lines, contain the matched text. Names in chip design are made by gluing words together — BYPASS, PASSTHRU, ERRCNT, clk_div — so this question has an answer far more often than it seems.

5. Question 3: Is It Greedy on Purpose?

The submitted /"(.*)"/g captured one value, cov_a.ucdb" and "cov_b.ucdb. .* is greedy: it takes as much as it can and gives back only enough for the rest of the pattern to match, so it runs from the first quote to the last quote on the line. With one quoted name per line it works, which is why it gets written. With two it silently merges them.

The corrected /"([^"]*)"/g says what a quoted name actually is — any characters that are not a quote — and captures both. A lazy .*? would also work here; the explicit character class is preferable because it states the rule rather than relying on the search order.

How to review it: for every .* or .+, ask what it is supposed to stop at, and whether that character can appear more than once on the line. If the answer is "it can", the pattern needs a class or a lazy quantifier.

6. Question 4: Are Literal Characters Escaped?

The submitted /\$finish(1) called/ matched nothing. The $ is escaped, but the parentheses are not, so (1) is a capturing group matching the character 1. The pattern actually looks for $finish1 called, which never appears. A missed end-of-run line becomes a NO_FINISH verdict, and nothing reports why.

The next line in the output is the instructive one. /\Q\$finish(1) called\E/ also matched nothing — and it looks like the textbook fix. \Q...\E quotes everything between them, but inside a pattern literal Perl first processes the backslashes and variable interpolation, then applies the quoting. So the \$ is quoted as well, and the pattern ends up requiring a literal backslash before the dollar sign. That "fix" was written for this chapter first, and the measurement caught it.

Two forms are correct, and both matched:

• Escape each special character by hand: /\$finish\(1\) called/. Fine for a short, fixed literal.
• Put the literal text in a variable and quote the variable: my $literal = '$finish(1) called'; /\Q$literal\E/. This is what \Q is for — text that arrives in a variable, from a file or from the user — and it is the only safe way to search for text you did not write yourself.

How to review it: look for ( ) [ ] { } . $ ^ * + ? | in any part of a pattern that is meant literally — file names (.), function calls (( )), bus indices ([ ]). And treat \Q inside a pattern literal as something to test, not something to trust.

7. Question 5: What If the Field Is Absent?

This question is about the code around the pattern. The three lines in item 5 are in this module's synthetic report format (Chapter 26.4); the middle path has no slack value, as happens with an unconstrained path.

The first submission used $1 without checking that the match succeeded, and gave the middle path a slack of −0.043 — the previous path's value. A failed match does not clear $1; it keeps whatever the last successful match left there. In a loop, that means the previous iteration's value. Measured separately while preparing this chapter, the stale value can even come from a different pattern entirely. The result is a violation that does not exist, attributed to a path that has no slack.

The second submission captured properly but wrote $s // 0, so the missing value became 0 — a path with exactly zero slack, which counts as met. Chapter 27.5 showed the same zero is also false in Perl, which makes it vanish from any truth test as well. A defaulted zero is a fabricated measurement.

The corrected code captures into a variable and tests defined, so the absent value stays absent — none — and the caller decides what an unconstrained path means.

How to review it: for every capture, find what happens on the line where the pattern does not match. If the code reads $1 after a match that was not tested, or replaces undef with a number, the absent case is producing data.

8. The Review as a Script

The review above was not done by reading. It was done by running each pattern on real lines and comparing with the truth — the same method as Chapter 27.5's tables, applied at review time. That suggests how to make the checklist cheap to use:

• Keep a corpus of real lines with traps in it, captured from the tools (Chapter 27.2). Lines that mention the keyword without being it, words that contain the keyword, lines with two of the thing being captured, lines where the field is missing.
• Ask for the table with the pattern. A pattern submitted without Chapter 27.5's table of matching and must-not-match lines is a pattern whose author has not yet answered these five questions.
• Run every submitted pattern against the corpus, and read the lines it matches that its author did not expect. That list is the review.

A reviewer who does only this will catch the defects in this chapter. A reviewer who only reads the pattern will catch the ones they happen to think of.

9. Exercises

1. Extend the corpus. Add a line to tb/corpus.v that defeats the corrected pattern for question 1 — a line that starts with ERROR: but is not an error report. Is that possible with Icarus's own output, or only with a testbench that prints it deliberately? What does that mean for how much to trust an anchor?

2. The word boundary version. Measure /\bPASS(ED)?\b/ on the corpus, then add an INFO line that defeats it. Write down the rule a reviewer should apply to choose between a word boundary and a whole-line match.

3. Lazy versus class. Find a line on which /"(.*?)"/g and /"([^"]*)"/g give different answers. (Think about a quoted name that contains an escaped quote.) Which one is right for your tool's output?

4. Find the stale $1. Search a flow you have access to for a use of $1, $2 or a later capture variable that is not inside the if that tested the match. For each one, construct the input on which it reports a stale value.

5. Review a real pattern. Take one regular expression from a parser you maintain, run it through all five questions, and write the corpus lines that prove each answer. If any question has no line, capture one from the tool before merging the next change to that pattern.

10. What To Remember

• Five questions for every pattern: anchored, bounded, greedy on purpose, escaped, absent field.
• Anchored. Measured: /ERROR/ counted 2 errors where there was 1 — an INFO line mentioned it.
• Bounded to the unit of meaning. Measured: /PASS/ called a failed run a pass, because of BYPASS.
• Greedy on purpose. Measured: /"(.*)"/g merged two file names into one capture; /"([^"]*)"/g got both.
• Escaped. Measured: /\$finish(1) called/ matched nothing — the parentheses were a group.
• \Q belongs on a variable, not a literal. Measured: /\Q\$finish(1) called\E/ also matched nothing; /\Q$literal\E/ worked.
• Absent field. Measured: an unchecked $1 gave a path the previous path's −0.043; // 0 gave it a fabricated zero. Test defined.
• Review by running, not reading: real corpus lines with traps, the pattern's own table (27.5), and the list of unexpected matches.

Chapter 27.9 widens the review from one pattern to a whole parser, and asks what it does with every input it will meet in production.