Merge branch 'yetamrra-require-braces'

CHANGELOG.md

@@ -8,12 +8,13 @@
 - Source paths: Use `-P dir1:dir2` or a `source-path=dir1` directive
                 to specify search paths for sourced files.
 - json1 format like --format=json but treats tabs as single characters
-- SC2249: Warn about `case` with missing default case (verbose)
-- SC2248: Warn about unquoted variables without special chars (verbose)
+- SC2250: Warn about variable references without braces (optional)
+- SC2249: Warn about `case` with missing default case (optional)
+- SC2248: Warn about unquoted variables without special chars (optional)
 - SC2247: Warn about $"(cmd)" and $"{var}"
 - SC2246: Warn if a shebang's interpreter ends with /
 - SC2245: Warn that Ksh ignores all but the first glob result in `[`
-- SC2243/SC2244: Suggest using explicit -n for `[ $foo ]` (verbose)
+- SC2243/SC2244: Suggest using explicit -n for `[ $foo ]` (optional)
 
 ### Changed
 - If a directive or shebang is not specified, a `.bash/.bats/.dash/.ksh`

src/ShellCheck/AST.hs

@@ -76,7 +76,7 @@ data Token =
     | T_DSEMI Id
     | T_Do Id
     | T_DollarArithmetic Id Token
-    | T_DollarBraced Id Token
+    | T_DollarBraced Id Bool Token
     | T_DollarBracket Id Token
     | T_DollarDoubleQuoted Id [Token]
     | T_DollarExpansion Id [Token]
@@ -253,7 +253,7 @@ analyze f g i =
     delve (T_Function id a b name body) = d1 body $ T_Function id a b name
     delve (T_Condition id typ token) = d1 token $ T_Condition id typ
     delve (T_Extglob id str l) = dl l $ T_Extglob id str
-    delve (T_DollarBraced id op) = d1 op $ T_DollarBraced id
+    delve (T_DollarBraced id braced op) = d1 op $ T_DollarBraced id braced
     delve (T_HereDoc id d q str l) = dl l $ T_HereDoc id d q str
 
     delve (TC_And id typ str t1 t2) = d2 t1 t2 $ TC_And id typ str
@@ -323,7 +323,7 @@ getId t = case t of
         T_NormalWord id _  -> id
         T_DoubleQuoted id _  -> id
         T_DollarExpansion id _  -> id
-        T_DollarBraced id _  -> id
+        T_DollarBraced id _ _ -> id
         T_DollarArithmetic id _  -> id
         T_BraceExpansion id _  -> id
         T_ParamSubSpecialChar id _ -> id

src/ShellCheck/ASTLib.hs

@@ -81,7 +81,7 @@ oversimplify token =
         (T_NormalWord _ l) -> [concat (concatMap oversimplify l)]
         (T_DoubleQuoted _ l) -> [concat (concatMap oversimplify l)]
         (T_SingleQuoted _ s) -> [s]
-        (T_DollarBraced _ _) -> ["${VAR}"]
+        (T_DollarBraced _ _ _) -> ["${VAR}"]
         (T_DollarArithmetic _ _) -> ["${VAR}"]
         (T_DollarExpansion _ _) -> ["${VAR}"]
         (T_Backticked _ _) -> ["${VAR}"]
@@ -133,11 +133,11 @@ isUnquotedFlag token = fromMaybe False $ do
     return $ "-" `isPrefixOf` str
 
 -- Given a T_DollarBraced, return a simplified version of the string contents.
-bracedString (T_DollarBraced _ l) = concat $ oversimplify l
+bracedString (T_DollarBraced _ _ l) = concat $ oversimplify l
 bracedString _ = error "Internal shellcheck error, please report! (bracedString on non-variable)"
 
 -- Is this an expansion of multiple items of an array?
-isArrayExpansion t@(T_DollarBraced _ _) =
+isArrayExpansion t@(T_DollarBraced _ _ _) =
     let string = bracedString t in
         "@" `isPrefixOf` string ||
             not ("#" `isPrefixOf` string) && "[@]" `isInfixOf` string
@@ -146,7 +146,7 @@ isArrayExpansion _ = False
 -- Is it possible that this arg becomes multiple args?
 mayBecomeMultipleArgs t = willBecomeMultipleArgs t || f t
   where
-    f t@(T_DollarBraced _ _) =
+    f t@(T_DollarBraced _ _ _) =
         let string = bracedString t in
             "!" `isPrefixOf` string
     f (T_DoubleQuoted _ parts) = any f parts

src/ShellCheck/Analytics.hs

@@ -223,6 +223,13 @@ optionalTreeChecks = [
         cdPositive = "case $? in 0) echo 'Success';; esac",
         cdNegative = "case $? in 0) echo 'Success';; *) echo 'Fail' ;; esac"
     }, nodeChecksToTreeCheck [checkDefaultCase])
+
+    ,(newCheckDescription {
+        cdName = "require-variable-braces",
+        cdDescription = "Suggest putting braces around all variable references",
+        cdPositive = "var=hello; echo $var",
+        cdNegative = "var=hello; echo ${var}"
+    }, nodeChecksToTreeCheck [checkVariableBraces])
     ]
 
 optionalCheckMap :: Map.Map String (Parameters -> Token -> [TokenComment])
@@ -754,7 +761,7 @@ checkShorthandIf _ _ = return ()
 prop_checkDollarStar = verify checkDollarStar "for f in $*; do ..; done"
 prop_checkDollarStar2 = verifyNot checkDollarStar "a=$*"
 prop_checkDollarStar3 = verifyNot checkDollarStar "[[ $* = 'a b' ]]"
-checkDollarStar p t@(T_NormalWord _ [b@(T_DollarBraced id _)])
+checkDollarStar p t@(T_NormalWord _ [b@(T_DollarBraced id _ _)])
       | bracedString b == "*"  =
     unless (isStrictlyQuoteFree (parentMap p) t) $
         warn id 2048 "Use \"$@\" (with quotes) to prevent whitespace problems."
@@ -825,7 +832,7 @@ checkArrayWithoutIndex params _ =
     doVariableFlowAnalysis readF writeF defaultMap (variableFlow params)
   where
     defaultMap = Map.fromList $ map (\x -> (x,())) arrayVariables
-    readF _ (T_DollarBraced id token) _ = do
+    readF _ (T_DollarBraced id _ token) _ = do
         map <- get
         return . maybeToList $ do
             name <- getLiteralString token
@@ -1267,7 +1274,7 @@ prop_checkArithmeticDeref12= verify checkArithmeticDeref "for ((i=0; $i < 3; i))
 prop_checkArithmeticDeref13= verifyNot checkArithmeticDeref "(( $$ ))"
 prop_checkArithmeticDeref14= verifyNot checkArithmeticDeref "(( $! ))"
 prop_checkArithmeticDeref15= verifyNot checkArithmeticDeref "(( ${!var} ))"
-checkArithmeticDeref params t@(TA_Expansion _ [b@(T_DollarBraced id _)]) =
+checkArithmeticDeref params t@(TA_Expansion _ [b@(T_DollarBraced id _ _)]) =
     unless (isException $ bracedString b) getWarning
   where
     isException [] = True
@@ -1302,8 +1309,7 @@ prop_checkComparisonAgainstGlob3 = verify checkComparisonAgainstGlob "[ $cow = *
 prop_checkComparisonAgainstGlob4 = verifyNot checkComparisonAgainstGlob "[ $cow = foo ]"
 prop_checkComparisonAgainstGlob5 = verify checkComparisonAgainstGlob "[[ $cow != $bar ]]"
 prop_checkComparisonAgainstGlob6 = verify checkComparisonAgainstGlob "[ $f != /* ]"
-
-checkComparisonAgainstGlob _ (TC_Binary _ DoubleBracket op _ (T_NormalWord id [T_DollarBraced _ _]))
+checkComparisonAgainstGlob _ (TC_Binary _ DoubleBracket op _ (T_NormalWord id [T_DollarBraced _ _ _]))
     | op `elem` ["=", "==", "!="] =
         warn id 2053 $ "Quote the right-hand side of " ++ op ++ " in [[ ]] to prevent glob matching."
 checkComparisonAgainstGlob params (TC_Binary _ SingleBracket op _ word)
@@ -1457,7 +1463,7 @@ prop_checkIndirectExpansion2 = verifyNot checkIndirectExpansion "${foo//$n/lol}"
 prop_checkIndirectExpansion3 = verify checkIndirectExpansion "${$#}"
 prop_checkIndirectExpansion4 = verify checkIndirectExpansion "${var${n}_$((i%2))}"
 prop_checkIndirectExpansion5 = verifyNot checkIndirectExpansion "${bar}"
-checkIndirectExpansion _ (T_DollarBraced i (T_NormalWord _ contents)) =
+checkIndirectExpansion _ (T_DollarBraced i _ (T_NormalWord _ contents)) =
     when (isIndirection contents) $
         err i 2082 "To expand via indirection, use arrays, ${!name} or (for sh only) eval."
   where
@@ -1467,7 +1473,7 @@ checkIndirectExpansion _ (T_DollarBraced i (T_NormalWord _ contents)) =
     isIndirectionPart t =
         case t of T_DollarExpansion _ _ ->  Just True
                   T_Backticked _ _ ->       Just True
-                  T_DollarBraced _ _ ->     Just True
+                  T_DollarBraced _ _ _ ->     Just True
                   T_DollarArithmetic _ _ -> Just True
                   T_Literal _ s -> if all isVariableChar s
                                     then Nothing
@@ -1494,7 +1500,7 @@ checkInexplicablyUnquoted params (T_NormalWord id tokens) = mapM_ check (tails t
     check (T_DoubleQuoted _ a:trapped:T_DoubleQuoted _ b:_) =
         case trapped of
             T_DollarExpansion id _ -> warnAboutExpansion id
-            T_DollarBraced id _ -> warnAboutExpansion id
+            T_DollarBraced id _ _ -> warnAboutExpansion id
             T_Literal id s ->
                 unless (quotesSingleThing a && quotesSingleThing b || isRegex (getPath (parentMap params) trapped)) $
                     warnAboutLiteral id
@@ -1515,7 +1521,7 @@ checkInexplicablyUnquoted params (T_NormalWord id tokens) = mapM_ check (tails t
     -- the quotes were probably intentional and harmless.
     quotesSingleThing x = case x of
         [T_DollarExpansion _ _] -> True
-        [T_DollarBraced _ _] -> True
+        [T_DollarBraced _ _ _] -> True
         [T_Backticked _ _] -> True
         _ -> False
 
@@ -1822,7 +1828,7 @@ checkSpacefulness' onFind params t =
 
     isExpansion t =
         case t of
-            (T_DollarBraced _ _ ) -> True
+            (T_DollarBraced _ _ _ ) -> True
             _ -> False
 
     isSpacefulWord :: (String -> Bool) -> [Token] -> Bool
@@ -1836,7 +1842,7 @@ checkSpacefulness' onFind params t =
           T_Extglob {}       -> True
           T_Literal _ s      -> s `containsAny` globspace
           T_SingleQuoted _ s -> s `containsAny` globspace
-          T_DollarBraced _ _ -> spacefulF $ getBracedReference $ bracedString x
+          T_DollarBraced _ _ _ -> spacefulF $ getBracedReference $ bracedString x
           T_NormalWord _ w   -> isSpacefulWord spacefulF w
           T_DoubleQuoted _ w -> isSpacefulWord spacefulF w
           _ -> False
@@ -1844,6 +1850,24 @@ checkSpacefulness' onFind params t =
         globspace = "*?[] \t\n"
         containsAny s = any (`elem` s)
 
+prop_CheckVariableBraces1 = verify checkVariableBraces "a='123'; echo $a"
+prop_CheckVariableBraces2 = verifyNot checkVariableBraces "a='123'; echo ${a}"
+prop_CheckVariableBraces3 = verifyNot checkVariableBraces "#shellcheck disable=SC2016\necho '$a'"
+prop_CheckVariableBraces4 = verifyNot checkVariableBraces "echo $* $1"
+checkVariableBraces params t =
+    case t of
+        T_DollarBraced id False _ ->
+            unless (name `elem` unbracedVariables) $
+                styleWithFix id 2250
+                    "Prefer putting braces around variable references even when not strictly required."
+                    (fixFor t)
+
+        _ -> return ()
+  where
+    name = getBracedReference $ bracedString t
+    fixFor token = fixWith [replaceStart (getId token) params 1 "${"
+                           ,replaceEnd (getId token) params 0 "}"]
+
 prop_checkQuotesInLiterals1 = verifyTree checkQuotesInLiterals "param='--foo=\"bar\"'; app $param"
 prop_checkQuotesInLiterals1a= verifyTree checkQuotesInLiterals "param=\"--foo='lolbar'\"; app $param"
 prop_checkQuotesInLiterals2 = verifyNotTree checkQuotesInLiterals "param='--foo=\"bar\"'; app \"$param\""
@@ -1874,7 +1898,7 @@ checkQuotesInLiterals params t =
         return []
     writeF _ _ _ _ = return []
 
-    forToken map (T_DollarBraced id t) =
+    forToken map (T_DollarBraced id _ t) =
         -- skip getBracedReference here to avoid false positives on PE
         Map.lookup (concat . oversimplify $ t) map
     forToken quoteMap (T_DoubleQuoted id tokens) =
@@ -1888,7 +1912,7 @@ checkQuotesInLiterals params t =
 
     squashesQuotes t =
         case t of
-            T_DollarBraced id _ -> "#" `isPrefixOf` bracedString t
+            T_DollarBraced id _ _ -> "#" `isPrefixOf` bracedString t
             _ -> False
 
     readF _ expr name = do
@@ -2135,10 +2159,10 @@ checkUnassignedReferences params t = warnings
     isInArray var t = any isArray $ getPath (parentMap params) t
       where
         isArray T_Array {} = True
-        isArray b@(T_DollarBraced _ _) | var /= getBracedReference (bracedString b) = True
+        isArray b@(T_DollarBraced _ _ _) | var /= getBracedReference (bracedString b) = True
         isArray _ = False
 
-    isGuarded (T_DollarBraced _ v) =
+    isGuarded (T_DollarBraced _ _ v) =
         rest `matches` guardRegex
       where
         name = concat $ oversimplify v
@@ -2224,7 +2248,7 @@ checkWhileReadPitfalls _ _ = return ()
 
 prop_checkPrefixAssign1 = verify checkPrefixAssignmentReference "var=foo echo $var"
 prop_checkPrefixAssign2 = verifyNot checkPrefixAssignmentReference "var=$(echo $var) cmd"
-checkPrefixAssignmentReference params t@(T_DollarBraced id value) =
+checkPrefixAssignmentReference params t@(T_DollarBraced id _ value) =
     check path
   where
     name = getBracedReference $ bracedString t
@@ -3026,7 +3050,7 @@ checkSplittingInArrays params t =
         T_DollarExpansion id _ -> forCommand id
         T_DollarBraceCommandExpansion id _ -> forCommand id
         T_Backticked id _ -> forCommand id
-        T_DollarBraced id str |
+        T_DollarBraced id _ str |
             not (isCountingReference part)
             && not (isQuotedAlternativeReference part)
             && not (getBracedReference (bracedString part) `elem` variablesWithoutSpaces)

src/ShellCheck/AnalyzerLib.hs

@@ -510,7 +510,7 @@ getModifiedVariables t =
             guard . not . null $ str
             return (t, token, str, DataString SourceChecked)
 
-        T_DollarBraced _ l -> maybeToList $ do
+        T_DollarBraced _ _ l -> maybeToList $ do
             let string = bracedString t
             let modifier = getBracedModifier string
             guard $ ":=" `isPrefixOf` modifier
@@ -702,7 +702,7 @@ getOffsetReferences mods = fromMaybe [] $ do
 
 getReferencedVariables parents t =
     case t of
-        T_DollarBraced id l -> let str = bracedString t in
+        T_DollarBraced id _ l -> let str = bracedString t in
             (t, t, getBracedReference str) :
                 map (\x -> (l, l, x)) (
                     getIndexReferences str
@@ -897,7 +897,7 @@ shouldIgnoreCode params code t =
         getPath (parentMap params) t
 
 -- Is this a ${#anything}, to get string length or array count?
-isCountingReference (T_DollarBraced id token) =
+isCountingReference (T_DollarBraced id _ token) =
     case concat $ oversimplify token of
         '#':_ -> True
         _     -> False
@@ -906,7 +906,7 @@ isCountingReference _ = False
 -- FIXME: doesn't handle ${a:+$var} vs ${a:+"$var"}
 isQuotedAlternativeReference t =
     case t of
-        T_DollarBraced _ _ ->
+        T_DollarBraced _ _ _ ->
             getBracedModifier (bracedString t) `matches` re
         _ -> False
   where

src/ShellCheck/Checks/Commands.hs

@@ -278,7 +278,7 @@ checkTrapQuotes = CommandCheck (Exactly "trap") (f . arguments) where
     warning id = warn id 2064 "Use single quotes, otherwise this expands now rather than when signalled."
     checkExpansions (T_DollarExpansion id _) = warning id
     checkExpansions (T_Backticked id _) = warning id
-    checkExpansions (T_DollarBraced id _) = warning id
+    checkExpansions (T_DollarBraced id _ _) = warning id
     checkExpansions (T_DollarArithmetic id _) = warning id
     checkExpansions _ = return ()
 
@@ -896,7 +896,7 @@ checkCatastrophicRm = CommandCheck (Basename "rm") $ \t ->
     getPotentialPath = getLiteralStringExt f
       where
         f (T_Glob _ str) = return str
-        f (T_DollarBraced _ word) =
+        f (T_DollarBraced _ _ word) =
             let var = onlyLiteralString word in
                 -- This shouldn't handle non-colon cases.
                 if any (`isInfixOf` var) [":?", ":-", ":="]

src/ShellCheck/Checks/ShellSupport.hs

@@ -232,7 +232,7 @@ checkBashisms = ForShell [Sh, Dash] $ \t -> do
     bashism t@(TA_Variable id str _) | isBashVariable str =
         warnMsg id $ str ++ " is"
 
-    bashism t@(T_DollarBraced id token) = do
+    bashism t@(T_DollarBraced id _ token) = do
         mapM_ check expansion
         when (isBashVariable var) $
                     warnMsg id $ var ++ " is"

src/ShellCheck/Data.hs

@@ -47,6 +47,12 @@ variablesWithoutSpaces = specialVariablesWithoutSpaces ++ [
     "COLUMNS", "HISTFILESIZE", "HISTSIZE", "LINES"
   ]
 
+specialVariables = specialVariablesWithoutSpaces ++ ["@", "*"]
+
+unbracedVariables = specialVariables ++ [
+    "0", "1", "2", "3", "4", "5", "6", "7", "8", "9"
+  ]
+
 arrayVariables = [
     "BASH_ALIASES", "BASH_ARGC", "BASH_ARGV", "BASH_CMDS", "BASH_LINENO",
     "BASH_REMATCH", "BASH_SOURCE", "BASH_VERSINFO", "COMP_WORDS", "COPROC",

src/ShellCheck/Parser.hs

@@ -69,7 +69,7 @@ variableChars = upper <|> lower <|> digit <|> oneOf "_"
 functionChars = variableChars <|> oneOf ":+?-./^@"
 -- Chars to allow in functions using the 'function' keyword
 extendedFunctionChars = functionChars <|> oneOf "[]*=!"
-specialVariable = oneOf "@*#?-$!"
+specialVariable = oneOf (concat specialVariables)
 paramSubSpecialChars = oneOf "/:+-=%"
 quotableChars = "|&;<>()\\ '\t\n\r\xA0" ++ doubleQuotableChars
 quotable = almostSpace <|> oneOf quotableChars
@@ -1634,7 +1634,7 @@ readDollarBraced = called "parameter expansion" $ do
     word <- readDollarBracedWord
     char '}'
     id <- endSpan start
-    return $ T_DollarBraced id word
+    return $ T_DollarBraced id True word
 
 prop_readDollarExpansion1= isOk readDollarExpansion "$(echo foo; ls\n)"
 prop_readDollarExpansion2= isOk readDollarExpansion "$(  )"
@@ -1661,7 +1661,7 @@ readDollarVariable = do
     let singleCharred p = do
         value <- wrapString ((:[]) <$> p)
         id <- endSpan start
-        return $ (T_DollarBraced id value)
+        return $ (T_DollarBraced id False value)
 
     let positional = do
         value <- singleCharred digit
@@ -1674,7 +1674,7 @@ readDollarVariable = do
     let regular = do
         value <- wrapString readVariableName
         id <- endSpan start
-        return (T_DollarBraced id value) `attempting` do
+        return (T_DollarBraced id False value) `attempting` do
             lookAhead $ char '['
             parseNoteAt pos ErrorC 1087 "Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet)."