Re: [PATCH] chainlint.pl: recognize test bodies defined via heredoc
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jul 8, 2024, 03:40 UTC
- Message-ID
- <CAPig+cRUOSHuNezZfVUT43JGT5BWA3LTbQvKCkD37DxXn=L6Jg@mail.gmail.com>
- In-Reply-To
- <20240706060143.GD698153@coredump.intra.peff.net>
On Sat, Jul 6, 2024 at 2:01 AM Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
> On Tue, Jul 02, 2024 at 07:50:34PM -0400, Eric Sunshine wrote: > I'll post some patches in a moment: > > [1/3]: chainlint.pl: fix line number reporting > [2/3]: t/chainlint: add test_expect_success call to test snippets > [3/3]: t/chainlint: add tests for test body in heredoc > > with the idea that we'd apply your patch here on top of what Junio has > queued in jk/test-body-in-here-doc, and then these three on top. For > Junio's sanity, I'll roll it all up into one series. But I wanted to > show it to you incrementally first, especially because I think the fixes > from patch 1/3 above should probably just get squashed in (or even > rewritten). I'll discuss the bugs they fix below.
Considering the excellent explanation you crafted in your patch, I'd like to say that it should remain separate from mine. However, since you caught the problems in review, it would be irresponsible of us to let my patch into the permanent history as-is. So, feel free to squash your fixes into my patch. Perhaps add a Co-authored-by:? The bit from your [1/3] commit message about incrementing $lineno for the heredoc-body case might be worth squashing in too?
I wrote one minor (perhaps non-actionable) comment in response to patch [3/3]. The patches all looked fine to me, so:
Acked-by: Eric Sunshine <sunshine@sunshineco.com>
Show 16 quoted lines
> > @@ -232,7 +234,8 @@ sub new {
> > my $self = bless {
> > buff => [],
> > stop => [],
> > - output => []
> > + output => [],
> > + heredocs => {},
> > } => $class;
> > $self->{lexer} = Lexer->new($self, $s);
> > return $self;
>
> I think initializing is not strictly necessary here, since we'd only try
> to read tags if we saw a here-doc. But there might be some invalid cases
> where we could convince higher-level code to look for tags even though
> there were none (and generate a perl warning about trying to dereference
> undef as a hashref).You're right, it's not necessary to initialize here, but it feels more consistent to do so. That said, I don't feel strongly either way.
Show 5 quoted lines
> On the flip side, what about cleaning up? The "heretags" array is
> emptied as we parse the heredocs in swallow_heredocs(). But I think once
> a ShellParser's $self->{heredocs}->{FOO} is written, it will hang around
> forever (even though it's valid only for that one command). Probably not
> a big deal, but there's probably some correct spot to reset it.There are a few reasons I wasn't overly concerned about cleaning up in this case:
(1) The parsers are all short-lived, so the collected heredoc bodies won't stick around long anyhow. For each test checked, a TestParser is created and destroyed. For each script mentioned on the command-line, a ScriptParser is created and destroyed. None of these parsers stick around for long, though, a ScriptParser outlives a TestParser.
(2) The heredoc bodies in question tend to be pretty small, so it's not consuming an inordinate amount of memory even if a single parser latches bodies of multiple heredocs.
(3) We tend to be quite consistent about naming our heredoc tag (i.e. "EOF", "EOT"), so a latched body in the parser's %heredocs hash is very likely to get overwritten, thus the hash is probably not going to eat up a lot of memory. Given the entire test suite, I'd be quite surprised if any one parser ever latches more than three heredoc bodies at a time, and the vast majority of parsers are likely latching zero or one heredoc body.
(4) I couldn't really think of a correct spot to reset %heredocs.
That said, after reading your message, I did try implementing an approach in which the heredoc body gets attached to the `<<` or `<<-` token. That way, a heredoc body would be cleaned along with its associated lexer token. However, the implementation got too ugly and increased cognitive load too much for my liking, so I abandoned it.
Show 16 quoted lines
> > sub check_test {
> > my $self = shift @_;
> > - my ($title, $body) = map(unwrap, @_);
> > + my $title = unwrap(shift @_);
> > + my $body = unwrap(shift @_);
> > + $body = shift @_ if $body eq '-';
> > $self->{ntests}++;
> > my $parser = TestParser->new(\$body);
> > my @tokens = $parser->parse();
>
> This has two problems related to line numbers. You can't see it in the
> context, but we later do:
>
> my $lineno = $_[1]->[3];
>
> Now that we're shifting @_, that array item is gone.Ugh, this is embarrassing. I did run chainlint.pl on t0600 in which I had intentionally broken some &&-chains, so I saw the output, but somehow I overlooked that it broke the line numbering entirely.
Show 7 quoted lines
> The second is that the line number for the here-doc is actually one past > the initial line number of the test_expect_success. That works > automatically for hanging single-quotes, since the newline from that > line is inside the quoted area. But for a here-doc, we have to account > for it manually. In my original patch I prepended "\n", but you can also > just increment $lineno (which is what I did in the fix I'm about to > send).
Nicely spotted. Simply incrementing $lineno does feel a bit hacky, but I agree that it is probably good enough for now; it doesn't seem likely that it will break any time soon. But I also agree with the commentary you wrote in patch [1/3] that it probably would be easy enough to latch the line number of the beginning of the heredoc body and employ that value. That would certainly be more robust.
Show 25 quoted lines
> > @@ -649,8 +654,13 @@ sub parse_cmd {
> > return @tokens unless @tokens && $tokens[0]->[0] =~ /^test_expect_(?:success|failure)$/;
> > my $n = $#tokens;
> > $n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\|)$/;
> > - $self->check_test($tokens[1], $tokens[2]) if $n == 2; # title body
> > - $self->check_test($tokens[2], $tokens[3]) if $n > 2; # prereq title body
> > + my $herebody;
> > + if ($n >= 2 && $tokens[$n-1]->[0] eq '-' && $tokens[$n]->[0] =~ /^<<-?(.+)$/) {
> > + $herebody = $self->{heredocs}->{$1};
> > + $n--;
> > + }
> > + $self->check_test($tokens[1], $tokens[2], $herebody) if $n == 2; # title body
> > + $self->check_test($tokens[2], $tokens[3], $herebody) if $n > 2; # prereq title body
> > return @tokens;
> > }
>
> OK, mostly as expected. I think the check for "-" here is redundant with
> what's in check_test(). We could just feed the heredoc body either way,
> and in the nonsense case of:
>
> test_expect_success 'title' 'test body' <<EOT
> nobody reads this!
> EOT
>
> the heredoc data would just be ignored.Right. I went back and forth with this, never sure if this code was overkill. On the other hand, we could make this more paranoid and complain if we see either of these cases:
(1) "-" but no heredoc (2) heredoc present but something other than "-"
Show 5 quoted lines
> Requiring "<<" at the end is somewhat limiting. E.g. this is valid: > > test_expect_success <<EOT 'title' - > the test body > EOT
True, I didn't even think about that.
Show 5 quoted lines
> I don't expect anybody to do that, but it would be nice to be more > robust if we can. I think the tokens are still wrapped at this point, so > we could read through all of them looking for "<<" anywhere, without > getting confused by "$(cat <<INNER_HEREDOC)". I think, anyway (I didn't > test).
Correct. The stuff inside "$(...)" does get parsed and linted, but by the time ScriptParser::parse_cmd() sees it, `$(cat <<INNER_HEREDOC)` is just a single (string) token.