Make SC2335 and friends optional (avoid-negated-conditions)

CHANGELOG.md

@@ -5,12 +5,15 @@
 - SC2330: Warn about unsupported glob matches with [[ .. ]] in BusyBox.
 - SC2331: Suggest using standard -e instead of unary -a in tests.
 - SC2332: Warn about `[ ! -o opt ]` being unconditionally true in Bash.
-- SC2335: Avoid double negative of a binary operator in test—suggest `[ a != b ]` over `[ ! a = b ]` and `! [ a = b ]`, and so forth.
 - SC3062: Warn about bashism `[ -o opt ]`.
+- Optional `avoid-negated-conditions`: suggest replacing `[ ! a -eq b ]`
+  with `[ a -ne b ]`, and similar for -ge/-lt/=/!=/etc (SC2335).
 - Precompiled binaries for Linux riscv64 (linux.riscv64)
 ### Changed
 - SC2002 about Useless Use Of Cat is now disabled by default. It can be
   re-enabled with `--enable=useless-use-of-cat` or equivalent directive.
+- SC2236/SC2237 about replacing `[ ! -n .. ]` with `[ -z ]` and vice versa
+  is now optional under `avoid-negated-conditions`.
 - SC2015 about `A && B || C` no longer triggers when B is a test command.
 - SC3012: Do not warn about `\<` and `\>` in test/[] as specified in POSIX.1-2024
 - Diff output now uses / as path separator on Windows

src/ShellCheck/Analytics.hs

@@ -183,7 +183,6 @@ nodeChecks = [
     ,checkPipeToNowhere
     ,checkForLoopGlobVariables
     ,checkSubshelledTests
-    ,checkInvertedStringTest
     ,checkRedirectionToCommand
     ,checkDollarQuoteParen
     ,checkUselessBang
@@ -233,6 +232,13 @@ optionalTreeChecks = [
         cdNegative = "[ -n \"$var\" ]"
     }, nodeChecksToTreeCheck [checkNullaryExpansionTest])
 
+    ,(newCheckDescription {
+        cdName = "avoid-negated-conditions",
+        cdDescription = "Suggest removing unnecessary comparison negations",
+        cdPositive = "[ ! \"$var\" -eq 1 ]",
+        cdNegative = "[ \"$var\" -ne 1 ]"
+    }, nodeChecksToTreeCheck [checkUnnecessarilyInvertedTest])
+
     ,(newCheckDescription {
         cdName = "add-default-case",
         cdDescription = "Suggest adding a default case in `case` statements",
@@ -3991,55 +3997,57 @@ checkSubshelledTests params t =
             T_Annotation {} -> True
             _ -> False
 
-prop_checkInvertedStringTest1 = verify checkInvertedStringTest "[ ! -z $var ]"
+prop_checkUnnecessarilyInvertedTest1 = verify checkUnnecessarilyInvertedTest "[ ! -z $var ]"
-prop_checkInvertedStringTest2 = verify checkInvertedStringTest "! [[ -n $var ]]"
+prop_checkUnnecessarilyInvertedTest2 = verify checkUnnecessarilyInvertedTest "! [[ -n $var ]]"
-prop_checkInvertedStringTest3 = verifyNot checkInvertedStringTest "! [ -x $var ]"
+prop_checkUnnecessarilyInvertedTest3 = verifyNot checkUnnecessarilyInvertedTest "! [ -x $var ]"
-prop_checkInvertedStringTest4 = verifyNot checkInvertedStringTest "[[ ! -w $var ]]"
+prop_checkUnnecessarilyInvertedTest4 = verifyNot checkUnnecessarilyInvertedTest "[[ ! -w $var ]]"
-prop_checkInvertedStringTest5 = verifyNot checkInvertedStringTest "[ -z $var ]"
+prop_checkUnnecessarilyInvertedTest5 = verifyNot checkUnnecessarilyInvertedTest "[ -z $var ]"
-prop_checkInvertedStringTest6 = verify checkInvertedStringTest "! [ $var != foo ]"
+prop_checkUnnecessarilyInvertedTest6 = verify checkUnnecessarilyInvertedTest "! [ $var != foo ]"
-prop_checkInvertedStringTest7 = verify checkInvertedStringTest "[[ ! $var == foo ]]"
+prop_checkUnnecessarilyInvertedTest7 = verify checkUnnecessarilyInvertedTest "[[ ! $var == foo ]]"
-prop_checkInvertedStringTest8 = verifyNot checkInvertedStringTest "! [[ $var =~ .* ]]"
+prop_checkUnnecessarilyInvertedTest8 = verifyNot checkUnnecessarilyInvertedTest "! [[ $var =~ .* ]]"
-prop_checkInvertedStringTest9 = verify checkInvertedStringTest "[ ! $var -eq 0 ]"
+prop_checkUnnecessarilyInvertedTest9 = verify checkUnnecessarilyInvertedTest "[ ! $var -eq 0 ]"
-prop_checkInvertedStringTest10 = verify checkInvertedStringTest "! [[ $var -gt 3 ]]"
+prop_checkUnnecessarilyInvertedTest10 = verify checkUnnecessarilyInvertedTest "! [[ $var -gt 3 ]]"
-checkInvertedStringTest _ t =
+checkUnnecessarilyInvertedTest _ t =
     case t of
         TC_Unary _ _ "!" (TC_Unary _ _ op _) ->
             case op of
                 "-n" -> style (getId t) 2236 "Use -z instead of ! -n."
                 "-z" -> style (getId t) 2236 "Use -n instead of ! -z."
                 _ -> return ()
-        TC_Unary _ _ "!" (TC_Binary _ _ op _ _) ->
-            case op of
-                "=" -> style (getId t) 2335 "Use a != b instead of ! a = b."
-                "==" -> style (getId t) 2335 "Use a != b instead of ! a == b."
-                "!=" -> style (getId t) 2335 "Use a = b instead of ! a != b."
-                "-eq" -> style (getId t) 2335 "Use a -ne b instead of ! a -eq b."
-                "-ne" -> style (getId t) 2335 "Use a -eq b instead of ! a -ne b."
-                "-gt" -> style (getId t) 2335 "Use a -le b instead of ! a -gt b."
-                "-ge" -> style (getId t) 2335 "Use a -lt b instead of ! a -ge b."
-                "-lt" -> style (getId t) 2335 "Use a -ge b instead of ! a -lt b."
-                "-le" -> style (getId t) 2335 "Use a -gt b instead of ! a -le b."
-                _ -> return ()
         T_Banged _ (T_Pipeline _ _
           [T_Redirecting _ _ (T_Condition _ _ (TC_Unary _ _ op _))]) ->
             case op of
                 "-n" -> style (getId t) 2237 "Use [ -z .. ] instead of ! [ -n .. ]."
                 "-z" -> style (getId t) 2237 "Use [ -n .. ] instead of ! [ -z .. ]."
                 _ -> return ()
+        TC_Unary _ _ "!" (TC_Binary _ bracketStyle op _ _) ->
+            maybeSuggestRewrite True bracketStyle (getId t) op
         T_Banged _ (T_Pipeline _ _
-          [T_Redirecting _ _ (T_Condition _ _ (TC_Binary _ _ op _ _))]) ->
+          [T_Redirecting _ _ (T_Condition _ _ (TC_Binary _ bracketStyle op _ _))]) ->
-            case op of
+            maybeSuggestRewrite False bracketStyle (getId t) op
-                "=" -> style (getId t) 2335 "Use [ a != b ] instead of ! [ a = b ]."
-                "==" -> style (getId t) 2335 "Use [[ a != b ]] instead of ! [[ a == b ]]."
-                "!=" -> style (getId t) 2335 "Use [ a = b ] instead of ! [ a != b ]."
-                "-eq" -> style (getId t) 2335 "Use [ a -ne b ] instead of ! [ a -eq b ]."
-                "-ne" -> style (getId t) 2335 "Use [ a -eq b ] instead of ! [ a -ne b ]."
-                "-gt" -> style (getId t) 2335 "Use [ a -le b ] instead of ! [ a -gt b ]."
-                "-ge" -> style (getId t) 2335 "Use [ a -lt b ] instead of ! [ a -ge b ]."
-                "-lt" -> style (getId t) 2335 "Use [ a -ge b ] instead of ! [ a -lt b ]."
-                "-le" -> style (getId t) 2335 "Use [ a -gt b ] instead of ! [ a -le b ]."
-                _ -> return ()
         _ -> return ()
+  where
+    inversionMap = Map.fromList [
+        ("=",  "!="),
+        ("==", "!="),
+        ("!=", "="),
+        ("-eq", "-ne"),
+        ("-ne", "-eq"),
+        ("-le", "-gt"),
+        ("-gt", "-le"),
+        ("-ge", "-lt"),
+        ("-lt", "-ge")
+      ]
+    maybeSuggestRewrite bangInside bracketStyle id op = sequence_ $ do
+        newOp <- Map.lookup op inversionMap
+        let oldExpr = "a " ++ op ++ " b"
+        let newExpr = "a " ++ newOp ++ " b"
+        let bracket s = if bracketStyle == SingleBracket then "[ " ++ s ++ " ]" else "[[ " ++ s ++ " ]]"
+        return $
+            if bangInside
+                then style id 2335 $ "Use " ++ newExpr ++ " instead of ! " ++ oldExpr ++ "."
+                else style id 2335 $ "Use " ++ (bracket newExpr) ++ " instead of ! " ++ (bracket oldExpr) ++ "."
+
 
 prop_checkRedirectionToCommand1 = verify checkRedirectionToCommand "ls > rm"
 prop_checkRedirectionToCommand2 = verifyNot checkRedirectionToCommand "ls > 'rm'"