Re: [PATCH 1/2] chainlint: make error messages self-explanatory
From: Patrick Steinhardt <hidden>
Date: 2024-08-29 10:03:39
On Thu, Aug 29, 2024 at 05:16:24AM -0400, Eric Sunshine wrote:
From: Eric Sunshine <redacted> The annotations emitted by chainlint to indicate detected problems are overly terse, so much so that developers new to the project -- those who should most benefit from the linting -- may find them baffling. For instance, although the author of chainlint and seasoned Git developers may understand that "?!AMP?!" is an abbreviation of "ampersand" and indicates a break in the &&-chain, this may not be obvious to newcomers. Similarly, although the annotation "?!LOOP?!" is understood by project regulars to indicate a missing `|| return 1` (or `|| exit 1` in a subshell), newcomers may find it more than a little perplexing. The "?!LOOP?!" case is particularly serious since it is likely that some newcomers are unaware that shell loops do not terminate automatically upon error, and it is more difficult for a newcomer to figure out how to correct the problem by examining surrounding code since `|| return 1` appears in test scrips relatively infrequently (compared, for instance, with &&-chaining). Address these shortcomings by emitting human-consumable messages which both explain the problem and give a strong hint about how to correct it.
A worthwhile goal indeed. As you say, especially figuring out how to fix the loop annotations is not exactly straight forward. [snip]
quoted hunk ↗ jump to hunk
diff --git a/t/chainlint.pl b/t/chainlint.pl index 5361f23b1d..d79f183dfd 100755 --- a/t/chainlint.pl +++ b/t/chainlint.pl@@ -9,7 +9,7 @@ # Input arguments are pathnames of shell scripts containing test definitions, # or globs referencing a collection of scripts. For each problem discovered, # the pathname of the script containing the test is printed along with the test -# name and the test body with a `?!FOO?!` annotation at the location of each +# name and the test body with a `?!ERR?!` annotation at the location of each # detected problem, where "FOO" is a tag such as "AMP" which indicates a broken # &&-chain. Returns zero if no problems are discovered, otherwise non-zero.@@ -181,7 +181,7 @@ sub swallow_heredocs { $self->{lineno} += () = $body =~ /\n/sg; next; } - push(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]); + push(@{$self->{parser}->{problems}}, ['HEREDOC', $tag]); $$b =~ /(?:\G|\n).*\z/gc; # consume rest of input my $body = substr($$b, $start, pos($$b) - $start); $self->{lineno} += () = $body =~ /\n/sg;
I was wondering why this is being changed here, as I found the old name to be easier to understand. Then I saw further down that you essentially use those as identifiers for the actual problem. Is there a specific reason why we now have the separate translation step? Couldn't we instead push the translated message here, directly?
quoted hunk ↗ jump to hunk
@@ -296,8 +297,11 @@ sub parse_group { sub parse_subshell { my $self = shift @_; - return ($self->parse(qr/^\)$/), - $self->expect(')')); + $self->{insubshell}++; + my @tokens = ($self->parse(qr/^\)$/), + $self->expect(')')); + $self->{insubshell}--; + return @tokens; } sub parse_case_pattern {
Okay. The subshell recursion level tracking here is required such that we can discern LOOPEXIT vs LOOPRETURN cases. Makes sense.
quoted hunk ↗ jump to hunk
@@ -641,7 +654,8 @@ sub check_test { for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) { my ($label, $token) = @$_; my $pos = $token->[2]; - $checked .= substr($body, $start, $pos - $start) . " ?!$label?! "; + my $err = format_problem($label, $token); + $checked .= substr($body, $start, $pos - $start) . " ?!ERR $err?! "; $start = $pos; } $checked .= substr($body, $start);diff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect index 338ecd5861..2efd65dcbd 100644 --- a/t/chainlint/arithmetic-expansion.expect +++ b/t/chainlint/arithmetic-expansion.expect@@ -4,6 +4,6 @@ 5 baz 6 ) && 7 ( -8 bar=$((42 + 1)) ?!AMP?! +8 bar=$((42 + 1)) ?!ERR missing '&&'?! 9 baz 10 )
I find the resulting error messages a bit confusing: to me it reads as if "ERR" is missing the ampersands. Is it actually useful to have the ERR prefix in the first place? We do not output anything but errors, so it feels somewhat redundant. Patrick