From 5e6383578d45908a61ecfa3563f915e4a8daefa0 Mon Sep 17 00:00:00 2001 From: Vidar Holen Date: Mon, 28 Jul 2025 15:16:22 -0700 Subject: [PATCH] Make SC2335 and friends optional (avoid-negated-conditions) --- CHANGELOG.md | 5 ++- src/ShellCheck/Analytics.hs | 80 ++++++++++++++++++++----------------- 2 files changed, 48 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b9a692..c3e4940 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index 05f2327..373d495 100644 --- a/src/ShellCheck/Analytics.hs +++ b/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_checkInvertedStringTest2 = verify checkInvertedStringTest "! [[ -n $var ]]" -prop_checkInvertedStringTest3 = verifyNot checkInvertedStringTest "! [ -x $var ]" -prop_checkInvertedStringTest4 = verifyNot checkInvertedStringTest "[[ ! -w $var ]]" -prop_checkInvertedStringTest5 = verifyNot checkInvertedStringTest "[ -z $var ]" -prop_checkInvertedStringTest6 = verify checkInvertedStringTest "! [ $var != foo ]" -prop_checkInvertedStringTest7 = verify checkInvertedStringTest "[[ ! $var == foo ]]" -prop_checkInvertedStringTest8 = verifyNot checkInvertedStringTest "! [[ $var =~ .* ]]" -prop_checkInvertedStringTest9 = verify checkInvertedStringTest "[ ! $var -eq 0 ]" -prop_checkInvertedStringTest10 = verify checkInvertedStringTest "! [[ $var -gt 3 ]]" -checkInvertedStringTest _ t = +prop_checkUnnecessarilyInvertedTest1 = verify checkUnnecessarilyInvertedTest "[ ! -z $var ]" +prop_checkUnnecessarilyInvertedTest2 = verify checkUnnecessarilyInvertedTest "! [[ -n $var ]]" +prop_checkUnnecessarilyInvertedTest3 = verifyNot checkUnnecessarilyInvertedTest "! [ -x $var ]" +prop_checkUnnecessarilyInvertedTest4 = verifyNot checkUnnecessarilyInvertedTest "[[ ! -w $var ]]" +prop_checkUnnecessarilyInvertedTest5 = verifyNot checkUnnecessarilyInvertedTest "[ -z $var ]" +prop_checkUnnecessarilyInvertedTest6 = verify checkUnnecessarilyInvertedTest "! [ $var != foo ]" +prop_checkUnnecessarilyInvertedTest7 = verify checkUnnecessarilyInvertedTest "[[ ! $var == foo ]]" +prop_checkUnnecessarilyInvertedTest8 = verifyNot checkUnnecessarilyInvertedTest "! [[ $var =~ .* ]]" +prop_checkUnnecessarilyInvertedTest9 = verify checkUnnecessarilyInvertedTest "[ ! $var -eq 0 ]" +prop_checkUnnecessarilyInvertedTest10 = verify checkUnnecessarilyInvertedTest "! [[ $var -gt 3 ]]" +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 _ _))]) -> - 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_Redirecting _ _ (T_Condition _ _ (TC_Binary _ bracketStyle op _ _))]) -> + maybeSuggestRewrite False bracketStyle (getId t) op _ -> 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'"