koalaman / shellcheck

ShellCheck, a static analysis tool for shell scripts
https://www.shellcheck.net
GNU General Public License v3.0
36.44k stars 1.78k forks source link

Spurious SC2086 with multiple files (0.9.0 regression) #2852

Open CyberShadow opened 1 year ago

CyberShadow commented 1 year ago

For bugs

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

main.sh:

#!/bin/bash
source b.sh

b.sh:

function fun() {
    local frame=0 str
    while str=$(caller $frame) ; do
        echo "$str"
    done
}

exit 1
exit

Here's what shellcheck currently says:

$ shellcheck -x -a ./main.sh

In b.sh line 3:
    while str=$(caller $frame) ; do
                           ^----^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    while str=$(caller "$frame") ; do

For more information:
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

Here's what I wanted or expected to see:

No warning, as with a single file, and as with ShellCheck 0.8.x

brother commented 1 year ago

I get SC2086 for $frame both while checking the b.sh file directly and while included. And that seems to be expected, if you want to silence it you should add a directive about it.

CyberShadow commented 1 year ago

I get SC2086 for $frame both while checking the b.sh file directly and while included.

There are some suspicious differences between the two. Parsing the file directly produces a different set of diagnostics. However, that is a separate problem, and not the one reported here.

The example above is a reduction from a larger script, reduced in a way that preserved the exact set of diagnostics.

And that seems to be expected

What makes you say that?

Please pay attention to the following:

brother commented 1 year ago

frame can never have any values that would necessitate quoting.

never in the current context. If that context changes it can however and it's easy to miss that part.

Having a recommendation to always add quotes is a nice way of avoiding that and as far as I can remember is the real world behind this decision.

the other warnings are about reachability and those seems valid as the function is not executed. the extra exit after the first exit is a clear example.

CyberShadow commented 1 year ago

That is a valid point of view, but it doesn't change that there is almost certainly a bug here, at least by nature of inconsistency, and the bug is a regression.

TinCanTech commented 1 year ago

This looks like a bug in shellcheck 0.8.0, fixed in 0.9.0.

CyberShadow commented 1 year ago

No, there is definitely a bug here. Please note the details of the test case.

This file produces no warnings:

#!/bin/bash
function fun() {
    local frame=0 str
    while str=$(caller $frame) ; do
        echo "$str"
    done
}

If this was actually about quoting $frame, then this would also produce warnings. But it (correctly) doesn't.

Things start to get wonky once you start adding multiple files or adding exit statements. This is what this bug is actually about.

CyberShadow commented 1 year ago

Worth noting that #2851 is very similar, possibly even having the same root cause. It should be easier to agree that the diagnostic emitted there is wrong.

brother commented 1 year ago

According to the changelog SC2086 should be more accurate in 0.9.0 and this seem to be a proof of the opposite.

  • SC2086: Now uses DFA to make more accurate predictions about values

Given that there are room for a PR.

What ever was the case in 0.8.0 I have no memory of. And as stated I am in the camp of quoting everything everywhere =)