diff --git a/CHANGELOG.md b/CHANGELOG.md index dce3e27..5ff01c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ - SC2316: Warn about 'local readonly foo' and similar (thanks, patrickxia!) - SC2317: Warn about unreachable commands - SC2318: Warn about backreferences in 'declare x=1 y=$x' +- SC2319/SC2320: Warn when $? refers to echo/printf/[ ]/[[ ]]/test ### Fixed - SC2086: Now uses DFA to make more accurate predictions about values diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index 1429e1b..07bf25b 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -205,6 +205,7 @@ nodeChecks = [ ,checkBatsTestDoesNotUseNegation ,checkCommandIsUnreachable ,checkSpacefulnessCfg + ,checkOverwrittenExitCode ] optionalChecks = map fst optionalTreeChecks @@ -4876,5 +4877,41 @@ checkCommandIsUnreachable params t = _ -> return () where id = getId t + +prop_checkOverwrittenExitCode1 = verify checkOverwrittenExitCode "x; [ $? -eq 1 ] || [ $? -eq 2 ]" +prop_checkOverwrittenExitCode2 = verifyNot checkOverwrittenExitCode "x; [ $? -eq 1 ]" +prop_checkOverwrittenExitCode3 = verify checkOverwrittenExitCode "x; echo \"Exit is $?\"; [ $? -eq 0 ]" +prop_checkOverwrittenExitCode4 = verifyNot checkOverwrittenExitCode "x; [ $? -eq 0 ]" +checkOverwrittenExitCode params t = + case t of + T_DollarBraced id _ val | getLiteralString val == Just "?" -> check id + _ -> return () + where + check id = sequence_ $ do + state <- CF.getIncomingState (cfgAnalysis params) id + let exitCodeIds = CF.exitCodes state + guard . not $ S.null exitCodeIds + + let idToToken = idMap params + exitCodeTokens <- sequence $ map (\k -> Map.lookup k idToToken) $ S.toList exitCodeIds + return $ do + when (all isCondition exitCodeTokens) $ + warn id 2319 "This $? refers to a condition, not a command. Assign to a variable to avoid it being overwritten." + when (all isPrinting exitCodeTokens) $ + warn id 2320 "This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten." + + isCondition t = + case t of + T_Condition {} -> True + T_SimpleCommand {} -> getCommandName t == Just "test" + _ -> False + + isPrinting t = + case getCommandBasename t of + Just "echo" -> True + Just "printf" -> True + _ -> False + + return [] runTests = $( [| $(forAllProperties) (quickCheckWithResult (stdArgs { maxSuccess = 1 }) ) |]) diff --git a/src/ShellCheck/AnalyzerLib.hs b/src/ShellCheck/AnalyzerLib.hs index e998f2c..88da89e 100644 --- a/src/ShellCheck/AnalyzerLib.hs +++ b/src/ShellCheck/AnalyzerLib.hs @@ -89,6 +89,8 @@ data Parameters = Parameters { hasPipefail :: Bool, -- A linear (bad) analysis of data flow variableFlow :: [StackData], + -- A map from Id to Token + idMap :: Map.Map Id Token, -- A map from Id to parent Token parentMap :: Map.Map Id Token, -- The shell type, such as Bash or Ksh @@ -218,6 +220,7 @@ makeParameters spec = params Sh -> True Ksh -> containsPipefail root, shellTypeSpecified = isJust (asShellType spec) || isJust (asFallbackShell spec), + idMap = getTokenMap root, parentMap = getParentTree root, variableFlow = getVariableFlow params root, tokenPositions = asTokenPositions spec, diff --git a/src/ShellCheck/CFGAnalysis.hs b/src/ShellCheck/CFGAnalysis.hs index 893c34a..bb90860 100644 --- a/src/ShellCheck/CFGAnalysis.hs +++ b/src/ShellCheck/CFGAnalysis.hs @@ -804,6 +804,7 @@ fulfillsDependency ctx entry dep = -- it won't be found by the normal check. DepIsRecursive node val | node == entry -> return True DepIsRecursive node val -> return $ val == any (\f -> entryPoint f == node) (cStack ctx) + DepExitCodes val -> (== val) <$> peekStack (\s k -> sExitCodes s) S.empty ctx () -- _ -> error $ "Unknown dep " ++ show dep where peek scope = peekStack getVariableWithScope $ if scope == GlobalScope then (unknownVariableState, GlobalScope) else (unsetVariableState, LocalScope)