Skip to content

Show info about set -e suppression during function calls #2303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
koalaman merged 1 commit into from
Sep 4, 2021
Merged

Show info about set -e suppression during function calls #2303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 86 additions & 12 deletions src/ShellCheck/Analytics.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,13 @@ optionalTreeChecks = [
cdPositive = "[ -e /etc/issue ]",
cdNegative = "[[ -e /etc/issue ]]"
}, checkRequireDoubleBracket)

,(newCheckDescription {
cdName = "check-set-e-suppressed",
cdDescription = "Notify when set -e is suppressed during function invocation",
cdPositive = "set -e; func() { cp *.txt ~/backup; rm *.txt; }; func && echo ok",
cdNegative = "set -e; func() { cp *.txt ~/backup; rm *.txt; }; func; echo ok"
}, checkSetESuppressed)
]

optionalCheckMap :: Map.Map String (Parameters -> Token -> [TokenComment])
Expand Down Expand Up @@ -393,6 +400,24 @@ replaceToken id params r =
surroundWith id params s = fixWith [replaceStart id params 0 s, replaceEnd id params 0 s]
fixWith fixes = newFix { fixReplacements = fixes }

analyse f t = execState (doAnalysis f t) []

-- Make a map from functions to definition IDs
functions t = Map.fromList $ analyse findFunctions t
findFunctions (T_Function id _ _ name _)
= modify ((name, id):)
findFunctions _ = return ()

-- Make a map from aliases to definition IDs
aliases t = Map.fromList $ analyse findAliases t
findAliases t@(T_SimpleCommand _ _ (_:args))
| t `isUnqualifiedCommand` "alias" = mapM_ getAlias args
findAliases _ = return ()
getAlias arg =
let string = onlyLiteralString arg
in when ('=' `elem` string) $
modify ((takeWhile (/= '=') string, getId arg):)
Copy link
Copy Markdown
Owner

@koalaman koalaman Aug 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more correct/expected to treat an alias invocation as a single command because that's almost universally what they are. This would happen if you just ignore aliases.

Sure it would basically mean copy-pasting findFunctions, but A. they're small, B. they do slightly different things (trying to find shell-only definitions vs find functions), and C. you could optionally add (name, containsSetE t) as the value to easily and efficiently avoid triggering on f() { set -e; false; echo foo; }; x=$(f);

Copy link
Copy Markdown
Contributor Author

@DoxasticFox DoxasticFox Aug 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring aliases SGTM, but I didn't do the optional bit because I worry it'll be brittle.


prop_checkEchoWc3 = verify checkEchoWc "n=$(echo $foo | wc -c)"
checkEchoWc _ (T_Pipeline id _ [a, b]) =
when (acmd == ["echo", "${VAR}"]) $
Expand Down Expand Up @@ -2239,21 +2264,11 @@ checkFunctionsUsedExternally params t =
findExecFlags = ["-exec", "-execdir", "-ok"]
dropFlags = dropWhile (\x -> "-" `isPrefixOf` fst x)

-- Make a map from functions/aliases to definition IDs
analyse f t = execState (doAnalysis f t) []
functions = Map.fromList $ analyse findFunctions t
findFunctions (T_Function id _ _ name _) = modify ((name, id):)
findFunctions t@(T_SimpleCommand id _ (_:args))
| t `isUnqualifiedCommand` "alias" = mapM_ getAlias args
findFunctions _ = return ()
getAlias arg =
let string = onlyLiteralString arg
in when ('=' `elem` string) $
modify ((takeWhile (/= '=') string, getId arg):)
functionsAndAliases = Map.union (functions t) (aliases t)

checkArg cmd (_, arg) = sequence_ $ do
literalArg <- getUnquotedLiteral arg -- only consider unquoted literals
definitionId <- Map.lookup literalArg functions
definitionId <- Map.lookup literalArg functionsAndAliases
return $ do
warn (getId arg) 2033
"Shell functions can't be passed to external commands."
Expand Down Expand Up @@ -4583,6 +4598,65 @@ checkArrayValueUsedAsIndex params _ =

_ -> Nothing

prop_checkSetESuppressed1 = verifyTree checkSetESuppressed "set -e; f(){ :; }; x=$(f)"
prop_checkSetESuppressed2 = verifyNotTree checkSetESuppressed "f(){ :; }; x=$(f)"
prop_checkSetESuppressed3 = verifyNotTree checkSetESuppressed "set -e; f(){ :; }; x=$(set -e; f)"
prop_checkSetESuppressed4 = verifyTree checkSetESuppressed "set -e; f(){ :; }; baz=$(set -e; f) || :"
prop_checkSetESuppressed5 = verifyNotTree checkSetESuppressed "set -e; f(){ :; }; baz=$(echo \"\") || :"
prop_checkSetESuppressed6 = verifyTree checkSetESuppressed "set -e; f(){ :; }; f && echo"
prop_checkSetESuppressed7 = verifyTree checkSetESuppressed "set -e; f(){ :; }; f || echo"
prop_checkSetESuppressed8 = verifyNotTree checkSetESuppressed "set -e; f(){ :; }; echo && f"
prop_checkSetESuppressed9 = verifyNotTree checkSetESuppressed "set -e; f(){ :; }; echo || f"
prop_checkSetESuppressed10 = verifyTree checkSetESuppressed "set -e; f(){ :; }; ! f"
prop_checkSetESuppressed11 = verifyTree checkSetESuppressed "set -e; f(){ :; }; if f; then :; fi"
prop_checkSetESuppressed12 = verifyTree checkSetESuppressed "set -e; f(){ :; }; if set -e; f; then :; fi"
prop_checkSetESuppressed13 = verifyTree checkSetESuppressed "set -e; f(){ :; }; while f; do :; done"
prop_checkSetESuppressed14 = verifyTree checkSetESuppressed "set -e; f(){ :; }; while set -e; f; do :; done"
prop_checkSetESuppressed15 = verifyTree checkSetESuppressed "set -e; f(){ :; }; until f; do :; done"
prop_checkSetESuppressed16 = verifyTree checkSetESuppressed "set -e; f(){ :; }; until set -e; f; do :; done"
prop_checkSetESuppressed17 = verifyNotTree checkSetESuppressed "set -e; f(){ :; }; g(){ :; }; g f"
prop_checkSetESuppressed18 = verifyNotTree checkSetESuppressed "set -e; shopt -s inherit_errexit; f(){ :; }; x=$(f)"
checkSetESuppressed params t =
if hasSetE params then runNodeAnalysis checkNode params t else []
where
checkNode _ (T_SimpleCommand _ _ (cmd:_)) = when (isFunction cmd) (checkCmd cmd)
checkNode _ _ = return ()
Copy link
Copy Markdown
Owner

@koalaman koalaman Aug 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this warning should trigger for things like { a; b; } || c and $(a; b) for the same reason why it triggers for f() { a; b; }; f || c and $(f)?

It's just a thought and I don't expect it of this PR.

Copy link
Copy Markdown
Contributor Author

@DoxasticFox DoxasticFox Aug 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I ran this PR on the monorepo at work, I found that set -e was mostly suppressed in functions which were used in command substitutions and if statements, not || or && conditions. (I think that's because people don't want to branch with && and || in production code.) So, admittedly, I don't have much empirical evidence to suggest it'll be that useful.

I also think set -e's behaviour is more insidious for functions than the cases you mention, because, one might reasonably expect functions and commands to respond similarly to set -e. i.e. You wouldn't expect the behaviour of the command/function to change, you'd expect the script's response to the command/function's exit code to change.

Nonetheless, I still think the cases you mention are unintuitive enough to warrant their own warnings, because nobody could know errexit is disabled in those contexts without being told. I don't really want to raise a PR though because I haven't seen those pop up much in the wild and I'm lazy 😂


functions_ = functions t

isFunction cmd = isJust $ do
literalArg <- getUnquotedLiteral cmd
Map.lookup literalArg functions_

checkCmd cmd = go $ getPath (parentMap params) cmd
where
go (child:parent:rest) = do
case parent of
T_Banged _ condition | child `isIn` [condition] -> informConditional "a ! condition" cmd
T_AndIf _ condition _ | child `isIn` [condition] -> informConditional "an && condition" cmd
T_OrIf _ condition _ | child `isIn` [condition] -> informConditional "an || condition" cmd
T_IfExpression _ condition _ | child `isIn` concatMap fst condition -> informConditional "an 'if' condition" cmd
T_UntilExpression _ condition _ | child `isIn` condition -> informConditional "an 'until' condition" cmd
T_WhileExpression _ condition _ | child `isIn` condition -> informConditional "a 'while' condition" cmd
T_DollarExpansion {} | not $ errExitEnabled parent -> informUninherited cmd
T_Backticked {} | not $ errExitEnabled parent -> informUninherited cmd
_ -> return ()
go (parent:rest)
go _ = return ()

informConditional condType t =
info (getId t) 2310 (
"This function is invoked in " ++ condType ++ " so set -e " ++
"will be disabled. Invoke separately if failures should " ++
"cause the script to exit.")
informUninherited t =
info (getId t) 2311 (
"Bash implicitly disabled set -e for this function " ++
"invocation because it's inside a command substitution. " ++
"Add set -e; before it or enable inherit_errexit.")
errExitEnabled t = hasInheritErrexit params || containsSetE t
isIn t cmds = getId t `elem` map getId cmds


return []
runTests = $( [| $(forAllProperties) (quickCheckWithResult (stdArgs { maxSuccess = 1 }) ) |])
22 changes: 17 additions & 5 deletions src/ShellCheck/AnalyzerLib.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ composeAnalyzers f g x = f x >> g x
data Parameters = Parameters {
-- Whether this script has the 'lastpipe' option set/default.
hasLastpipe :: Bool,
-- Whether this script has the 'inherit_errexit' option set/default.
hasInheritErrexit :: Bool,
-- Whether this script has 'set -e' anywhere.
hasSetE :: Bool,
-- A linear (bad) analysis of data flow
Expand Down Expand Up @@ -196,7 +198,12 @@ makeParameters spec =
Dash -> False
Sh -> False
Ksh -> True,

hasInheritErrexit =
case shellType params of
Bash -> containsInheritErrexit root
Dash -> True
Sh -> True
Ksh -> False,
shellTypeSpecified = isJust (asShellType spec) || isJust (asFallbackShell spec),
parentMap = getParentTree root,
variableFlow = getVariableFlow params root,
Expand All @@ -219,18 +226,23 @@ containsSetE root = isNothing $ doAnalysis (guard . not . isSetE) root
_ -> False
re = mkRegex "[[:space:]]-[^-]*e"

-- Does this script mention 'shopt -s lastpipe' anywhere?
-- Also used as a hack.
containsLastpipe root =
containsShopt shopt root =
isNothing $ doAnalysis (guard . not . isShoptLastPipe) root
where
isShoptLastPipe t =
case t of
T_SimpleCommand {} ->
t `isUnqualifiedCommand` "shopt" &&
("lastpipe" `elem` oversimplify t)
(shopt `elem` oversimplify t)
_ -> False

-- Does this script mention 'shopt -s inherit_errexit' anywhere?
containsInheritErrexit = containsShopt "inherit_errexit"

-- Does this script mention 'shopt -s lastpipe' anywhere?
-- Also used as a hack.
containsLastpipe = containsShopt "lastpipe"


prop_determineShell0 = determineShellTest "#!/bin/sh" == Sh
prop_determineShell1 = determineShellTest "#!/usr/bin/env ksh" == Ksh
Expand Down