chainlint: annotate original test definition rather than token stream

When chainlint detects problems in a test, such as a broken &&-chain, it prints out the test with "?!FOO?!" annotations inserted at each problem location. However, rather than annotating the original test definition, it instead dumps out a parsed token representation of the test. Since it lacks comments, indentations, here-doc bodies, and so forth, this tokenized representation can be difficult for the test author to digest and relate back to the original test definition. However, now that each parsed token carries positional information, the location of a detected problem can be pinpointed precisely in the original test definition. Therefore, take advantage of this information to annotate the test definition itself rather than annotating the parsed token stream, thus making it easier for a test author to relate a problem back to the source. Maintaining the positional meta-information associated with each detected problem requires a slight change in how the problems are managed internally. In particular, shell syntax such as: msg="total: $(cd data; wc -w *.txt) words" requires the lexical analyzer to recursively invoke the parser in order to detect problems within the $(...) expression inside the double-quoted string. In this case, the recursive parse context will detect the broken &&-chain between the `cd` and `wc` commands, returning the token stream: cd data ; ?!AMP?! wc -w *.txt However, the parent parse context will see everything inside the double-quotes as a single string token: "total: $(cd data ; ?!AMP?! wc -w *.txt) words" losing whatever positional information was attached to the ";" token where the problem was detected. One way to preserve the positional information of a detected problem in a recursive parse context within a string would be to attach the positional information to the annotation textually; for instance: "total: $(cd data ; ?!AMP:21:22?! wc -w *.txt) words" and then extract the positional information when annotating the original test definition. However, a cleaner and much simpler approach is to maintain the list of detected problems separately rather than embedding the problems as annotations directly in the parsed token stream. Not only does this ensure that positional information within recursive parse contexts is not lost, but it keeps the token stream free from non-token pollution, which may simplify implementation of validations added in the future since they won't have to handle non-token "?!FOO!?" items specially. Finally, the chainlint self-test "expect" files need a few mechanical adjustments now that the original test definitions are emitted rather than the parsed token stream. In particular, the following items missing from the historic parsed-token output are now preserved verbatim: * indentation (and whitespace, in general) * comments * here-doc bodies * here-doc tag quoting (i.e. "\EOF") * line-splices (i.e. "\" at the end of a line) Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Eric Sunshine committed Nov 8, 2022 at 19:08 UTC 73c768dae9ea4838736693965b25ba34e941ac88
22 files changed +163 -33
t/chainlint.pl
+24 -7
@@ -459,6 +459,13 @@ package TestParser;
459
460 use base 'ShellParser';
461
462 +sub new {
463 + my $class = shift @_;
464 + my $self = $class->SUPER::new(@_);
465 + $self->{problems} = [];
466 + return $self;
467 +}
468 +
469 sub find_non_nl {
470 my $tokens = shift @_;
471 my $n = shift @_;
@@ -498,7 +505,7 @@ sub parse_loop_body {
505 return @tokens if ends_with(\@tokens, [qr/^\|\|$/, "\n", qr/^echo$/, qr/^.+$/]);
506 # flag missing "return/exit" handling explicit failure in loop body
507 my $n = find_non_nl(\@tokens);
501 - splice(@tokens, $n + 1, 0, ['?!LOOP?!', $tokens[$n]->[1], $tokens[$n]->[2]]);
508 + push(@{$self->{problems}}, ['LOOP', $tokens[$n]]);
509 return @tokens;
510 }
511
@@ -511,6 +518,7 @@ my @safe_endings = (
518
519 sub accumulate {
520 my ($self, $tokens, $cmd) = @_;
521 + my $problems = $self->{problems};
522
523 # no previous command to check for missing "&&"
524 goto DONE unless @$tokens;
@@ -531,13 +539,13 @@ sub accumulate {
539 # failure explicitly), then okay for all preceding commands to be
540 # missing "&&"
541 if ($$cmd[0]->[0] =~ /^(?:false|return|exit)$/) {
534 - @$tokens = grep {$_->[0] !~ /^\?!AMP\?!$/} @$tokens;
542 + @$problems = grep {$_->[0] ne 'AMP'} @$problems;
543 goto DONE;
544 }
545
546 # flag missing "&&" at end of previous command
547 my $n = find_non_nl($tokens);
540 - splice(@$tokens, $n + 1, 0, ['?!AMP?!', $$tokens[$n]->[1], $$tokens[$n]->[2]]) unless $n < 0;
548 + push(@$problems, ['AMP', $tokens->[$n]]) unless $n < 0;
549
550 DONE:
551 $self->SUPER::accumulate($tokens, $cmd);
@@ -594,12 +602,21 @@ sub check_test {
602 $self->{ntests}++;
603 my $parser = TestParser->new(\$body);
604 my @tokens = $parser->parse();
597 - return unless $emit_all || grep {$_->[0] =~ /\?![^?]+\?!/} @tokens;
605 + my $problems = $parser->{problems};
606 + return unless $emit_all || @$problems;
607 my $c = main::fd_colors(1);
599 - my $checked = join(' ', map {$_->[0]} @tokens);
608 + my $start = 0;
609 + my $checked = '';
610 + for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {
611 + my ($label, $token) = @$_;
612 + my $pos = $token->[2];
613 + $checked .= substr($body, $start, $pos - $start) . " ?!$label?! ";
614 + $start = $pos;
615 + }
616 + $checked .= substr($body, $start);
617 $checked =~ s/^\n//;
601 - $checked =~ s/^ //mg;
602 - $checked =~ s/ $//mg;
618 + $checked =~ s/(\s) \?!/$1?!/mg;
619 + $checked =~ s/\?! (\s)/?!$1/mg;
620 $checked =~ s/(\?![^?]+\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;
621 $checked .= "\n" unless $checked =~ /\n$/;
622 push(@{$self->{output}}, "$c->{blue}# chainlint: $title$c->{reset}\n$checked");
t/chainlint/block-comment.expect
+2
@@ -1,6 +1,8 @@
1 (
2 {
3 + # show a
4 echo a &&
5 + # show b
6 echo b
7 }
8 )
t/chainlint/case-comment.expect
+3
@@ -1,7 +1,10 @@
1 (
2 case "$x" in
3 + # found foo
4 x) foo ;;
5 + # found other
6 *)
7 + # treat it as bar
8 bar
9 ;;
10 esac
t/chainlint/close-subshell.expect
+2 -1
@@ -15,7 +15,8 @@
15 ) | wuzzle &&
16 (
17 bop
18 -) | fazz fozz &&
18 +) | fazz \
19 + fozz &&
20 (
21 bup
22 ) |
t/chainlint/comment.expect
+4
@@ -1,4 +1,8 @@
1 (
2 + # comment 1
3 nothing &&
4 + # comment 2
5 something
6 + # comment 3
7 + # comment 4
8 )
t/chainlint/double-here-doc.expect
+12 -2
@@ -1,2 +1,12 @@
1 -run_sub_test_lib_test_err run-inv-range-start "--run invalid range start" --run="a-5" <<-EOF &&
2 -check_sub_test_lib_test_err run-inv-range-start <<-EOF_OUT 3 <<-EOF_ERR
1 +run_sub_test_lib_test_err run-inv-range-start \
2 + "--run invalid range start" \
3 + --run="a-5" <<-\EOF &&
4 +test_expect_success "passing test #1" "true"
5 +test_done
6 +EOF
7 +check_sub_test_lib_test_err run-inv-range-start \
8 + <<-\EOF_OUT 3<<-EOF_ERR
9 +> FATAL: Unexpected exit with code 1
10 +EOF_OUT
11 +> error: --run: invalid non-numeric in range start: ${SQ}a-5${SQ}
12 +EOF_ERR
t/chainlint/empty-here-doc.expect
+2 -1
@@ -1,3 +1,4 @@
1 git ls-tree $tree path > current &&
2 -cat > expected <<EOF &&
2 +cat > expected <<\EOF &&
3 +EOF
4 test_output
t/chainlint/for-loop.expect
+3 -1
@@ -2,7 +2,9 @@
2 for i in a b c
3 do
4 echo $i ?!AMP?!
5 - cat <<-EOF ?!LOOP?!
5 + cat <<-\EOF ?!LOOP?!
6 + bar
7 + EOF
8 done ?!AMP?!
9 for i in a b c; do
10 echo $i &&
t/chainlint/here-doc-close-subshell.expect
+3 -1
@@ -1,2 +1,4 @@
1 (
2 - cat <<-INPUT)
2 + cat <<-\INPUT)
3 + fizz
4 + INPUT
t/chainlint/here-doc-indent-operator.expect
+8 -2
@@ -1,5 +1,11 @@
1 -cat > expect <<-EOF &&
1 +cat >expect <<- EOF &&
2 +header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
3 +num_commits: $1
4 +chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data
5 +EOF
6
3 -cat > expect <<-EOF ?!AMP?!
7 +cat >expect << -EOF ?!AMP?!
8 +this is not indented
9 +-EOF
10
11 cleanup
t/chainlint/here-doc-multi-line-command-subst.expect
+4 -1
@@ -1,5 +1,8 @@
1 (
2 - x=$(bobble <<-END &&
2 + x=$(bobble <<-\END &&
3 + fossil
4 + vegetable
5 + END
6 wiffle) ?!AMP?!
7 echo $x
8 )
t/chainlint/here-doc-multi-line-string.expect
+3 -1
@@ -1,5 +1,7 @@
1 (
2 - cat <<-TXT && echo "multi-line
2 + cat <<-\TXT && echo "multi-line
3 string" ?!AMP?!
4 + fizzle
5 + TXT
6 bap
7 )
t/chainlint/here-doc.expect
+21 -3
@@ -1,7 +1,25 @@
1 -boodle wobba gorgo snoot wafta snurb <<EOF &&
1 +boodle wobba \
2 + gorgo snoot \
3 + wafta snurb <<EOF &&
4 +quoth the raven,
5 +nevermore...
6 +EOF
7
8 cat <<-Arbitrary_Tag_42 >foo &&
9 +snoz
10 +boz
11 +woz
12 +Arbitrary_Tag_42
13
5 -cat <<zump >boo &&
14 +cat <<"zump" >boo &&
15 +snoz
16 +boz
17 +woz
18 +zump
19
7 -horticulture <<EOF
20 +horticulture <<\EOF
21 +gomez
22 +morticia
23 +wednesday
24 +pugsly
25 +EOF
t/chainlint/if-then-else.expect
+3 -1
@@ -8,7 +8,9 @@
8 echo foo
9 else
10 echo foo &&
11 - cat <<-EOF
11 + cat <<-\EOF
12 + bar
13 + EOF
14 fi ?!AMP?!
15 echo poodle
16 ) &&
t/chainlint/incomplete-line.expect
+8 -2
@@ -1,4 +1,10 @@
1 -line 1 line 2 line 3 line 4 &&
1 +line 1 \
2 +line 2 \
3 +line 3 \
4 +line 4 &&
5 (
3 - line 5 line 6 line 7 line 8
6 + line 5 \
7 + line 6 \
8 + line 7 \
9 + line 8
10 )
t/chainlint/inline-comment.expect
+2 -2
@@ -1,6 +1,6 @@
1 (
2 - foobar &&
3 - barfoo ?!AMP?!
2 + foobar && # comment 1
3 + barfoo ?!AMP?! # wrong position for &&
4 flibble "not a # comment"
5 ) &&
6
t/chainlint/loop-detect-status.expect
+1 -1
@@ -2,7 +2,7 @@
2 do
3 printf "Generating blob $i/$blobcount\r" >& 2 &&
4 printf "blob\nmark :$i\ndata $blobsize\n" &&
5 -
5 + #test-tool genrandom $i $blobsize &&
6 printf "%-${blobsize}s" $i &&
7 echo "M 100644 :$i $i" >> commit &&
8 i=$(($i+1)) ||
t/chainlint/nested-here-doc.expect
+25 -2
@@ -1,7 +1,30 @@
1 cat <<ARBITRARY >foop &&
2 +naddle
3 +fub <<EOF
4 + nozzle
5 + noodle
6 +EOF
7 +formp
8 +ARBITRARY
9
10 (
4 - cat <<-INPUT_END &&
5 - cat <<-EOT ?!AMP?!
11 + cat <<-\INPUT_END &&
12 + fish are mice
13 + but geese go slow
14 + data <<EOF
15 + perl is lerp
16 + and nothing else
17 + EOF
18 + toink
19 + INPUT_END
20 +
21 + cat <<-\EOT ?!AMP?!
22 + text goes here
23 + data <<EOF
24 + data goes here
25 + EOF
26 + more test here
27 + EOT
28 +
29 foobar
30 )
t/chainlint/nested-subshell-comment.expect
+2
@@ -2,6 +2,8 @@
2 foo &&
3 (
4 bar &&
5 + # bottles wobble while fiddles gobble
6 + # minor numbers of cows (or do they?)
7 baz &&
8 snaff
9 ) ?!AMP?!
t/chainlint/subshell-here-doc.expect
+24 -4
@@ -1,10 +1,30 @@
1 (
2 - echo wobba gorgo snoot wafta snurb <<-EOF &&
2 + echo wobba \
3 + gorgo snoot \
4 + wafta snurb <<-EOF &&
5 + quoth the raven,
6 + nevermore...
7 + EOF
8 +
9 cat <<EOF >bip ?!AMP?!
4 - echo <<-EOF >bop
10 + fish fly high
11 +EOF
12 +
13 + echo <<-\EOF >bop
14 + gomez
15 + morticia
16 + wednesday
17 + pugsly
18 + EOF
19 ) &&
20 (
7 - cat <<-ARBITRARY >bup &&
8 - cat <<-ARBITRARY3 >bup3 &&
21 + cat <<-\ARBITRARY >bup &&
22 + glink
23 + FIZZ
24 + ARBITRARY
25 + cat <<-"ARBITRARY3" >bup3 &&
26 + glink
27 + FIZZ
28 + ARBITRARY3
29 meep
30 )
t/chainlint/t7900-subtree.expect
+4
@@ -4,12 +4,16 @@ sub2
4 sub3
5 sub4" &&
6 chks_sub=$(cat <<TXT | sed "s,^,sub dir/,"
7 +$chks
8 +TXT
9 ) &&
10 chkms="main-sub1
11 main-sub2
12 main-sub3
13 main-sub4" &&
14 chkms_sub=$(cat <<TXT | sed "s,^,sub dir/,"
15 +$chkms
16 +TXT
17 ) &&
18 subfiles=$(git ls-files) &&
19 check_equal "$subfiles" "$chkms
t/chainlint/while-loop.expect
+3 -1
@@ -2,7 +2,9 @@
2 while true
3 do
4 echo foo ?!AMP?!
5 - cat <<-EOF ?!LOOP?!
5 + cat <<-\EOF ?!LOOP?!
6 + bar
7 + EOF
8 done ?!AMP?!
9 while true; do
10 echo foo &&