From 95b1185882845afe0bafed2ebc9b086af86c05be Mon Sep 17 00:00:00 2001
From: Vidar Holen <spam@vidarholen.net>
Date: Wed, 22 May 2019 17:06:48 -0700
Subject: [PATCH] Inform about ineffectual ! on commands (fixes #1531)

---
 CHANGELOG.md                |  1 +
 src/ShellCheck/Analytics.hs | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 81bd2fa..262849f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@
 - 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
+- SC2251: Inform about ineffectual ! in front of commands
 - 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)
diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs
index 0db41e8..36b941e 100644
--- a/src/ShellCheck/Analytics.hs
+++ b/src/ShellCheck/Analytics.hs
@@ -190,6 +190,7 @@ nodeChecks = [
     ,checkInvertedStringTest
     ,checkRedirectionToCommand
     ,checkDollarQuoteParen
+    ,checkUselessBang
     ]
 
 optionalChecks = map fst optionalTreeChecks
@@ -3357,5 +3358,41 @@ checkDefaultCase _ t =
         pg <- wordToExactPseudoGlob pat
         return $ pseudoGlobIsSuperSetof pg [PGMany]
 
+prop_checkUselessBang1 = verify checkUselessBang "! true; rest"
+prop_checkUselessBang2 = verify checkUselessBang "while true; do ! true; done"
+prop_checkUselessBang3 = verifyNot checkUselessBang "if ! true; then true; fi"
+prop_checkUselessBang4 = verifyNot checkUselessBang "( ! true )"
+prop_checkUselessBang5 = verifyNot checkUselessBang "{ ! true; }"
+prop_checkUselessBang6 = verifyNot checkUselessBang "x() { ! [ x ]; }"
+checkUselessBang params t = mapM_ check (getNonReturningCommands t)
+  where
+    check t =
+        case t of
+            T_Banged id _ ->
+                info id 2251 "This ! is not on a condition and skips errexit. Use { ! ...; } to errexit, or verify usage."
+            _ -> return ()
+
+    -- Get all the subcommands that aren't likely to be the return value
+    getNonReturningCommands :: Token -> [Token]
+    getNonReturningCommands t =
+        case t of
+            T_Script _ _ list -> dropLast list
+            T_BraceGroup _ list -> dropLast list
+            T_Subshell _ list -> dropLast list
+            T_WhileExpression _ conds cmds -> dropLast conds ++ cmds
+            T_UntilExpression _ conds cmds -> dropLast conds ++ cmds
+            T_ForIn _ _ _ list -> list
+            T_ForArithmetic _ _ _ _ list -> list
+            T_Annotation _ _ t -> getNonReturningCommands t
+            T_IfExpression _ conds elses ->
+                concatMap (dropLast . fst) conds ++ concatMap snd conds ++ elses
+            _ -> []
+
+    dropLast t =
+        case t of
+            [_] -> []
+            x:rest -> x : dropLast rest
+            _ -> []
+
 return []
 runTests =  $( [| $(forAllProperties) (quickCheckWithResult (stdArgs { maxSuccess = 1 }) ) |])