Add a check for checking return code after a `local` or `declare` line

[thegranddesign]

Given:

declare foo="$(some_script_that_fails_with_exit_code_200)"

[ $? == 0 ] && echo 'yes'

Will always echo yes because the return code is the code from the declare command and not from the subshell. The proper way to write this would be:

declare foo

foo="$(some_script_that_fails_with_exit_code_200)"

[ $? == 0 ] && echo 'yes'

It would be great if there was a check that could catch this.

[brother]

It does warn about that.

"SC2155: Declare and assign separately to avoid masking return values."

https://github.com/koalaman/shellcheck/wiki/SC2155

[thegranddesign]

Ah, I didn't see that. Does it throw it all the time or just when it detects that you need to?

Splitting all variable assignment from declaration is overkill. If it does it all the time I may have completely disabled that check in vim.

Lemme take a look.

[thegranddesign]

@brother yeah, I disabled this check. It's unneeded 90% of the time. It should throw an error only if, in the following line, you're checking the return value somehow.

[thegranddesign]

Otherwise you just get tons of line noise:

declare foo; foo="blah";

# and

declare foo;

foo="blah";

[kurahaupo]

The proper way to write this would be:

declare foo

foo="$(some_script_that_fails_with_exit_code_200)"

[ $? == 0 ] && echo 'yes'

Ahem, I think you will find that Shellcheck recommends AGAINST that
expression, on two counts.

Firstly the '==' test is not POSIX, and we assume you're using [ ] instead
of [[ ]] because you care about POSIX.

Secondly, NEVER test for $? just against zero; that's vacuous line noise for
no good reason.

Instead use either

  if foo="$(some_script_that_fails_with_nonzero_exit_code)"
  then
    echo yes
  fi

or

  foo="$(some_script_that_fails_with_nonzero_exit_code)" &&
  echo yes

Only use $? if you need to keep it for later use, or to make a distinction
other than zero/nonzero. (And if you're doing the latter, take care that $?
doesn't change half way through doing your test -- so for safety, treat it
as the first case: stash it for later use, and then do whatever combination
of tests you actually want. And this turns out to be handy when you want to
print out debug or trace messages.)

[thegranddesign]

@kurahaupo

Firstly, (Ahem) that was a basic code snippet to demonstrate the behavior from shellcheck I'm looking at. It's not supposed to be held up at some pristine code example. 😉

Secondly, I hate putting assignments in my if statements. I put all of my assignments together as much as possible and then put all of my logic together.

I do always use the style you demonstrated (without the assignment) if all I want is to base the conditional on the return code, but if I want the value as well, I assign first and then do the logic later.

[pikrzysztof]

So why am I not warned about this code?

#!/usr/bin/env bash
set -eu
f() {
        local x="$5"  # << should warn here to say that local will mask bad assignment
        echo "$x"
}
f 1

I get a warning when I run f instead of f 1, but

[kurahaupo]

@pikrzysztof depending on what you mean by "bad assignment" there are two responses, but both have the same outcome.

x=$5 can never have a non-zero exit status, so there's nothing for local to hide.

When set -u is in effect and an unset parameter is expanded, the Shell exits before the assignment is done (which is before local is invoked), so again, it can't hide anything.

[pikrzysztof]

@kurahaupo my bad, you're right. Thanks for explaining!