diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e87af8..b7a5fc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - SC2102 about repetitions in ranges no longer triggers on [[ -v arr[xx] ]] +- SC2155 now recognizes `typeset` and local read-only `declare` statements - SC2290: Warn about misused = in declare & co, which were not caught by SC2270+ - The flag --color=auto no longer outputs color when TERM is "dumb" or unset diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index 141529d..e5fd4cc 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -169,7 +169,6 @@ nodeChecks = [ ,checkTestArgumentSplitting ,checkConcatenatedDollarAt ,checkTildeInPath - ,checkMaskedReturns ,checkReadWithoutR ,checkLoopVariableReassignment ,checkTrailingBracket @@ -2970,31 +2969,6 @@ checkTestArgumentSplitting params t = err (getId token) 2255 "[ ] does not apply arithmetic evaluation. Evaluate with $((..)) for numbers, or use string comparator for strings." -prop_checkMaskedReturns1 = verify checkMaskedReturns "f() { local a=$(false); }" -prop_checkMaskedReturns2 = verify checkMaskedReturns "declare a=$(false)" -prop_checkMaskedReturns3 = verify checkMaskedReturns "declare a=\"`false`\"" -prop_checkMaskedReturns4 = verify checkMaskedReturns "readonly a=$(false)" -prop_checkMaskedReturns5 = verify checkMaskedReturns "readonly a=\"`false`\"" -prop_checkMaskedReturns6 = verifyNot checkMaskedReturns "declare a; a=$(false)" -prop_checkMaskedReturns7 = verifyNot checkMaskedReturns "f() { local -r a=$(false); }" -prop_checkMaskedReturns8 = verifyNot checkMaskedReturns "a=$(false); readonly a" -checkMaskedReturns _ t@(T_SimpleCommand id _ (cmd:rest)) = sequence_ $ do - name <- getCommandName t - guard $ name `elem` ["declare", "export", "readonly"] - || name == "local" && "r" `notElem` map snd (getAllFlags t) - return $ mapM_ checkArgs rest - where - checkArgs (T_Assignment id _ _ _ word) | any hasReturn $ getWordParts word = - warn id 2155 "Declare and assign separately to avoid masking return values." - checkArgs _ = return () - - hasReturn t = case t of - T_Backticked {} -> True - T_DollarExpansion {} -> True - _ -> False -checkMaskedReturns _ _ = return () - - prop_checkReadWithoutR1 = verify checkReadWithoutR "read -a foo" prop_checkReadWithoutR2 = verifyNot checkReadWithoutR "read -ar foo" prop_checkReadWithoutR3 = verifyNot checkReadWithoutR "read -t 0" diff --git a/src/ShellCheck/Checks/Commands.hs b/src/ShellCheck/Checks/Commands.hs index 0d8deae..ec012c2 100644 --- a/src/ShellCheck/Checks/Commands.hs +++ b/src/ShellCheck/Checks/Commands.hs @@ -95,12 +95,12 @@ commandChecks = [ ,checkSourceArgs ,checkChmodDashr ,checkXargsDashi - ,checkArgComparison "local" - ,checkArgComparison "declare" - ,checkArgComparison "export" - ,checkArgComparison "readonly" - ,checkArgComparison "typeset" ] + ++ map checkArgComparison declaringCommands + ++ map checkMaskedReturns declaringCommands + +declaringCommands = ["local", "declare", "export", "readonly", "typeset"] + optionalChecks = map fst optionalCommandChecks optionalCommandChecks :: [(CheckDescription, CommandCheck)] @@ -1174,5 +1174,62 @@ checkArgComparison str = CommandCheck (Exactly str) wordsWithEqual T_NormalWord _ (x:_) -> getId x _ -> getId t + +prop_checkMaskedReturns1 = verify (checkMaskedReturns "local") "f() { local a=$(false); }" +prop_checkMaskedReturns2 = verify (checkMaskedReturns "declare") "declare a=$(false)" +prop_checkMaskedReturns3 = verify (checkMaskedReturns "declare") "declare a=\"`false`\"" +prop_checkMaskedReturns4 = verify (checkMaskedReturns "readonly") "readonly a=$(false)" +prop_checkMaskedReturns5 = verify (checkMaskedReturns "readonly") "readonly a=\"`false`\"" +prop_checkMaskedReturns6 = verifyNot (checkMaskedReturns "declare") "declare a; a=$(false)" +prop_checkMaskedReturns7 = verifyNot (checkMaskedReturns "local") "f() { local -r a=$(false); }" +prop_checkMaskedReturns8 = verifyNot (checkMaskedReturns "readonly") "a=$(false); readonly a" +prop_checkMaskedReturns9 = verify (checkMaskedReturns "typeset") "#!/bin/ksh\n f() { typeset -r x=$(false); }" +prop_checkMaskedReturns10 = verifyNot (checkMaskedReturns "typeset") "#!/bin/ksh\n function f { typeset -r x=$(false); }" +prop_checkMaskedReturns11 = verifyNot (checkMaskedReturns "typeset") "#!/bin/bash\n f() { typeset -r x=$(false); }" +prop_checkMaskedReturns12 = verify (checkMaskedReturns "typeset") "typeset -r x=$(false);" +prop_checkMaskedReturns13 = verify (checkMaskedReturns "typeset") "f() { typeset -g x=$(false); }" +prop_checkMaskedReturns14 = verify (checkMaskedReturns "declare") "declare x=${ false; }" +prop_checkMaskedReturns15 = verify (checkMaskedReturns "declare") "f() { declare x=$(false); }" +checkMaskedReturns str = CommandCheck (Exactly str) checkCmd + where + checkCmd t = do + path <- getPathM t + shell <- asks shellType + sequence_ $ do + name <- getCommandName t + + let flags = map snd (getAllFlags t) + let hasDashR = "r" `elem` flags + let hasDashG = "g" `elem` flags + let isInScopedFunction = any (isScopedFunction shell) path + + let isLocal = not hasDashG && isLocalInFunction name && isInScopedFunction + let isReadOnly = name == "readonly" || hasDashR + + -- Don't warn about local variables that are declared readonly, + -- because the workaround `local x; x=$(false); local -r x;` is annoying + guard . not $ isLocal && isReadOnly + + return $ mapM_ checkArgs $ arguments t + + checkArgs (T_Assignment id _ _ _ word) | any hasReturn $ getWordParts word = + warn id 2155 "Declare and assign separately to avoid masking return values." + checkArgs _ = return () + + isLocalInFunction = (`elem` ["local", "declare", "typeset"]) + isScopedFunction shell t = + case t of + T_BatsTest {} -> True + -- In ksh, only functions declared with 'function' have their own scope + T_Function _ (FunctionKeyword hasFunction) _ _ _ -> shell /= Ksh || hasFunction + _ -> False + + hasReturn t = case t of + T_Backticked {} -> True + T_DollarExpansion {} -> True + T_DollarBraceCommandExpansion {} -> True + _ -> False + + return [] runTests = $( [| $(forAllProperties) (quickCheckWithResult (stdArgs { maxSuccess = 1 }) ) |])