Reviewing Shared Flow Code

Part II · Module 27 · Chapter 27.8 · Reviewing Shared Flow Code

Shared flow code has a property that makes reviewing it different: it is run by people who will not read it. A defect in your own script costs you an afternoon; the same defect in flow/lib/ costs everyone who sources it, repeatedly, and is diagnosed by somebody with no idea the file exists.

So the review is written here as six specimens with one deliberate defect each, and a script that looks for them.

1. The specimens

Those six are the registry's review list made concrete. Writing them as files rather than as bullet points is the point: a checklist you cannot run against anything is a checklist nobody applies, and Chapter 24.7 §1's specimen suite made the same argument for parsers.

2. The linter

Six rules, one per specimen, each a text check over the file:

# 1. a relative path in a source or open
if {[regexp {(source|open)\s+([a-zA-Z][\w./-]*)} $t -> cmd arg]} {
    if {[file pathtype $arg] ne "absolute" && ![string match "\$*" $arg]} {
        ::lint::finding $path $n cwd-dependent \
            "$cmd of a relative path '$arg' — depends on the launch directory"
    }
}
# 2. an error printed rather than raised
if {[regexp {puts\s+(stderr\s+)?"?ERROR} $t]} { ... }
# 3. channel bookkeeping
if {[regexp {\[open\s} $t]}  { incr opened }
if {[regexp {^close\s|\sclose\s} $t]} { incr closed }
# 4. a hidden global
if {[regexp {^global\s+(\w+)} $t -> g]} { ... }
# 5. an unconditional successful exit
if {[regexp {^exit\s+0\s*$} $t]} { ... }
# 6. division without a guard on the divisor
if {[regexp {expr\s*\{[^\}]*/\s*\$(\w+)} $t -> d]} { ... }

=== the six specimens, each with one deliberate defect ===
    bad1.tcl:2     cwd-dependent      source of a relative path 'flow/lib/vars.tcl' — depends on the launch directory
    bad1.tcl:3     cwd-dependent      open of a relative path 'logs/run.log' — depends on the launch directory
    bad1.tcl       unclosed-channel   1 open(s) but 0 close(s) — a channel leaks on some path
    bad2.tcl:4     printed-error      prints ERROR instead of raising it — the caller cannot tell
    bad4.tcl:3     hidden-global      procedure reaches out to global 'CORNER' — pass it or namespace it
    bad4.tcl:8     hidden-global      procedure reaches out to global 'CORNER' — pass it or namespace it
    bad5.tcl:7     always-zero        exit 0 with no condition — the caller is told everything was fine
    bad6.tcl:3     unguarded-divisor  divides by $core_area with no check that it is non-zero
    8 finding(s) across 6 specimen(s)

Eight findings. Count the specimens in that list, though: bad3.tcl is not there.

3. The false negative

=== specimen 3 was not flagged. Here it is ===
    # SPECIMEN 3 — opens a channel and never closes it, on the error path.
    proc write_report {path rows} {
        set fh [open $path w]
        foreach r $rows {
            if {[llength $r] != 2} { error "malformed row: $r" }
            puts $fh [join $r ,]
        }
        close $fh
    }

=== the channel count the linter uses ===
    opens 1, closes 1 -> the count check passes
    findings: 0

The fix in the specimen is the standard one:

proc write_report {path rows} {
    set fh [open $path w]
    set code [catch {
        foreach r $rows {
            if {[llength $r] != 2} { error "malformed row: $r" }
            puts $fh [join $r ,]
        }
    } err]
    close $fh                      ;# ALWAYS, on both paths
    if {$code} { return -code error $err }
}

close before the re-raise, unconditionally. Tcl 8.5 has no try/finally (Chapter 14.5), so catch plus an unconditional cleanup plus return -code error is the idiom — and it is worth recognising that this shape is why finally exists in later versions.

4. The false positive

=== and the same checks against the real shared libraries ===
    log.tcl:70     unguarded-divisor  divides by $total with no check that it is non-zero
    1 finding(s) across 7 shared file(s)

One finding in the real libraries. Here is the line:

=== and the finding in the REAL library is a false positive ===
    log.tcl:70  set share [expr {$total > 0 ? 100.0 * $ms / $total : 0.0}]
    the divisor IS guarded, by the ternary the regex cannot see

The divisor is guarded — $total > 0 ? is right there in the same expression. The regex matches / $total and has no way to know that a conditional protects it.

5. The defect in the linter's own source

Writing rule 6 broke the linter in a way worth keeping.

=== a regex containing a bare closing brace, in a braced Tcl word ===
    the file says:  set re {foo[^}]*bar}
    sourcing it  :  1 / extra characters after close-brace

=== the same regex with the brace escaped ===
    the file says:  set re {foo[^\}]*bar}
parsed ok, re = foo[^\}]*bar
matches fooXYZbar: 1
matches foo}bar  : 0
    sourcing it  :  0 / ok

=== why this is worse than a runtime error ===
    it is a PARSE error, so it fails before any line of the file runs,
    and it cannot be caught from inside the same file.

My rule 6 was originally {expr\s*\{[^}]*/\s*\$(\w+)}. The } inside the negated character class closes the braced Tcl word, because Tcl's brace quoting counts braces and does not know it is looking at a regex.

Two things make this worse than an ordinary typo:

It is a parse error, not a runtime error. The file fails before any line of it executes, so a catch inside the file cannot see it — the demonstration above had to put the bad form in a separate file and source it to catch anything at all.

The message points nowhere useful. extra characters after close-brace names a symptom at a position that may be far from the regex, which is Chapter 14.8's subject and the same class as the MDX comment hazard that bit this track's own authoring.

The fix is [^\}], and the escaped form still correctly excludes } — matches foo}bar : 0 confirms the class did not silently become something else.

6. What a text check can and cannot do

Being precise, since §3 and §4 are both about the same limit.

It can find a spelling. global, exit 0, a relative path in a source — these are lexical facts, and a regex is the right tool.

It cannot see control flow. Whether a close executes on every path, whether a divisor is guarded, whether a variable is set before use — these need the parse tree at minimum and a flow analysis in general.

It cannot see intent. exit 0 at the end of a script that genuinely succeeded is correct; rule 5 flags it anyway.

So the linter's place in the review is as the first pass, catching the mechanical things so human attention goes to the rest. It is not the review. The review is §7.

And the one thing worth adding to any such tool: a self-test. The six specimens are that test — the linter is only known to work because it was run against files whose defects were known in advance, which is Chapter 24.7 §1's argument and the reason bad3's escape was discoverable at all.

7. The review

What to check before shared flow code goes in flow/lib/:

• Nothing depends on the launch directory. Every path derived from file normalize [info script], and every path handed onward too (Chapter 27.1 §4).
• Inputs are validated at entry, with a message naming what is missing and what it is for (Chapter 27.4).
• Errors are raised, not printed. A caller cannot catch a puts.
• Every error carries a machine-readable code, so a caller can distinguish causes (Chapter 14.2).
• Every channel is closed on every path, including the error path — which catch plus unconditional close is how you get in 8.5.
• No hidden globals. State belongs in a namespace variable, passed explicitly, or returned.
• Helpers are idempotent where they establish state and overwrite where they produce an artefact (Chapter 27.7 §5).
• The exit status reflects the outcome, with setup failures distinguished from flow failures (Chapter 27.1 §7).
• Durations use clock milliseconds, not a difference of clock seconds (Chapter 27.6 §2).
• A log line names its step and its level, and is flushed.
• Nothing a vendor command touches is outside the seam, or the tool-agnostic claim is false (Chapter 27.5).
• Sourcing the file twice is harmless.
• It has been run from somewhere absurd — not reasoned about.

8. Exercises

Exercise 1. What property of shared flow code makes reviewing it different?

Exercise 2. Why write the review as specimens rather than a list?

Exercise 3. bad3.tcl has one open and one close. Why does the channel leak?

Exercise 4. How was the leak proven real rather than theoretical?

Exercise 5. What does a leaked channel per failure do to a long regression, and why is the symptom confusing?

Exercise 6. Write the 8.5 idiom for closing a channel on both paths.

Exercise 7. log.tcl:70 was flagged. Why is it a false positive?

Exercise 8. Why is a false positive worse than a false negative in a linter?

Exercise 9. Give three responses to a false-positive rule, and the one non-response.

Exercise 10. Why is a bare } inside a braced regex worse than an ordinary typo?

Exercise 11. Name two things a text check can do and two it cannot.

Exercise 12. What review item never appears on these lists, and what goes wrong without it?