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

Warn against variable substring removal #2290

Closed
2 tasks done
hseg opened this issue Aug 2, 2021 · 1 comment
Closed
2 tasks done

Warn against variable substring removal #2290

hseg opened this issue Aug 2, 2021 · 1 comment

Comments

@hseg
Copy link

hseg commented Aug 2, 2021

For new checks and feature suggestions

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

#!/bin/bash
base="[hello] [world]"
pref="[hello]
suff="[world]"
pref="${base%$suff}"
# sets to $base, not "hello ", because $base doesn't end in any of the characters d,l,o,r,w

Here's what shellcheck currently says:

Nothing

Here's what I wanted or expected to see:

Warning: the substring removal expansion accepts a pattern, not a literal string.
Use slicing operators instead:

"${base%suff}" -> "${base:0:${#base}-${#suff}}"
"${base#pref}" -> "${base:${#pref}}"

(note: this advice isn't perfect for suffixes, since if the "suffix" is longer than the base, this will silently remove len(suff)-len(base) characters from the end of $base)

@koalaman
Copy link
Owner

koalaman commented Aug 3, 2021

Rather than using length, the pattern can be quoted: pref="${base%"$suff"}". ShellCheck warns about missing quotes causing pattern matching in several other cases, so this makes sense.

I had a look at 26 cases. 16 used it on file paths and URLs where they ideally should be quoted, 5 were constant values that didn't need but could be quoted, 4 used it on other things where they should ideally have been quoted, and 1 was a utility function where I wasn't sure if it was meant to be used with patterns or not.

While it's a nice and low false positive rate, I likely has a low IRL impact, so I'm not sure about the signal to noise ratio. Let's try it out and optionally adjust it later.

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

2 participants