diff --git a/CHANGELOG.md b/CHANGELOG.md index 38b8e49..4b510ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ - SC2295: Warn about "${x#$y}" treating $y as a pattern when not quoted - SC2296-SC2301: Improved warnings for bad parameter expansions - SC2302/SC2303: Warn about loops over array values when using them as keys +- SC2304-SC2306: Warn about unquoted globs in expr arguments +- SC2307: Warn about insufficient number of arguments to expr +- SC2308: Suggest other approaches for non-standard expr extensions ### Fixed - SC2102 about repetitions in ranges no longer triggers on [[ -v arr[xx] ]] diff --git a/src/ShellCheck/AnalyzerLib.hs b/src/ShellCheck/AnalyzerLib.hs index 633543a..662eff5 100644 --- a/src/ShellCheck/AnalyzerLib.hs +++ b/src/ShellCheck/AnalyzerLib.hs @@ -142,7 +142,7 @@ producesComments c s = do prRoot pr let spec = defaultSpec pr let params = makeParameters spec - return . not . null $ runChecker params c + return . not . null $ filterByAnnotation spec params $ runChecker params c makeComment :: Severity -> Id -> Code -> String -> TokenComment makeComment severity id code note = diff --git a/src/ShellCheck/Checks/Commands.hs b/src/ShellCheck/Checks/Commands.hs index 6c7bdc5..76ac9a7 100644 --- a/src/ShellCheck/Checks/Commands.hs +++ b/src/ShellCheck/Checks/Commands.hs @@ -57,7 +57,7 @@ commandChecks :: [CommandCheck] commandChecks = [ checkTr ,checkFindNameGlob - ,checkNeedlessExpr + ,checkExpr ,checkGrepRe ,checkTrapQuotes ,checkReturn @@ -254,19 +254,74 @@ checkFindNameGlob = CommandCheck (Basename "find") (f . arguments) where acc b -prop_checkNeedlessExpr = verify checkNeedlessExpr "foo=$(expr 3 + 2)" -prop_checkNeedlessExpr2 = verify checkNeedlessExpr "foo=`echo \\`expr 3 + 2\\``" -prop_checkNeedlessExpr3 = verifyNot checkNeedlessExpr "foo=$(expr foo : regex)" -prop_checkNeedlessExpr4 = verifyNot checkNeedlessExpr "foo=$(expr foo \\< regex)" -checkNeedlessExpr = CommandCheck (Basename "expr") f where - f t = +prop_checkExpr = verify checkExpr "foo=$(expr 3 + 2)" +prop_checkExpr2 = verify checkExpr "foo=`echo \\`expr 3 + 2\\``" +prop_checkExpr3 = verifyNot checkExpr "foo=$(expr foo : regex)" +prop_checkExpr4 = verifyNot checkExpr "foo=$(expr foo \\< regex)" +prop_checkExpr5 = verify checkExpr "# shellcheck disable=SC2003\nexpr match foo bar" +prop_checkExpr6 = verify checkExpr "# shellcheck disable=SC2003\nexpr foo : fo*" +prop_checkExpr7 = verify checkExpr "# shellcheck disable=SC2003\nexpr 5 -3" +prop_checkExpr8 = verifyNot checkExpr "# shellcheck disable=SC2003\nexpr \"$@\"" +prop_checkExpr9 = verifyNot checkExpr "# shellcheck disable=SC2003\nexpr 5 $rest" +prop_checkExpr10 = verify checkExpr "# shellcheck disable=SC2003\nexpr length \"$var\"" +prop_checkExpr11 = verify checkExpr "# shellcheck disable=SC2003\nexpr foo > bar" +prop_checkExpr12 = verify checkExpr "# shellcheck disable=SC2003\nexpr 1 | 2" +prop_checkExpr13 = verify checkExpr "# shellcheck disable=SC2003\nexpr 1 * 2" +prop_checkExpr14 = verify checkExpr "# shellcheck disable=SC2003\nexpr \"$x\" >= \"$y\"" + +checkExpr = CommandCheck (Basename "expr") f where + f t = do when (all (`notElem` exceptions) (words $ arguments t)) $ style (getId $ getCommandTokenOrThis t) 2003 "expr is antiquated. Consider rewriting this using $((..)), ${} or [[ ]]." + + case arguments t of + [lhs, op, rhs] -> do + checkOp lhs + case getWordParts op of + [T_Glob _ "*"] -> + err (getId op) 2304 + "* must be escaped to multiply: \\*. Modern $((x * y)) avoids this issue." + [T_Literal _ ":"] | isGlob rhs -> + warn (getId rhs) 2305 + "Quote regex argument to expr to avoid it expanding as a glob." + _ -> return () + + [single] | not (willSplit single) -> + warn (getId single) 2307 + "'expr' expects 3+ arguments but sees 1. Make sure each operator/operand is a separate argument, and escape <>&|." + + [first, second] | + (fromMaybe "" $ getLiteralString first) /= "length" + && not (willSplit first || willSplit second) -> do + checkOp first + warn (getId t) 2307 + "'expr' expects 3+ arguments, but sees 2. Make sure each operator/operand is a separate argument, and escape <>&|." + + (first:rest) -> do + checkOp first + forM_ rest $ \t -> + -- We already find 95%+ of multiplication and regex earlier, so don't bother classifying this further. + when (isGlob t) $ warn (getId t) 2306 "Escape glob characters in arguments to expr to avoid pathname expansion." + + _ -> return () + -- These operators are hard to replicate in POSIX - exceptions = [ ":", "<", ">", "<=", ">=" ] + exceptions = [ ":", "<", ">", "<=", ">=", + -- We can offer better suggestions for these + "match", "length", "substr", "index"] words = mapMaybe getLiteralString + checkOp side = + case getLiteralString side of + Just "match" -> msg "'expr match' has unspecified results. Prefer 'expr str : regex'." + Just "length" -> msg "'expr length' has unspecified results. Prefer ${#var}." + Just "substr" -> msg "'expr substr' has unspecified results. Prefer 'cut' or ${var#???}." + Just "index" -> msg "'expr index' has unspecified results. Prefer x=${var%%[chars]*}; $((${#x}+1))." + _ -> return () + where + msg = info (getId side) 2308 + prop_checkGrepRe1 = verify checkGrepRe "cat foo | grep *.mp3" prop_checkGrepRe2 = verify checkGrepRe "grep -Ev cow*test *.mp3"