On Mon, Oct 24, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
> On Tue, Sep 13 2022, Eric Sunshine via GitGitGadget wrote:
> > When `chainlint.pl` detects problems in a test definition, it emits the
> > test definition with "?!FOO?!" annotations highlighting the problems it
> > discovered. For instance, given this problematic test:
> >
> > test_expect_success 'discombobulate frobnitz' '
> > (echo balderdash; echo gnabgib) >expect &&
> > '
> >
> > chainlint.pl will output:
> >
> > # chainlint: t1234-confusing.sh
> > # chainlint: discombobulate frobnitz
> > (echo balderdash ; ?!AMP?! echo gnabgib) >expect &&
>
> I've noticed that chainlint.pl is better in some ways, but that the
> "deparse" output tends to be quite jarring. but I can't find version of
> it that emitted this "will output" here.
There is no such version.
> Before this patch, or fb41727b7ed (t: retire unused chainlint.sed,
> 2022-09-01) we'd emit this instead:
>
> ( echo balderdash ; ?!AMP?! echo gnabgib ) > expect &&
>
> The difference is in whitespace, e.g. "( ", not "(", "> " not ">". This
> is just because it's emitting "raw" tokenizer tokens.
>
> Was there maybe some local version where the whitespace munging you're
> doing against $checked was different & this commit message was left
> over?
No, I botched the commit message. I typed the example test in by hand
and then, also by hand, typed in the example output, forgetting to
insert the spaces which you correctly noted are missing from the
example output. I should have run the example test through
chainlint.pl and copy/pasted its output into the commit message. (I
did, in fact, run the sample test through chanlint.pl _after_
hand-typing the example output, and compared them by eye but missed
most of the whitespace differences.)
> Anyway, that sort of an aside, but I did go hunting for the version with slightly better whitespace output.
Sorry, my fault for a faulty commit message.
> But to get to the actual point: I've found the new chainlint.pl output
> harder to read sometimes, because it goes through this parse & deparse
> state, so you're preserving "\n"''s.
>
> Whereas the old "sed" output also sucked because we couldn't note where
> the issue was, but we spewed out the test source verbatim.
Somewhat verbatim. chainlint.sed did swallow blank lines and comment
lines, and it folded multi-line strings into one-line strings.
> But it seem to me that we could get much better output if the
> ShellParser/Lexer etc. just kept enough state to emit "startcol",
> "endcol" and "linenum" along with every token, or something like that
> (you might want offsets from the beginning of the parsed source
> instead).
>
> Then when it has errors it could emit the actual source passed in, and
> even do gcc/clang-style underlining.
>
> I poked at getting that working for a few minutes, but quickly saw that
> someone more familiar with the code could do it much quicker, so
> consider the above a feature request :)
Yes, there should be better integration between the lexer and parser
for emitting errors. Unfortunately, it didn't occur to me during
implementation, and I only thought about it when Peff mentioned the
difficult-to-read output in a different part of this discussion.
An alternative, somewhat hacky approach, might be to simply retain
whitespace as tokens in the token stream. That would require less
retrofitting of the lexer, though perhaps more complexity/ugliness in
the parser. It wouldn't give you gcc/clang-level underlining, etc.,
but would more or less preserve whitespace in the test definition.
Definitely not a proper solution, but perhaps "good enough".
> Another thing: When a test *ends* in a "&&" (common when you copy/paste
> e.g. "test_cmp expect actual &&\n" from another test) it doesn't spot
> it, but instead we get all the way to the eval/117, i.e. "broken
> &&-chain or run-away HERE-DOC".
Yes, I recall considering that case and others, but decided that
that's probably outside the scope of the linter. In particular, a
trailing "&&" is a plain old syntax error, and the shell itself is
perfectly capable of diagnosing that problem along with all other
syntax errors, and you'll find out about syntax errors in your code
when the shell tries running it. The linter, on the other hand, is
meant to catch semantic problems (per the project's best-practices) in
what is assumed to be syntactically valid shell code. I suppose the
linter could be made to complain about this syntax error and others,
but it seems unnecessary to bloat it by duplicating behavior already
provided by the shell itself.
It is unfortunate, though, that the shell's "syntax error" output gets
swallowed by the eval/117 checker in test-lib.sh and turned into a
somewhat less useful message. I'm not quite sure how we can fix the
eval/117 checker to not swallow genuine syntax errors like that,
unless we perhaps specially recognize exit code 2 and, um, do
something...
> More feature requests (because for some reason you've got infinite time,
> but I don't :): This software is really close to being able to also
> change the tests on the fly. If you could define callbacks where you
> could change subsets of the parse stream, say a single command like:
>
> grep some.*rx file
>
> So we could rewrite that into:
>
> if ! grep some.*rx foo
> then
> cat foo &&
> false
> fi
>
> And other interesting auto-fixups and borderline coccinelle
> transformations, e.g. changing our various:
>
> test "$(git ...) = "" &&
>
> Into:
>
> git ... >out &&
> test_must_be_empty out
The lexer/parser implemented for chainlint.pl might indeed be useful
for such transformations. I could imagine a tool which someone runs on
an old-style test script to help update it to modern conventions,
after which the person would, of course, carefully check all the
applied transformations. That's not something we'd necessarily want to
do project-wide, but might be handy when already working on a test
script for some other reason.
