diff --git a/CHANGELOG.md b/CHANGELOG.md index ca29f28..38b8e49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - SC2293/SC2294: Warn when calling `eval` with arrays - SC2295: Warn about "${x#$y}" treating $y as a pattern when not quoted - SC2296-SC2301: Improved warnings for bad parameter expansions +- SC2302/SC2303: Warn about loops over array values when using them as keys ### Fixed - SC2102 about repetitions in ranges no longer triggers on [[ -v arr[xx] ]] diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index d670977..b768a0f 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -65,6 +65,7 @@ treeChecks = [ ,checkArrayAssignmentIndices ,checkUseBeforeDefinition ,checkAliasUsedInSameParsingUnit + ,checkArrayValueUsedAsIndex ] runAnalytics :: AnalysisSpec -> [TokenComment] @@ -4484,6 +4485,89 @@ checkUnquotedParameterExpansionPattern params x = surroundWith (getId t) params "\"" +prop_checkArrayValueUsedAsIndex1 = verifyTree checkArrayValueUsedAsIndex "for i in ${arr[@]}; do echo ${arr[i]}; done" +prop_checkArrayValueUsedAsIndex2 = verifyTree checkArrayValueUsedAsIndex "for i in ${arr[@]}; do echo ${arr[$i]}; done" +prop_checkArrayValueUsedAsIndex3 = verifyTree checkArrayValueUsedAsIndex "for i in ${arr[@]}; do echo $((arr[i])); done" +prop_checkArrayValueUsedAsIndex4 = verifyTree checkArrayValueUsedAsIndex "for i in ${arr1[@]} ${arr2[@]}; do echo ${arr1[$i]}; done" +prop_checkArrayValueUsedAsIndex5 = verifyTree checkArrayValueUsedAsIndex "for i in ${arr1[@]} ${arr2[@]}; do echo ${arr2[$i]}; done" +prop_checkArrayValueUsedAsIndex7 = verifyNotTree checkArrayValueUsedAsIndex "for i in ${arr[@]}; do echo ${arr[K]}; done" +prop_checkArrayValueUsedAsIndex8 = verifyNotTree checkArrayValueUsedAsIndex "for i in ${arr[@]}; do i=42; echo ${arr[i]}; done" +prop_checkArrayValueUsedAsIndex9 = verifyNotTree checkArrayValueUsedAsIndex "for i in ${arr[@]}; do echo ${arr2[i]}; done" + +checkArrayValueUsedAsIndex params _ = + doVariableFlowAnalysis read write Map.empty (variableFlow params) + where + write loop@T_ForIn {} _ name (DataString (SourceFrom words)) = do + modify $ Map.insert name (loop, mapMaybe f words) + return [] + where + f x = do + name <- getArrayName x + return (x, name) + + write _ _ name _ = do + modify $ Map.delete name + return [] + + read _ t name = do + varMap <- get + return $ fromMaybe [] $ do + (loop, arrays) <- Map.lookup name varMap + (arrayRef, arrayName) <- getArrayIfUsedAsIndex name t + -- Is this one of the 'for' arrays? + (loopWord, _) <- find ((==arrayName) . snd) arrays + -- Are we still in this loop? + guard $ getId loop `elem` map getId (getPath parents t) + return [ + makeComment WarningC (getId loopWord) 2302 "This loops over values. To loop over keys, use \"${!array[@]}\".", + makeComment WarningC (getId arrayRef) 2303 $ (e4m name) ++ " is an array value, not a key. Use directly or loop over keys instead." + ] + + parents = parentMap params + + getArrayName :: Token -> Maybe String + getArrayName t = do + [T_DollarBraced _ _ l] <- return $ getWordParts t + let str = concat $ oversimplify l + guard $ getBracedModifier str == "[@]" && not ("!" `isPrefixOf` str) + return $ getBracedReference str + + -- This is much uglier than it should be + getArrayIfUsedAsIndex :: String -> Token -> Maybe (Token, String) + getArrayIfUsedAsIndex name t = + case t of + T_DollarBraced _ _ list -> do + let ref = getBracedReference $ concat $ oversimplify list + guard $ ref == name + -- We found a $name. Look up the chain to see if it's ${arr[$name]} + list@T_NormalWord {} <- Map.lookup (getId t) parents + (T_DollarBraced _ _ parentList) <- Map.lookup (getId list) parents + (T_Literal _ head : index : T_Literal _ tail : _) <- return $ getWordParts parentList + let str = concat $ oversimplify list + let modifier = getBracedModifier str + guard $ getId index == getId t + guard $ "[${VAR}]" `isPrefixOf` modifier + return (t, getBracedReference str) + + T_NormalWord wordId list -> do + -- We found just name. Check if it's part of ${something[name]} + parent@(T_DollarBraced _ _ parentList) <- Map.lookup wordId parents + let str = concat $ oversimplify t + let modifier = getBracedModifier str + guard $ ("[" ++ name ++ "]") `isPrefixOf` modifier + return (parent, getBracedReference str) + + TA_Variable indexId ref [] -> do + -- We found arithmetic name. See if it's part of arithmetic arr[name] + guard $ ref == name + (TA_Sequence seqId [element]) <- Map.lookup indexId parents + guard $ getId element == indexId + parent@(TA_Variable arrayId arrayName [element]) <- Map.lookup seqId parents + guard $ getId element == seqId + return (parent, arrayName) + + _ -> Nothing + return [] runTests = $( [| $(forAllProperties) (quickCheckWithResult (stdArgs { maxSuccess = 1 }) ) |])