Re: [PATCH 1/2] chainlint: make error messages self-explanatory

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 29, 2024, 10:03 UTC
Message-ID: <ZtBHbftK7vdTEz93@tanuki>
In-Reply-To: <20240829091625.41297-2-ericsunshine@charter.net>

On Thu, Aug 29, 2024 at 05:16:24AM -0400, Eric Sunshine wrote:

Show 21 quoted lines

> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> 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]

Show 22 quoted lines

> 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?

Show 14 quoted lines

> @@ -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.

Show 22 quoted lines

> @@ -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