False positive for SC2154

[bje-]

For bugs

• Rule SC2154
• Version online
• [X] I tried on shellcheck.net and verified that this is still a problem on the latest commit

Here's a snippet or screenshot that shows the problem:

#!/bin/sh
trap 'ret=$?; exit $ret' 0

Here's what shellcheck currently says:

In foo.sh line 2:
trap 'ret=$?; exit $ret' 0
     ^-- SC2154: ret is referenced but not assigned.

Here's what I wanted or expected to see:

No warning.

[koalaman]

Indeed. This is due to the naive check added way back in #86. I'd love to remove this hack all together and encourage people to trap with functions, but it's probably more realistic to counter-kludge it to not warn in this case

[kurahaupo]

Or as noted in #839 just warn that

trap 'err=$? ; my_exit_func ; exit $err' EXIT

can be simplified to just

trap my_exit_func EXIT

since the EXIT trap saves $?.

[bje-]

Ping?

[koalaman]

Unfortunately this kind of indirect assignment&expansion continues to be a pain point without a great solution.

[kurahaupo]

@koalaman would having a specific warning that suggests changing ret=$? ; some_commands ; exit $ret to some_commands override and prevent this warning?

If not, would it be feasible simply to suppress all warnings inside the first arg of trap, and instead issue a generalized suggestion to use a named function?

Otherwise, perhaps the interior of a single-quoted trap string or single-quoted eval string could be treated like the body of a function?

[koalaman]

@kurahaupo I like this idea. It's somewhat rare but it does happen:

• https://github.com/tailscale/tailscale/blob/1007983159ca29ad1ba3ed8bc005ca8dc8b0f03a/logtail/example/logreprocess/demo.sh#L20
• https://github.com/minamarkham/formation/blob/70a331a4d440eb840a19f972602211ed9d18d374/slay#L7
• https://github.com/hyperledger/besu/blob/5e0301a749ac49a611a26c1df84eee5c0ee198f6/docker/tests/dgoss#L73

Plus some variations/attempts:

• https://github.com/hubotio/hubot/blob/7795fe53fb1a3dde85537589149cab0f0b58404a/bin/e2e-test.sh#L7
• https://github.com/rkt/rkt/blob/171c416fac02516604e48d2e65153dd55a12513e/Documentation/examples/build-container/play-example/build-play-example.sh#L13

And equally interesting, some places that use cmd; exit $? seemingly thinking it's the original exit code:

• https://github.com/LiteOS/LiteOS/blob/2f8fdf9c444339262f03f61f6652af2d8a4b4255/tests/cmockery/packages/rpm.sh#L42
• https://github.com/babun/babun/blob/b5c01c4f9d2cf7e2b01756e78d26a62530ff2f6b/babun-core/tools/procps.sh#L14

[kurahaupo]

Aaargh, those references are depressing! (So much crummy code, so little lifetime left to complain about it.)

Those latter two are "special": if the cleanup succeeds then explicitly exit using an (implicitly calculated) exit status of signal+128, otherwise exit implicitly with the same exit code. I wonder what they thought they were achieving.

The more often I see things like this, the more I wish that Bash would set $? to a number outside the range 0~255 when indicating a signal. Any 256*n+128 other than n=0 would suffice, but my personal preference would be n=-1, and then treat exit $n as kill -$((n&127)) $$ when n<0.

Perhaps I should write a patch to implement shopt -s signal_exit?

Hmm, a new raise built-in would be nice, with a default of SIGABRT rather than SIGTERM, that takes an optional signal number (and truncates to 7 bits).