Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potentially warn if local variable is declared with the assignment of an unquoted command substitution #1556

Closed
2 tasks done
ghost opened this issue Apr 25, 2019 · 2 comments

Comments

@ghost
Copy link

ghost commented Apr 25, 2019

For new checks and feature suggestions

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

#!/bin/dash
hi() { echo "hello world"; }
test1() { local var=$(hi); echo "1: $var"; }
test2() { local var="$(hi)"; echo "2: $var"; }
test3() { local var=; var=$(hi); echo "3: $var"; }
test1; test2; test3

Here's what shellcheck currently says:

Line 3	SC2155: Declare and assign separately to avoid masking return values.
Line 4	SC2155: Declare and assign separately to avoid masking return values.

Here's what I wanted or expected to see:

A warning for Line 2 that declaring - and assigning the expansion of a command substitution to - a local variable in a single pass can be affected by word splitting in some shells, unless the expansion is double quoted. For example, in both dash and posh, the above program prints:-

1: hello
2: hello world
3: hello world

Whereas in most other shells supporting the local keyword - including bash, busybox ash, mksh and loksh - the program behaves exactly as one would expect:-

1: hello world
2: hello world
3: hello world

This is a surprising pitfall. Given that dash is a popular sh implementation and given the popularity of the local keyword - despite being a POSIX transgression - it may well result in confusion. As such, I think it merits a warning - be it as a facet of SC2086 or otherwise.

@koalaman
Copy link
Owner

Definitely agree. This is also an issue with export and is an artifact of how bash and shellcheck both do parsing to allow local foo=(bar baz) which would otherwise be invalid

@koalaman
Copy link
Owner

Interestingly, this was (re-)added in Dash in 2018 and released in 2020 for 0.5.11 in anticipation of POSIX starting to mandate the behavior: https://austingroupbugs.net/view.php?id=351

Until then, ShellCheck will warn about this in sh. Thanks!

Kangie pushed a commit to Kangie/shellcheck that referenced this issue Feb 27, 2023
Merge of upstream through the stable 0.8.0 release.

Conflicts:
	ShellCheck.cabal
	src/ShellCheck/Analytics.hs
	src/ShellCheck/AnalyzerLib.hs

Manual changes:
	Moved maskedReturn isPortageBuild check into Commands.hs

Changelog:
----------------------------------------------------------------
Christian Nassif-Haynes (2):
      Show info about `set -e` suppression during function calls
      Add extra checks for masked return codes

Fabian Wolff (1):
      Do not suggest `grep -c` as a replacement for `grep -l/-L | wc -l`

Jens Petersen (1):
      move readme to extra-doc-files and add changelog to releases

Kamil Cukrowski (1):
      Add a comma to function characters

Matthias Diener (1):
      Clarify 'which'

Rebecca Cran (1):
      Fix typo in SC2006 message: "backticked" vs "backticks"

Vidar Holen (81):
      Merge pull request koalaman#2181 from matthiasdiener/patch-1
      Make x-comparison warning default
      Stable version v0.7.2
      Post-release CHANGELOG update
      Update Cabal version for Hackage
      Add wait between GitHub and Docker to allow replication
      Fix haddock failures (fixes koalaman#2216)
      Treat ${arr[*]} like $* for SC2048
      Fix bad warning for ${#arr[*]}. Fixes koalaman#2218.
      Sanity check command names (fixes koalaman#2227)
      Merge pull request koalaman#2241 from Kamilcuk/master
      Merge pull request koalaman#2238 from bcran/legacy-backticks-msg
      Merge pull request koalaman#2234 from juhp/patch-1
      SC2181: Add '!' in suggestion as appropriate (fixes koalaman#2189)
      Add :/. to chars recognized for \alias suppression (fixes koalaman#2287)
      Don't warn when line starts with &> (fixes koalaman#2281)
      Re-add warnings about 'declare var = value' (fixes koalaman#2279)
      Don't warn about repeated range in [[ -v arr[xxx] ]] (fixes koalaman#2285)
      Don't print colors when $TERM is 'dumb' or unset (fixes koalaman#2260)
      Have SC2155 trigger on 'typeset' as well (fixes koalaman#2262)
      Warn about quoting in assignments to sh declaration utilities (fixes koalaman#1556)
      Fix broken test from previous commit
      Warn about unquoted blanks in echo (fixes koalaman#377)
      Allow printf/return/assignments after exec (fixes koalaman#2249)
      Don't consider [ -n/-z/-v $var ] assignments for subshell modification (fixes koalaman#2217)
      Optionally suggest [[ over [ in Bash scripts (-o require-double-brackets) (fixes koalaman#887)
      Avoid trigger SC2181 on composite $? checks (fixes koalaman#1167)
      Warn about eval'ing arrays
      Merge pull request koalaman#2289 from nafigator/master
      SC2295 Warn about unquoted variables in PE patterns (fixes koalaman#2290)
      Switch build status badge from TravisCI to GitHub
      Remove defunct SonarQube plugin link (fixes koalaman#2292)
      Extend warnings about spaces around = to 'let'
      Suppress SC2167 when name is "_" (fixes koalaman#2298)
      Improve warnings for bad parameter expansion (fixes koalaman#2297)
      Warn about looping over array values and using them as keys
      Don't warn about variables guarded with :+ (fixes koalaman#2296)
      Recognize wait -p as assigning a variable (fixes koalaman#2179)
      Improve warnings for expr (fixes koalaman#2033)
      Add `rg` to list of commands ignored for SC2016 (fixes koalaman#2209)
      Don't warn about unused variables starting with _ (fixes koalaman#1498)
      Merge pull request koalaman#2307 from a1346054/fixes
      Fix parsing of [$var] (fixes koalaman#2309)
      Allow running this repo as a pre-commit hook
      Revert "Allow running this repo as a pre-commit hook"
      Add pre-commit instructions
      Add shellcheck-precommit hook to README.md
      Improve warnings about unnecessary subshells (fixes koalaman#2169)
      Warn about strings for numerical operators in [[ ]] (fixes koalaman#2312)
      Merge pull request koalaman#2303 from DoxasticFox/set-e-functions
      Allow specifying external-sources=true in shellcheckrc (fixes koalaman#1818)
      Merge pull request koalaman#2318 from FabianWolff/grep-lL-wc-l
      Allow `disable=all` to disable all warnings (fixes koalaman#2323)
      Remove SC1004 (fixes koalaman#2326)
      Suppress SC2094 when both are input redirections (fixes koalaman#2325)
      Don't trigger SC2140 on ${x+"a" "b"} (fixes koalaman#2265)
      Strip lines containing "STRIP" from ./striptests
      Add a `setgitversion` script to update the version string with git
      The removed check was SC1004, not SC1003
      Disable UUOC for cat with unquoted variable (fixes koalaman#2333)
      Don't emit SC2140 when trapped string is /, = or : (fixes koalaman#2334)
      Merge pull request koalaman#2320 from DoxasticFox/set-e-proc-sub
      Mention check-extra-masked-returns in changelog
      Add suggestion level in text for TTY output (fixes koalaman#2339)
      Mention require-double-brackets in CHANGELOG
      Warn about `read foo[i]` expanding as glob (fixes koalaman#2345)
      For `while getopts; do case ..` checks, make sure variable matches
      Skip SC2214 if variable is modified in loop (fixes koalaman#2351)
      Mention known incompatibilities in man page
      Treat typeset similar to declare (fixes koalaman#2354)
      Give more examples of what ShellCheck looks for
      Have quickscripts search for relevant paths (fixes koalaman#2286)
      Warn about [^..] in Dash (fixes koalaman#2361)
      Consider all forms of TA_Assignment to remove spaces (fixes koalaman#2364)
      Include `local -r` in check-extra-masked-returns (fixes koalaman#2362)
      Update release checklist
      Update distro tests
      Update stack resolver
      Update copyright years
      Fix bad version on stable releases
      Stable version 0.8.0

Yancharuk Alexander (2):
      Minor changes in README
      Review fixes in README

a1346054 (2):
      Fix redirect in LICENSE file
      Remove trailing whitespace

 .github/workflows/build.yml            |  27 +-
 .github_deploy                         |   1 -
 CHANGELOG.md                           |  51 +-
 LICENSE                                |   2 +-
 README.md                              |  38 +-
 ShellCheck.cabal                       |  12 +-
 quickrun                               |  10 +-
 quicktest                              |  11 +-
 setgitversion                          |  11 +
 shellcheck.1.md                        |  50 +-
 shellcheck.hs                          |  24 +-
 snap/snapcraft.yaml                    |   4 +-
 src/ShellCheck/AST.hs                  |   1 +
 src/ShellCheck/ASTLib.hs               |  74 ++-
 src/ShellCheck/Analytics.hs            | 841 ++++++++++++++++++++++++++++-----
 src/ShellCheck/AnalyzerLib.hs          | 182 +++++--
 src/ShellCheck/Checker.hs              |  84 +++-
 src/ShellCheck/Checks/Commands.hs      | 271 ++++++++++-
 src/ShellCheck/Checks/ShellSupport.hs  |   6 +-
 src/ShellCheck/Data.hs                 |   4 +-
 src/ShellCheck/Formatter/CheckStyle.hs |   2 +-
 src/ShellCheck/Formatter/Diff.hs       |   5 +-
 src/ShellCheck/Formatter/Format.hs     |  21 +-
 src/ShellCheck/Formatter/GCC.hs        |   2 +-
 src/ShellCheck/Formatter/JSON1.hs      |   2 +-
 src/ShellCheck/Formatter/TTY.hs        |   4 +-
 src/ShellCheck/Interface.hs            |  14 +-
 src/ShellCheck/Parser.hs               |  76 ++-
 stack.yaml                             |   2 +-
 striptests                             |   2 +-
 test/buildtest                         |   2 +
 test/check_release                     |   8 +-
 test/distrotest                        |  10 +-
 test/stacktest                         |   5 +-
 34 files changed, 1553 insertions(+), 306 deletions(-)
 create mode 100755 setgitversion

BUG=b:259131253
TEST=stack test; Compare cros lint checks on ebuilds

Change-Id: I5ca3fb27faa59b2f11369c2a150a419dee289977
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant