Bogus SC2030/SC2031 (regression in 0.8.0?)

[kolyshkin]

Note this seems to be a regression in 0.8.0, as I did not had this bug with 0.7.1. This is why I am hesitant to call it a duplicate of https://github.com/koalaman/shellcheck/issues/280 (plus the repro is way simpler).

For bugs

• Rule Id (if any, e.g. SC1000): SC2030, SC2031
• My shellcheck version (shellcheck --version or 'online'): online
• [x] I tried on shellcheck.net and verified that this is still a problem on the latest commit
• [ ] It's not reproducible on shellcheck.net, but I think that's because it's an OS, configuration or encoding issue

For new checks and feature suggestions

• [x] shellcheck.net (i.e. the latest commit) currently gives no useful warnings about this
• [ ] I searched through https://github.com/koalaman/shellcheck/issues and didn't find anything related

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

#!/usr/bin/env bats

@test test1 {
        run true
        [ "$status" -eq 0 ]
}

function test_sub() {
        run true
        [ "$status" -eq 0 ]
}

@test test2 {
        test_sub
}

Here's what shellcheck currently says:

Line 3	SC2030: Modification of status is local (to subshell caused by @bats test).
Line 10	SC2031: status was modified in a subshell. That change might be lost.

Here's what I wanted or expected to see:

No warnings

[kolyshkin]

I found out that the order of functions matter (while it should not).

The example above gives an error. If you move function test_sub to above @test test1, the SC2030/SC2031 error is gone.

#!/usr/bin/env bats

function test_sub() {
        run true
        [ "$status" -eq 0 ]
}

@test test1 {
        run true
        [ "$status" -eq 0 ]
}

@test test2 {
        test_sub
}

[kolyshkin]

OK, this seems like a regression between 0.7.1 and 0.7.2. I will try to git-bisect it.

[kolyshkin]

OK, the regression seems to be that in versions prior to 0.7.1 the following code does not result in a warning:

@test test1 {
	run true
	[ "$status" -eq 0 ]
}

function test_sub() {
	local status # <-- Added this

	run true
	[ "$status" -eq 0 ]
}

@test test2 {
	test_sub
}

The major difference from the example in the description is added local status to function test_sub().

This code works for 0.7.1 and fails with 0.8.0.

I have bisected this to commit fbc8d2cb2f8070f820c9337851bb97478e40e710 "Don't consider [ -n/-z/-v $var ] assignments for subshell modification (fixes #2217)"

[kolyshkin]

Now, I was thinking that declaring the variable as local should be enough to get rid of these warnings -- since this is local, it is obvious that it won't be available outside of its scope. Alas, it does not fix things.

Ordering issue described above in https://github.com/koalaman/shellcheck/issues/2431#issuecomment-1026287640 is also strange.

[kolyshkin]

since this is local, it is obvious that it won't be available outside of its scope

I was half-wrong with this -- variables declared as local are available to whatever this function calls. Nevertheless, if you declare something as local, it's clear your callers won't see it.

This prevents upgrading shellcheck to v0.8.0 in https://github.com/opencontainers/runc CI. If there's anything I help with to solve this issue, please let me know, @koalaman.

[martin-schulze-vireso]

I just wanted to open an issue for these exact same messages with .bats files but in a broader manner. I plan on recommending shellcheck with Bats in general. However, for users with existing large codebases this might be punishing: For our ~4.5kloc of code in .bats files we have 42 instances of #shellcheck disable=SC2031,SC2031 or one about every 100 lines of code.

The instances can be sorted into following categories:

1. 38x export variable: subsequent commands should see the value
2. 2x PATH modification
3. 1x communication between setup and test: https://github.com/bats-core/bats-core/blob/61abd097ba592a1c44a649af16d7e0171631eaf5/docs/examples/package-tarball.bats#L33-L34
4. 1x a test that expects this error

I think the export case should definitely be an exception, right?

[kolyshkin]

The cause of this https://github.com/koalaman/shellcheck/issues/280#issuecomment-77736551 (and probably some changes in parsing that went into 0.8.0). As I understand, this is not easy to fix :(

[martin-schulze-vireso]

For my usecase above, having exceptions for exported variables and PATH would reduce the amount of bogus messages to an acceptable level.

[rivy]

As a work-around, replacing [...] with the equivalent test ... fixes the issue (stops the incorrect warning pileup).

[debarshiray]

The cause of this #280 (comment) (and probably some changes in parsing that went into 0.8.0). As I understand, this is not easy to fix :(

There's a slightly longer explanation at https://github.com/koalaman/shellcheck/issues/2217#issuecomment-887666265

[debarshiray]

In my case, something like this is triggering SC2030 and SC2031:

# shellcheck shell=bats

@test "HISTFILESIZE 1" {
  if [ "$HISTFILESIZE" = "" ]; then
    HISTFILESIZE=1001
  fi

  export HISTFILESIZE

  echo "$HISTFILESIZE"
}

@test "HISTFILESIZE 2" {
  if [ "$HISTFILESIZE" = "" ]; then
    HISTFILESIZE=2001
  fi

  export HISTFILESIZE

  echo "$HISTFILESIZE"
}

... with both ShellCheck 0.9.0 and the online version:

Line 5:
    HISTFILESIZE=1001
    ^-- [SC2030](https://www.shellcheck.net/wiki/SC2030) (info): Modification of HISTFILESIZE is local (to subshell caused by @bats test).
 
Line 14:
  if [ "$HISTFILESIZE" = "" ]; then
        ^-- [SC2031](https://www.shellcheck.net/wiki/SC2031) (info): HISTFILESIZE was modified in a subshell. That change might be lost.

[debarshiray]

As a work-around, replacing [...] with the equivalent test ... fixes the issue (stops the incorrect warning pileup).

It doesn't seem to work anymore.