shellcheck icon indicating copy to clipboard operation
shellcheck copied to clipboard

Spurious SC2086 with multiple files (0.9.0 regression)

Open CyberShadow opened this issue 2 years ago • 8 comments

For bugs

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

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

CyberShadow avatar Oct 28 '23 20:10 CyberShadow

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.

brother avatar Oct 30 '23 07:10 brother

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:

  • frame can never have any values that would necessitate quoting.
  • ShellCheck can generally observe when variables can never have any values that would necessitate quoting, and does not emit SC2086 in those cases.
  • This is a regression compared to the behavior of ShellCheck 0.8.1.

CyberShadow avatar Oct 30 '23 10:10 CyberShadow

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.

brother avatar Oct 30 '23 11:10 brother

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.

CyberShadow avatar Oct 30 '23 11:10 CyberShadow

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

TinCanTech avatar Oct 30 '23 12:10 TinCanTech

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 avatar Oct 30 '23 12:10 CyberShadow

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.

CyberShadow avatar Oct 30 '23 12:10 CyberShadow

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 =)

brother avatar Oct 30 '23 13:10 brother