False positive SC2030/SC2031 in 0.7.2?

[Stikus]

For bugs

• Rule Id (if any, e.g. SC1000): SC2030/SC2031
• My shellcheck version (shellcheck --version or "online"): 0.7.2
• [x] The rule's wiki page does not already cover this (e.g. https://shellcheck.net/wiki/SC2031)
• [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:

#!/bin/bash

test1() {
     echo "Test" | if [[ -n "$LOGFILE" ]]; then tee -a "$LOGFILE"; else cat; fi
}

test2() {
        echo "ERROR" | tee -a "$LOGFILE"
}

Here's what shellcheck currently says:

$ shellcheck myscript

Line 4:
     echo "Test" | if [[ -n "$LOGFILE" ]]; then tee -a "$LOGFILE"; else cat; fi
                             ^-- SC2030: Modification of LOGFILE is local (to subshell caused by pipeline).

Line 8:
        echo "ERROR" | tee -a "$LOGFILE"
                               ^-- SC2031: LOGFILE was modified in a subshell. That change might be lost.

Here's what I wanted or expected to see:

Nothing as in 0.7.1 shellcheck.

Everything was ok before last update. Can you check please?

[ljharb]

I'm also seeing the same false positive with -z, new in v0.7.2 - also in for ALIAS_PATH in "${NVM_ALIAS_DIR}/lts/${ALIAS}"*; do on the "ALIAS" variable.

[chaimleib]

I'm also on v0.7.2, seeing false positives with SC2030/SC2031 on this script:

#!/bin/bash
images=
for arg in "$@"; do
  case "$arg" in
    --images)
      images=y
      ;;
  esac
done
echo "Doing stuff${images:+ with images }..."
{
  echo 'stuff'
  if [ -n "$images" ]; then
    echo 'images'
  fi
} | cat
echo "Done with stuff${images:+ and images}"

There should be no errors. Instead, I see:

In SC2031.sh line 13:
  if [ -n "$images" ]; then
           ^-----^ SC2030: Modification of images is local (to subshell caused by pipeline).

In SC2031.sh line 17:
echo "Done with stuff${images:+ and images}"
                     ^--------------------^ SC2031: images was modified in a subshell. That chan
ge might be lost.

For more information:
  https://www.shellcheck.net/wiki/SC2030 -- Modification of images is local (...
  https://www.shellcheck.net/wiki/SC2031 -- images was modified in a subshell...

I'm also confused why the "Done" message triggers an error, but not the "Doing" message.

[chaimleib]

Oh. Maybe it mis-categorizes [ -n "$images" ] as a variable modification?

[odkr]

I get the same error when I use ${var-} in v0.7.2 and on https://www.shellcheck.net/.

For example, given a shellscript no-mod.sh:

#!/bin/sh
readonly GLOBAL='I cannot be changed.'
# shellcheck disable=2024,2234
sub() ( [ "${GLOBAL-}" ]; )
: "$GLOBAL"

Then calling shellcheck no-mod.sh prints:

[ "${GLOBAL-}" ]
   ^--------^ SC2030: Modification of GLOBAL is local (to subshell caused by (..) group).

In no-mod.sh line 10:
: "$GLOBAL"
   ^-----^ SC2031: GLOBAL was modified in a subshell. That change might be lost.

[bland328]

I'm also seeing unexpected SC2030/2031 warnings in peculiar situations. Here's some Bash code which demonstrates the problem using www.shellcheck.net.

This code...

#!/bin/bash
x=3
echo "test message$( [[ $x ]] && echo -n " (x == '$x')" )"
echo "x == '$x'"

...results in these inappropriate warnings, since x is not modified after it is first declared:

Line 3:
echo "test message$( [[ $x ]] && echo -n " (x == '$x')" )"
                        ^-- SC2030: Modification of x is local (to subshell caused by $(..) expansion).

Line 4:
echo "x == '$x'"
            ^-- SC2031: x was modified in a subshell. That change might be lost.

Curiously, both warnings disappear when the final echo is removed, like this:

#!/bin/bash
x=3
echo "test message$( [[ $x ]] && echo -n " (x == '$x')" )"

[TinCanTech]

Curiously, both warnings disappear when the final echo is removed, like this:

The subshell modification is no longer in effect because you are no longer referencing x

[bland328]

@TinCanTech, I certainly wouldn't argue with that logic. However, my primary point that is there is no subshell modification of × in the first place. Or, perhaps subshell modification of x is somehow implied, but if that's the case, I can't spot it.

Do the SC2030/2031 warnings for the code I posted make sense to you?

[TinCanTech]

The subshell is: "test message$( [[ $x ]] && echo -n " (x == '$x')" )" ______________^ right there Perhaps shellcheck is misinterpreting it somewhat ..

Honestly, I don't know if it is actually possible for shellcheck to do bash batter than bash does ..

[bland328]

@TinCanTech, I didn't mean to imply there's no subshell in the sample code I posted; there's absolutely a subshell in there.

The problem I'm reporting is that shellcheck is improperly warning of a modification of x within the subshell, though x is only referenced within the subshell.

[TinCanTech]

x is referenced in a subshell .. shellcheck noticed .. and you got warned.

What are you expecting shellcheck to do ?

[bland328]

@TinCanTech, I appreciate you taking the time and effort to contribute, but there's obviously some fundamental misunderstanding going on here that I'll attempt to clear up, with apologies if you already grasp some or all of this.

x is referenced in a subshell .. shellcheck noticed .. and you got warned.

Yes, but there's nothing wrong with referencing a pre-existing variable from a subshell--it's standard procedure. For example, shellcheck doesn't warn about this:

x=1623974553
y="$( date --date="@$x" )"
echo "$y"

That said, modifying a variable within a subshell is a different story: while perfectly legal and often useful, it is also often problematic, because that modified variable exists solely within the scope of the subshell; that is, after the subshell exits, no modifications to variables made within the subshell "bubble up" to the parent shell. This can be a confounding result, particularly for novice Bash coders, which is why warning SC2030 exists.

However, SC2030 doesn't apply to the sample code I provided in any way of which I can conceive because in that code no modifications to any variables are made within the subshell.

What are you expecting shellcheck to do ?

I'm hoping for shellcheck to not warn about modifications to variables within subshells where no such modifications exist.

[ljharb]

In other words, this false positive is worse than no check at all, because it trains developers to override warnings.

[TinCanTech]

@bland328 I do appreciate your point-of-view .. however

shellcheck gave you a valid warning but it did not re-interpret your code.

The reason that echo x spewed a warning is because you already used x in a subshell, shellcheck worked.

if you are confident that this is something you can ignore then simply ignore it.

[TinCanTech]

@ljharb then simply allow the warning to persist .. until you, the developer, decide how to treat it.

Would you prefer that shellcheck no longer warn you at all ? then why use shellcheck at all ?

[TinCanTech]

In other words, this false positive is worse than no check at all, because it trains developers

"trains developers" .. brevity forbids my reply ..

I guess, should let google run things or let people be responsible

[chaimleib]

The warning would have been valid and useful if this were the program:

#!/bin/bash
x=3
echo "test message$( x=0; echo -n " (x == '$x')" )"
echo "x == '$x'"

A novice dev might forget that on the last line, x=0 gets forgotten, and x=3 again.

But in the original program, there was no assignment in the subshell, and the same confusion doesn’t exist. This should give no error:

#!/bin/bash
x=3
echo "test message$( [[ $x ]] && echo -n " (x == '$x')" )"
echo "x == '$x'"

[TinCanTech]

A novice dev might forget that

shellcheck is aimed at that target audience ..

[chaimleib]

The problem is that the error message on the original program is not just useless, it’s incorrect.

Line 3:
echo "test message$( [[ $x ]] && echo -n " (x == '$x')" )"
                        ^-- SC2030: Modification of x is local (to subshell caused by $(..) expansion).

Nowhere on line 3 is x getting modified. There is no “modification of x” there. There is a read, but no write to x. For the same reason, the next warning is also in error:

Line 4:
echo "x == '$x'"
            ^-- SC2031: x was modified in a subshell. That change might be lost.

[ljharb]

@TinCanTech yes, "no warning at all" is always utterly preferable over "a warning that is wrong"

[TinCanTech]

The warning is not wrong, x was used in a subshell and therefore its value is in doubt.

[Stikus]

Before version 0.7.2 everything was ok, now it's not. This is a bug, I don't know what to talk about.

[bland328]

@ljharb and @Stikus, agreed on all points.

And in case it helps anyone chasing this bug, I've noticed that both the POSIX-style (single-bracket) test and the Bash-style (double-bracket) tests now improperly inspire SC2030, but the C-style (double-parenthesis) arithmetic test does not:

#!/bin/bash
flag=1
#word="foo$( [ $flag ] && echo bar )"    # improper SC2030 warning
#word="foo$( [[ $flag ]] && echo bar )"  # improper SC2030 warning
word="foo$( (( flag )) && echo bar )"   # no warning
echo "$word ($flag)"

[ljharb]

@TinCanTech using a value in a subshell does not make its value "in doubt", and that's not what the warning says. The warning says "modification in a subshell", so if it ever appears when the value is not modified in a subshell, it's broken, and it's pretty bizarre to claim otherwise.

[TinCanTech]

The warning is totally valid, you simply do not appreciate it, so ignore it.

I could write an entire book: Ways to cause shellcheck to trip over itself but what would be the value in that ?

[ljharb]

No, it’s not valid. There is nothing unsafe about the code being warned.

[TinCanTech]

shellckeck has warned you of a potential problem in your code, therefore the warning is valid.

[ljharb]

It isn't a potential problem. There is literally nothing that can possibly go wrong with it, because nothing is being modified in the subshell.

[TinCanTech]

Then you can ignore it ..

[ljharb]

Teaching people to ignore linter warnings is actively harmful, hence this entire issue thread trying to fix the broken behavior of shellcheck that produces the false positive.

[TinCanTech]

One of the main points of shellcheck is for you to deliberately use a directive to ignore a specific issue you have with your script when you don't like shellchecks opinion: eg. # shellcheck disable=2031

shellcheck is not teaching people to ignore warnings, it is making you aware of issues so that you can choose how to handle them.

[ljharb]

Ignore comments are for legitimate potential issues that you know are OK for your use case. Their purpose isn’t to paper over bugs, like this one (although obviously that’s what we’ll all do in the meantime). The bug still needs to be fixed.

[TinCanTech]

I disagree that this is a bug in shellcheck - As I understand it, shellcheck does not interpret your code it only matches patterns. I think it would be very difficult for shellcheck to distingish between: pattern_found_in_subshell VS pattern_found_in_subshell_but_no_change_made

Of course, I have been wrong before ..

[ljharb]

Then it shouldn’t be attempting to detect it at all.

[matthewpersico]

I realize I am wading into this late here but the variable LOGFILE never appears on the left hand side of an assignment. Why is shellcheck throwing an hissy fit about the variable being changed?

[matthewpersico]

I mean let's unwind all the esoteric comp sci 101 arguments here. If you don't find the text LOGFILE= in the line, the stated error is not applicable. (FYI, if you're on an iPhone like I am, the backtick is generated by holding the single apostrophe key until alternates pop up; it's the leftmost option. 😉)

[bland328]

Every release of shellcheck I've ever used handled this case properly, with the exception of version 0.7.2.

This all feels disturbingly like I was chatting with some fellow cartographers about a flaw in a newly-revised map, and a flat-earther came along to school us all. 🤦🏻‍♂️

[TinCanTech]

Yeah .. so shellcheck 0.7.1 does not complain either, looks like a regression . .

[bland328]

I've combed through the source a bit, but lacking previous Haskell exposure, it's a little painful and slow-going; I suspect the problematic change is likely in the getModifiedVariableCommand() function within Analytics.hs (involving new tests at lines 495-496?), but can't yet prove it.

In hopes it might help @koalaman or anyone else taking a look, here's a little more evidence about what works and what doesn't, re the SC2030/2031 regression:

#!/bin/bash
flag=1
#word="foo$( [ $flag ] && echo bar )"          # improper SC2030
#word="foo$( [[ $flag ]] && echo bar )"        # improper SC2030
#word="foo$( [[ -z $flag ]] && echo bar )"     # improper SC2030
#word="foo$( [[ -n $flag ]] && echo bar )"     # improper SC2030
#word="foo$( [[ $flag != '' ]] && echo bar )"  # works
word="foo$( (( flag )) && echo bar )"         # works
echo "$word ($flag)"

[koalaman]

Yup yup, the root cause was that ShellCheck considered [ -n "$var" ] an assignment, a hack originally intended to convey "the user checked it, so from now on you can assume this variable is assigned".

The earliest version of this bug is from 0d74140650 in 2015, when export foo was considered evidence that foo was previously declared and now in use. ( export foo; ); echo $foo triggers this in v0.3.6.

A more recognizable form of this bug came in 41b6e3d5e from 2017, when [ -v var ] was tagged as an assignment for the previously mentioned hacky reason. ( [ -v var ] ); echo $var will trigger this warning in v0.4.7. The issue as reported was from 50067ddf94c0 in 2020 when -n, -z and [ "$var" ] were similarly tagged.

The real cause is that ShellCheck's quote-unquote dataflow analysis was a quick hack from 2012 that has remained virtually unchanged since. It simply assumes the script runs from top to bottom ignoring all flow control, so something like if true; then a=1; else b=2; fi; x() { c=3; }; while false; do d=4; done will be interpreted the same as just a=1; b=2; c=3; d=4. There's a multitude of odd and mildly annoying behavior due to this oversimplification, and it's basically a solved problem in the literature. It's just never been the top priority.

For now it's patched up by ignoring all such fake assignments for the purposes for subshell modification detection.

[ljharb]

yay, thanks!

[bland328]

Fantastic! Thanks for the detailed explanation.