From b7a8b090d2dbdc3783060514a799cd57f20b3cbd Mon Sep 17 00:00:00 2001
From: Vidar Holen <spam@vidarholen.net>
Date: Sun, 25 Feb 2018 13:27:01 -0800
Subject: [PATCH] SC2229: Warn about 'read $var'

---
 CHANGELOG.md                  |  1 +
 ShellCheck/AnalyzerLib.hs     | 29 ++++++++++++++++++++
 ShellCheck/Checks/Commands.hs | 51 +++++++++++++++++++++++++++--------
 3 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5d3f28e..22d1d0a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,6 @@
 ## Latest - ???
 ### Added
+- SC2229: Warn about 'read $var'
 - SC2227: Warn about redirections in the middle of 'find' commands
 - SC2224,SC2225,SC2226: Warn when using mv/cp/ln without a destination
 - SC2223: Quote warning specific to `: ${var=value}`
diff --git a/ShellCheck/AnalyzerLib.hs b/ShellCheck/AnalyzerLib.hs
index 361a67e..efb4aea 100644
--- a/ShellCheck/AnalyzerLib.hs
+++ b/ShellCheck/AnalyzerLib.hs
@@ -837,7 +837,36 @@ isQuotedAlternativeReference t =
   where
     re = mkRegex "(^|\\]):?\\+"
 
+-- getOpts "erd:u:" will parse a SimpleCommand like
+--     read -re -d : -u 3 bar
+-- into
+--     Just [("r", -re), ("e", -re), ("d", :), ("u", 3), ("", bar)]
+-- where flags with arguments map to arguments, while others map to themselves.
+-- Any unrecognized flag will result in Nothing.
+getOpts :: String -> Token -> Maybe [(String, Token)]
+getOpts string cmd = process flags
+  where
+    flags = getAllFlags cmd
+    flagList (c:':':rest) = ([c], True) : flagList rest
+    flagList (c:rest) = ([c], False) : flagList rest
+    flagList [] = []
+    flagMap = Map.fromList $ ("", False) : flagList string
 
+    process [] = return []
+    process [(token, flag)] = do
+        takesArg <- Map.lookup flag flagMap
+        guard $ not takesArg
+        return [(flag, token)]
+    process ((token1, flag1):rest2@((token2, flag2):rest)) = do
+        takesArg <- Map.lookup flag1 flagMap
+        if takesArg
+            then do
+                guard $ flag2 == ""
+                more <- process rest
+                return $ (flag1, token2) : more
+            else do
+                more <- process rest2
+                return $ (flag1, token1) : more
 
 return []
 runTests =  $( [| $(forAllProperties) (quickCheckWithResult (stdArgs { maxSuccess = 1 }) ) |])
diff --git a/ShellCheck/Checks/Commands.hs b/ShellCheck/Checks/Commands.hs
index 161fb4b..5ecc8cc 100644
--- a/ShellCheck/Checks/Commands.hs
+++ b/ShellCheck/Checks/Commands.hs
@@ -90,6 +90,7 @@ commandChecks = [
     ,checkLetUsage
     ,checkMvArguments, checkCpArguments, checkLnArguments
     ,checkFindRedirections
+    ,checkReadExpansions
     ]
 
 buildCommandMap :: [CommandCheck] -> Map.Map CommandName (Token -> Analysis)
@@ -588,19 +589,47 @@ prop_checkExportedExpansions1 = verify checkExportedExpansions "export $foo"
 prop_checkExportedExpansions2 = verify checkExportedExpansions "export \"$foo\""
 prop_checkExportedExpansions3 = verifyNot checkExportedExpansions "export foo"
 prop_checkExportedExpansions4 = verifyNot checkExportedExpansions "export ${foo?}"
-checkExportedExpansions = CommandCheck (Exactly "export") (check . arguments)
+checkExportedExpansions = CommandCheck (Exactly "export") (mapM_ check . arguments)
   where
-    check = mapM_ checkForVariables
-    checkForVariables f =
-        case getWordParts f of
-            [t@(T_DollarBraced {})] -> potentially $ do
-                let contents = bracedString t
-                let name = getBracedReference contents
-                guard $ name == contents
-                return . warn (getId t) 2163 $
-                    "This does not export '" ++ name ++ "'. Remove $/${} for that, or use ${var?} to quiet."
-            _ -> return ()
+    check t = potentially $ do
+        var <- getSingleUnmodifiedVariable t
+        let name = bracedString var
+        return . warn (getId t) 2163 $
+            "This does not export '" ++ name ++ "'. Remove $/${} for that, or use ${var?} to quiet."
 
+prop_checkReadExpansions1 = verify checkReadExpansions "read $var"
+prop_checkReadExpansions2 = verify checkReadExpansions "read -r $var"
+prop_checkReadExpansions3 = verifyNot checkReadExpansions "read -p $var"
+prop_checkReadExpansions4 = verifyNot checkReadExpansions "read -rd $delim name"
+prop_checkReadExpansions5 = verify checkReadExpansions "read \"$var\""
+prop_checkReadExpansions6 = verify checkReadExpansions "read -a $var"
+prop_checkReadExpansions7 = verifyNot checkReadExpansions "read $1"
+prop_checkReadExpansions8 = verifyNot checkReadExpansions "read ${var?}"
+checkReadExpansions = CommandCheck (Exactly "read") check
+  where
+    options = getOpts "sreu:n:N:i:p:a:"
+    getVars cmd = fromMaybe [] $ do
+        opts <- options cmd
+        return . map snd $ filter (\(x,_) -> x == "" || x == "a") opts
+
+    check cmd = mapM_ warning $ getVars cmd
+    warning t = potentially $ do
+        var <- getSingleUnmodifiedVariable t
+        let name = bracedString var
+        guard $ isVariableName name   -- e.g. not $1
+        return . warn (getId t) 2229 $
+            "This does not read '" ++ name ++ "'. Remove $/${} for that, or use ${var?} to quiet."
+
+-- Return the single variable expansion that makes up this word, if any.
+-- e.g. $foo -> $foo, "$foo"'' -> $foo , "hello $name" -> Nothing
+getSingleUnmodifiedVariable :: Token -> Maybe Token
+getSingleUnmodifiedVariable word =
+    case getWordParts word of
+        [t@(T_DollarBraced {})] ->
+            let contents = bracedString t
+                name = getBracedReference contents
+            in guard (contents == name) >> return t
+        _ -> Nothing
 
 prop_checkAliasesUsesArgs1 = verify checkAliasesUsesArgs "alias a='cp $1 /a'"
 prop_checkAliasesUsesArgs2 = verifyNot checkAliasesUsesArgs "alias $1='foo'"