From 95a8cf93c90b5b8a215cd20c4d79ea1032b86acc Mon Sep 17 00:00:00 2001 From: Ng Zhi An Date: Sat, 22 Dec 2018 21:59:38 +0800 Subject: [PATCH 1/2] Add check for ambiguous nullary test Given an input like `if [[ $(a) ]]; then ...`, this is a implicit `-n` test, so it works like `if [[ -n $(a) ]]; then ...`. Users might confuse this for a check for the exit code of the command a, which should be tested with: if a; then ... We warn the user to be more explicity and specifity the `-n`. Fixes #1416 --- src/ShellCheck/Analytics.hs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index 9b407f8..5b8d1bd 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -170,6 +170,7 @@ nodeChecks = [ ,checkSubshelledTests ,checkInvertedStringTest ,checkRedirectionToCommand + ,checkNullaryExpansionTest ] @@ -3096,5 +3097,21 @@ checkRedirectionToCommand _ t = warn id 2238 "Redirecting to/from command name instead of file. Did you want pipes/xargs (or quote to ignore)?" _ -> return () +prop_checkNullaryExpansionTest1 = verify checkNullaryExpansionTest "[[ $(a) ]]" +prop_checkNullaryExpansionTest2 = verify checkNullaryExpansionTest "[[ $a ]]" +prop_checkNullaryExpansionTest3 = verifyNot checkNullaryExpansionTest "[[ $a=1 ]]" +prop_checkNullaryExpansionTest4 = verifyNot checkNullaryExpansionTest "[[ -n $(a) ]]" +checkNullaryExpansionTest _ t = + case t of + TC_Nullary _ _ (T_NormalWord id [T_DollarExpansion _ [T_Pipeline _ [] [x]]]) -> + when (isJust (getCommand x)) $ + style id 2243 ( + "To check for the exit code of a command, remove the conditional expression, e.g. if foo; ...") + TC_Nullary _ _ (T_NormalWord id [t2]) -> + when ((not . isConstant) t2) $ + style id 2244 ( + "Use -n to check for null string, e.g. [[ -n $var ]].") + _ -> return () + return [] runTests = $( [| $(forAllProperties) (quickCheckWithResult (stdArgs { maxSuccess = 1 }) ) |]) From 73822c35887c1d54f50ebafa1a9de7425228ffab Mon Sep 17 00:00:00 2001 From: Vidar Holen Date: Fri, 28 Dec 2018 19:01:31 -0800 Subject: [PATCH 2/2] Allow SC2243 and SC2244 to trigger with quotes, add fix --- CHANGELOG.md | 1 + src/ShellCheck/ASTLib.hs | 5 ++++- src/ShellCheck/Analytics.hs | 24 +++++++++++++++--------- src/ShellCheck/AnalyzerLib.hs | 13 ++++++++----- 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c239e7..11e1994 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## Since previous release ### Added - Preliminary support for fix suggestions +- SC2243/SC2244: Suggest using explicit -n for `[ $foo ]` ## v0.6.0 - 2018-12-02 ### Added diff --git a/src/ShellCheck/ASTLib.hs b/src/ShellCheck/ASTLib.hs index ef72b09..5f3b68c 100644 --- a/src/ShellCheck/ASTLib.hs +++ b/src/ShellCheck/ASTLib.hs @@ -485,8 +485,11 @@ wordsCanBeEqual x y = fromMaybe True $ -- Is this an expansion that can be quoted, -- e.g. $(foo) `foo` $foo (but not {foo,})? isQuoteableExpansion t = case t of + T_DollarBraced {} -> True + _ -> isCommandSubstitution t + +isCommandSubstitution t = case t of T_DollarExpansion {} -> True T_DollarBraceCommandExpansion {} -> True T_Backticked {} -> True - T_DollarBraced {} -> True _ -> False diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index 5b8d1bd..21f429f 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -3101,16 +3101,22 @@ prop_checkNullaryExpansionTest1 = verify checkNullaryExpansionTest "[[ $(a) ]]" prop_checkNullaryExpansionTest2 = verify checkNullaryExpansionTest "[[ $a ]]" prop_checkNullaryExpansionTest3 = verifyNot checkNullaryExpansionTest "[[ $a=1 ]]" prop_checkNullaryExpansionTest4 = verifyNot checkNullaryExpansionTest "[[ -n $(a) ]]" -checkNullaryExpansionTest _ t = +prop_checkNullaryExpansionTest5 = verify checkNullaryExpansionTest "[[ \"$a$b\" ]]" +prop_checkNullaryExpansionTest6 = verify checkNullaryExpansionTest "[[ `x` ]]" +checkNullaryExpansionTest params t = case t of - TC_Nullary _ _ (T_NormalWord id [T_DollarExpansion _ [T_Pipeline _ [] [x]]]) -> - when (isJust (getCommand x)) $ - style id 2243 ( - "To check for the exit code of a command, remove the conditional expression, e.g. if foo; ...") - TC_Nullary _ _ (T_NormalWord id [t2]) -> - when ((not . isConstant) t2) $ - style id 2244 ( - "Use -n to check for null string, e.g. [[ -n $var ]].") + TC_Nullary _ _ word -> + case getWordParts word of + [t] | isCommandSubstitution t -> + styleWithFix id 2243 "Prefer explicit -n to check for output (or run command without [/[[ to check for success)." fix + + -- If they're constant, you get SC2157 &co + x | all (not . isConstant) x -> + styleWithFix id 2244 "Prefer explicit -n to check non-empty string (or use =/-ne to check boolean/integer)." fix + _ -> return () + where + id = getId word + fix = fixWith [replaceStart id params 0 "-n "] _ -> return () return [] diff --git a/src/ShellCheck/AnalyzerLib.hs b/src/ShellCheck/AnalyzerLib.hs index 306c139..90a79e0 100644 --- a/src/ShellCheck/AnalyzerLib.hs +++ b/src/ShellCheck/AnalyzerLib.hs @@ -155,11 +155,14 @@ err id code str = addComment $ makeComment ErrorC id code str info id code str = addComment $ makeComment InfoC id code str style id code str = addComment $ makeComment StyleC id code str -warnWithFix id code str fix = addComment $ - let comment = makeComment WarningC id code str in - comment { - tcFix = Just fix - } +warnWithFix :: MonadWriter [TokenComment] m => Id -> Code -> String -> Fix -> m () +warnWithFix = addCommentWithFix WarningC +styleWithFix :: MonadWriter [TokenComment] m => Id -> Code -> String -> Fix -> m () +styleWithFix = addCommentWithFix StyleC + +addCommentWithFix :: MonadWriter [TokenComment] m => Severity -> Id -> Code -> String -> Fix -> m () +addCommentWithFix severity id code str fix = + addComment $ makeCommentWithFix severity id code str fix makeCommentWithFix :: Severity -> Id -> Code -> String -> Fix -> TokenComment makeCommentWithFix severity id code str fix =