From fa15c0a454be93603418a56ed0d625e5c76a83fc Mon Sep 17 00:00:00 2001 From: Patrick Xia Date: Thu, 5 May 2022 16:09:02 -0700 Subject: [PATCH 1/2] add SC2316: error on multiple declarations like 'readonly local' --- src/ShellCheck/Checks/Commands.hs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/ShellCheck/Checks/Commands.hs b/src/ShellCheck/Checks/Commands.hs index 72d8c09..bad4cf2 100644 --- a/src/ShellCheck/Checks/Commands.hs +++ b/src/ShellCheck/Checks/Commands.hs @@ -100,6 +100,7 @@ commandChecks = [ ] ++ map checkArgComparison ("alias" : declaringCommands) ++ map checkMaskedReturns declaringCommands + ++ map checkMultipleDeclaring declaringCommands optionalChecks = map fst optionalCommandChecks @@ -941,6 +942,21 @@ checkLocalScope = CommandCheck (Exactly "local") $ \t -> unless (any isFunctionLike path) $ err (getId $ getCommandTokenOrThis t) 2168 "'local' is only valid in functions." +prop_checkMultipleDeclaring1 = verify (checkMultipleDeclaring "local") "q() { local readonly var=1; }" +prop_checkMultipleDeclaring2 = verifyNot (checkMultipleDeclaring "local") "q() { local var=1; }" +prop_checkMultipleDeclaring3 = verify (checkMultipleDeclaring "readonly") "readonly local foo=5" +prop_checkMultipleDeclaring4 = verify (checkMultipleDeclaring "export") "export readonly foo=5" +prop_checkMultipleDeclaring5 = verifyNot (checkMultipleDeclaring "local") "f() { local -r foo=5; }" +prop_checkMultipleDeclaring6 = verifyNot (checkMultipleDeclaring "declare") "declare -rx foo=5" +checkMultipleDeclaring cmd = CommandCheck (Exactly cmd) (mapM_ check . arguments) + where + check t = sequence_ $ do + lit <- getLiteralString t + guard $ lit `elem` declaringCommands + return $ err (getId $ getCommandTokenOrThis t) 2316 $ + "This applies " ++ cmd ++ " to the variable named " ++ lit ++ + ", which is probably not what you want. Use a separate command or the appropriate `declare` options instead." + prop_checkDeprecatedTempfile1 = verify checkDeprecatedTempfile "var=$(tempfile)" prop_checkDeprecatedTempfile2 = verifyNot checkDeprecatedTempfile "tempfile=$(mktemp)" checkDeprecatedTempfile = CommandCheck (Basename "tempfile") $ From 282155268829ffe450a21eb728ba0417c9c2578d Mon Sep 17 00:00:00 2001 From: Rune Juhl Jacobsen Date: Tue, 14 Dec 2021 16:00:47 +0100 Subject: [PATCH 2/2] Fix bug in 2126 when using after/before flags with grep Using `--after-context`/`-A` or `--before-context`/`-B` would give a warning recommending the user to use `grep -c`, even though that would give a different result than using `grep | wc -l`: ```fundamental $ echo -e "1\n2\n3" | grep -cA 3 1 1 $ echo -e "1\n2\n3" | grep -A 3 1 | wc -l 3 ``` --- src/ShellCheck/Analytics.hs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index d48104e..e6e47ea 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -545,6 +545,10 @@ prop_checkPipePitfalls15 = verifyNot checkPipePitfalls "foo | grep bar | wc -cmw prop_checkPipePitfalls16 = verifyNot checkPipePitfalls "foo | grep -r bar | wc -l" prop_checkPipePitfalls17 = verifyNot checkPipePitfalls "foo | grep -l bar | wc -l" prop_checkPipePitfalls18 = verifyNot checkPipePitfalls "foo | grep -L bar | wc -l" +prop_checkPipePitfalls19 = verifyNot checkPipePitfalls "foo | grep -A2 bar | wc -l" +prop_checkPipePitfalls20 = verifyNot checkPipePitfalls "foo | grep -B999 bar | wc -l" +prop_checkPipePitfalls21 = verifyNot checkPipePitfalls "foo | grep --after-context 999 bar | wc -l" +prop_checkPipePitfalls22 = verifyNot checkPipePitfalls "foo | grep -B 1 --after-context 999 bar | wc -l" checkPipePitfalls _ (T_Pipeline id _ commands) = do for ["find", "xargs"] $ \(find:xargs:_) -> @@ -566,10 +570,10 @@ checkPipePitfalls _ (T_Pipeline id _ commands) = do let flagsGrep = maybe [] (map snd . getAllFlags) $ getCommand grep flagsWc = maybe [] (map snd . getAllFlags) $ getCommand wc in - unless (any (`elem` ["l", "files-with-matches", "L", "files-without-matches", "o", "only-matching", "r", "R", "recursive"]) flagsGrep + unless (any (`elem` ["l", "files-with-matches", "L", "files-without-matches", "o", "only-matching", "r", "R", "recursive", "A", "after-context", "B", "before-context"]) flagsGrep || any (`elem` ["m", "chars", "w", "words", "c", "bytes", "L", "max-line-length"]) flagsWc || null flagsWc) $ - style (getId grep) 2126 "Consider using grep -c instead of grep|wc -l." + style (getId grep) 2126 "Consider using 'grep -c' instead of 'grep|wc -l'." didLs <- fmap or . sequence $ [ for' ["ls", "grep"] $