On Sat, Mar 25, 2023 at 4:51 AM Jeff King [off-list ref] wrote:

On Sat, Mar 25, 2023 at 04:41:08AM -0400, Jeff King wrote:

quoted

Right, the chainlint.pl one is much more thorough. I just wondered if
there were any cases we were worried about it missing, that the internal
one catches. We found one in this thread, but as discussed, it is not a
problem (presumably chainlint.pl catches a "real" case where an
earlier line is hidden by the "&", but I wouldn't mind seeing it
complain here as a matter of style/future-proofing).

Hmm, actually chainlint.pl does not seem to catch this:

-- >8 --
test_expect_success 'ok, first line cannot break &&-chain' '
        true &
        pid=$!
'

test_expect_success 'not ok, failure is lost' '
        false &&
        true &
        pid=$!
'
-- >8 --

Right, that's one of the "special cases" I mentioned earlier; an
intentional simplification of implementation to keep the complexity
level down. Although the linter is genuinely parsing the shell code,
it doesn't really understand or check shell semantics, and is just
using simple heuristics to detect the common types of &&-breakage and
missing `return 1`.

This particular simplification is that if it encounters one of the
special cases in which some construct (such as "&") should not be
considered as a break in the &&-chain, it clears all "??AMP??" flags
which come before that point in the current parse context.

More specifically, it's not even building a parse tree; it's just
trying to detect problems on-the-fly as it parses, so when it finds
something like "&" which is _not_ a breakage, it can't easily go back
and recheck which earlier "??AMP??" annotations are still needed. So,
it just clears the earlier ones unconditionally with the hope of not
letting too many false-negatives through. It would certainly be
possible to make it do a better job of detection, but doing so would
complicate the code quite a bit. (Eventually, I think it would be best
to build a parse tree, which would make it easier to incorporate other
linting ideas I have in mind, but I don't have any immediate plans to
do so.)

It's a little funny, because we actually background the whole "false &&
true" chain. So if you did "wait $pid" at the end, you would see the
failure. But the test in this thread doesn't actually do that (it
depends on kill after 2 seconds not finding the pid). Plus in general
this seems like an accident that we should be flagging.

As above, it was a judgement call regarding linter implementation
complexity versus letting a few potential breakages slip through
undetected. As it stands, we get a pretty big pay off detecting >99.9%
of real-world breakage without the additional complexity. (That is not
an argument against improving its accuracy in the future, but rather
an explanation of the current state of the linter.)