invalid shell script from diff when fixing SC2248 and SC2250 simultaneously

[defg]

For bugs

• Rule Id (if any, e.g. SC1000): SC2248 and SC2250
• My shellcheck version (shellcheck --version or "online"): 0.7.0
• [X] The rule's wiki page does not already cover this (e.g. https://shellcheck.net/wiki/SC2086)
• [X] I tried on shellcheck.net and verified that this is still a problem on the latest commit (I tried as best I could. shellcheck.net doesn't appear to support optional rules which are necessary to trigger this bug.)

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

## BUG
$ cat test.sh
#!/bin/bash

V="foo"
echo $V bar
$ ./test.sh 
foo bar
# Note that variable V is missing both surrounding quotes and curly braces
$ shellcheck -e SC2086 -o all -f gcc test.sh
test.sh:4:6: note: Prefer double quoting even when variables don't contain special characters. [SC2248]
test.sh:4:6: note: Prefer putting braces around variable references even when not strictly required. [SC2250]
# The diff leaves the script broken. A quote is missing inside the braces.
# ${V"} was written instead of the correct value of "${V}"
$ shellcheck -e SC2086 -o all -f diff test.sh
--- a/test.sh
+++ b/test.sh
@@ -1,4 +1,4 @@
 #!/bin/bash

 V="foo"
-echo $V bar
+echo ${V"} bar
$  shellcheck -e SC2086 -o all -f diff test.sh | patch
patching file test.sh
$ ./test.sh 
./test.sh: line 4: unexpected EOF while looking for matching `"'
./test.sh: line 5: syntax error: unexpected end of file

## WORKAROUND
$ cat test.sh
#!/bin/bash

V="foo"
echo $V bar
$ ./test.sh 
foo bar
# Patching seperately works as expected for SC2248 and SC2250
$ shellcheck -e SC2086,SC2248 -o all -f diff test.sh
--- a/test.sh
+++ b/test.sh
@@ -1,4 +1,4 @@
 #!/bin/bash

 V="foo"
-echo $V bar
+echo ${V} bar

$ shellcheck -e SC2086,SC2250 -o all -f diff test.sh
--- a/test.sh
+++ b/test.sh
@@ -1,4 +1,4 @@
 #!/bin/bash

 V="foo"
-echo $V bar
+echo "$V" bar
# Patching in order works correctly
$ shellcheck -e SC2086,SC2248 -o all -f diff test.sh | patch
patching file test.sh
$ cat test.sh
#!/bin/bash

V="foo"
echo ${V} bar
$ shellcheck -e SC2086 -o all -f gcc test.sh
test.sh:4:6: note: Prefer double quoting even when variables don't contain special characters. [SC2248]
$ shellcheck -e SC2086,SC2250 -o all -f diff test.sh | patch
patching file test.sh
$ shellcheck -e SC2086 -o all -f gcc test.sh
$ cat test.sh
#!/bin/bash

V="foo"
echo "${V}" bar
$ ./test.sh 
foo bar

Here's what shellcheck currently says:

When using diff currently, $VAR is converted to ${VAR"}

Here's what I wanted or expected to see:

When using diff, $VAR should be converted to "${VAR}"

[Fire-]

I'm seeing this as well, with both SC2086 and SC2248 colliding with SC2250.

  git push origin HEAD:$GIT_BRANCH;                        
                       ^---------^ SC2086: Double quote to prevent globbing and word splitting.
                       ^---------^ SC2250: Prefer putting braces around variable references even when not strictly required.

  fail! "Submitted corrections for next run." $code
                                              ^---^ SC2248: Prefer double quoting even when variables don't contain special characters.
                                              ^---^ SC2250: Prefer putting braces around variable references even when not strictly required.

-  git push origin HEAD:$GIT_BRANCH;                        
-  fail! "Submitted corrections for next run." $code
-
+  git push origin HEAD:${GIT_BRANCH"};                        
+  fail! "Submitted corrections for next run." $${ode
+"}

If I manually quote a reference that's breaking ($code) and re-run, it does not have as much of an issue ( compare $code and $GIT_BRANCH below )

  git push origin HEAD:$GIT_BRANCH;                        
                       ^---------^ SC2086: Double quote to prevent globbing and word splitting.
                       ^---------^ SC2250: Prefer putting braces around variable references even when not strictly required.

  fail! "Submitted corrections for next run." "$code"
                                               ^---^ SC2250: Prefer putting braces around variable references even when not strictly required.

-  git push origin HEAD:$GIT_BRANCH;                        
-  fail! "Submitted corrections for next run." "$code"
+  git push origin HEAD:${GIT_BRANCH"};                        
+  fail! "Submitted corrections for next run." "${code}"

[Vogtinator]

I think this also triggers a wrong diff in subsequent lines, the replacement is clearly garbage.

(from https://github.com/openSUSE/combustion/blob/2a6a997198f3d56a7c72664365819bbb0f563904/combustion):

 # Prepare chroot
 for i in proc sys dev; do
-       mount --rbind /$i /sysroot/$i
-done
+       mount --rbind /${i"} /sysroot/$${
+"}done
 mount --make-rslave /sysroot
 
 # Mount everything we can, errors deliberately ignored
@@ -146,18 +146,18 @@
 findmnt /sysroot/tmp >/dev/null || mount -t tmpfs tmpfs /sysroot/tmp
 
 # Fake a netconfig setup
-if [ -r /etc/resolv.conf ]; then
+if [ [-r /etc/resolv.conf ]; ]then
        mkdir -p /sysroot/run/netconfig/
        cp /etc/resolv.conf /sysroot/run/netconfig/resolv.conf
 
-       if ! [ -e /sysroot/etc/resolv.conf ]; then
+       if ! [[ -e /sysroot/etc/resolv.conf ]]; then
                if ln -sf /run/netconfig/resolv.conf /sysroot/etc/resolv.conf; then
                        delete_resolv_conf=1
                fi
        fi
 fi
 
-if [ -x /sysroot/usr/sbin/transactional-update ]; then
+if [ [-x /sysroot/usr/sbin/transactional-update ]; ]then
        # t-u doesn't allow running arbitrary commands and
        # also ignores the shell's exit code, so DIY.
        if ! chroot /sysroot transactional-update shell <<EOF; then

[ElectreAAS]

Using shellcheck version 0.8.0 (the latest available on apt at time of writing), I still get the problem. It even got very very wrong sometimes:

-cat $diffile
+cat $di${file
 
-for ign in $IGNORE ; do
+f"}or ign in $IGN${RE ; d}o

Note that this happens only with -f diff, but not in human-readable format.

Downloading the latest from snap (0.9.0) fixes this, but there is no word on this bugged behaviour on the wiki.

[brother]

Downloading the latest from snap (0.9.0) fixes this, but there is no word on this bugged behaviour on the wiki.

the wiki more or less assumes using the latest version, in some ways even using local compiled from HEAD. But you can edit the wiki and make a note that it does not work properly at all in versions prior to 0.9.0, the wiki is open for edits and created by us in the community together. make a note at the bottom maybe. If possible link info for when it was corrected.