bvanassche commented 1 year ago

Rule id: SC2320 Version: https://github.com/koalaman/shellcheck/releases/download/stable/shellcheck-stable.linux.x86_64.tar.xz from 2023-01-11.

This is documented on https://www.shellcheck.net/wiki/SC2320. However, I still think that this should be fixed.
[X] I tried on https://www.shellcheck.net/ and verified that this is still a problem on the latest commit

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

git clone https://github.com/osandov/blktests.git
make check

Here's what shellcheck currently says:

2023-01-12T00:02:31.9746941Z common/multipath-over-rdma:134:24: warning: This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten. [SC2319]
2023-01-12T00:02:31.9747763Z common/null_blk:47:39: warning: This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. [SC2320]
2023-01-12T00:02:31.9748468Z common/null_blk:54:43: warning: This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. [SC2320]
2023-01-12T00:02:31.9749603Z tests/nvmeof-mp/rc:75:42: warning: This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. [SC2320]
2023-01-12T00:02:31.9750371Z tests/nvmeof-mp/rc:118:26: warning: This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten. [SC2319]
2023-01-12T00:02:31.9751192Z tests/srp/rc:99:42: warning: This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. [SC2320]
2023-01-12T00:02:31.9751829Z tests/srp/rc:105:44: warning: This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. [SC2320]
2023-01-12T00:02:31.9752642Z tests/srp/rc:263:26: warning: This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten. [SC2319]
2023-01-12T00:02:31.9753540Z tests/srp/rc:523:12: warning: This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. [SC2320]
2023-01-12T00:02:31.9754272Z tests/zbd/rc:83:14: note: $/${} is unnecessary on arithmetic variables. [SC2004]
2023-01-12T00:02:31.9754769Z tests/zbd/rc:85:14: note: $/${} is unnecessary on arithmetic variables. [SC2004]
2023-01-12T00:02:31.9755426Z tests/zbd/rc:87:13: note: $/${} is unnecessary on arithmetic variables. [SC2004]
2023-01-12T00:02:31.9755908Z tests/zbd/rc:88:13: note: $/${} is unnecessary on arithmetic variables. [SC2004]
2023-01-12T00:02:31.9756542Z tests/zbd/rc:89:13: note: $/${} is unnecessary on arithmetic variables. [SC2004]
2023-01-12T00:02:31.9757016Z tests/zbd/rc:95:14: note: $/${} is unnecessary on arithmetic variables. [SC2004]
2023-01-12T00:02:31.9757656Z tests/zbd/rc:97:14: note: $/${} is unnecessary on arithmetic variables. [SC2004]
2023-01-12T00:02:31.9758197Z tests/zbd/rc:128:10: warning: This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. [SC2320]
2023-01-12T00:02:31.9758995Z tests/srp/014:57:26: warning: This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten. [SC2319]
2023-01-12T00:02:31.9759596Z tests/srp/014:69:26: warning: This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten. [SC2319]

Here's what I wanted or expected to see:

None of the above messages. I think all of the above messages are either not useful or indicate a bug in shellcheck.

TriMoon commented 1 year ago

Just like any warnings in any computer language, they are not errors...

If you're sure it works as intended you can place disable comments in the code or in the options for shellcheck... It is just helping devs to code better...

Note:

No i have not cloned the repo to check the files... If you want them checked provide a link to the repo instead of a clone URL...

Those about $? (SC2320) are proof of bad coding, which you should correct...

https://github.com/osandov/blktests/blob/master/common/multipath-over-rdma#L134 Does this code have any chance of returning different return codes at all? :thinking: :rofl:
```
[ -n "$1" ] || return $?
```
I'm sure the rest are similar... :rofl:

https://github.com/osandov/blktests/issues/106#issue-1533930432

bvanassche commented 1 year ago

Thanks for having taken the time to provide a detailed reply. However, it is not clear to me why [ ... ] || return $? is considered bad style? My view is that [ ... ] || return $? is idiomatic in shell code.

The URL of the blktests project is https://github.com/osandov/blktests.

The blktests project has adopted a "zero shellcheck warnings" policy.

There are 152 occurrences of " || return $?" in the blktests project. Changing all occurrences would require a significant effort.

austin987 commented 1 year ago

https://www.shellcheck.net/wiki/SC2320

port19x commented 1 year ago

Thanks for having taken the time to provide a detailed reply. However, it is not clear to me why [ ... ] || return $? is considered bad style? My view is that [ ... ] || return $? is idiomatic in shell code.

The URL of the blktests project is https://github.com/osandov/blktests.

The blktests project has adopted a "zero shellcheck warnings" policy.

There are 152 occurrences of " || return $?" in the blktests project. Changing all occurrences would require a significant effort.

It's bad style because it references the previous result and introduces coupling.

If we have some code, as in the wiki page, like the following

mycommand
echo "Command exited with $?"

And we now, in the process of development or refactoring, switch up our structure

mycommand
myfailingcommand
echo "Command exited with $?"

We get unexpeced behaviour, where as with something more idiomatic like

mycommand
A=$?
myfailingcommand
echo "Command exited with $A"

We at least somewhat decouple the initial command from the next check.

It's kinda bad either way. || and && exist. But if you have to do it, don't shoot yourself in the foot any harder than you have to.

I initially confused this with SC2312, which may be more enlightening to you

TriMoon commented 1 year ago

@bvanassche

My view is that [ ... ] || return $? is idiomatic in shell code.

Do you actually understand what it does, step-by-step? :thinking: W all understand what the human means here, but how does bash process that line? If you understand THAT you will understand that the $? used here is completely "bad-code"... It should use return ### (xxx being any number, 0 <= xxx <= 255), but never $?.

Changing all occurrences would require a significant effort.

Fixing bad code always requires efforts, the magnitude is just related to the amount of bad coding in the first place. But it makes the code-base much better and prevents bugs that are hard, to impossible, to find if not fixed.

People think that writing shorter (cryptic) code makes it faster, which is just not true. outside of direct assembler code... Interpreted languages convert the code in the same final code for the CPU no matter if you use cryptic code like in the example we talk about or use the longer 3 line version... eg: these are same for the final code provided to the CPU:

[ ... ] || return xxx

if test ....; then
  return xxx
fi

But as you can see this is WAY more human friendly to read for the human, with less opportunity to create a bug with.

oliv3r commented 1 year ago

For SC2312 particularly, I've opened #2775

koalaman / shellcheck

Shellcheck incorrectly complains about echo and printf commands #2663

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

Here's what shellcheck currently says:

Here's what I wanted or expected to see:

Note: